Skip to content

Commit b404d83

Browse files
{Profile} az login: Add HTTP timeout and fix callback arg parsing
Code-review follow-ups on the OIDC token-refresh feature: - Add an explicit connect/read timeout to the GitHub Actions and Azure DevOps OIDC token requests via a shared _oidc_http_request helper. Without it, a stalled connection would hang token acquisition (and, for --federated-identity, every concurrent refresh thread holding the lock) indefinitely on a long-running CI job. Timeout and connection failures now raise a clear CLIError. - Parse the --federated-token-callback command with POSIX rules on every platform. The previous posix=False path on Windows left literal quote characters in argv, so a quoted argument containing spaces (e.g. an audience) reached the program corrupted. POSIX parsing handles quoting correctly and consistently; the Windows backslash caveat is documented in the help text and docstring. Adds tests for the request timeout path and quoted-argument parsing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 01bd46f commit b404d83

3 files changed

Lines changed: 56 additions & 11 deletions

File tree

‎src/azure-cli-core/azure/cli/core/auth/identity.py‎

Lines changed: 30 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -485,6 +485,10 @@ def get_environment_credential():
485485
# MSAL may invoke a client_assertion callback concurrently; this lock keeps the dispatcher fetch thread-safe.
486486
_federated_id_token_lock = threading.Lock()
487487

488+
# Connect/read timeouts (seconds) for OIDC ID token requests, so a network stall fails fast on a
489+
# long-running CI job instead of hanging the whole token acquisition indefinitely.
490+
_OIDC_HTTP_TIMEOUT = (10, 30)
491+
488492

