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

[peterrsongg]

Description

I reordered the list of credential profile types based on the credential-provider-chain SEP. This doesn't affect the functionality of the PR it was just for logical reordering.

This PR allows a setup like the following to work correctly:

[profile test-profile]
role_arn=arn:aws:iam::<account_id>:role/s3-read-only-role
source_profile=source-profile
role_session_name=testing
sso_session=my-sso-2
region=us-east-1
[profile source-profile]
sso_session = my-sso-2
sso_account_id = <account_id>
sso_role_name = AdministratorAccess
region = us-east-1
[sso-session my-sso-2]
sso_start_url = https://***
sso_region = us-east-1
sso_registration_scopes = sso:account:access

where test-profile will be looked at as an assume role profile instead of an sso profile like it was previously. This is the correct behavior as the SEP states:

If either the sso_account_id or sso_role_name configurations values are present the profile MUST be resolved by the SSO credential provider.

We weren't following the SEP and were assuming that if any sso property is set then it is an SSO profile. Now we only resolve credentials via SSO if the profile contains sso_account_id or sso_role_name.

This then allows a user to login via SSO in powershell or the cli

Invoke-AWSSSOLogin -profileName test-profie

which will start the SSOLogin process and then assume the role arn:aws:iam::<account_id>:role/s3-read-only-role specified in the original profile.

And then the user can call

aws sts-get-caller-identity --profile test-profile

{
    "UserId": "****:testing",
    "Account": "<account_id>",
    "Arn": "arn:aws:sts::<account_id>:assumed-role/s3-read-only-role/testing"
}

I also added the sso properties (not including sso_account_id and sso_role_name) to the other assume role/ credential source credential profile type in accordance with the credentials-provider-chain SEP.

Motivation and Context

internal ticket: dotnet-8049

Testing

DRY_RUN-ed9ce705-b59d-4b7a-847b-ae15c0fbceb4

Screenshots (if appropriate)

The following console app works

Amazon.AWSConfigs.LoggingConfig.LogResponses = Amazon.ResponseLoggingOption.Always;
Amazon.AWSConfigs.LoggingConfig.LogTo = Amazon.LoggingOptions.Console;
Amazon.AWSConfigs.LoggingConfig.LogMetrics = true;
var profileName = "test-profile";
Environment.SetEnvironmentVariable("AWS_PROFILE", profileName);
var config = new AmazonSecurityTokenServiceConfig
{
    RegionEndpoint = RegionEndpoint.USEast1
};
var s3Config = new AmazonS3Config
{
    RegionEndpoint = RegionEndpoint.USEast1
};
using (var stsClient = new AmazonSecurityTokenServiceClient(config))
using (var s3Client = new AmazonS3Client(s3Config))
{
    var stsResponse = await stsClient.GetCallerIdentityAsync(new GetCallerIdentityRequest());
    Console.WriteLine("Caller identity is {0}", stsResponse.Arn);
    var s3Response = await s3Client.ListBucketsAsync(new ListBucketsRequest { });
    s3Response.Buckets.ForEach(x => Console.WriteLine(x.BucketName));
//call below will correctly fail because the test-profile only has read-only access.
    await s3Client.PutObjectAsync(new PutObjectRequest
    {
        BucketName = "aws-net-sdk-1625686759",
        Key = "testing123",
        ContentBody = " hello world"
    }).ConfigureAwait(false);
}

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• My code follows the code style of this project
• My change requires a change to the documentation
• I have updated the documentation accordingly
• I have read the README document
• I have added tests to cover my changes
• All new and existing tests passed

License

• I confirm that this pull request can be released under the Apache 2 license

[normj]

It is really hard to review this PR since you reordered the dictionary. Very challenging to see what you changed specific for the source profile fix. In the future I would suggest making the reorder and source profile separate PRs or add comments where you made changes for source profile.

My assumption is in the assume role scenarios you add permutations that include the non-keyed off SSO parameters. Assuming that is the change I'm okay with the PR but I don't have enough time before going on PTO to now compare this reordered list to the SEP.

[ashishdhingra]

@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.

[peterrsongg]

@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.

[peterrsongg]

@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.

[ashishdhingra]

@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.

[peterrsongg]

@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.

[dscpinheiro]

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).

[eddiemcs3]

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.

[ashishdhingra]

@peterrsongg I added my comment.

[peterrsongg]

It is really hard to review this PR since you reordered the dictionary. Very challenging to see what you changed specific for the source profile fix. In the future I would suggest making the reorder and source profile separate PRs or add comments where you made changes for source profile.

My assumption is in the assume role scenarios you add permutations that include the non-keyed off SSO parameters. Assuming that is the change I'm okay with the PR but I don't have enough time before going on PTO to now compare this reordered list to the SEP.

yup i just added permutations with sso properties. sorry for the churn on the diff

[peterrsongg]

I only added sso properties to any profile type that is AssumeRole

[ashishdhingra]

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.

Discussed with the team, precedence change will be addressed later.