Skip to content

Conversation

awschristou
Copy link
Contributor

Description

This change adds support for AWS SSO related Credentials Profiles to resolve credentials.

I implemented this in a similar manner to MFA credentials support.
In this change, SSO Support was not implemented for .NET Framework 3.5, or netstandard 1.3.

Motivation and Context

This change opens the door for applications being built with the AWS SDK for .NET to work with SSO based credentials.

Testing

Tests have been added with the code

I also did the following locally to verify the changes:

  • added an AWS SSO based profile to my shared credentials file
  • make a throwaway test locally that loads this profile, and resolves its credentials (both sync and async)
  • verified that the SSO login flow is followed, and credentials are returned
  • verified that cached SSO tokens are also honored

Screenshots (if appropriate)

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

@@ -0,0 +1,46 @@
/*
* Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved.
Copy link
Member

Choose a reason for hiding this comment

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

Don't put a year on the copyrights. Otherwise it just gets very out of date. This should be removed in any of the new files added.

catch (Exception e)
#pragma warning restore CA1031 // Do not catch general exception types
{
_logger.Error(e, "Error saving SSO Token Cache");
Copy link
Member

Choose a reason for hiding this comment

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

Instead of saying "Error" can we say "Warning" and they say what the consequences are for not having the token saved. I assume this means the credentials will not be reused by other processes.

Copy link
Contributor Author

@awschristou awschristou Dec 10, 2020

Choose a reason for hiding this comment

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

I've added the consequences but left the log level as error so that the exception is logged

/// <returns>SHA1 hash for <paramref name="text"/></returns>
private static string GenerateSha1Hash(string text)
{
using (var sha1 = new SHA1Managed())
Copy link
Member

Choose a reason for hiding this comment

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

Can you move the direct usage of SHA1Managed into Amazon.Util.CryptoUtil? In the past we have had to abstract which implementation of the crypto algorithms to use. Currently now that we just support .NET Framework and Standard they use the same implementation but I would like to maintain the abstraction.

catch (Exception e)
{
logger.Error(e, "Unexpected exception while polling for SSO Token.");
Console.WriteLine($"Poll Exception: {e.Message}");
Copy link
Member

Choose a reason for hiding this comment

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

Don't write to the Console

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I forgot to remove those

catch (Exception e)
{
logger.Error(e, "Unexpected exception while polling for SSO Token.");
Console.WriteLine($"Poll Exception: {e.Message}");
Copy link
Member

Choose a reason for hiding this comment

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

Don't write to the console

/// <param name="startUrl">The configured sso_start_url for the profile that credentials are being resolved for.</param>
public SsoTokenCache(string startUrl)
: this(
Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), ".aws", "sso", "cache"),
Copy link
Member

Choose a reason for hiding this comment

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

Because we have sometimes had trouble with Environment.GetFolderPath(Environment.SpecialFolder.UserProfile) throwing an exception if running under a system user. Can we call a separate utility method that resolves the default cache path and it Environment.GetFolderPath(Environment.SpecialFolder.UserProfile) throws an exception return null. Then through the cache if the path is null either switched to not doing any caching or use an in memory cache.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The spec prohibits keeping the token in cache, so I will set this class to do no caching when it is unable to resolve a UserProfile

public class SsoToken
{
/// <summary>
/// A base64 encoded string returned by the SSOOIDC service (IAmazonSSOOIDC).
Copy link
Member

Choose a reason for hiding this comment

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

I assume the caller doesn't have to do the base64 encoding, just take the value from IAmazonSSOOIDC and use that. If that is true can we not talk about base64 encoding here as that is SSO implementation detail that is transparent to the user and the SDK.

I call this out because we have had issues with users misreading our docs and thinking they have to base64 a string that is already base 64 encoded. I recognize this is not likely to be called by users since it is in an internal namespace but it could get copied and pasted somewhere.

@normj
Copy link
Member

normj commented Dec 14, 2020

I just have the one last question about multiple requests triggering the SSO flow as the same time. Lets ask the Python team what they are doing so we are consistent with them.

Copy link
Member

@ashovlin ashovlin left a comment

Choose a reason for hiding this comment

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

Overall question - why the .NET Framework 3.5 restriction? Fine with .NET Standard 1.3 given https://aws.amazon.com/blogs/developer/net-standard-1-3-in-aws-sdk-for-net-is-now-in-maintenance-mode/, but we don't have any current plans to deprecate net35. Discussed briefly with Norm and we're ultimately okay with it, but just curious.

@awschristou
Copy link
Contributor Author

why the .NET Framework 3.5 restriction

Some necessary SDK or Crypto calls were not available (I can't recall what it was, but I'll remember to document why next time)

- support not implemented for .NET Framework 3.5.
@normj
Copy link
Member

normj commented Jan 20, 2021

This has been squashed merged and released in version 3.5.2 of AWSSDK.Core

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants