Improvement/cldsrv 428 put apis implicit deny #5450

benzekrimaha · 2023-11-21T11:02:45Z

PR opened after closing : #5325

Bucket policies are not correctly interpreted, this is part of the following epic to fix that: scality/Arsenal#2181

This PR is aiming to update put apis since object and bucket authorisations are made at API level and need to support implicit denies, ticket linked to this issue here : https://scality.atlassian.net/browse/CLDSRV-428

PRs providing implicit Deny logic to CS for processing in this PR
scality/Arsenal#2181
https://github.com/scality/Vault/pull/2135
#5322
#5420
#5432

Green CI for tests against zenko on 8.7 branch here : https://github.com/scality/Zenko/actions/runs/6956317229/job/18929846711
https://github.com/scality/Zenko/actions/runs/6967777856
https://github.com/scality/Zenko/actions/runs/6967837993/job/18962214972

I'll be bumping a new CLDSRV once the reviews done as these are changes that will be tested against Integration

(cherry picked from commit 8878814) (cherry picked from commit 4b3983f) (cherry picked from commit 62e15fa)

bert-e · 2023-11-21T11:02:48Z

Hello benzekrimaha,

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.

Status report is not available.

bert-e · 2023-11-21T11:02:54Z

Incorrect fix version

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

7.10.34

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

7.10.34
7.70.31
8.6.13
8.7.32
8.8.6

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

benzekrimaha · 2023-11-21T11:07:23Z

ping

bert-e · 2023-11-21T11:07:32Z

Request integration branches

Waiting for integration branch creation to be requested by the user.

To request integration branches, please comment on this pull request with the following command:

/create_integration_branches

Alternatively, the /approve and /create_pull_requests commands will automatically
create the integration branches.

benzekrimaha · 2023-11-21T12:32:43Z

/create_integration_branches

bert-e · 2023-11-21T12:32:53Z

Conflict

A conflict has been raised during the creation of
integration branch w/7.70/improvement/CLDSRV-428-put-apis-implicitDeny with contents from improvement/CLDSRV-428-put-apis-implicitDeny
and development/7.70.

I have not created the integration branch.

Here are the steps to resolve this conflict:

 $ git fetch
 $ git checkout -B w/7.70/improvement/CLDSRV-428-put-apis-implicitDeny origin/development/7.70
 $ git merge origin/improvement/CLDSRV-428-put-apis-implicitDeny
 $ # <intense conflict resolution>
 $ git commit
 $ git push -u origin w/7.70/improvement/CLDSRV-428-put-apis-implicitDeny

The following options are set: create_integration_branches

benzekrimaha · 2023-11-21T13:23:29Z

ping

bert-e · 2023-11-21T13:23:40Z

Conflict

A conflict has been raised during the creation of
integration branch w/8.6/improvement/CLDSRV-428-put-apis-implicitDeny with contents from w/7.70/improvement/CLDSRV-428-put-apis-implicitDeny
and development/8.6.

I have not created the integration branch.

Here are the steps to resolve this conflict:

 $ git fetch
 $ git checkout -B w/8.6/improvement/CLDSRV-428-put-apis-implicitDeny origin/development/8.6
 $ git merge origin/w/7.70/improvement/CLDSRV-428-put-apis-implicitDeny
 $ # <intense conflict resolution>
 $ git commit
 $ git push -u origin w/8.6/improvement/CLDSRV-428-put-apis-implicitDeny

The following options are set: create_integration_branches

benzekrimaha · 2023-11-21T14:35:42Z

ping

bert-e · 2023-11-21T14:35:54Z

Conflict

A conflict has been raised during the creation of
integration branch w/8.7/improvement/CLDSRV-428-put-apis-implicitDeny with contents from w/8.6/improvement/CLDSRV-428-put-apis-implicitDeny
and development/8.7.

I have not created the integration branch.

Here are the steps to resolve this conflict:

 $ git fetch
 $ git checkout -B w/8.7/improvement/CLDSRV-428-put-apis-implicitDeny origin/development/8.7
 $ git merge origin/w/8.6/improvement/CLDSRV-428-put-apis-implicitDeny
 $ # <intense conflict resolution>
 $ git commit
 $ git push -u origin w/8.7/improvement/CLDSRV-428-put-apis-implicitDeny

