aws / aws-sdk-net-extensions-cognito

An extension library to assist in the Amazon Cognito User Pools authentication process
Apache License 2.0
102 stars 49 forks source link

Unable to create new access token from only refresh token. #69

Closed duaneking closed 3 years ago

duaneking commented 3 years ago

The Readme shows a refresh token scenario that requires that the users username be known in advance so that the users CognitoUser object can be retrieved and its methods thus called.

The code itself in the readme is not functional; the constants "username" etc are clearly placeholders meant to show that a developer should use the correct username for that specific CognitoUser in question, the same as I would presume the other fields are meant to be used for pool id and client id configuration parameters,, but that's not even represented in the signature of the method itself, so at best its a non-working example that's actively misleading, if not outright hostile to users, because it gives them false hope of a working solution.

And in case I was wrong on this, I actually tried to use the code after setting up config values.. and found that attempting to actually use it just results in an error in my tests, in my case a "NotAuthorizedException" in my tests, no doubt because that user doesn't exist in my test user pool as I'm otherwise able to generate a access token log in a user, etc. It does not work, so I'm confused as to why the readme actively misdirects people.

I'm also noticing that it uses DateTime.Now instead of DateTime.UtcNow in a few places, so there is another issue.

Another issue: When an API client hits a 401 and the server challenges the bearer, it now is told that the AccessToken has expired, but the redirect doesn't pass in any other data at all.

Simply put: How do we actually get a new auth token from only a refresh token that according to the spec is everything we should need, if that is all we have, in the call? How can we securely create a new access tokens for a user without knowing the username of the user in advance, using only the refresh token itself as input?

I'm asking for a working solution. Not hypotheticals. Thanks.

ashishdhingra commented 3 years ago

Hi @duaneking,

Good morning.

The example in Readme works fine, it just provides basic guidance on how to use refresh token. Please refer the below working code sample that has capability to use RefreshToken. Kindly note that this is a sample (console) application and you might want to move the secrets to a configuration file. And yes, you should have the correct username that exists in CognitoUserPool to use the RefreshToken. Kindly note that this sample uses Amazon.Extensions.CognitoAuthentication package.

using System;
using System.Threading.Tasks;
using Amazon;
using Amazon.CognitoIdentityProvider;
using Amazon.Extensions.CognitoAuthentication;
using Amazon.Runtime;

namespace CognitoStartWithRefreshTokenAuthAsync
{
    class Program
    {
        private static string userPoolId = "<<userpool_id>>";
        private static string clientId = "<<client_id>>";
        private static string clientSecret = "<<client_secret>>";
        private static RegionEndpoint regionEndpoint = RegionEndpoint.USEast2; // Change the region appropriately. Credentials are loaded from default profile.

        static void Main(string[] args)
        {
            Console.WriteLine("User Name: ");
            string userName = Console.ReadLine();
            while (string.IsNullOrWhiteSpace(userName))
            {
                Console.WriteLine("Please enter a valid User Name.");
                userName = Console.ReadLine();
            }

            Console.WriteLine("Do you have a Refresh Token (Y/N): ");
            char hasRefreshTokenResponse = Convert.ToChar(Console.Read());
            bool hasRefreshToken = (char.ToLower(hasRefreshTokenResponse) == 'y');
            Console.WriteLine();

            Console.WriteLine("Password: ");
            string password = Console.ReadLine();
            while (string.IsNullOrWhiteSpace(password) && !hasRefreshToken)
            {
                Console.WriteLine("Please enter a valid Password.");
                password = Console.ReadLine();
            }

            Console.WriteLine("Existing Refresh Token: ");
            string refreshToken = Console.ReadLine();
            while (string.IsNullOrWhiteSpace(refreshToken) && hasRefreshToken)
            {
                Console.WriteLine("Please enter a valid Refresh Token.");
                refreshToken = Console.ReadLine();
            }

            AuthFlowResponse authFlowResponse = (!string.IsNullOrWhiteSpace(refreshToken) ? GetCredsFromRefreshAsync(userName, refreshToken).GetAwaiter().GetResult() : GetCredentials(userName, password).GetAwaiter().GetResult());
        }

