Repository navigation
CLDSRV-1021: Evaluate the bucket policy against the copy source #6324
+541
−16
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -242,6 +242,9 @@ function objectCopy(authInfo, request, sourceBucket, sourceObject, sourceVersion | |
| */ | ||
| checkQuota: true, | ||
| request, | ||
| // The request targets the destination, the bucket policy must be | ||
| // evaluated against the source | ||
| policyTarget: { bucketName: sourceBucket, objectKey: sourceObject, versionId: sourceVersionId }, | ||
|
Contributor
There was a problem hiding this comment. 1. A cross-account versioned copy ignores the requester's IAM policy (caused by this PR)Root cause, shared by issues 1 to 3:
Context: In cross-account access, the bucket owner's policy must allow it and the requester's own account (its IAM) must allow it too. Before this PR, the source bucket's policy never matched on cross-bucket copies, so this was denied (the false deny tracked in S3C-11363, which this PR probably fixes). Now its Allow statements match, and the cross-account branch ( Status: Reproduced: development/9.3 denies, this PR allows (tested with Reproduce:
Fix:
Contributor
There was a problem hiding this comment. 2. A bucket-policy Deny on
|
||
| }; | ||
| const valPutParams = { | ||
|
Contributor
There was a problem hiding this comment. 5. CopyObject never checks the ACL and tagging actions on the destination (already broken before this PR)
|
||
| authInfo, | ||
|
|
||
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,203 @@ | ||
| const assert = require('assert'); | ||
| const { | ||
| S3Client, | ||
| PutObjectCommand, | ||
| GetObjectCommand, | ||
| CopyObjectCommand, | ||
| CreateMultipartUploadCommand, | ||
| UploadPartCopyCommand, | ||
| AbortMultipartUploadCommand, | ||
| PutBucketPolicyCommand, | ||
| DeleteBucketPolicyCommand, | ||
| } = require('@aws-sdk/client-s3'); | ||
| const { | ||
| IAMClient, | ||
| CreateUserCommand, | ||
| CreateAccessKeyCommand, | ||
| DeleteAccessKeyCommand, | ||
| DeleteUserCommand, | ||
| } = require('@aws-sdk/client-iam'); | ||
| const { v4: uuid } = require('uuid'); | ||
|
|
||
| const getConfig = require('../support/config'); | ||
| const BucketUtility = require('../../lib/utility/bucket-util'); | ||
| const { config } = require('../../../../../lib/Config'); | ||
|
|
||
| const bucketName = `copy-source-bp-${uuid()}`; | ||
| const userName = `copy-source-bp-user-${uuid()}`; | ||
| const forbiddenKey = 'forbidden/myfile'; | ||
| const allowedKey = 'allowed/myfile'; | ||
| const mpuKey = 'allowed/mpu-copy'; | ||
| const allowedContent = 'allowed content'; | ||
|
|
||
| // this test needs a real vault to create an IAM user | ||
| const isVaultScality = config.backends.auth !== 'mem'; | ||
| const vaultHost = config.vaultd?.host || 'localhost'; | ||
|
|
||
| const describeWithVault = isVaultScality ? describe : describe.skip; | ||
|
|
||
| describeWithVault('Copy source with a bucket policy scoped to key prefixes', () => { | ||
| const bucketUtilAccount = new BucketUtility('default'); | ||
| const s3ClientAccount = bucketUtilAccount.s3; | ||
|
|
||
| const iamConfig = getConfig('default', { region: 'us-east-1' }); | ||
| iamConfig.endpoint = `http://${vaultHost}:8600`; // define outside of getConfig for Integration | ||
| const iamClient = new IAMClient(iamConfig); | ||
|
|
||
| let userS3Client; | ||
| let accessKeyId; | ||
| let uploadId; | ||
|
|
||
| async function assertAccessDenied(promise) { | ||
| await assert.rejects(promise, { name: 'AccessDenied' }); | ||
| } | ||
|
|
||
| before(async () => { | ||
| await bucketUtilAccount.createOne(bucketName); | ||
| await s3ClientAccount.send( | ||
| new PutObjectCommand({ | ||
| Bucket: bucketName, | ||
| Key: forbiddenKey, | ||
| Body: 'forbidden content', | ||
| }), | ||
| ); | ||
| await s3ClientAccount.send( | ||
| new PutObjectCommand({ | ||
| Bucket: bucketName, | ||
| Key: allowedKey, | ||
| Body: allowedContent, | ||
| }), | ||
| ); | ||
| await s3ClientAccount.send( | ||
| new PutBucketPolicyCommand({ | ||
| Bucket: bucketName, | ||
| Policy: JSON.stringify({ | ||
| Version: '2012-10-17', | ||
| Statement: [ | ||
| { | ||
| Sid: 'AllowReadWriteOnAllowedPrefix', | ||
| Effect: 'Allow', | ||
| Principal: '*', | ||
| Action: ['s3:GetObject', 's3:PutObject'], | ||
| Resource: `arn:aws:s3:::${bucketName}/allowed/*`, | ||
| }, | ||
| { | ||
| Sid: 'DenyReadOnForbiddenPrefix', | ||
| Effect: 'Deny', | ||
| Principal: '*', | ||
| Action: 's3:GetObject', | ||
| Resource: `arn:aws:s3:::${bucketName}/forbidden/*`, | ||
| }, | ||
| ], | ||
| }), | ||
| }), | ||
| ); | ||
|
|
||
| // IAM user without any identity policy | ||
| await iamClient.send(new CreateUserCommand({ UserName: userName })); | ||
| const accessKeyResponse = await iamClient.send(new CreateAccessKeyCommand({ UserName: userName })); | ||
| accessKeyId = accessKeyResponse.AccessKey.AccessKeyId; | ||
| userS3Client = new S3Client( | ||
| getConfig('default', { | ||
| credentials: { | ||
| accessKeyId, | ||
| secretAccessKey: accessKeyResponse.AccessKey.SecretAccessKey, | ||
| }, | ||
| }), | ||
| ); | ||
|
|
||
| const mpuResponse = await userS3Client.send( | ||
| new CreateMultipartUploadCommand({ | ||
| Bucket: bucketName, | ||
| Key: mpuKey, | ||
| }), | ||
| ); | ||
| uploadId = mpuResponse.UploadId; | ||
| }); | ||
|
|
||
| after(async () => { | ||
| if (uploadId) { | ||
| await s3ClientAccount.send( | ||
| new AbortMultipartUploadCommand({ | ||
| Bucket: bucketName, | ||
| Key: mpuKey, | ||
| UploadId: uploadId, | ||
| }), | ||
| ); | ||
| } | ||
| await s3ClientAccount.send(new DeleteBucketPolicyCommand({ Bucket: bucketName })); | ||
| await bucketUtilAccount.empty(bucketName); | ||
| await bucketUtilAccount.deleteOne(bucketName); | ||
| if (accessKeyId) { | ||
| await iamClient.send(new DeleteAccessKeyCommand({ UserName: userName, AccessKeyId: accessKeyId })); | ||
| } | ||
| await iamClient.send(new DeleteUserCommand({ UserName: userName })); | ||
| }); | ||
|
|
||
| it('should deny GetObject on a source denied by the bucket policy', async () => { | ||
| await assertAccessDenied( | ||
| userS3Client.send( | ||
| new GetObjectCommand({ | ||
| Bucket: bucketName, | ||
| Key: forbiddenKey, | ||
| }), | ||
| ), | ||
| ); | ||
| }); | ||
|
|
||
| it('should deny CopyObject from a source denied by the bucket policy', async () => { | ||
| await assertAccessDenied( | ||
| userS3Client.send( | ||
| new CopyObjectCommand({ | ||
| Bucket: bucketName, | ||
| Key: 'allowed/myfile-copy', | ||
| CopySource: `${bucketName}/${forbiddenKey}`, | ||
| }), | ||
| ), | ||
| ); | ||
| }); | ||
|
|
||
| it('should deny UploadPartCopy from a source denied by the bucket policy', async () => { | ||
| await assertAccessDenied( | ||
| userS3Client.send( | ||
| new UploadPartCopyCommand({ | ||
| Bucket: bucketName, | ||
| Key: mpuKey, | ||
| UploadId: uploadId, | ||
| PartNumber: 1, | ||
| CopySource: `${bucketName}/${forbiddenKey}`, | ||
| }), | ||
| ), | ||
| ); | ||
| }); | ||
|
|
||
| it('should allow CopyObject from a source allowed by the bucket policy', async () => { | ||
| await userS3Client.send( | ||
| new CopyObjectCommand({ | ||
| Bucket: bucketName, | ||
| Key: 'allowed/allowed-copy', | ||
| CopySource: `${bucketName}/${allowedKey}`, | ||
| }), | ||
| ); | ||
| const getResponse = await userS3Client.send( | ||
| new GetObjectCommand({ | ||
| Bucket: bucketName, | ||
| Key: 'allowed/allowed-copy', | ||
| }), | ||
| ); | ||
| assert.strictEqual(await getResponse.Body.transformToString(), allowedContent); | ||
| }); | ||
|
|
||
| it('should allow UploadPartCopy from a source allowed by the bucket policy', async () => { | ||
| const res = await userS3Client.send( | ||
| new UploadPartCopyCommand({ | ||
| Bucket: bucketName, | ||
| Key: mpuKey, | ||
| UploadId: uploadId, | ||
| PartNumber: 2, | ||
| CopySource: `${bucketName}/${allowedKey}`, | ||
| }), | ||
| ); | ||
| assert(res.CopyPartResult.ETag, 'Expected a part ETag'); | ||
| }); | ||
| }); |
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4.
s3:VersionIdis checked against the internal version ID (low severity)Context:
policyTarget.versionIdis the decoded internal ID (parseCopySource.js:29).s3:VersionIdagainst the external ID the client sent, so a version-specific Deny never matches on a copy.bucketPolicyAuth.js:642) passes a raw string, so it passes without reflecting production.s3:VersionId, although it only validates the first key in each condition operator.prepareRequestContexts.js:83-84), so IAM policies usings3:VersionIdare probably affected too. Not reproduced, since that depends on Vault.Reproduce (unit level): Write a policy that denies
s3:GetObjectwhens3:VersionIdequals the external version ID. CallisObjAuthorizedwithpolicyTarget.versionId = VersionID.decode(external), which is whatapi.jspasses. The Deny doesn't apply.Fix: Have
parseCopySourcealso return the version ID exactly as the client sent it. Use that forpolicyTarget.versionIdand for the Vault request, and keep the decoded ID for the metadata lookup. Update the test to use a real encoded ID.