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.

Sign up for GitHub

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

Jump to bottom

aws/credentials: Fix race between credentials retrieval and expiration check #3448

Merged
merged 5 commits into from
Oct 20, 2020
Merged

aws/credentials: Fix race between credentials retrieval and expiration check #3448

merged 5 commits into from
Oct 20, 2020

Conversation

danieler15
Copy link
Contributor

I believe there is a race condition in the Credentials GetWithContext() method.

When Credentials calls provider.Retrieve() from within singleRetrieve, presumably the provider may update some internal state to reflect that the credentials it just returned from Retrieve() are not expired. This will cause subsequent calls to Credentials.isExpired() to return false.

However, there is a time between when Credentials calls provider.Retrieve(), and when Credentials actually stores the new credentials that it retrieved. If in this time, another goroutine tries to Get() credentials, it will see that the provider is not expired, and return the old expired cached credentials. Hopefully the added test illustrates this scenario.

I ran into this issue after upgrading the version of the sdk I was using. It seems that a RWMutex had been in use here but was removed in #3127

@jasdel
Copy link
Contributor

jasdel commented Aug 6, 2020

Thanks for taking the time to report and submit a change for this issue @danieler15. It looks like something in the test or run is causing the tests to get stuck and deadlock for several of the SDK versions/platforms.

@jasdel jasdel added the needs-review This issue or pull request needs review from a core team member. label Aug 11, 2020
@rittneje rittneje mentioned this pull request Sep 28, 2020
Closed
3 tasks
danieler15 and others added 3 commits October 19, 2020 15:37
@danieler15 @jasdel
eliminate credentials race between retrieve and isExpired
be99982
@danieler15 @jasdel
remove unused constructor
96dbe74
@jasdel
aws/credentials: Fix race condition with credentials expire
3308209 
Fixes aws#834 by adding read write locks to calls to the credentials
provider's `IsExpired` method. Previously there was a race condition
where the credentials could be refreshing, set the Provider's Expiry,
while another goroutine is checking if the credentials are expired. In
this case the second goroutine would receive the old credentials,
incorrectly.

This change also adds protection to GetWithContext that the check for if
the credentials are not expired will not be blocked by the write mutex
in the provider's Retrieve method.

Incorporates some of the changes in
aws#3563 to remove the atomic value
around `creds`.
@jasdel jasdel mentioned this pull request Oct 20, 2020
Closed
@jasdel
Copy link
Contributor

jasdel commented Oct 20, 2020 •
edited
Loading

Updated this pr with the chagnes in PR #3563 by @rittneje 's to remove creds being an atomic value. Also added fixup to a few of the test cases, to not make assumptions of how waiting and Context.Done methods are called.

@jasdel jasdel merged commit a409487 into aws:master Oct 20, 2020
@rittneje rittneje mentioned this pull request Oct 21, 2020
Closed
aws-sdk-go-automation pushed a commit that referenced this pull request Oct 21, 2020
Release v1.35.12 (2020-10-21)
d7c163f 
===

### Service Client Updates
* `service/cloudfront`: Updates service API and documentation
  * CloudFront adds support for managing the public keys for signed URLs and signed cookies directly in CloudFront (it no longer requires the AWS root account).
* `service/ec2`: Updates service API and documentation
  * instance-storage-info nvmeSupport added to DescribeInstanceTypes API
* `service/globalaccelerator`: Updates service API and documentation
* `service/glue`: Updates service API and documentation
  * AWS Glue crawlers now support incremental crawls for the Amazon Simple Storage Service (Amazon S3) data source.
* `service/kendra`: Updates service API and documentation
  * This release adds custom data sources: a new data source type that gives you full control of the documents added, modified or deleted during a data source sync while providing run history metrics.
* `service/organizations`: Updates service documentation
  * AWS Organizations renamed the 'master account' to 'management account'.

### SDK Bugs
* `aws/credentials`: Fixed a race condition checking if credentials are expired. ([#3448](#3448))
  * Fixes [#3524](#3524)
* `internal/ini`: Fixes ini file parsing for cases when Right Hand Value is missed in the last statement of the ini file ([#3596](#3596))
  * related to [#2800](#2800)
@aws-sdk-go-automation aws-sdk-go-automation mentioned this pull request Oct 21, 2020
Merged
aws-sdk-go-automation added a commit that referenced this pull request Oct 21, 2020
@aws-sdk-go-automation
Release v1.35.12 (2020-10-21) (#3607)
b655064 
Release v1.35.12 (2020-10-21)
===

### Service Client Updates
* `service/cloudfront`: Updates service API and documentation
  * CloudFront adds support for managing the public keys for signed URLs and signed cookies directly in CloudFront (it no longer requires the AWS root account).
* `service/ec2`: Updates service API and documentation
  * instance-storage-info nvmeSupport added to DescribeInstanceTypes API
* `service/globalaccelerator`: Updates service API and documentation
* `service/glue`: Updates service API and documentation
  * AWS Glue crawlers now support incremental crawls for the Amazon Simple Storage Service (Amazon S3) data source.
* `service/kendra`: Updates service API and documentation
  * This release adds custom data sources: a new data source type that gives you full control of the documents added, modified or deleted during a data source sync while providing run history metrics.
* `service/organizations`: Updates service documentation
  * AWS Organizations renamed the 'master account' to 'management account'.

### SDK Bugs
* `aws/credentials`: Fixed a race condition checking if credentials are expired. ([#3448](#3448))
  * Fixes [#3524](#3524)
* `internal/ini`: Fixes ini file parsing for cases when Right Hand Value is missed in the last statement of the ini file ([#3596](#3596)) 
  * related to [#2800](#2800)
@danieler15
Copy link
Contributor Author

Thanks @jasdel for fixing this up and merging! I had spent some time trying to fix these tests awhile ago but then admittedly got distracted by other work. Glad to see this merged

bdwyertech added a commit to bdwyertech/go-aws-cred-cachier that referenced this pull request Oct 26, 2020
@bdwyertech
Update aws-sdk-go to address potential race condition checking creden…
8f241dc 
…tial expiration status

- ref: aws/aws-sdk-go#3448

Signed-off-by: Brian Dwyer <Brian.Dwyer@broadridge.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasdel jasdel jasdel approved these changes

@skmcgrail skmcgrail skmcgrail approved these changes

@skotambkar skotambkar Awaiting requested review from skotambkar

Assignees
No one assigned
Labels
needs-review This issue or pull request needs review from a core team member.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@danieler15 @jasdel @skmcgrail