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

Fix bearer token invalidation bug by calling AddResponses() twice on 401 errors #763

Merged
merged 1 commit into from Aug 7, 2023
Merged

Conversation

iain-macdonald
Copy link
Contributor

@iain-macdonald iain-macdonald commented Aug 3, 2023

Issue #, if available:
#686

Description of changes:
There's a pretty detailed description of the bug that this PR fixes at #686. This fix addresses the first sentence of the second paragraph there:

However, the soci-snapshotter code that provides HTTP Responses to the authorizer only ever provides the most recent HTTP Response, so the logic above is never exercised. Instead, it always hits the n == 1 condition in invalidAuthorization.

Instead of just calling AddResponses() with the one response, this PR changes that logic to provide the same 401 response twice to AddResponses() to force invalidation of the expired token and then adds a second call to AddResponses() with just once 401 response to fetch a fresh bearer token before proceeding to retry fetching resources from the remote registry.

Testing performed:
We have been using this fix in production at BuildBuddy to stream public container images from remote registries without incident:

Kern also kindly offered to add an integration test for this bug in #686.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@iain-macdonald iain-macdonald requested a review from a team as a code owner August 3, 2023 22:04
@sparr
Copy link
Contributor

sparr commented Aug 4, 2023

* 9958f9a "Fix bearer token invalidation bug by calling AddResponses() twice on 401 errors" ... FAIL
- FAIL - does not have a valid DCO

Could you amend this commit to include a signoff? git commit -s will do so automatically.

@iain-macdonald
Copy link
Contributor Author

Oh shoot, sorry about that. Just added one, confirmed it works with make check and re-pushed.

…401 errors

Signed-off-by: Iain Macdonald <xiainx@gmail.com>
Copy link
Contributor

@Kern-- Kern-- left a comment

Choose a reason for hiding this comment

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

Thank you!

Copy link
Contributor

@sparr sparr left a comment

Choose a reason for hiding this comment

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

Created blocked issue to fix this when the commented upstream PRs are released

@sparr sparr merged commit 86c249e into awslabs:main Aug 7, 2023
5 checks passed
@sparr
Copy link
Contributor

sparr commented Aug 9, 2023

For future reference, if you put "closes" or "fixes" or a few other key words before the issue # in the PR description, the issue will get closed when the PR is merged. https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

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.

None yet

3 participants