489493
def get_federated_id_token():
490494
"""Acquire a fresh OIDC ID token from the current CI/CD provider.
@@ -508,6 +512,21 @@ def get_federated_id_token():
508512
"--federated-token instead. See https://github.com/Azure/azure-cli/issues/28708 for details.")
509513

510514

515+
def _oidc_http_request(method, url, headers, provider):
516+
"""Issue an OIDC ID token HTTP request with an explicit timeout.
517+
518+
A missing timeout would let a stalled connection hang token acquisition forever on a long-running CI
519+
job, so we bound it and surface a clear error on timeout or connection failure.
520+
"""
521+
import requests
522+
try:
523+
return method(url, headers=headers, timeout=_OIDC_HTTP_TIMEOUT)
524+
except requests.exceptions.Timeout:
525+
raise CLIError('Timed out requesting an ID token from {}. Please retry.'.format(provider))
526+
except requests.exceptions.RequestException as ex:
527+
raise CLIError('Failed to reach the {} OIDC endpoint: {}'.format(provider, ex))
528+
529+
511530
def _get_id_token_github():
512531
"""Fetch a fresh OIDC ID token from the GitHub Actions token service.
513532
@@ -534,7 +553,7 @@ def _get_id_token_github():
534553
'Accept': 'application/json; api-version=2.0',
535554
'Content-Type': 'application/json'
536555
}
537-
response = requests.get(url, headers=headers)
556+
response = _oidc_http_request(requests.get, url, headers, 'GitHub Actions')
538557
if not response.ok:
539558
raise CLIError('Failed to retrieve an ID token from GitHub Actions: {} {}'.format(
540559
response.status_code, response.reason))
@@ -586,7 +605,7 @@ def _get_id_token_azure_devops():
586605
# Prevents the service from responding with a redirect HTTP status code.
587606
'X-TFS-FedAuthRedirect': 'Suppress',
588607
}
589-
response = requests.post(url, headers=headers)
608+
response = _oidc_http_request(requests.post, url, headers, 'Azure DevOps')
590609
if not response.ok:
591610
raise CLIError('Failed to retrieve an ID token from Azure DevOps: {} {}'.format(
592611
response.status_code, response.reason))
@@ -605,15 +624,18 @@ def _build_command_assertion_callback(command):
605624
is expected to print a single OIDC token to stdout; MSAL invokes the returned callable whenever it
606625
needs a fresh assertion, so refresh works with any CI/CD provider.
607626
608-
The command is parsed into an argument vector and run WITHOUT a shell (shell=False). Because the command
609-
is persisted in the service principal entry and re-executed on every refresh, avoiding a shell prevents a
610-
tampered token cache from turning into arbitrary code execution. Users who need shell features such as
611-
pipes or redirection should either point to a script file or wrap the pipeline explicitly, e.g.
627+
The command is parsed into an argument vector (with POSIX rules on every platform) and run WITHOUT a
628+
shell (shell=False). Because the command is persisted in the service principal entry and re-executed on
629+
every refresh, avoiding a shell prevents a tampered token cache from turning into arbitrary code
630+
execution. Users who need shell features such as pipes or redirection should either point to a script
631+
file or wrap the pipeline explicitly, e.g.
612632
--federated-token-callback "bash -c 'curl ... | jq -r .value'".
633+
On Windows, use forward slashes in paths or escape backslashes (POSIX parsing treats '\\' as an escape).
613634
"""
614635
import shlex
615-
# posix=False keeps Windows paths (backslashes) intact; args are still executed without a shell.
616-
args = shlex.split(command, posix=not sys.platform.startswith('win'))
636+
# POSIX parsing on every platform so quoted arguments containing spaces are handled correctly and
637+
# consistently. The trade-off is that backslashes are escape characters (see the Windows note above).
638+
args = shlex.split(command, posix=True)
617639
if not args:
618640
raise CLIError('--federated-token-callback: the command is empty.')
619641

‎src/azure-cli-core/azure/cli/core/auth/tests/test_identity.py‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -324,6 +324,15 @@ def test_federated_token_callback_no_shell_interpretation(self):
324324
sp_auth.get_msal_client_credential()['client_assertion']()
325325
assert run_mock.call_args.args[0] == ['get-token', ';', 'rm', '-rf', '/']
326326

327+
def test_federated_token_callback_quoted_argument(self):
328+
# A quoted argument containing spaces must reach the program without the surrounding quotes.
329+
sp_auth = ServicePrincipalAuth.build_from_credential(
330+
'tenant1', 'sp_id1', {'client_assertion_callback': 'mytool --aud "api://x y"'})
331+
with mock.patch('subprocess.run') as run_mock:
332+
run_mock.return_value = mock.MagicMock(stdout='tok\n', stderr='')
333+
sp_auth.get_msal_client_credential()['client_assertion']()
334+
assert run_mock.call_args.args[0] == ['mytool', '--aud', 'api://x y']
335+
327336
@mock.patch('subprocess.run')
328337
def test_federated_token_callback_empty_output(self, run_mock):
329338
run_mock.return_value = mock.MagicMock(stdout=' \n', stderr='')
@@ -415,6 +424,19 @@ def test_get_federated_id_token_github(self, get_mock):
415424
assert called_url.startswith('https://github.example/token?foo=bar&audience=')
416425
assert 'api%3A//AzureADTokenExchange' in called_url
417426
assert get_mock.call_args.kwargs['headers']['Authorization'] == 'bearer request_token'
427+
# A timeout is always passed so a network stall fails fast instead of hanging the CI job.
428+
assert get_mock.call_args.kwargs['timeout'] is not None
429+
430+
@mock.patch.dict(os.environ, {
431+
'ACTIONS_ID_TOKEN_REQUEST_URL': 'https://github.example/token',
432+
'ACTIONS_ID_TOKEN_REQUEST_TOKEN': 'request_token'
433+
}, clear=True)
434+
@mock.patch('requests.get')
435+
def test_get_federated_id_token_github_timeout(self, get_mock):
436+
import requests
437+
get_mock.side_effect = requests.exceptions.Timeout()
438+
with self.assertRaisesRegex(CLIError, 'Timed out'):
439+
get_federated_id_token()
418440

419441
@mock.patch.dict(os.environ, {
420442
'ACTIONS_ID_TOKEN_REQUEST_URL': 'https://github.example/token',

‎src/azure-cli/azure/cli/command_modules/profile/__init__.py‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -94,9 +94,10 @@ def load_arguments(self, command):
9494
options_list=['--federated-token-callback', '--federated-token-cmd'],
9595
help='A command that prints a fresh OIDC federated token to stdout. Azure CLI runs it on '
9696
'demand to refresh the token, so it works with any CI/CD provider. The command runs '
97-
'without a shell; to use pipes or redirection, point to a script file or wrap it, '
98-
'e.g. "bash -c \'...\'". Cannot be combined with --federated-token or '
99-
'--federated-identity.')
97+
'without a shell (arguments are parsed with POSIX rules; on Windows use forward '
98+
'slashes or escape backslashes). To use pipes or redirection, point to a script '
99+
'file or wrap it, e.g. "bash -c \'...\'". Cannot be combined with --federated-token '
100+
'or --federated-identity.')
100101

101102
# Managed identity
102103
c.argument('identity', options_list=('-i', '--identity'), action='store_true',

0 commit comments

Comments
 (0)