CLDSRV-1021: Evaluate the bucket policy against the copy source

[tcarmet]

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.

[bert-e]

Hello tcarmet,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Available options

name	description	privileged	authored
/after_pull_request	Wait for the given pull request id to be merged before continuing with the current one.
/bypass_author_approval	Bypass the pull request author's approval	⭐
/bypass_build_status	Bypass the build and test status	⭐
/bypass_commit_size	Bypass the check on the size of the changeset TBA	⭐
/bypass_incompatible_branch	Bypass the check on the source branch prefix	⭐
/bypass_jira_check	Bypass the Jira issue check	⭐
/bypass_peer_approval	Bypass the pull request peers' approval	⭐
/bypass_leader_approval	Bypass the pull request leaders' approval	⭐
/bypass_source_branch_lineage	Bypass the cross-branch contamination check	⭐
/approve	Instruct Bert-E that the author has approved the pull request.		✍️
/create_pull_requests	Allow the creation of integration pull requests.
/create_integration_branches	Allow the creation of integration branches.
/no_octopus	Prevent Wall-E from doing any octopus merge and use multiple consecutive merge instead
/unanimity	Change review acceptance criteria from one reviewer at least to all reviewers
/wait	Instruct Bert-E not to run until further notice.

Available commands

name	description	privileged
/help	Print Bert-E's manual in the pull request.
/status	Print Bert-E's current status in the pull request.
/clear	Remove all comments from Bert-E from the history TBA
/retry	Re-start a fresh build TBA
/build	Re-start a fresh build TBA
/force_reset	Delete integration branches & pull requests, and restart merge process from the beginning.
/reset	Try to remove integration branches unless there are commits on them which do not appear on the source branch.

Status report is not available.

[bert-e]

Incorrect fix version

The Fix Version/s in issue CLDSRV-1021 contains:

• None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

• 9.3.23

• 9.4.8

• 9.5.0

Please check the Fix Version/s of CLDSRV-1021, or the target
branch of this pull request.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.37%. Comparing base (b961ac9) to head (ca2dde6).
✅ All tests successful. No failed tests found.

Additional details and impacted files

Files with missing lines	Coverage Δ
lib/api/apiUtils/authorization/permissionChecks.js	97.30% <100.00%> (+0.04%)	⬆️
lib/api/objectCopy.js	91.73% <ø> (+0.39%)	⬆️
lib/api/objectPutCopyPart.js	87.57% <ø> (+0.59%)	⬆️
lib/metadata/metadataUtils.js	94.73% <100.00%> (ø)

... and 2 files with indirect coverage changes

@@                                    Coverage Diff                                     @@
##           bugfix/CLDSRV-1039-versioned-copy-source-authorization    #6324      +/-   ##
==========================================================================================
+ Coverage                                                   85.32%   85.37%   +0.04%     
==========================================================================================
  Files                                                         206      206              
  Lines                                                       13441    13446       +5     
==========================================================================================
+ Hits                                                        11469    11479      +10     
+ Misses                                                       1972     1967       -5     

Flag	Coverage Δ
file-ft-tests	68.31% <71.42%> (-0.01%)	⬇️
file-ft-tests-null-compat	68.83% <71.42%> (-0.09%)	⬇️
kmip-ft-tests	28.35% <28.57%> (-0.02%)	⬇️
mongo-v0-ft-tests	69.58% <71.42%> (+0.07%)	⬆️
mongo-v1-ft-tests	69.52% <71.42%> (-0.03%)	⬇️
multiple-backend	36.79% <28.57%> (-0.02%)	⬇️
s3c-ft-tests-v0	64.03% <100.00%> (+0.03%)	⬆️
s3c-ft-tests-v0-null-compat	64.11% <100.00%> (+0.03%)	⬆️
s3c-ft-tests-v1	64.01% <100.00%> (+0.01%)	⬆️
sur-tests	35.94% <28.57%> (-0.81%)	⬇️
sur-tests-inflights	37.78% <28.57%> (-0.01%)	⬇️
unit	71.70% <100.00%> (+0.01%)	⬆️
utapi-v2-tests	34.59% <28.57%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[leif-scality]

Is prettier required for 9.3?

[bert-e]

Incorrect fix version

The Fix Version/s in issue CLDSRV-1021 contains:

• None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

• 9.3.23

• 9.4.8

• 9.5.0

• 9.6.0

Please check the Fix Version/s of CLDSRV-1021, or the target
branch of this pull request.

[leif-scality]

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:VersionId into a bucket policy)

[leif-scality]

@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?

[davidtencer]

@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.

[leif-scality]

@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]

Second pass on 66872d0. Two issues, both inline:

• Issue 8 (inline on parseCopySource.js): a repeated versionId in x-amz-copy-source can 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.

[leif-scality]

@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.

[bert-e]

Incorrect fix version

The Fix Version/s in issue CLDSRV-1021 contains:

• None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

• 9.3.24

• 9.4.8

• 9.5.0

• 9.6.0

Please check the Fix Version/s of CLDSRV-1021, or the target
branch of this pull request.

[bert-e]

Incorrect fix version

The Fix Version/s in issue CLDSRV-1021 contains:

• None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

• 9.3.24

• 9.4.9

• 9.5.0

• 9.6.0

Please check the Fix Version/s of CLDSRV-1021, or the target
branch of this pull request.

[tcarmet]

@leif-scality I split this PR into 4, so each topic can be reviewed on its own and the review is easier:

• CLDSRV-1039: Authorize a copy of a specific source version like a GetObject #6331 (CLDSRV-1039): issues 1 to 3, plus the quota check. Merges first.
• CLDSRV-1021: Evaluate the bucket policy against the copy source #6324 (CLDSRV-1021, this PR): Stacked
• CLDSRV-1021: Compare s3:VersionId with the copy source version ID as sent #6332 (CLDSRV-1021): issues 4 and 8. Stacked on this PR.
• CLDSRV-1040: Authorize the UploadPartCopy destination as objectPutPart #6333 (CLDSRV-1040): issue 9, independent.

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.