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

Adding support of Token for Temporary Credentials #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DanOertelt
Copy link

This library works fine when using full AWS credentials however it does not work when using temporary credentials like when running on EC2 instance and using the 'embedded EC2' credentials that come from the IAM.
The temporary credentials require one to add a token header, otherwise the request is not validly signed. While this can be theoretically added outside of this library, I would consider it actual part of the signing.

This library works fine when using full AWS credentials however it does not work when using temporary credentials like when running on EC2 instance and using the 'embedded EC2' credentials that come from the IAM. 
The temporary credentials require one to add a token header, otherwise the request is not validly signed. While this can be theoretically added outside of this library, I would consider it actual part of the signing.
@DanOertelt
Copy link
Author

Any chance reviewing this pull request, please?

Thanks, Daniel

@tsibelman
Copy link
Owner

@DanOertelt I already did you can see my comment inline

@DanOertelt
Copy link
Author

I've to admit I'm new to Github, but I can't just locate any inline comment. And I've tried to look pretty hard. It is still possible I'm missing it, but...

Thanks, Daniel

request.Headers.TryAddWithoutValidation("Authorization", $"{algorithm} Credential={_access_key}/{credential_scope}, SignedHeaders={signed_headers}, Signature={signature}");

if (!string.IsNullOrEmpty(_token))
Copy link
Owner

Choose a reason for hiding this comment

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

Thank you very much for your pull request, but I think this change can be a bit confusing for the users, as I see in the documentation

"... When you add the X-Amz-Security-Token parameter to the query string, some services require that you include this parameter in the canonical (signed) request. For other services, you add this parameter at the end, after you calculate the signature. "

So dependent on the service used it may act differently, right now the code is not handling this distinction and implements only the second strategy, for with you actually don't need to do anything special. As a user of a library if I encounter the option to pass x-amz-security-token, I would expect it to support the first scenario.

I personally used this library only for API Gateway communications but right now it not blocking other use cases.

Do you also use it for API Gateway?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the review, Yes, I use it for API Gateway too right now. It took me a bit to figure out why the code stopped working when deployed to EC2. In the end I realized that using the temporary credentials requires me to add the Token as well (locally, I used permanent credentials, hence it worked).

Yes, I get your point. The documentation indeed sounds as if there are two ways in which one should include the Token depending on the service. Handling it properly would require the library to know which service uses which approach and then implement both cases.

Implementing both cases does not seem to be a problem. But knowing which service uses which could be a problem. Or should the API of the library allow the user to actually specify which case they want to use? Thoughts?

Copy link
Owner

Choose a reason for hiding this comment

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

Ideally one of the two scenarios could be detected based on the service name, but I don't know where the list of service behaviors can be found, so until such list exists it needs to be passed as some enum flag to a constructor.

But I don't sure if this makes anything that much simpler for the end user, is just as easy to add the header either before or after the signer is executed.

Copy link
Author

Choose a reason for hiding this comment

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

Well, I honestly think detecting it on service name would be a nightmare as that would require constant maintenance and we know AWS is adding services in quite rapid pace.

I agree that the action of adding the header etc. is not that complex and hence you questioning if it makes sense to do it in the library is somewhat reasonable. However I would argue that just knowing how the header should be formed and what is its proper name as well as the mere presence of it in the method signature is of great value as it reminds the user of the library that there is something like a token that needs to be considered under certain circumstances.

I spent several hours wondering what is the culprit of my application using your library working locally and not working on EC2 instance could be before I ran into the token concept that I was not aware of.

Copy link
Owner

Choose a reason for hiding this comment

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

I see that you saying if you could change your PR to address both scenarios I can merge it and publish a new nuget

@tsibelman
Copy link
Owner

@DanOertelt Sorry it looks like I commented but did not committed the review

@NeilBostrom
Copy link

@tsibelman @DanOertelt Shame this didn't go anywhere. I could really do with this feature.

@tsibelman
Copy link
Owner

tsibelman commented May 30, 2019

@NeilBostrom it very easy to add this token in your own code, but problematic for a library to know when to add it

@joelbyren
Copy link

I was using this lib and got it working with user access key/secret, but I couldn't get it to work with temporary credentials (from assume role). Turned out I had not included X-Amz-Security-Token, and this reply helped me find it! Thank you!

@emonhaider
Copy link

I was using this lib and got it working with user access key/secret, but I couldn't get it to work with temporary credentials (from assume role). Turned out I had not included X-Amz-Security-Token, and this reply helped me find it! Thank you!

Can you please confirm if the header has to be added prior to signing the request?

@joelbyren
Copy link

I was using this lib and got it working with user access key/secret, but I couldn't get it to work with temporary credentials (from assume role). Turned out I had not included X-Amz-Security-Token, and this reply helped me find it! Thank you!

Can you please confirm if the header has to be added prior to signing the request?

X-Amz-Security-Token is added after signing. In the code I use, credentials does not always have a token (it depends on the credential used), so I use the UseToken property to know, and then the Token property as header value. This is with the C# version of AWS SDK.

@albertocorrales
Copy link

Could this PR be merged? I also needed to add the token header in my code after signing, so it would be awesome if this was added as part of the sign method, being token optional.

@ltoshev
Copy link

ltoshev commented Jun 24, 2021

I don't think you need this PR, you just pass your token outside here is a sample:

var signer = new AWS4RequestSigner(parts[0], parts[1]);
var content = new StringContent(JsonConvert.SerializeObject(obj), Encoding.UTF8, "application/json");
var request = new HttpRequestMessage
{
Method = HttpMethod.Post,
RequestUri = new Uri(url),
Content = content
};
request.Headers.TryAddWithoutValidation("X-Amz-Security-Token", parts[2]);

        try
        {
            request = await signer.Sign(request, "execute-api", "eu-west-1");
            var client = new HttpClient();
            var response = await client.SendAsync(request);
            var responseStr = await response.Content.ReadAsStringAsync();
            var resp = JsonConvert.DeserializeObject<InvalidateCacheResponse>(responseStr);
            return resp.Success;
        }
        catch (Exception ex) 
        {
            ExceptionManager.Publish(ex);
        }

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.

7 participants