The following options are set: create_integration_branches

benzekrimaha · 2023-11-21T14:43:54Z

ping

bert-e · 2023-11-21T14:44:08Z

Integration data created

I have created the integration data for the additional destination branches.

this pull request will merge improvement/CLDSRV-428-put-apis-implicitDeny into
development/7.10
w/7.70/improvement/CLDSRV-428-put-apis-implicitDeny will be merged into development/7.70
w/8.6/improvement/CLDSRV-428-put-apis-implicitDeny will be merged into development/8.6
w/8.7/improvement/CLDSRV-428-put-apis-implicitDeny will be merged into development/8.7
w/8.8/improvement/CLDSRV-428-put-apis-implicitDeny will be merged into development/8.8

The following branches will NOT be impacted:

development/7.4

You can set option create_pull_requests if you need me to create
integration pull requests in addition to integration branches, with:

@bert-e create_pull_requests

The following options are set: create_integration_branches

bert-e · 2023-11-21T14:44:11Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
2 peers

The following options are set: create_integration_branches

(cherry picked from commit 402078a) (cherry picked from commit fd691c6) (cherry picked from commit 78ec4b8)

bert-e · 2023-11-21T14:56:29Z

History mismatch

Merge commit #c55e153902f161d09b545e8717c26b4a5de6bad0 on the integration branch
w/7.70/improvement/CLDSRV-428-put-apis-implicitDeny is merging a branch which is neither the current
branch improvement/CLDSRV-428-put-apis-implicitDeny nor the development branch
development/7.70.

