From 734e7bbcff673422100dcd08c6ef7abf42ae782a Mon Sep 17 00:00:00 2001 From: aemous Date: Wed, 11 Nov 2026 16:11:13 -0500 Subject: [PATCH] Validate state in the OAuth callback server When a caller passes expected_state to AuthCodeFetcher.get_auth_code_and_state, OAuthCallbackHandler only ends the wait on a GET /oauth/callback whose state matches (constant time compare). Other paths get 404, a missing or wrong state gets 400, and the wait continues. Each connection gets _REQUEST_TIMEOUT, and the authorization server's error and error_description are kept in AuthCodeFetcher.error. aws sso login and aws login now pass their state. The server also logs handler errors, such as a browser disconnecting early, at debug instead of printing a traceback. --- .../next-release/enhancement-sso-12768.json | 5 + awscli/botocore/utils.py | 4 +- awscli/customizations/login/utils.py | 4 +- awscli/customizations/sso/utils.py | 135 ++++++++- tests/functional/sso/test_login.py | 11 + tests/unit/customizations/login/test_utils.py | 3 + tests/unit/customizations/sso/test_utils.py | 273 ++++++++++++++++++ 7 files changed, 421 insertions(+), 14 deletions(-) create mode 100644 .changes/next-release/enhancement-sso-12768.json diff --git a/.changes/next-release/enhancement-sso-12768.json b/.changes/next-release/enhancement-sso-12768.json new file mode 100644 index 000000000000..048b5e1b6552 --- /dev/null +++ b/.changes/next-release/enhancement-sso-12768.json @@ -0,0 +1,5 @@ +{ + "type": "enhancement", + "category": "``sso``", + "description": "The authorization code callback server for ``aws sso login`` and ``aws login`` now ignores requests on other paths or without the expected ``state``, and keeps waiting for the real redirect instead of failing. It also no longer prints a traceback when the browser disconnects early." +} diff --git a/awscli/botocore/utils.py b/awscli/botocore/utils.py index 50a728948f0e..6f68938d1c57 100644 --- a/awscli/botocore/utils.py +++ b/awscli/botocore/utils.py @@ -3737,7 +3737,9 @@ def _get_new_token( # Open/display the link, then block until the redirect uri is hit and # the auth code is retrieved self._on_pending_authorization(**authorization_args) - auth_code, state = self._auth_code_fetcher.get_auth_code_and_state() + auth_code, state = self._auth_code_fetcher.get_auth_code_and_state( + expected_state=str(expected_state) + ) if auth_code is None: raise AuthorizationCodeLoadError( diff --git a/awscli/customizations/login/utils.py b/awscli/customizations/login/utils.py index ca74d4fb729b..d92018739056 100644 --- a/awscli/customizations/login/utils.py +++ b/awscli/customizations/login/utils.py @@ -242,7 +242,9 @@ def fetch_token(self): self._on_pending_authorization( **self._get_browser_handler_args(authorization_uri) ) - auth_code, state = self._auth_code_fetcher.get_auth_code_and_state() + auth_code, state = self._auth_code_fetcher.get_auth_code_and_state( + expected_state=str(self._expected_state) + ) if auth_code is None: raise LoginAuthorizationCodeError( diff --git a/awscli/customizations/sso/utils.py b/awscli/customizations/sso/utils.py index 97be3b3da1b9..91303cfef203 100644 --- a/awscli/customizations/sso/utils.py +++ b/awscli/customizations/sso/utils.py @@ -11,6 +11,7 @@ # ANY KIND, either express or implied. See the License for the specific # language governing permissions and limitations under the License. import datetime +import hmac import json import logging import os @@ -256,10 +257,13 @@ class AuthCodeFetcher: _REQUEST_TIMEOUT = 10 # How long we wait overall for the callback _OVERALL_TIMEOUT = 60 * 10 + # The path of the redirect URI that the callback is expected on + _REDIRECT_PATH = '/oauth/callback' def __init__(self, redirect_port=None): self._auth_code = None self._state = None + self._error = None self._is_done = False # We do this so that the request handler can have a reference to this @@ -267,23 +271,47 @@ def __init__(self, redirect_port=None): try: handler = partial(OAuthCallbackHandler, self) server_port = 0 if redirect_port is None else redirect_port - self.http_server = HTTPServer(('127.0.0.1', server_port), handler) + self.http_server = _QuietHTTPServer( + ('127.0.0.1', server_port), handler + ) self.http_server.timeout = self._REQUEST_TIMEOUT except OSError as e: raise AuthCodeFetcherError(error_msg=e) def redirect_uri_without_port(self): - return 'http://127.0.0.1/oauth/callback' + return f'http://127.0.0.1{self._REDIRECT_PATH}' def redirect_uri_with_port(self): return ( - f'http://127.0.0.1:{self.http_server.server_port}/oauth/callback' + f'http://127.0.0.1:{self.http_server.server_port}' + f'{self._REDIRECT_PATH}' ) - def get_auth_code_and_state(self): + @property + def error(self): + """The error the authorization server redirected back with, as a + dict with ``error`` and ``error_description`` keys, or None. Only + recorded when waiting with an ``expected_state``. + """ + if self._error is None: + return None + return dict(self._error) + + def get_auth_code_and_state(self, expected_state=None): """Blocks until the expected redirect request with either the authorization code/state or and error is handled + + When ``expected_state`` (a string) is given, only a GET on the + redirect URI's path whose ``state`` matches it ends the wait, and + each connection gets ``_REQUEST_TIMEOUT`` to send its request. + Anything else is rejected and the wait continues. """ + if expected_state is not None: + self.http_server.RequestHandlerClass = partial( + OAuthCallbackHandler, + self, + expected_state=expected_state, + ) LOG.debug(f'Waiting for auth code at {self.redirect_uri_with_port()}') start = time.time() while ( @@ -302,15 +330,43 @@ def set_auth_code_and_state(self, auth_code, state): self._state = state self._is_done = True + def set_error_and_state(self, error, error_description, state): + self._error = { + 'error': error, + 'error_description': error_description, + } + self.set_auth_code_and_state(None, state) + + +class _QuietHTTPServer(HTTPServer): + def handle_error(self, request, client_address): + # The default prints a traceback to stderr, e.g. when the browser + # disconnects before reading the page. + LOG.debug('Auth callback request failed', exc_info=True) + class OAuthCallbackHandler(BaseHTTPRequestHandler): """HTTP handler to handle OAuth callback requests, extracting the auth code and state parameters, and displaying a page directing the user to return to the CLI. + + When ``expected_state`` is given, any local process or web page can + reach the callback server, so only a GET on the redirect URI's path with + the expected state is acted on. Everything else gets an error response + and the wait continues. """ - def __init__(self, auth_code_fetcher, *args, **kwargs): + def __init__( + self, auth_code_fetcher, *args, expected_state=None, **kwargs + ): self._auth_code_fetcher = auth_code_fetcher + self._expected_state = None + if expected_state is not None: + self._expected_state = expected_state.encode('utf-8') + # StreamRequestHandler.setup() applies this to the connection, + # so a client that never sends its request can't hold up the + # server + self.timeout = auth_code_fetcher._REQUEST_TIMEOUT super().__init__(*args, **kwargs) def log_message(self, format, *args): @@ -323,13 +379,11 @@ def version_string(self): return f'AWS CLI/{awscli_version}' def do_GET(self): - self.send_response(200) - self.end_headers() - with open( - os.path.join(os.path.dirname(__file__), 'index.html'), - 'rb', - ) as file: - self.wfile.write(file.read()) + if self._expected_state is not None: + self._handle_strict_callback() + return + + self._send_index_page() query_params = parse_qs(urlparse(self.path).query) @@ -344,6 +398,63 @@ def do_GET(self): query_params['state'][0], ) + def _handle_strict_callback(self): + parsed_url = urlparse(self.path) + if parsed_url.path != self._auth_code_fetcher._REDIRECT_PATH: + self._send_text(404, 'Not found.') + return + + query_params = parse_qs(parsed_url.query) + state = query_params.get('state', [None])[0] + if state is None or not hmac.compare_digest( + state.encode('utf-8'), self._expected_state + ): + self._send_text( + 400, + 'This request does not belong to the sign-in in progress. ' + 'Return to the terminal and use the URL it displayed.', + ) + return + + if 'error' in query_params: + self._auth_code_fetcher.set_error_and_state( + query_params['error'][0], + query_params.get('error_description', [None])[0], + state, + ) + elif 'code' in query_params: + self._auth_code_fetcher.set_auth_code_and_state( + query_params['code'][0], + state, + ) + else: + self._send_text( + 400, + 'The request has neither an authorization code nor an error.', + ) + return + + # The page shows the error from the query string, if there is one + self._send_index_page() + + def _send_index_page(self): + self.send_response(200) + self.end_headers() + with open( + os.path.join(os.path.dirname(__file__), 'index.html'), + 'rb', + ) as file: + self.wfile.write(file.read()) + + def _send_text(self, status, text): + LOG.debug(f'Rejected OAuth callback request with HTTP {status}') + body = f'{text}\n'.encode() + self.send_response(status) + self.send_header('Content-Type', 'text/plain; charset=utf-8') + self.send_header('Content-Length', str(len(body))) + self.end_headers() + self.wfile.write(body) + class InvalidSSOConfigError(ConfigurationError): pass diff --git a/tests/functional/sso/test_login.py b/tests/functional/sso/test_login.py index b78498f34666..02b8118bd236 100644 --- a/tests/functional/sso/test_login.py +++ b/tests/functional/sso/test_login.py @@ -475,6 +475,17 @@ def test_login_auth_sso_no_authorization_code_throws_error(self): _, stderr, _ = self.run_cmd('sso login', expected_rc=255) self.assertIn('Failed to retrieve an authorization code.', stderr) + def test_login_auth_sso_waits_for_expected_state(self): + content = self.get_sso_session_config('test-session') + self.set_config_file_content(content=content) + self.add_oidc_auth_code_responses(self.access_token) + + self.run_cmd('sso login') + fetcher = self.fetcher_mock.return_value + fetcher.get_auth_code_and_state.assert_called_once_with( + expected_state='00000000-0000-0000-0000-000000000000' + ) + def test_login_auth_sso_state_mismatch_throws_error(self): self.fetcher_mock.return_value.get_auth_code_and_state.return_value = ( "abc", diff --git a/tests/unit/customizations/login/test_utils.py b/tests/unit/customizations/login/test_utils.py index 422f8e38a8f6..834103194e6e 100644 --- a/tests/unit/customizations/login/test_utils.py +++ b/tests/unit/customizations/login/test_utils.py @@ -145,6 +145,9 @@ def test_same_device_fetch_token_success( assert token == EXPECTED_TOKEN assert session_id == 'arn:aws:sts::012345678901:assumed-role/Admin/admin' mock_on_pending_auth.assert_called_once() + mock_auth_code_fetcher.get_auth_code_and_state.assert_called_once_with( + expected_state=str(mock_same_device_fetcher._expected_state) + ) mock_client.create_o_auth2_token.assert_called_with( tokenInput={ 'clientId': 'arn:aws:signin:::devtools/same-device', diff --git a/tests/unit/customizations/sso/test_utils.py b/tests/unit/customizations/sso/test_utils.py index 1d99aa25eb94..af4973a81b25 100644 --- a/tests/unit/customizations/sso/test_utils.py +++ b/tests/unit/customizations/sso/test_utils.py @@ -268,6 +268,9 @@ def makefile(self, *args, **kwargs): def sendall(self, data): pass + def settimeout(self, timeout): + pass + class TestOAuthCallbackHandler: """Tests for OAuthCallbackHandler, which handles @@ -312,6 +315,151 @@ def test_missing_expected_query_params(self): fetcher.set_auth_code_and_state.assert_not_called() +def _create_mock_fetcher(): + fetcher = mock.Mock(AuthCodeFetcher) + fetcher._REQUEST_TIMEOUT = AuthCodeFetcher._REQUEST_TIMEOUT + fetcher._REDIRECT_PATH = AuthCodeFetcher._REDIRECT_PATH + return fetcher + + +class TestStrictOAuthCallbackHandler: + """Tests for OAuthCallbackHandler when it is given an expected state""" + + def _handle(self, request_line, expected_state='123'): + fetcher = _create_mock_fetcher() + request = MockRequest(request_line + b' HTTP/1.1\r\n\r\n') + handler = mock.MagicMock() + with ( + mock.patch.object( + OAuthCallbackHandler, 'send_response' + ) as send_response, + mock.patch.object(OAuthCallbackHandler, 'send_header'), + mock.patch.object(OAuthCallbackHandler, 'end_headers'), + ): + OAuthCallbackHandler( + fetcher, + request, + mock.MagicMock(), + handler, + expected_state=expected_state, + ) + return fetcher, send_response + + def test_expected_state_and_code(self): + fetcher, send_response = self._handle( + b'GET /oauth/callback?state=123&code=456' + ) + fetcher.set_auth_code_and_state.assert_called_once_with('456', '123') + send_response.assert_called_once_with(200) + + @pytest.mark.parametrize( + 'path', + [ + b'/?state=123&code=456', + b'/callback?state=123&code=456', + b'/oauth/callback/extra?state=123&code=456', + ], + ) + def test_other_path_is_rejected(self, path): + fetcher, send_response = self._handle(b'GET ' + path) + fetcher.set_auth_code_and_state.assert_not_called() + fetcher.set_error_and_state.assert_not_called() + send_response.assert_called_once_with(404) + + @pytest.mark.parametrize( + 'query', + [ + b'code=456', + b'state=999&code=456', + b'state=1234&code=456', + b'state=&code=456', + b'error=access_denied', + b'state=999&error=access_denied', + ], + ) + def test_missing_or_wrong_state_is_rejected(self, query): + fetcher, send_response = self._handle(b'GET /oauth/callback?' + query) + fetcher.set_auth_code_and_state.assert_not_called() + fetcher.set_error_and_state.assert_not_called() + send_response.assert_called_once_with(400) + + def test_error_with_expected_state(self): + fetcher, send_response = self._handle( + b'GET /oauth/callback?state=123&error=access_denied' + b'&error_description=User%20denied' + ) + fetcher.set_error_and_state.assert_called_once_with( + 'access_denied', 'User denied', '123' + ) + fetcher.set_auth_code_and_state.assert_not_called() + send_response.assert_called_once_with(200) + + def test_error_without_description(self): + fetcher, _ = self._handle( + b'GET /oauth/callback?state=123&error=access_denied' + ) + fetcher.set_error_and_state.assert_called_once_with( + 'access_denied', None, '123' + ) + + def test_neither_code_nor_error_is_rejected(self): + fetcher, send_response = self._handle(b'GET /oauth/callback?state=123') + fetcher.set_auth_code_and_state.assert_not_called() + fetcher.set_error_and_state.assert_not_called() + send_response.assert_called_once_with(400) + + def test_sets_per_connection_timeout_only_when_strict(self): + # setup() applies self.timeout to the connection, so it must be + # set before BaseRequestHandler.__init__ calls setup() + timeouts = [] + + def record_timeout(handler): + timeouts.append(handler.timeout) + + fetcher = _create_mock_fetcher() + fetcher._REQUEST_TIMEOUT = 7 + patch = mock.patch.object + with ( + patch(OAuthCallbackHandler, 'setup', record_timeout), + patch(OAuthCallbackHandler, 'handle'), + patch(OAuthCallbackHandler, 'finish'), + ): + for expected_state in ('123', None): + OAuthCallbackHandler( + fetcher, + MockRequest(b''), + mock.MagicMock(), + mock.MagicMock(), + expected_state=expected_state, + ) + assert timeouts == [7, None] + + def test_uses_the_fetchers_redirect_path(self): + fetcher = _create_mock_fetcher() + fetcher._REDIRECT_PATH = '/custom/callback' + with ( + mock.patch.object( + OAuthCallbackHandler, 'send_response' + ) as send_response, + mock.patch.object(OAuthCallbackHandler, 'send_header'), + mock.patch.object(OAuthCallbackHandler, 'end_headers'), + ): + for path in (b'/oauth/callback', b'/custom/callback'): + OAuthCallbackHandler( + fetcher, + MockRequest( + b'GET ' + + path + + b'?state=123&code=456 HTTP/1.1\r\n\r\n' + ), + mock.MagicMock(), + mock.MagicMock(), + expected_state='123', + ) + assert send_response.call_args_list == [mock.call(404), mock.call(200)] + fetcher.set_auth_code_and_state.assert_called_once_with('456', '123') + + def _create_auth_code_fetcher(**kwargs): # Binding the fetcher's HTTPServer does a reverse DNS lookup of the host, # which can stall for seconds. The resulting server name is unused. @@ -378,6 +526,131 @@ def test_get_auth_code_and_state_timeout(): _create_auth_code_fetcher().get_auth_code_and_state() +class TestAuthCodeFetcherWithExpectedState: + """Tests for the AuthCodeFetcher when waiting for a specific state""" + + expected_state = 'expected-state' + + def setup_method(self): + self.fetcher = _create_auth_code_fetcher() + self.url = self.fetcher.redirect_uri_with_port() + self.result = {} + + def wait(): + try: + self.result['value'] = self.fetcher.get_auth_code_and_state( + expected_state=self.expected_state + ) + except Exception as e: + self.result['error'] = e + + self.server_thread = threading.Thread(target=wait) + self.server_thread.daemon = True + self.server_thread.start() + self.http = urllib3.PoolManager(retries=False) + + def teardown_method(self): + if self.server_thread.is_alive(): + self.fetcher.set_auth_code_and_state(None, None) + # Wake up handle_request so the wait loop sees it's done + try: + urllib3.PoolManager(retries=False).request( + 'GET', self.url, timeout=1 + ) + except urllib3.exceptions.HTTPError: + pass + self.server_thread.join(timeout=5) + + def _get_result(self): + self.server_thread.join(timeout=5) + assert not self.server_thread.is_alive() + return self.result['value'] + + def test_expected_auth_code(self): + response = self.http.request( + 'GET', f'{self.url}?code=1234&state={self.expected_state}' + ) + assert response.status == 200 + assert self._get_result() == ('1234', self.expected_state) + assert self.fetcher.error is None + + def test_stray_requests_do_not_end_the_wait(self): + base = self.url.replace('/oauth/callback', '') + stray = [ + (f'{base}/?code=1&state={self.expected_state}', 404), + (f'{base}/favicon.ico', 404), + (f'{self.url}?code=1&state=wrong', 400), + (f'{self.url}?code=1', 400), + (f'{self.url}?error=access_denied', 400), + ] + for url, status in stray: + assert self.http.request('GET', url).status == status + assert self.server_thread.is_alive() + + response = self.http.request( + 'GET', f'{self.url}?code=1234&state={self.expected_state}' + ) + assert response.status == 200 + assert self._get_result() == ('1234', self.expected_state) + + def test_error_with_expected_state(self): + response = self.http.request( + 'GET', + f'{self.url}?error=access_denied&error_description=Denied' + f'&state={self.expected_state}', + ) + assert response.status == 200 + assert self._get_result() == (None, self.expected_state) + assert self.fetcher.error == { + 'error': 'access_denied', + 'error_description': 'Denied', + } + + +@mock.patch( + 'awscli.customizations.sso.utils.AuthCodeFetcher._REQUEST_TIMEOUT', 0.2 +) +def test_idle_connection_does_not_block_strict_wait(): + fetcher = _create_auth_code_fetcher() + result = {} + + def wait(): + result['value'] = fetcher.get_auth_code_and_state(expected_state='abc') + + thread = threading.Thread(target=wait, daemon=True) + thread.start() + port = fetcher.http_server.server_port + # Open a connection and never send a request + with socket.create_connection(('127.0.0.1', port)): + thread.join(timeout=0.5) + response = urllib3.PoolManager(retries=False).request( + 'GET', + f'http://127.0.0.1:{port}/oauth/callback?code=1&state=abc', + timeout=5, + ) + assert response.status == 200 + thread.join(timeout=5) + assert result['value'] == ('1', 'abc') + + +def test_auth_code_fetcher_server_logs_handler_errors_at_debug(capsys): + fetcher = _create_auth_code_fetcher() + try: + try: + raise BrokenPipeError('client went away') + except BrokenPipeError: + with mock.patch('awscli.customizations.sso.utils.LOG') as log: + fetcher.http_server.handle_error( + mock.Mock(), ('127.0.0.1', 1234) + ) + log.debug.assert_called_once_with( + 'Auth callback request failed', exc_info=True + ) + assert capsys.readouterr().err == '' + finally: + fetcher.http_server.server_close() + + def test_auth_code_fetcher_uses_redirect_port(): redirect_port = _get_available_port() fetcher = _create_auth_code_fetcher(redirect_port=redirect_port)