[V2] Add the OAuth Authorization Code Flow with PKCE by ashovlin · Pull Request #8947 · aws/aws-cli

ashovlin · 2024-09-26T14:30:17Z

Issue #, if available: N/A

Description of changes:
This adds support for the OAuth2.0 authorization code flow with PKCE to the aws sso login command. It is the new default behavior, but users can fall back to the current, device code flow using the new --use-device-code option.

Auth code + PKCE is recommended for devices with browsers and addresses some phishing vectors that are inherent in the device code flow.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

ashovlin · 2024-09-26T14:45:43Z

@@ -0,0 +1,176 @@
+<!DOCTYPE html>


For reviewers, I lifted this from the IDE integration.

I did not include some of the extra styles and fonts, since having everything in one file simplifies serving the resources from the local server. But let me know if you'd like the additional fonts/icons.

Unrelated to the above from an internal discussion, we need to make sure that this file is included in the source distribution. DOes it need to be in manifest.in?

It shouldn't need to be since it's under the top-level package directory (awscli). We can generate the sdist to double check. Also I don't think v2 will use a manifest.in, the sdist file inclusions outside of the core package should be listed here.

ashovlin · 2024-09-26T14:48:41Z

+    def _extract_resolved_endpoint(self, params, **kwargs):
+        """Event handler for before-call that will extract the resolved endpoint
+        for a given request without actually running it
+        """
+        # This will contain any path and query params specific to
+        # the operation/input, so extract just the scheme and hostname
+        if params['url']:
+            parsed = urlparse(params['url'])
+            self._base_endpoint = f'{parsed.scheme}://{parsed.netloc}'
+
+        # Return a tuple containing the "response" to short-circuit the request
+        return botocore.awsrequest.AWSResponse(None, 200, {}, None), {}
+
+    def _get_base_authorization_uri(self):
+        """Simulates an SSO-OIDC request so that we can extract the "base"
+        endpoint for the current client to use for the un-modeled Authorize
+        operation
+        """
+        self._client.meta.events.register('before-call',
+                                          self._extract_resolved_endpoint)
+        self._client.register_client(
+            clientName='temp',
+            clientType='public'
+        )
+        self._client.meta.events.unregister('before-call',
+                                            self._extract_resolved_endpoint)
+
+        return self._base_endpoint


For reviewers, this is so that we can resolve the "base" endpoint using the endpoint rules now that client.meta.endpoint_url is not routed through the endpoint resolver.

It's a little awkward since we're half-running this request, but this is possible using botocore's current public interface.

A possibly cleaner alternative would be to expose client._resolve_endpoint_ruleset publicly, or allow some other public instantiation of the SSO-OIDC endpoint resolver.

nateprewitt

There's a mix of formatting nits and some functionality questions baked in here. Let me know if I can clarify anything.

nateprewitt · 2024-09-26T22:03:06Z

