CLDSRV-674: handle cross-account bucket policies #5855
Conversation
Hello leif-scality,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Branches have divergedThis pull request's source branch To avoid any integration risks, please re-synchronize them using one of the
Note: If you choose to rebase, you may have to ask me to rebuild |
leif-scality
force-pushed
the
bugfix/CLDSRV-674
branch
2 times, most recently
from
ccf75c1 to
b4639ec
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leif-scality
force-pushed
the
bugfix/CLDSRV-674
branch
from
b4639ec to
15d4bab
Compare
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leif-scality
force-pushed
the
bugfix/CLDSRV-674
branch
from
15d4bab to
1d88478
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| return false; | ||
| } | ||
|
|
||
| const checkPrincipalResult = { |
There was a problem hiding this comment.
| const checkPrincipalResult = { | |
| const checkPrincipalResult = Object.freeze({ |
| } | ||
|
|
||
| const checkBucketPolicyResult = { |
There was a problem hiding this comment.
| const checkBucketPolicyResult = { | |
| const checkBucketPolicyResult = Object.freeze({ |
(this is to ensure it will properly acts as an enum, as otherwise in js, we cannot protect against modification, just to be on the safe side...)
| for (const p of principal.CanonicalUser) { | ||
| if (out === checkPrincipalResult.OK) { | ||
| break; | ||
| } | ||
|
|
||
| const res = _checkPrincipal(canonicalID, p, bucketOwnerCanonicalID, canonicalID); | ||
| if (res !== checkPrincipalResult.KO) { | ||
| out = res; | ||
| } | ||
| } | ||
| return out; |
There was a problem hiding this comment.
to avoid nested if/for and duplication we should be able to use a helper function
something like:
function _findBestPrincipalMatch(principalArray, checkFunc) {
let bestMatch = checkPrincipalResult.KO;
if (!principalArray) {
return bestMatch;
}
const principals = Array.isArray(principalArray) ? principalArray : [principalArray];
for (const p of principals) {
const result = checkFunc(p);
if (result === checkPrincipalResult.OK) {
return checkPrincipalResult.OK; // Highest permission, can exit early
}
if (result > bestMatch) {
bestMatch = result;
}
}
return bestMatch;
}
function _checkPrincipals(canonicalID, arn, principal, bucketOwnerCanonicalID) {
if (principal === '*') {
// Handle anonymous or authenticated wildcard
if (arn === undefined) {
return checkPrincipalResult.OK;
}
return _getPermissionLevel(arn, bucketOwnerCanonicalID, canonicalID); // Assuming _getPermissionLevel is refactored
}
if (principal.CanonicalUser) {
return _findBestPrincipalMatch(principal.CanonicalUser, p =>
_checkPrincipal(canonicalID, p, bucketOwnerCanonicalID, canonicalID));
}
if (principal.AWS) {
return _findBestPrincipalMatch(principal.AWS, p =>
_checkPrincipal(arn, p, bucketOwnerCanonicalID, canonicalID));
}
return checkPrincipalResult.KO;
}| if (_getAccountId(requester) !== principal) { | ||
| return checkPrincipalResult.KO; | ||
| } | ||
|
|
||
| if (!_isRootUser(requester)) { | ||
| return bucketOwnerCanonicalID === requesterCanonicalID ? | ||
| checkPrincipalResult.OK : checkPrincipalResult.CROSS_ACCOUNT_OK; | ||
| } |
There was a problem hiding this comment.
This is duplicated multiple time, how about something like this?
function _getPermissionLevel(requester, bucketOwnerCanonicalID, requesterCanonicalID) {
if (_isRootUser(requester)) {
return checkPrincipalResult.OK;
}
return bucketOwnerCanonicalID === requesterCanonicalID
? checkPrincipalResult.OK
: checkPrincipalResult.CROSS_ACCOUNT_OK;
}
function _checkPrincipal(requester, principal, bucketOwnerCanonicalID, requesterCanonicalID) {
if (principal === '*') {
return _getPermissionLevel(requester, bucketOwnerCanonicalID, requesterCanonicalID);
}
if (requester === undefined) { // Unauthenticated user
return checkPrincipalResult.KO;
}
if (principal === requester) {
return _getPermissionLevel(requester, bucketOwnerCanonicalID, requesterCanonicalID);
}
if (_isAccountId(principal) && _getAccountId(requester) === principal) {
return _getPermissionLevel(requester, bucketOwnerCanonicalID, requesterCanonicalID);
}
if (principal.endsWith('root') && _getAccountId(requester) === _getAccountId(principal)) {
return _getPermissionLevel(requester, bucketOwnerCanonicalID, requesterCanonicalID);
}
return checkPrincipalResult.KO;
}There was a problem hiding this comment.
created one function for each principal type, and inlined the if statements
| @@ -117,7 +117,7 @@ function checkBucketAcls(bucket, requestType, canonicalID, mainApiCall) { | |||
| // authorization check should just return true so can move on to check | |||
| // rights at the object level. | |||
| return (requestTypeParsed === 'objectPutACL' || requestTypeParsed === 'objectGetACL' | |||
| || requestTypeParsed === 'objectGet' || requestTypeParsed === 'objectHead'); | |||
| || requestTypeParsed === 'objectGet' || requestTypeParsed === 'objectHead'); | |||
There was a problem hiding this comment.
Avoid linting / indent changes it might create many conflicts with the integration branches above that have different rules and might have already fixed it
There was a problem hiding this comment.
they were added by yarn run lint --fix, do I remove them anyway?
leif-scality
force-pushed
the
bugfix/CLDSRV-674
branch
from
1d88478 to
d4a82f0
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| return false; | ||
| } | ||
| if (principal === requester) { | ||
|
|
||
| // Vault returns the following arn when the account makes requests 'arn:aws:iam::295412255825:/master/', | ||
| // with an empty resource type ('user/' prefix missing). | ||
| const arns = arn.split(':'); | ||
| const resource = arns.at(-1); | ||
|
|
||
| // If we start with '/' is because we have a empty resource type so we know it is a root account. | ||
| if (resource.startsWith('/')) { | ||
| return true; | ||
| } | ||
| if (_isAccountId(principal)) { | ||
| return _getAccountId(requester) === principal; | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| const checkPrincipalResult = Object.freeze({ |
There was a problem hiding this comment.
Nit: better to define consts at the top of the file for readability
| function _checkCrossAccount(requesterARN, requesterCanonicalID, bucketOwnerCanonicalID) { | ||
| if (!_isRootUser(requesterARN)) { |
There was a problem hiding this comment.
for comments consistency:
| function _checkCrossAccount(requesterARN, requesterCanonicalID, bucketOwnerCanonicalID) { | |
| if (!_isRootUser(requesterARN)) { | |
| function _checkCrossAccount(requesterARN, requesterCanonicalID, bucketOwnerCanonicalID) { | |
| // Vault returns ARNs like 'arn:aws:iam::123456789012:/master/' for root accounts | |
| // with an empty resource type (missing 'user/' prefix) | |
| if (!_isRootUser(requesterARN)) { |
|
|
||
| // Vault returns the following arn when the account makes requests 'arn:aws:iam::295412255825:/master/', | ||
| // with an empty resource type ('user/' prefix missing). | ||
| const arns = arn.split(':'); | ||
| const resource = arns.at(-1); | ||
|
|
||
| // If we start with '/' is because we have a empty resource type so we know it is a root account. | ||
| if (resource.startsWith('/')) { |
There was a problem hiding this comment.
Missing protection against smaller arns:
| // Vault returns the following arn when the account makes requests 'arn:aws:iam::295412255825:/master/', | |
| // with an empty resource type ('user/' prefix missing). | |
| const arns = arn.split(':'); | |
| const resource = arns.at(-1); | |
| // If we start with '/' is because we have a empty resource type so we know it is a root account. | |
| if (resource.startsWith('/')) { | |
| // Vault returns the following arn when the account makes requests 'arn:aws:iam::295412255825:/master/', | |
| // with an empty resource type ('user/' prefix missing). | |
| const arns = arn.split(':'); | |
| const resource = arns.at(-1); | |
| if (arns.length < 6) { | |
| return false; | |
| } | |
| // If we start with '/' is because we have a empty resource type so we know it is a root account. | |
| if (resource.startsWith('/')) { |
| // Vault returns the following arn when the account makes requests 'arn:aws:iam::295412255825:/master/', | ||
| // with an empty resource type ('user/' prefix missing). | ||
| const arns = arn.split(':'); | ||
| const resource = arns.at(-1); |
There was a problem hiding this comment.
Just a not on the .at, it seems this is quite recent (es2022), as you target the 7.70 branch, we will want to be careful if the change is later backported to a hotfix branch, that might still use an older nodejs version, as if I'm not mistaken, this one was introduced in later v16 nodejs version, it's definitely not in the first v16 versions.
There was a problem hiding this comment.
replace at with arns[arns.length - 1]
|
@williamlardier I kept the diagram because the one in citadel is higher level than this one, I also think the diagram makes the function easier to understand. |
| } | ||
|
|
||
| // checkBucketPolicy FSM. |
There was a problem hiding this comment.
Finite state machine
| const ok = principalMatch === checkPrincipalResult.OK && actionMatch && resourceMatch && conditionsMatch; | ||
| const okCross = principalMatch === checkPrincipalResult.CROSS_ACCOUNT_OK | ||
| && actionMatch && resourceMatch && conditionsMatch; | ||
| switch (permission) { | ||
| case checkBucketPolicyResult.DEFAULT_DENY: | ||
| if ((ok || okCross) && s.Effect === 'Deny') { | ||
| return checkBucketPolicyResult.EXPLICIT_DENY; | ||
| } else if (ok && s.Effect === 'Allow') { | ||
| permission = checkBucketPolicyResult.ALLOW; | ||
| } else if (okCross && s.Effect === 'Allow') { | ||
| permission = checkBucketPolicyResult.CROSS_ACCOUNT_ALLOW; | ||
| } |
There was a problem hiding this comment.
| const ok = principalMatch === checkPrincipalResult.OK && actionMatch && resourceMatch && conditionsMatch; | |
| const okCross = principalMatch === checkPrincipalResult.CROSS_ACCOUNT_OK | |
| && actionMatch && resourceMatch && conditionsMatch; | |
| switch (permission) { | |
| case checkBucketPolicyResult.DEFAULT_DENY: | |
| if ((ok || okCross) && s.Effect === 'Deny') { | |
| return checkBucketPolicyResult.EXPLICIT_DENY; | |
| } else if (ok && s.Effect === 'Allow') { | |
| permission = checkBucketPolicyResult.ALLOW; | |
| } else if (okCross && s.Effect === 'Allow') { | |
| permission = checkBucketPolicyResult.CROSS_ACCOUNT_ALLOW; | |
| } | |
| const ok = principalMatch === checkPrincipalResult.OK && actionMatch && resourceMatch && conditionsMatch; | |
| const okCross = principalMatch === checkPrincipalResult.CROSS_ACCOUNT_OK | |
| && actionMatch && resourceMatch && conditionsMatch; | |
| const anyOk = ok || okCross; | |
| switch (permission) { | |
| case checkBucketPolicyResult.DEFAULT_DENY: | |
| if (anyOk && s.Effect === 'Deny') { | |
| return checkBucketPolicyResult.EXPLICIT_DENY; | |
| } else if (ok && s.Effect === 'Allow') { | |
| permission = checkBucketPolicyResult.ALLOW; | |
| } else if (okCross && s.Effect === 'Allow') { | |
| permission = checkBucketPolicyResult.CROSS_ACCOUNT_ALLOW; | |
| } |
to avoid the multiple || if you want
| return _checkCrossAccount(requesterARN, requesterCanonicalID, bucketOwnerCanonicalID); | ||
| } | ||
|
|
||
| if (principal.endsWith('root') && _getAccountId(principal) === _getAccountId(requesterARN)) { |
There was a problem hiding this comment.
Shouldn't it be /root to avoid valid user names ending in root?
| if (principal.endsWith('root') && _getAccountId(principal) === _getAccountId(requesterARN)) { | |
| if (principal.endsWith('/root') && _getAccountId(principal) === _getAccountId(requesterARN)) { |
maybe a more formal check here would be better
There was a problem hiding this comment.
I think we should do :root, /root is not a valid ARN, /account_name is only returned by vault when using the root access key's as explained in _IsRoot.
https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_principal.html#principal-accounts
leif-scality
force-pushed
the
bugfix/CLDSRV-674
branch
from
f97e991 to
969e154
Compare
Handle cross account cases when using bucket policies
https://scality.atlassian.net/browse/S3C-9896