fixup: update test

lib/api/apiUtils/authorization/permissionChecks.js

@@ -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,
 };

lib/api/bucketPutPolicy.js

@@ -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) {

lib/api/objectPutRetention.js

@@ -47,8 +47,23 @@ function objectPutRetention(authInfo, request, log, callback) {
     };
 
     return async.waterfall([
-        next => standardMetadataValidateBucketAndObj(metadataValParams, request.actionImplicitDenies, log,
-        (err, bucket, objectMD) => {
+        next => {
+            log.trace('parsing retention information');
+            parseRetentionXml(request.post, log,
+                (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 });
@@ -73,13 +88,8 @@ function objectPutRetention(authInfo, request, log, callback) {
                         'Bucket is missing Object Lock Configuration'
                     ), bucket);
                 }
-            return next(null, bucket, objectMD);
+                return next(null, bucket, retentionInfo, objectMD);
             }),
-        (bucket, objectMD, next) => {
-            log.trace('parsing retention information');
-            parseRetentionXml(request.post, log,
-                (err, retentionInfo) => next(err, bucket, retentionInfo, objectMD));
-        },
         (bucket, retentionInfo, objectMD, next) => {
             const hasGovernanceBypass = hasGovernanceBypassHeader(request.headers);
             if (hasGovernanceBypass && authInfo.isRequesterAnIAMUser()) {

tests/functional/aws-node-sdk/test/bucket/putBucketPolicy.js

@@ -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));
+        });
     });
 });

tests/unit/api/bucketPutPolicy.js

@@ -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();
         });
     });