        public static async Task<AuthFlowResponse> GetCredentials(string userName, string password)
        {
            var provider = new AmazonCognitoIdentityProviderClient(new AnonymousAWSCredentials(), FallbackRegionFactory.GetRegionEndpoint());
            var userPool = new CognitoUserPool(userPoolId, clientId, provider, clientSecret);
            var user = new CognitoUser(userName, clientId, userPool, provider, clientSecret);

            AuthFlowResponse authResponse = await user.StartWithSrpAuthAsync(new InitiateSrpAuthRequest()
            {
                Password = password
            }).ConfigureAwait(false);

            while (authResponse.AuthenticationResult == null)
            {
                if (authResponse.ChallengeName == ChallengeNameType.NEW_PASSWORD_REQUIRED)
                {
                    Console.WriteLine("Enter your desired new password: ");
                    string newPassword = Console.ReadLine();

                    authResponse = await user.RespondToNewPasswordRequiredAsync(new RespondToNewPasswordRequiredRequest()
                    {
                        SessionID = authResponse.SessionID,
                        NewPassword = newPassword
                    });
                }
                else if (authResponse.ChallengeName == ChallengeNameType.SMS_MFA)
                {
                    Console.WriteLine("Enter the MFA Code sent to your device: ");
                    string mfaCode = Console.ReadLine();

                    authResponse = await user.RespondToSmsMfaAuthAsync(new RespondToSmsMfaRequest()
                    {
                        SessionID = authResponse.SessionID,
                        MfaCode = mfaCode

                    }).ConfigureAwait(false);
                }
                else
                {
                    Console.WriteLine("Unrecognized authentication challenge.");
                    return null;
                }
            }

            return authResponse;
        }

        public static async Task<AuthFlowResponse> GetCredsFromRefreshAsync(string userName, string refreshToken)
        {
            AmazonCognitoIdentityProviderClient provider = new AmazonCognitoIdentityProviderClient(new AnonymousAWSCredentials(), FallbackRegionFactory.GetRegionEndpoint());
            CognitoUserPool userPool = new CognitoUserPool(userPoolId, clientId, provider, clientSecret);

            CognitoUser user = new CognitoUser(userName, clientId, userPool, provider, clientSecret);

            user.SessionTokens = new CognitoUserSession(null, null, refreshToken, DateTime.Now, DateTime.Now.AddHours(1));

            InitiateRefreshTokenAuthRequest refreshRequest = new InitiateRefreshTokenAuthRequest()
            {
                AuthFlowType = AuthFlowType.REFRESH_TOKEN_AUTH
            };

            return await user.StartWithRefreshTokenAuthAsync(refreshRequest).ConfigureAwait(false);
        }
    }
}

This sample takes guidance from Amazon CognitoAuthentication Extension Library Examples. Hope this helps.

Thanks, Ashish

duaneking commented 3 years ago

Thank you for posting code that proves my reported defect actually exists.

Now how would we go about using only a refresh token of a previously logged in user without the username to generate a new access token for that user, per the spec this currently violates?

ashishdhingra commented 3 years ago

Thank you for posting code that proves my reported defect actually exists.

Now how would we go about using only a refresh token of a previously logged in user without the username to generate a new access token for that user, per the spec this currently violates?

Hi @duaneking,

The Amazon.Extensions.CognitoAuthentication package provides wrapper for using classes in Amazon.CognitoIdentityProvider and Amazon.CognitoIdentity namespaces. It's design makes actions (like using the RefreshToken, etc.) on top of the CognitoUser, refer https://github.com/aws/aws-sdk-net-extensions-cognito/blob/e9a2447ba179b7795eede4619906f48d0d3ee863/src/Amazon.Extensions.CognitoAuthentication/CognitoUserAuthentication.cs#L33.

So, unfortunately, you cannot use RefreshToken without the CognitoUser when using this package. Although you may try using the Amazon.CognitoIdentityProvider directly, but that would add complexity for your use case.

Thanks, Ashish

duaneking commented 3 years ago

