Bypass bucket policies on internal endpoint

Currently, bucket policies affect also internal processes (backbeat...),
which can allow end users to "break" some features (replication,
lifecycle...).

The topic was discussed in [1], and the short term solution is to use
some sort of "internal" cloudserver, which would not evaluate bucket
policies.

This is addressed by this commit, which adds the ability to create an
"internal" endpoint on a separate port, where bucket policies are
ignored.

[1] https://scality.atlassian.net/wiki/spaces/OS/pages/2895347722/Authorizing+Internal+Services+S3+Operations

Issue: CLDSRV-650

Author: francoisferrand

lib/api/apiUtils/authorization/permissionChecks.js

@@ -358,7 +358,7 @@ function processBucketPolicy(requestType, bucket, canonicalID, arn, bucketOwner,
     request, aclPermission, results, actionImplicitDenies) {
     const bucketPolicy = bucket.getBucketPolicy();
     let processedResult = results[requestType];
-    if (!bucketPolicy) {
+    if (!bucketPolicy || request?.bypassUserBucketPolicies) {
         processedResult = actionImplicitDenies[requestType] === false && aclPermission;
     } else {
         const bucketPolicyPermission = checkBucketPolicy(bucketPolicy, requestType, canonicalID, arn,

lib/server.js

@@ -26,7 +26,6 @@ const {
 
 const HttpAgent = require('agentkeepalive');
 const QuotaService = require('./quotas/quotas');
-const routes = arsenal.s3routes.routes;
 const { parseLC, MultipleBackendGateway } = arsenal.storage.data;
 const websiteEndpoints = _config.websiteEndpoints;
 let client = dataWrapper.client;
@@ -95,11 +94,29 @@ class S3Server {
         this.started = false;
     }
 
+    /**
+     * Route requests on 'internal s3' port
+     * Same as routeRequest, but ignoring user's bucket policy. This should be used only for
+     * backbeat and other internal/system processes that must not be affected by user's bucket
+     * policy.
+     * Note that this is not a temporary measure: eventually this should be improved to better
+     * identify and isolate internal/system calls vs user calls, so that "system" bucket policies
+     * may be applied to system requests, and the existing "user" bucket policies apply only to
+     * user requests.
+     * @param {http.IncomingMessage} req - http request object
+     * @param {http.ServerResponse} res - http response object
+     * @returns {undefined}
+     */
+    internalRouteRequest(req, res) {
+        req.bypassUserBucketPolicies = true; // eslint-disable-line no-param-reassign
+        return this.routeRequest(req, res);
+    }
+
     /**
      * Route requests on 's3' port
      * @param {http.IncomingMessage} req - http request object
      * @param {http.ServerResponse} res - http response object
-     * @returns {void}
+     * @returns {undefined}
      */
     routeRequest(req, res) {
         monitoringClient.httpActiveRequests.inc();
@@ -153,14 +170,14 @@ class S3Server {
                 vault,
             },
         };
-        routes(req, res, params, logger, this.config);
+        arsenal.s3routes.routes(req, res, params, logger, this.config);
     }
 
     /**
      * Route requests on 'admin' port
      * @param {http.IncomingMessage} req - http request object
      * @param {http.ServerResponse} res - http response object
-     * @returns {void}
+     * @returns {undefined}
      */
     routeAdminRequest(req, res) {
         // use proxied hostname if needed
@@ -207,7 +224,7 @@ class S3Server {
      * @param {http.RequestListener} listener - Callback to handle requests
      * @param {number} port - Port to listen on
      * @param {string | undefined} ipAddress - IpAddress to listen on
-     * @returns {void}
+     * @returns {undefined}
      */
     _startServer(listener, port, ipAddress) {
         // Todo: http.globalAgent.maxSockets, http.globalAgent.maxFreeSockets
@@ -318,10 +335,10 @@ class S3Server {
             // Start internal API server(s)
             if (this.config.internalListenOn.length > 0) {
                 this.config.internalListenOn.forEach(item => {
-                    this._startServer(this.routeRequest, item.port, item.ip);
+                    this._startServer(this.internalRouteRequest, item.port, item.ip);
                 });
             } else if (this.config.internalPort) {
-                this._startServer(this.routeRequest, this.config.internalPort);
+                this._startServer(this.internalRouteRequest, this.config.internalPort);
             }
 
             // Start metrics server(s)

tests/unit/api/bucketPolicyAuth.js

@@ -346,6 +346,34 @@ describe('bucket policy authorization', () => {
             assert.equal(allowed, false);
             done();
         });
+
+        it('should bypass bucket policy when request.bypassUserBucketPolicies is true', function () {
+            // Create a request with the bypassUserBucketPolicies flag initially not set
+            const request = {
+                bucketName,
+                socket: {},
+            };
+
+            // Add a policy to explicitely deny access
+            const newPolicy = this.test.basePolicy;
+            newPolicy.Statement[0] = {
+                Effect: 'Deny',
+                Principal: { CanonicalUser: [bucketOwnerCanonicalId] },
+                Resource: `arn:aws:s3:::${bucketName}`,
+                Action: 's3:*',
+            };
+            bucket.setBucketPolicy(newPolicy);
+
+            // Check that the policy denies access, as expected
+            assert.ok(!isBucketAuthorized(bucket, bucAction,
+                bucketOwnerCanonicalId, user1AuthInfo, log, request));
+
+            // But with bypassUserBucketPolicies set to true, it should still be authorized
+            // based on ACL permissions (which we mock to return true)
+            request.bypassUserBucketPolicies = true;
+            assert.ok(isBucketAuthorized(bucket, bucAction,
+                bucketOwnerCanonicalId, user1AuthInfo, log, request));
+        });
     });
 
     describe('isObjAuthorized with no policy set', () => {
@@ -427,6 +455,35 @@ describe('bucket policy authorization', () => {
             assert.equal(allowed, false);
             done();
         });
+
+        it('should bypass bucket policy when request.bypassUserBucketPolicies is true', function () {
+            // Create a request with the bypassUserBucketPolicies flag initially not set
+            const request = {
+                bucketName,
+                socket: {},
+            };
+
+            // Add a policy to explicitely deny access
+            const newPolicy = this.test.basePolicy;
+            newPolicy.Statement[0] = {
+                Effect: 'Deny',
+                Principal: { CanonicalUser: [bucketOwnerCanonicalId] },
+                Resource: `arn:aws:s3:::${bucketName}`,
+                Action: 's3:*',
+            };
+            bucket.setBucketPolicy(newPolicy);
+
+            // Check that the policy denies access, as expected
+            assert.ok(!isObjAuthorized(bucket, object, 'objectGet',
+                bucketOwnerCanonicalId, user1AuthInfo, log, request));
+
+            // But with bypassUserBucketPolicies set to true, it should still be authorized
+            // based on ACL permissions (which we mock to return true)
+            request.bypassUserBucketPolicies = true;
+            assert.ok(isObjAuthorized(bucket, object, 'objectGet',
+                bucketOwnerCanonicalId, user1AuthInfo, log, request));
+        });
+
         it('should deny access to non-object owner if two statements apply ' +
         'to principal but one denies access', function itFn(done) {
             const newPolicy = this.test.basePolicy;

tests/unit/server.js

@@ -2,6 +2,7 @@
 
 const assert = require('assert');
 const sinon = require('sinon');
+const arsenal = require('arsenal');
 const logger = require('../../lib/utilities/logger');
 const { config: defaultConfig } = require('../../lib/Config');
 const { S3Server } = require('../../lib/server');
@@ -83,7 +84,7 @@ describe('S3Server', () => {
             await waitReady();
 
             assert.strictEqual(startServerStub.callCount, 2);
-            assert(startServerStub.calledWith(server.routeRequest, 9000));
+            assert(startServerStub.calledWith(server.internalRouteRequest, 9000));
         });
 
         it('should start internal API servers from internalListenOn array', async () => {
@@ -98,8 +99,8 @@ describe('S3Server', () => {
             await waitReady();
 
             assert.strictEqual(startServerStub.callCount, 3);
-            assert(startServerStub.calledWith(server.routeRequest, 9000, '127.0.0.1'));
-            assert(startServerStub.calledWith(server.routeRequest, 9001, '0.0.0.0'));
+            assert(startServerStub.calledWith(server.internalRouteRequest, 9000, '127.0.0.1'));
+            assert(startServerStub.calledWith(server.internalRouteRequest, 9001, '0.0.0.0'));
             assert(startServerStub.calledWith(server.routeAdminRequest));
             assert.strictEqual(startServerStub.neverCalledWith(server.routeRequest, 9999), true);
         });
@@ -143,8 +144,53 @@ describe('S3Server', () => {
 
             assert.strictEqual(startServerStub.callCount, 3);
             assert(startServerStub.calledWith(server.routeRequest, 8000));
-            assert(startServerStub.calledWith(server.routeRequest, 9000));
+            assert(startServerStub.calledWith(server.internalRouteRequest, 9000));
             assert(startServerStub.calledWith(server.routeAdminRequest, 8002));
         });
     });
+
+    describe('internalRouteRequest', () => {
+        const resp = {
+            on: () => { },
+            setHeader: () => { },
+            writeHead: () => { },
+            end: () => { },
+        };
+
+        let req;
+
+        beforeEach(() => {
+            req = {
+                headers: {},
+                socket: {
+                    setNoDelay: () => { },
+                },
+                url: 'http://localhost:8000',
+            };
+        });
+
+        afterEach(() => {
+            sinon.restore();
+        });
+
+        it('should bypass bucket policy for internal requests', () => {
+            const routesMock = sinon.stub().callsFake(req => {
+                assert(req.bypassUserBucketPolicies);
+            });
+            sinon.stub(arsenal.s3routes, 'routes').value(routesMock);
+
+            server.internalRouteRequest(req, resp);
+            sinon.assert.calledOnce(routesMock);
+        });
+
+        it('should bypass bucket policy for routes requests', () => {
+            const routesMock = sinon.stub().callsFake(req => {
+                assert(!req.bypassUserBucketPolicies);
+            });
+            sinon.stub(arsenal.s3routes, 'routes').value(routesMock);
+
+            server.routeRequest(req, resp);
+            sinon.assert.calledOnce(routesMock);
+        });
+    });
 });