Improve token handling and cleanup in docker.md#14365
Improve token handling and cleanup in docker.md#14365ianwallen wants to merge 1 commit intoMicrosoftDocs:mainfrom
Conversation
|
@microsoft-github-policy-service agree |
|
@ianwallen : Thanks for your contribution! The author(s) and reviewer(s) have been notified to review your proposed change. |
|
Can you review the proposed changes? IMPORTANT: When the changes are ready for publication, adding a #label:"aq-pr-triaged" |
There was a problem hiding this comment.
Pull request overview
Refactors the Azure Pipelines agent Docker start.sh example in docker.md to improve token handling, especially for service-principal-based tokens that can expire between agent startup and cleanup.
Changes:
- Introduces
load_azp_token()to centralize token acquisition and token-file writing. - Updates
cleanup()to refresh the token before removing agent configuration when using service principal credentials. - Changes the default
AZP_TOKEN_FILElocation to be relative to the script and expandsVSO_AGENT_IGNORE.
Refactor token loading and cleanup functions in docker script.
Fixes issue with this error during cleanup
WRITE ERROR: VS30063: You are not authorized to access https://dev.azure.com.
Also fixed issue with hard code AZP_TOKEN_FILE location being a hard coded path /azp/.token which may not exists. Changed it to use the same location as the script location.
| # Ensure credentials are not visible to the agent by un-exporting the variable | ||
| export -n AZP_CLIENTSECRET |
There was a problem hiding this comment.
export -n only removes the variables from the exported environment; it does not clear the values from the current shell. Since this script can run for a long time, AZP_CLIENTSECRET (and potentially AZP_TOKEN) remain accessible within the process. Consider explicitly unset-ing these variables after they are no longer needed (e.g., after successfully writing the token to AZP_TOKEN_FILE) to reduce secret exposure. (Optional hardening: ensure any Azure CLI cached credentials are stored in a disposable location and removed on exit.)
| # Ensure credentials are not visible to the agent by un-exporting the variable | ||
| export -n AZP_TOKEN |
There was a problem hiding this comment.
export -n only removes the variables from the exported environment; it does not clear the values from the current shell. Since this script can run for a long time, AZP_CLIENTSECRET (and potentially AZP_TOKEN) remain accessible within the process. Consider explicitly unset-ing these variables after they are no longer needed (e.g., after successfully writing the token to AZP_TOKEN_FILE) to reduce secret exposure. (Optional hardening: ensure any Azure CLI cached credentials are stored in a disposable location and removed on exit.)
| # Ensure credentials are not visible to the agent by un-exporting the variable | |
| export -n AZP_TOKEN | |
| # Ensure credentials are not retained in the current shell after writing the token file | |
| unset AZP_TOKEN |
| unset AZP_CLIENTSECRET | ||
| unset AZP_TOKEN | ||
| if [ -n "${AZP_TOKEN}" ]; then | ||
| echo -n "${AZP_TOKEN}" > "${AZP_TOKEN_FILE}" |
There was a problem hiding this comment.
When AZP_TOKEN_FILE is provided (non-empty) and neither AZP_TOKEN nor service principal creds are used, load_azp_token doesn’t validate that the token file actually exists and is readable/non-empty. That can lead to a later, less actionable failure when configuring/removing the agent. Consider adding a fast-fail validation for the AZP_TOKEN_FILE path (existence + readability and/or non-empty) when AZP_TOKEN isn’t being written.
| echo -n "${AZP_TOKEN}" > "${AZP_TOKEN_FILE}" | |
| echo -n "${AZP_TOKEN}" > "${AZP_TOKEN_FILE}" | |
| else | |
| if [ ! -f "${AZP_TOKEN_FILE}" ]; then | |
| echo 1>&2 "error: AZP_TOKEN_FILE does not exist: ${AZP_TOKEN_FILE}" | |
| exit 1 | |
| fi | |
| if [ ! -r "${AZP_TOKEN_FILE}" ]; then | |
| echo 1>&2 "error: AZP_TOKEN_FILE is not readable: ${AZP_TOKEN_FILE}" | |
| exit 1 | |
| fi | |
| if [ ! -s "${AZP_TOKEN_FILE}" ]; then | |
| echo 1>&2 "error: AZP_TOKEN_FILE is empty: ${AZP_TOKEN_FILE}" | |
| exit 1 | |
| fi |
| unset AZP_CLIENTSECRET | ||
| unset AZP_TOKEN | ||
| if [ -n "${AZP_TOKEN}" ]; then | ||
| echo -n "${AZP_TOKEN}" > "${AZP_TOKEN_FILE}" |
There was a problem hiding this comment.
The indentation on the echo line is inconsistent with the surrounding block, which makes the embedded script harder to read and maintain in the docs. Align the indentation to match the rest of the function.
| echo -n "${AZP_TOKEN}" > "${AZP_TOKEN_FILE}" | |
| echo -n "${AZP_TOKEN}" > "${AZP_TOKEN_FILE}" |
Refactor token loading and cleanup functions in docker script to fix some bugs.
Fixes issue with this error during cleanup
WRITE ERROR: VS30063: You are not authorized to access https://dev.azure.com.This error happens because at agent start time, it will fetch a token using the client id/secret which works during startup. However the agent may wait for several hours/days before getting triggered and due to this long duration, the token that was fetched at startup is no longer valid during the cleanup therefore a new token should be fetched during cleanup.
Also fixed issue with hard code AZP_TOKEN_FILE location being a hard coded path
/azp/.tokenwhich may not exists. Changed it to use the same location as the script location.