Skip to content

Fix: Allow some sso properties to be used in conjunction with source_profile and credential_source #3755

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 17, 2025
Merged

Fix: Allow some sso properties to be used in conjunction with source_profile and credential_source #3755

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 9 additions & 0 deletions generator/.DevConfigs/b24980c4-b6f6-4181-9789-fda85df054a3.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"core": {
"changeLogMessages": [
"[Breaking Change] Allow source_profile to be used in conjunction with sso_session. If the profile specified via source_profile has sso_session, and the sso_session section is correctly configured, credentials will be retrieved from sso. The sso credentials will then be used to assume the role specified in the original profile. Previous behavior was that it assumed the role specified in source_profile, which does not follow the assume role profile chaining pattern."
],
"type": "patch",
"updateMinimum": true
}
}
41 changes: 34 additions & 7 deletions sdk/src/Core/Amazon.Runtime/CredentialManagement/Internal/CredentialProfileTypeDetector.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,43 +101,62 @@ public static class CredentialProfileTypeDetector
{
new HashSet<string> { RoleArn, SourceProfile },
new HashSet<string> { RoleArn, SourceProfile, AwsAccountId },
new HashSet<string> { RoleArn, SourceProfile, SsoSession, SsoRegion, SsoRegistrationScopes, SsoStartUrl },
new HashSet<string> { RoleArn, SourceProfile, AwsAccountId, SsoSession, SsoRegion, SsoRegistrationScopes, SsoStartUrl },
}
},
{
CredentialProfileType.AssumeRoleCredentialSource, new List<HashSet<string>>()
{
new HashSet<string> { RoleArn, CredentialSource },
new HashSet<string> { RoleArn, CredentialSource, AwsAccountId }
new HashSet<string> { RoleArn, CredentialSource, SsoSession, SsoRegion, SsoRegistrationScopes, SsoStartUrl },
new HashSet<string> { RoleArn, CredentialSource, AwsAccountId },
new HashSet<string> { RoleArn, CredentialSource, AwsAccountId, SsoSession, SsoRegion, SsoRegistrationScopes, SsoStartUrl }
}
},
{
CredentialProfileType.AssumeRoleExternal, new List<HashSet<string>>()
{
new HashSet<string> { ExternalID, RoleArn, SourceProfile },
new HashSet<string> { ExternalID, RoleArn, SourceProfile, SsoSession, SsoRegion, SsoRegistrationScopes, SsoStartUrl },
new HashSet<string> { ExternalID, RoleArn, SourceProfile, AwsAccountId },
new HashSet<string> { ExternalID, RoleArn, SourceProfile, AwsAccountId, SsoSession, SsoRegion, SsoRegistrationScopes, SsoStartUrl}
}
},
{
CredentialProfileType.AssumeRoleExternalMFA, new List<HashSet<string>>()
{
new HashSet<string> { ExternalID, RoleArn, SourceProfile, MfaSerial },
new HashSet<string> { ExternalID, RoleArn, SourceProfile, MfaSerial, SsoSession, SsoRegion, SsoRegistrationScopes, SsoStartUrl }
}
},
{ CredentialProfileType.AssumeRoleExternalMFA, new List<HashSet<string>>() { new HashSet<string> { ExternalID, RoleArn, SourceProfile, MfaSerial } } },
{
CredentialProfileType.AssumeRoleWithWebIdentity, new List<HashSet<string>>()
{
new HashSet<string> { RoleArn, WebIdentityTokenFile },
new HashSet<string> { RoleArn, WebIdentityTokenFile, SsoSession, SsoRegion, SsoRegistrationScopes, SsoStartUrl },
new HashSet<string> { RoleArn, WebIdentityTokenFile, CredentialSource },
new HashSet<string> { RoleArn, WebIdentityTokenFile, CredentialSource, SsoSession, SsoRegion, SsoRegistrationScopes, SsoStartUrl },
new HashSet<string> { RoleArn, WebIdentityTokenFile, CredentialSource, AwsAccountId },
new HashSet<string> { RoleArn, WebIdentityTokenFile, CredentialSource, AwsAccountId, SsoSession, SsoRegion, SsoRegistrationScopes, SsoStartUrl },
}
},
{
CredentialProfileType.AssumeRoleWithWebIdentitySessionName, new List<HashSet<string>>()
{
new HashSet<string> { RoleArn, WebIdentityTokenFile, RoleSessionName },
new HashSet<string> { RoleArn, WebIdentityTokenFile, RoleSessionName, AwsAccountId } ,
new HashSet<string> { RoleArn, WebIdentityTokenFile, RoleSessionName, SsoSession, SsoRegion, SsoRegistrationScopes, SsoStartUrl },
new HashSet<string> { RoleArn, WebIdentityTokenFile, RoleSessionName, AwsAccountId },
new HashSet<string> { RoleArn, WebIdentityTokenFile, RoleSessionName, AwsAccountId, SsoSession, SsoRegion, SsoRegistrationScopes, SsoStartUrl } ,
}
},
{
CredentialProfileType.AssumeRoleMFA, new List<HashSet<string>>()
{
new HashSet<string> { MfaSerial, RoleArn, SourceProfile },
new HashSet<string> { MfaSerial, RoleArn, SourceProfile, SsoSession, SsoRegion, SsoRegistrationScopes, SsoStartUrl },
new HashSet<string> { MfaSerial, RoleArn, SourceProfile, AwsAccountId },
new HashSet<string> { MfaSerial, RoleArn, SourceProfile, AwsAccountId, SsoSession, SsoRegion, SsoRegistrationScopes, SsoStartUrl },
}
},
{ CredentialProfileType.Basic, new List<HashSet<string>>()
Expand Down Expand Up @@ -165,36 +184,46 @@ public static class CredentialProfileTypeDetector
CredentialProfileType.AssumeRoleSessionName, new List<HashSet<string>>()
{
new HashSet<string> { RoleArn, SourceProfile, RoleSessionName },
new HashSet<string> { RoleArn, SourceProfile, RoleSessionName, SsoSession, SsoRegion, SsoRegistrationScopes, SsoStartUrl},
new HashSet<string> { RoleArn, SourceProfile, RoleSessionName, AwsAccountId },
new HashSet<string> { RoleArn, SourceProfile, RoleSessionName, AwsAccountId, SsoSession, SsoRegion, SsoRegistrationScopes, SsoStartUrl },
}
},
{
CredentialProfileType.AssumeRoleCredentialSourceSessionName, new List<HashSet<string>>()
{
new HashSet<string> { RoleArn, CredentialSource, RoleSessionName },
new HashSet<string> { RoleArn, CredentialSource, RoleSessionName, SsoSession, SsoRegion, SsoRegistrationScopes, SsoStartUrl },
new HashSet<string> { RoleArn, CredentialSource, RoleSessionName, AwsAccountId},
new HashSet<string> { RoleArn, CredentialSource, RoleSessionName, AwsAccountId, SsoSession, SsoRegion, SsoRegistrationScopes, SsoStartUrl },
}
},
{
CredentialProfileType.AssumeRoleExternalSessionName, new List<HashSet<string>>()
{
new HashSet<string> { ExternalID, RoleArn, SourceProfile, RoleSessionName },
new HashSet<string> { ExternalID, RoleArn, SourceProfile, RoleSessionName, SsoSession, SsoRegion, SsoRegistrationScopes, SsoStartUrl },
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm unsure if mixing SSO properties with other assume role credential type is a scalable solution. If we ignore SSO credentials for a while, could we mix other credential type with assume role?

Or May be we should document the possible new design with code comments.

new HashSet<string> { ExternalID, RoleArn, SourceProfile, RoleSessionName, AwsAccountId },
new HashSet<string> { ExternalID, RoleArn, SourceProfile, RoleSessionName, AwsAccountId, SsoSession, SsoRegion, SsoRegistrationScopes, SsoStartUrl },
}
},
{
CredentialProfileType.AssumeRoleExternalMFASessionName, new List<HashSet<string>>()
{
new HashSet<string> { ExternalID, MfaSerial, RoleArn, SourceProfile, RoleSessionName },
new HashSet<string> { ExternalID, MfaSerial, RoleArn, SourceProfile, RoleSessionName, SsoSession, SsoRegion, SsoRegistrationScopes, SsoStartUrl },
new HashSet<string> { ExternalID, MfaSerial, RoleArn, SourceProfile, RoleSessionName, AwsAccountId },
new HashSet<string> { ExternalID, MfaSerial, RoleArn, SourceProfile, RoleSessionName, AwsAccountId, SsoSession, SsoRegion, SsoRegistrationScopes, SsoStartUrl },
}
},
{ CredentialProfileType.SSO, new List<HashSet<string>>() { new HashSet<string> { SsoAccountId, SsoRegion, SsoRegistrationScopes, SsoRoleName, SsoStartUrl, SsoSession } } },
{
CredentialProfileType.AssumeRoleMFASessionName, new List<HashSet<string>>()
{
new HashSet<string> { MfaSerial, RoleArn, SourceProfile, RoleSessionName },
new HashSet<string> { MfaSerial, RoleArn, SourceProfile, RoleSessionName, SsoSession, SsoRegion, SsoRegistrationScopes, SsoStartUrl },
new HashSet<string> { MfaSerial, RoleArn, SourceProfile, RoleSessionName, AwsAccountId },
new HashSet<string> { MfaSerial, RoleArn, SourceProfile, RoleSessionName, AwsAccountId, SsoSession, SsoRegion, SsoRegistrationScopes, SsoStartUrl }
}
},
};
Expand Down Expand Up @@ -238,11 +267,9 @@ public static string GetUserFriendlyCredentialType(CredentialProfileType? profil

HashSet<string> propertyNames = GetPropertyNames(profileOptions);

// Spec: If one or more of the SSO properties is present, the profile MUST be resolved by the SSO credential provider.
if (propertyNames.Any(propertyName => SsoProperties.Contains(propertyName)))
{
//SPEC: if sso_account_id or sso_role_name exist credentials MUST be resolved by the sso credential provider.
Copy link
Contributor

@ashishdhingra ashishdhingra Apr 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@peterrsongg I'm not sure if this is entirely true based on precedence order SEP. If both SSO and assume role properties are present, then assume role takes precedence. Please advise if you have tested profile having source_profile along with assume role and sso_account_id properties? Per your comment SSO would take precedence. But in this case it should be treated as SSO. (Please check behavior in AWS CLI). Kindly refer initial thread I started in AWS-SDK channel (please find link to it in out team channel). There SSO SEP owner agreed that the SSO SEP should explicitly call out only if other provider properties are not present.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ashishdhingra I think we need to figure out the source of truth. In the same SEP where it says if sso_session is present it MUST be resolved via SSO it also says that if sso_account_id or sso_role_name is present then it MUST be resolved via SSO. There is no mention of the search order precedence. I feel like we may have jumped the gun here. It feels like i'm making the change before the SEP has even been updated. What if during sep review they change the behavior again and we ship already?

But also,

If we make the change to the SEP which says we MUST resolve SSO if certain properties are present but only if there isn't a higher order precedence profile type, then that is a BIG runtime breaking change in my opinion which we should not implement. There would be a whole lot of tests that would break because we would return a different set of credentials. For example this test would fail because according to the search order precedence SEP a profile with access-key and secret-access-key takes precedence over SSO. That imo is too big of a runtime breaking change. Before my change ANY sso property meant sso, now we are relaxing that to only sso_account_id or sso_role_name. But if we relax it even further and say sso_account_id, sso_role_name = SSO but only if there is no higher search order precedence profile then that affects more than just assume role profiles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ashishdhingra I tested with the profile setup from the internal ticket. It's in the description. It correctly assumes the role because the profile doesn't contain sso_account_id. If sso_account_id was present it wouldn't assume the role.

Copy link
Contributor

Choose a reason for hiding this comment

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

@peterrsongg Since we need not support legacy SSO provider per precedence SEP, we should be able to remove initial check for selective SSO properties. We should be able to keep existing order in that dictionary where SSO is the last one.

May be in that loop, we can check if the current element in loop is SSO and then use IsSubsetOf without breaking existing credential providers.

Yes, SSO SEP should explicitly mention about if any required properties for provider of higher precedence is found in SSO profile. Perhaps, this is the reason it's not implemented consistently across SDKs. You check with AWS CLI and Kotlin oncall since they marked customers scenario as PASS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ashishdhingra But what I'm saying is supporting the precedence sep over the sso provider sep is a pretty big runtime breaking change that we should discuss more as a team, and something we should only do once it is confirmed in the SEP. If we do that, any flavor of AssumeRoleAWSCredentials, BasicAwsCredentials, and SessionAwsCredentials would take precedence over SSO. That is a lot of credential providers that are all of a sudden being used over SSO. The initial scope of this task, and PR was just for AssumeRole. This would add Basic and Session to the mix as well. I'm not blaming you, I'm just saying, this is a one-way door at this point being so close to V4, and I'm not comfortable making that change unilaterally without team discussion.

May be in that loop, we can check if the current element in loop is SSO and then use IsSubsetOf without breaking existing credential providers.

We would still need to add the sso properties to the type property dictionary though, because if for example
you had a set of properties = { "ExternalID", "MfaSerial", "RoleArn", "SourceProfile", "RoleSessionName", "SsoSession"}, and now we use IsSubsetOf, the list we're matching against wouldn't have SSO properties in it, and it wouldn't resolve properly. But even if we DO add sso properties and use IsSubsetOf then we have the problem that we would resolve a different profile type.

{ "ExternalID", "MfaSerial", "RoleArn", "SourceProfile", "RoleSessionName" }

would've resolved AssumeRoleExternalMFA but now that we do IsSubsetOf it would resolve

AssumeRoleExternalSessionName which breaks unit tests. It would still use the same credential provider but our test coverage here isn't all that great, and it would require lots of testing to make sure we don't break something. Initially I did try what you had suggested, but it broke a lot of unit tests so I stopped.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Peter that Since we need not support legacy SSO provider per precedence SEP is too much of a breaking change at this point (when GA is really close).

On this PR, we should focus on fixing the inconsistency .NET has with the CLI for the specific report we got. Anything other than that can wait (and as you two already commented likely require coordination with the other SDKs).

Copy link

Choose a reason for hiding this comment

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

I agree with both @peterrsongg and @dscpinheiro to move forward fixing our inconsistency and we will need to revisit the precedence SEP at a later time.

if (propertyNames.Contains(SsoAccountId) || propertyNames.Contains(SsoRoleName))
return CredentialProfileType.SSO;
}

// brute force algorithm - but it's a very small set
foreach (var pair in TypePropertyDictionary)
Expand Down
2 changes: 1 addition & 1 deletion sdk/test/UnitTests/Custom/Runtime/Credentials/AWSCredentialsFactoryTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,7 +493,7 @@ public void GetSsoCredentialsWithMissingFields()

AssertExtensions.ExpectException(() =>
AWSCredentialsFactory.GetAWSCredentials(SsoProfileMissingFields, ProfileStore),
typeof(ArgumentNullException));
typeof(InvalidDataException));
}

[TestMethod]
Expand Down