From e9229140c3c9a9afdcd3daa75c798e94bb750dc6 Mon Sep 17 00:00:00 2001 From: "Calvin S." Date: Fri, 23 Oct 2026 00:08:12 -0700 Subject: [PATCH 1/2] fix(aks-diagnostic): move SAS token off argv to an environment variable 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//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 --- .../cloud-init/artifacts/aks-diagnostic.py | 76 +++++++++++++------ 1 file changed, 52 insertions(+), 24 deletions(-) diff --git a/parts/linux/cloud-init/artifacts/aks-diagnostic.py b/parts/linux/cloud-init/artifacts/aks-diagnostic.py index 4199c6f6dd1..5b4ea83854a 100644 --- a/parts/linux/cloud-init/artifacts/aks-diagnostic.py +++ b/parts/linux/cloud-init/artifacts/aks-diagnostic.py @@ -5,13 +5,20 @@ This script uploads a diagnostic log file to an Azure Blob Storage container using a REST API call. Usage: - python aks-diagnostic.py [file_path] + AKS_DIAGNOSTIC_SAS_TOKEN= python aks-diagnostic.py [--file-path ] + + # Positional sas_token is still accepted for backward compatibility, but is deprecated: + # any local process able to read this process's command line (e.g. via /proc//cmdline) + # can see the token. Prefer the AKS_DIAGNOSTIC_SAS_TOKEN environment variable instead. + python aks-diagnostic.py [sas_token] [--file-path ] Arguments: container : Name of the blob container. storage_account_url : Full URL of the Azure storage account (e.g., https://.blob.core.windows.net/). - sas_token : Shared Access Signature token for authentication. - file_path (optional) : Path to the log file to upload (default: /var/lib/waagent/logcollector/logs.zip). + sas_token (deprecated) : Shared Access Signature token for authentication, as a positional argument. + Prefer the AKS_DIAGNOSTIC_SAS_TOKEN environment variable, which does not + appear in the process's command line. + --file-path (optional) : Path to the log file to upload (default: /var/lib/waagent/logcollector/logs.zip). """ import requests import os @@ -26,6 +33,10 @@ RETRY_DELAY = 2 # seconds MAX_FILE_SIZE = 1024 * 1024 * 1024 # 1000MB DEFAULT_LOG_PATH = "/var/lib/waagent/logcollector/logs.zip" +# Preferred way to pass the SAS token: environment variables are not visible in +# /proc//cmdline the way CLI arguments are, so this avoids exposing the secret +# to any other local process that can list or read process command lines. +SAS_TOKEN_ENV_VAR = "AKS_DIAGNOSTIC_SAS_TOKEN" def parse_arguments(): """ @@ -38,9 +49,14 @@ def parse_arguments(): description='Upload diagnostic log files to Azure Blob Storage', formatter_class=argparse.RawDescriptionHelpFormatter, epilog=""" - Examples: + Examples (preferred - SAS token via environment variable): + AKS_DIAGNOSTIC_SAS_TOKEN="sv=2020-08-04&ss=b&srt=co&sp=rwdlac&se=2023-01-01T00:00:00Z&st=2022-01-01T00:00:00Z&spr=https&sig=..." \\ + python3 aks-diagnostic.py mycontainer https://mystorageaccount.blob.core.windows.net + AKS_DIAGNOSTIC_SAS_TOKEN="sv=2020-08-04&ss=b&srt=co&sp=rwdlac&se=2023-01-01T00:00:00Z&st=2022-01-01T00:00:00Z&spr=https&sig=..." \\ + python3 aks-diagnostic.py mycontainer https://mystorageaccount.blob.core.windows.net --file-path /custom/path/logs.zip + + Examples (deprecated - SAS token as CLI argument, visible via /proc//cmdline): python3 aks-diagnostic.py mycontainer https://mystorageaccount.blob.core.windows.net "sv=2020-08-04&ss=b&srt=co&sp=rwdlac&se=2023-01-01T00:00:00Z&st=2022-01-01T00:00:00Z&spr=https&sig=..." - python3 aks-diagnostic.py mycontainer https://mystorageaccount.blob.core.windows.net "sv=2020-08-04&ss=b&srt=co&sp=rwdlac&se=2023-01-01T00:00:00Z&st=2022-01-01T00:00:00Z&spr=https&sig=..." /custom/path/logs.zip """ ) @@ -54,11 +70,17 @@ def parse_arguments(): ) parser.add_argument( 'sas_token', - help='Shared Access Signature token for authentication' + nargs='?', + default=None, + help=( + 'Shared Access Signature token for authentication. Deprecated: visible to other ' + f'local processes via /proc//cmdline. Prefer the {SAS_TOKEN_ENV_VAR} ' + 'environment variable instead, which takes precedence if both are set.' + ) ) parser.add_argument( - 'file_path', - nargs='?', + '--file-path', + dest='file_path', default=DEFAULT_LOG_PATH, help=f'Path to the log file to upload (default: {DEFAULT_LOG_PATH})' ) @@ -114,26 +136,32 @@ def validate_arguments(args): if not args.container or not args.container.replace('-', '').replace('.', '').isalnum(): print(f"Error: Invalid container name: {args.container}") sys.exit(1) - + + # Resolve the SAS token. The environment variable is preferred since it does not appear + # in /proc//cmdline the way a CLI argument does; it takes precedence if both are set. + # The positional CLI argument is kept only for backward compatibility with existing callers. + sas_token = os.environ.get(SAS_TOKEN_ENV_VAR) or args.sas_token + if not sas_token: + print( + f"Error: No SAS token provided. Set the {SAS_TOKEN_ENV_VAR} environment variable " + "(preferred), or pass it as a positional argument (deprecated, visible to other " + "local processes via /proc//cmdline)." + ) + sys.exit(1) + if args.sas_token and not os.environ.get(SAS_TOKEN_ENV_VAR): + print( + f"Warning: SAS token was passed as a CLI argument; it is visible to other local " + f"processes via /proc//cmdline. Prefer the {SAS_TOKEN_ENV_VAR} environment " + "variable instead.", + file=sys.stderr + ) + # Validate SAS token (basic check) - if not args.sas_token or 'sig=' not in args.sas_token: + if 'sig=' not in sas_token: print("Error: Invalid SAS token format") sys.exit(1) - - return args.container, args.storage_account_url, args.sas_token, args.file_path - - -def handleArgs(args): - if len(args) < 3: - print("Usage: python aks-diagnostic.py [file_path]") - return None, None, None, None - - container = args[0] - storage_account_url = args[1] - sas_token = args[2] - file_path = args[3] if len(args) > 3 else DEFAULT_LOG_PATH - return container, storage_account_url, sas_token, file_path + return args.container, args.storage_account_url, sas_token, args.file_path def redact_sas_token(url): From e8a35c0d09c50f8e57befff3e245ba6cd50d523f Mon Sep 17 00:00:00 2001 From: "Calvin S." Date: Fri, 23 Oct 2026 00:59:14 -0700 Subject: [PATCH 2/2] fix(aks-diagnostic): restore backward compat for positional file_path 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 --- .../cloud-init/artifacts/aks-diagnostic.py | 39 +++++++++++++------ 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/parts/linux/cloud-init/artifacts/aks-diagnostic.py b/parts/linux/cloud-init/artifacts/aks-diagnostic.py index 5b4ea83854a..bfb4ac6091c 100644 --- a/parts/linux/cloud-init/artifacts/aks-diagnostic.py +++ b/parts/linux/cloud-init/artifacts/aks-diagnostic.py @@ -68,24 +68,41 @@ def parse_arguments(): 'storage_account_url', help='Full URL of the Azure storage account (e.g., https://.blob.core.windows.net/)' ) + # sas_token and file_path are both trailing optional positionals in the original CLI + # (sas_token was required, file_path was not), which argparse cannot express directly + # once sas_token also becomes optional (two consecutive nargs='?' positionals are + # ambiguous). Instead, capture them together via nargs='*' and resolve by count below, + # so every pre-existing invocation shape - (), (token,), (token, file_path) - keeps + # behaving exactly as before. parser.add_argument( - 'sas_token', - nargs='?', - default=None, + 'positional', + nargs='*', + metavar='[sas_token] [file_path]', help=( - 'Shared Access Signature token for authentication. Deprecated: visible to other ' - f'local processes via /proc//cmdline. Prefer the {SAS_TOKEN_ENV_VAR} ' - 'environment variable instead, which takes precedence if both are set.' + 'Deprecated positional form: [sas_token] [file_path]. sas_token is visible to ' + f'other local processes via /proc//cmdline - prefer the {SAS_TOKEN_ENV_VAR} ' + 'environment variable instead (takes precedence if both are set). file_path ' + f'defaults to {DEFAULT_LOG_PATH} if omitted.' ) ) parser.add_argument( '--file-path', - dest='file_path', - default=DEFAULT_LOG_PATH, - help=f'Path to the log file to upload (default: {DEFAULT_LOG_PATH})' + dest='file_path_flag', + default=None, + help=( + 'Path to the log file to upload (default: ' + f'{DEFAULT_LOG_PATH}). Preferred over the positional file_path form.' + ) ) - - return parser.parse_args() + + parsed = parser.parse_args() + if len(parsed.positional) > 2: + parser.error(f"unrecognized arguments: {' '.join(parsed.positional[2:])}") + + parsed.sas_token = parsed.positional[0] if len(parsed.positional) >= 1 else None + positional_file_path = parsed.positional[1] if len(parsed.positional) >= 2 else None + parsed.file_path = parsed.file_path_flag or positional_file_path or DEFAULT_LOG_PATH + return parsed def validate_arguments(args):