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

Add back md5 to zarr object url signing #1497

Merged
merged 2 commits into from
Feb 21, 2023
Merged

Conversation

danlamanna
Copy link
Contributor

Related #1494

The MD5 parameter needs to be part of the original signed URL, otherwise S3 will reject the upload URL. This means the MD5 header used by the client is mandatory.

The CLI will need to update the /files step to take a [{path: foo, md5: bar}, ...] structure instead of a plain list of paths.

The MD5 parameter needs to be part of the original signed URL,
otherwise S3 will reject the upload URL. This means the MD5 header used
by the client is mandatory.
@jjnesbitt
Copy link
Member

@jwodder This will require another update from the CLI

@jwodder
Copy link
Member

jwodder commented Feb 21, 2023

Will the CLI still need to supply a Content-MD5 header when PUTing file contents?

@danlamanna
Copy link
Contributor Author

Will the CLI still need to supply a Content-MD5 header when PUTing file contents?

Yes, that's what allows S3 to refuse it at time of upload if it's incorrect.

@jwodder
Copy link
Member

jwodder commented Feb 21, 2023

PR to CLI: dandi/dandi-cli#1215

@danlamanna
Copy link
Contributor Author

danlamanna commented Feb 21, 2023

Merging this side of things first. Please hold off merging the other side until I confirm this is working.

@danlamanna danlamanna merged commit 9e7b39f into master Feb 21, 2023
@danlamanna danlamanna deleted the add-zarr-md5-signing-urls branch February 21, 2023 20:39
@jwodder
Copy link
Member

jwodder commented Feb 21, 2023

I tried to use my CLI PR to upload to staging, but it failed with a 403 and the message "The request signature we calculated does not match the signature you provided. Check your key and signing method."

@danlamanna
Copy link
Contributor Author

Yeah, sorry about this. We're in an unfortunate situation where our local object store behaves differently than S3 in this specific case, so we're developing a bit blind. I'll update when I figure out what's going on.

@dandibot
Copy link
Member

🚀 PR was released in v0.3.18 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants