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❌ Patch coverage is
Additional details and impacted files
@@ Coverage Diff @@
## development/9.3 #6333 +/- ##
================================================
Coverage 85.34% 85.34%
================================================
Files 207 207
Lines 13466 13467 +1
================================================
+ Hits 11492 11493 +1
Misses 1974 1974
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| { apiMethod: 'objectCopy', destApiMethod: 'objectPut' }, | ||
| { apiMethod: 'objectPutCopyPart', destApiMethod: 'objectPutPart' }, | ||
| ].forEach(({ apiMethod, destApiMethod }) => { | ||
| it(`should authorize the destination of ${apiMethod} as ${destApiMethod}`, () => { |
There was a problem hiding this comment.
Should we have functional test as well on this ?
There was a problem hiding this comment.
I've addressed Leif's comment with an extra unit test that runs UploadPartCopy through api.callApiMethod, with Vault's answers keyed by API method. It checks that the copy is denied when IAM denies s3:PutObject on the destination, which covers our case.
I'm not adding a functional test for now. It would need the IAM policy changed mid-upload, because starting the upload already requires s3:PutObject, and that would risk making the test flaky.
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
left a comment
leif-scality
left a comment
There was a problem hiding this comment.
I checked it: with IAM denying s3:PutObject, an UploadPartCopy into an existing key is stored on base and denied with this PR.
Could you add a test that checks the request is denied? The current tests only check what is sent to Vault. Something like this, through api.callApiMethod (like tests/unit/api/api.js):
- Create a bucket, a source object, and the destination key, then start an upload on that key.
- Stub
auth.server.doAuthto denys3:PutObjectand key each result byrc._apiMethod, like Vault:requestContexts.map(rc => { const denied = rc.getAction() === 's3:PutObject'; return { isAllowed: !denied, isImplicit: denied, action: rc._apiMethod }; })
- Call
api.callApiMethod('objectPutCopyPart', ...)and expectAccessDenied.
This test fails without the fix and passes with it. Setting request.actionImplicitDenies by hand won't work, because it skips the code this PR changes.
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
Ask the authorization service about the UploadPartCopy destination as objectPutPart, the API method checked on the destination, so its implicit deny is no longer read as an allow.
…estination - Run UploadPartCopy through api.callApiMethod with Vault's results keyed by API method, so the test covers how the API reads them - Check that a copy into an existing key is denied when IAM denies s3:PutObject, and allowed otherwise
a612206 to
29b6231
Compare
|
/create_integration_branches |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option The following options are set: create_integration_branches |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following options are set: create_integration_branches |
|
/approve |
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
This pull request did not target the following hotfix branch(es) so they
Please check the status of the associated issue CLDSRV-1040. Goodbye tcarmet. The following options are set: approve, create_integration_branches |
UploadPartCopy asked the authorization service about its destination as a PutObject but checked the answer as an UploadPart. So when the destination key already existed, an identity policy denying the write was ignored and the part was stored. The destination is now authorized under the same API method it's checked with.