diff --git a/lib/api/api.js b/lib/api/api.js index c7f6e0e591..487dd7d0ce 100644 --- a/lib/api/api.js +++ b/lib/api/api.js @@ -90,7 +90,6 @@ const { buildRateChecksFromConfig, checkRateLimitsForRequest, } = require('./apiUtils/rateLimit/helpers'); -const rateLimitCache = require('./apiUtils/rateLimit/cache'); const monitoringMap = policies.actionMaps.actionMonitoringMapS3; @@ -288,12 +287,8 @@ function callApiHandler(apiMethod, apiHandler, request, response, log, callback) // 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; - } + if (request.rateLimitTargetAccount !== undefined) { + authOptions.targetAccount = request.rateLimitTargetAccount; } return async.waterfall( @@ -419,7 +414,7 @@ function callApiHandler(apiMethod, apiHandler, request, response, log, callback) log, (err, res) => { request.accountQuotas = infos?.accountQuota; - request.accountLimits = infos?.limits; + request.rateLimitTargetAccountLimits = infos?.limits; if (err) { return next(err); } @@ -509,6 +504,10 @@ const api = { checks.push(...buildRateChecksFromConfig('bucket', request.bucketName, rateLimitConfig.bucket)); } + if (rateLimitConfig.bucketOwner !== undefined) { + request.rateLimitTargetAccount = rateLimitConfig.bucketOwner; + } + if (rateLimitConfig.account !== undefined) { request.rateLimitAccountAlreadyChecked = true; checks.push(...buildRateChecksFromConfig('account', rateLimitConfig.bucketOwner, rateLimitConfig.account)); diff --git a/lib/api/apiUtils/rateLimit/config.js b/lib/api/apiUtils/rateLimit/config.js index 84c4e49a35..8661c7f386 100644 --- a/lib/api/apiUtils/rateLimit/config.js +++ b/lib/api/apiUtils/rateLimit/config.js @@ -156,17 +156,17 @@ const { rateLimitDefaultConfigCacheTTL, rateLimitDefaultBurstCapacity } = requir * @property {object} defaultConfig - Default config applied if no resource specific configuration is found. * @property {number} configCacheTTL - Number of milliseconds to cache per resource configs * @property {number} defaultBurstCapacity - Default used if resource does not specify a burst capacity. -*/ + */ const rateLimitClassConfigSchema = Joi.object({ defaultConfig: Joi.object({ requestsPerSecond: Joi.object({ limit: Joi.number().integer().min(0).required(), - burstCapacity: Joi.number().positive(), + burstCapacity: Joi.number().min(0), }), }), configCacheTTL: Joi.number().integer().positive().default(rateLimitDefaultConfigCacheTTL), - defaultBurstCapacity: Joi.number().positive().default(rateLimitDefaultBurstCapacity), + defaultBurstCapacity: Joi.number().min(0).default(rateLimitDefaultBurstCapacity), }).default({ defaultConfig: undefined, configCacheTTL: rateLimitDefaultConfigCacheTTL, @@ -233,17 +233,17 @@ function transformClassConfig(resourceClass, validatedCfg, nodes) { if (limit > 0 && limit < nodes) { throw new Error( `rateLimiting.${resourceClass}.defaultConfig.` + - `requestsPerSecond.limit (${limit}) must be >= ` + - `nodes (${nodes}) ` + - 'or 0 (unlimited). Each node enforces limit/nodes locally. ' + - `With limit < ${nodes}, per-node rate would be < 1 req/s, effectively blocking traffic.` + `requestsPerSecond.limit (${limit}) must be >= ` + + `nodes (${nodes}) ` + + 'or 0 (unlimited). Each node enforces limit/nodes locally. ' + + `With limit < ${nodes}, per-node rate would be < 1 req/s, effectively blocking traffic.`, ); } // Store both the original limit and the calculated values defaultConfig.RequestsPerSecond = { Limit: limit, - BurstCapacity: burstCapacity || validatedCfg.defaultBurstCapacity, + BurstCapacity: burstCapacity ?? validatedCfg.defaultBurstCapacity, }; } @@ -263,14 +263,11 @@ function transformClassConfig(resourceClass, validatedCfg, nodes) { */ function parseRateLimitConfig(rateLimitingConfig) { // Validate configuration using Joi schema - const { error: validationError, value: validated } = rateLimitConfigSchema.validate( - rateLimitingConfig, - { - abortEarly: false, // Return all validation errors at once - allowUnknown: false, // Don't allow key not present in schema - convert: false, // Don't do type coercion (e.g. "1" -> 1) - } - ); + const { error: validationError, value: validated } = rateLimitConfigSchema.validate(rateLimitingConfig, { + abortEarly: false, // Return all validation errors at once + allowUnknown: false, // Don't allow key not present in schema + convert: false, // Don't do type coercion (e.g. "1" -> 1) + }); if (validationError) { const details = validationError.details.map(d => d.message).join('; '); @@ -284,11 +281,7 @@ function parseRateLimitConfig(rateLimitingConfig) { nodes: validated.nodes, tokenBucketBufferSize: validated.tokenBucketBufferSize, tokenBucketRefillThreshold: validated.tokenBucketRefillThreshold, - error: new ArsenalError( - validated.error.code, - validated.error.statusCode, - validated.error.message, - ), + error: new ArsenalError(validated.error.code, validated.error.statusCode, validated.error.message), }; parsed.bucket = transformClassConfig('bucket', validated.bucket, parsed.nodes); diff --git a/lib/api/apiUtils/rateLimit/helpers.js b/lib/api/apiUtils/rateLimit/helpers.js index 07811b021c..cebe83c425 100644 --- a/lib/api/apiUtils/rateLimit/helpers.js +++ b/lib/api/apiUtils/rateLimit/helpers.js @@ -1,7 +1,12 @@ const { config } = require('../../../Config'); +const vault = require('../../../auth/vault'); const cache = require('./cache'); const { getTokenBucket } = require('./tokenBucket'); -const { policies: { actionMaps: { actionMapBucketRateLimit } } } = require('arsenal'); +const { + policies: { + actionMaps: { actionMapBucketRateLimit }, + }, +} = require('arsenal'); const rateLimitApiActions = Object.keys(actionMapBucketRateLimit); @@ -58,7 +63,7 @@ function extractBucketRateLimitConfig(bucketMD, log) { cfg: { ...config.rateLimiting.bucket.defaultConfig, source: 'global', - } + }, }); return { @@ -76,24 +81,24 @@ function extractBucketRateLimitConfig(bucketMD, log) { * 1. Per-account configuration (from request) * 2. Global default configuration * - * @param {object} authInfo - Instance of AuthInfo class with requester's info - * @param {object} request - request object given by router + * @param {string} canonicalId - canonicalId of target account + * @param {object} accountLimits - account rate limit config * @param {object} log - Logger instance * @returns {object} Rate limit config */ -function extractAccountRateLimitConfig(authInfo, request, log) { +function extractAccountRateLimitConfig(canonicalId, accountLimits, log) { // Try per-account config first - if (request.accountLimits) { + if (accountLimits) { const merged = { RequestsPerSecond: { ...config.rateLimiting.account.defaultConfig.RequestsPerSecond, - ...(request.accountLimits.RequestsPerSecond || {}), - source: request.accountLimits.RequestsPerSecond !== undefined ? 'resource' : 'global', + ...(accountLimits.RequestsPerSecond || {}), + source: accountLimits.RequestsPerSecond !== undefined ? 'resource' : 'global', }, }; log.debug('Extracted per-account rate limit config', { - accountId: authInfo.getCanonicalID(), + canonicalId, cfg: merged, }); @@ -108,21 +113,13 @@ function extractAccountRateLimitConfig(authInfo, request, log) { }; log.debug('Using global default rate limit config', { - accountId: authInfo.getCanonicalID(), + canonicalId, cfg, }); return cfg; } -function extractRateLimitConfigFromRequest(request, authInfo, bucketMD, log) { - const limitConfig = { - bucket: extractBucketRateLimitConfig(bucketMD, log), - account: extractAccountRateLimitConfig(authInfo, request, log), - }; - return limitConfig; -} - function getCachedRateLimitConfig(request) { const cachedConfig = {}; const cachedBucketConfig = cache.getCachedConfig(cache.namespace.bucket, request.bucketName); @@ -132,10 +129,10 @@ function getCachedRateLimitConfig(request) { const cachedOwner = cache.getCachedBucketOwner(request.bucketName); if (cachedOwner !== undefined) { + cachedConfig.bucketOwner = cachedOwner; const cachedAccountConfig = cache.getCachedConfig(cache.namespace.account, cachedOwner); if (cachedAccountConfig !== undefined) { cachedConfig.account = cachedAccountConfig; - cachedConfig.bucketOwner = cachedOwner; } } @@ -173,7 +170,7 @@ function checkRateLimitsForRequest(checks, log) { source: check.source, }); - return { allowed: false, rateLimitSource: `${check.resourceClass}:${check.source}`}; + return { allowed: false, rateLimitSource: `${check.resourceClass}:${check.source}` }; } buckets.push(bucket); @@ -192,12 +189,44 @@ function checkRateLimitsForRequest(checks, log) { return { allowed: true }; } +async function fetchAccountRateLimitConfig(canonicalId, log) { + return new Promise((resolve, reject) => + vault.getAccountLimitsByCanonicalId(canonicalId, log, (err, res) => { + if (err) { + reject(err); + } else { + resolve(res); + } + }), + ); +} + +async function resolveRateLimitConfig(request, authInfo, bucketMD, log) { + let accountLimits = request.rateLimitTargetAccountLimits; + // Account limits need to be fetched from Vault in 2 cases + // 1) A cross-account request where the bucket owner was not found in the cache. + // 2) An anonymous request as no previous call to Vault was made. + if ( + (!request.rateLimitTargetAccount && authInfo.getCanonicalID() !== bucketMD.getOwner()) || + (authInfo.isRequesterPublicUser && authInfo.isRequesterPublicUser()) + ) { + accountLimits = await fetchAccountRateLimitConfig(bucketMD.getOwner(), log); + } + + const limitConfig = { + bucket: extractBucketRateLimitConfig(bucketMD, log), + account: extractAccountRateLimitConfig(bucketMD.getOwner(), accountLimits, log), + }; + + return limitConfig; +} + module.exports = { rateLimitApiActions, extractBucketRateLimitConfig, - extractRateLimitConfigFromRequest, buildRateChecksFromConfig, checkRateLimitsForRequest, getCachedRateLimitConfig, requestNeedsRateCheck, + resolveRateLimitConfig, }; diff --git a/lib/metadata/metadataUtils.js b/lib/metadata/metadataUtils.js index f82e00503b..35c6e01de6 100644 --- a/lib/metadata/metadataUtils.js +++ b/lib/metadata/metadataUtils.js @@ -16,7 +16,7 @@ const cache = require('../api/apiUtils/rateLimit/cache'); const { rateLimitApiActions, requestNeedsRateCheck, - extractRateLimitConfigFromRequest, + resolveRateLimitConfig, buildRateChecksFromConfig, checkRateLimitsForRequest, } = require('../api/apiUtils/rateLimit/helpers'); @@ -273,65 +273,68 @@ function validateBucket(bucket, params, log, actionImplicitDenies = {}) { function checkRateLimitIfNeeded(request, authInfo, bucketMD, log, callback) { // Skip if already checked or not enabled if (!requestNeedsRateCheck(request)) { - return process.nextTick(callback, null); + process.nextTick(callback, null); + return; } - // Extract rate limit config from bucket metadata and cache it - const checks = []; - const rateLimitConfig = extractRateLimitConfigFromRequest(request, authInfo, bucketMD, log); + resolveRateLimitConfig(request, authInfo, bucketMD, log) + .then(rateLimitConfig => { + const checks = []; + 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, + ); + checks.push(...buildRateChecksFromConfig('bucket', bucketMD.getName(), rateLimitConfig.bucket)); + // eslint-disable-next-line no-param-reassign + request.rateLimitBucketAlreadyChecked = true; + } - cache.setCachedBucketOwner(bucketMD.getName(), bucketMD.getOwner(), config.rateLimiting.bucket.configCacheTTL); + if (!request.rateLimitAccountAlreadyChecked && rateLimitConfig.account !== undefined) { + const targetAccount = request.rateLimitTargetAccount + ? request.rateLimitTargetAccount + : bucketMD.getOwner(); - if (!request.rateLimitBucketAlreadyChecked && rateLimitConfig.bucket !== undefined) { - cache.setCachedConfig( - cache.namespace.bucket, - bucketMD.getName(), - rateLimitConfig.bucket, - 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 && - !(authInfo.isRequesterPublicUser && authInfo.isRequesterPublicUser()) - ) { - const targetAccount = request.rateLimitTargetAccount - ? request.rateLimitTargetAccount - : authInfo.getCanonicalID(); - - cache.setCachedConfig( - cache.namespace.account, - targetAccount, - rateLimitConfig.account, - config.rateLimiting.account.configCacheTTL, - ); - checks.push(...buildRateChecksFromConfig('account', targetAccount, rateLimitConfig.account)); - // eslint-disable-next-line no-param-reassign - request.rateLimitAccountAlreadyChecked = true; - } + cache.setCachedConfig( + cache.namespace.account, + targetAccount, + rateLimitConfig.account, + config.rateLimiting.account.configCacheTTL, + ); + checks.push(...buildRateChecksFromConfig('account', targetAccount, rateLimitConfig.account)); + // eslint-disable-next-line no-param-reassign + request.rateLimitAccountAlreadyChecked = true; + } - const { allowed, rateLimitSource } = checkRateLimitsForRequest(checks, log); - if (!allowed) { - log.addDefaultFields({ - rateLimited: true, - rateLimitSource, - }); + const { allowed, rateLimitSource } = checkRateLimitsForRequest(checks, log); + if (!allowed) { + log.addDefaultFields({ + rateLimited: true, + rateLimitSource, + }); - if (request.serverAccessLog) { - /* eslint-disable no-param-reassign */ - request.serverAccessLog.rateLimited = true; - request.serverAccessLog.rateLimitSource = rateLimitSource; - /* eslint-enable no-param-reassign */ - } + if (request.serverAccessLog) { + /* eslint-disable no-param-reassign */ + request.serverAccessLog.rateLimited = true; + request.serverAccessLog.rateLimitSource = rateLimitSource; + /* eslint-enable no-param-reassign */ + } - return process.nextTick(callback, config.rateLimiting.error); - } + process.nextTick(callback, config.rateLimiting.error); + return; + } - return process.nextTick(callback, null); + process.nextTick(callback, null); + }) + .catch(err => callback(err)); } /** standardMetadataValidateBucketAndObj - retrieve bucket and object md from metadata diff --git a/package.json b/package.json index 45a9d65778..ae87c88935 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.22", + "arsenal": "git+https://github.com/scality/Arsenal#8.4.23", "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.7", + "vaultclient": "scality/vaultclient#8.5.8", "werelogs": "scality/werelogs#semver:^8.2.4", "ws": "^8.18.0", "xml2js": "^0.6.2" diff --git a/tests/unit/api/api.js b/tests/unit/api/api.js index ef1c8490b9..4b56489273 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 { config } = require('../../../lib/Config'); const rateLimitCache = require('../../../lib/api/apiUtils/rateLimit/cache'); const DummyRequest = require('../DummyRequest'); const { default: AuthInfo } = require('arsenal/build/lib/auth/AuthInfo'); @@ -155,8 +156,28 @@ describe('api.callApiMethod', () => { }); describe('cross-account rate limiting target account', () => { + beforeEach(() => { + 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 } }, + }, + }); + }); + afterEach(() => { rateLimitCache.bucketOwnerCache.clear(); + rateLimitCache.configCache.clear(); }); it('should pass the cached bucket owner to doAuth as targetAccount', done => { @@ -193,6 +214,54 @@ describe('api.callApiMethod', () => { }); api.callApiMethod('bucketGet', request, response, log); }); + + it('should not set targetAccount when rate limiting is disabled', done => { + sandbox.stub(config, 'rateLimiting').value({ enabled: false }); + request.bucketName = 'rl-bucket'; + 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); + }); + + it('should set targetAccount from the cached owner even when the account config is cached', done => { + request.bucketName = 'rl-bucket'; + rateLimitCache.setCachedBucketOwner('rl-bucket', 'owner-canonical-id', 30000); + rateLimitCache.setCachedConfig( + rateLimitCache.namespace.account, + 'owner-canonical-id', + { RequestsPerSecond: { Limit: 100, BurstCapacity: 1, source: 'resource' } }, + 30000, + ); + authServer.doAuth.resetBehavior(); + authServer.doAuth.callsFake((req, log, cb, awsService, requestContexts, authOptions) => { + assert.strictEqual(authOptions.targetAccount, 'owner-canonical-id'); + assert.strictEqual(request.rateLimitAccountAlreadyChecked, true); + done(); + }); + api.callApiMethod('bucketGet', request, response, log); + }); + + it('should store the limits returned by doAuth as the target account limits', done => { + request.bucketName = 'rl-bucket'; + const limits = { RequestsPerSecond: { Limit: 250 } }; + authServer.doAuth.resetBehavior(); + authServer.doAuth.callsArgWith(2, null, new AuthInfo({}), [{ isAllowed: true, isImplicit: false }], null, { + accountQuota: 5000, + limits, + }); + sandbox.stub(api, 'bucketGet').callsFake((userInfo, _request, log, cb) => cb()); + api.callApiMethod('bucketGet', request, response, log, err => { + assert.ifError(err); + assert.strictEqual(request.rateLimitTargetAccountLimits, limits); + assert.strictEqual(request.accountLimits, undefined); + done(); + }); + }); }); describe('MD5 checksum validation', () => { diff --git a/tests/unit/api/apiUtils/rateLimit/config.js b/tests/unit/api/apiUtils/rateLimit/config.js index e6fbd35e74..54fef13c18 100644 --- a/tests/unit/api/apiUtils/rateLimit/config.js +++ b/tests/unit/api/apiUtils/rateLimit/config.js @@ -138,10 +138,7 @@ describe('parseRateLimitConfig', () => { serviceUserArn: 'arn:aws:iam::123456789012:user/rate-limit-service', }; - assert.throws( - () => parseRateLimitConfig(config), - /rateLimiting configuration is invalid/ - ); + assert.throws(() => parseRateLimitConfig(config), /rateLimiting configuration is invalid/); }); }); @@ -151,7 +148,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), - /rateLimiting configuration is invalid.*"serviceUserArn" is required/ + /rateLimiting configuration is invalid.*"serviceUserArn" is required/, ); }); @@ -162,7 +159,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), - /rateLimiting configuration is invalid.*"serviceUserArn" must be a string/ + /rateLimiting configuration is invalid.*"serviceUserArn" must be a string/, ); }); }); @@ -195,7 +192,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), - /rateLimiting configuration is invalid.*"nodes" must be a positive number/ + /rateLimiting configuration is invalid.*"nodes" must be a positive number/, ); }); @@ -207,7 +204,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), - /rateLimiting configuration is invalid.*"nodes" must be a positive number/ + /rateLimiting configuration is invalid.*"nodes" must be a positive number/, ); }); @@ -219,7 +216,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), - /rateLimiting configuration is invalid.*"nodes" must be an integer/ + /rateLimiting configuration is invalid.*"nodes" must be an integer/, ); }); @@ -231,7 +228,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), - /rateLimiting configuration is invalid.*"nodes" must be a number/ + /rateLimiting configuration is invalid.*"nodes" must be a number/, ); }); }); @@ -264,7 +261,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), - /rateLimiting configuration is invalid.*"tokenBucketBufferSize" must be a positive number/ + /rateLimiting configuration is invalid.*"tokenBucketBufferSize" must be a positive number/, ); }); @@ -276,7 +273,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), - /rateLimiting configuration is invalid.*"tokenBucketBufferSize" must be a positive number/ + /rateLimiting configuration is invalid.*"tokenBucketBufferSize" must be a positive number/, ); }); @@ -288,7 +285,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), - /rateLimiting configuration is invalid.*"tokenBucketBufferSize" must be an integer/ + /rateLimiting configuration is invalid.*"tokenBucketBufferSize" must be an integer/, ); }); }); @@ -321,7 +318,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), - /rateLimiting configuration is invalid.*"tokenBucketRefillThreshold" must be a positive number/ + /rateLimiting configuration is invalid.*"tokenBucketRefillThreshold" must be a positive number/, ); }); @@ -333,7 +330,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), - /rateLimiting configuration is invalid.*"tokenBucketRefillThreshold" must be a positive number/ + /rateLimiting configuration is invalid.*"tokenBucketRefillThreshold" must be a positive number/, ); }); @@ -345,7 +342,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), - /rateLimiting configuration is invalid.*"tokenBucketRefillThreshold" must be an integer/ + /rateLimiting configuration is invalid.*"tokenBucketRefillThreshold" must be an integer/, ); }); }); @@ -359,7 +356,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), - /rateLimiting configuration is invalid.*"bucket" must be of type object/ + /rateLimiting configuration is invalid.*"bucket" must be of type object/, ); }); @@ -391,7 +388,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), - /rateLimiting configuration is invalid.*"bucket.defaultConfig" must be of type object/ + /rateLimiting configuration is invalid.*"bucket.defaultConfig" must be of type object/, ); }); @@ -407,7 +404,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), - /rateLimiting configuration is invalid.*"bucket.defaultConfig.requestsPerSecond" must be of type object/ + /rateLimiting configuration is invalid.*"bucket.defaultConfig.requestsPerSecond" must be of type object/, ); }); @@ -443,7 +440,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), // eslint-disable-next-line max-len - /rateLimiting configuration is invalid.*"bucket.defaultConfig.requestsPerSecond.limit" must be larger than or equal to 0/ + /rateLimiting configuration is invalid.*"bucket.defaultConfig.requestsPerSecond.limit" must be larger than or equal to 0/, ); }); @@ -461,7 +458,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), - /rateLimiting configuration is invalid.*"bucket.defaultConfig.requestsPerSecond.limit" is required/ + /rateLimiting configuration is invalid.*"bucket.defaultConfig.requestsPerSecond.limit" is required/, ); }); }); @@ -482,7 +479,7 @@ describe('parseRateLimitConfig', () => { const result = parseRateLimitConfig(config); assert.strictEqual( result.bucket.defaultConfig.RequestsPerSecond.BurstCapacity, - constants.rateLimitDefaultBurstCapacity + constants.rateLimitDefaultBurstCapacity, ); }); @@ -500,9 +497,7 @@ describe('parseRateLimitConfig', () => { }; const result = parseRateLimitConfig(config); - assert.strictEqual( - result.bucket.defaultConfig.RequestsPerSecond.BurstCapacity, 20 - ); + assert.strictEqual(result.bucket.defaultConfig.RequestsPerSecond.BurstCapacity, 20); }); it('should throw if burstCapacity is negative', () => { @@ -521,11 +516,11 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), // eslint-disable-next-line max-len - /rateLimiting configuration is invalid.*"bucket.defaultConfig.requestsPerSecond.burstCapacity" must be a positive number/ + /rateLimiting configuration is invalid.*"bucket.defaultConfig.requestsPerSecond.burstCapacity" must be larger than or equal to 0/, ); }); - it('should throw if burstCapacity is zero', () => { + it('should accept zero burstCapacity', () => { const config = { serviceUserArn: 'arn:aws:iam::123456789012:user/rate-limit-service', bucket: { @@ -538,11 +533,8 @@ describe('parseRateLimitConfig', () => { }, }; - assert.throws( - () => parseRateLimitConfig(config), - // eslint-disable-next-line max-len - /rateLimiting configuration is invalid.*"bucket.defaultConfig.requestsPerSecond.burstCapacity" must be a positive number/ - ); + const result = parseRateLimitConfig(config); + assert.strictEqual(result.bucket.defaultConfig.RequestsPerSecond.BurstCapacity, 0); }); it('should throw if burstCapacity is not a number', () => { @@ -561,7 +553,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), // eslint-disable-next-line max-len - /rateLimiting configuration is invalid.*"bucket.defaultConfig.requestsPerSecond.burstCapacity" must be a number/ + /rateLimiting configuration is invalid.*"bucket.defaultConfig.requestsPerSecond.burstCapacity" must be a number/, ); }); @@ -579,10 +571,53 @@ describe('parseRateLimitConfig', () => { }; const result = parseRateLimitConfig(config); - assert.strictEqual( - result.bucket.defaultConfig.RequestsPerSecond.BurstCapacity, 1.5 + assert.strictEqual(result.bucket.defaultConfig.RequestsPerSecond.BurstCapacity, 1.5); + }); + + it('should accept zero defaultBurstCapacity and apply it when burstCapacity is omitted', () => { + const config = { + serviceUserArn: 'arn:aws:iam::123456789012:user/rate-limit-service', + bucket: { + defaultConfig: { + requestsPerSecond: { + limit: 100, + }, + }, + defaultBurstCapacity: 0, + }, + }; + + const result = parseRateLimitConfig(config); + assert.strictEqual(result.bucket.defaultBurstCapacity, 0); + assert.strictEqual(result.bucket.defaultConfig.RequestsPerSecond.BurstCapacity, 0); + }); + + it('should throw if defaultBurstCapacity is negative', () => { + const config = { + serviceUserArn: 'arn:aws:iam::123456789012:user/rate-limit-service', + bucket: { + defaultBurstCapacity: -1, + }, + }; + + assert.throws( + () => parseRateLimitConfig(config), + /rateLimiting configuration is invalid.*"bucket.defaultBurstCapacity" must be larger than or equal to 0/, ); }); + + it('should accept float defaultBurstCapacity', () => { + const config = { + serviceUserArn: 'arn:aws:iam::123456789012:user/rate-limit-service', + bucket: { + defaultBurstCapacity: 1.5, + }, + }; + + const result = parseRateLimitConfig(config); + assert.strictEqual(result.bucket.defaultBurstCapacity, 1.5); + assert.strictEqual(result.bucket.defaultConfig.RequestsPerSecond.BurstCapacity, 1.5); + }); }); describe('bucket.configCacheTTL validation', () => { @@ -608,7 +643,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), - /rateLimiting configuration is invalid.*"bucket.configCacheTTL" must be a positive number/ + /rateLimiting configuration is invalid.*"bucket.configCacheTTL" must be a positive number/, ); }); @@ -622,7 +657,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), - /rateLimiting configuration is invalid.*"bucket.configCacheTTL" must be a positive number/ + /rateLimiting configuration is invalid.*"bucket.configCacheTTL" must be a positive number/, ); }); @@ -636,7 +671,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), - /rateLimiting configuration is invalid.*"bucket.configCacheTTL" must be an integer/ + /rateLimiting configuration is invalid.*"bucket.configCacheTTL" must be an integer/, ); }); @@ -650,7 +685,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), - /rateLimiting configuration is invalid.*"bucket.configCacheTTL" must be a number/ + /rateLimiting configuration is invalid.*"bucket.configCacheTTL" must be a number/, ); }); }); @@ -664,7 +699,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), - /rateLimiting configuration is invalid.*"error" must be of type object/ + /rateLimiting configuration is invalid.*"error" must be of type object/, ); }); @@ -707,7 +742,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), - /rateLimiting configuration is invalid.*"error.statusCode" must be larger than or equal to 400/ + /rateLimiting configuration is invalid.*"error.statusCode" must be larger than or equal to 400/, ); }); @@ -722,7 +757,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), - /rateLimiting configuration is invalid.*"error.statusCode" must be less than or equal to 599/ + /rateLimiting configuration is invalid.*"error.statusCode" must be less than or equal to 599/, ); }); @@ -737,7 +772,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), - /rateLimiting configuration is invalid.*"error.statusCode" must be an integer/ + /rateLimiting configuration is invalid.*"error.statusCode" must be an integer/, ); }); @@ -752,7 +787,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), - /rateLimiting configuration is invalid.*"error.statusCode" must be a number/ + /rateLimiting configuration is invalid.*"error.statusCode" must be a number/, ); }); @@ -781,7 +816,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), - /rateLimiting configuration is invalid.*"error.message" must be a string/ + /rateLimiting configuration is invalid.*"error.message" must be a string/, ); }); @@ -838,7 +873,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), - /rateLimiting configuration is invalid.*"error.code" must be a string/ + /rateLimiting configuration is invalid.*"error.code" must be a string/, ); }); }); @@ -857,10 +892,7 @@ describe('parseRateLimitConfig', () => { }, }; - assert.throws( - () => parseRateLimitConfig(config), - /requestsPerSecond\.limit \(3\) must be >= nodes \(5\)/ - ); + assert.throws(() => parseRateLimitConfig(config), /requestsPerSecond\.limit \(3\) must be >= nodes \(5\)/); }); }); @@ -929,10 +961,7 @@ describe('parseRateLimitConfig', () => { }, }; - assert.throws( - () => parseRateLimitConfig(config), - /requestsPerSecond\.limit \(7\) must be >= nodes \(10\)/ - ); + assert.throws(() => parseRateLimitConfig(config), /requestsPerSecond\.limit \(7\) must be >= nodes \(10\)/); }); it('should throw if account is not an object', () => { @@ -943,7 +972,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), - /rateLimiting configuration is invalid.*"account" must be of type object/ + /rateLimiting configuration is invalid.*"account" must be of type object/, ); }); @@ -982,7 +1011,7 @@ describe('parseRateLimitConfig', () => { const result = parseRateLimitConfig(config); assert.strictEqual( result.account.defaultConfig.RequestsPerSecond.BurstCapacity, - constants.rateLimitDefaultBurstCapacity + constants.rateLimitDefaultBurstCapacity, ); }); @@ -1000,9 +1029,7 @@ describe('parseRateLimitConfig', () => { }; const result = parseRateLimitConfig(config); - assert.strictEqual( - result.account.defaultConfig.RequestsPerSecond.BurstCapacity, 20 - ); + assert.strictEqual(result.account.defaultConfig.RequestsPerSecond.BurstCapacity, 20); }); it('should accept float burstCapacity', () => { @@ -1019,9 +1046,7 @@ describe('parseRateLimitConfig', () => { }; const result = parseRateLimitConfig(config); - assert.strictEqual( - result.account.defaultConfig.RequestsPerSecond.BurstCapacity, 1.5 - ); + assert.strictEqual(result.account.defaultConfig.RequestsPerSecond.BurstCapacity, 1.5); }); it('should throw if burstCapacity is negative', () => { @@ -1040,11 +1065,11 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), // eslint-disable-next-line max-len - /rateLimiting configuration is invalid.*"account.defaultConfig.requestsPerSecond.burstCapacity" must be a positive number/ + /rateLimiting configuration is invalid.*"account.defaultConfig.requestsPerSecond.burstCapacity" must be larger than or equal to 0/, ); }); - it('should throw if burstCapacity is zero', () => { + it('should accept zero burstCapacity', () => { const config = { serviceUserArn: 'arn:aws:iam::123456789012:user/rate-limit-service', account: { @@ -1057,11 +1082,8 @@ describe('parseRateLimitConfig', () => { }, }; - assert.throws( - () => parseRateLimitConfig(config), - // eslint-disable-next-line max-len - /rateLimiting configuration is invalid.*"account.defaultConfig.requestsPerSecond.burstCapacity" must be a positive number/ - ); + const result = parseRateLimitConfig(config); + assert.strictEqual(result.account.defaultConfig.RequestsPerSecond.BurstCapacity, 0); }); it('should throw if burstCapacity is not a number', () => { @@ -1080,7 +1102,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), // eslint-disable-next-line max-len - /rateLimiting configuration is invalid.*"account.defaultConfig.requestsPerSecond.burstCapacity" must be a number/ + /rateLimiting configuration is invalid.*"account.defaultConfig.requestsPerSecond.burstCapacity" must be a number/, ); }); }); @@ -1094,7 +1116,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), - /rateLimiting configuration is invalid.*"unknownField" is not allowed/ + /rateLimiting configuration is invalid.*"unknownField" is not allowed/, ); }); @@ -1108,7 +1130,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), - /rateLimiting configuration is invalid.*"bucket.unknownField" is not allowed/ + /rateLimiting configuration is invalid.*"bucket.unknownField" is not allowed/, ); }); @@ -1123,7 +1145,7 @@ describe('parseRateLimitConfig', () => { assert.throws( () => parseRateLimitConfig(config), - /rateLimiting configuration is invalid.*"error.unknownField" is not allowed/ + /rateLimiting configuration is invalid.*"error.unknownField" is not allowed/, ); }); }); @@ -1171,7 +1193,7 @@ describe('parseRateLimitConfig', () => { assert.strictEqual(result.bucket.defaultConfig.RequestsPerSecond.Limit, 100); assert.strictEqual( result.bucket.defaultConfig.RequestsPerSecond.BurstCapacity, - constants.rateLimitDefaultBurstCapacity + constants.rateLimitDefaultBurstCapacity, ); }); diff --git a/tests/unit/api/apiUtils/rateLimit/helpers.js b/tests/unit/api/apiUtils/rateLimit/helpers.js index 953f335ce0..a3ff6bfddd 100644 --- a/tests/unit/api/apiUtils/rateLimit/helpers.js +++ b/tests/unit/api/apiUtils/rateLimit/helpers.js @@ -1,6 +1,9 @@ const assert = require('assert'); const sinon = require('sinon'); +const { errors } = require('arsenal'); +const constants = require('../../../../../constants'); const { config } = require('../../../../../lib/Config'); +const vault = require('../../../../../lib/auth/vault'); const cache = require('../../../../../lib/api/apiUtils/rateLimit/cache'); const helpers = require('../../../../../lib/api/apiUtils/rateLimit/helpers'); const tokenBucket = require('../../../../../lib/api/apiUtils/rateLimit/tokenBucket'); @@ -45,8 +48,11 @@ describe('Rate limit helpers', () => { for (const action of helpers.rateLimitApiActions) { const request = { apiMethod: action }; - assert.strictEqual(helpers.requestNeedsRateCheck(request), false, - `Expected false for rate limit action: ${action}`); + assert.strictEqual( + helpers.requestNeedsRateCheck(request), + false, + `Expected false for rate limit action: ${action}`, + ); } }); @@ -228,7 +234,7 @@ describe('Rate limit helpers', () => { bucket: { defaultBurstCapacity: 1, }, - }) + }), ); afterEach(() => sinon.restore()); @@ -241,8 +247,11 @@ describe('Rate limit helpers', () => { it('should allow request when bucket has capacity', () => { const check = { - resourceClass: 'bkt', resourceId: 'test-bucket', measure: 'rps', - config: { limit: 100, burstCapacity: 1000, source: 'bucket' }, source: 'bucket', + resourceClass: 'bkt', + resourceId: 'test-bucket', + measure: 'rps', + config: { limit: 100, burstCapacity: 1000, source: 'bucket' }, + source: 'bucket', }; // Pre-populate token bucket with tokens @@ -258,8 +267,11 @@ describe('Rate limit helpers', () => { it('should deny request when bucket has no tokens', () => { const check = { - resourceClass: 'bkt', resourceId: 'test-bucket', measure: 'rps', - config: { limit: 100, burstCapacity: 1000, source: 'bucket' }, source: 'bucket', + resourceClass: 'bkt', + resourceId: 'test-bucket', + measure: 'rps', + config: { limit: 100, burstCapacity: 1000, source: 'bucket' }, + source: 'bucket', }; const bucket = tokenBucket.getTokenBucket('bkt', 'test-bucket', 'rps', check.config, mockLog); @@ -274,8 +286,11 @@ describe('Rate limit helpers', () => { it('should not consume tokens when denied', () => { const check = { - resourceClass: 'bkt', resourceId: 'test-bucket', measure: 'rps', - config: { limit: 100, burstCapacity: 1000, source: 'bucket' }, source: 'bucket', + resourceClass: 'bkt', + resourceId: 'test-bucket', + measure: 'rps', + config: { limit: 100, burstCapacity: 1000, source: 'bucket' }, + source: 'bucket', }; const bucket = tokenBucket.getTokenBucket('bkt', 'test-bucket', 'rps', check.config, mockLog); @@ -288,12 +303,18 @@ describe('Rate limit helpers', () => { it('should consume tokens from all buckets when all have capacity', () => { const check1 = { - resourceClass: 'bkt', resourceId: 'bucket-1', measure: 'rps', - config: { limit: 100, burstCapacity: 1000, source: 'bucket' }, source: 'bucket', + resourceClass: 'bkt', + resourceId: 'bucket-1', + measure: 'rps', + config: { limit: 100, burstCapacity: 1000, source: 'bucket' }, + source: 'bucket', }; const check2 = { - resourceClass: 'acc', resourceId: 'account-1', measure: 'rps', - config: { limit: 200, burstCapacity: 1000, source: 'account' }, source: 'account', + resourceClass: 'acc', + resourceId: 'account-1', + measure: 'rps', + config: { limit: 200, burstCapacity: 1000, source: 'account' }, + source: 'account', }; const bucket1 = tokenBucket.getTokenBucket('bkt', 'bucket-1', 'rps', check1.config, mockLog); @@ -310,12 +331,18 @@ describe('Rate limit helpers', () => { it('should deny on first exhausted bucket and not consume other buckets', () => { const check1 = { - resourceClass: 'bkt', resourceId: 'bucket-1', measure: 'rps', - config: { limit: 100, burstCapacity: 1000, source: 'bucket' }, source: 'bucket', + resourceClass: 'bkt', + resourceId: 'bucket-1', + measure: 'rps', + config: { limit: 100, burstCapacity: 1000, source: 'bucket' }, + source: 'bucket', }; const check2 = { - resourceClass: 'acc', resourceId: 'account-1', measure: 'rps', - config: { limit: 200, burstCapacity: 1000, source: 'account' }, source: 'account', + resourceClass: 'acc', + resourceId: 'account-1', + measure: 'rps', + config: { limit: 200, burstCapacity: 1000, source: 'account' }, + source: 'account', }; const bucket1 = tokenBucket.getTokenBucket('bkt', 'bucket-1', 'rps', check1.config, mockLog); @@ -332,8 +359,11 @@ describe('Rate limit helpers', () => { it('should log debug info when request is denied', () => { const check = { - resourceClass: 'bkt', resourceId: 'test-bucket', measure: 'rps', - config: { limit: 100, burstCapacity: 1000, source: 'bucket' }, source: 'bucket', + resourceClass: 'bkt', + resourceId: 'test-bucket', + measure: 'rps', + config: { limit: 100, burstCapacity: 1000, source: 'bucket' }, + source: 'bucket', }; const bucket = tokenBucket.getTokenBucket('bkt', 'test-bucket', 'rps', check.config, mockLog); @@ -342,9 +372,9 @@ describe('Rate limit helpers', () => { helpers.checkRateLimitsForRequest([check], mockLog); - const deniedCall = mockLog.debug.getCalls().find( - call => call.args[0] === 'Rate limit check: denied (no tokens available)' - ); + const deniedCall = mockLog.debug + .getCalls() + .find(call => call.args[0] === 'Rate limit check: denied (no tokens available)'); assert(deniedCall, 'Should have logged denied message'); const logArgs = deniedCall.args[1]; assert.strictEqual(logArgs.resourceClass, 'bkt'); @@ -355,8 +385,11 @@ describe('Rate limit helpers', () => { it('should log trace info when request is allowed', () => { const check = { - resourceClass: 'bkt', resourceId: 'test-bucket', measure: 'rps', - config: { limit: 100, burstCapacity: 1000, source: 'bucket' }, source: 'bucket', + resourceClass: 'bkt', + resourceId: 'test-bucket', + measure: 'rps', + config: { limit: 100, burstCapacity: 1000, source: 'bucket' }, + source: 'bucket', }; // Pre-populate token bucket @@ -366,9 +399,9 @@ describe('Rate limit helpers', () => { const result = helpers.checkRateLimitsForRequest([check], mockLog); assert.strictEqual(result.allowed, true); - const allowedCall = mockLog.trace.getCalls().find( - call => call.args[0] === 'Rate limit check: allowed (token consumed)' - ); + const allowedCall = mockLog.trace + .getCalls() + .find(call => call.args[0] === 'Rate limit check: allowed (token consumed)'); assert(allowedCall, 'Should have logged allowed message'); assert.strictEqual(allowedCall.args[1].resourceClass, 'bkt'); assert.strictEqual(allowedCall.args[1].resourceId, 'test-bucket'); @@ -376,8 +409,11 @@ describe('Rate limit helpers', () => { it('should handle multiple sequential requests correctly', () => { const check = { - resourceClass: 'bkt', resourceId: 'test-bucket', measure: 'rps', - config: { limit: 100, burstCapacity: 1000, source: 'bucket' }, source: 'bucket', + resourceClass: 'bkt', + resourceId: 'test-bucket', + measure: 'rps', + config: { limit: 100, burstCapacity: 1000, source: 'bucket' }, + source: 'bucket', }; // Pre-populate token bucket with multiple tokens @@ -396,7 +432,26 @@ describe('Rate limit helpers', () => { }); }); - describe('extractRateLimitConfigFromRequest', () => { + describe('resolveRateLimitConfig', () => { + const ownerId = 'owner-canonical-id'; + let vaultStub; + + function makeBucket(rpsData) { + return { + getName: () => 'test-bucket', + getOwner: () => ownerId, + getRateLimitConfiguration: () => + rpsData === undefined ? null : { getData: () => ({ RequestsPerSecond: rpsData }) }, + }; + } + + function makeAuthInfo(canonicalId, isPublic = false) { + return { + getCanonicalID: () => canonicalId, + isRequesterPublicUser: () => isPublic, + }; + } + beforeEach(() => { sandbox.stub(config, 'rateLimiting').value({ enabled: true, @@ -414,28 +469,22 @@ describe('Rate limit helpers', () => { }, }, }); + vaultStub = sandbox.stub(vault, 'getAccountLimitsByCanonicalId'); }); - it('should return both bucket and account configs', () => { - const mockBucket = { - getName: () => 'test-bucket', - getRateLimitConfiguration: () => ({ - getData: () => ({ - RequestsPerSecond: { Limit: 200 }, - }), - }), - }; - const mockAuthInfo = { - getCanonicalID: () => 'account-123', - }; + it('should return both bucket and account configs', async () => { const request = { - accountLimits: { + rateLimitTargetAccountLimits: { RequestsPerSecond: { Limit: 500 }, }, }; - const result = helpers.extractRateLimitConfigFromRequest( - request, mockAuthInfo, mockBucket, mockLog); + const result = await helpers.resolveRateLimitConfig( + request, + makeAuthInfo(ownerId), + makeBucket({ Limit: 200 }), + mockLog, + ); assert.deepStrictEqual(result.bucket, { RequestsPerSecond: { BurstCapacity: 1, Limit: 200, source: 'resource' }, @@ -443,20 +492,16 @@ describe('Rate limit helpers', () => { assert.deepStrictEqual(result.account, { RequestsPerSecond: { BurstCapacity: 2, Limit: 500, source: 'resource' }, }); + assert.strictEqual(vaultStub.called, false); }); - it('should use global defaults when no per-resource configs exist', () => { - const mockBucket = { - getName: () => 'test-bucket', - getRateLimitConfiguration: () => null, - }; - const mockAuthInfo = { - getCanonicalID: () => 'account-123', - }; - const request = {}; - - const result = helpers.extractRateLimitConfigFromRequest( - request, mockAuthInfo, mockBucket, mockLog); + it('should use global defaults when no per-resource configs exist', async () => { + const result = await helpers.resolveRateLimitConfig( + {}, + makeAuthInfo(ownerId), + makeBucket(undefined), + mockLog, + ); assert.deepStrictEqual(result.bucket, { RequestsPerSecond: { BurstCapacity: 1, source: 'global' }, @@ -466,49 +511,176 @@ describe('Rate limit helpers', () => { }); }); - it('should extract per-account config with resource source', () => { - const mockBucket = { - getName: () => 'test-bucket', - getRateLimitConfiguration: () => null, - }; - const mockAuthInfo = { - getCanonicalID: () => 'account-123', - }; + it('should extract per-account config with resource source', async () => { const request = { - accountLimits: { + rateLimitTargetAccountLimits: { RequestsPerSecond: { Limit: 300, BurstCapacity: 5 }, }, }; - const result = helpers.extractRateLimitConfigFromRequest( - request, mockAuthInfo, mockBucket, mockLog); + const result = await helpers.resolveRateLimitConfig( + request, + makeAuthInfo(ownerId), + makeBucket(undefined), + mockLog, + ); assert.deepStrictEqual(result.account, { RequestsPerSecond: { BurstCapacity: 5, Limit: 300, source: 'resource' }, }); }); - it('should use global source when accountLimits has no RequestsPerSecond', () => { - const mockBucket = { - getName: () => 'test-bucket', - getRateLimitConfiguration: () => null, + it('should use global source when target account limits have no RequestsPerSecond', async () => { + const request = { + rateLimitTargetAccountLimits: { + RequestsPerSecond: undefined, + }, }; - const mockAuthInfo = { - getCanonicalID: () => 'account-123', + + const result = await helpers.resolveRateLimitConfig( + request, + makeAuthInfo(ownerId), + makeBucket(undefined), + mockLog, + ); + + assert.deepStrictEqual(result.account, { + RequestsPerSecond: { BurstCapacity: 2, source: 'global' }, + }); + }); + + it('should reuse the limits from the request on a cross-account cache hit', async () => { + // The bucket owner was cached, so doAuth was already given the + // target account and returned the owner's limits. + const request = { + rateLimitTargetAccount: ownerId, + rateLimitTargetAccountLimits: { + RequestsPerSecond: { Limit: 400 }, + }, }; + + const result = await helpers.resolveRateLimitConfig( + request, + makeAuthInfo('other-canonical-id'), + makeBucket(undefined), + mockLog, + ); + + assert.deepStrictEqual(result.account, { + RequestsPerSecond: { BurstCapacity: 2, Limit: 400, source: 'resource' }, + }); + assert.strictEqual(vaultStub.called, false); + }); + + it('should fetch the owner limits from Vault on a cross-account cache miss', async () => { + vaultStub.yields(null, { RequestsPerSecond: { Limit: 750 } }); + // No rateLimitTargetAccount: the bucket owner was not cached, so + // doAuth returned the requester's limits, not the owner's. const request = { - accountLimits: { - RequestsPerSecond: undefined, + rateLimitTargetAccountLimits: { + RequestsPerSecond: { Limit: 1 }, }, }; - const result = helpers.extractRateLimitConfigFromRequest( - request, mockAuthInfo, mockBucket, mockLog); + const result = await helpers.resolveRateLimitConfig( + request, + makeAuthInfo('other-canonical-id'), + makeBucket(undefined), + mockLog, + ); + + assert.strictEqual(vaultStub.calledOnce, true); + assert.strictEqual(vaultStub.firstCall.args[0], ownerId); + assert.deepStrictEqual(result.account, { + RequestsPerSecond: { BurstCapacity: 2, Limit: 750, source: 'resource' }, + }); + }); + + it('should fetch the owner limits from Vault for an anonymous requester', async () => { + vaultStub.yields(null, { RequestsPerSecond: { Limit: 60 } }); + + const result = await helpers.resolveRateLimitConfig( + {}, + makeAuthInfo(constants.publicId, true), + makeBucket(undefined), + mockLog, + ); + assert.strictEqual(vaultStub.calledOnce, true); + assert.strictEqual(vaultStub.firstCall.args[0], ownerId); + assert.deepStrictEqual(result.account, { + RequestsPerSecond: { BurstCapacity: 2, Limit: 60, source: 'resource' }, + }); + }); + + it('should fetch the owner limits for an anonymous requester on their own bucket', async () => { + // An anonymous request never went through account auth, so even + // when the canonical IDs match there are no limits on the request. + vaultStub.yields(null, { RequestsPerSecond: { Limit: 60 } }); + const ownerBucket = makeBucket(undefined); + sandbox.stub(ownerBucket, 'getOwner').value(() => constants.publicId); + + await helpers.resolveRateLimitConfig({}, makeAuthInfo(constants.publicId, true), ownerBucket, mockLog); + + assert.strictEqual(vaultStub.calledOnce, true); + assert.strictEqual(vaultStub.firstCall.args[0], constants.publicId); + }); + + it('should not fetch from Vault for a same-account request', async () => { + const result = await helpers.resolveRateLimitConfig( + {}, + makeAuthInfo(ownerId), + makeBucket(undefined), + mockLog, + ); + + assert.strictEqual(vaultStub.called, false); + assert.deepStrictEqual(result.account, { + RequestsPerSecond: { BurstCapacity: 2, source: 'global' }, + }); + }); + + it('should fall back to global defaults when Vault returns no limits', async () => { + vaultStub.yields(null, undefined); + + const result = await helpers.resolveRateLimitConfig( + {}, + makeAuthInfo('other-canonical-id'), + makeBucket(undefined), + mockLog, + ); + + assert.strictEqual(vaultStub.calledOnce, true); assert.deepStrictEqual(result.account, { RequestsPerSecond: { BurstCapacity: 2, source: 'global' }, }); }); + + it('should reject when Vault returns an error', async () => { + vaultStub.yields(errors.InternalError); + + await assert.rejects( + helpers.resolveRateLimitConfig({}, makeAuthInfo('other-canonical-id'), makeBucket(undefined), mockLog), + err => err.is.InternalError, + ); + }); + + it('should key the account config under the bucket owner, not the requester', async () => { + vaultStub.yields(null, { RequestsPerSecond: { Limit: 10 } }); + + await helpers.resolveRateLimitConfig( + {}, + makeAuthInfo('other-canonical-id'), + makeBucket(undefined), + mockLog, + ); + + const debugCall = mockLog.debug + .getCalls() + .find(call => call.args[0] === 'Extracted per-account rate limit config'); + assert(debugCall, 'Should have logged the per-account config'); + assert.strictEqual(debugCall.args[1].canonicalId, ownerId); + }); }); describe('buildRateChecksFromConfig', () => { @@ -646,14 +818,16 @@ describe('Rate limit helpers', () => { assert.strictEqual(result.bucketOwner, 'owner-123'); }); - it('should not return account config when bucket owner is cached but account config is not', () => { + it('should return the bucket owner without an account config when only the owner is cached', () => { + // The owner is reported even without a cached account config so the + // caller can still pass it to Vault as the rate limit target account. cache.setCachedBucketOwner('test-bucket', 'owner-123', 30000); const request = { bucketName: 'test-bucket' }; const result = helpers.getCachedRateLimitConfig(request); assert.strictEqual(result.account, undefined); - assert.strictEqual(result.bucketOwner, undefined); + assert.strictEqual(result.bucketOwner, 'owner-123'); }); it('should not return account config when bucket owner is not cached', () => { diff --git a/tests/unit/metadata/metadataUtils.spec.js b/tests/unit/metadata/metadataUtils.spec.js index b9dc4da84f..b722bdb9c1 100644 --- a/tests/unit/metadata/metadataUtils.spec.js +++ b/tests/unit/metadata/metadataUtils.spec.js @@ -25,6 +25,7 @@ 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'); +const vault = require('../../../lib/auth/vault'); describe('validateBucket', () => { it('action bucketPutPolicy by bucket owner', () => { @@ -212,11 +213,15 @@ describe('storeServerAccessLogInfo - copySource aclRequired', () => { describe('checkRateLimitIfNeeded cross-account rate limiting', () => { let sandbox; let request; + let vaultStub; const otherCanonicalId = otherAuthInfo.getCanonicalID(); beforeEach(() => { sandbox = sinon.createSandbox(); + // Vault is consulted for the bucket owner's limits whenever they could + // not have been returned by the request's own authentication. + vaultStub = sandbox.stub(vault, 'getAccountLimitsByCanonicalId').yields(null, undefined); sandbox.stub(config, 'rateLimiting').value({ enabled: true, serviceUserArn: 'arn:aws:iam::000000000000:user/rate-limit-service-user', @@ -273,8 +278,8 @@ describe('checkRateLimitIfNeeded cross-account rate limiting', () => { }); }); - it('should key the account rate limit under the requester canonical ID by default', done => { - request.accountLimits = { RequestsPerSecond: { Limit: 100 } }; + it('should key the account rate limit under the bucket owner for a same-account request', done => { + request.rateLimitTargetAccountLimits = { RequestsPerSecond: { Limit: 100 } }; validateBucketRequest(authInfo, err => { assert.ifError(err); const cached = rateLimitCache.getCachedConfig(rateLimitCache.namespace.account, ownerCanonicalId); @@ -283,12 +288,14 @@ describe('checkRateLimitIfNeeded cross-account rate limiting', () => { }); assert(tokenBucket.getAllTokenBuckets().has(`account:${ownerCanonicalId}:rps`)); assert.strictEqual(request.rateLimitAccountAlreadyChecked, true); + // The requester is the owner, so their own auth already returned the limits + assert.strictEqual(vaultStub.called, false); done(); }); }); it('should key the account rate limit under the target account when the request carries one', done => { - request.accountLimits = { RequestsPerSecond: { Limit: 100 } }; + request.rateLimitTargetAccountLimits = { RequestsPerSecond: { Limit: 100 } }; request.rateLimitTargetAccount = ownerCanonicalId; validateBucketRequest(otherAuthInfo, () => { assert(rateLimitCache.getCachedConfig(rateLimitCache.namespace.account, ownerCanonicalId)); @@ -298,12 +305,13 @@ describe('checkRateLimitIfNeeded cross-account rate limiting', () => { ); assert(tokenBucket.getAllTokenBuckets().has(`account:${ownerCanonicalId}:rps`)); assert(!tokenBucket.getAllTokenBuckets().has(`account:${otherCanonicalId}:rps`)); + assert.strictEqual(vaultStub.called, false); done(); }); }); it('should deny a cross-account request when the target account limit is exhausted', done => { - request.accountLimits = { RequestsPerSecond: { Limit: 100 } }; + request.rateLimitTargetAccountLimits = { RequestsPerSecond: { Limit: 100 } }; request.rateLimitTargetAccount = ownerCanonicalId; const ownerBucket = tokenBucket.getTokenBucket( 'account', @@ -320,8 +328,24 @@ describe('checkRateLimitIfNeeded cross-account rate limiting', () => { }); }); - it('should rate limit against the requester account when no target account is present', done => { - request.accountLimits = { RequestsPerSecond: { Limit: 100 } }; + it('should fetch the owner limits from Vault when no target account is present', done => { + // Cross-account request with a bucket owner cache miss: the requester's + // own auth returned their limits, not the owner's, so Vault is asked. + vaultStub.yields(null, { RequestsPerSecond: { Limit: 100 } }); + validateBucketRequest(otherAuthInfo, () => { + assert.strictEqual(vaultStub.calledOnce, true); + assert.strictEqual(vaultStub.firstCall.args[0], ownerCanonicalId); + assert.deepStrictEqual(rateLimitCache.getCachedConfig(rateLimitCache.namespace.account, ownerCanonicalId), { + RequestsPerSecond: { BurstCapacity: 1, Limit: 100, source: 'resource' }, + }); + assert(tokenBucket.getAllTokenBuckets().has(`account:${ownerCanonicalId}:rps`)); + assert(!tokenBucket.getAllTokenBuckets().has(`account:${otherCanonicalId}:rps`)); + done(); + }); + }); + + it('should rate limit a cross-account request against the owner, not the requester', done => { + vaultStub.yields(null, { RequestsPerSecond: { Limit: 100 } }); const ownerBucket = tokenBucket.getTokenBucket( 'account', ownerCanonicalId, @@ -331,35 +355,96 @@ describe('checkRateLimitIfNeeded cross-account rate limiting', () => { ); 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`)); + assert(err); + assert(err.is.SlowDown); done(); }); }); - it('should skip the account rate limit check for public requesters', done => { + it('should rate limit public requesters against the bucket owner', done => { const publicAuthInfo = makeAuthInfo(constants.publicId); - request.accountLimits = { RequestsPerSecond: { Limit: 100 } }; + vaultStub.yields(null, { RequestsPerSecond: { Limit: 100 } }); validateBucketRequest(publicAuthInfo, () => { - assert.strictEqual(request.rateLimitAccountAlreadyChecked, undefined); + // Anonymous requests never authenticate an account, so the owner's + // limits always come from Vault. + assert.strictEqual(vaultStub.calledOnce, true); + assert.strictEqual(vaultStub.firstCall.args[0], ownerCanonicalId); + assert.strictEqual(request.rateLimitAccountAlreadyChecked, true); + assert.deepStrictEqual(rateLimitCache.getCachedConfig(rateLimitCache.namespace.account, ownerCanonicalId), { + RequestsPerSecond: { BurstCapacity: 1, Limit: 100, source: 'resource' }, + }); assert.strictEqual( rateLimitCache.getCachedConfig(rateLimitCache.namespace.account, constants.publicId), undefined, ); - // The bucket owner is still cached for later cross-account attribution + assert(tokenBucket.getAllTokenBuckets().has(`account:${ownerCanonicalId}:rps`)); + assert(!tokenBucket.getAllTokenBuckets().has(`account:${constants.publicId}:rps`)); 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 => { + it('should deny a public request when the bucket owner limit is exhausted', done => { + const publicAuthInfo = makeAuthInfo(constants.publicId); + vaultStub.yields(null, { RequestsPerSecond: { Limit: 100 } }); + const ownerBucket = tokenBucket.getTokenBucket( + 'account', + ownerCanonicalId, + 'rps', + { limit: 100, burstCapacity: 1000 }, + log, + ); + ownerBucket.tokens = 0; + validateBucketRequest(publicAuthInfo, err => { + assert(err); + assert(err.is.SlowDown); + done(); + }); + }); + + it('should refetch the owner limits for a public requester even with a target account', done => { + // The cached bucket owner made doAuth return the owner's limits, but an + // anonymous request has no account auth, so Vault is still consulted. const publicAuthInfo = makeAuthInfo(constants.publicId); - request.accountLimits = { RequestsPerSecond: { Limit: 100 } }; request.rateLimitTargetAccount = ownerCanonicalId; + request.rateLimitTargetAccountLimits = { RequestsPerSecond: { Limit: 5 } }; + vaultStub.yields(null, { RequestsPerSecond: { Limit: 100 } }); validateBucketRequest(publicAuthInfo, () => { - assert.strictEqual(request.rateLimitAccountAlreadyChecked, undefined); + assert.strictEqual(vaultStub.calledOnce, true); + assert.deepStrictEqual(rateLimitCache.getCachedConfig(rateLimitCache.namespace.account, ownerCanonicalId), { + RequestsPerSecond: { BurstCapacity: 1, Limit: 100, source: 'resource' }, + }); + done(); + }); + }); + + it('should fall back to the global account defaults when Vault returns no limits', done => { + const publicAuthInfo = makeAuthInfo(constants.publicId); + vaultStub.yields(null, undefined); + validateBucketRequest(publicAuthInfo, () => { + assert.deepStrictEqual(rateLimitCache.getCachedConfig(rateLimitCache.namespace.account, ownerCanonicalId), { + RequestsPerSecond: { BurstCapacity: 1, source: 'global' }, + }); + // No Limit in the global defaults, so no token bucket is created + assert(!tokenBucket.getAllTokenBuckets().has(`account:${ownerCanonicalId}:rps`)); + done(); + }); + }); + + it('should propagate a Vault error to the callback', done => { + vaultStub.yields(errors.InternalError); + validateBucketRequest(otherAuthInfo, err => { + assert(err); + assert(err.is.InternalError); + done(); + }); + }); + + it('should still cache the bucket owner when the account config was already checked', done => { + request.rateLimitAccountAlreadyChecked = true; + validateBucketRequest(authInfo, err => { + assert.ifError(err); + assert.strictEqual(rateLimitCache.getCachedBucketOwner(bucket.getName()), ownerCanonicalId); assert.strictEqual( rateLimitCache.getCachedConfig(rateLimitCache.namespace.account, ownerCanonicalId), undefined, diff --git a/yarn.lock b/yarn.lock index ca1db57c42..85328e79fb 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.22": - version "8.4.22" - resolved "git+https://github.com/scality/Arsenal#64484bec1e2b8948f7834f65d29ddc7461560afd" +"arsenal@git+https://github.com/scality/Arsenal#8.4.23": + version "8.4.23" + resolved "git+https://github.com/scality/Arsenal#c3f156440b27ac722240e86679de80ad2c43a45e" dependencies: "@aws-sdk/client-kms" "^3.975.0" "@aws-sdk/client-s3" "^3.975.0" @@ -12603,9 +12603,9 @@ 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" +vaultclient@scality/vaultclient#8.5.8: + version "8.5.8" + resolved "https://codeload.github.com/scality/vaultclient/tar.gz/05decff559dacb387c436f9fb4923eaf1f1fa5cb" dependencies: "@aws-crypto/sha256-universal" "^5.2.0" "@smithy/signature-v4" "^4.1.0"