From decfc8ff230393121fadcdb96384e1266dc529fd Mon Sep 17 00:00:00 2001 From: williamlardier Date: Tue, 10 Dec 2024 16:50:43 +0100 Subject: [PATCH] Unify implicit deny handling in normal and backbeat routes - Also split backbeat routers - Better use the callback functions - Do not return twice to the client in case of error and quota evaluation (finalizer hooks) - Remove account quota from backbeat proxy route: as not used in this case. Issue: CLDSRV-591 --- lib/api/api.js | 46 +++-- lib/routes/routeBackbeat.js | 332 +++++++++++++++++------------------- tests/unit/routeBackbeat.js | 5 - 3 files changed, 184 insertions(+), 199 deletions(-) diff --git a/lib/api/api.js b/lib/api/api.js index 5a98eb0da6..bb9390a042 100644 --- a/lib/api/api.js +++ b/lib/api/api.js @@ -119,6 +119,26 @@ function checkAuthResults(authResults, apiMethod, log) { } /* eslint-disable no-param-reassign */ +function handleAuthorizationResults(request, authorizationResults, apiMethod, returnTagCount, log, callback) { + if (authorizationResults) { + const checkedResults = checkAuthResults(authorizationResults, apiMethod, log); + if (checkedResults instanceof Error) { + return callback(checkedResults); + } + returnTagCount = checkedResults.returnTagCount; + request.actionImplicitDenies = checkedResults.isImplicitDeny; + } else { + // create an object of keys apiMethods with all values to false: + // for backward compatibility, all apiMethods are allowed by default + // thus it is explicitly allowed, so implicit deny is false + request.actionImplicitDenies = request.apiMethods.reduce((acc, curr) => { + acc[curr] = false; + return acc; + }, {}); + } + return callback(); +} + const api = { callApiMethod(apiMethod, request, response, log, callback) { // Attach the apiMethod method to the request, so it can used by monitoring in the server @@ -148,7 +168,7 @@ const api = { objectKey: request.objectKey, }); } - let returnTagCount = true; + const returnTagCount = true; const validationRes = validateQueryAndHeaders(request, log); if (validationRes.error) { @@ -263,27 +283,18 @@ const api = { return next(null, userInfo, authResultsWithTags, streamingV4Params, infos); }, ), + (userInfo, authorizationResults, streamingV4Params, infos, next) => + handleAuthorizationResults(request, authorizationResults, apiMethod, returnTagCount, log, err => { + if (err) { + return next(err); + } + return next(null, userInfo, authorizationResults, streamingV4Params, infos); + }), ], (err, userInfo, authorizationResults, streamingV4Params, infos) => { if (err) { return callback(err); } request.accountQuotas = infos?.accountQuota; - if (authorizationResults) { - const checkedResults = checkAuthResults(authorizationResults, apiMethod, log); - if (checkedResults instanceof Error) { - return callback(checkedResults); - } - returnTagCount = checkedResults.returnTagCount; - request.actionImplicitDenies = checkedResults.isImplicitDeny; - } else { - // create an object of keys apiMethods with all values to false: - // for backward compatibility, all apiMethods are allowed by default - // thus it is explicitly allowed, so implicit deny is false - request.actionImplicitDenies = apiMethods.reduce((acc, curr) => { - acc[curr] = false; - return acc; - }, {}); - } const methodCallback = (err, ...results) => async.forEachLimit(request.finalizerHooks, 5, (hook, done) => hook(err, done), () => callback(err, ...results)); @@ -369,6 +380,7 @@ const api = { websiteGet: website, websiteHead: website, checkAuthResults, + handleAuthorizationResults, }; module.exports = api; diff --git a/lib/routes/routeBackbeat.js b/lib/routes/routeBackbeat.js index 7327a395df..462d75425a 100644 --- a/lib/routes/routeBackbeat.js +++ b/lib/routes/routeBackbeat.js @@ -23,10 +23,10 @@ const locationStorageCheck = require('../api/apiUtils/object/locationStorageCheck'); const { dataStore } = require('../api/apiUtils/object/storeObject'); const prepareRequestContexts = require( -'../api/apiUtils/authorization/prepareRequestContexts'); + '../api/apiUtils/authorization/prepareRequestContexts'); const { decodeVersionId } = require('../api/apiUtils/object/versioning'); const locationKeysHaveChanged - = require('../api/apiUtils/object/locationKeysHaveChanged'); + = require('../api/apiUtils/object/locationKeysHaveChanged'); const { standardMetadataValidateBucketAndObj, metadataGetObject } = require('../metadata/metadataUtils'); const { config } = require('../Config'); @@ -39,7 +39,7 @@ const { listLifecycleNonCurrents } = require('../api/backbeat/listLifecycleNonCu const { listLifecycleOrphanDeleteMarkers } = require('../api/backbeat/listLifecycleOrphanDeleteMarkers'); const { objectDeleteInternal } = require('../api/objectDelete'); const { validateQuotas } = require('../api/apiUtils/quotas/quotaUtils'); -const { checkAuthResults } = require('../api/api'); +const { handleAuthorizationResults } = require('../api/api'); const { CURRENT_TYPE, NON_CURRENT_TYPE, ORPHAN_DM_TYPE } = constants.lifecycleListing; @@ -128,7 +128,7 @@ function _getRequestPayload(req, cb) { payload.push(chunk); payloadLen += chunk.length; }).on('error', cb) - .on('end', () => cb(null, Buffer.concat(payload, payloadLen).toString())); + .on('end', () => cb(null, Buffer.concat(payload, payloadLen).toString())); } function _checkMultipleBackendRequest(request, log) { @@ -195,7 +195,7 @@ function _checkMultipleBackendRequest(request, log) { const location = locationConstraints[headers['x-scal-storage-class']]; const storageTypeList = storageType.split(','); const isValidLocation = location && - storageTypeList.includes(location.type); + storageTypeList.includes(location.type); if (!isValidLocation) { errMessage = 'invalid request: invalid location constraint in request'; log.debug(errMessage, { @@ -301,19 +301,19 @@ function handleTaggingOperation(request, response, type, dataStoreVersionId, } } return dataClient.objectTagging(type, request.objectKey, - request.bucketName, objectMD, log, err => { - if (err) { - log.error(`error during object tagging: ${type}`, { - error: err, - method: 'handleTaggingOperation', - }); - return callback(err); - } - const dataRetrievalInfo = { - versionId: dataStoreVersionId, - }; - return _respond(response, dataRetrievalInfo, log, callback); - }); + request.bucketName, objectMD, log, err => { + if (err) { + log.error(`error during object tagging: ${type}`, { + error: err, + method: 'handleTaggingOperation', + }); + return callback(err); + } + const dataRetrievalInfo = { + versionId: dataStoreVersionId, + }; + return _respond(response, dataRetrievalInfo, log, callback); + }); } function _getLastModified(locations, log, cb) { @@ -557,7 +557,8 @@ function putMetadata(request, response, bucketInfo, objMd, log, callback) { } log.trace('putting object version', { - objectKey: request.objectKey, omVal, options }); + objectKey: request.objectKey, omVal, options + }); return metadata.putObjectMD(bucketName, objectKey, omVal, options, log, (err, md) => { if (err) { @@ -582,26 +583,26 @@ function putMetadata(request, response, bucketInfo, objMd, log, callback) { objectKey, }); async.eachLimit(objMd.location, 5, - (loc, next) => dataWrapper.data.delete(loc, log, err => { - if (err) { - log.warn('error removing old data location key', { + (loc, next) => dataWrapper.data.delete(loc, log, err => { + if (err) { + log.warn('error removing old data location key', { + bucketName, + objectKey, + locationKey: loc, + error: err.message, + }); + } + // do not forward the error to let other + // locations be deleted + next(); + }), + () => { + log.debug('done removing old data locations', { + method: 'putMetadata', bucketName, objectKey, - locationKey: loc, - error: err.message, }); - } - // do not forward the error to let other - // locations be deleted - next(); - }), - () => { - log.debug('done removing old data locations', { - method: 'putMetadata', - bucketName, - objectKey, }); - }); } return _respond(response, md, log, callback); }); @@ -910,7 +911,7 @@ function completeMultipartUpload(request, response, log, callback) { // lib/api/completeMultipartUpload.js. const { key, dataStoreType, dataStoreVersionId } = - retrievalInfo; + retrievalInfo; let size; let dataStoreETag; if (skipMpuPartProcessing(retrievalInfo)) { @@ -918,7 +919,7 @@ function completeMultipartUpload(request, response, log, callback) { dataStoreETag = retrievalInfo.eTag; } else { const { aggregateSize, aggregateETag } = - generateMpuAggregateInfo(parts); + generateMpuAggregateInfo(parts); size = aggregateSize; dataStoreETag = aggregateETag; } @@ -1307,35 +1308,39 @@ const indexEntrySchema = joi.object({ const indexingSchema = joi.array().items(indexEntrySchema).min(1); +function respondToRequest(err, response, log, callback) { + responseJSONBody(err, null, response, log); + // The callback is optional, as it is only used for testing purposes + // but value may be set to non-undefined or null due to the arsenal + // routes implementation + if (callback && typeof callback === 'function') { + return callback(err); + } + return undefined; +} + function routeIndexingAPIs(request, response, userInfo, log, callback) { const route = backbeatRoutes[request.method][request.resourceType]; if (!['GET', 'POST'].includes(request.method)) { - responseJSONBody(errors.MethodNotAllowed, null, response, log); - return callback(errors.MethodNotAllowed); + return respondToRequest(errors.MethodNotAllowed, response, log, callback); } if (request.method === 'GET') { - return route(request, response, userInfo, log, err => { - if (err) { - responseJSONBody(err, null, response, log); - } - return callback(err); - }); + return route(request, response, userInfo, log, err => + respondToRequest(err, response, log, callback)); } const op = request.query.operation; if (!op || typeof route[op] !== 'function') { log.error('Invalid operataion parameter', { operation: op }); - responseJSONBody(errors.BadRequest, null, response, log); - return callback(errors.BadRequest); + return respondToRequest(errors.BadRequest, response, log, callback); } return _getRequestPayload(request, (err, payload) => { if (err) { - responseJSONBody(err, null, response, log); - return callback(err); + return respondToRequest(err, response, log, callback); } let parsedIndex; @@ -1344,15 +1349,53 @@ function routeIndexingAPIs(request, response, userInfo, log, callback) { parsedIndex = joi.attempt(JSON.parse(payload), indexingSchema, 'invalid payload'); } catch (err) { log.error('Unable to parse index request body', { error: err }); - responseJSONBody(errors.BadRequest, null, response, log); - return callback(errors.BadRequest); + return respondToRequest(errors.BadRequest, response, log, callback); } - return route[op](parsedIndex, request, response, userInfo, log, err => { + return route[op](parsedIndex, request, response, userInfo, log, err => + respondToRequest(err, response, log, callback)); + }); +} + +function routeBackbeatAPIProxy(request, response, requestContexts, apiMethods, log, callback) { + const path = request.url.replace('/_/backbeat/api', '/_/'); + const { host, port } = config.backbeat; + const target = `http://${host}:${port}${path}`; + + return async.waterfall([ + next => auth.server.doAuth(request, log, (err, userInfo, authorizationResults) => { if (err) { - responseJSONBody(err, null, response, log); + log.debug('authentication error', { + error: err, + method: request.method, + bucketName: request.bucketName, + objectKey: request.objectKey, + }); } - return callback(err); + return next(err, userInfo, authorizationResults); + }, 's3', requestContexts), + (userInfo, authorizationResults, next) => handleAuthorizationResults( + authorizationResults, apiMethods[0], log, err => next(err, userInfo)), + ], (err, userInfo) => { + if (err) { + return respondToRequest(err, response, log, callback); + } + // FIXME for now, any authenticated user can access API + // routes. We should introduce admin accounts or accounts + // with admin privileges, and restrict access to those + // only. + if (userInfo.getCanonicalID() === constants.publicId) { + log.debug('unauthenticated access to API routes', { + method: request.method, + bucketName: request.bucketName, + objectKey: request.objectKey, + }); + return respondToRequest(errors.AccessDenied, response, log, callback); + } + return backbeatProxy.web(request, response, { target }, err => { + log.error('error proxying request to api server', + { error: err.message }); + return respondToRequest(errors.ServiceUnavailable, response, log, callback); }); }); } @@ -1397,66 +1440,9 @@ function routeBackbeat(clientIP, request, response, log, callback) { log.debug('unable to proxy backbeat api request', { backbeatConfig: config.backbeat, }); - responseJSONBody(errors.MethodNotAllowed, null, response, log); - return callback(errors.MethodNotAllowed); + return respondToRequest(errors.MethodNotAllowed, response, log, callback); } - const path = request.url.replace('/_/backbeat/api', '/_/'); - const { host, port } = config.backbeat; - const target = `http://${host}:${port}${path}`; - - // TODO CLDSRV-591: shall we use the authorization results here? - return auth.server.doAuth(request, log, (err, userInfo, authorizationResults, streamingV4Params, infos) => { - if (err) { - log.debug('authentication error', { - error: err, - method: request.method, - bucketName: request.bucketName, - objectKey: request.objectKey, - }); - responseJSONBody(err, null, response, log); - return callback(err); - } - // eslint-disable-next-line no-param-reassign - request.accountQuotas = infos?.accountQuota; - if (authorizationResults) { - const checkedResults = checkAuthResults(authorizationResults, apiMethods[0], log); - if (checkedResults instanceof Error) { - responseJSONBody(errors.AccessDenied, null, response, log); - return callback(errors.AccessDenied); - } - // eslint-disable-next-line no-param-reassign - request.actionImplicitDenies = checkedResults.isImplicitDeny; - } else { - // create an object of keys apiMethods with all values to false: - // for backward compatibility, all apiMethods are allowed by default - // thus it is explicitly allowed, so implicit deny is false - // eslint-disable-next-line no-param-reassign - request.actionImplicitDenies = apiMethods.reduce((acc, curr) => { - // eslint-disable-next-line no-param-reassign - acc[curr] = false; - return acc; - }, {}); - } - // FIXME for now, any authenticated user can access API - // routes. We should introduce admin accounts or accounts - // with admin privileges, and restrict access to those - // only. - if (userInfo.getCanonicalID() === constants.publicId) { - log.debug('unauthenticated access to API routes', { - method: request.method, - bucketName: request.bucketName, - objectKey: request.objectKey, - }); - responseJSONBody(errors.AccessDenied, null, response, log); - return callback(errors.AccessDenied); - } - return backbeatProxy.web(request, response, { target }, err => { - log.error('error proxying request to api server', - { error: err.message }); - responseJSONBody(errors.ServiceUnavailable, null, response, log); - return callback(errors.ServiceUnavailable); - }); - }, 's3', requestContexts); + return routeBackbeatAPIProxy(request, response, requestContexts, apiMethods, log, callback); } const useMultipleBackend = @@ -1485,67 +1471,49 @@ function routeBackbeat(clientIP, request, response, log, callback) { resourceType: request.resourceType, query: request.query, }); - responseJSONBody(errors.MethodNotAllowed, null, response, log); - return callback(errors.MethodNotAllowed); + return respondToRequest(errors.MethodNotAllowed, response, log, callback); } - return async.waterfall([next => auth.server.doAuth( - // TODO CLDSRV-591: shall we use the authorization results here? - request, log, (err, userInfo, authorizationResults, streamingV4Params, infos) => { - if (err) { - log.debug('authentication error', { - error: err, - method: request.method, - bucketName: request.bucketName, - objectKey: request.objectKey, - }); - } - // eslint-disable-next-line no-param-reassign - request.accountQuotas = infos?.accountQuota; - if (authorizationResults) { - const checkedResults = checkAuthResults(authorizationResults, apiMethods[0], log); - if (checkedResults instanceof Error) { - return callback(errors.MethodNotAllowed); + const isObjectRequest = _isObjectRequest(request); + + return async.waterfall([ + next => auth.server.doAuth( + request, log, (err, userInfo, authorizationResults, streamingV4Params, infos) => { + if (err) { + log.debug('authentication error', { + error: err, + method: request.method, + bucketName: request.bucketName, + objectKey: request.objectKey, + }); } // eslint-disable-next-line no-param-reassign - request.actionImplicitDenies = checkedResults.isImplicitDeny; - } else { - // create an object of keys apiMethods with all values to false: - // for backward compatibility, all apiMethods are allowed by default - // thus it is explicitly allowed, so implicit deny is false - // eslint-disable-next-line no-param-reassign - request.actionImplicitDenies = apiMethods.reduce((acc, curr) => { - // eslint-disable-next-line no-param-reassign - acc[curr] = false; - return acc; - }, {}); - } - return next(err, userInfo); - }, 's3', requestContexts), + request.accountQuotas = infos?.accountQuota; + return next(err, userInfo, authorizationResults); + }, 's3', requestContexts), + (userInfo, authorizationResults, next) => + handleAuthorizationResults(request, authorizationResults, apiMethods[0], {}, log, err => + next(err, userInfo)), (userInfo, next) => { // TODO: understand why non-object requests (batchdelete) were not authenticated - if (!_isObjectRequest(request)) { + if (!isObjectRequest) { if (userInfo.getCanonicalID() === constants.publicId) { log.debug(`unauthenticated access to backbeat ${request.resourceType} routes`, { method: request.method, bucketName: request.bucketName, objectKey: request.objectKey, }); - return responseJSONBody( - errors.AccessDenied, null, response, log); + return next(errors.AccessDenied); } if (request.resourceType === 'index') { - return routeIndexingAPIs(request, response, userInfo, log, callback); + return routeIndexingAPIs(request, response, userInfo, log, + err => next(err, null, null)); } const route = backbeatRoutes[request.method][request.resourceType]; - return route(request, response, userInfo, log, err => { - if (err) { - responseJSONBody(err, null, response, log); - } - return callback(err); - }); + return route(request, response, userInfo, log, + err => next(err, null, null)); } const decodedVidResult = decodeVersionId(request.query); @@ -1554,7 +1522,7 @@ function routeBackbeat(clientIP, request, response, log, callback) { versionId: request.query.versionId, error: decodedVidResult, }); - return responseJSONBody(errors.InvalidArgument, null, response, log); + return next(errors.InvalidArgument); } const versionId = decodedVidResult; if (useMultipleBackend) { @@ -1569,9 +1537,14 @@ function routeBackbeat(clientIP, request, response, log, callback) { requestType: request.apiMethods || 'ReplicateObject', request, }; - return standardMetadataValidateBucketAndObj(mdValParams, request.actionImplicitDenies, log, next); + return standardMetadataValidateBucketAndObj(mdValParams, request.actionImplicitDenies, log, + (err, bucketMd, objMd) => next(err, bucketMd, objMd)); }, (bucketInfo, objMd, next) => { + // Function was already called + if (!isObjectRequest) { + return next(); + } if (!useMultipleBackend) { return backbeatRoutes[request.method][request.resourceType]( request, response, bucketInfo, objMd, log, next); @@ -1583,25 +1556,30 @@ function routeBackbeat(clientIP, request, response, log, callback) { return backbeatRoutes[request.method][request.resourceType][request.query.operation]( request, response, log, next); }], - err => async.forEachLimit( - // Finalizer hooks are used in a quota context and ensure consistent - // metrics in case of API errors. No operation required if the API - // completed successfully. - request.finalizerHooks, - 5, - (hook, done) => hook(err, done), - () => { - if (err) { - responseJSONBody(err, null, response, log); - return callback(err); - } - log.debug('backbeat route response sent successfully', - { method: request.method, - bucketName: request.bucketName, - objectKey: request.objectKey }); - return callback(); - }, - )); + err => { + if (err) { + return async.forEachLimit( + // Finalizer hooks are used in a quota context and ensure consistent + // metrics in case of API errors. No operation required if the API + // completed successfully. + request.finalizerHooks, + 5, + (hook, done) => hook(err, done), + () => { + if (err) { + return respondToRequest(err, response, log, callback); + } + log.debug('backbeat route response sent successfully', { + method: request.method, + bucketName: request.bucketName, + objectKey: request.objectKey + }); + return respondToRequest(null, response, log, callback); + }, + ); + } + return respondToRequest(null, response, log, callback); + }); } diff --git a/tests/unit/routeBackbeat.js b/tests/unit/routeBackbeat.js index 42e57c6909..8dc881f258 100644 --- a/tests/unit/routeBackbeat.js +++ b/tests/unit/routeBackbeat.js @@ -23,8 +23,6 @@ const testBucket = { namespace, headers: { 'host': `${bucketName}.s3.amazonaws.com`, - // set versioning - }, url: `/${bucketName}`, actionImplicitDenies: false, @@ -47,7 +45,6 @@ describe('routeBackbeat', () => { let response; beforeEach(() => { - // Mock backbeatRoutes sinon.stub(backbeatRoutes, 'PUT').returns({ data: sinon.stub(), metadata: sinon.stub(), @@ -86,7 +83,6 @@ describe('routeBackbeat', () => { index: sinon.stub(), }); - // Mock request and response request = new DummyRequest( { method: 'GET', @@ -131,7 +127,6 @@ describe('routeBackbeat', () => { target: `${bucketName}/${objectName}`, operation: null, versionId: false, - // not sending any body here, so expect error expect: errors.MalformedPOSTRequest, }, {