Compare commits

...

12 Commits

Author SHA1 Message Date
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 226 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 { config } = require('../../../Config');
const {
allAuthedUsersId, bucketOwnerActions, logId, publicId,
@ -230,6 +231,20 @@ function _checkBucketPolicyResources(request, 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) {
// account or user arn is of format 'arn:aws:iam::<12-digit-acct-id>:etc...
return arn.substr(13, 12);
@ -287,12 +302,13 @@ function checkBucketPolicy(policy, requestType, canonicalID, arn, bucketOwner, l
const principalMatch = _checkPrincipals(canonicalID, arn, s.Principal);
const actionMatch = _checkBucketPolicyActions(requestType, s.Action, 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
return 'explicitDeny';
}
if (principalMatch && actionMatch && resourceMatch && s.Effect === 'Allow') {
if (principalMatch && actionMatch && resourceMatch && conditionsMatch && s.Effect === 'Allow') {
permission = 'allow';
}
copiedStatement = copiedStatement.splice(1);
@ -450,6 +466,55 @@ 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
@ -479,6 +544,7 @@ module.exports = {
checkBucketAcls,
checkObjectAcls,
validatePolicyResource,
validatePolicyConditions,
isLifecycleSession,
evaluateBucketPolicyWithIAM,
};

View File

@ -4,7 +4,7 @@ const { errors, models } = require('arsenal');
const collectCorsHeaders = require('../utilities/collectCorsHeaders');
const metadata = require('../metadata/wrapper');
const { standardMetadataValidateBucket } = require('../metadata/metadataUtils');
const { validatePolicyResource } =
const { validatePolicyResource, validatePolicyConditions } =
require('./apiUtils/authorization/permissionChecks');
const { BucketPolicy } = models;
@ -17,8 +17,7 @@ const { BucketPolicy } = models;
function _checkNotImplementedPolicy(policyString) {
// bucket names and key names cannot include "", so including those
// isolates not implemented keys
return policyString.includes('"Condition"')
|| policyString.includes('"Service"')
return policyString.includes('"Service"')
|| policyString.includes('"Federated"');
}
@ -70,6 +69,16 @@ function bucketPutPolicy(authInfo, request, log, callback) {
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,
(err, bucket) => {
if (err) {

View File

@ -82,8 +82,46 @@ function objectPutRetention(authInfo, request, log, callback) {
(bucket, objectMD, next) => {
log.trace('parsing retention information');
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) => {
const hasGovernanceBypass = hasGovernanceBypassHeader(request.headers);
if (hasGovernanceBypass && authInfo.isRequesterAnIAMUser()) {

View File

@ -156,5 +156,101 @@ describe('aws-sdk test put bucket policy', () => {
s3.putBucketPolicy(params, err =>
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 =
{ StringEquals: { 's3:x-amz-acl': ['public-read'] } };
{ IpAddress: { 'aws:SourceIp': '123.123.123.123' } };
bucketPutPolicy(authInfo, getPolicyRequest(expectedBucketPolicy), log,
err => {
assert.strictEqual(err.is.NotImplemented, true);
assert.ifError(err);
done();
});
});