The problem is that requiring the username actively violates security standards and opens up users to more risk by leaking identity data that the refresh token itself is designed to protect against.

A refresh token, once expired, is designed to be opaque and not leak identity information. Requiring the username in this way violates that, and so such an implementation would fail a DFIR audit as it would require the identity linked to the refresh token be leaked in the requests/repositories that store it.

This design is thus inherently insecure, and violates the claim that Amazon Cognito is HIPAA eligible and PCI DSS, SOC, and ISO/IEC 27001, ISO/IEC 27017, ISO/IEC 27018, and ISO 9001 compliant because according to you, this is designed to require additional credentials and that violates https://tools.ietf.org/html/rfc6749#section-6, do I understand you correctly?

ashishdhingra commented 3 years ago

The problem is that requiring the username actively violates security standards and opens up users to more risk by leaking identity data that the refresh token itself is designed to protect against.

A refresh token, once expired, is designed to be opaque and not leak identity information. Requiring the username in this way violates that, and so such an implementation would fail a DFIR audit as it would require the identity linked to the refresh token be leaked in the requests/repositories that store it.

This design is thus inherently insecure, and violates the claim that Amazon Cognito is HIPAA eligible and PCI DSS, SOC, and ISO/IEC 27001, ISO/IEC 27017, ISO/IEC 27018, and ISO 9001 compliant because according to you, this is designed to require additional credentials and that violates https://tools.ietf.org/html/rfc6749#section-6, do I understand you correctly?

Hi @duaneking,

Thanks for your response. As mentioned earlier, this package is designed to invoke requests mostly around CognitoUser. I'm not sure if design could be changed.

I also see that you are trying to automate some tests which require you to store this data, may be in the code itself (in turn in the code repository). However storing this data in repository itself is a bad design, and this should instead be stored in external data source (e.g. database) and could be fetched at runtime.

In the actual production application as well, this data would be stored in some store, and may follow the below workflow:

This is just the general thought on the application workflow.

However, I will have a developer look at your concern and would post any updates accordingly.

Thanks, Ashish

duaneking commented 3 years ago

I also see that you are trying to automate some tests..

Not true. I have no idea where you got this bad assumption from. I'm working on a production system, not only tests as you claim.

The goal is actually to never store the tokens in any way on the server side, because if they are not stored then they can not be leaked.

ashishdhingra commented 3 years ago

The goal is actually to never store the tokens in any way on the server side, because if they are not stored then they can not be leaked.

@duaneking Could you please elaborate on your use case on how you plan to store and use RefreshToken? Is the RefreshToken stored only in user session and used only when the access token expires (in this case also, RefreshToken is tied to a user and when user session ends, it gets rid of the tokens as well)?

duaneking commented 3 years ago

Use case is an industry standard standard mobile app refresh token flow.

duaneking commented 3 years ago

Also, your code above uses DateTime.Now and that's a defect, you should be using DateTime,.UtcNow in all cases.

duaneking commented 3 years ago

Ok so attempting to leverage that code or rewrite it but use the same call stack is resulting in nothing but exceptions.

First part of the resulting stack trace with our internal stuff removed since it would only confuse you and was never reached anyway:

