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

preserve x-amz-content-sha256 if specified in presign_v4 #870

Merged
merged 2 commits into from
Apr 13, 2020

Conversation

vsoch
Copy link
Contributor

@vsoch vsoch commented Apr 6, 2020

I have the following use case:

  • I'm deploying Minio alongside a server (uwsgi with Django, that's not important)
  • A command line client, scs-library-client through Singularity is handling parsing an object to ask for mutlipart upload presigned URLs from my server. It conforms to the S3 API for Multipart upload.
  • This means that my server has endpoints to Start a multipart upload, and then a makes subsequent requests to retrieve a pre-signed URL, one for each part.
  • The Singularity client then uses the signed URL to interact with Minio (for those wondering this does mean that the external URL (127.0.0.1:9000) is different from the internal one (minio:9000) and to get around this I needed to make two clients, one for internal interaction (starting the multipart upload) and the other would have been used to generated the signed urls but I wound up just using it to create a prefix, I can elaborate more on this if anyone is interested).
  • After upload, Singularity again pings the server with a list of parts (numbers and ETags) that are then used for the (internal) s3 client to complete_multipart_upload.

The reason for this PR comes down to when Singularity uses the signed URL. I was using the signed url generated by the s3 client, that resulted in a signature mismatch. I next used the same flow that I saw for my working (single) PUT object request (also a signed URL from the Singularity client) that generates a URL and then uses signers.presign_v4. This continually failed. I tried about 1000 things, but the trick was that I needed to give the sha256 sum provided by the Singularity client to be included in the signature generation. Since I wasn't able to specify it for presign_v4, I essentially reproduced the function and used it as a variable. The error disappeared and my chunks sped through my localhost like a chicken with it's butt on fire! 🐔 🔥

So - this small fix will allow for an interested user that wants to use the minio-py client to generate a presigned url in the case that a server is making a request on behalf of a command line client (where the object to be uploaded lives and is not accessible to the server generating the URLs). The command line client sends the sha256sum to the server (which is not able to calculate it). While I think it would be great for minio-py to have a fully supported function to generate these presigned URLS more easily (akin to how you can generate one for a single PUT request), in the meantime this small change to allow specifying the content_hash_hex will be useful to users that have similar use case to me. For future reference, the code I am referring to is singularityhub/sregistry#298.

Signed-off-by: vsoch vsochat@stanford.edu

@vsoch
Copy link
Contributor Author

vsoch commented Apr 6, 2020

hey maintainers! It looks like this is some kind of race condition? Or at least the failed RUN is failing because it's reporting a bucket is not empty. Let me know if there is something I can do on my end - my changes to the function should not have led to any functional changes in the code as the default for my parameter is None, and it gets set to exactly what it was before.

@harshavardhana
Copy link
Member

hey maintainers! It looks like this is some kind of race condition? Or at least the failed RUN is failing because it's reporting a bucket is not empty. Let me know if there is something I can do on my end - my changes to the function should not have led to any functional changes in the code as the default for my parameter is None, and it gets set to exactly what it was before.

Don't worry about it we need to fix the Windows CI build

minio/signer.py Outdated Show resolved Hide resolved
minio/signer.py Outdated Show resolved Hide resolved
@vsoch vsoch force-pushed the add/api-presign_v4-content-hash branch from 36a6a33 to dc8070b Compare April 6, 2020 21:45
@vsoch
Copy link
Contributor Author

vsoch commented Apr 6, 2020

So to keep it generic we already take headers so if that covers it, then we don't need additional param - just honor x-amz-content-sha256 from input headers.

@harshavardhana I noticed that and tried supplying the same content hash via the headers variable, this resulted in the invalid signature still. I haven't looked closely at generate_canonical_request but it takes in content_sha256 separately so it must be handled differently, or at least having it provided alongside a header with the correct sha256 somehow adds an incorrect value.

@harshavardhana
Copy link
Member

@harshavardhana I noticed that and tried supplying the same content hash via the headers variable, this resulted in the invalid signature still. I haven't looked closely at generate_canonical_request but it takes in content_sha256 separately so it must be handled differently, or at least having it provided alongside a header with the correct sha256 somehow adds an incorrect value.

The reason is that the code should look for x-amz-content-sha256 present on headers and use that instead

minio/signer.py Outdated Show resolved Hide resolved
@vsoch vsoch force-pushed the add/api-presign_v4-content-hash branch from dc8070b to 8d6f770 Compare April 7, 2020 18:26
minio/signer.py Show resolved Hide resolved
minio/signer.py Outdated Show resolved Hide resolved
In the case that the user wants to use the minio-py client to generate
a presigned url, also in the case that a server is making a request on
behalf of a command line client (where the object to be uploaded lives)
this means that the command line client will need to send the sha256sum
to the server (which is not able to calculate it). In practice I was
attempting this workflow (singularity/sregistry PR minio#298) and getting
an invalid signature every time. I figured out that the sha256sum was
important because it was being sent to the server from the Singularity
command line tool, and the trick was getting it into the function
preload_v4 so that it would be included in the signature calculation.
Once I was able to do this, my pre-signed urls for multipart upload
worked like a charm! While I think it would be great for minio-py to have
a fully supported function to generate these URLS more easily, in the
meantime this small change to allow specifying the content_hash_hex
will be useful to users that have similar use case to me

Signed-off-by: vsoch <vsochat@stanford.edu>
@vsoch vsoch force-pushed the add/api-presign_v4-content-hash branch from 8d6f770 to 2bfb52c Compare April 7, 2020 18:28
@vsoch
Copy link
Contributor Author

vsoch commented Apr 7, 2020

The one last bit I'd want to ask about is where we can document this? If it's included in this repository I can add it here, otherwise you can point me to where a user would want to find this detail.

@harshavardhana
Copy link
Member

The one last bit I'd want to ask about is where we can document this? If it's included in this repository I can add it here, otherwise you can point me to where a user would want to find this detail.

Just add it as a comment, we don't really want to encourage this as most of our users are not aware of AWS S3 behavior and we would like to keep that abstracted.

@vsoch
Copy link
Contributor Author

vsoch commented Apr 7, 2020

okay, I'll add a comment to before the loop. One second!

The update to this function for presign_v4 will allow for the function
to work on behalf of a client call that wants to generate a pre-signed
url for another server, where the other server has sent the sha256sum
to the client generating the URL, and it needs to be added to the signature.
Without exposing this content_hash_hex via the headers{} dictionary,
it will not be included in the signature, and the signature calculated
by the receiving minio server (that does include it) will report
a signature mismatch. This means that if you need to include the sha256sum
in the signature, just add it to the headers dictionary.

Signed-off-by: vsoch <vsochat@stanford.edu>
@vsoch
Copy link
Contributor Author

vsoch commented Apr 7, 2020

okay I added a comment, and I also added a very detailed commit message so it's present in the git history too (and unexpected source of goodness sometimes!)

@harshavardhana harshavardhana changed the title Adding content_hash_hex parameter to signers.presign_v4 preserve x-amz-content-sha256 if specified in presign_v4 Apr 7, 2020
@harshavardhana
Copy link
Member

ping @vadmeste

Copy link
Member

@vadmeste vadmeste left a comment

Choose a reason for hiding this comment

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

LGTM

@kannappanr kannappanr merged commit 261c034 into minio:master Apr 13, 2020
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

5 participants