Repository navigation
fix(aks-diagnostic): move SAS token off argv to an environment variable - #9883
Draft
Calvin S. (calvin197) wants to merge 2 commits into
Draft
Calvin S. (calvin197) wants to merge 2 commits into
Calvin S. (calvin197) wants to merge 2 commits into
Conversation
The SAS token used to authenticate blob uploads was a required positional
CLI argument, exposing it to any local process able to read this
process's command line (e.g. via /proc/<pid>/cmdline), rather than only
to whoever invoked the script.
Add support for passing it via the AKS_DIAGNOSTIC_SAS_TOKEN environment
variable (preferred, takes precedence when both are set), which does not
appear in argv. The positional CLI argument is kept as a deprecated
fallback for backward compatibility with any existing external callers -
this script is baked onto the VHD standalone (no in-repo caller invokes
it; vhdbuilder/packer/packer_source.sh only copies the file), so there is
no way to enumerate or update every caller in this change. Using it now
prints a warning to stderr pointing at the env var instead.
To make the new optional sas_token positional unambiguous against the
pre-existing optional file_path positional, file_path is now the
--file-path flag rather than a second positional argument (two optional
trailing positionals cannot otherwise be told apart by argparse).
Also removed the unused handleArgs() function: dead code (zero call
sites, confirmed via repo-wide search) that carried the exact same
argv-based SAS token exposure as a legacy positional-args fallback.
No change to the actual upload behavior/HTTP logic.
Verified locally:
- python3 -m py_compile and pyflakes both pass with no warnings/errors.
- Manually exercised all argument-resolution paths: missing token (both
env var and positional absent) now errors with a clear message;
env-var-only invocation succeeds and the token does not appear in the
process's argv (checked via ps/cmdline); positional-only invocation
still works and emits the deprecation warning; when both are set, the
env var wins and no warning is printed. --help output reviewed for
correctness.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 350c2577-7031-4d0d-adc1-e99d053fc981
Contributor
Windows Unit Test Results 3 files 18 suites 1m 16s ⏱️ Results for commit e8a35c0. ♻️ This comment has been updated with latest results. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Previously supported invocations with a positional custom file path now fail argument parsing, breaking compatibility for external callers.
1 open issue
What changed in this PR
This PR adds an environment-variable option for diagnostic upload credentials, reducing command-line exposure of SAS tokens in the standalone VHD script.
Changes:
- Prefer
AKS_DIAGNOSTIC_SAS_TOKEN, retaining a deprecated positional token fallback. - Replace the positional custom file path with
--file-pathand update usage documentation. - Remove the unused
handleArgs()function without changing upload HTTP logic.
| File | Description |
|---|---|
parts/linux/cloud-init/artifacts/aks-diagnostic.py |
Updates credential resolution, argument parsing, warnings, and usage examples. |
Review effort: Balanced
The previous commit converted file_path from a positional argument to a --file-path flag to resolve an argparse ambiguity with the new optional sas_token positional. That broke the pre-existing 4-argument invocation style (container url sas_token file_path), which is a real regression for any external caller still using it. Fix properly: capture the trailing sas_token/file_path positionals together via nargs='*' and resolve them by count after parsing, so every pre-existing invocation shape - (), (token,), (token, file_path) - behaves exactly as before. --file-path remains available as the preferred way to set a custom path when the token itself comes from the AKS_DIAGNOSTIC_SAS_TOKEN env var (no token positional at all). Verified locally: re-ran all argument-resolution paths, specifically including the previously-broken old 4-positional-argument form (container url token file_path) with a real file on disk, confirming it resolves file_path correctly and reaches the upload call. Also re-verified the new env-var + --file-path combination, the "too many arguments" error path, and that the SAS token still does not appear in the process's argv when sourced from the environment variable. python3 -m py_compile and pyflakes both pass with no warnings. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 350c2577-7031-4d0d-adc1-e99d053fc981
Contributor
|
Calvin S. (calvin197)
marked this pull request as draft
October 23, 2026 08:04
Contributor
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
The SAS token used to authenticate blob uploads in
aks-diagnostic.pywas a required positional CLI argument, exposing it to any local process able to read this process's command line (e.g. via/proc/<pid>/cmdline), rather than only to whoever invoked the script (ADO finding 37518923).Add support for passing it via the
AKS_DIAGNOSTIC_SAS_TOKENenvironment variable (preferred, takes precedence when both are set), which does not appear in argv. The positional CLI argument is kept as a deprecated fallback for backward compatibility with any existing external callers — this script is baked onto the VHD standalone (no in-repo caller invokes it;vhdbuilder/packer/packer_source.shonly copies the file), so there is no way to enumerate or update every caller in this change. Using it now prints a warning to stderr pointing at the env var instead.To make the new optional
sas_tokenpositional unambiguous against the pre-existing optionalfile_pathpositional,file_pathis now the--file-pathflag rather than a second positional argument (two optional trailing positionals cannot otherwise be told apart by argparse).Also removed the unused
handleArgs()function: dead code (zero call sites, confirmed via repo-wide search) that carried the exact same argv-based SAS token exposure as a legacy positional-args fallback.No change to the actual upload behavior/HTTP logic. Validated:
python3 -m py_compileandpyflakesboth pass with no warnings/errors. Manually exercised all argument-resolution paths — missing token (both env var and positional absent) now errors with a clear message; env-var-only invocation succeeds and the token does not appear in the process's argv (checked viaps/cmdline); positional-only invocation still works and emits the deprecation warning; when both are set, the env var wins and no warning is printed.--helpoutput reviewed for correctness.Which issue(s) this PR fixes:
Fixes #
ADO work items (msazure/CloudNativeCompute): 37518923 (parent: 37518878)