Exception":{"ErrorType":"Unknown","ErrorCode":"NotAuthorizedException","RequestId":"<removed>","StatusCode":"BadRequest","Retryable":null,"TargetSite":"Boolean HandleException(Amazon.Runtime.IExecutionContext, Amazon.Runtime.Internal.HttpErrorResponseException)","StackTrace":" at Amazon.Runtime.Internal.HttpErrorResponseExceptionHandler.HandleException(IExecutionContext executionContext, HttpErrorResponseException exception)\r\n at Amazon.Runtime.Internal.ExceptionHandler1.Handle(IExecutionContext executionContext, Exception exception)\r\n at Amazon.Runtime.Internal.ErrorHandler.ProcessException(IExecutionContext executionContext, Exception exception)\r\n at Amazon.Runtime.Internal.ErrorHandler.InvokeAsync[T](IExecutionContext executionContext)\r\n at Amazon.Runtime.Internal.CallbackHandler.InvokeAsync[T](IExecutionContext executionContext)\r\n at Amazon.Runtime.Internal.EndpointDiscoveryHandler.InvokeAsync[T](IExecutionContext executionContext)\r\n at Amazon.Runtime.Internal.EndpointDiscoveryHandler.InvokeAsync[T](IExecutionContext executionContext)\r\n at Amazon.Runtime.Internal.CredentialsRetriever.InvokeAsync[T](IExecutionContext executionContext)\r\n at Amazon.Runtime.Internal.RetryHandler.InvokeAsync[T](IExecutionContext executionContext)\r\n at Amazon.Runtime.Internal.RetryHandler.InvokeAsync[T](IExecutionContext executionContext)\r\n at Amazon.Runtime.Internal.CallbackHandler.InvokeAsync[T](IExecutionContext executionContext)\r\n at Amazon.Runtime.Internal.CallbackHandler.InvokeAsync[T](IExecutionContext executionContext)\r\n at Amazon.Runtime.Internal.ErrorCallbackHandler.InvokeAsync[T](IExecutionContext executionContext)\r\n at Amazon.Runtime.Internal.MetricsHandler.InvokeAsync[T](IExecutionContext executionContext)\r\n at Amazon.Extensions.CognitoAuthentication.CognitoUser.StartWithRefreshTokenAuthAsync(InitiateRefreshTokenAuthRequest refreshTokenRequest)\r\n

For the second part, its claiming the errors is due to a "Invalid Refresh Token" error as part of a NotAuthorizedException despite the fact that it is in fact the full unedited refresh token.

"Message":"Invalid Refresh Token.","Data":[],"InnerException":{"Response":{"StatusCode":"BadRequest","IsSuccessStatusCode":false,"ContentType":"application/x-amz-json-1.1","ContentLength":70,"ResponseBody":{"$type":"HttpResponseMessageBody"},"$type":"HttpClientResponseData"},"TargetSite":"Void MoveNext()","StackTrace":" at Amazon.Runtime.HttpWebRequestMessage.GetResponseAsync(CancellationToken cancellationToken)\r\n at Amazon.Runtime.Internal.HttpHandler1.InvokeAsync[T](IExecutionContext executionContext)\r\n at Amazon.Runtime.Internal.Unmarshaller.InvokeAsync[T](IExecutionContext executionContext)\r\n at Amazon.Runtime.Internal.ErrorHandler.InvokeAsync[T](IExecutionContext executionContext)","Message":"Exception of type 'Amazon.Runtime.Internal.HttpErrorResponseException' was thrown.","Data":[],"InnerException":null,"HelpLink":null,"Source":"AWSSDK.Core","HResult":-2146233088,"$type":"HttpErrorResponseException"},"HelpLink":null,"Source":"AWSSDK.Core","HResult":-2146233088,"$type":"NotAuthorizedException"}

So I read this as the code saying that the newly created refresh token that was just generated a few seconds earlier via a login is not even a valid refresh token. So why is Cognito issuing invalid refresh tokens with usable and valid access tokens?

I'm not really sure how else to say this politely, but the code above does not work, as shown above by the exception saying that a perfectly valid refresh token that was just issued is not valid.

ashishdhingra commented 3 years ago

Ok so attempting to leverage that code or rewrite it but use the same call stack is resulting in nothing but exceptions.

@duaneking Kindly check if you have configured the AWS credentials, refer Credential and profile resolution for more details.

duaneking commented 3 years ago

With respect, everything is correctly configured as this same code base registers users, logs them in, generates access, id, and refresh tokens, ad also validates JWT's etc. Refresh is the only thing that does not work.

This minimal code repos the exception in visual studio:

` var refreshToken = "put the refresh token here";

var email = "user inside cognitopool yes replace this";

CognitoUser user = _userManager.FindByEmailAsync(email)?.Result;

user.SessionTokens = new CognitoUserSession(null, null, refreshToken, DateTime.UtcNow, DateTime.UtcNow.AddHours(1));

var refreshRequest = new InitiateRefreshTokenAuthRequest() { AuthFlowType = AuthFlowType.REFRESH_TOKEN_AUTH };

var authResponse = user.StartWithRefreshTokenAuthAsync(refreshRequest).ConfigureAwait(false).GetAwaiter().GetResult(); `

ashishdhingra commented 3 years ago

With respect, everything is correctly configured as this same code base registers users, logs them in, generates access, id, and refresh tokens, ad also validates JWT's etc. Refresh is the only thing that does not work.

This minimal code repos the exception in visual studio:

` var refreshToken = "put the refresh token here";

var email = "user inside cognitopool yes replace this";

CognitoUser user = _userManager.FindByEmailAsync(email)?.Result;

user.SessionTokens = new CognitoUserSession(null, null, refreshToken, DateTime.UtcNow, DateTime.UtcNow.AddHours(1));

var refreshRequest = new InitiateRefreshTokenAuthRequest() { AuthFlowType = AuthFlowType.REFRESH_TOKEN_AUTH };

var authResponse = user.StartWithRefreshTokenAuthAsync(refreshRequest).ConfigureAwait(false).GetAwaiter().GetResult(); `

@duaneking Thanks for your reply. I will give it a try using the sample app provided in the repo, adding the RefreshToken logic manually and let you know the findings.

duaneking commented 3 years ago

image

Code to reproduce based on your posted code:

private void ACogitoGeneratedRefreshTokenIsNotValid(string clientId, string userPoolId, string email, string refreshToken)
{
var provider = new AmazonCognitoIdentityProviderClient(new AnonymousAWSCredentials(), FallbackRegionFactory.GetRegionEndpoint());
var userPool = new CognitoUserPool(userPoolId, clientId, provider);
CognitoUser user = new CognitoUser(email, clientId, userPool, provider);
user.SessionTokens = new CognitoUserSession(null, null, refreshToken, DateTime.UtcNow, DateTime.UtcNow.AddHours(1));
var refreshRequest = new InitiateRefreshTokenAuthRequest() { AuthFlowType = AuthFlowType.REFRESH_TOKEN_AUTH };

// This call throws every time.
var authResponse = user.StartWithRefreshTokenAuthAsync(refreshRequest).ConfigureAwait(false).GetAwaiter().GetResult();

Console.WriteLine(authResponse.AuthenticationResult.AccessToken);
}
ashishdhingra commented 3 years ago

StartWithRefreshTokenAuthAsync

Hi @duaneking,

I was able to correctly execute the RefreshToken auth flow using Visual Studio:

Screen Shot 2021-04-08 at 4 53 36 PM

In case you app client has client secret as well, you would need to pass them to CognitoUserPool and CognitoUser constructors.

Kindly note that we are using Amazon.Extensions.CognitoAuthentication version 2.2.0 package.

Hope this helps.

Thanks, Ashish

normj commented 3 years ago

Hi @duaneking

I agree the design of the CognitoUser and refreshing tokens is too dependent on knowing the user id. I tested and was able to use the GetCredsFromRefreshAsync code sample with passing null in for the user-id. I think we should work on making a more intuitive workflow for refreshing tokens then this user-id based approach. I am assuming you created your Cognito app client without generating a client secret. If you do create the app client with a client secret then you will have to have a user-id to generate the secret hash for the request.

Not sure why you are getting the invalid refresh token error. In my testing I got this error when I was accidently using a refresh token that was created from a different app client id then the app client I was using to refresh with. I have also seen issues when Device tracking is enabled but the device key is not passed along.

Thanks for pointing out the improper use of DateTime.Now. I updated the README.md file to use DateTime.UtcNow.

Norm

duaneking commented 3 years ago

Edited after some tests:

@ashishdhingra Please see below.

@normj This has been repo'ing when the single user pool in that region only has only one client, so its not a wrong client issue either. I made the config as simple as possible to try to mitigate the risk of my own self doing something temporarily stupid. Device tracking has been turned off this entire time, but I just turned it off AGAIN by turning it on, then turning it off again, and now I'm no longer getting that exception but its also not giving me a new refresh token, only the old one, so I cant assume its working and I'm even more confused because the idea of a refresh token that doesn't cycle or change or expire when used seems very insecure so I'm assuming now the lack of a exception is progress but don't consider this to be fixed.