+
+        if state != expected_state:
+            raise AuthorizationCodeLoadError(
+                error_msg='State parameter does not match expected value.'


Would it be useful to include one or both of those? Or is this mechanism opaque to the end user? I'm just not sure what I'm supposed to do if I get this error.

It is opaque because it's generated internally. This is guarding against CSFR, in case an an attacker is able to hit the redirect endpoint with their authorization code, injecting the attackers credentials into the user's application/workflow.

https://datatracker.ietf.org/doc/html/rfc6749#section-10.12

Users shouldn't see it short of a bug, bit flip, or if something is hitting the redirect before the normal worfklow does.

Do you think it's worth adding something like "You can try the SSO login workflow again."?

VS Code as a reference: https://github.com/aws/aws-toolkit-vscode/blob/3b6197b2cfa8345a0775096f06e0983aec50102b/packages/core/src/auth/sso/server.ts#L34-L38

hssyoo

This looks solid. I mainly had smaller comments on following the repo's conventions.

I also noticed that we didn't add tests for SSOTokenFetcherAuth in tests/unit/botocore/test_utils.py.

hssyoo · 2024-10-01T14:30:03Z

+
+    def test_expected_auth_code(self):
+        expected_code = '1234'
+        expected_state = '4567'


Since the client code uses uuid.uuid4() (which returns a UUID object) to assign the expected state, let's make sure the test reflects that.

I think this one is okay. From the viewpoint of the AuthCodeFetcher/OAuthCallbackHandler, the state is just a string that it gets from the query parameters.

It's over in SSOTokenFetcherAuth that it generates the UUID, the compares the string that was roundtripped to the original value. Here it's important to handle UUID vs. string and where the original revision has the bug.

hssyoo · 2024-10-01T14:39:47Z

+  "category": "sso",
+  "description": "Add support and default to the OAuth 2.0 Authorization Code Flow with PKCE for aws sso login."


nit: You can wrap text in double backticks (``) to render in RST as inline code. Here, let's wrap sso and aws sso login.

Done via e306e3e

hssyoo · 2024-10-01T14:54:50Z

                 on_pending_authorization=None, force_refresh=False,
-                 registration_scopes=None, session_name=None):
+                 registration_scopes=None, session_name=None,
+                 fallback_to_device_flow=False):


Suggested change

fallback_to_device_flow=False):

use_device_code=False):

To me, fallback_to_device_flow implies that it'll attempt to use PKCE and fallback to device auth if it fails but it looks like setting this to True will always result in device auth being used. And in general I tend to prefer keeping the parameter name unless we're doing extra stuff under the hood.

Done via e306e3e (didn't accept suggestion on GitHub since we had to rename in multiple places).

hssyoo · 2024-10-01T15:42:11Z

+        query_params = parse_qs(urlparse(self.path).query)
+
+        if 'error' in query_params:
+            self._auth_code_fetcher._is_done = True


non-blocking: It feels a bit weird to explicitly set AuthCodeFetcher internal attributes here. I wonder if it's worth exposing an AuthCodeFetcher method (eg set_auth_code_state()) that can be called here instead. Feel free to take it or leave it.

Done via e306e3e

hssyoo · 2024-10-01T18:10:40Z

+        if scopes:
+            register_kwargs['scopes'] = scopes
+        else:
+            register_kwargs['scopes'] = [self._AUTH_GRANT_DEFAULT_SCOPE]


We can just add 'scopes': scopes or [self._AUTH_GRANT_DEFAULT_SCOPE] to register_kwargs

Done via e306e3e

hssyoo · 2024-10-01T18:19:57Z

+
+        query_params = parse_qs(urlparse(self.path).query)
+
+        if 'error' in query_params:


Is there anything useful we can extract from query_params['error'] for the error message when auth_code is None?

We show it in the browser via:

aws-cli/awscli/customizations/sso/index.html

Lines 168 to 172 in e306e3e

function showErrorMessage(errorText) {

document.getElementById('approved-auth').classList.add('hidden')

document.getElementById('denied-auth').classList.remove('hidden')

document.getElementById('errorMessage').innerText = errorText

}

Open to repeating it in the console if you want? Might be useful for someone providing us debug logs.

Non-blocking: I'm in favor of it being visible in console via debug logs.

hssyoo · 2024-10-01T18:26:40Z

        self.assertIsNone(captured_env.get('LD_LIBRARY_PATH'))
+
+
+class TestAuthCodeFetcher(unittest.TestCase):


Generally, we want to use pytest and move away from unittest unless we're adding a couple test cases to an existing test class/module. Since this is a new test class let's use pytest.

Done via e306e3e

hssyoo · 2024-10-01T18:47:14Z

+        self.assertEqual(self.fetcher._auth_code, expected_code)
+        self.assertEqual(self.fetcher._state, expected_state)


I think this assertion could be cleaner if we asserted against the returned values for self.fetcher.get_auth_code_and_state(). To do this we could return early if _is_done is true in AuthCodeFetcher:

def get_auth_code_and_state(self): """Blocks until the expected redirect request with either the authorization code/state or and error is handled """ if self._is_done: return self._auth_code, self._state while not self._is_done: self.http_server.handle_request() self.http_server.server_close() return self._auth_code, self._state

Done via e306e3e

hssyoo · 2024-10-01T18:49:29Z

+        self.assertEqual(response.status, 200)
+        self.assertEqual(self.fetcher._auth_code, None)
+        self.assertEqual(self.fetcher._state, None)
+


It'd be nice to also have unit tests for OAuthCallbackHandler. At a minimum, we should mock return values in do_GET() and assert that the self._auth_code_fetcher attributes are set.

Done via 6cbf403

ashovlin · 2024-11-11T20:59:22Z

c96d05c addresses two issues found during internal testing:

When defaulting sso:account:access as a default scope, we need to do that for the authorize call in addition to the client registration
Correct query_param key for scopes should be scopes, not scope.

Added assertions to existing unit tests for both cases.

hssyoo

🏆

This adds support for the OAuth2.0 authorization code flow with PKCE to the aws sso login command. It is the new default behavior, but users can fall back to the device code flow using the new --use-device-code option.

ashovlin commented Sep 26, 2024

View reviewed changes

ashovlin requested review from hssyoo and nateprewitt September 26, 2024 18:18

nateprewitt reviewed Sep 26, 2024

View reviewed changes

hssyoo reviewed Oct 1, 2024

View reviewed changes

ashovlin force-pushed the shovlia/sso-auth-pkce branch from 6cbf403 to 0f50729 Compare October 17, 2024 16:01

ashovlin requested review from aemous, hssyoo and nateprewitt November 11, 2024 20:59

hssyoo approved these changes Nov 14, 2024

View reviewed changes

Add the OAuth Authorization Code Flow with PKCE

e0ee9eb

This adds support for the OAuth2.0 authorization code flow with PKCE to the aws sso login command. It is the new default behavior, but users can fall back to the device code flow using the new --use-device-code option.

ashovlin force-pushed the shovlia/sso-auth-pkce branch from 0fc2814 to e0ee9eb Compare November 15, 2024 15:31

ashovlin mentioned this pull request Nov 15, 2024

Add additional user agent metadata for the SSO auth code flow ashovlin/aws-cli#1

Merged

Add additional user agent metadata for the SSO auth code flow

e78d532

ashovlin merged commit 130005a into aws:v2 Nov 18, 2024

ashovlin deleted the shovlia/sso-auth-pkce branch November 18, 2024 18:04

		"category": "sso",
		"description": "Add support and default to the OAuth 2.0 Authorization Code Flow with PKCE for aws sso login."


		query_params = parse_qs(urlparse(self.path).query)

		if 'error' in query_params:

	function showErrorMessage(errorText) {
	document.getElementById('approved-auth').classList.add('hidden')
	document.getElementById('denied-auth').classList.remove('hidden')
	document.getElementById('errorMessage').innerText = errorText
	}

		self.assertIsNone(captured_env.get('LD_LIBRARY_PATH'))


		class TestAuthCodeFetcher(unittest.TestCase):

		self.assertEqual(self.fetcher._auth_code, expected_code)
		self.assertEqual(self.fetcher._state, expected_state)

Uh oh!

Conversation

ashovlin commented Sep 26, 2024

Uh oh!

ashovlin Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ashovlin Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nateprewitt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ashovlin Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hssyoo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ashovlin commented Nov 11, 2024

Uh oh!

hssyoo left a comment

Choose a reason for hiding this comment

ashovlin Sep 26, 2024 •

edited

Loading

ashovlin Sep 26, 2024 •

edited

Loading

ashovlin Oct 15, 2024 •

edited

Loading