From 130300aa2e9430085a03479cec3d517ec6f0d37d Mon Sep 17 00:00:00 2001 From: Nicolas Humbert Date: Fri, 22 Nov 2024 16:53:30 +0100 Subject: [PATCH] CLDSRV-584 Limit backbeat API versioning check to replication operations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Context: In Artesca, when creating a location (whether used for replication or not), you specify both the endpoint and the bucket name where data will be stored. At this stage, Zenko checks that the bucket has versioning enabled. In S3C, it is a bit different, a “replication location” consists of a list of S3C endpoints (replicationEndpoints), and the destination bucket is specified later in the API replication rule (with put-bucket-replication API). Currently, S3C does not check if the destination bucket has versioning enabled. This means data could be replicated from a versioned source bucket to a non-versioned or suspended destination bucket, which can cause issues. e.g. https://scality.atlassian.net/browse/S3C-821 Current solution: A validation step was added to the Backbeat API in cloudserver. This check rejects any requests made to buckets that either do not have versioning enabled or have versioning suspended. This check makes sure the Backbeat API only works with buckets that have versioning enabled. This prevents operations on destination buckets where versioning is disabled or suspended, hense maintaining proper version control. On the source, we should be fine. Cloudserver prevents setting up replication on a bucket if its versioning is disabled or suspended, and it prevents changing a bucket’s versioning to suspended while replication is ongoing. Issue: The Backbeat API is also used for lifecycle, which works on both versioned and non-versioned buckets. The current versioning check affects the lifecycle operations, which should be allowed on non-versioned buckets. Proposed solution: Modify the Backbeat API to apply the bucket versioning check only to actions related to the replication destination buckets (such as PUT, POST, and DELETE requests). Allow GET actions like getMetadata and headLocation, which are used in workflows that don’t affect replication destination buckets. This change will ensure that replication only targets versioned buckets and allows lifecycle operations on non-versioned buckets. --- lib/routes/routeBackbeat.js | 5 +- tests/unit/routes/routeBackbeat.js | 142 +++++++++++++++++++++++++++++ 2 files changed, 146 insertions(+), 1 deletion(-) create mode 100644 tests/unit/routes/routeBackbeat.js diff --git a/lib/routes/routeBackbeat.js b/lib/routes/routeBackbeat.js index c2225b6a76..defffcc4da 100644 --- a/lib/routes/routeBackbeat.js +++ b/lib/routes/routeBackbeat.js @@ -1289,7 +1289,10 @@ function routeBackbeat(clientIP, request, response, log) { [request.query.operation](request, response, log, next); } const versioningConfig = bucketInfo.getVersioningConfiguration(); - if (!versioningConfig || versioningConfig.Status !== 'Enabled') { + // The following makes sure that only replication destination-related operations (non-GET requests) + // target buckets with versioning enabled. + // This allows lifecycle operations (which use GET requests) to work on non-versioned buckets. + if (request.method !== 'GET' && (!versioningConfig || versioningConfig.Status !== 'Enabled')) { log.debug('bucket versioning is not enabled', { method: request.method, bucketName: request.bucketName, diff --git a/tests/unit/routes/routeBackbeat.js b/tests/unit/routes/routeBackbeat.js new file mode 100644 index 0000000000..788d2eaf48 --- /dev/null +++ b/tests/unit/routes/routeBackbeat.js @@ -0,0 +1,142 @@ +const assert = require('assert'); +const sinon = require('sinon'); +const metadataUtils = require('../../../lib/metadata/metadataUtils'); +const metadata = require('../../../lib/metadata/wrapper'); +const { DummyRequestLogger } = require('../helpers'); +const DummyRequest = require('../DummyRequest'); + +const log = new DummyRequestLogger(); + +function prepareDummyRequest(headers = {}) { + const request = new DummyRequest({ + hostname: 'localhost', + method: 'PUT', + url: '/_/backbeat/metadata/bucket0/key0', + port: 80, + headers, + socket: { + remoteAddress: '0.0.0.0', + }, + }, '{"replicationInfo":"{}"}'); + return request; +} + +describe('routeBackbeat', () => { + let mockResponse; + let mockRequest; + let sandbox; + let endPromise; + let resolveEnd; + let routeBackbeat; + + beforeEach(() => { + sandbox = sinon.createSandbox(); + + // create a Promise that resolves when response.end is called + endPromise = new Promise((resolve) => { resolveEnd = resolve; }); + + mockResponse = { + statusCode: null, + body: null, + setHeader: () => {}, + writeHead: sandbox.spy(statusCode => { + mockResponse.statusCode = statusCode; + }), + end: sandbox.spy((body, encoding, callback) => { + mockResponse.body = JSON.parse(body); + if (callback) callback(); + resolveEnd(); // Resolve the Promise when end is called + }), + }; + + mockRequest = prepareDummyRequest(); + + sandbox.stub(metadataUtils, 'standardMetadataValidateBucketAndObj'); + + // Clear require cache for routeBackbeat to make sure fresh module with stubbed dependencies + delete require.cache[require.resolve('../../../lib/routes/routeBackbeat')]; + routeBackbeat = require('../../../lib/routes/routeBackbeat'); + }); + + afterEach(() => { + sandbox.restore(); + }); + + it('should reject non-GET requests when versioning is disabled', async () => { + metadataUtils.standardMetadataValidateBucketAndObj.callsFake((params, denies, log, callback) => { + const bucketInfo = { + getVersioningConfiguration: () => ({ Status: 'Disabled' }), + }; + const objMd = {}; + callback(null, bucketInfo, objMd); + }); + + routeBackbeat('127.0.0.1', mockRequest, mockResponse, log); + + void await endPromise; + + assert.strictEqual(mockResponse.statusCode, 409); + assert.strictEqual(mockResponse.body.code, 'InvalidBucketState'); + }); + + it('should allow GET requests regardless of versioning', async () => { + mockRequest.method = 'GET'; + + metadataUtils.standardMetadataValidateBucketAndObj.callsFake((params, denies, log, callback) => { + const bucketInfo = { + getVersioningConfiguration: () => ({ Status: 'Disabled' }), + }; + const objMd = {}; + callback(null, bucketInfo, objMd); + }); + + routeBackbeat('127.0.0.1', mockRequest, mockResponse, log); + + void await endPromise; + + assert.strictEqual(mockResponse.statusCode, 200); + assert.deepStrictEqual(mockResponse.body, { Body: '{}' }); + }); + + it('should allow non-GET requests when versioning is enabled', async () => { + mockRequest.method = 'PUT'; + mockRequest.destroy = () => {}; + + sandbox.stub(metadata, 'putObjectMD').callsFake((bucketName, objectKey, omVal, options, logParam, cb) => { + cb(null, {}); + }); + + metadataUtils.standardMetadataValidateBucketAndObj.callsFake((params, denies, log, callback) => { + const bucketInfo = { + getVersioningConfiguration: () => ({ Status: 'Enabled' }), + isVersioningEnabled: () => true, + }; + const objMd = {}; + callback(null, bucketInfo, objMd); + }); + + routeBackbeat('127.0.0.1', mockRequest, mockResponse, log); + + void await endPromise; + + assert.strictEqual(mockResponse.statusCode, 200); + assert.deepStrictEqual(mockResponse.body, {}); + }); + + it('should reject non-GET requests when versioning is disabled', async () => { + metadataUtils.standardMetadataValidateBucketAndObj.callsFake((params, denies, log, callback) => { + const bucketInfo = { + getVersioningConfiguration: () => null, + }; + const objMd = {}; + callback(null, bucketInfo, objMd); + }); + + routeBackbeat('127.0.0.1', mockRequest, mockResponse, log); + + void await endPromise; + + assert.strictEqual(mockResponse.statusCode, 409); + assert.strictEqual(mockResponse.body.code, 'InvalidBucketState'); + }); +});