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

feat(sso): use SSOTokenProvider in SSOCredentialProvider #4145

Merged
merged 7 commits into from Nov 11, 2022
Merged

Conversation

kuhe
Copy link
Contributor

@kuhe kuhe commented Nov 4, 2022

Issue

internal JS-3637

Description

  • use the SSO token provider if a compatible sso profile format is detected.

Testing

  • created SSO organization in test account
  • able to call S3 list buckets with unexpired SSO token from file using legacy config format
  • able to call S3 list buckets with unexpired SSO token from file using new config format
  • able to call S3 list buckets with refresh SSO token from file using new config format

Additional test instructions will be documented internally.

@kuhe kuhe self-assigned this Nov 4, 2022
@kuhe kuhe marked this pull request as ready for review November 7, 2022 19:34
@kuhe kuhe requested a review from a team as a code owner November 7, 2022 19:34
@kuhe
Copy link
Contributor Author

kuhe commented Nov 9, 2022

corresponding v2 PR: aws/aws-sdk-js#4267

@@ -71,7 +71,7 @@ export const fromSso =
ssoToken = await getSSOTokenFromFile(ssoSessionName);
} catch (e) {
throw new TokenProviderError(
`The SSO session associated with this profile is invalid. ${REFRESH_MESSAGE}`,
`The SSO session associated with this profile is invalid. ${REFRESH_MESSAGE}\n${e}`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The refresh message provides relevant information. I don't think we should rethrow error here.

Suggested change
`The SSO session associated with this profile is invalid. ${REFRESH_MESSAGE}\n${e}`,
`The SSO session associated with this profile is invalid. ${REFRESH_MESSAGE}`,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to have this information when receiving a bug report, for example.

It could say "file not found: ~/.aws/sso/cache/abcde.json". And we would then know the user either didn't do SSO sign in or the filename hash used was wrong

It could alternatively be a JSON parse error (less likely).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up removing the underlying error, but changed the wording of the thrown error to hint at the file not found issue.

kuhe and others added 4 commits November 10, 2022 16:32
Co-authored-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>
Co-authored-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>
Co-authored-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>
@kuhe kuhe merged commit e4ea7d0 into aws:main Nov 11, 2022
@kuhe kuhe deleted the feat/sso branch November 11, 2022 00:50
const profile = profiles[profileName];

if (profile.sso_session) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kuhe I am getting the error Cannot read properties of undefined (reading 'sso_session') with code running inside an ECS task after this change. I believe this change here does not account for when profile is undefined.

I believe this should be:

if (profile && profile.sso_session) {

Previously this function would have exited with if (!isSsoProfile(profile)) below, since that checks for undefined.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 I'm getting the same error Cannot read properties of undefined (reading 'sso_session') when running inside AWS EKS. (Version 3.209.0)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 getting same error when running inside AWS Elasticbeanstalk (Version 3.209.0):

Nov 13 22:30:24 ip-10-0-2-9 web: TypeError: Cannot read properties of undefined (reading 'sso_session')
Nov 13 22:30:24 ip-10-0-2-9 web: at /var/app/current/node_modules/@aws-sdk/credential-provider-sso/dist-cjs/fromSSO.js:15:21
Nov 13 22:30:24 ip-10-0-2-9 web: at async coalesceProvider (/var/app/current/node_modules/@aws-sdk/property-provider/dist-cjs/memoize.js:14:24)
Nov 13 22:30:24 ip-10-0-2-9 web: at async SignatureV4.credentialProvider (/var/app/current/node_modules/@aws-sdk/property-provider/dist-cjs/memoize.js:33:24)
Nov 13 22:30:24 ip-10-0-2-9 web: at async SignatureV4.signRequest (/var/app/current/node_modules/@aws-sdk/signature-v4/dist-cjs/SignatureV4.js:86:29)
Nov 13 22:30:24 ip-10-0-2-9 web: at async /var/app/current/node_modules/@aws-sdk/middleware-signing/dist-cjs/middleware.js:16:18
Nov 13 22:30:24 ip-10-0-2-9 web: at async StandardRetryStrategy.retry (/var/app/current/node_modules/@aws-sdk/middleware-retry/dist-cjs/StandardRetryStrategy.js:51:46)
Nov 13 22:30:24 ip-10-0-2-9 web: at async /var/app/current/node_modules/@aws-sdk/middleware-logger/dist-cjs/loggerMiddleware.js:6:22
Nov 13 22:30:24 ip-10-0-2-9 web: at async getAWSSecrets (/var/app/current/packages/server/dist/infrastructure/settings/config.factory.js:104:21)
Nov 13 22:30:24 ip-10-0-2-9 web: at async InstanceWrapper.ConfigAWSFactory [as metatype] (/var/app/current/packages/server/dist/infrastructure/settings/config.factory.js:61:21)
Nov 13 22:30:24 ip-10-0-2-9 web: at async Injector.instantiateClass (/var/app/current/node_modules/@nestjs/core/injector/injector.js:344:37)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Submitted PR to fix #4186

@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants