Repository navigation
Conversation
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
... and 1 file with indirect coverage changes @@ Coverage Diff @@
## development/9.3 #6324 +/- ##
===================================================
- Coverage 85.30% 84.24% -1.07%
===================================================
Files 207 207
Lines 13468 13473 +5
===================================================
- Hits 11489 11350 -139
- Misses 1979 2123 +144
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Is prettier required for 9.3? |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
There was a problem hiding this comment.
Audit of the copy source authorization. Issues 1, 2, 3 and 4 are inline and need fixing in this PR. All were reproduced end to end against cloudserver and a real Vault (7.89.0). Line numbers refer to this PR's branch.
The rest were already broken before this PR and have their own tickets, so nothing is needed for them here:
- CLDSRV-1026: CopyObject: authorize the ACL and tagging headers on the destination (issue 5)
- CLDSRV-1017: Multi-object delete by IAM user applies the last key's authorization verdict to all keys (issue 6, existing, fixed on 9.5 and 9.6)
- CLDSRV-1025: DeleteObjects: check the bucket policy against each key, not the bucket (issue 6)
- CLDSRV-1028: IAM tag conditions: evaluate a copy's source read against the source's tags (issue 7)
- ARSN-661: Policy evaluator: a Deny with a tag condition hides the Allow of the same policy (issue 7)
- CLDSRV-1029: PutBucketPolicy: validate every condition key, not only the first of each operator (lets issue 4's
s3:VersionIdinto a bucket policy)
|
@davidtencer this PR also fixes an older security bug that the description doesn't mention. Do you want a separate ticket for it? The bug: when a copy request named a specific version of the source object, the requester's permission to read that object was never checked. A user in the same account could copy an object they weren't allowed to read into a place they could write, then read the copy there. This worked on unversioned buckets too. It existed before CLDSRV-1021. A ticket would let us list it in the release notes and decide on backports separately. In any case, @tcarmet, could you add a line about it to the PR description? |
|
@leif-scality and @tcarmet yes please I'd like to have an S3C bug ticket created to document the bug in release notes even though 2 tickets are fixed by the same PR. |
@tcarmet I can do the ticket |
leif-scality
left a comment
leif-scality
left a comment
There was a problem hiding this comment.
Second pass on 66872d0. Two issues, both inline:
- Issue 8 (inline on
parseCopySource.js): a repeatedversionIdinx-amz-copy-sourcecan crash the worker. Added by commit 3, so it should be fixed in this PR. - Issue 9 (inline on
objectPutCopyPart.js): UploadPartCopy ignores an IAM deny on the destination when the destination key already exists. This was already broken before this PR. It is the same bug commit 3 fixes for the source, a few lines away, so it is cheap to fix here. If you'd rather keep it out of this PR, like CLDSRV-1026, tell me and I'll open a ticket.
Line numbers refer to this PR's branch.
|
@davidtencer @tcarmet Created S3C-11581 for the versioned-copy IAM bypass (review issue 3). It also covers issue 2 (same root cause, also fixed by commit 3). Assigned to @tcarmet, linked to CLDSRV-1021 and S3C-11468. |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
0e4759f to
ca2dde6
Compare
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
|
@leif-scality I split this PR into 4, so each topic can be reviewed on its own and the review is easier:
If more topics come up, I'll look at each one individually and either create another stacked PR, fix the existing ones, or open a ticket. |
leif-scality
left a comment
leif-scality
left a comment
There was a problem hiding this comment.
This fixes CLDSRV-1021 and I found no regressions.
#6332 should be merged too, to close the remaining s3:VersionId case.
ca2dde6 to
6bf0773
Compare
6bf0773 to
6e38f5b
Compare
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
| Resource: `arn:aws:s3:::${bucketName}/allowed/*`, | ||
| }, | ||
| { | ||
| Sid: 'DenyReadOnForbiddenPrefix', |
There was a problem hiding this comment.
This Deny statement normally has no real effect because the IAM user has no allow policy attached to the forbidden/ prefix (or to any key) that would be the fallback, so it's denied by default, and it would still be denied even if the bucket policies aren't enforced I think.
I believe you could make the deny statement matter for the tests using the forbidden prefix - to ensure that the bucket deny policies are enforced on the source object - by either running the requests as the account, or attaching an identity policy to the test IAM user granting access to the forbidden prefix (and just that prefix so the bucket policy Allow rule also matters).
There was a problem hiding this comment.
Thank you for that, I've modified both unit and functional tests setup towards your argument. Let me know what you think. Added as a new commit for now so you can review it individually (I'll squash it to the test commit later on).
Without an IAM allow on the source, the copy is denied by default and the bucket policy Deny statement has no effect on the outcome. Granting the source read in IAM makes the Deny the only thing denying it.
CopyObject and UploadPartCopy evaluated the bucket policy for the copy source's read against the destination key instead of the source key. So a requester allowed to write to one prefix of a bucket could copy, and then read, objects under a prefix the bucket policy explicitly denies them reading. With this change, reading the copy source is authorized against the source bucket and key, the same as a direct GetObject.
Stacked on #6331, which must merge first: once the source bucket policy applies, a cross-account copy of a specific version would otherwise go through without the requester's own permission.