INTEGRATION [PR#6239 > development/9.4] Send signatureVersion and authType unswapped to Vault checkPolicies - #6246
Open
bert-e wants to merge 19 commits into
Conversation
multiObjectDelete assigned authType to signatureVersion and vice versa when building the checkPolicies request context, so Vault validated and evaluated each field against the wrong value. Issue: CLDSRV-963
…ed-auth-params-check-policies' into w/9.2/improvement/CLDSRV-963-fix-swapped-auth-params-check-policies
…-swapped-auth-params-check-policies' into w/9.3/improvement/CLDSRV-963-fix-swapped-auth-params-check-policies
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| callback => initializeMultiObjectDeleteWithBatchingSupport(bucketName, inPlay, log, callback), | ||
| (cache, callback) => | ||
| async.forEachLimit( | ||
| inPlay, |
Comment on lines
+268
to
+548
| config.multiObjectDeleteConcurrency, | ||
| (entry, moveOn) => { | ||
| async.waterfall( | ||
| [ | ||
| callback => callback(...decodeObjectVersion(entry, bucketName)), | ||
| // for obj deletes, no need to check acl's at object level | ||
| // (authority is at the bucket level for obj deletes) | ||
| (versionId, callback) => | ||
| metadataUtils.metadataGetObject( | ||
| bucketName, | ||
| entry.key, | ||
| versionId, | ||
| cache, | ||
| log, | ||
| (err, objMD) => callback(err, objMD, versionId), | ||
| ), | ||
| (objMD, versionId, callback) => { | ||
| if (!objMD) { | ||
| const verCfg = bucket.getVersioningConfiguration(); | ||
| // To adhere to AWS behavior, create a delete marker | ||
| // if trying to delete an object that does not exist | ||
| // when versioning has been configured | ||
| if (verCfg && !entry.versionId) { | ||
| log.debug('trying to delete specific version ' + 'that does not exist'); | ||
| return callback(null, objMD, versionId); | ||
| } | ||
| // otherwise if particular key does not exist, AWS | ||
| // returns success for key so add to successfullyDeleted | ||
| // list and move on | ||
| successfullyDeleted.push({ entry }); | ||
| return callback(skipError); | ||
| } | ||
| if ( | ||
| versionId && | ||
| objMD.location && | ||
| Array.isArray(objMD.location) && | ||
| objMD.location[0] | ||
| ) { | ||
| // we need this information for data deletes to AWS | ||
| // eslint-disable-next-line no-param-reassign | ||
| objMD.location[0].deleteVersion = true; | ||
| } | ||
| return callback(null, objMD, versionId); | ||
| }, | ||
| (objMD, versionId, callback) => { | ||
| // AWS only returns an object lock error if a version id | ||
| // is specified, else continue to create a delete marker | ||
| if (!versionId || !bucket.isObjectLockEnabled()) { | ||
| return callback(null, null, objMD, versionId); | ||
| } | ||
| const hasGovernanceBypass = hasGovernanceBypassHeader(request.headers); | ||
| if (hasGovernanceBypass && isRequesterNonAccountUser(authInfo)) { | ||
| return checkUserGovernanceBypass( | ||
| request, | ||
| authInfo, | ||
| bucket, | ||
| entry.key, | ||
| log, | ||
| error => { | ||
| if (error && error.is.AccessDenied) { | ||
| log.debug( | ||
| 'user does not have BypassGovernanceRetention and object is locked', | ||
| { error }, | ||
| ); | ||
| return callback(objectLockedError); | ||
| } | ||
| if (error) { | ||
| return callback(error); | ||
| } | ||
| return callback(null, hasGovernanceBypass, objMD, versionId); | ||
| }, | ||
| ); | ||
| } | ||
| return callback(null, hasGovernanceBypass, objMD, versionId); | ||
| }, | ||
| (hasGovernanceBypass, objMD, versionId, callback) => { | ||
| // AWS only returns an object lock error if a version id | ||
| // is specified, else continue to create a delete marker | ||
| if (!versionId || !bucket.isObjectLockEnabled()) { | ||
| return callback(null, objMD, versionId); | ||
| } | ||
| const objLockInfo = new ObjectLockInfo({ | ||
| mode: objMD.retentionMode, | ||
| date: objMD.retentionDate, | ||
| legalHold: objMD.legalHold || false, | ||
| }); | ||
|
|
||
| // If the object can not be deleted raise an error | ||
| if (!objLockInfo.canModifyObject(hasGovernanceBypass)) { | ||
| log.debug('trying to delete locked object'); | ||
| return callback(objectLockedError); | ||
| } | ||
| // If the object can not be deleted raise an error | ||
| if (!objLockInfo.canModifyObject(hasGovernanceBypass)) { | ||
| log.debug('trying to delete locked object'); | ||
| return callback(objectLockedError); | ||
| } | ||
|
|
||
| return callback(null, objMD, versionId); | ||
| }, | ||
| (objMD, versionId, callback) => { | ||
| const bytes = processBytesToWrite('objectDelete', bucket, versionId, 0, objMD); | ||
| return validateQuotas(request, bucket, request.accountQuotas, ['objectDelete'], | ||
| 'objectDelete', bytes, false, log, err => callback(err, objMD, versionId)); | ||
| }, | ||
| (objMD, versionId, callback) => { | ||
| const options = preprocessingVersioningDelete( | ||
| bucketName, bucket, objMD, versionId, config.nullVersionCompatMode); | ||
| const deleteInfo = {}; | ||
| if (options && options.deleteData) { | ||
| options.overheadField = overheadField; | ||
| deleteInfo.deleted = true; | ||
| if (!_deleteRequiresOplogUpdate(objMD, bucket)) { | ||
| options.doesNotNeedOpogUpdate = true; | ||
| } | ||
| if (objMD.uploadId) { | ||
| options.replayId = objMD.uploadId; | ||
| } | ||
| return services.deleteObject(bucketName, objMD, | ||
| entry.key, options, config.multiObjectDeleteEnableOptimizations, log, | ||
| 's3:ObjectRemoved:Delete', (err, toDelete) => { | ||
| if (err) { | ||
| return callback(err); | ||
| return callback(null, objMD, versionId); | ||
| }, | ||
| (objMD, versionId, callback) => { | ||
| const bytes = processBytesToWrite('objectDelete', bucket, versionId, 0, objMD); | ||
| return validateQuotas( | ||
| request, | ||
| bucket, | ||
| request.accountQuotas, | ||
| ['objectDelete'], | ||
| 'objectDelete', | ||
| bytes, | ||
| false, | ||
| log, | ||
| err => callback(err, objMD, versionId), | ||
| ); | ||
| }, | ||
| (objMD, versionId, callback) => { | ||
| const options = preprocessingVersioningDelete( | ||
| bucketName, | ||
| bucket, | ||
| objMD, | ||
| versionId, | ||
| config.nullVersionCompatMode, | ||
| ); | ||
| const deleteInfo = {}; | ||
| if (options && options.deleteData) { | ||
| options.overheadField = overheadField; | ||
| deleteInfo.deleted = true; | ||
| if (!_deleteRequiresOplogUpdate(objMD, bucket)) { | ||
| options.doesNotNeedOpogUpdate = true; | ||
| } | ||
| if (objMD.uploadId) { | ||
| options.replayId = objMD.uploadId; | ||
| } | ||
| return services.deleteObject( | ||
| bucketName, | ||
| objMD, | ||
| entry.key, | ||
| options, | ||
| config.multiObjectDeleteEnableOptimizations, | ||
| log, | ||
| 's3:ObjectRemoved:Delete', | ||
| (err, toDelete) => { | ||
| if (err) { | ||
| return callback(err); | ||
| } | ||
| if (toDelete) { | ||
| deleteFromStorage = deleteFromStorage.concat(toDelete); | ||
| } | ||
| return callback(null, objMD, deleteInfo); | ||
| }, | ||
| ); | ||
| } | ||
| deleteInfo.newDeleteMarker = true; | ||
| // This call will create a delete-marker | ||
| return createAndStoreObject( | ||
| bucketName, | ||
| bucket, | ||
| entry.key, | ||
| objMD, | ||
| authInfo, | ||
| canonicalID, | ||
| null, | ||
| request, | ||
| deleteInfo.newDeleteMarker, | ||
| null, | ||
| overheadField, | ||
| log, | ||
| 's3:ObjectRemoved:DeleteMarkerCreated', | ||
| (err, result) => callback(err, objMD, deleteInfo, result.versionId), | ||
| ); | ||
| }, | ||
| ], | ||
| (err, objMD, deleteInfo, versionId) => { | ||
| if (err === skipError) { | ||
| // Object doesn't exist - log without object size (AWS behavior) | ||
| queueInternalLogRequest(request, { | ||
| objectKey: entry.key, | ||
| objectSize: null, | ||
| error: null, | ||
| }); | ||
| return moveOn(); | ||
| } else if (err === objectLockedError) { | ||
| errorResults.push({ entry, error: errors.AccessDenied, objectLocked: true }); | ||
| // Log locked object with size if available | ||
| const objectSize = | ||
| objMD && objMD['content-length'] ? objMD['content-length'] : null; | ||
| queueInternalLogRequest(request, { | ||
| objectKey: entry.key, | ||
| objectSize, | ||
| error: errors.AccessDenied, | ||
| }); | ||
| return moveOn(); | ||
| } else if (err) { | ||
| log.error('error deleting object', { error: err, entry }); | ||
| errorResults.push({ entry, error: err }); | ||
| // Log error case with size if available | ||
| const objectSize = | ||
| objMD && objMD['content-length'] ? objMD['content-length'] : null; | ||
| queueInternalLogRequest(request, { objectKey: entry.key, objectSize, error: err }); | ||
| return moveOn(); | ||
| } | ||
| if (toDelete) { | ||
| deleteFromStorage = deleteFromStorage.concat(toDelete); | ||
| if (deleteInfo.deleted && objMD['content-length']) { | ||
| numOfObjectsRemoved++; | ||
| totalContentLengthDeleted += objMD['content-length']; | ||
| } | ||
| return callback(null, objMD, deleteInfo); | ||
| }); | ||
| } | ||
| deleteInfo.newDeleteMarker = true; | ||
| // This call will create a delete-marker | ||
| return createAndStoreObject(bucketName, bucket, entry.key, | ||
| objMD, authInfo, canonicalID, null, request, | ||
| deleteInfo.newDeleteMarker, null, overheadField, log, | ||
| 's3:ObjectRemoved:DeleteMarkerCreated', (err, result) => | ||
| callback(err, objMD, deleteInfo, result.versionId)); | ||
| }, | ||
| ], (err, objMD, deleteInfo, versionId) => { | ||
| if (err === skipError) { | ||
| // Object doesn't exist - log without object size (AWS behavior) | ||
| queueInternalLogRequest(request, { objectKey: entry.key, objectSize: null, error: null }); | ||
| return moveOn(); | ||
| } else if (err === objectLockedError) { | ||
| errorResults.push({ entry, error: errors.AccessDenied, objectLocked: true }); | ||
| // Log locked object with size if available | ||
| const objectSize = objMD && objMD['content-length'] ? objMD['content-length'] : null; | ||
| queueInternalLogRequest(request, { objectKey: entry.key, objectSize, error: errors.AccessDenied }); | ||
| return moveOn(); | ||
| } else if (err) { | ||
| log.error('error deleting object', { error: err, entry }); | ||
| errorResults.push({ entry, error: err }); | ||
| // Log error case with size if available | ||
| const objectSize = objMD && objMD['content-length'] ? objMD['content-length'] : null; | ||
| queueInternalLogRequest(request, { objectKey: entry.key, objectSize, error: err }); | ||
| return moveOn(); | ||
| } | ||
| if (deleteInfo.deleted && objMD['content-length']) { | ||
| numOfObjectsRemoved++; | ||
| totalContentLengthDeleted += objMD['content-length']; | ||
| } | ||
| let isDeleteMarker; | ||
| let deleteMarkerVersionId; | ||
| // - If trying to delete an object that does not exist (if a new | ||
| // delete marker was created) | ||
| // - Or if an object exists but no version was specified | ||
| // return DeleteMarkerVersionId equals the versionID of the marker | ||
| // you just generated and DeleteMarker tag equals true | ||
| if (deleteInfo.newDeleteMarker) { | ||
| isDeleteMarker = true; | ||
| deleteMarkerVersionId = versionIdUtils.encode(versionId); | ||
| // In this case we are putting a new object (i.e., the delete | ||
| // marker), so we decrement the numOfObjectsRemoved value. | ||
| numOfObjectsRemoved--; | ||
| // If trying to delete a delete marker, DeleteMarkerVersionId equals | ||
| // deleteMarker's versionID and DeleteMarker equals true | ||
| } else if (objMD && objMD.isDeleteMarker) { | ||
| isDeleteMarker = true; | ||
| deleteMarkerVersionId = entry.versionId; | ||
| } | ||
| successfullyDeleted.push({ | ||
| entry, isDeleteMarker, | ||
| deleteMarkerVersionId, | ||
| }); | ||
| // Queue successful deletion with object size | ||
| const objectSize = objMD && objMD['content-length'] ? objMD['content-length'] : null; | ||
| queueInternalLogRequest(request, { objectKey: entry.key, objectSize, error: null }); | ||
| return moveOn(); | ||
| }); | ||
| }, | ||
| // end of forEach func | ||
| err => { | ||
| // Batch delete all objects | ||
| const onDone = () => callback(err, quietSetting, errorResults, numOfObjectsRemoved, | ||
| successfullyDeleted, totalContentLengthDeleted, bucket); | ||
|
|
||
| if (err && deleteFromStorage.length === 0) { | ||
| log.trace('no objects to delete from data backend'); | ||
| return onDone(); | ||
| } | ||
| // If error but we have objects in the list, delete them to ensure | ||
| // consistent state. | ||
| log.trace('deleting objects from data backend'); | ||
| let isDeleteMarker; | ||
| let deleteMarkerVersionId; | ||
| // - If trying to delete an object that does not exist (if a new | ||
| // delete marker was created) | ||
| // - Or if an object exists but no version was specified | ||
| // return DeleteMarkerVersionId equals the versionID of the marker | ||
| // you just generated and DeleteMarker tag equals true | ||
| if (deleteInfo.newDeleteMarker) { | ||
| isDeleteMarker = true; | ||
| deleteMarkerVersionId = versionIdUtils.encode(versionId); | ||
| // In this case we are putting a new object (i.e., the delete | ||
| // marker), so we decrement the numOfObjectsRemoved value. | ||
| numOfObjectsRemoved--; | ||
| // If trying to delete a delete marker, DeleteMarkerVersionId equals | ||
| // deleteMarker's versionID and DeleteMarker equals true | ||
| } else if (objMD && objMD.isDeleteMarker) { | ||
| isDeleteMarker = true; | ||
| deleteMarkerVersionId = entry.versionId; | ||
| } | ||
| successfullyDeleted.push({ | ||
| entry, | ||
| isDeleteMarker, | ||
| deleteMarkerVersionId, | ||
| }); | ||
| // Queue successful deletion with object size | ||
| const objectSize = objMD && objMD['content-length'] ? objMD['content-length'] : null; | ||
| queueInternalLogRequest(request, { objectKey: entry.key, objectSize, error: null }); | ||
| return moveOn(); | ||
| }, | ||
| ); | ||
| }, | ||
| // end of forEach func | ||
| err => { | ||
| // Batch delete all objects | ||
| const onDone = () => | ||
| callback( | ||
| err, | ||
| quietSetting, | ||
| errorResults, | ||
| numOfObjectsRemoved, | ||
| successfullyDeleted, | ||
| totalContentLengthDeleted, | ||
| bucket, | ||
| ); | ||
|
|
||
| // Split the array into chunks | ||
| const chunks = []; | ||
| while (deleteFromStorage.length > 0) { | ||
| chunks.push(deleteFromStorage.splice(0, config.multiObjectDeleteConcurrency)); | ||
| } | ||
| if (err && deleteFromStorage.length === 0) { | ||
| log.trace('no objects to delete from data backend'); | ||
| return onDone(); | ||
| } | ||
| // If error but we have objects in the list, delete them to ensure | ||
| // consistent state. | ||
| log.trace('deleting objects from data backend'); | ||
|
|
||
| return async.each(chunks, (chunk, done) => data.batchDelete(chunk, null, null, | ||
| logger.newRequestLoggerFromSerializedUids(log.getSerializedUids()), done), | ||
| err => { | ||
| if (err) { | ||
| log.error('error deleting objects from data backend', { error: err }); | ||
| return onDone(err); | ||
| // Split the array into chunks | ||
| const chunks = []; | ||
| while (deleteFromStorage.length > 0) { | ||
| chunks.push(deleteFromStorage.splice(0, config.multiObjectDeleteConcurrency)); | ||
| } | ||
| return onDone(); | ||
| }); | ||
| }), | ||
| ], (err, ...results) => { | ||
| // if general error from metadata return error | ||
| if (err) { | ||
| monitoring.promMetrics('DELETE', bucketName, err.code, | ||
| 'multiObjectDelete'); | ||
| return next(err); | ||
| } | ||
| return next(null, ...results); | ||
| }); | ||
|
|
||
| return async.each( | ||
| chunks, | ||
| (chunk, done) => | ||
| data.batchDelete( | ||
| chunk, | ||
| null, | ||
| null, | ||
| logger.newRequestLoggerFromSerializedUids(log.getSerializedUids()), | ||
| done, | ||
| ), | ||
| err => { | ||
| if (err) { | ||
| log.error('error deleting objects from data backend', { error: err }); | ||
| return onDone(err); | ||
| } | ||
| return onDone(); | ||
| }, | ||
| ); | ||
| }, | ||
| ), | ||
| ], | ||
| (err, ...results) => { | ||
| // if general error from metadata return error |
| callback => callback(...decodeObjectVersion(entry, bucketName)), | ||
| // for obj deletes, no need to check acl's at object level | ||
| // (authority is at the bucket level for obj deletes) | ||
| (versionId, callback) => |
Comment on lines
+278
to
+286
| entry.key, | ||
| versionId, | ||
| cache, | ||
| log, | ||
| (err, objMD) => callback(err, objMD, versionId), | ||
| ), | ||
| (objMD, versionId, callback) => { | ||
| if (!objMD) { | ||
| const verCfg = bucket.getVersioningConfiguration(); |
Comment on lines
+287
to
+314
| // To adhere to AWS behavior, create a delete marker | ||
| // if trying to delete an object that does not exist | ||
| // when versioning has been configured | ||
| if (verCfg && !entry.versionId) { | ||
| log.debug('trying to delete specific version ' + 'that does not exist'); | ||
| return callback(null, objMD, versionId); | ||
| } | ||
| // otherwise if particular key does not exist, AWS | ||
| // returns success for key so add to successfullyDeleted | ||
| // list and move on | ||
| successfullyDeleted.push({ entry }); | ||
| return callback(skipError); | ||
| } | ||
| if ( | ||
| versionId && | ||
| objMD.location && | ||
| Array.isArray(objMD.location) && | ||
| objMD.location[0] | ||
| ) { | ||
| // we need this information for data deletes to AWS | ||
| // eslint-disable-next-line no-param-reassign | ||
| objMD.location[0].deleteVersion = true; | ||
| } | ||
| return callback(null, objMD, versionId); | ||
| }, | ||
| (objMD, versionId, callback) => { | ||
| // AWS only returns an object lock error if a version id | ||
| // is specified, else continue to create a delete marker |
Comment on lines
594
to
+630
| if (err) { | ||
| log.trace('error checking policies', { | ||
| error: err, | ||
| method: 'multiObjectDelete.checkPolicies', | ||
| }); | ||
| log.trace('error retrieving bucket metadata', { error: err }); | ||
| return next(err); | ||
| } | ||
| if (objects.length !== authorizationResults.length) { | ||
| log.error('vault did not return correct number of ' + | ||
| 'authorization results', { | ||
| authorizationResultsLength: | ||
| authorizationResults.length, | ||
| objectsLength: objects.length, | ||
| }); | ||
| return next(errors.InternalError); | ||
| // check whether bucket has transient or deleted flag | ||
| if (bucketShield(bucketMD, 'objectDelete')) { | ||
| return next(errors.NoSuchBucket); | ||
| } | ||
| // Convert authorization results into an easier to handle format | ||
| const actionImplicitDenies = authorizationResults.reduce((acc, curr, idx) => { | ||
| const apiMethod = authorizationResults[idx].action; | ||
| // eslint-disable-next-line no-param-reassign | ||
| acc[apiMethod] = curr.isImplicit; | ||
| return acc; | ||
| }, {}); | ||
| for (let i = 0; i < authorizationResults.length; i++) { | ||
| const result = authorizationResults[i]; | ||
| // result is { isAllowed: true, | ||
| // arn: arn:aws:s3:::bucket/object, | ||
| // versionId: sampleversionId } unless not allowed | ||
| // in which case no isAllowed key will be present | ||
| const slashIndex = result.arn.indexOf('/'); | ||
| if (slashIndex === -1) { | ||
| log.error('wrong arn format from vault'); | ||
| return next(errors.InternalError); | ||
| } | ||
| const entry = { | ||
| key: result.arn.slice(slashIndex + 1), | ||
| versionId: result.versionId, | ||
| }; | ||
| // Deny immediately if there is an explicit deny | ||
| if (!result.isImplicit && !result.isAllowed) { | ||
| metadataUtils.storeServerAccessLogInfo(request, bucketMD, raftSessionId); | ||
| // The implicit deny flag is ignored in the DeleteObjects API, as authorization only | ||
| // affects the objects. | ||
| if (!isBucketAuthorized(bucketMD, 'objectDelete', canonicalID, authInfo, log, request)) { | ||
| log.trace("access denied due to bucket acl's"); | ||
| // if access denied at the bucket level, no access for | ||
| // any of the objects so all results will be error results | ||
| objects.forEach(entry => { | ||
| errorResults.push({ | ||
| entry, | ||
| error: errors.AccessDenied, | ||
| }); | ||
| continue; | ||
| }); | ||
| // by sending an empty array as the objects array | ||
| // async.forEachLimit below will not actually | ||
| // make any calls to metadata or data but will continue on | ||
| // to the next step to build xml | ||
| return next(null, quietSetting, errorResults, [], bucketMD); | ||
| } | ||
| return next(null, quietSetting, errorResults, objects, bucketMD); | ||
| }); | ||
| }, | ||
| function checkPolicies(quietSetting, errorResults, objects, bucketMD, next) { | ||
| // track keys that are still on track to be deleted | ||
| const inPlay = []; |
Comment on lines
+631
to
+758
| // if request from account, no need to check policies | ||
| // all objects are inPlay so send array of object keys | ||
| // as inPlay argument | ||
| if (!isRequesterNonAccountUser(authInfo)) { | ||
| return next(null, quietSetting, errorResults, objects, bucketMD); | ||
| } | ||
|
|
||
| // TODO: once arsenal's extractParams is separated from doAuth | ||
| // function, refactor so only extract once and send | ||
| // params on to this api | ||
| const authParams = auth.server.extractParams(request, log, 's3', request.query); | ||
| const requestContextParams = { | ||
| constantParams: { | ||
| headers: request.headers, | ||
| query: request.query, | ||
| generalResource: request.bucketName, | ||
| requesterIp: ip, | ||
| sslEnabled: isSecure, | ||
| apiMethod: 'objectDelete', | ||
| awsService: 's3', | ||
| locationConstraint: null, | ||
| requesterInfo: authInfo, | ||
| signatureVersion: authParams.params.data.signatureVersion, | ||
| authType: authParams.params.data.authType, | ||
| signatureAge: authParams.params.data.signatureAge, | ||
| }, | ||
| parameterize: { | ||
| // eslint-disable-next-line | ||
| specificResource: objects.map(entry => { | ||
| return { | ||
| key: entry.key, | ||
| versionId: entry.versionId, | ||
| }; | ||
| }), | ||
| }, | ||
| }; | ||
| return vault.checkPolicies( | ||
| requestContextParams, | ||
| authInfo.getArn(), | ||
| log, | ||
| (err, authorizationResults) => { | ||
| // there were no policies so received a blanket AccessDenied | ||
| if (err?.is?.AccessDenied) { | ||
| objects.forEach(entry => { | ||
| errorResults.push({ | ||
| entry, | ||
| error: errors.AccessDenied, | ||
| }); | ||
| }); | ||
| // send empty array for inPlay | ||
| return next(null, quietSetting, errorResults, [], bucketMD); | ||
| } | ||
| if (err) { | ||
| log.trace('error checking policies', { | ||
| error: err, | ||
| method: 'multiObjectDelete.checkPolicies', | ||
| }); | ||
| return next(err); | ||
| } | ||
| if (objects.length !== authorizationResults.length) { | ||
| log.error('vault did not return correct number of ' + 'authorization results', { | ||
| authorizationResultsLength: authorizationResults.length, | ||
| objectsLength: objects.length, | ||
| }); | ||
| return next(errors.InternalError); | ||
| } | ||
| // Convert authorization results into an easier to handle format | ||
| const actionImplicitDenies = authorizationResults.reduce((acc, curr, idx) => { | ||
| const apiMethod = authorizationResults[idx].action; | ||
| // eslint-disable-next-line no-param-reassign | ||
| acc[apiMethod] = curr.isImplicit; | ||
| return acc; | ||
| }, {}); | ||
| for (let i = 0; i < authorizationResults.length; i++) { | ||
| const result = authorizationResults[i]; | ||
| // result is { isAllowed: true, | ||
| // arn: arn:aws:s3:::bucket/object, | ||
| // versionId: sampleversionId } unless not allowed | ||
| // in which case no isAllowed key will be present | ||
| const slashIndex = result.arn.indexOf('/'); | ||
| if (slashIndex === -1) { | ||
| log.error('wrong arn format from vault'); | ||
| return next(errors.InternalError); | ||
| } | ||
| const entry = { | ||
| key: result.arn.slice(slashIndex + 1), | ||
| versionId: result.versionId, | ||
| }; | ||
| // Deny immediately if there is an explicit deny | ||
| if (!result.isImplicit && !result.isAllowed) { | ||
| errorResults.push({ | ||
| entry, | ||
| error: errors.AccessDenied, | ||
| }); | ||
| continue; | ||
| } | ||
|
|
||
| // Evaluate against the bucket policies | ||
| const areAllActionsAllowed = evaluateBucketPolicyWithIAM( | ||
| bucketMD, | ||
| Object.keys(actionImplicitDenies), | ||
| canonicalID, | ||
| authInfo, | ||
| actionImplicitDenies, | ||
| log, | ||
| request); | ||
| // Evaluate against the bucket policies | ||
| const areAllActionsAllowed = evaluateBucketPolicyWithIAM( | ||
| bucketMD, | ||
| Object.keys(actionImplicitDenies), | ||
| canonicalID, | ||
| authInfo, | ||
| actionImplicitDenies, | ||
| log, | ||
| request, | ||
| ); | ||
|
|
||
| if (areAllActionsAllowed) { | ||
| if (validObjectKeys.includes(entry.key)) { | ||
| inPlayInternal.push(entry.key); | ||
| if (areAllActionsAllowed) { | ||
| if (validObjectKeys.includes(entry.key)) { | ||
| inPlayInternal.push(entry.key); | ||
| } else { | ||
| inPlay.push(entry); | ||
| } | ||
| } else { | ||
| inPlay.push(entry); | ||
| errorResults.push({ | ||
| entry, | ||
| error: errors.AccessDenied, | ||
| }); | ||
| } | ||
| } else { | ||
| errorResults.push({ | ||
| entry, | ||
| error: errors.AccessDenied, | ||
| }); | ||
| } | ||
| } | ||
| return next(null, quietSetting, errorResults, inPlay, bucketMD); | ||
| }); | ||
| }, | ||
| function handleInternalFiles(quietSetting, errorResults, inPlay, bucketMD, next) { | ||
| return async.each(inPlayInternal, | ||
| (localInPlay, next) => deleteVeeamCapabilities(bucketName, localInPlay, bucketMD, log, next), | ||
| err => next(err, quietSetting, errorResults, inPlay, bucketMD)); | ||
| }, | ||
| function getObjMetadataAndDeleteStep(quietSetting, errorResults, inPlay, | ||
| bucket, next) { | ||
| return getObjMetadataAndDelete(authInfo, canonicalID, request, | ||
| bucketName, bucket, quietSetting, errorResults, inPlay, | ||
| log, next); | ||
| return next(null, quietSetting, errorResults, inPlay, bucketMD); | ||
| }, | ||
| ); | ||
| }, | ||
| function handleInternalFiles(quietSetting, errorResults, inPlay, bucketMD, next) { | ||
| return async.each( | ||
| inPlayInternal, |
Comment on lines
+759
to
+765
| (localInPlay, next) => deleteVeeamCapabilities(bucketName, localInPlay, bucketMD, log, next), | ||
| err => next(err, quietSetting, errorResults, inPlay, bucketMD), | ||
| ); | ||
| }, | ||
| function getObjMetadataAndDeleteStep(quietSetting, errorResults, inPlay, bucket, next) { | ||
| return getObjMetadataAndDelete( | ||
| authInfo, |
| (localInPlay, next) => deleteVeeamCapabilities(bucketName, localInPlay, bucketMD, log, next), | ||
| err => next(err, quietSetting, errorResults, inPlay, bucketMD), | ||
| ); | ||
| }, |
Comment on lines
+766
to
+779
| canonicalID, | ||
| request, | ||
| bucketName, | ||
| bucket, | ||
| quietSetting, | ||
| errorResults, | ||
| inPlay, | ||
| log, | ||
| next, | ||
| ); | ||
| }, | ||
| ], | ||
| ( | ||
| err, |
benzekrimaha
force-pushed
the
w/9.4/improvement/CLDSRV-963-fix-swapped-auth-params-check-policies
branch
2 times, most recently
from
August 4, 2026 18:50
ef58fa9 to
ee9b155
Compare
…eck-policies' into w/9.4/improvement/CLDSRV-963-fix-swapped-auth-params-check-policies
benzekrimaha
force-pushed
the
w/9.4/improvement/CLDSRV-963-fix-swapped-auth-params-check-policies
branch
from
August 4, 2026 18:53
ee9b155 to
f348fc5
Compare
…licies' into tmp/octopus/w/9.2/improvement/CLDSRV-963-fix-swapped-auth-params-check-policies
…eck-policies' into w/9.3/improvement/CLDSRV-963-fix-swapped-auth-params-check-policies
…x-swapped-auth-params-check-policies' into tmp/octopus/w/9.4/improvement/CLDSRV-963-fix-swapped-auth-params-check-policies
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| }; | ||
|
|
||
| bucketPut(userAuthInfo, request, log, err => { | ||
| assert(err && err.AccessDenied); |
There was a problem hiding this comment.
Weak assertion — if the error is something other than AccessDenied, this fails with an unhelpful AssertionError: false == true. Per review criteria, prefer asserting the error message directly.
Suggested change
| assert(err && err.AccessDenied); | |
| assert.strictEqual(err.message, 'AccessDenied'); |
Contributor
There was a problem hiding this comment.
Strengthened on the source PR: now assert.strictEqual(err.is.AccessDenied, true), matching the existing idiom in this file (err.message on ArsenalError is the description, not the code, so the literal suggestion would not match).
| } | ||
| return waterfall( | ||
| [ | ||
| next => _parseXML(request, log, next), |
Comment on lines
+235
to
+255
| (locationConstraint, next) => { | ||
| if (!isRequesterNonAccountUser(authInfo)) { | ||
| return next(null, locationConstraint); | ||
| } | ||
|
|
||
| const authParams = auth.server.extractParams(request, log, 's3', request.query); | ||
| const requestConstantParams = authBucketPut( | ||
| authParams, bucketName, locationConstraint, request, authInfo | ||
| ); | ||
| const authParams = auth.server.extractParams(request, log, 's3', request.query); | ||
| const requestConstantParams = authBucketPut( | ||
| authParams, | ||
| bucketName, | ||
| locationConstraint, | ||
| request, | ||
| authInfo, | ||
| ); | ||
|
|
||
| return vault.checkPolicies( | ||
| requestConstantParams.map(_buildConstantParams), | ||
| authInfo.getArn(), | ||
| log, | ||
| _handleAuthResults(locationConstraint, log, next), | ||
| ); | ||
| }, | ||
| (locationConstraint, next) => createBucket(authInfo, bucketName, | ||
| request.headers, locationConstraint, log, (err, previousBucket) => { | ||
| // if bucket already existed, gather any relevant cors | ||
| // headers | ||
| const corsHeaders = collectCorsHeaders( | ||
| request.headers.origin, request.method, previousBucket); | ||
| if (err) { | ||
| return next(err, corsHeaders); | ||
| } | ||
| pushMetric('createBucket', log, { | ||
| authInfo, | ||
| bucket: bucketName, | ||
| }); | ||
| monitoring.promMetrics('PUT', bucketName, '200', 'createBucket'); | ||
| return next(null, corsHeaders); | ||
| }), | ||
| ], callback); | ||
| return vault.checkPolicies( | ||
| requestConstantParams.map(_buildConstantParams), | ||
| authInfo.getArn(), | ||
| log, | ||
| _handleAuthResults(locationConstraint, log, next), | ||
| ); | ||
| }, |
Comment on lines
+256
to
+270
| (locationConstraint, next) => | ||
| createBucket(authInfo, bucketName, request.headers, locationConstraint, log, (err, previousBucket) => { | ||
| // if bucket already existed, gather any relevant cors | ||
| // headers | ||
| const corsHeaders = collectCorsHeaders(request.headers.origin, request.method, previousBucket); | ||
| if (err) { | ||
| return next(err, corsHeaders); | ||
| } | ||
| pushMetric('createBucket', log, { | ||
| authInfo, | ||
| bucket: bucketName, | ||
| }); | ||
| monitoring.promMetrics('PUT', bucketName, '200', 'createBucket'); | ||
| return next(null, corsHeaders); | ||
| }), |
…licies' into tmp/octopus/w/9.2/improvement/CLDSRV-963-fix-swapped-auth-params-check-policies
…eck-policies' into tmp/octopus/w/9.3/improvement/CLDSRV-963-fix-swapped-auth-params-check-policies
…eck-policies' into tmp/octopus/w/9.4/improvement/CLDSRV-963-fix-swapped-auth-params-check-policies
| }; | ||
|
|
||
| bucketPut(userAuthInfo, request, log, err => { | ||
| assert.strictEqual(err.is.AccessDenied, true); |
There was a problem hiding this comment.
Weak assertion — if err is a different error, this fails with an unhelpful AssertionError: false == true. Per review criteria, prefer asserting the error message directly so failures reveal the actual error name.
Suggested change
| assert.strictEqual(err.is.AccessDenied, true); | |
| assert.strictEqual(err.message, 'AccessDenied'); |
Contributor
There was a problem hiding this comment.
Addressed in 0f2f742: the assertion now reports the received error on failure (assert.strictEqual(err.is.AccessDenied, true, expected AccessDenied, got ${err && err.message})). Kept the err.is.X form since it is the established idiom in this test suite (16 existing usages in tests/unit, none using err.message equality).
…licies' into tmp/octopus/w/9.2/improvement/CLDSRV-963-fix-swapped-auth-params-check-policies
…eck-policies' into tmp/octopus/w/9.3/improvement/CLDSRV-963-fix-swapped-auth-params-check-policies
…eck-policies' into tmp/octopus/w/9.4/improvement/CLDSRV-963-fix-swapped-auth-params-check-policies
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request has been created automatically.
It is linked to its parent pull request #6239.
Do not edit this pull request directly.
If you need to amend/cancel the changeset on branch
w/9.4/improvement/CLDSRV-963-fix-swapped-auth-params-check-policies, please follow thisprocedure:
git fetch git checkout w/9.4/improvement/CLDSRV-963-fix-swapped-auth-params-check-policies # <amend or cancel the changeset by _adding_ new commits> git push origin w/9.4/improvement/CLDSRV-963-fix-swapped-auth-params-check-policiesPlease always comment pull request #6239 instead of this one.