It is likely due to a rebase of the branch improvement/CLDSRV-428-put-apis-implicitDeny and the
merge is not possible until all related w/* branches are deleted or updated.

Please use the reset command to have me reinitialize these branches.

The following options are set: create_integration_branches

benzekrimaha · 2023-11-21T14:56:40Z

/reset

bert-e · 2023-11-21T14:56:50Z

Reset complete

I have successfully deleted this pull request's integration branches.

The following options are set: create_integration_branches

bert-e · 2023-11-21T14:57:01Z

Conflict

A conflict has been raised during the creation of
integration branch w/7.70/improvement/CLDSRV-428-put-apis-implicitDeny with contents from improvement/CLDSRV-428-put-apis-implicitDeny
and development/7.70.

I have not created the integration branch.

Here are the steps to resolve this conflict:

 $ git fetch
 $ git checkout -B w/7.70/improvement/CLDSRV-428-put-apis-implicitDeny origin/development/7.70
 $ git merge origin/improvement/CLDSRV-428-put-apis-implicitDeny
 $ # <intense conflict resolution>
 $ git commit
 $ git push -u origin w/7.70/improvement/CLDSRV-428-put-apis-implicitDeny

The following options are set: create_integration_branches

benzekrimaha · 2023-11-21T15:19:06Z

ping

bert-e · 2023-11-21T15:19:17Z

Conflict

A conflict has been raised during the creation of
integration branch w/8.6/improvement/CLDSRV-428-put-apis-implicitDeny with contents from w/7.70/improvement/CLDSRV-428-put-apis-implicitDeny
and development/8.6.

I have not created the integration branch.

Here are the steps to resolve this conflict:

 $ git fetch
 $ git checkout -B w/8.6/improvement/CLDSRV-428-put-apis-implicitDeny origin/development/8.6
 $ git merge origin/w/7.70/improvement/CLDSRV-428-put-apis-implicitDeny
 $ # <intense conflict resolution>
 $ git commit
 $ git push -u origin w/8.6/improvement/CLDSRV-428-put-apis-implicitDeny

The following options are set: create_integration_branches

benzekrimaha · 2023-11-21T15:47:14Z

ping

bert-e · 2023-11-21T15:47:26Z

Conflict

A conflict has been raised during the creation of
integration branch w/8.7/improvement/CLDSRV-428-put-apis-implicitDeny with contents from w/8.6/improvement/CLDSRV-428-put-apis-implicitDeny
and development/8.7.

I have not created the integration branch.

Here are the steps to resolve this conflict:

 $ git fetch
 $ git checkout -B w/8.7/improvement/CLDSRV-428-put-apis-implicitDeny origin/development/8.7
 $ git merge origin/w/8.6/improvement/CLDSRV-428-put-apis-implicitDeny
 $ # <intense conflict resolution>
 $ git commit
 $ git push -u origin w/8.7/improvement/CLDSRV-428-put-apis-implicitDeny

The following options are set: create_integration_branches

benzekrimaha · 2023-11-21T15:52:06Z

ping

bert-e · 2023-11-21T15:52:20Z

Integration data created

I have created the integration data for the additional destination branches.

this pull request will merge improvement/CLDSRV-428-put-apis-implicitDeny into
development/7.10
w/7.70/improvement/CLDSRV-428-put-apis-implicitDeny will be merged into development/7.70
w/8.6/improvement/CLDSRV-428-put-apis-implicitDeny will be merged into development/8.6
w/8.7/improvement/CLDSRV-428-put-apis-implicitDeny will be merged into development/8.7
w/8.8/improvement/CLDSRV-428-put-apis-implicitDeny will be merged into development/8.8

The following branches will NOT be impacted:

development/7.4

You can set option create_pull_requests if you need me to create
integration pull requests in addition to integration branches, with:

@bert-e create_pull_requests

The following options are set: create_integration_branches

bert-e · 2023-11-21T15:52:23Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
2 peers

The following options are set: create_integration_branches

benzekrimaha · 2023-11-22T13:10:24Z

@bert-e create_pull_requests

bert-e · 2023-11-22T13:10:44Z

Integration data created

I have created the integration data for the additional destination branches.

this pull request will merge improvement/CLDSRV-428-put-apis-implicitDeny into
development/7.10
w/7.70/improvement/CLDSRV-428-put-apis-implicitDeny will be merged into development/7.70 (pull request INTEGRATION [PR#5450 > development/7.70] Improvement/cldsrv 428 put apis implicit deny #5451)
w/8.6/improvement/CLDSRV-428-put-apis-implicitDeny will be merged into development/8.6 (pull request INTEGRATION [PR#5450 > development/8.6] Improvement/cldsrv 428 put apis implicit deny #5452)
w/8.7/improvement/CLDSRV-428-put-apis-implicitDeny will be merged into development/8.7 (pull request INTEGRATION [PR#5450 > development/8.7] Improvement/cldsrv 428 put apis implicit deny #5453)
w/8.8/improvement/CLDSRV-428-put-apis-implicitDeny will be merged into development/8.8 (pull request INTEGRATION [PR#5450 > development/8.8] Improvement/cldsrv 428 put apis implicit deny #5454)

The following branches will NOT be impacted:

development/7.4

Follow integration pull requests if you would like to be notified of
build statuses by email.

The following options are set: create_pull_requests, create_integration_branches

bert-e · 2023-11-22T13:10:47Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
2 peers

The following options are set: create_pull_requests, create_integration_branches

KazToozs

With context that the 7.70 apis are being added seperately, looks good to me

anurag4DSB

It is complicated to review the PR due to the linter changes.
Would you be able to help point out in the tests which tests actually have logic changes and need review?

Other than that some over all logic looks good.

Lastly, the commit messages need to be self explanatory and it should be a commit story. Here we have a large commit cherry-picked and then a "fixups" commit. Once this is merged, looking back in the commit history it will be extremely difficult to debug.

If we are making 1 to 2 big commit, I would recommend removing all linter changes and only have logical changes or having a commit story with short logical commits.

anurag4DSB · 2023-11-23T19:24:23Z

lib/metadata/metadataUtils.js

+    if (!Array.isArray(requestType)) {
+        requestType = [requestType];
+    }
+    async.waterfall([


Ideally, we should add a return here to make sure event loop execution as per our need.

done here : https://github.com/scality/cloudserver/pull/5456/files#diff-459659e46eee4c3ab38fd506823fd48fbccb62998dfb6d110d4bb7cf7c1ee036R236

lib/api/bucketPutObjectLock.js

anurag4DSB · 2023-11-23T19:37:06Z

lib/metadata/metadataUtils.js

+    }
+    return null;
+}
+

 /** metadataValidateBucketAndObj - retrieve bucket and object md from metadata


NIT: JS doc needs an update as well.

done here : https://github.com/scality/cloudserver/pull/5456/files#diff-459659e46eee4c3ab38fd506823fd48fbccb62998dfb6d110d4bb7cf7c1ee036R216
and here : https://github.com/scality/cloudserver/pull/5456/files#diff-459659e46eee4c3ab38fd506823fd48fbccb62998dfb6d110d4bb7cf7c1ee036R323

anurag4DSB · 2023-11-23T19:49:26Z

lib/api/objectPutCopyPart.js

@@ -249,6 +243,9 @@ function objectPutCopyPart(authInfo, request, sourceBucket,
            splitter,
            next,
        ) {
+            const originalIdentityAuthzResults = request.actionImplicitDenies;
+            // eslint-disable-next-line no-param-reassign


Why are we disabling this linter rule eslint-disable-next-line no-param-reassign?

Given the asynchronous nature of the operations and the manipulation of shared objects (like request), there's a risk of race conditions. Care should be taken to ensure that the shared state is managed safely.

same for other places.

The request object is unique per API call so there's no risk of race conditions accessing it so I think that we can still delete on the request object.

anurag4DSB · 2023-11-23T19:54:05Z

lib/api/objectPutRetention.js

        },
+        (retentionInfo, next) => standardMetadataValidateBucketAndObj(metadataValParams, request.actionImplicitDenies,


Question: Why did we change the sequence here?
Do we know why it was the other way around before?

anurag4DSB · 2023-11-23T20:02:06Z

tests/multipleBackend/objectPutPart.js

@@ -3,20 +3,17 @@ const async = require('async');
 const crypto = require('crypto');
 const { parseString } = require('xml2js');
 const AWS = require('aws-sdk');
+const { metadata } = require('arsenal').storage.metadata.inMemory.metadata;


adding comment here as well for shorthand
but might be at other places.

bert-e · 2023-11-23T20:12:14Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
2 peers

The following reviewers are expecting changes from the author, or must review again:

@anurag4DSB

The following options are set: create_pull_requests, create_integration_branches

anurag4DSB · 2023-11-23T20:20:41Z

Adding integration PR reveiw here:
https://github.com/scality/cloudserver/pull/5451/commits the 4th commit message should prefixed with a ticket number "fix lint"
If this linter is required by the CI then its okay to add.

I haven't reviewed other integration branches, as I am uncertain which merge commit has extra changes with all the linter changes. Removing the linters will help with that, or I need your help in pointing out which merge commits have more logic changes. I am thinking of changes which might have taken place during conflicts (if any)

tests/unit/api/objectPutTagging.js

lib/api/objectPutACL.js

williamlardier · 2023-11-23T18:47:56Z

lib/api/objectPut.js

@@ -58,7 +58,7 @@ function objectPut(authInfo, request, streamingV4Params, log, callback) {
    }
    const invalidSSEError = errors.InvalidArgument.customizeDescription(
        'The encryption method specified is not supported');
-    const requestType = 'objectPut';
+    const requestType = request.apiMethods || 'objectPut';


For consistency between the request property name and the defaults, we might want to set all defaults as arrays, even if we recreate one if needed later

in the standardMetadataValidateBucketAndObj function the requestType is set to an array if it's not the case , so the outcome will be the same

williamlardier · 2023-11-23T18:52:59Z

lib/api/objectPutPart.js

+            // eslint-disable-next-line no-param-reassign
+            delete request.actionImplicitDenies;


I have done it in the PoC but I'm not sure it's safe enough; if we make additional APIs with HTTP calls based on the current request object in any API, we might run into problems because of the new properties we've added for convenience.
Another approach I had in mind was to create a new parameter (or extend the authResult of the 'AuthInfo' class) for all APIs to transmit the authz results in a safe way, to avoid this kind of code. If not possible, we should at least store the field names in a globally-defined constant and use an helper to cleanup the request everytime we need to. And document it

As the request object is unique per api call this delete will not be impacting other API calls , how will the additional properties impact new API calls ?
For the change in authInfo this will alter all the changes that have already been made and merged for this epic.
Cc : @anurag4DSB

lib/metadata/metadataUtils.js

tests/multipleBackend/objectPutCopyPart.js

benzekrimaha · 2023-11-27T16:20:56Z

Closing this PR , a new one is opened here : #5456

CLDSRV-428: put apis updated for implicit deny

43f9c4a

(cherry picked from commit 8878814) (cherry picked from commit 4b3983f) (cherry picked from commit 62e15fa)

benzekrimaha force-pushed the improvement/CLDSRV-428-put-apis-implicitDeny branch from 9df29db to c55e153 Compare November 21, 2023 11:04

CLDSRV-428: Fixups for put apis for implicit deny

e590b47

(cherry picked from commit 402078a) (cherry picked from commit fd691c6) (cherry picked from commit 78ec4b8)

benzekrimaha force-pushed the improvement/CLDSRV-428-put-apis-implicitDeny branch from af38aae to e590b47 Compare November 21, 2023 14:56

benzekrimaha marked this pull request as ready for review November 22, 2023 13:10

bert-e mentioned this pull request Nov 22, 2023

INTEGRATION [PR#5450 > development/8.8] Improvement/cldsrv 428 put apis implicit deny #5454

Closed

benzekrimaha requested review from anurag4DSB and KazToozs November 22, 2023 13:11

KazToozs approved these changes Nov 23, 2023

View reviewed changes

anurag4DSB requested changes Nov 23, 2023

View reviewed changes

williamlardier reviewed Nov 23, 2023

View reviewed changes

benzekrimaha mentioned this pull request Nov 27, 2023

CLDSRV-428: put apis updated for implicit deny #5456

Merged

benzekrimaha closed this Nov 27, 2023

		},
		(retentionInfo, next) => standardMetadataValidateBucketAndObj(metadataValParams, request.actionImplicitDenies,

		// eslint-disable-next-line no-param-reassign
		delete request.actionImplicitDenies;

Improvement/cldsrv 428 put apis implicit deny #5450

Improvement/cldsrv 428 put apis implicit deny #5450

Conversation

benzekrimaha commented Nov 21, 2023 • edited Loading

bert-e commented Nov 21, 2023

Hello benzekrimaha,

bert-e commented Nov 21, 2023

Incorrect fix version

benzekrimaha commented Nov 21, 2023

bert-e commented Nov 21, 2023

Request integration branches

benzekrimaha commented Nov 21, 2023

bert-e commented Nov 21, 2023

Conflict

benzekrimaha commented Nov 21, 2023

bert-e commented Nov 21, 2023

Conflict

benzekrimaha commented Nov 21, 2023

bert-e commented Nov 21, 2023

Conflict

benzekrimaha commented Nov 21, 2023

bert-e commented Nov 21, 2023

Integration data created

bert-e commented Nov 21, 2023

Waiting for approval

bert-e commented Nov 21, 2023

History mismatch

benzekrimaha commented Nov 21, 2023

bert-e commented Nov 21, 2023

Reset complete

bert-e commented Nov 21, 2023

Conflict

benzekrimaha commented Nov 21, 2023

bert-e commented Nov 21, 2023

Conflict

benzekrimaha commented Nov 21, 2023

bert-e commented Nov 21, 2023

Conflict

benzekrimaha commented Nov 21, 2023

bert-e commented Nov 21, 2023

Integration data created

bert-e commented Nov 21, 2023

Waiting for approval

benzekrimaha commented Nov 22, 2023

bert-e commented Nov 22, 2023

Integration data created

bert-e commented Nov 22, 2023

Waiting for approval

KazToozs left a comment

Choose a reason for hiding this comment

anurag4DSB left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bert-e commented Nov 23, 2023

Waiting for approval

anurag4DSB commented Nov 23, 2023

Choose a reason for hiding this comment

benzekrimaha Nov 24, 2023 • edited Loading

Choose a reason for hiding this comment

williamlardier Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

benzekrimaha Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

benzekrimaha commented Nov 27, 2023

benzekrimaha commented Nov 21, 2023 •

edited

Loading

anurag4DSB left a comment •

edited

Loading

benzekrimaha Nov 24, 2023 •

edited

Loading

williamlardier Nov 23, 2023 •

edited

Loading

benzekrimaha Nov 27, 2023 •

edited

Loading