Compare commits

...

13 Commits

Author SHA1 Message Date
Will Toozs 970de50c69
Merge remote-tracking branch 'origin/w/7.70/improvement/CLDSRV-436-bp-conditions' into w/8.6/improvement/CLDSRV-436-bp-conditions 2023-12-15 10:48:35 +01:00
Will Toozs 7b5ba98a57
Merge remote-tracking branch 'origin/improvement/CLDSRV-436-bp-conditions' into w/7.70/improvement/CLDSRV-436-bp-conditions 2023-12-15 10:31:07 +01:00
Will Toozs 329dbcc4dc
fixup: update test 2023-12-14 19:13:09 +01:00
Will Toozs be51322c79
fixup: lint 2023-12-14 19:04:00 +01:00
Will Toozs 718468f53b
fixup: another test 2023-12-14 18:55:17 +01:00
Will Toozs 452878deec
fixup: update and improve tests with validation logic 2023-12-14 18:50:12 +01:00
Will Toozs 7ce2e6fc44
CLDSRV-436: add new policy condition validation logic 2023-12-14 18:49:22 +01:00
Will Toozs 40e4019ac3
fixup: lint 2023-12-13 17:24:25 +01:00
Will Toozs b11f3cfc5d
CLDSRV-436: update put object retention API 2023-12-13 17:18:59 +01:00
Will Toozs 4ea06f22ed
fixup: permchecks requirements 2023-12-13 17:09:55 +01:00
Will Toozs 133c6f05f0
CLDSRV-436: update PutPolicy tests 2023-12-13 17:09:55 +01:00
Will Toozs f8fe00e114
CLDSRV-436: update PutPolicy logic 2023-12-13 17:09:54 +01:00
Will Toozs a1839e36ea
CLDSRV-436: add conditions logic 2023-12-13 17:09:49 +01:00
5 changed files with 250 additions and 17 deletions

View File

