Add sso session and token provider support #4885

wty-Bryant · 2023-06-15T20:11:51Z

Update sso credential provider logic to support both sso token provider and legacy sso config.
The manual test of sso-session section support in shared config file follows steps below:

running the sso login flow with the AWS CLI
using the access token generated, calling S3 List Buckets on the configured AWS account
when the access token expires, re-calling S3 List buckets and see it succeed with an updated expiration in the SSO cached token

This PR will resolve #4649 with prvious PRs on the same branch.

# Conflicts: # CHANGELOG_PENDING.md

…rom main branch

# Conflicts: # CHANGELOG_PENDING.md

Add sso token provider

sync main to feat-sso-session

* Merge logic of resolving sso section in shared config file * Modify and Merge shared config unit test case * Modify and Merge logic of shared config loaded from files --------- Co-authored-by: Tianyi Wang <wty@amazon.com>

sync main into current branch

sync main branch into current branch

sync main into branch

* Update and Merge logic of sso credential provider to support token provider * Add and Merge sso credential provider unit test data * Modify and Merge sso credential provider's token provider field and unit test * Modify and Merge sso credential provider's token provider and unit test * Modify and Merge sso credential provider's unit test --------- Co-authored-by: Tianyi Wang <wty@amazon.com>

sync main into current branch

aajtodd

Looks good. I pulled it down and played with it locally and it works as expected.

Housekeeping notes:

Be sure to squash when you merge and update the commit description and comment to be something meaningful (GH will default to all commit messages which is not useful).
Remember to get a second reviewer before merging.

aajtodd · 2023-06-20T12:18:30Z

CHANGELOG_PENDING.md

@@ -3,3 +3,5 @@
 ### SDK Enhancements

 ### SDK Bugs
+* `aws/credentials/ssocreds`: Implement SSO token provider to support for `sso-session` in AWS shared config.


nit: "Implement SSO token provider to support for ..."

sync main into current branch

isaiahvita · 2023-07-05T15:25:04Z

aws/auth/bearer/token.go

+	"time"
+)
+
+type Token struct {


nit: did your IDE not complain about no comment/documentation here? this is a public type, so it should have some documentation here.

aws/credentials/ssocreds/provider_test.go

aws/credentials/ssocreds/testdata/custom_cached_token.json

fitz-res · 2023-07-05T19:08:30Z

aws/auth/bearer/token.go

+}
+
+// Expired returns if the token's Expires time is before or equal to the time
+// provided. If CanExpires is false, Expired will always return false.


Suggested change

// provided. If CanExpires is false, Expired will always return false.

// provided. If CanExpire is false, Expired will always return false.

isaiahvita · 2023-07-05T20:45:49Z

aws/credentials/ssocreds/provider_test.go

+
+	return p.Response()
+}
+
 func (m mockClient) GetRoleCredentialsWithContext(ctx aws.Context, params *sso.GetRoleCredentialsInput, _ ...request.Option) (*sso.GetRoleCredentialsOutput, error) {


i know this is not part of your changeset. so this is just a comment for the future. but i dont think we should be including the Go testing framework object in the mocked client.

when validating the errors in the mocked client, it shouldnt directly invoke the testing object framework, but we should actually mock the returned errors, and then catch those in the test case execution

isaiahvita · 2023-07-05T21:03:41Z

aws/credentials/ssocreds/provider_test.go

+					return bearer.Token{}, fmt.Errorf("mock token provider return error")
+				},
+			},
+			AccountID:   "012345678901",


nit: whats the purpose of AccountID Region RoleName, and StartURL? removing them doesnt change the outcome of the test, and the mocked token provider doesnt seem to be reacting to them at all.

StartURL will be used to get legacy ssoToken while token provider is unavailable, AccountID and RoleName will be checked with expected value in mock SSO Client while calling GetRoleCredentialsWithContext. But some error case won't reach that step so I will just remove their fields. Additionally Region is not used in all cases so I will remove it from test case struct.

isaiahvita · 2023-07-05T22:05:41Z

aws/credentials/ssocreds/sso_cached_token.go

