rgw: reject encrypted object COPY before supported #20739
Conversation
|
@mattbenjamin Could you take a look? |
src/rgw/rgw_rados.cc
Outdated
| } | ||
| RGWObjManifest manifest; | ||
| bufferlist::iterator miter = attr_iter2->second.begin(); | ||
| decode(manifest, miter); |
There was a problem hiding this comment.
how about:
RGWObjManifest manifest;
try {
bufferlist::iterator miter = attr_iter2->second.begin();
decode(manifest, miter);
} catch (buffer::error& err) {
ldout(cct, 0) << "ERROR: couldn't decode manifest" << dendl;
return false;
}
There was a problem hiding this comment.
iirc, some buffer operators are among the only Ceph interfaces that throw c++ exceptions, so this is probably sensible
src/rgw/rgw_rados.cc
Outdated
| if (attr_iter2 == attrs.end() || attr_iter2->second.length() == 0) { | ||
| return false; | ||
| } | ||
| RGWObjManifest manifest; |
There was a problem hiding this comment.
so the decoded manifest is local to rgw_s3_is_multipart_encrypted? that's probably ok, but I wonder if having the manifest...manifest at a higher level is helpful, longer run?
There was a problem hiding this comment.
+1 it looks like RGWRados::copy_obj() already has access to a decoded version of the manifest in astate->manifest?
|
if the source object is encrypted with sse-c, we should be requiring the i think the safest thing to do for now is reject COPY operations with 400 Bad Request if the source object has any |
Jeegn-Chen
force-pushed
the
wip-reject-sse-mpcp
branch
2 times, most recently
from
e802e3b
to
c5a2645
Compare
Jeegn-Chen
changed the title
|
Updated according to comments |
src/rgw/rgw_rados.cc
Outdated
| @@ -8097,6 +8107,13 @@ int RGWRados::copy_obj(RGWObjectCtx& obj_ctx, | |||
| if (ret < 0) { | |||
| return ret; | |||
| } | |||
| if (rgw_s3_is_encrypted(src_attrs)) { | |||
There was a problem hiding this comment.
i think this could just be if (src_atts.count(RGW_ATTR_CRYPT_MODE)) and we wouldn't need the extra member function
src/rgw/rgw_rados.cc
Outdated
| // Current encryption implemenation rely on the part sequence in manifest | ||
| // but current copy operation will result in part sequence change | ||
| // To Do: need more comprehensive design to support this kind of copy operation | ||
| ldout(cct, 0) << "ERROR: failed to copy encrypted multipart object " << src_obj << dendl; |
There was a problem hiding this comment.
the log message and comment still refer to multipart objects, but this code path applies to non-multipart objects too
src/rgw/rgw_rados.cc
Outdated
| // but current copy operation will result in part sequence change | ||
| // To Do: need more comprehensive design to support this kind of copy operation | ||
| ldout(cct, 0) << "ERROR: failed to copy encrypted multipart object " << src_obj << dendl; | ||
| return -ERR_INVALID_REQUEST; |
There was a problem hiding this comment.
I'd vote for a 501, -ERR_NOT_IMPLEMENTED error code unless it really is a invalid request
Jeegn-Chen
force-pushed
the
wip-reject-sse-mpcp
branch
from
c5a2645
to
f6e69e0
Compare
Jeegn-Chen
changed the title
|
Updated according to comments @cbodley @theanalyst |
src/rgw/rgw_rados.cc
Outdated
| if (src_attrs.count(RGW_ATTR_CRYPT_MODE)) { | ||
| // Current implementation does not follow S3 spec and even | ||
| // may result in data corruption silently when copying | ||
| // multipart objects accorss pools. So reject COPY operations |
There was a problem hiding this comment.
trivail one, 'accorss'
Jeegn-Chen
force-pushed
the
wip-reject-sse-mpcp
branch
from
f6e69e0
to
d7c4fbf
Compare
Current implementation does not follow S3 spec and even may result in data corruption silently when copying multipart objects accorss pools. So reject COPY operations on encrypted objects before it is fully functional. Fixes: http://tracker.ceph.com/issues/23232 Signed-off-by: Jeegn Chen <jeegnchen@gmail.com>
Jeegn-Chen
force-pushed
the
wip-reject-sse-mpcp
branch
from
d7c4fbf
to
a1513ef
Compare
Fixes: http://tracker.ceph.com/issues/23232
Signed-off-by: Jeegn Chen jeegnchen@gmail.com