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: Authorization Policy does applies SAS token twice on retry requests #1331

Conversation

dmweis
Copy link
Contributor

@dmweis dmweis commented Aug 9, 2023

Background

I encountered a bug where put_block operations would sporadically fail with http status code 403 - Forbidden when authenticating using a SAS token.

Detailed of the error were:
"AuthenticationFailed"
"Server failed to authenticate the request. Make sure the value of Authorization header is formed correctly including the signature."
<AuthenticationErrorDetail>Signature fields not well formed.</AuthenticationErrorDetail>

Upon investigating I narrowed it down to the Authorization Policy applying SAS token params multiple times for retry requests.

Simple way to diagnose this issues when using reqwest as the url library is to run Env Logger filter with "azure_core::http_client::reqwest=debug" which will cause reqwest to print the full URL for all requests.

Details

I used the same approach as the Access Key policy already uses and simply check the presence of sig param in the path. This doesn't feel like a very foolproof solution but it fixes the issue for now.

It seems that the python SDK suffered from a similar issues and they chose the same approach for resolving it Azure/azure-storage-python#304

The tests I added feel a little awkward because I had to mock the policy setup. If there is an already existing mock that would allow me to test this easily I am happy to update the PR!

I don't believe this should be an issue for header based authentication since insert_header would override an existing header if it was present

@dmweis
Copy link
Contributor Author

dmweis commented Aug 12, 2023

I see there are CI failures but I think that's due to #1332

@demoray
Copy link
Contributor

demoray commented Aug 16, 2023

@dmweis can you rebase from main to pull in fixes for #1332

@dmweis dmweis force-pushed the dmw/fix-authentication-failure-with-sas-tokens-on-retries branch from c583d20 to beba21b Compare August 23, 2023 21:14
@dmweis
Copy link
Contributor Author

dmweis commented Aug 23, 2023

@dmweis can you rebase from main to pull in fixes for #1332

Done!

@demoray demoray merged commit fd707ca into Azure:main Aug 24, 2023
16 checks passed
@demoray
Copy link
Contributor

demoray commented Aug 24, 2023

Thanks for the contribution!

@dmweis dmweis deleted the dmw/fix-authentication-failure-with-sas-tokens-on-retries branch August 26, 2023 17:26
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.

3 participants