s3: Fix precondition failed in CopyObjectPart when src is encrypted #7276
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #7276 +/- ##
==========================================
+ Coverage 47.9% 48.36% +0.45%
==========================================
Files 281 296 +15
Lines 45965 46364 +399
==========================================
+ Hits 22021 22425 +404
+ Misses 21900 21883 -17
- Partials 2044 2056 +12
Continue to review full report at Codecov.
|
poornas
requested changes
Feb 22, 2019
vadmeste
changed the title
|
A refactor is needed to move the pre-condition checks inside the GetObjectNInfo() @vadmeste that way we can selectively avoid decrypting the ETag to match. |
Or, we can ensure that ETag in ObjectInfo always contain the real etag but we return the encrypted format (32 bytes) when responding to users (head object, list objects, etc..) Is there any written specification about S3 behavior ? other than the etag should not be md5sum in case of SSE-C ? |
|
It is the output, that is how it used to be . But many tools verify the md5sum like s3cmd, aws-sdk-java and they selectively ignore sse-c. So we ended up keeping this style. |
|
@vadmeste ,https://github.com/poornas/minio/tree/fixcopyETag on top of your commit should fix the etag issue - essentially bringing in the etag pre-condition check prior to decrypting so we can use last 32 bytes of encrypted ETag for ssec encrypted objects. |
|
Thanks @poornas for the work. |
|
Thanks @poornas ! The following diff is still needed for minio-go tests successful: diff --git a/cmd/object-api-utils.go b/cmd/object-api-utils.go
index bd46eff6..fdf537c5 100644
--- a/cmd/object-api-utils.go
+++ b/cmd/object-api-utils.go
@@ -558,6 +558,19 @@ func NewGetObjectReader(rs *HTTPRangeSpec, oi ObjectInfo, pcfn CheckCopyPrecondi
}
}
fn = func(inputReader io.Reader, _ http.Header, pcfn CheckCopyPreconditionFn, cFns ...func()) (r *GetObjectReader, err error) {
+
+ cFns = append(cleanUpFns, cFns...)
+
+ if pcfn != nil {
+ if ok := pcfn(oi, ""); ok {
+ // Call the cleanup funcs
+ for i := len(cFns) - 1; i >= 0; i-- {
+ cFns[i]()
+ }
+ return nil, PreConditionFailed{}
+ }
+ }
+
// Decompression reader.
snappyReader := snappy.NewReader(inputReader)
// Apply the skipLen and limit on the
@@ -569,7 +582,7 @@ func NewGetObjectReader(rs *HTTPRangeSpec, oi ObjectInfo, pcfn CheckCopyPrecondi
r = &GetObjectReader{
ObjInfo: oi,
pReader: decReader,
- cleanUpFns: append(cleanUpFns, cFns...),
+ cleanUpFns: cFns,
precondFn: pcfn,
}
return r, nil
@@ -581,10 +594,23 @@ func NewGetObjectReader(rs *HTTPRangeSpec, oi ObjectInfo, pcfn CheckCopyPrecondi
return nil, 0, 0, err
}
fn = func(inputReader io.Reader, _ http.Header, pcfn CheckCopyPreconditionFn, cFns ...func()) (r *GetObjectReader, err error) {
+
+ cFns = append(cleanUpFns, cFns...)
+
+ if pcfn != nil {
+ if ok := pcfn(oi, ""); ok {
+ // Call the cleanup funcs
+ for i := len(cFns) - 1; i >= 0; i-- {
+ cFns[i]()
+ }
+ return nil, PreConditionFailed{}
+ }
+ }
+
r = &GetObjectReader{
ObjInfo: oi,
pReader: inputReader,
- cleanUpFns: append(cleanUpFns, cFns...),
+ cleanUpFns: cFns,
precondFn: pcfn,
}
return r, nil
diff --git a/cmd/object-handlers-common.go b/cmd/object-handlers-common.go
index f14a9cc2..e31e69ae 100644
--- a/cmd/object-handlers-common.go
+++ b/cmd/object-handlers-common.go
@@ -96,7 +96,7 @@ func checkCopyObjectPreconditions(ctx context.Context, w http.ResponseWriter, r
}
}
- ssec := crypto.SSEC.IsRequested(r.Header)
+ ssec := crypto.SSECopy.IsRequested(r.Header)
// x-amz-copy-source-if-match : Return the object only if its entity tag (ETag) is the
// same as the one specified; otherwise return a 412 (precondition failed).would you review them and push it again in your branch so I can cherry-pick your commit ? |
good catch @vadmeste , updated the branch and tested. |
vadmeste
force-pushed
the
fix-copy-encrypted
branch
2 times, most recently
from
693af64
to
30f12fc
Compare
|
@vadmeste gateway-s3 test seem to be failing |
|
This can be reproduced with |
|
@vadmeste, added gateway precondition check for the s3 gateway test failure |
vadmeste
force-pushed
the
fix-copy-encrypted
branch
2 times, most recently
from
4d7c544
to
2edafc4
Compare
poornas
reviewed
Feb 27, 2019
|
You are the official reviewer @poornas |
harshavardhana
previously approved these changes
Feb 27, 2019
| @@ -486,7 +488,7 @@ func NewGetObjectReader(rs *HTTPRangeSpec, oi ObjectInfo, cleanUpFns ...func()) | |||
| // a reader that returns the desired range of | |||
| // encrypted bytes. The header parameter is used to | |||
| // provide encryption parameters. | |||
| fn = func(inputReader io.Reader, h http.Header, cFns ...func()) (r *GetObjectReader, err error) { | |||
| fn = func(inputReader io.Reader, h http.Header, pcfn CheckCopyPreconditionFn, cFns ...func()) (r *GetObjectReader, err error) { | |||
There was a problem hiding this comment.
S3 behavior requires precondition to be validated before validating the range - but this can be fixed in a different PR @vadmeste
kannappanr
reviewed
Feb 27, 2019
cmd/gateway/s3/gateway-s3.go
Outdated
| @@ -467,6 +467,9 @@ func (l *s3Objects) CopyObject(ctx context.Context, srcBucket string, srcObject | |||
| // metadata input is already a trickled down value from interpreting x-amz-metadata-directive at | |||
| // handler layer. So what we have right now is supposed to be applied on the destination object anyways. | |||
| // So preserve it by adding "REPLACE" directive to save all the metadata set by CopyObject API. | |||
| if srcOpts.CheckCopyPrecondFn != nil && srcOpts.CheckCopyPrecondFn(srcInfo, "") { | |||
There was a problem hiding this comment.
Code should be above the comment.
aead
reviewed
Feb 27, 2019
aead
reviewed
Feb 27, 2019
9 tasks
vadmeste
force-pushed
the
fix-copy-encrypted
branch
from
2edafc4
to
5dd4e0f
Compare
|
vadmeste
force-pushed
the
fix-copy-encrypted
branch
3 times, most recently
from
c4b3017
to
1d03c8a
Compare
vadmeste
force-pushed
the
fix-copy-encrypted
branch
from
1d03c8a
to
9f048bf
Compare
CopyObject precondition checks into GetObjectReader in order to perform SSE-C pre-condition checks using the last 32 bytes of encrypted ETag rather than the decrypted ETag This also necessitates moving precondition checks for gateways to gateway layer rather than object handler check
vadmeste
force-pushed
the
fix-copy-encrypted
branch
from
9f048bf
to
eb3b42b
Compare
|
@poornas we still need the following diff after I investigated new mint errors with gateways: diff --git a/cmd/disk-cache.go b/cmd/disk-cache.go
index 2774ddfe..7cf2bff4 100644
--- a/cmd/disk-cache.go
+++ b/cmd/disk-cache.go
@@ -254,8 +254,7 @@ func (c cacheObjects) GetObjectNInfo(ctx context.Context, bucket, object string,
cleanupBackend := func() { bkReader.Close() }
cleanupPipe := func() { pipeReader.Close() }
- gr = NewGetObjectReaderFromReader(teeReader, bkReader.ObjInfo, opts.CheckCopyPrecondFn, cleanupBackend, cleanupPipe)
- return gr, nil
+ return NewGetObjectReaderFromReader(teeReader, bkReader.ObjInfo, opts.CheckCopyPrecondFn, cleanupBackend, cleanupPipe)
}
// Uses cached-object to serve the request. If object is not cached it serves the request from the backend and also
diff --git a/cmd/fs-v1.go b/cmd/fs-v1.go
index a5ae1e5b..fabbbe6d 100644
--- a/cmd/fs-v1.go
+++ b/cmd/fs-v1.go
@@ -500,7 +500,7 @@ func (fs *FSObjects) GetObjectNInfo(ctx context.Context, bucket, object string,
if hasSuffix(object, slashSeparator) {
// The lock taken above is released when
// objReader.Close() is called by the caller.
- return NewGetObjectReaderFromReader(bytes.NewBuffer(nil), objInfo, opts.CheckCopyPrecondFn, nsUnlocker), nil
+ return NewGetObjectReaderFromReader(bytes.NewBuffer(nil), objInfo, opts.CheckCopyPrecondFn, nsUnlocker)
}
// Take a rwPool lock for NFS gateway type deployment
rwPoolUnlocker := func() {}
diff --git a/cmd/gateway/azure/gateway-azure.go b/cmd/gateway/azure/gateway-azure.go
index eb4bd608..fc7750ac 100644
--- a/cmd/gateway/azure/gateway-azure.go
+++ b/cmd/gateway/azure/gateway-azure.go
@@ -651,7 +651,7 @@ func (a *azureObjects) GetObjectNInfo(ctx context.Context, bucket, object string
// Setup cleanup function to cause the above go-routine to
// exit in case of partial read
pipeCloser := func() { pr.Close() }
- return minio.NewGetObjectReaderFromReader(pr, objInfo, opts.CheckCopyPrecondFn, pipeCloser), nil
+ return minio.NewGetObjectReaderFromReader(pr, objInfo, opts.CheckCopyPrecondFn, pipeCloser)
}
// GetObject - reads an object from azure. Supports additional
diff --git a/cmd/gateway/b2/gateway-b2.go b/cmd/gateway/b2/gateway-b2.go
index 7a1e8493..03c54716 100644
--- a/cmd/gateway/b2/gateway-b2.go
+++ b/cmd/gateway/b2/gateway-b2.go
@@ -417,7 +417,7 @@ func (l *b2Objects) GetObjectNInfo(ctx context.Context, bucket, object string, r
// Setup cleanup function to cause the above go-routine to
// exit in case of partial read
pipeCloser := func() { pr.Close() }
- return minio.NewGetObjectReaderFromReader(pr, objInfo, opts.CheckCopyPrecondFn, pipeCloser), nil
+ return minio.NewGetObjectReaderFromReader(pr, objInfo, opts.CheckCopyPrecondFn, pipeCloser)
}
// GetObject reads an object from B2. Supports additional
diff --git a/cmd/gateway/gcs/gateway-gcs.go b/cmd/gateway/gcs/gateway-gcs.go
index ec7c07cc..f45964af 100644
--- a/cmd/gateway/gcs/gateway-gcs.go
+++ b/cmd/gateway/gcs/gateway-gcs.go
@@ -753,7 +753,7 @@ func (l *gcsGateway) GetObjectNInfo(ctx context.Context, bucket, object string,
// Setup cleanup function to cause the above go-routine to
// exit in case of partial read
pipeCloser := func() { pr.Close() }
- return minio.NewGetObjectReaderFromReader(pr, objInfo, opts.CheckCopyPrecondFn, pipeCloser), nil
+ return minio.NewGetObjectReaderFromReader(pr, objInfo, opts.CheckCopyPrecondFn, pipeCloser)
}
// GetObject - reads an object from GCS. Supports additional
diff --git a/cmd/gateway/oss/gateway-oss.go b/cmd/gateway/oss/gateway-oss.go
index 52d3fbd0..ccb80c14 100644
--- a/cmd/gateway/oss/gateway-oss.go
+++ b/cmd/gateway/oss/gateway-oss.go
@@ -569,7 +569,7 @@ func (l *ossObjects) GetObjectNInfo(ctx context.Context, bucket, object string,
// Setup cleanup function to cause the above go-routine to
// exit in case of partial read
pipeCloser := func() { pr.Close() }
- return minio.NewGetObjectReaderFromReader(pr, objInfo, opts.CheckCopyPrecondFn, pipeCloser), nil
+ return minio.NewGetObjectReaderFromReader(pr, objInfo, opts.CheckCopyPrecondFn, pipeCloser)
}
// GetObject reads an object on OSS. Supports additional
diff --git a/cmd/gateway/s3/gateway-s3.go b/cmd/gateway/s3/gateway-s3.go
index d5c0cd40..9ec42b3a 100644
--- a/cmd/gateway/s3/gateway-s3.go
+++ b/cmd/gateway/s3/gateway-s3.go
@@ -400,7 +400,7 @@ func (l *s3Objects) GetObjectNInfo(ctx context.Context, bucket, object string, r
// Setup cleanup function to cause the above go-routine to
// exit in case of partial read
pipeCloser := func() { pr.Close() }
- return minio.NewGetObjectReaderFromReader(pr, objInfo, opts.CheckCopyPrecondFn, pipeCloser), nil
+ return minio.NewGetObjectReaderFromReader(pr, objInfo, opts.CheckCopyPrecondFn, pipeCloser)
}
// GetObject reads an object from S3. Supports additional
diff --git a/cmd/object-api-utils.go b/cmd/object-api-utils.go
index 3e553820..e32cf237 100644
--- a/cmd/object-api-utils.go
+++ b/cmd/object-api-utils.go
@@ -428,13 +428,22 @@ type GetObjectReader struct {
// NewGetObjectReaderFromReader sets up a GetObjectReader with a given
// reader. This ignores any object properties.
-func NewGetObjectReaderFromReader(r io.Reader, oi ObjectInfo, pcfn CheckCopyPreconditionFn, cleanupFns ...func()) *GetObjectReader {
+func NewGetObjectReaderFromReader(r io.Reader, oi ObjectInfo, pcfn CheckCopyPreconditionFn, cleanupFns ...func()) (*GetObjectReader, error) {
+ if pcfn != nil {
+ if ok := pcfn(oi, ""); ok {
+ // Call the cleanup funcs
+ for i := len(cleanupFns) - 1; i >= 0; i-- {
+ cleanupFns[i]()
+ }
+ return nil, PreConditionFailed{}
+ }
+ }
return &GetObjectReader{
ObjInfo: oi,
pReader: r,
cleanUpFns: cleanupFns,
precondFn: pcfn,
- }
+ }, nil
}
// ObjReaderFn is a function type that takes a reader and returns
diff --git a/cmd/xl-v1-object.go b/cmd/xl-v1-object.go
index 3ef075ea..5b6cf025 100644
--- a/cmd/xl-v1-object.go
+++ b/cmd/xl-v1-object.go
@@ -182,7 +182,7 @@ func (xl xlObjects) GetObjectNInfo(ctx context.Context, bucket, object string, r
nsUnlocker()
return nil, toObjectErr(err, bucket, object)
}
- return NewGetObjectReaderFromReader(bytes.NewBuffer(nil), objInfo, opts.CheckCopyPrecondFn, nsUnlocker), nil
+ return NewGetObjectReaderFromReader(bytes.NewBuffer(nil), objInfo, opts.CheckCopyPrecondFn, nsUnlocker)
}
var objInfo ObjectInfo |
vadmeste
force-pushed
the
fix-copy-encrypted
branch
from
40842da
to
c5960fc
Compare
Mint Automation
7276-c5960fc/mint-dist-xl.sh.log: |
|
Can I get some reviews here ? |
|
@aead can you please review this PR? |
poornas
approved these changes
Mar 6, 2019
aead
approved these changes
Mar 6, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
CopyObject precondition checks into GetObjectReader
in order to perform SSE-C pre-condition checks using the
last 32 bytes of encrypted ETag rather than the decrypted
ETag
This also necessitates moving precondition checks for
gateways to gateway layer rather than object handler check
Motivation and Context
Fix issue
Regression
No
How Has This Been Tested?
This PR tests this use case minio/minio-go#1078
Otherwise, to test manually, you can upload a large file, e.g. 600 Mb, upload it
encrypted (SSE-C), then copy it in the same bucket with mc.
Types of changes
Checklist:
mintPR # here: )