Skip to content

Fixes issue where 2 clients from the same service would share a signature cache. #1054

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

Merged
merged 2 commits into from
Jul 18, 2016
Merged

Fixes issue where 2 clients from the same service would share a signature cache. #1054

merged 2 commits into from
Jul 18, 2016

Conversation

chrisradek
Copy link
Contributor

This issue would result in the same signed key to be used by 2 different clients if they were configured to use the same region and accessKeyId. (Very unlikely if possible).

This issue also resulted in the signature cache being cleared anytime the client making a request was alternated with another client of the same service type.

Fixes #1020
Fixes #1052

/cc @LiuJoyceC
I added a unique client id to each client created to differentiate them when caching the signature. It's hidden for now as I didn't want it to be easy for users to find, but I am open to creating a method on Service to return the ID as well.

…ture cache.

This issue would result in the same signed key to be used by 2 different clients if they were configured to use the same region and accessKeyId. (Very unlikely if possible).

This issue also resulted in the signature cache being cleared anytime the client making a request was alternated with another client of the same service type.

Fixes #1020
Fixes #1052
@coveralls
Copy link

coveralls commented Jul 12, 2016

Coverage Status

Coverage increased (+0.04%) to 89.06% when pulling d535f96 on chrisradek:fix/signatureCache into 9787eaa on aws:master.

@@ -117,13 +118,13 @@ AWS.Signers.V4 = inherit(AWS.Signers.RequestSigner, {
return AWS.util.crypto.hmac(kCredentials, this.stringToSign(datetime), 'hex');
}

cachedSecret[this.serviceName] = {
cachedSecret[cacheIdentifier] = {
region: this.request.region, date: date,
key: kCredentials, akid: credentials.accessKeyId
};
Copy link
Contributor

Choose a reason for hiding this comment

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

We should put a maximum size constraint on cachedSecret. Before, there was only one cache per service (and only the last signature key for that service is kept), so the cache was necessarily bounded by the number of services. There is no upper bound on the number of service clients generated. Some applications could be generating a new service client for every request. The service client objects get garbage-collected eventually, but the cache doesn't and could grow to millions of objects fairly quickly and hog a lot of memory. Perhaps we could have a cacheIdentifierQueue of some sort so that when it's length reaches a certain number, we can get the oldest cacheIdentifier and delete it from cachedSecret.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point! I like the idea of having a queue keep track of what should be removed from cache first.

@coveralls
Copy link

coveralls commented Jul 15, 2016

Coverage Status

Coverage decreased (-0.2%) to 88.808% when pulling 09d0807 on chrisradek:fix/signatureCache into 9787eaa on aws:master.

@chrisradek
Copy link
Contributor Author

Code has been updated to only cache up to 50 service clients at a time.

@LiuJoyceC
Copy link
Contributor

🚢

@chrisradek chrisradek merged commit a737d93 into aws:master Jul 18, 2016
@chrisradek chrisradek deleted the fix/signatureCache branch July 18, 2016 20:52
chrisradek added a commit that referenced this pull request Jul 19, 2016
LiuJoyceC added a commit that referenced this pull request Jul 26, 2016
LiuJoyceC added a commit that referenced this pull request Jul 29, 2016
@lock
Copy link

lock bot commented Sep 28, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@lock lock bot locked as resolved and limited conversation to collaborators Sep 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Credential caching Cloudwatch SDK cache's key signing across objects
3 participants