I do get new access tokens, but that seems really insecure to not cycle the refresh token or expire it after its used. You are correct in that there is no secret for this test, and so per your feedback, a user id should not be required. I appreciate the acknowledgment about the bad specific user focused design, and am eager to see improvement.

I'm understandably extremely frustrated given that its Sunday and I'm working on this. I don't even know what made it get past the exception as no code on my side changed based on a git-diff, and I can only assume something on the backend that's deployed and outside of my scope changed; that is not a good feeling.

The only thing I can think of is that the AWS console renders the wrong value and my clicking on settings changed something that should have been changed before but did not actually change, as on my end I changed nothing about the configuration as far as my accessibility tools go. But the AWS console for Cogitio has multiple known defects like that, most visible when it comes to custom fields, so I can't assume the console is perfect, either, as much as I would like to. The AWS console in general is a poor system, especially when things go wrong. But that would not explain why the refresh token I get now when I use the old one used does not change or expire as its still the same string after its used multiple times to get new access tokens, and from a security perspective, thats just painful and would not pass an audit.. so I'm assuming either somebody from AWS did something to the instance, or I have fixed one issue and removed the exception in the way only to now get to a much bigger issue.

Please confirm.

normj commented 3 years ago

Glad you have been able to make some headway and sorry for the frustrating experience getting to a working stage. Cognito does not currently update refresh tokens when using them to get new access tokens. So if you are getting new access tokens from the refresh call then you are working correctly.

Refresh tokens do expire. Their length is configurable when creating an app client for your Cognito user pool.

image

As whether refresh tokens should change after using them to get refresh tokens that is a question for the Cognito service. I'm on the AWS .NET Team and can just answer for this library. From my understanding through refresh tokens have their own lifecycle that is separate then access tokens and are used very differently. The access token is sent to the resource server and the refresh token is sent to the authorization server (Cognito) along with the client id and optional hashes of client secret if that was enabled. So far the refresh token to be compromised and useful the client id and client secret also have to be compromised.

Again I'm not on the Cognito service and they would know better then I would. I believe you are also have conversations with them so I will focus this issue on things we can control with this library.

HaikuJock commented 3 years ago

@normj

Thanks for pointing out the improper use of DateTime.Now. I updated the README.md file to use DateTime.UtcNow.

Norm

If we are using DateTime.UtcNow for the creation of the CognitoUserSession then shouldn't CognitoUserSession.IsValid also use DateTime.UtcNow?

duaneking commented 3 years ago

@normj Refresh tokens should change after they are used even once as they should be expiring after use for security. Anything less means that an old key is now an attack vector.

normj commented 3 years ago

@HaikuJock Version 2.2.1 was just released which switches all usages of DateTime.Now to DateTime.UtcNow.

@duaneking Since refresh tokens creation is controlled by the service there is nothing this library can do for your request of generating a different refresh token every time it is used to get a new access token. I'm going to close this issue because I think we have answered the specific issue as best we can from the viewpoint of this library. I want to avoid this issue being used for other issues like the DateTime issue, which was a good catch, because it makes the issues hard to track. If you discover other issues with the library please open up new issues.

github-actions[bot] commented 3 years ago

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see. If you need more assistance, please either tag a team member or open a new issue that references this one. If you wish to keep having a conversation with other community members under this issue feel free to do so.

duaneking commented 3 years ago

There needs to be a bug filed against the cognito service for refresh tokens not changing after they are redeemed the first time; Expiring them out based only on time is a critical defect; And them using the SDK team as a meatshield for their service defects is not good engineering, it also actively promotes a lack of customer focus by that team.

The security expectation is that once used or redeemed to generate a new access token, a refresh token should change and that refresh token should no longer be able to generate a new access token, because a refresh token should only be able to generate a new access token and refresh token pair only once, and when used, should be considered used and expired. Its a one time token, meant to be used once.