CLDSRV-945: Support cross account rate limiting

lib/api/api.js

@@ -91,6 +91,7 @@ const {
     buildRateChecksFromConfig,
     checkRateLimitsForRequest,
 } = require('./apiUtils/rateLimit/helpers');
+const rateLimitCache = require('./apiUtils/rateLimit/cache');
 
 const monitoringMap = policies.actionMaps.actionMonitoringMapS3;
 
@@ -287,6 +288,18 @@ function callApiHandler(apiMethod, apiHandler, request, response, log, callback)
     // Attach the names to the current request
     request.apiMethods = apiMethods;
 
+    // If we have a cached bucket owner for the targeted bucket, pass it to
+    // Vault as a hint so the returned rate limit config is the target account.
+    // Only included when the cache has a hit.
+    const authOptions = {};
+    if (request.bucketName) {
+        const cachedOwner = rateLimitCache.getCachedBucketOwner(request.bucketName);
+        if (cachedOwner) {
+            request.rateLimitTargetAccount = cachedOwner;
+            authOptions.targetAccount = cachedOwner;
+        }
+    }
+
     return async.waterfall([
         next => auth.server.doAuth(
             request, log, (err, userInfo, authorizationResults, streamingV4Params, infos) => {
@@ -301,7 +314,7 @@ function callApiHandler(apiMethod, apiHandler, request, response, log, callback)
                     return next(arsenalError);
                 }
                 return next(null, userInfo, authorizationResults, streamingV4Params, infos);
-            }, 's3', requestContexts),
+            }, 's3', requestContexts, authOptions),
         (userInfo, authorizationResults, streamingV4Params, infos, next) => {
             const authNames = { accountName: userInfo.getAccountDisplayName() };
             if (userInfo.isRequesterAnIAMUser()) {

lib/metadata/metadataUtils.js

@@ -269,27 +269,36 @@ function checkRateLimitIfNeeded(request, authInfo, bucketMD, log, callback) {
     const checks = [];
     const rateLimitConfig = extractRateLimitConfigFromRequest(request, authInfo, bucketMD, log);
 
+    cache.setCachedBucketOwner(bucketMD.getName(), bucketMD.getOwner(), config.rateLimiting.bucket.configCacheTTL);
+
     if (!request.rateLimitBucketAlreadyChecked && rateLimitConfig.bucket !== undefined) {
         cache.setCachedConfig(
             cache.namespace.bucket,
             bucketMD.getName(),
             rateLimitConfig.bucket,
             config.rateLimiting.bucket.configCacheTTL
         );
-        cache.setCachedBucketOwner(bucketMD.getName(), bucketMD.getOwner(), config.rateLimiting.bucket.configCacheTTL);
         checks.push(...buildRateChecksFromConfig('bucket', bucketMD.getName(), rateLimitConfig.bucket));
         // eslint-disable-next-line no-param-reassign
         request.rateLimitBucketAlreadyChecked = true;
     }
 
-    if (!request.rateLimitAccountAlreadyChecked && rateLimitConfig.account !== undefined) {
+    if (
+        !request.rateLimitAccountAlreadyChecked
+        && rateLimitConfig.account !== undefined
+        && !(authInfo.isRequesterPublicUser && authInfo.isRequesterPublicUser())
+    ) {
+        const targetAccount = request.rateLimitTargetAccount
+            ? request.rateLimitTargetAccount
+            : authInfo.getCanonicalID();
+
         cache.setCachedConfig(
             cache.namespace.account,
-            authInfo.getCanonicalID(),
+            targetAccount,
             rateLimitConfig.account,
             config.rateLimiting.account.configCacheTTL
         );
-        checks.push(...buildRateChecksFromConfig('account', authInfo.getCanonicalID(), rateLimitConfig.account));
+        checks.push(...buildRateChecksFromConfig('account', targetAccount, rateLimitConfig.account));
         // eslint-disable-next-line no-param-reassign
         request.rateLimitAccountAlreadyChecked = true;
     }

package.json

@@ -1,6 +1,6 @@
 {
     "name": "@zenko/cloudserver",
-    "version": "9.3.13",
+    "version": "9.3.14",
     "description": "Zenko CloudServer, an open-source Node.js implementation of a server handling the Amazon S3 protocol",
     "main": "index.js",
     "engines": {
@@ -30,7 +30,7 @@
         "@azure/storage-blob": "^12.28.0",
         "@hapi/joi": "^17.1.1",
         "@smithy/node-http-handler": "^3.0.0",
-        "arsenal": "git+https://github.com/scality/Arsenal#8.4.16",
+        "arsenal": "git+https://github.com/scality/Arsenal#8.4.19",
         "async": "2.6.4",
         "bucketclient": "scality/bucketclient#8.2.7",
         "bufferutil": "^4.0.8",
@@ -55,7 +55,7 @@
         "utf-8-validate": "^6.0.5",
         "utf8": "^3.0.0",
         "uuid": "^11.0.3",
-        "vaultclient": "scality/vaultclient#8.5.3",
+        "vaultclient": "scality/vaultclient#8.5.7",
         "werelogs": "scality/werelogs#semver:^8.2.4",
         "ws": "^8.18.0",
         "xml2js": "^0.6.2"

tests/unit/api/api.js

@@ -1,6 +1,7 @@
 const sinon = require('sinon');
 const { errors, auth } = require('arsenal');
 const api = require('../../../lib/api/api');
+const rateLimitCache = require('../../../lib/api/apiUtils/rateLimit/cache');
 const DummyRequest = require('../DummyRequest');
 const { default: AuthInfo } = require('arsenal/build/lib/auth/AuthInfo');
 const assert = require('assert');
@@ -149,6 +150,47 @@ describe('api.callApiMethod', () => {
         });
     });
 
+    describe('cross-account rate limiting target account', () => {
+        afterEach(() => {
+            rateLimitCache.bucketOwnerCache.clear();
+        });
+
+        it('should pass the cached bucket owner to doAuth as targetAccount', done => {
+            request.bucketName = 'rl-bucket';
+            rateLimitCache.setCachedBucketOwner('rl-bucket', 'owner-canonical-id', 30000);
+            // Reset the default callsArgWith behavior so only the fake runs
+            authServer.doAuth.resetBehavior();
+            authServer.doAuth.callsFake((req, log, cb, awsService, requestContexts, authOptions) => {
+                assert.strictEqual(authOptions.targetAccount, 'owner-canonical-id');
+                assert.strictEqual(request.rateLimitTargetAccount, 'owner-canonical-id');
+                done();
+            });
+            api.callApiMethod('bucketGet', request, response, log);
+        });
+
+        it('should not set targetAccount when no bucket owner is cached', done => {
+            request.bucketName = 'rl-bucket';
+            authServer.doAuth.resetBehavior();
+            authServer.doAuth.callsFake((req, log, cb, awsService, requestContexts, authOptions) => {
+                assert.deepStrictEqual(authOptions, {});
+                assert.strictEqual(request.rateLimitTargetAccount, undefined);
+                done();
+            });
+            api.callApiMethod('bucketGet', request, response, log);
+        });
+
+        it('should not set targetAccount when the request has no bucket name', done => {
+            rateLimitCache.setCachedBucketOwner('rl-bucket', 'owner-canonical-id', 30000);
+            authServer.doAuth.resetBehavior();
+            authServer.doAuth.callsFake((req, log, cb, awsService, requestContexts, authOptions) => {
+                assert.deepStrictEqual(authOptions, {});
+                assert.strictEqual(request.rateLimitTargetAccount, undefined);
+                done();
+            });
+            api.callApiMethod('bucketGet', request, response, log);
+        });
+    });
+
     describe('MD5 checksum validation', () => {
         const methodsWithChecksumValidation = [
             'bucketPutACL',

tests/unit/metadata/metadataUtils.spec.js

@@ -1,7 +1,7 @@
 const assert = require('assert');
 const sinon = require('sinon');
 
-const { models } = require('arsenal');
+const { errors, models } = require('arsenal');
 const { BucketInfo } = models;
 const { DummyRequestLogger, makeAuthInfo } = require('../helpers');
 
@@ -19,8 +19,13 @@ const {
     metadataGetObjects,
     metadataGetObject,
     storeServerAccessLogInfo,
+    standardMetadataValidateBucket,
 } = require('../../../lib/metadata/metadataUtils');
 const metadata = require('../../../lib/metadata/wrapper');
+const { config } = require('../../../lib/Config');
+const constants = require('../../../constants');
+const rateLimitCache = require('../../../lib/api/apiUtils/rateLimit/cache');
+const tokenBucket = require('../../../lib/api/apiUtils/rateLimit/tokenBucket');
 
 describe('validateBucket', () => {
     it('action bucketPutPolicy by bucket owner', () => {
@@ -183,3 +188,145 @@ describe('storeServerAccessLogInfo - copySource aclRequired', () => {
         assert.strictEqual(request.serverAccessLog.aclRequired, undefined);
     });
 });
+
+describe('checkRateLimitIfNeeded cross-account rate limiting', () => {
+    let sandbox;
+    let request;
+
+    const otherCanonicalId = otherAuthInfo.getCanonicalID();
+
+    beforeEach(() => {
+        sandbox = sinon.createSandbox();
+        sandbox.stub(config, 'rateLimiting').value({
+            enabled: true,
+            serviceUserArn: 'arn:aws:iam::000000000000:user/rate-limit-service-user',
+            nodes: 1,
+            tokenBucketBufferSize: 50,
+            tokenBucketRefillThreshold: 20,
+            error: errors.SlowDown,
+            bucket: {
+                configCacheTTL: 30000,
+                defaultConfig: { RequestsPerSecond: { BurstCapacity: 1 } },
+            },
+            account: {
+                configCacheTTL: 30000,
+                defaultConfig: { RequestsPerSecond: { BurstCapacity: 1 } },
+            },
+        });
+        sandbox.stub(metadata, 'getBucket').yields(null, bucket);
+        rateLimitCache.configCache.clear();
+        rateLimitCache.bucketOwnerCache.clear();
+        tokenBucket.getAllTokenBuckets().clear();
+        request = { apiMethod: 'bucketGet', bucketName: bucket.getName() };
+    });
+
+    afterEach(() => {
+        sandbox.restore();
+        rateLimitCache.configCache.clear();
+        rateLimitCache.bucketOwnerCache.clear();
+        tokenBucket.getAllTokenBuckets().clear();
+    });
+
+    // Rate limiting runs before bucket authorization, so the requester may
+    // still get AccessDenied from validateBucket afterwards; these tests
+    // assert on the rate limit side effects, not the final auth outcome.
+    function validateBucketRequest(requesterAuthInfo, cb) {
+        standardMetadataValidateBucket({
+            authInfo: requesterAuthInfo,
+            bucketName: bucket.getName(),
+            requestType: 'bucketGet',
+            request,
+        }, false, log, cb);
+    }
+
+    it('should cache the bucket owner even when the bucket config was already checked', done => {
+        request.rateLimitBucketAlreadyChecked = true;
+        validateBucketRequest(authInfo, err => {
+            assert.ifError(err);
+            assert.strictEqual(rateLimitCache.getCachedBucketOwner(bucket.getName()), ownerCanonicalId);
+            done();
+        });
+    });
+
+    it('should key the account rate limit under the requester canonical ID by default', done => {
+        request.accountLimits = { RequestsPerSecond: { Limit: 100 } };
+        validateBucketRequest(authInfo, err => {
+            assert.ifError(err);
+            const cached = rateLimitCache.getCachedConfig(rateLimitCache.namespace.account, ownerCanonicalId);
+            assert.deepStrictEqual(cached, {
+                RequestsPerSecond: { BurstCapacity: 1, Limit: 100, source: 'resource' },
+            });
+            assert(tokenBucket.getAllTokenBuckets().has(`account:${ownerCanonicalId}:rps`));
+            assert.strictEqual(request.rateLimitAccountAlreadyChecked, true);
+            done();
+        });
+    });
+
+    it('should key the account rate limit under the target account when the request carries one', done => {
+        request.accountLimits = { RequestsPerSecond: { Limit: 100 } };
+        request.rateLimitTargetAccount = ownerCanonicalId;
+        validateBucketRequest(otherAuthInfo, () => {
+            assert(rateLimitCache.getCachedConfig(rateLimitCache.namespace.account, ownerCanonicalId));
+            assert.strictEqual(
+                rateLimitCache.getCachedConfig(rateLimitCache.namespace.account, otherCanonicalId),
+                undefined);
+            assert(tokenBucket.getAllTokenBuckets().has(`account:${ownerCanonicalId}:rps`));
+            assert(!tokenBucket.getAllTokenBuckets().has(`account:${otherCanonicalId}:rps`));
+            done();
+        });
+    });
+
+    it('should deny a cross-account request when the target account limit is exhausted', done => {
+        request.accountLimits = { RequestsPerSecond: { Limit: 100 } };
+        request.rateLimitTargetAccount = ownerCanonicalId;
+        const ownerBucket = tokenBucket.getTokenBucket(
+            'account', ownerCanonicalId, 'rps', { limit: 100, burstCapacity: 1000 }, log);
+        ownerBucket.tokens = 0;
+        validateBucketRequest(otherAuthInfo, err => {
+            assert(err);
+            assert(err.is.SlowDown);
+            done();
+        });
+    });
+
+    it('should rate limit against the requester account when no target account is present', done => {
+        request.accountLimits = { RequestsPerSecond: { Limit: 100 } };
+        const ownerBucket = tokenBucket.getTokenBucket(
+            'account', ownerCanonicalId, 'rps', { limit: 100, burstCapacity: 1000 }, log);
+        ownerBucket.tokens = 0;
+        validateBucketRequest(otherAuthInfo, err => {
+            // The exhausted owner account bucket must not affect the request:
+            // the requester is limited against their own account.
+            assert(!err || !err.is.SlowDown);
+            assert(tokenBucket.getAllTokenBuckets().has(`account:${otherCanonicalId}:rps`));
+            done();
+        });
+    });
+
+    it('should skip the account rate limit check for public requesters', done => {
+        const publicAuthInfo = makeAuthInfo(constants.publicId);
+        request.accountLimits = { RequestsPerSecond: { Limit: 100 } };
+        validateBucketRequest(publicAuthInfo, () => {
+            assert.strictEqual(request.rateLimitAccountAlreadyChecked, undefined);
+            assert.strictEqual(
+                rateLimitCache.getCachedConfig(rateLimitCache.namespace.account, constants.publicId),
+                undefined);
+            // The bucket owner is still cached for later cross-account attribution
+            assert.strictEqual(rateLimitCache.getCachedBucketOwner(bucket.getName()), ownerCanonicalId);
+            done();
+        });
+    });
+
+    it('should skip the account rate limit check for public requesters even with a target account', done => {
+        const publicAuthInfo = makeAuthInfo(constants.publicId);
+        request.accountLimits = { RequestsPerSecond: { Limit: 100 } };
+        request.rateLimitTargetAccount = ownerCanonicalId;
+        validateBucketRequest(publicAuthInfo, () => {
+            assert.strictEqual(request.rateLimitAccountAlreadyChecked, undefined);
+            assert.strictEqual(
+                rateLimitCache.getCachedConfig(rateLimitCache.namespace.account, ownerCanonicalId),
+                undefined);
+            done();
+        });
+    });
+});

yarn.lock

@@ -6591,9 +6591,9 @@ arraybuffer.prototype.slice@^1.0.4:
   optionalDependencies:
     ioctl "^2.0.2"
 
-"arsenal@git+https://github.com/scality/Arsenal#8.4.16":
-  version "8.4.16"
-  resolved "git+https://github.com/scality/Arsenal#65223fdfc37f4471332c44775def9e985b320855"
+"arsenal@git+https://github.com/scality/Arsenal#8.4.19":
+  version "8.4.19"
+  resolved "git+https://github.com/scality/Arsenal#059ecf4a3c49b0b0d2c0f9ec0b01dc76fea7b767"
   dependencies:
     "@aws-sdk/client-kms" "^3.975.0"
     "@aws-sdk/client-s3" "^3.975.0"
@@ -12603,6 +12603,17 @@ vaultclient@scality/vaultclient#8.5.3:
     werelogs scality/werelogs#8.2.0
     xml2js "^0.6.2"
 
+vaultclient@scality/vaultclient#8.5.7:
+  version "8.5.7"
+  resolved "https://codeload.github.com/scality/vaultclient/tar.gz/f1022abcaa164c5d6046371491a1a310bb13bf4d"
+  dependencies:
+    "@aws-crypto/sha256-universal" "^5.2.0"
+    "@smithy/signature-v4" "^4.1.0"
+    commander "^11.0.0"
+    httpagent "git+https://github.com/scality/httpagent#1.1.0"
+    werelogs scality/werelogs#8.2.0
+    xml2js "^0.6.2"
+
 verror@1.10.0:
   version "1.10.0"
   resolved "https://registry.yarnpkg.com/verror/-/verror-1.10.0.tgz#3a105ca17053af55d6e270c1f8288682e18da400"