Merge pull request #907 from scality/bugfix/S3C-2489-cherry-pick-bucket-policies

Bugfix/s3 c 2489 cherry pick bucket policies

index.js

@@ -107,6 +107,7 @@ module.exports = {
           require('./lib/models/ReplicationConfiguration'),
         LifecycleConfiguration:
             require('./lib/models/LifecycleConfiguration'),
+        BucketPolicy: require('./lib/models/BucketPolicy'),
     },
     metrics: {
         StatsClient: require('./lib/metrics/StatsClient'),

lib/models/BucketInfo.js

@@ -2,6 +2,7 @@ const assert = require('assert');
 const { WebsiteConfiguration } = require('./WebsiteConfiguration');
 const ReplicationConfiguration = require('./ReplicationConfiguration');
 const LifecycleConfiguration = require('./LifecycleConfiguration');
+const BucketPolicy = require('./BucketPolicy');
 
 // WHEN UPDATING THIS NUMBER, UPDATE MODELVERSION.MD CHANGELOG
 const modelVersion = 6;
@@ -47,12 +48,14 @@ class BucketInfo {
     * @param {string[]} [cors[].exposeHeaders] - headers expose to applications
     * @param {object} [replicationConfiguration] - replication configuration
     * @param {object} [lifecycleConfiguration] - lifecycle configuration
+    * @param {object} [bucketPolicy] - bucket policy
     */
     constructor(name, owner, ownerDisplayName, creationDate,
                 mdBucketModelVersion, acl, transient, deleted,
                 serverSideEncryption, versioningConfiguration,
                 locationConstraint, websiteConfiguration, cors,
-                replicationConfiguration, lifecycleConfiguration) {
+                replicationConfiguration, lifecycleConfiguration,
+                bucketPolicy) {
         assert.strictEqual(typeof name, 'string');
         assert.strictEqual(typeof owner, 'string');
         assert.strictEqual(typeof ownerDisplayName, 'string');
@@ -112,6 +115,9 @@ class BucketInfo {
         if (lifecycleConfiguration) {
             LifecycleConfiguration.validateConfig(lifecycleConfiguration);
         }
+        if (bucketPolicy) {
+            BucketPolicy.validatePolicy(bucketPolicy);
+        }
         const aclInstance = acl || {
             Canned: 'private',
             FULL_CONTROL: [],
@@ -137,6 +143,7 @@ class BucketInfo {
         this._replicationConfiguration = replicationConfiguration || null;
         this._cors = cors || null;
         this._lifecycleConfiguration = lifecycleConfiguration || null;
+        this._bucketPolicy = bucketPolicy || null;
         return this;
     }
     /**
@@ -160,6 +167,7 @@ class BucketInfo {
             cors: this._cors,
             replicationConfiguration: this._replicationConfiguration,
             lifecycleConfiguration: this._lifecycleConfiguration,
+            bucketPolicy: this._bucketPolicy,
         };
         if (this._websiteConfiguration) {
             bucketInfos.websiteConfiguration =
@@ -180,7 +188,8 @@ class BucketInfo {
             obj.creationDate, obj.mdBucketModelVersion, obj.acl,
             obj.transient, obj.deleted, obj.serverSideEncryption,
             obj.versioningConfiguration, obj.locationConstraint, websiteConfig,
-            obj.cors, obj.replicationConfiguration, obj.lifecycleConfiguration);
+            obj.cors, obj.replicationConfiguration, obj.lifecycleConfiguration,
+            obj.bucketPolicy);
     }
 
     /**
@@ -203,7 +212,8 @@ class BucketInfo {
             data._transient, data._deleted, data._serverSideEncryption,
             data._versioningConfiguration, data._locationConstraint,
             data._websiteConfiguration, data._cors,
-            data._replicationConfiguration, data._lifecycleConfiguration);
+            data._replicationConfiguration, data._lifecycleConfiguration,
+            data._bucketPolicy);
     }
 
     /**
@@ -331,6 +341,23 @@ class BucketInfo {
         this._lifecycleConfiguration = lifecycleConfiguration;
         return this;
     }
+    /**
+     * Get bucket policy statement
+     * @return {object|null} bucket policy statement or `null` if the bucket
+     * does not have a bucket policy
+     */
+    getBucketPolicy() {
+        return this._bucketPolicy;
+    }
+    /**
+     * Set bucket policy statement
+     * @param {object} bucketPolicy - bucket policy
+     * @return {BucketInfo} - bucket info instance
+     */
+    setBucketPolicy(bucketPolicy) {
+        this._bucketPolicy = bucketPolicy;
+        return this;
+    }
     /**
      * Get cors resource
      * @return {object[]} cors

lib/models/BucketPolicy.js

@@ -0,0 +1,143 @@
+const assert = require('assert');
+
+const errors = require('../errors');
+const { validateResourcePolicy } = require('../policy/policyValidator');
+
+/**
+ * Format of json policy:
+ *  {
+ *      "Id": "Policy id",
+ *      "Version": "version date",
+ *      "Statement": [
+ *          {
+ *              "Sid": "Statement id",
+ *              "Effect": "Allow",
+ *              "Principal": "*",
+ *              "Action": "s3:*",
+ *              "Resource": "arn:aws:s3:::examplebucket/bucket2/object"
+ *          },
+ *          {
+ *              "Sid": "Statement id",
+ *              "Effect": "Deny",
+ *              "Principal": {
+ *                  "AWS": ["arn:aws:iam::<account_id>", "different_account_id"]
+ *              },
+ *              "Action": [ "s3:*" ],
+ *              "Resource": [
+ *                  "arn:aws:s3:::examplebucket", "arn:aws:s3:::otherbucket/*"],
+ *              "Condition": {
+ *                 "StringNotLike": {
+ *                      "aws:Referer": [
+ *                          "http://www.example.com/", "http://example.com/*"]
+ *                  }
+ *              }
+ *          }
+ *      ]
+ *  }
+ */
+
+const objectActions = [
+    's3:AbortMultipartUpload',
+    's3:DeleteObject',
+    's3:DeleteObjectTagging',
+    's3:GetObject',
+    's3:GetObjectAcl',
+    's3:GetObjectTagging',
+    's3:ListMultipartUploadParts',
+    's3:PutObject',
+    's3:PutObjectAcl',
+    's3:PutObjectTagging',
+];
+
+class BucketPolicy {
+    /**
+     * Create a Bucket Policy instance
+     * @param {string} json - the json policy
+     * @return {object} - BucketPolicy instance
+     */
+    constructor(json) {
+        this._json = json;
+        this._policy = {};
+    }
+
+    /**
+     * Get the bucket policy
+     * @return {object} - the bucket policy or error
+     */
+    getBucketPolicy() {
+        const policy = this._getPolicy();
+        return policy;
+    }
+
+    /**
+     * Get the bucket policy array
+     * @return {object} - contains error if policy validation fails
+     */
+    _getPolicy() {
+        if (!this._json || this._json === '') {
+            return { error: errors.MalformedPolicy.customizeDescription(
+                'request json is empty or undefined') };
+        }
+        const validSchema = validateResourcePolicy(this._json);
+        if (validSchema.error) {
+            return validSchema;
+        }
+        this._setStatementArray();
+        const valAcRes = this._validateActionResource();
+        if (valAcRes.error) {
+            return valAcRes;
+        }
+
+        return this._policy;
+    }
+
+    _setStatementArray() {
+        this._policy = JSON.parse(this._json);
+        if (!Array.isArray(this._policy.Statement)) {
+            const statement = this._policy.Statement;
+            this._policy.Statement = [statement];
+        }
+    }
+
+    /**
+     * Validate action and resource are compatible
+     * @return {error} - contains error or empty obj
+     */
+    _validateActionResource() {
+        const invalid = this._policy.Statement.every(s => {
+            const actions = typeof s.Action === 'string' ?
+                [s.Action] : s.Action;
+            const resources = typeof s.Resource === 'string' ?
+                [s.Resource] : s.Resource;
+            const objectAction = actions.some(a =>
+                a.includes('Object') || objectActions.includes(a));
+            // wildcardObjectAction checks for actions such as 's3:*' or
+            // 's3:Put*' but will return false for actions such as
+            // 's3:PutBucket*'
+            const wildcardObjectAction = actions.some(
+                a => a.includes('*') && !a.includes('Bucket'));
+            const objectResource = resources.some(r => r.includes('/'));
+            return ((objectAction && !objectResource) ||
+                (objectResource && !objectAction && !wildcardObjectAction));
+        });
+        if (invalid) {
+            return { error: errors.MalformedPolicy.customizeDescription(
+                'Action does not apply to any resource(s) in statement') };
+        }
+        return {};
+    }
+
+    /**
+     * Call resource policy schema validation function
+     * @param {object} policy - the bucket policy object to validate
+     * @return {undefined}
+     */
+    static validatePolicy(policy) {
+        // only the BucketInfo constructor calls this function
+        // and BucketInfo will always be passed an object
+        const validated = validateResourcePolicy(JSON.stringify(policy));
+        assert.deepStrictEqual(validated, { error: null, valid: true });
+    }
+}
+
+module.exports = BucketPolicy;

lib/policyEvaluator/evaluator.js

@@ -67,7 +67,7 @@ function isResourceApplicable(requestContext, statementResource, log) {
  * @param {Object} log - logger
  * @return {boolean} true if applicable, false if not
  */
-function isActionApplicable(requestAction, statementAction, log) {
+evaluators.isActionApplicable = (requestAction, statementAction, log) => {
     if (!Array.isArray(statementAction)) {
         // eslint-disable-next-line no-param-reassign
         statementAction = [statementAction];
@@ -89,7 +89,7 @@ function isActionApplicable(requestAction, statementAction, log) {
         { requestAction });
     // If no match found, return false
     return false;
-}
+};
 
 /**
  * Check whether request meets policy conditions
@@ -209,14 +209,14 @@ evaluators.evaluatePolicy = (requestContext, policy, log) => {
         // If affirmative action is in policy and request action is not
         // applicable, move on to next statement
         if (currentStatement.Action &&
-            !isActionApplicable(requestContext.getAction(),
+            !evaluators.isActionApplicable(requestContext.getAction(),
             currentStatement.Action, log)) {
             continue;
         }
         // If NotAction is in policy and action matches NotAction in policy,
         // move on to next statement
         if (currentStatement.NotAction &&
-            isActionApplicable(requestContext.getAction(),
+            evaluators.isActionApplicable(requestContext.getAction(),
             currentStatement.NotAction, log)) {
             continue;
         }

lib/s3routes/routes/routeGET.js

@@ -73,9 +73,9 @@ function routerGET(request, response, api, log, statsClient, dataRetrievalFn) {
                 });
         } else if (request.query.policy !== undefined) {
             api.callApiMethod('bucketGetPolicy', request, response, log,
-                (err, json, corsHeaders) => {
+                (err, xml, corsHeaders) => {
                     routesUtils.statsReport500(err, statsClient);
-                    return routesUtils.responseJSONBody(err, json, response,
+                    return routesUtils.responseXMLBody(err, xml, response,
                         log, corsHeaders);
                 });
         } else {

tests/unit/models/BucketInfo.js

@@ -115,6 +115,18 @@ const testLifecycleConfiguration = {
         },
     ],
 };
+
+const testBucketPolicy = {
+    Version: '2012-10-17',
+    Statement: [
+        {
+            Effect: 'Allow',
+            Principal: '*',
+            Resource: 'arn:aws:s3:::examplebucket',
+            Action: 's3:*',
+        },
+    ],
+};
 // create a dummy bucket to test getters and setters
 
 Object.keys(acl).forEach(
@@ -132,7 +144,8 @@ Object.keys(acl).forEach(
             testWebsiteConfiguration,
             testCorsConfiguration,
             testReplicationConfiguration,
-            testLifecycleConfiguration);
+            testLifecycleConfiguration,
+            testBucketPolicy);
 
         describe('serialize/deSerialize on BucketInfo class', () => {
             const serialized = dummyBucket.serialize();
@@ -158,6 +171,7 @@ Object.keys(acl).forEach(
                         dummyBucket._replicationConfiguration,
                     lifecycleConfiguration:
                         dummyBucket._lifecycleConfiguration,
+                    bucketPolicy: dummyBucket._bucketPolicy,
                 };
                 assert.strictEqual(serialized, JSON.stringify(bucketInfos));
                 done();
@@ -257,6 +271,10 @@ Object.keys(acl).forEach(
                 assert.deepStrictEqual(dummyBucket.getLifecycleConfiguration(),
                     testLifecycleConfiguration);
             });
+            it('getBucketPolicy should return policy', () => {
+                assert.deepStrictEqual(
+                    dummyBucket.getBucketPolicy(), testBucketPolicy);
+            });
         });
 
         describe('setters on BucketInfo class', () => {
@@ -378,6 +396,22 @@ Object.keys(acl).forEach(
                 assert.deepStrictEqual(dummyBucket.getLifecycleConfiguration(),
                     newLifecycleConfig);
             });
+            it('setBucketPolicy should set bucket policy', () => {
+                const newBucketPolicy = {
+                    Version: '2012-10-17',
+                    Statement: [
+                        {
+                            Effect: 'Deny',
+                            Principal: '*',
+                            Resource: 'arn:aws:s3:::examplebucket',
+                            Action: 's3:*',
+                        },
+                    ],
+                };
+                dummyBucket.setBucketPolicy(newBucketPolicy);
+                assert.deepStrictEqual(
+                    dummyBucket.getBucketPolicy(), newBucketPolicy);
+            });
         });
     })
 );

tests/unit/models/BucketPolicy.js

@@ -0,0 +1,105 @@
+const assert = require('assert');
+
+const BucketPolicy = require('../../../lib/models/BucketPolicy');
+
+const testBucketPolicy = {
+    Version: '2012-10-17',
+    Statement: [
+        {
+            Effect: 'Allow',
+            Principal: '*',
+            Resource: 'arn:aws:s3:::examplebucket',
+            Action: 's3:GetBucketLocation',
+        },
+    ],
+};
+const mismatchErr = 'Action does not apply to any resource(s) in statement';
+
+function createPolicy(key, value) {
+    const newPolicy = Object.assign({}, testBucketPolicy);
+    newPolicy.Statement[0][key] = value;
+    return newPolicy;
+}
+
+function checkErr(policy, err, message) {
+    assert.strictEqual(policy.error[err], true);
+    assert.strictEqual(policy.error.description, message);
+}
+
+describe('BucketPolicy class getBucketPolicy', () => {
+    beforeEach(() => {
+        testBucketPolicy.Statement[0].Resource = 'arn:aws:s3:::examplebucket';
+        testBucketPolicy.Statement[0].Action = 's3:GetBucketLocation';
+    });
+
+    it('should return MalformedPolicy error if request json is empty', done => {
+        const bucketPolicy = new BucketPolicy('').getBucketPolicy();
+        const errMessage = 'request json is empty or undefined';
+        checkErr(bucketPolicy, 'MalformedPolicy', errMessage);
+        done();
+    });
+
+    it('should return MalformedPolicy error if request action is for objects ' +
+    'but resource refers to bucket', done => {
+        const newPolicy = createPolicy('Action', 's3:GetObject');
+        const bucketPolicy = new BucketPolicy(JSON.stringify(newPolicy))
+            .getBucketPolicy();
+        checkErr(bucketPolicy, 'MalformedPolicy', mismatchErr);
+        done();
+    });
+
+    it('should return MalformedPolicy error if request action is for objects ' +
+    'but does\'t include \'Object\' and resource refers to bucket', done => {
+        const newPolicy = createPolicy('Action', 's3:AbortMultipartUpload');
+        const bucketPolicy = new BucketPolicy(JSON.stringify(newPolicy))
+            .getBucketPolicy();
+        checkErr(bucketPolicy, 'MalformedPolicy', mismatchErr);
+        done();
+    });
+
+    it('should return MalformedPolicy error if request action is for objects ' +
+    '(with wildcard) but resource refers to bucket', done => {
+        const newPolicy = createPolicy('Action', 's3:GetObject*');
+        const bucketPolicy = new BucketPolicy(JSON.stringify(newPolicy))
+            .getBucketPolicy();
+        checkErr(bucketPolicy, 'MalformedPolicy', mismatchErr);
+        done();
+    });
+
+    it('should return MalformedPolicy error if request resource refers to ' +
+    'object but action is for buckets', done => {
+        const newPolicy = createPolicy('Resource',
+            'arn:aws:s3:::examplebucket/*');
+        const bucketPolicy = new BucketPolicy(JSON.stringify(newPolicy))
+            .getBucketPolicy();
+        checkErr(bucketPolicy, 'MalformedPolicy', mismatchErr);
+        done();
+    });
+
+    it('should return MalformedPolicy error if request resource refers to ' +
+    'object but action is for buckets (with wildcard)', done => {
+        const newPolicy = createPolicy('Resource',
+            'arn:aws:s3:::examplebucket/*');
+        newPolicy.Statement[0].Action = 's3:GetBucket*';
+        const bucketPolicy = new BucketPolicy(JSON.stringify(newPolicy))
+            .getBucketPolicy();
+        checkErr(bucketPolicy, 'MalformedPolicy', mismatchErr);
+        done();
+    });
+
+    it('should successfully get a valid policy', done => {
+        const bucketPolicy = new BucketPolicy(JSON.stringify(testBucketPolicy))
+            .getBucketPolicy();
+        assert.deepStrictEqual(bucketPolicy, testBucketPolicy);
+        done();
+    });
+
+    it('should successfully get a valid policy with wildcard in action',
+    done => {
+        const newPolicy = createPolicy('Action', 's3:Get*');
+        const bucketPolicy = new BucketPolicy(JSON.stringify(newPolicy))
+            .getBucketPolicy();
+        assert.deepStrictEqual(bucketPolicy, newPolicy);
+        done();
+    });
+});