-
Notifications
You must be signed in to change notification settings - Fork 95
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
Return HTTP 400 if content-md5 does not match #596
Conversation
Fixes #596 Respect content-md5 headers on object uploads and return HTTP 400 with appropriate error body when it does not match the calculated digest of the object body. Also refactor the handling of content-md5 headers for multipart part uploads so that is matches the handling for normal object uploads.
thoughts on making a separate TODO for writing a client-test for this? |
yeah good idea |
I'm reviewing locally merged or rebased with #622. |
|
client test passed, but int-test is failing. Will see if this is happening on develop too. |
Fixes #596 Respect content-md5 headers on object uploads and return HTTP 400 with appropriate error body when it does not match the calculated digest of the object body. Also refactor the handling of content-md5 headers for multipart part uploads so that is matches the handling for normal object uploads.
Rebased from latest |
@@ -70,6 +71,22 @@ api_error({error, Reason}, RD, Ctx) -> | |||
error_response(StatusCode, _Code, _Message, RD, Ctx) -> | |||
{{halt, StatusCode}, RD, Ctx}. | |||
|
|||
%% @TODO Figure out how this should actually be formatted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what way are we not sure how this should be formatted? Does this just mean we haven't tested that this makes clients throw the correct exception? If so, that can be part of #624.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, this is also just for OpenStack, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, they don't have any documentation I have found about expected error responses or error formatting and I haven't had a chance to manually determine it yet so punting for now.
+1, great work |
Fixes #596 Respect content-md5 headers on object uploads and return HTTP 400 with appropriate error body when it does not match the calculated digest of the object body. Also refactor the handling of content-md5 headers for multipart part uploads so that is matches the handling for normal object uploads.
See #624 to track testing of this PR. |
_ = maybe_update_manifest_with_confirmation(ManiPid, Manifest), | ||
_ = riak_cs_gc:gc_active_manifests(Bucket, Key, RiakPid), | ||
gen_fsm:reply(From, {error, invalid_digest}), | ||
{stop, invalid_digest, State}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This causes a crash report to be printed. I might argue that this is still a 'normal' exit.
On PUT, Riak currently will not return HTTP 400 if the Content-MD5 header and the MD5 of the body do not match. S3 does. It does return the correct MD5 as the ETAG, and some clients do recognize this and throw and exception. Returning HTTP 400 is easy. Making sure we properly garbage collect the 'bad' version is a bit trickier.