-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[v2][sso][feedback-wanted] Configure SSO session support #7364
Conversation
Codecov Report
@@ Coverage Diff @@
## v2 #7364 +/- ##
=======================================
Coverage 93.71% 93.71%
=======================================
Files 352 352
Lines 36355 36355
Branches 5227 5227
=======================================
Hits 34070 34070
Misses 1660 1660
Partials 625 625 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
a3cf58e
to
7b86cbe
Compare
This commit includes three new features: * Add support for configure SSO sessions as part of the `configure sso` command. When running this command, users will be prompted for a SSO session to start in which they can create a new SSO session or reuse an existing SSO session when configuring a profile. * Add `configure sso-session` command. This allows users to configure a SSO session without having to reconfigure a profile. * Add `--sso-session` argument to `sso login` command. This allows users to refresh the SSO access token explicitly through a configured SSO session instead of having to specify a profile that is configured to use the SSO session in order to refresh the token.
This commit both adds the new test cases and refactors the old test cases to leverage pytest fixtures and make setup logic more reusable across test cases.
7b86cbe
to
cd34669
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor things but overall looks good.
_CMD_PROMPT_USAGE = ( | ||
'To keep an existing value, hit enter when prompted for the value. When ' | ||
'you are prompted for information, the current value will be displayed in ' | ||
'[brackets]. If the config item has no value, it is displayed as ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if the double space was intended. It's not used above at " for the value. When", so we may want to make them consistent.
'[brackets]. If the config item has no value, it is displayed as ' | |
'[brackets]. If the config item has no value, it is displayed as ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it was not intended here. I think it was copied from the previous docstrings. I'll fix it.
document, 'Scope values must be separated by commas') | ||
|
||
def _is_comma_separated_list(self, value): | ||
value.strip() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this strip call for? It shouldn't modify value
in place, it only returns the stripped value. Did we mean to assign this? If not maybe a comment is needed here as this seems wrong/magic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not needed. I think is just an artifact that did not get cleaned up from a refactoring.
profile_name = self._prompter.get_value(default_profile, text) | ||
default_profile = None | ||
if sso_account_id and sso_role_name: | ||
default_profile = '{}-{}'.format(sso_role_name, sso_account_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
default_profile = '{}-{}'.format(sso_role_name, sso_account_id) | |
default_profile = f'{sso_role_name}-{sso_account_id}' |
awscli/customizations/sso/utils.py
Outdated
scope = scope.strip() | ||
if scope: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to you, but a rare opportunity for a walrus!
scope = scope.strip() | |
if scope: | |
if scope := scope.strip(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I think this was just copied from over from a previous definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⛵
It seems like there's now no way to configure SSO sessions unless you do it interactively. For example you can set profile settings like: aws configure set profile.profile-name.region eu-west-1 # works fine But you cannot set SSO session settings in a similar fashion: aws configure set sso-session.session-name.sso_region eu-west-1 # does not work Are there any plans to add support for non-interactive configuration? |
|
This pull request adds support for configuring
sso-session
sections with the AWS CLI. Specifically, it adds the following new functionality:configure sso
command to support creating and selectingsso-session
sections as part of configuring a SSO-enabled profile.configure sso-session
command for creating and updatingsso-session
sections.--sso-session
argument tosso login
command to enable direct SSO login with a configuredsso-session
As part of the review for this functionality, we are seeking community feedback on the updates to the
configure sso
command and the newconfigure sso-session
command. See end of this overview for details on how to give feedback.Background
sso-session
sections, there is no guarantee that the SDK or tool you are using will respect your configuredsso-session
section.In the near future, AWS SDKs and tools plan to roll out support for respecting the
sso-session
section as part of the AWS SSO credential provider. Thesso-session
section is a named grouping of configuration variables for acquiring SSO access tokens, which can then be used to acquire AWS credentials. Furthermore, when an access token derived from asso-session
configuration is cached, it is done by session name instead of start URL.Currently in order to configure the AWS CLI and SDKs to use SSO, a profile is configured via the following format with all SSO-related configuration specified in the profile.
With the rollout, users will be able to declare a
sso-session
section that can be associated to a profile:This also allows
sso-session
configurations to be reused across profiles:Furthermore, registration scopes can be configured as part of a
sso-session
. These scopes define the permissions associated to the registered OIDC client and access tokens retrieved by the client. For example, the below configuration provides access for listing accounts/roles and acquiring AWS credentials:This pull request aims to enable interactive configuration of the
sso-session
sections and associations to profiles.Demo
Below shows the new
configure sso
flow for configuring both profiles and SSO sessions:sso-demo-pr.mp4
Note: The
configure sso-session
command uses the same interactive prompts as used in the updatedconfigure sso
commandNote on legacy configuration format
Users can still use the
configure sso
command to configure SSO access using the profile-only format. To opt-into this format, enter no answer for the session name prompt and follow the same prompts as before this change:How to give feedback
configure sso
updates or the newconfigure sso-session
command, leave a comment on this pull request.