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

[BUG] SAS token SAS_EXPIRY_TIME encoding results in an invalid signature #22042

Closed
3 tasks done
fcofdez opened this issue Jun 3, 2021 · 11 comments · Fixed by #25524
Closed
3 tasks done

[BUG] SAS token SAS_EXPIRY_TIME encoding results in an invalid signature #22042

fcofdez opened this issue Jun 3, 2021 · 11 comments · Fixed by #25524
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. Storage Storage Service (Queues, Blobs, Files)

Comments

@fcofdez
Copy link
Contributor

fcofdez commented Jun 3, 2021

Describe the bug
After introducing #20520 SAS query params are sanitized but it seems like the SAS_EXPIRY_TIME gets modified and it invalidates the provided signature.

Exception or Stack Trace

<?xml version="1.0" encoding="utf-8"?><Error><Code>AuthenticationFailed</Code><Message>Server failed to authenticate the request. Make sure the value of Authorization header is formed correctly including the signature.
RequestId:76972c6c-201e-0090-1453-582f4d000000
Time:2021-06-03T08:38:25.4063780Z</Message><AuthenticationErrorDetail>Signature did not match. String to sign used was rwdl

2021-07-20T13:21:00Z
/blob/<account>/<container>



2018-11-09
c

To Reproduce
Provide a SAS token with SAS_EXPIRY_TIME without seconds, i.e. se=2021-07-20T13%3A21Z&sp=rwdl&sv=2018-11-09&sr=c&sig=<redacted> that converts the http request query params into se=2021-07-20T13%3A21%3A00Z&sp=rwdl&sv=2018-11-09&sr=c&sig=<redacted> making the signature invalid. This can be reproduced with curl easily.

Expected behavior
Generate a valid http request

Setup (please complete the following information):

  • Version of the Library used: 12.11.1

Additional context
Add any other context about the problem here.

Information Checklist
Kindly make sure that you have added all the following information above and checkoff the required fields otherwise we will treat the issuer as an incomplete report

  • Bug Description Added
  • Repro Steps Added
  • Setup information Added
@ghost ghost added needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. customer-reported Issues that are reported by GitHub users external to the Azure organization. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that labels Jun 3, 2021
@alzimmermsft alzimmermsft added Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files) labels Jun 3, 2021
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Jun 3, 2021
@alzimmermsft
Copy link
Member

@gapra-msft could you take a look into this?

@gapra-msft
Copy link
Member

Hi @fcofdez,

Thank you for reporting this issue. We will take a look at what could be causing the problem and get back to you if we have any questions.

@jaschrep-msft
Copy link
Member

@fcofdez, can I get some better information about how exactly you're encountering this? You're specifically pointing to a PR that just aligned connection string SAS ingestion with what every other SAS ingestion mechanism already did.

  • Are you encountering this new issue when providing a SAS through connection strings?
  • If you try to ingest a SAS via a different mechanism in the library, do you hit this same error?
    • Before the mentioned PR?
    • After the mentioned PR?
  • Where are you getting this SAS?

@fcofdez
Copy link
Contributor Author

fcofdez commented Jun 10, 2021

@jaschrep-msft we provide the SAS token using a connection string, but as far as I can see in the code that shouldn't make any difference.

new BlobServiceClientBuilder()
            .connectionString(connectionString)
  • Where are you getting this SAS?
    It comes from our CI infrastructure as we caught this running integration tests.

I think the problem comes when the SAS query parameters are manipulated, as it seems like the signature is computed using the SAS_EXPIRY_TIME literal value. To test this theory I run a HTTP request using curl with the SAS query parameters as they are provided in our tests and the request worked correctly. When I modified the SAS_EXPIRY_TIME query parameter and included seconds to it (as https://github.com/Azure/azure-sdk-for-java/pull/20520/files#diff-119366ace1e5390d2aff952c5a6b8a0292d3a98638687277f8c5f41b373c4090R90 does), the request failed.

@fcofdez
Copy link
Contributor Author

fcofdez commented Jun 10, 2021

I've been checking the docs https://docs.microsoft.com/en-gb/rest/api/storageservices/formatting-datetime-values and it seems like YYYY-MM-DDThh:mm<TZDSuffix> is a valid date format. Just to add more context to the issue.

@fcofdez
Copy link
Contributor Author

fcofdez commented Jun 10, 2021

As a final note, the token was generated with the CLI

az storage container generate-sas 

@fcofdez
Copy link
Contributor Author

fcofdez commented Jun 30, 2021

Any news on this?

@fcofdez
Copy link
Contributor Author

fcofdez commented Sep 9, 2021

I'm wondering if there's been some progress on this?

@rickle-msft
Copy link
Contributor

Hey, @fcofdez. Apologies for the delay. It looks like we slipped on this quite a bit, so thank you for following up.

I think you are correct that the PR in question will break a sas that doesn't have seconds. The code will eventually hit a line that tries to parse and then encode the sas query parameters and in so doing formats the date values it finds as "yyyy-MM-dd'T'HH:mm:ss'Z'".

We will work on getting a fix out for this

@rickle-msft rickle-msft removed the question The issue doesn't require a change to the product in order to be resolved. Most issues start as that label Sep 10, 2021
@fcofdez
Copy link
Contributor Author

fcofdez commented Oct 6, 2021

Hi again,
I was wondering if there's been progress on this issue? is there anything I can do to help on this? Thanks!

@rickle-msft
Copy link
Contributor

@fcofdez Thank you for checking in. Unfortunately, we haven't had an opportunity to get to many fixes like this lately. This is near the top of our list of bugs to address once work for supporting new service versions calms a bit.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants