Skip to content
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

sso_session in config should maybe disallow sso_start_url and sso_region #2835

Open
2 tasks
benkehoe opened this issue Dec 12, 2022 · 7 comments
Open
2 tasks
Labels
configuration feature-request This issue requests a feature. needs-review This issue or pull request needs review from a core team member. p2 This is a standard priority issue sso

Comments

@benkehoe
Copy link

benkehoe commented Dec 12, 2022

Describe the feature

The new support for sso_session in config is great! But it's allowed to create a profile that has both sso_session and sso_start_url and sso_region. This is causing an issue here benkehoe/aws-sso-util#83 but more generally, it allows an ambiguous profile to be created:

[profile my-profile]
sso_start_url = https://foo.awsapps.com/start
sso_region = us-east-1
sso_session = my-session
sso_account_id = 123456789012
sso_role_name = MyRole

[sso-session my-session]
sso_start_url = https://bar.awsapps.com/start
sso_region = us-west-2

with SDKs before session support, this will grab a token from the cache for https://foo.awsapps.com/start and with SDKs after, it'll use https://bar.awsapps.com/start (via the cache entry for the session). While this is generally accepted for different types of credential configuration (and in aws/aws-sdk-go#3763 I specifically asked for SSO + credential process config to be allowed), allowing it for the same type of credential configuration seems weird to me, and possibly a source for errors.

I can see no good reason someone would want to have both inline and session-based SSO config in the same profile. Maybe it'd be better if it caused an error to keep people from expressing config that they don't intend.

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

SDK version used

1.29.10 and later

Environment details (OS name and version, etc.)

N/A

@benkehoe benkehoe added feature-request This issue requests a feature. needs-triage This issue or PR still needs to be triaged. labels Dec 12, 2022
@tim-finnigan
Copy link
Contributor

Hi @benkehoe thanks for reaching out. When attempting to use those profile configurations I received this error message:

The value for sso_start_url is inconsistent between profile (https://foo.awsapps.com/start) and sso-session (https://bar.awsapps.com/start).

And if the sso_start_url is the same for both profiles then I receive this error:

The value for sso_region is inconsistent between profile (us-east-1) and sso-session (us-west-2).

So since the conflicting settings fail to validate then I think this scenario is accounted for. But please let me know if I'm misunderstanding this or if there's anything you'd like to expand on.

@tim-finnigan tim-finnigan added response-requested Waiting on additional info and feedback. configuration sso and removed needs-triage This issue or PR still needs to be triaged. labels Dec 14, 2022
@github-actions
Copy link

Greetings! It looks like this issue hasn’t been active in longer than five days. We encourage you to check if this is still an issue in the latest release. In the absence of more information, we will be closing this issue soon. If you find that this is still a problem, please feel free to provide a comment or upvote with a reaction on the initial post to prevent automatic closure. If the issue is already closed, please feel free to open a new one.

@benkehoe
Copy link
Author

Ok, I think I see now. I recently ceased to have access to two separate Identity Center instances, so when investigating benkehoe/aws-sso-util#83 I had to base my understanding on the code.

So what I'm seeing is that cache entries created using the old process (changed some time before CLI v2.9.0) are not forward compatible, so if, say, someone was using CLI v2.7.0 and boto3 v1.26.33, with the valid configuration:

[profile my-profile]
sso_start_url = https://foo.awsapps.com/start
sso_region = us-east-1
sso_session = my-session
sso_account_id = 123456789012
sso_role_name = MyRole

[sso-session my-session]
sso_start_url = https://foo.awsapps.com/start
sso_region = us-east-1

then
aws sso login --profile my-profile followed by

import boto3
session = boto3.Session(profile_name='my-profile')
session.client('sts').get_caller_identity()

will raise an error botocore.exceptions.SSOTokenLoadError: Error loading SSO Token: Token for tmp-session does not exist. It looks like AWS CLI v2.8.0 refused to perform login on such a profile with the error Inline SSO configuration and sso_session cannot be configured on the same profile. I guess that makes sense to me, and I'm asking why it was changed for 2.9.0

@github-actions github-actions bot removed closing-soon response-requested Waiting on additional info and feedback. labels Dec 20, 2022
@tim-finnigan
Copy link
Contributor

Sorry this issue fell off my radar. There were a few sso-session changes in 2.9.0 and I don't have a lot of context on those but can try to find out more information. The Token...does not exist error was added here in botocore.

@tim-finnigan tim-finnigan added needs-review This issue or pull request needs review from a core team member. p2 This is a standard priority issue labels Mar 8, 2023
@benkehoe
Copy link
Author

benkehoe commented Mar 9, 2023

I'm having trouble locating in the code for CLI 2.8.0 the error that I got: Inline SSO configuration and sso_session cannot be configured on the same profile. So I can't point to the logic that was used.

My question is, why is it good to let people have both kinds of configuration on a profile when they agree, leaving them to get an error later when there's drift? Wouldn't it be better to have it give an upfront error, especially when the CLI had that behavior at one point?

@bbbush
Copy link

bbbush commented Mar 25, 2023

[profile my-profile]
sso_session = my-session
sso_account_id = 123456789012
sso_role_name = MyRole

not sure if my understanding is correct. When "sso_session" is configured, botocore assumes that the cache exists and one can read the expiration date? before trying to refresh https://github.com/boto/botocore/blob/develop/botocore/tokens.py#L306 vs. the immediate throw of exception when cache does not exist https://github.com/boto/botocore/blob/develop/botocore/utils.py#L3100

@amberkushwaha
Copy link

Describe the main circuit log in three module sector in the botocore logging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
configuration feature-request This issue requests a feature. needs-review This issue or pull request needs review from a core team member. p2 This is a standard priority issue sso
Projects
None yet
Development

No branches or pull requests

4 participants