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

Fix: Only Validate SSO profile configuration when attempting to use SSO credentials #3769

Merged
merged 1 commit into from
Feb 1, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG_PENDING.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,5 @@
### SDK Enhancements

### SDK Bugs
* `aws/session`: Fixed a bug that prevented credentials from being sourced from the environment if the loaded shared config profile contained partial SSO configuration. ([#3769](https://github.com/aws/aws-sdk-go/pull/3769))
* Fixes ([#3768](https://github.com/aws/aws-sdk-go/issues/3768))
10 changes: 7 additions & 3 deletions aws/session/credentials.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ func resolveCredsFromProfile(cfg *aws.Config,
)

case sharedCfg.hasSSOConfiguration():
creds = resolveSSOCredentials(cfg, sharedCfg, handlers)
creds, err = resolveSSOCredentials(cfg, sharedCfg, handlers)

case len(sharedCfg.CredentialProcess) != 0:
// Get credentials from CredentialProcess
Expand Down Expand Up @@ -155,7 +155,11 @@ func resolveCredsFromProfile(cfg *aws.Config,
return creds, nil
}

func resolveSSOCredentials(cfg *aws.Config, sharedCfg sharedConfig, handlers request.Handlers) *credentials.Credentials {
func resolveSSOCredentials(cfg *aws.Config, sharedCfg sharedConfig, handlers request.Handlers) (*credentials.Credentials, error) {
if err := sharedCfg.validateSSOConfiguration(); err != nil {
return nil, err
}

cfgCopy := cfg.Copy()
cfgCopy.Region = &sharedCfg.SSORegion

Expand All @@ -167,7 +171,7 @@ func resolveSSOCredentials(cfg *aws.Config, sharedCfg sharedConfig, handlers req
sharedCfg.SSOAccountID,
sharedCfg.SSORoleName,
sharedCfg.SSOStartURL,
)
), nil
}

// valid credential source values
Expand Down
27 changes: 25 additions & 2 deletions aws/session/credentials_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,22 @@ func TestSharedConfigCredentialSource(t *testing.T) {
"assume_sso_and_static_arn",
},
},
{
name: "invalid sso configuration",
profile: "sso_invalid",
expectedError: fmt.Errorf("profile \"sso_invalid\" is configured to use SSO but is missing required configuration: sso_region, sso_start_url"),
},
{
name: "environment credentials with invalid sso",
profile: "sso_invalid",
expectedAccessKey: "access_key",
expectedSecretKey: "secret_key",
init: func() (func(), error) {
os.Setenv("AWS_ACCESS_KEY", "access_key")
os.Setenv("AWS_SECRET_KEY", "secret_key")
return func() {}, nil
},
},
}

for i, c := range cases {
Expand Down Expand Up @@ -308,8 +324,15 @@ func TestSharedConfigCredentialSource(t *testing.T) {
Handlers: handlers,
EC2IMDSEndpoint: c.sessOptEC2IMDSEndpoint,
})
if e, a := c.expectedError, err; e != a {
t.Fatalf("expected %v, but received %v", e, a)

if c.expectedError != nil {
var errStr string
if err != nil {
errStr = err.Error()
}
if e, a := c.expectedError.Error(), errStr; !strings.Contains(a, e) {
t.Fatalf("expected %v, but received %v", e, a)
}
}

if c.expectedError != nil {
Expand Down
12 changes: 6 additions & 6 deletions aws/session/shared_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ const (

// sharedConfig represents the configuration fields of the SDK config files.
type sharedConfig struct {
Profile string

// Credentials values from the config file. Both aws_access_key_id and
// aws_secret_access_key must be provided together in the same file to be
// considered valid. The values will be ignored if not a complete group.
Expand Down Expand Up @@ -201,6 +203,8 @@ func loadSharedConfigIniFiles(filenames []string) ([]sharedConfigFile, error) {
}

func (cfg *sharedConfig) setFromIniFiles(profiles map[string]struct{}, profile string, files []sharedConfigFile, exOpts bool) error {
cfg.Profile = profile

// Trim files from the list that don't exist.
var skippedFiles int
var profileNotFoundErr error
Expand Down Expand Up @@ -365,10 +369,6 @@ func (cfg *sharedConfig) validateCredentialsConfig(profile string) error {
return err
}

if err := cfg.validateSSOConfiguration(profile); err != nil {
return err
}

return nil
}

Expand Down Expand Up @@ -409,7 +409,7 @@ func (cfg *sharedConfig) validateCredentialType() error {
return nil
}

func (cfg *sharedConfig) validateSSOConfiguration(profile string) error {
func (cfg *sharedConfig) validateSSOConfiguration() error {
if !cfg.hasSSOConfiguration() {
return nil
}
Expand All @@ -433,7 +433,7 @@ func (cfg *sharedConfig) validateSSOConfiguration(profile string) error {

if len(missing) > 0 {
return fmt.Errorf("profile %q is configured to use SSO but is missing required configuration: %s",
profile, strings.Join(missing, ", "))
cfg.Profile, strings.Join(missing, ", "))
}

return nil
Expand Down
39 changes: 30 additions & 9 deletions aws/session/shared_config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,19 +30,23 @@ func TestLoadSharedConfig(t *testing.T) {
{
Filenames: []string{"file_not_exists"},
Profile: "default",
Expected: sharedConfig{},
Expected: sharedConfig{
Profile: "default",
},
},
{
Filenames: []string{testConfigFilename},
Expected: sharedConfig{
Region: "default_region",
Profile: "default",
Region: "default_region",
},
},
{
Filenames: []string{testConfigOtherFilename, testConfigFilename},
Profile: "config_file_load_order",
Expected: sharedConfig{
Region: "shared_config_region",
Profile: "config_file_load_order",
Region: "shared_config_region",
Creds: credentials.Value{
AccessKeyID: "shared_config_akid",
SecretAccessKey: "shared_config_secret",
Expand All @@ -54,7 +58,8 @@ func TestLoadSharedConfig(t *testing.T) {
Filenames: []string{testConfigFilename, testConfigOtherFilename},
Profile: "config_file_load_order",
Expected: sharedConfig{
Region: "shared_config_other_region",
Profile: "config_file_load_order",
Region: "shared_config_other_region",
Creds: credentials.Value{
AccessKeyID: "shared_config_other_akid",
SecretAccessKey: "shared_config_other_secret",
Expand All @@ -66,9 +71,11 @@ func TestLoadSharedConfig(t *testing.T) {
Filenames: []string{testConfigOtherFilename, testConfigFilename},
Profile: "assume_role",
Expected: sharedConfig{
Profile: "assume_role",
RoleARN: "assume_role_role_arn",
SourceProfileName: "complete_creds",
SourceProfile: &sharedConfig{
Profile: "complete_creds",
Creds: credentials.Value{
AccessKeyID: "complete_creds_akid",
SecretAccessKey: "complete_creds_secret",
Expand All @@ -81,6 +88,7 @@ func TestLoadSharedConfig(t *testing.T) {
Filenames: []string{testConfigOtherFilename, testConfigFilename},
Profile: "assume_role_invalid_source_profile",
Expected: sharedConfig{
Profile: "assume_role_invalid_source_profile",
RoleARN: "assume_role_invalid_source_profile_role_arn",
SourceProfileName: "profile_not_exists",
},
Expand All @@ -93,11 +101,13 @@ func TestLoadSharedConfig(t *testing.T) {
Filenames: []string{testConfigOtherFilename, testConfigFilename},
Profile: "assume_role_w_creds",
Expected: sharedConfig{
Profile: "assume_role_w_creds",
RoleARN: "assume_role_w_creds_role_arn",
ExternalID: "1234",
RoleSessionName: "assume_role_w_creds_session_name",
SourceProfileName: "assume_role_w_creds",
SourceProfile: &sharedConfig{
Profile: "assume_role_w_creds",
Creds: credentials.Value{
AccessKeyID: "assume_role_w_creds_akid",
SecretAccessKey: "assume_role_w_creds_secret",
Expand All @@ -110,6 +120,7 @@ func TestLoadSharedConfig(t *testing.T) {
Filenames: []string{testConfigOtherFilename, testConfigFilename},
Profile: "assume_role_wo_creds",
Expected: sharedConfig{
Profile: "assume_role_wo_creds",
RoleARN: "assume_role_wo_creds_role_arn",
SourceProfileName: "assume_role_wo_creds",
},
Expand All @@ -127,6 +138,7 @@ func TestLoadSharedConfig(t *testing.T) {
Filenames: []string{testConfigOtherFilename, testConfigFilename},
Profile: "assume_role_with_credential_source",
Expected: sharedConfig{
Profile: "assume_role_with_credential_source",
RoleARN: "assume_role_with_credential_source_role_arn",
CredentialSource: credSourceEc2Metadata,
},
Expand All @@ -135,12 +147,15 @@ func TestLoadSharedConfig(t *testing.T) {
Filenames: []string{testConfigOtherFilename, testConfigFilename},
Profile: "multiple_assume_role",
Expected: sharedConfig{
Profile: "multiple_assume_role",
RoleARN: "multiple_assume_role_role_arn",
SourceProfileName: "assume_role",
SourceProfile: &sharedConfig{
Profile: "assume_role",
RoleARN: "assume_role_role_arn",
SourceProfileName: "complete_creds",
SourceProfile: &sharedConfig{
Profile: "complete_creds",
Creds: credentials.Value{
AccessKeyID: "complete_creds_akid",
SecretAccessKey: "complete_creds_secret",
Expand All @@ -154,9 +169,11 @@ func TestLoadSharedConfig(t *testing.T) {
Filenames: []string{testConfigOtherFilename, testConfigFilename},
Profile: "multiple_assume_role_with_credential_source",
Expected: sharedConfig{
Profile: "multiple_assume_role_with_credential_source",
RoleARN: "multiple_assume_role_with_credential_source_role_arn",
SourceProfileName: "assume_role_with_credential_source",
SourceProfile: &sharedConfig{
Profile: "assume_role_with_credential_source",
RoleARN: "assume_role_with_credential_source_role_arn",
CredentialSource: credSourceEc2Metadata,
},
Expand All @@ -166,12 +183,15 @@ func TestLoadSharedConfig(t *testing.T) {
Filenames: []string{testConfigOtherFilename, testConfigFilename},
Profile: "multiple_assume_role_with_credential_source2",
Expected: sharedConfig{
Profile: "multiple_assume_role_with_credential_source2",
RoleARN: "multiple_assume_role_with_credential_source2_role_arn",
SourceProfileName: "multiple_assume_role_with_credential_source",
SourceProfile: &sharedConfig{
Profile: "multiple_assume_role_with_credential_source",
RoleARN: "multiple_assume_role_with_credential_source_role_arn",
SourceProfileName: "assume_role_with_credential_source",
SourceProfile: &sharedConfig{
Profile: "assume_role_with_credential_source",
RoleARN: "assume_role_with_credential_source_role_arn",
CredentialSource: credSourceEc2Metadata,
},
Expand All @@ -182,20 +202,23 @@ func TestLoadSharedConfig(t *testing.T) {
Filenames: []string{testConfigFilename},
Profile: "with_sts_regional",
Expected: sharedConfig{
Profile: "with_sts_regional",
STSRegionalEndpoint: endpoints.RegionalSTSEndpoint,
},
},
{
Filenames: []string{testConfigFilename},
Profile: "with_s3_us_east_1_regional",
Expected: sharedConfig{
Profile: "with_s3_us_east_1_regional",
S3UsEast1RegionalEndpoint: endpoints.RegionalS3UsEast1Endpoint,
},
},
{
Filenames: []string{testConfigFilename},
Profile: "sso_creds",
Expected: sharedConfig{
Profile: "sso_creds",
SSOAccountID: "012345678901",
SSORegion: "us-west-2",
SSORoleName: "TestRole",
Expand All @@ -206,25 +229,23 @@ func TestLoadSharedConfig(t *testing.T) {
Filenames: []string{testConfigFilename},
Profile: "source_sso_creds",
Expected: sharedConfig{
Profile: "source_sso_creds",
RoleARN: "source_sso_creds_arn",
SourceProfileName: "sso_creds",
SourceProfile: &sharedConfig{
Profile: "sso_creds",
SSOAccountID: "012345678901",
SSORegion: "us-west-2",
SSORoleName: "TestRole",
SSOStartURL: "https://127.0.0.1/start",
},
},
},
{
Filenames: []string{testConfigFilename},
Profile: "invalid_sso_creds",
Err: fmt.Errorf("profile \"invalid_sso_creds\" is configured to use SSO but is missing required configuration: sso_region, sso_role_name, sso_start_url"),
},
{
Filenames: []string{testConfigFilename},
Profile: "sso_and_static",
Expected: sharedConfig{
Profile: "sso_and_static",
Creds: credentials.Value{
AccessKeyID: "sso_and_static_akid",
SecretAccessKey: "sso_and_static_secret",
Expand Down
5 changes: 5 additions & 0 deletions aws/session/testdata/credential_source_config
Original file line number Diff line number Diff line change
Expand Up @@ -52,3 +52,8 @@ sso_account_id = 012345678901
sso_region = us-west-2
sso_role_name = TestRole
sso_start_url = https://THIS_SHOULD_NOT_BE_IN_TESTDATA_CACHE/start

[profile sso_invalid]
sso_account_id = 012345678901
sso_role_name = TestRole