From 96fcbb7c99b765284e62d6380780dff38f3661f2 Mon Sep 17 00:00:00 2001 From: Taylor McKinnon Date: Wed, 8 Jul 2026 07:43:53 -0700 Subject: [PATCH 1/5] impr(CLDSRV-945): Support cross-account rate limiting --- lib/api/api.js | 15 ++- lib/metadata/metadataUtils.js | 17 ++- tests/unit/api/api.js | 42 ++++++ tests/unit/metadata/metadataUtils.spec.js | 149 +++++++++++++++++++++- 4 files changed, 217 insertions(+), 6 deletions(-) diff --git a/lib/api/api.js b/lib/api/api.js index 0f6d39f5ef..bc8dafff34 100644 --- a/lib/api/api.js +++ b/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()) { diff --git a/lib/metadata/metadataUtils.js b/lib/metadata/metadataUtils.js index 8e81bf662c..a4933d12a8 100644 --- a/lib/metadata/metadataUtils.js +++ b/lib/metadata/metadataUtils.js @@ -269,6 +269,8 @@ 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, @@ -276,20 +278,27 @@ function checkRateLimitIfNeeded(request, authInfo, bucketMD, log, callback) { 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; } diff --git a/tests/unit/api/api.js b/tests/unit/api/api.js index 9d08d33207..0939b1e6ff 100644 --- a/tests/unit/api/api.js +++ b/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', diff --git a/tests/unit/metadata/metadataUtils.spec.js b/tests/unit/metadata/metadataUtils.spec.js index 4affcf29b8..0fe6b03c64 100644 --- a/tests/unit/metadata/metadataUtils.spec.js +++ b/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(); + }); + }); +}); From a612dc072298d1ce1b7bee53311d2ba093c4bd82 Mon Sep 17 00:00:00 2001 From: Taylor McKinnon Date: Wed, 8 Jul 2026 15:47:55 -0700 Subject: [PATCH 2/5] Bump arsenal to 8.4.19 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index e330afd66a..6d2c5ffe68 100644 --- a/package.json +++ b/package.json @@ -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", From 4ad05a4f0c8101470825c206e7999d21cf787b9b Mon Sep 17 00:00:00 2001 From: Taylor McKinnon Date: Thu, 9 Jul 2026 07:57:08 -0700 Subject: [PATCH 3/5] Bump vaultclient to 8.5.7 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 6d2c5ffe68..c4746cf481 100644 --- a/package.json +++ b/package.json @@ -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" From badb02909450a86db16d1e8e2997514dfbb5b7af Mon Sep 17 00:00:00 2001 From: Taylor McKinnon Date: Thu, 9 Jul 2026 08:10:06 -0700 Subject: [PATCH 4/5] Regenerate lock file --- yarn.lock | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/yarn.lock b/yarn.lock index f815159f9c..a4ba12d26f 100644 --- a/yarn.lock +++ b/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" From f03007f253a4f30df1d8a4ac1b9b0d76bb333e4f Mon Sep 17 00:00:00 2001 From: Taylor McKinnon Date: Thu, 9 Jul 2026 07:57:27 -0700 Subject: [PATCH 5/5] Bump project version --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index c4746cf481..26349643a2 100644 --- a/package.json +++ b/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": {