+	UnknownFields map[string]interface{} `json:"-"`
+}
+
+func (t cachedToken) MarshalJSON() ([]byte, error) {


can you add some documentation for this function and func (t *cachedToken) UnmarshalJSON on why we need a custom marshaller/unmarshaller? we need documentation on public functions, and its not immediately obvious why the standard Go marshaller doesnt suffice.

i see your added documentation now. and i think i get it. but the documentation as is isnt very clear. how about this:

MarshalJSON provides custom marshalling because the standard library Go marshaller ignores unknown/unspecified fields when marshalling from a struct. https://pkg.go.dev/encoding/json#Marshal This function adds some extra validation to the known fields and captures unknown fields.

fix this and UnmarshalJSON and then ship it.

isaiahvita · 2023-07-05T22:09:53Z

aws/session/shared_config_test.go

@@ -507,6 +528,15 @@ func TestLoadSharedConfigFromFile(t *testing.T) {
 				S3UseARNRegion: true,
 			},
 		},
+		{
+			Profile: "sso-session-success",


whats the difference between this test and the other test above that tests sso-session-success?

isaiahvita · 2023-07-06T16:24:50Z

aws/credentials/ssocreds/sso_cached_token.go

+	UnknownFields map[string]interface{} `json:"-"`
+}
+
+func (t cachedToken) MarshalJSON() ([]byte, error) {


i see your added documentation now. and i think i get it. but the documentation as is isnt very clear. how about this:

MarshalJSON provides custom marshalling because the standard library Go marshaller ignores unknown/unspecified fields when marshalling from a struct. https://pkg.go.dev/encoding/json#Marshal This function adds some extra validation to the known fields and captures unknown fields.

fix this and UnmarshalJSON and then ship it.

isaiahvita · 2023-07-06T16:26:50Z

aws/credentials/ssocreds/sso_cached_token.go

+	fields[key] = value
+}
+
+// UnmarshalJSON decode cachedToken known/unknown fields from json format


i see your added documentation now. and i think i get it. but the documentation as is isnt very clear. how about this:

UnmarshalJSON provides custom unmarshalling because the standard library Go unmarshaller ignores unknown/unspecified fields when unmarshalling from a struct. https://pkg.go.dev/encoding/json#Unmarshal This function adds some extra validation to the known fields and captures unknown fields.

fix this and MarshalJSON and ship it

