Repository navigation
Conversation
…Object Authorize the source of a versioned copy with objectGetVersion, the action checked by the authorization service, so its implicit deny is no longer read as an allow and bucket policy statements on s3:GetObjectVersion apply. Keep evaluating quotas for such a copy.
Hello tcarmet,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
| const copySourceApiName = apiName === 'objectGetVersion' ? 'objectGet' : apiName; | ||
| if (actionNeedQuotaCheckCopy(copySourceApiName, apiMethod)) { |
There was a problem hiding this comment.
This looks kinda convoluted to me, as actionNeedQuotaCheckCopy is a super simple function that may not deserve to be in Arsenal IMHO.
I think one of the following could be done:
- the whole logic could probably be just pasted there and then the new
objectGetVersioncase be inserted naturally in the tested condition - the copy-specific actions may be inserted in the
actionNeedQuotaCheckconstant object in arsenal, and the extra source action condition can be pasted in here, but this may have side-effects asactionNeedQuotaCheckis used elsewhere.
There was a problem hiding this comment.
Agreed. I went with your first option and inlined the condition, so objectGetVersion is now handled directly in the check and the Arsenal helper is no longer used here. It's in a separate commit (e3d8467) so it's easier to review, and I'll squash it once approved.
leif-scality
left a comment
leif-scality
left a comment
There was a problem hiding this comment.
LGTM. This fixes CLDSRV-1039 and I found no regressions. #6324 should be merged too, to close the remaining cross-bucket case (CLDSRV-1021).
@davidtencer For the release notes: users or roles allowed s3:GetObject but not s3:GetObjectVersion will now get AccessDenied when they copy a specific version (matches AWS).
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
... and 2 files with indirect coverage changes @@ Coverage Diff @@
## development/9.3 #6331 +/- ##
===================================================
+ Coverage 85.33% 85.35% +0.01%
===================================================
Files 206 206
Lines 13440 13441 +1
===================================================
+ Hits 11469 11472 +3
+ Misses 1971 1969 -2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
When a copy named a specific version of its source (
?versionId=, includingnull), the source read was checked as a GetObject, while the identity check had been made for GetObjectVersion. So the requester's identity policy was never applied, and bucket policy statements ons3:GetObjectVersionwere ignored. A copy of a specific version is now authorized like a GetObject of that version, and its quota is still evaluated. As a result, a bucket policy Deny meant to protect every version must covers3:GetObjectVersion(ors3:GetObject*), not onlys3:GetObject.