@ -1,5 +1,6 @@
const { evaluators, actionMaps, RequestContext } = require('arsenal').policies; const { evaluators, actionMaps, RequestContext, requestUtils } = require('arsenal').policies;
const constants = require('../../../../constants'); const constants = require('../../../../constants');
const { config } = require('../../../Config');
const { allAuthedUsersId, bucketOwnerActions, logId, publicId, arrayOfAllowed } = constants; const { allAuthedUsersId, bucketOwnerActions, logId, publicId, arrayOfAllowed } = constants;
@ -125,7 +126,7 @@ function checkBucketAcls(bucket, requestType, canonicalID, mainApiCall) {
// authorization check should just return true so can move on to check // authorization check should just return true so can move on to check
// rights at the object level. // rights at the object level.
return (requestTypeParsed === 'objectPutACL' || requestTypeParsed === 'objectGetACL' return (requestTypeParsed === 'objectPutACL' || requestTypeParsed === 'objectGetACL'
|| requestTypeParsed === 'objectGet' || requestTypeParsed === 'objectHead'); || requestTypeParsed === 'objectGet' || requestTypeParsed === 'objectHead');
} }
function checkObjectAcls(bucket, objectMD, requestType, canonicalID, requesterIsNotUser, function checkObjectAcls(bucket, objectMD, requestType, canonicalID, requesterIsNotUser,
@ -246,6 +247,20 @@ function _checkBucketPolicyResources(request, resource, log) {
return evaluators.isResourceApplicable(requestContext, resource, log); return evaluators.isResourceApplicable(requestContext, resource, log);
} }
function _checkBucketPolicyConditions(request, conditions, log) {
const ip = request ? requestUtils.getClientIp(request, config) : undefined;
if (!conditions) {
return true;
}
// build request context from the request!
const requestContext = new RequestContext(request.headers, request.query,
request.bucketName, request.objectKey, ip,
request.connection.encrypted, request.resourceType, 's3', null, null,
null, null, null, null, null, null, null, null, null,
request.objectLockRetentionDays);
return evaluators.meetConditions(requestContext, conditions, log);
}
function _getAccountId(arn) { function _getAccountId(arn) {
// account or user arn is of format 'arn:aws:iam::<12-digit-acct-id>:etc... // account or user arn is of format 'arn:aws:iam::<12-digit-acct-id>:etc...
return arn.substr(13, 12); return arn.substr(13, 12);
@ -303,12 +318,13 @@ function checkBucketPolicy(policy, requestType, canonicalID, arn, bucketOwner, l
const principalMatch = _checkPrincipals(canonicalID, arn, s.Principal); const principalMatch = _checkPrincipals(canonicalID, arn, s.Principal);
const actionMatch = _checkBucketPolicyActions(requestType, s.Action, log); const actionMatch = _checkBucketPolicyActions(requestType, s.Action, log);
const resourceMatch = _checkBucketPolicyResources(request, s.Resource, log); const resourceMatch = _checkBucketPolicyResources(request, s.Resource, log);
const conditionsMatch = _checkBucketPolicyConditions(request, s.Condition, log);
if (principalMatch && actionMatch && resourceMatch && s.Effect === 'Deny') { if (principalMatch && actionMatch && resourceMatch && conditionsMatch && s.Effect === 'Deny') {
// explicit deny trumps any allows, so return immediately // explicit deny trumps any allows, so return immediately
return 'explicitDeny'; return 'explicitDeny';
} }
if (principalMatch && actionMatch && resourceMatch && s.Effect === 'Allow') { if (principalMatch && actionMatch && resourceMatch && conditionsMatch && s.Effect === 'Allow') {
permission = 'allow'; permission = 'allow';
} }
copiedStatement = copiedStatement.splice(1); copiedStatement = copiedStatement.splice(1);
@ -381,7 +397,7 @@ function evaluateBucketPolicyWithIAM(bucket, requestTypesInput, canonicalID, aut
arn = authInfo.getArn(); arn = authInfo.getArn();
} }
return processBucketPolicy(_requestType, bucket, canonicalID, arn, bucket.getOwner(), log, return processBucketPolicy(_requestType, bucket, canonicalID, arn, bucket.getOwner(), log,
request, true, results, actionImplicitDenies); request, true, results, actionImplicitDenies);
}); });
} }
@ -399,8 +415,8 @@ function isObjAuthorized(bucket, objectMD, requestTypesInput, canonicalID, authI
? _requestType.slice(0, -7) : _requestType; ? _requestType.slice(0, -7) : _requestType;
const bucketOwner = bucket.getOwner(); const bucketOwner = bucket.getOwner();
if (!objectMD) { if (!objectMD) {
// User is already authorized on the bucket for FULL_CONTROL or WRITE or // User is already authorized on the bucket for FULL_CONTROL or WRITE or
// bucket has canned ACL public-read-write // bucket has canned ACL public-read-write
if (parsedMethodName === 'objectPut' || parsedMethodName === 'objectDelete') { if (parsedMethodName === 'objectPut' || parsedMethodName === 'objectDelete') {
results[_requestType] = actionImplicitDenies[_requestType] === false; results[_requestType] = actionImplicitDenies[_requestType] === false;
return results[_requestType]; return results[_requestType];
@ -428,8 +444,8 @@ function isObjAuthorized(bucket, objectMD, requestTypesInput, canonicalID, authI
// - account is the bucket owner // - account is the bucket owner
// - requester is account, not user // - requester is account, not user
if (bucketOwnerActions.includes(parsedMethodName) if (bucketOwnerActions.includes(parsedMethodName)
&& (bucketOwner === canonicalID) && (bucketOwner === canonicalID)
&& requesterIsNotUser) { && requesterIsNotUser) {
results[_requestType] = actionImplicitDenies[_requestType] === false; results[_requestType] = actionImplicitDenies[_requestType] === false;
return results[_requestType]; return results[_requestType];
} }
@ -466,6 +482,78 @@ function validatePolicyResource(bucketName, policy) {
}); });
} }
function checkIp(value) {
const isValid = /^(\d{1,3}\.){3}\d{1,3}(\/\d{1,2})?$/.test(value);
if (isValid) {
return null;
}
return 'Invalid IP address in Conditions';
}
// This function checks all bucket policy conditions if the values provided
// are valid for the condition type. If not it returns a relevant Malformed policy error string
function validatePolicyConditions(policy) {
const validConditions = [
{ conditionKey: 'aws:SourceIp', conditionValueTypeChecker: checkIp },
{ conditionKey: 's3:object-lock-remaining-retention-days' },
];
// keys where value type does not seem to be checked by AWS:
// - s3:object-lock-remaining-retention-days
// there can be multiple statements in the policy, each with a Condition enclosure
for (let i = 0; i < policy.Statement.length; i++) {
const s = policy.Statement[i];
if (s.Condition) {
const conditionOperators = Object.keys(s.Condition);
// there can be multiple condition operations in the Condition enclosure
for (let j = 0; j < conditionOperators.length; j++) {
const conditionOperator = conditionOperators[j];
const conditionKey = Object.keys(s.Condition[conditionOperator])[0];
const conditionValue = s.Condition[conditionOperator][conditionKey];
const validCondition = validConditions.find(validCondition =>
validCondition.conditionKey === conditionKey
);
// this is the seen behaviour on AWS... don't ask me why
if (!validCondition && !conditionKey.startsWith('aws:')) {
return 'Policy has an invalid condition key';
}
let conditionValueTypeError;
if (validCondition && validCondition.conditionValueTypeChecker) {
conditionValueTypeError = validCondition.conditionValueTypeChecker(conditionValue);
}
if (conditionValueTypeError) {
return conditionValueTypeError;
}
}
}
}
return null;
}
/** isLifecycleSession - check if it is the Lifecycle assumed role session arn.
* @param {string} arn - Amazon resource name - example:
* arn:aws:sts::257038443293:assumed-role/rolename/backbeat-lifecycle
* @return {boolean} true if Lifecycle assumed role session arn, false if not.
*/
function isLifecycleSession(arn) {
if (!arn) {
return false;
}
const arnSplits = arn.split(':');
const service = arnSplits[2];
const resourceNames = arnSplits[arnSplits.length - 1].split('/');
const resourceType = resourceNames[0];
const sessionName = resourceNames[resourceNames.length - 1];
return (service === 'sts'
&& resourceType === assumedRoleArnResourceType
&& sessionName === backbeatLifecycleSessionName);
}
module.exports = { module.exports = {
isBucketAuthorized, isBucketAuthorized,
isObjAuthorized, isObjAuthorized,
@ -476,5 +564,7 @@ module.exports = {
checkBucketAcls, checkBucketAcls,
checkObjectAcls, checkObjectAcls,
validatePolicyResource, validatePolicyResource,
validatePolicyConditions,
isLifecycleSession,
evaluateBucketPolicyWithIAM, evaluateBucketPolicyWithIAM,
}; };

View File

@ -4,7 +4,7 @@ const { errors, models } = require('arsenal');
const collectCorsHeaders = require('../utilities/collectCorsHeaders'); const collectCorsHeaders = require('../utilities/collectCorsHeaders');
const metadata = require('../metadata/wrapper'); const metadata = require('../metadata/wrapper');
const { standardMetadataValidateBucket } = require('../metadata/metadataUtils'); const { standardMetadataValidateBucket } = require('../metadata/metadataUtils');
const { validatePolicyResource } = const { validatePolicyResource, validatePolicyConditions } =
require('./apiUtils/authorization/permissionChecks'); require('./apiUtils/authorization/permissionChecks');
const { BucketPolicy } = models; const { BucketPolicy } = models;
@ -17,9 +17,8 @@ const { BucketPolicy } = models;
function _checkNotImplementedPolicy(policyString) { function _checkNotImplementedPolicy(policyString) {
// bucket names and key names cannot include "", so including those // bucket names and key names cannot include "", so including those
// isolates not implemented keys // isolates not implemented keys
return policyString.includes('"Condition"') return policyString.includes('"Service"')
|| policyString.includes('"Service"') || policyString.includes('"Federated"');
|| policyString.includes('"Federated"');
} }
/** /**
@ -70,6 +69,16 @@ function bucketPutPolicy(authInfo, request, log, callback) {
return next(null, bucketPolicy); return next(null, bucketPolicy);
}); });
}, },
(bucketPolicy, next) => {
process.nextTick(() => {
// if policy contains conditions, validate them, and return relevant error if there is one
const conditionsError = validatePolicyConditions(bucketPolicy);
if (conditionsError) {
return next(errors.MalformedPolicy.customizeDescription(conditionsError));
}
return next(null, bucketPolicy);
});
},
(bucketPolicy, next) => standardMetadataValidateBucket(metadataValParams, request.actionImplicitDenies, log, (bucketPolicy, next) => standardMetadataValidateBucket(metadataValParams, request.actionImplicitDenies, log,
(err, bucket) => { (err, bucket) => {
if (err) { if (err) {

View File

@ -83,8 +83,46 @@ function objectPutRetention(authInfo, request, log, callback) {
(bucket, objectMD, next) => { (bucket, objectMD, next) => {
log.trace('parsing retention information'); log.trace('parsing retention information');
parseRetentionXml(request.post, log, parseRetentionXml(request.post, log,
(err, retentionInfo) => next(err, bucket, retentionInfo, objectMD)); (err, retentionInfo) => {
if (err) {
log.trace('error parsing retention information',
{ error: err });
return next(err);
}
const remainingDays = Math.ceil(
(new Date(retentionInfo.date) - Date.now()) / (1000 * 3600 * 24));
metadataValParams.request.objectLockRetentionDays = remainingDays;
return next(null, retentionInfo);
});
}, },
(retentionInfo, next) => standardMetadataValidateBucketAndObj(metadataValParams,
request.actionImplicitDenies, log, (err, bucket, objectMD) => {
if (err) {
log.trace('request authorization failed',
{ method: 'objectPutRetention', error: err });
return next(err);
}
if (!objectMD) {
const err = reqVersionId ? errors.NoSuchVersion :
errors.NoSuchKey;
log.trace('error no object metadata found',
{ method: 'objectPutRetention', error: err });
return next(err, bucket);
}
if (objectMD.isDeleteMarker) {
log.trace('version is a delete marker',
{ method: 'objectPutRetention' });
return next(errors.MethodNotAllowed, bucket);
}
if (!bucket.isObjectLockEnabled()) {
log.trace('object lock not enabled on bucket',
{ method: 'objectPutRetention' });
return next(errors.InvalidRequest.customizeDescription(
'Bucket is missing Object Lock Configuration'
), bucket);
}
return next(null, bucket, retentionInfo, objectMD);
}),
(bucket, retentionInfo, objectMD, next) => { (bucket, retentionInfo, objectMD, next) => {
const hasGovernanceBypass = hasGovernanceBypassHeader(request.headers); const hasGovernanceBypass = hasGovernanceBypassHeader(request.headers);
if (hasGovernanceBypass && isRequesterNonAccountUser(authInfo)) { if (hasGovernanceBypass && isRequesterNonAccountUser(authInfo)) {

View File

@ -156,5 +156,101 @@ describe('aws-sdk test put bucket policy', () => {
s3.putBucketPolicy(params, err => s3.putBucketPolicy(params, err =>
assertError(err, 'MalformedPolicy', done)); assertError(err, 'MalformedPolicy', done));
}); });
it('should allow bucket policy with valid SourceIp condition', done => {
const params = getPolicyParams({
key: 'Condition', value: {
IpAddress: {
'aws:SourceIp': '192.168.100.0/24',
},
},
});
s3.putBucketPolicy(params, err => assertError(err, null, done));
});
it('should not allow bucket policy with invalid SourceIp format', done => {
const params = getPolicyParams({
key: 'Condition', value: {
IpAddress: {
'aws:SourceIp': '192.168.100', // Invalid IP format
},
},
});
s3.putBucketPolicy(params, err => assertError(err, 'MalformedPolicy', done));
});
it('should allow bucket policy with valid s3:object-lock-remaining-retention-days condition', done => {
const params = getPolicyParams({
key: 'Condition', value: {
NumericGreaterThanEquals: {
's3:object-lock-remaining-retention-days': '30',
},
},
});
s3.putBucketPolicy(params, err => assertError(err, null, done));
});
// yep, this is the expected behaviour
it('should not reject policy with invalid s3:object-lock-remaining-retention-days value', done => {
const params = getPolicyParams({
key: 'Condition', value: {
NumericGreaterThanEquals: {
's3:object-lock-remaining-retention-days': '-1', // Invalid value
},
},
});
s3.putBucketPolicy(params, err => assertError(err, null, done));
});
// this too ¯\_(ツ)_/¯
it('should not reject policy with a key starting with aws:', done => {
const params = getPolicyParams({
key: 'Condition', value: {
NumericGreaterThanEquals: {
'aws:have-a-nice-day': 'blabla', // Invalid value
},
},
});
s3.putBucketPolicy(params, err => assertError(err, null, done));
});
it('should reject policy with a key that does not exist that does not start with aws:', done => {
const params = getPolicyParams({
key: 'Condition', value: {
NumericGreaterThanEquals: {
'have-a-nice-day': 'blabla', // Invalid value
},
},
});
s3.putBucketPolicy(params, err => assertError(err, 'MalformedPolicy', done));
});
it('should enforce policies with both SourceIp and s3:object-lock conditions together', done => {
const params = getPolicyParams({
key: 'Condition', value: {
IpAddress: {
'aws:SourceIp': '192.168.100.0/24',
},
NumericGreaterThanEquals: {
's3:object-lock-remaining-retention-days': '30',
},
},
});
s3.putBucketPolicy(params, err => assertError(err, null, done));
});
it('should return error if a condition one of the condition values is invalid', done => {
const params = getPolicyParams({
key: 'Condition', value: {
IpAddress: {
'aws:SourceIp': '192.168.100',
},
NumericGreaterThanEquals: {
's3:object-lock-remaining-retention-days': '30',
},
},
});
s3.putBucketPolicy(params, err => assertError(err, 'MalformedPolicy', done));
});
}); });
}); });

View File

@ -78,12 +78,12 @@ describe('putBucketPolicy API', () => {
}); });
}); });
it('should return error if policy contains conditions', done => { it('should not return error if policy contains conditions', done => {
expectedBucketPolicy.Statement[0].Condition = expectedBucketPolicy.Statement[0].Condition =
{ StringEquals: { 's3:x-amz-acl': ['public-read'] } }; { IpAddress: { 'aws:SourceIp': '123.123.123.123' } };
bucketPutPolicy(authInfo, getPolicyRequest(expectedBucketPolicy), log, bucketPutPolicy(authInfo, getPolicyRequest(expectedBucketPolicy), log,
err => { err => {
assert.strictEqual(err.is.NotImplemented, true); assert.ifError(err);
done(); done();
}); });
}); });