Hi, this is a simple change - I have a setup with AWS SSO, specifically `sso_session` ([docs](https://docs.aws.amazon.com/cli/latest/userguide/sso-configure-profile-token.html#:~:text=This%20results%20in%20creating%20the%20sso%2Dsession%20section%20and%20named%20profile%20in%20~/.aws/config%20that%20looks%20like%20the%20following%3A)). It got implemented in the AWS Go SDK fairly recently aws/aws-sdk-go#4885 - [v1.44.298](https://github.com/aws/aws-sdk-go/releases/tag/v1.44.298) Since it's just a patch version upgrade, it was just a simple matter of bumping the version. Thanks

Tianyi Wang and others added 30 commits May 23, 2023 17:56

Squash and Merge sso token provider code and unit test

8855191

Squash and Merge ssocred rfc3339 code location change

3bf12a6

Squash and Merge pending changelog content

9e666ea

Deprecate smithy go dependency and Merge bearer token code

e3644bf

Deprecate smithy go dependency and Merge bearer token code

22968a0

Deprecate go-cmp dependency

6354a34

Merge changed sso token provider error format

3e9df76

Merge branch 'main' into feature-token-provider

df54980

# Conflicts: # CHANGELOG_PENDING.md

Merge modified pending changelog

14a703c

Deleted sms smoke json model that is generated from unknown merging f…

6f16394

…rom main branch

Modify and merge sso token provider logic

d0894cf

Merge branch 'feat-sso-session' into feature-token-provider

d5e1072

# Conflicts: # CHANGELOG_PENDING.md

Modify and Merge changelog entry

45be7cf

Merge branch 'feat-sso-session' into feature-token-provider

7b47ebb

# Conflicts: # CHANGELOG_PENDING.md

Modify and Merge changelog entry

e35aaee

Modify and Merge changelog entry

3154ff0

Modify and Merge changelog entry

3c021ea

Modify and Merge bearer token directory and sso token provider var name

359ee1e

Modify and Merge changelog entry

19a523b

Modify and Merge changelog entry

da6f599

Merge branch 'feat-sso-session' into feature-token-provider

8cf9168

Modify and Merge context used in token provider

8affb62

Modify and Merge ssocreds test tag

2ffc7e2

Modify and Merge ssocreds token provider test tag

6e4eb05

Modify and Merge ssocreds cached token test tag

b2ca51c

Modify and Merge ssocreds cached token test tag

9b2a2a1

Modify and Merge ssocreds token provider test tag

7564841

Modify and Merge ssocreds token provider test tag

1558f1a

Modify and Merge bearer token directory

89c821a

Merge pull request #4853 from aws/feature-token-provider

1077388

Add sso token provider

Tianyi Wang and others added 8 commits May 31, 2023 15:32

Merge branch 'main' into feat-sso-session

b646a31

Merge branch 'main' into feat-sso-session

29a8523

sync main to feat-sso-session

Update shared config logic to resolve sso section (#4868)

3216ed5

* Merge logic of resolving sso section in shared config file * Modify and Merge shared config unit test case * Modify and Merge logic of shared config loaded from files --------- Co-authored-by: Tianyi Wang <wty@amazon.com>

Merge branch 'main' into feat-sso-session

c479d68

sync main into current branch

Merge branch 'main' into feat-sso-session

1f7d087

sync main branch into current branch

Merge branch 'main' into feat-sso-session

62eb32d

sync main into branch

Merge branch 'main' into feat-sso-session

3003a27

sync main into current branch

jim-barber-he mentioned this pull request Jun 15, 2023

Chamber is not working with recommended AWS SSO config that uses sso-session segmentio/chamber#396

Closed

aajtodd approved these changes Jun 20, 2023

View reviewed changes

Tianyi Wang added 2 commits June 20, 2023 10:30

Merge branch 'main' into feat-sso-session

6e4da8f

sync main into current branch

Modify and Merge changelog

9cabea6

wty-Bryant requested review from isaiahvita and removed request for isaiahvita June 20, 2023 18:39

begelundmuller mentioned this pull request Jun 23, 2023

Runtime: Update AWS SDK to fix new SSO support rilldata/rill#2692

Open

gimbo mentioned this pull request Jun 28, 2023

error configuring S3 Backend: no valid credential sources for S3 Backend found. hashicorp/terraform#32448

Closed

ewbankkit mentioned this pull request Jun 30, 2023

[Bug]: Lack of support for sso-session in .aws/config hashicorp/terraform-provider-aws#28263

Closed

sokopro-vile mentioned this pull request Jul 5, 2023

aws-sdk-go doens't support new sso-session in a shared config #4649

Closed

isaiahvita reviewed Jul 5, 2023

View reviewed changes

fitz-res reviewed Jul 5, 2023

View reviewed changes

isaiahvita reviewed Jul 5, 2023

View reviewed changes

Tianyi Wang added 2 commits July 6, 2023 11:08

Merge branch 'main' into feat-sso-session

ba18cd2

Modify and Merge ssoCredProvider test cases and some doc

d85ea03

isaiahvita approved these changes Jul 6, 2023

View reviewed changes

Modify and Merge sso cached token doc

3ab0945

wty-Bryant merged commit d744468 into main Jul 6, 2023

wty-Bryant deleted the feat-sso-session branch July 6, 2023 19:44

matan129 mentioned this pull request Dec 6, 2023

vendor: Update AWS SDK peak/s5cmd#684

Merged

RanVaknin mentioned this pull request Mar 8, 2024

sso-session config file section doen't work. #5153

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sso session and token provider support #4885

Add sso session and token provider support #4885

wty-Bryant commented Jun 15, 2023

aajtodd left a comment

aajtodd Jun 20, 2023

isaiahvita Jul 5, 2023

fitz-res Jul 5, 2023

isaiahvita Jul 5, 2023

isaiahvita Jul 5, 2023

wty-Bryant Jul 6, 2023 •

edited

Loading

isaiahvita Jul 5, 2023

isaiahvita Jul 6, 2023

isaiahvita Jul 5, 2023

isaiahvita Jul 6, 2023

isaiahvita Jul 6, 2023 •

edited

Loading

	// provided. If CanExpires is false, Expired will always return false.
	// provided. If CanExpire is false, Expired will always return false.

Add sso session and token provider support #4885

Add sso session and token provider support #4885

Conversation

wty-Bryant commented Jun 15, 2023

aajtodd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wty-Bryant Jul 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaiahvita Jul 6, 2023 • edited Loading

Choose a reason for hiding this comment

wty-Bryant Jul 6, 2023 •

edited

Loading

isaiahvita Jul 6, 2023 •

edited

Loading