diff --git a/lib/api/apiUtils/authorization/permissionChecks.js b/lib/api/apiUtils/authorization/permissionChecks.js index 66ab3d1e20..d82a1bc392 100644 --- a/lib/api/apiUtils/authorization/permissionChecks.js +++ b/lib/api/apiUtils/authorization/permissionChecks.js @@ -509,8 +509,10 @@ function processBucketPolicy(requestType, bucket, canonicalID, arn, bucketOwner, request, aclPermission, results, actionImplicitDenies) { const bucketPolicy = bucket.getBucketPolicy(); let processedResult = results[requestType]; + let aclRequired = false; if (!bucketPolicy || request?.bypassUserBucketPolicies) { processedResult = actionImplicitDenies[requestType] === false && aclPermission; + aclRequired = true; } else { const bucketPolicyPermission = checkBucketPolicy(bucketPolicy, requestType, canonicalID, arn, bucketOwner, log, request, actionImplicitDenies); @@ -525,9 +527,10 @@ function processBucketPolicy(requestType, bucket, canonicalID, arn, bucketOwner, processedResult = true; } else { processedResult = actionImplicitDenies[requestType] === false && aclPermission; + aclRequired = true; } } - return processedResult; + return { allowed: processedResult, aclRequired }; } function isBucketAuthorized(bucket, requestTypesInput, canonicalID, authInfo, log, request, @@ -564,8 +567,13 @@ function isBucketAuthorized(bucket, requestTypesInput, canonicalID, authInfo, lo _requestType = 'objectGet'; actionImplicitDenies.objectGet = actionImplicitDenies.objectGet || false; } - return processBucketPolicy(_requestType, bucket, canonicalID, arn, bucket.getOwner(), log, - request, aclPermission, results, actionImplicitDenies); + const { allowed, aclRequired } = processBucketPolicy(_requestType, bucket, canonicalID, arn, + bucket.getOwner(), log, request, aclPermission, results, actionImplicitDenies); + if (aclRequired && request?.serverAccessLog) { + // eslint-disable-next-line no-param-reassign + request.serverAccessLog.aclRequired = 'Yes'; + } + return allowed; }); } @@ -582,8 +590,9 @@ function evaluateBucketPolicyWithIAM(bucket, requestTypesInput, canonicalID, aut if (authInfo) { arn = authInfo.getArn(); } - return processBucketPolicy(_requestType, bucket, canonicalID, arn, bucket.getOwner(), log, + const { allowed } = processBucketPolicy(_requestType, bucket, canonicalID, arn, bucket.getOwner(), log, request, true, results, actionImplicitDenies); + return allowed; }); } @@ -641,8 +650,13 @@ function isObjAuthorized(bucket, objectMD, requestTypesInput, canonicalID, authI } const aclPermission = checkObjectAcls(bucket, objectMD, parsedMethodName, canonicalID, requesterIsNotUser, isUserUnauthenticated, mainApiCall); - return processBucketPolicy(_requestType, bucket, canonicalID, arn, bucketOwner, + const { allowed, aclRequired } = processBucketPolicy(_requestType, bucket, canonicalID, arn, bucketOwner, log, request, aclPermission, results, actionImplicitDenies); + if (aclRequired && request?.serverAccessLog) { + // eslint-disable-next-line no-param-reassign + request.serverAccessLog.aclRequired = 'Yes'; + } + return allowed; }); } diff --git a/lib/metadata/metadataUtils.js b/lib/metadata/metadataUtils.js index cf0ef7ce36..c8588f9859 100644 --- a/lib/metadata/metadataUtils.js +++ b/lib/metadata/metadataUtils.js @@ -50,6 +50,11 @@ function storeServerAccessLogInfo(request, bucket, raftSessionId, options = {}) target.enabled = true; target.loggingEnabled = bucket.getBucketLoggingStatus().getLoggingEnabled(); } + // Source auth runs on the same request object and overwrites the + // destination's aclRequired. Move the source value to its log entry + // and restore the destination value that was saved before source auth. + target.aclRequired = request.serverAccessLog.aclRequired; + request.serverAccessLog.aclRequired = options.savedAclRequired; } else { // Default behavior: store in main serverAccessLog request.serverAccessLog.raftSessionID = raftSessionId; @@ -326,6 +331,10 @@ function standardMetadataValidateBucketAndObj(params, actionImplicitDenies, log, if (!Array.isArray(requestType)) { requestType = [requestType]; } + if (params.serverAccessLogOptions?.copySource) { + // eslint-disable-next-line no-param-reassign + params.serverAccessLogOptions.savedAclRequired = request?.serverAccessLog?.aclRequired; + } async.waterfall([ next => { // versionId may be 'null', which asks metadata to fetch the null key specifically diff --git a/lib/utilities/serverAccessLogger.js b/lib/utilities/serverAccessLogger.js index 4bd5fbd6d1..69bfb00175 100644 --- a/lib/utilities/serverAccessLogger.js +++ b/lib/utilities/serverAccessLogger.js @@ -495,7 +495,7 @@ function buildLogEntry(req, params, options) { tlsVersion: req.socket?.encrypted ? req.socket.getCipher()['version'] : req.headers?.['x-ssl-protocol'] ?? undefined, - aclRequired: options.aclRequired ?? undefined, // TODO: CLDSRV-774 + aclRequired: options.aclRequired ?? undefined, // hostID: undefined, // NOT IMPLEMENTED // accessPointARN: undefined, // NOT IMPLEMENTED @@ -557,6 +557,7 @@ function logBatchDeleteObject(req, requestID, objectKey, objectSize, error) { errorCode, objectKey, requestID, + aclRequired: req.serverAccessLog?.aclRequired, }); serverAccessLogger.write(`${JSON.stringify(logEntry)}\n`); @@ -589,7 +590,7 @@ function logServerAccess(req, res) { totalTime: calculateTotalTime(params.startTime, params.onFinishEndTime), turnAroundTime: calculateTurnAroundTime(params.startTurnAroundTime, endTurnAroundTime), versionId: req.query?.versionId, - aclRequired: undefined, // TODO: CLDSRV-774 + aclRequired: req.serverAccessLog.aclRequired, referer: req.headers?.referer, userAgent: req.headers?.['user-agent'], requestID, @@ -659,6 +660,7 @@ function logCopySourceAccess(req, requestID, operation, sourceBucket, sourceObje httpCode, errorCode, requestID, + aclRequired: params.aclRequired, }); serverAccessLogger.write(`${JSON.stringify(logEntry)}\n`); diff --git a/package.json b/package.json index 106be62fc4..df705b3cc9 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@zenko/cloudserver", - "version": "9.2.33", + "version": "9.2.34", "description": "Zenko CloudServer, an open-source Node.js implementation of a server handling the Amazon S3 protocol", "main": "index.js", "engines": { diff --git a/tests/functional/aws-node-sdk/test/serverAccessLogs/testServerAccessLogFile.js b/tests/functional/aws-node-sdk/test/serverAccessLogs/testServerAccessLogFile.js index 6b25cd29f6..654cb313fb 100644 --- a/tests/functional/aws-node-sdk/test/serverAccessLogs/testServerAccessLogFile.js +++ b/tests/functional/aws-node-sdk/test/serverAccessLogs/testServerAccessLogFile.js @@ -143,7 +143,7 @@ describe('Server Access Logs - File Output', async () => { 'authenticationType': 'AuthHeader', // STATIC // 'hostHeader': '', // UNKNOWN 'tlsVersion': null, // TODO: Add https tests. - 'aclRequired': null, // TODO: Add https tests. + 'aclRequired': null, // DYNAMIC (absent for owner, "Yes" when ACL is consulted) 'bucketOwner': '79a59df900b949e55d96a1e698fbacedfd6e09d98eacf8f8d5218e7cd47ef2be', // DYNAMIC bucketName, // DYNAMIC // 'req_id': '', // UNKNOWN diff --git a/tests/unit/api/apiUtils/permissionChecks.js b/tests/unit/api/apiUtils/permissionChecks.js index 8d059b9206..03005ed541 100644 --- a/tests/unit/api/apiUtils/permissionChecks.js +++ b/tests/unit/api/apiUtils/permissionChecks.js @@ -1,11 +1,15 @@ const assert = require('assert'); -const { isLifecycleSession, checkBucketPolicyResult, checkBucketPolicy } = +const { isLifecycleSession, checkBucketPolicyResult, checkBucketPolicy, + isBucketAuthorized, isObjAuthorized, evaluateBucketPolicyWithIAM } = require('../../../../lib/api/apiUtils/authorization/permissionChecks.js'); const { DummyRequestLogger } = require('../../helpers'); const stubLog = new DummyRequestLogger(); +const ownerCanonicalId = '79a59df900b949e55d96a1e698fbacedfd6e09d98eacf8f8d5218e7cd47ef2be'; +const otherCanonicalId = 'aaaaaaaabbbbbbbbccccccccddddddddeeeeeeeeffffffffgggggggghhhhhhh1'; + describe('authInfoHelper', () => { const tests = [ { @@ -708,3 +712,214 @@ describe('checkBucketPolicy Principal logic', () => { }); }); }); + +describe('aclRequired field in isBucketAuthorized', () => { + function makeBucket(owner, acl, bucketPolicy) { + return { + getOwner: () => owner, + getAcl: () => ({ + Canned: acl?.Canned || '', + FULL_CONTROL: acl?.FULL_CONTROL || [], + READ: acl?.READ || [], + READ_ACP: acl?.READ_ACP || [], + WRITE: acl?.WRITE || [], + WRITE_ACP: acl?.WRITE_ACP || [], + }), + getBucketPolicy: () => bucketPolicy || null, + }; + } + + function makeRequest() { + return { serverAccessLog: {}, socket: { remoteAddress: '127.0.0.1' }, headers: {} }; + } + + function makeAuthInfo(canonicalID, arn, isIAMUser = false) { + return { + getCanonicalID: () => canonicalID, + getArn: () => arn, + isRequesterAnIAMUser: () => isIAMUser, + }; + } + + it('should not set aclRequired when requester is bucket owner', () => { + const request = makeRequest(); + const bucket = makeBucket(ownerCanonicalId); + const authInfo = makeAuthInfo(ownerCanonicalId, 'arn:aws:iam::123456789012:/owner/'); + isBucketAuthorized(bucket, 'bucketGet', ownerCanonicalId, authInfo, stubLog, + request, { bucketGet: false }); + assert.strictEqual(request.serverAccessLog.aclRequired, undefined); + }); + + it('should set aclRequired to Yes for non-owner with no bucket policy', () => { + const request = makeRequest(); + const bucket = makeBucket(ownerCanonicalId, { READ: [otherCanonicalId] }); + const authInfo = makeAuthInfo(otherCanonicalId, 'arn:aws:iam::999999999999:user/other'); + isBucketAuthorized(bucket, 'bucketGet', otherCanonicalId, authInfo, stubLog, + request, { bucketGet: false }); + assert.strictEqual(request.serverAccessLog.aclRequired, 'Yes'); + }); + + it('should not set aclRequired when bucket policy explicitly allows', () => { + const request = makeRequest(); + // Use an IAM user in the bucket owner's account so principal match is OK (not CROSS_ACCOUNT) + // and the owner early-return doesn't fire (requester is IAM user, not account root) + const iamUserCanonicalId = ownerCanonicalId; + const bucket = makeBucket(ownerCanonicalId, {}, { + Statement: [{ + Effect: 'Allow', + Principal: { AWS: 'arn:aws:iam::123456789012:user/iamuser' }, + Action: ['s3:ListBucket'], + Resource: ['arn:aws:s3:::test-bucket'], + }], + }); + const authInfo = makeAuthInfo(iamUserCanonicalId, 'arn:aws:iam::123456789012:user/iamuser', true); + isBucketAuthorized(bucket, 'bucketGet', iamUserCanonicalId, authInfo, stubLog, + request, { bucketGet: false }); + assert.strictEqual(request.serverAccessLog.aclRequired, undefined); + }); + + it('should not set aclRequired when bucket policy explicitly denies', () => { + const request = makeRequest(); + const iamUserCanonicalId = ownerCanonicalId; + const bucket = makeBucket(ownerCanonicalId, { READ: [iamUserCanonicalId] }, { + Statement: [{ + Effect: 'Deny', + Principal: { AWS: 'arn:aws:iam::123456789012:user/iamuser' }, + Action: ['s3:ListBucket'], + Resource: ['arn:aws:s3:::test-bucket'], + }], + }); + const authInfo = makeAuthInfo(iamUserCanonicalId, 'arn:aws:iam::123456789012:user/iamuser', true); + isBucketAuthorized(bucket, 'bucketGet', iamUserCanonicalId, authInfo, stubLog, + request, { bucketGet: false }); + assert.strictEqual(request.serverAccessLog.aclRequired, undefined); + }); + + it('should set aclRequired to Yes when bucket policy returns DEFAULT_DENY', () => { + const request = makeRequest(); + // Policy grants PutObject to a different principal — nothing matches + // the bucketGet request from otherCanonicalId, so checkBucketPolicy + // returns DEFAULT_DENY and falls back to ACL evaluation. + const bucket = makeBucket(ownerCanonicalId, { READ: [otherCanonicalId] }, { + Statement: [{ + Effect: 'Allow', + Principal: { AWS: 'arn:aws:iam::111111111111:root' }, + Action: ['s3:PutObject'], + Resource: ['arn:aws:s3:::test-bucket/*'], + }], + }); + const authInfo = makeAuthInfo(otherCanonicalId, 'arn:aws:iam::999999999999:user/other'); + isBucketAuthorized(bucket, 'bucketGet', otherCanonicalId, authInfo, stubLog, + request, { bucketGet: false }); + assert.strictEqual(request.serverAccessLog.aclRequired, 'Yes'); + }); + + it('should handle missing serverAccessLog gracefully', () => { + const request = {}; + const bucket = makeBucket(ownerCanonicalId, { READ: [otherCanonicalId] }); + const authInfo = makeAuthInfo(otherCanonicalId, 'arn:aws:iam::999999999999:user/other'); + assert.doesNotThrow(() => { + isBucketAuthorized(bucket, 'bucketGet', otherCanonicalId, authInfo, stubLog, + request, { bucketGet: false }); + }); + }); +}); + +describe('aclRequired field in isObjAuthorized', () => { + function makeBucket(owner, acl, bucketPolicy) { + return { + getOwner: () => owner, + getName: () => 'test-bucket', + getAcl: () => ({ + Canned: acl?.Canned || '', + FULL_CONTROL: acl?.FULL_CONTROL || [], + READ: acl?.READ || [], + READ_ACP: acl?.READ_ACP || [], + WRITE: acl?.WRITE || [], + WRITE_ACP: acl?.WRITE_ACP || [], + }), + getBucketPolicy: () => bucketPolicy || null, + }; + } + + function makeRequest() { + return { serverAccessLog: {} }; + } + + function makeAuthInfo(canonicalID, arn) { + return { + getCanonicalID: () => canonicalID, + getArn: () => arn, + isRequesterAnIAMUser: () => false, + }; + } + + function makeObjectMD(ownerId) { + return { + 'owner-id': ownerId, + acl: { + Canned: '', + FULL_CONTROL: [], + READ: [], + READ_ACP: [], + WRITE: [], + WRITE_ACP: [], + }, + }; + } + + it('should not set aclRequired when requester is object owner', () => { + const request = makeRequest(); + const bucket = makeBucket(ownerCanonicalId); + const objectMD = makeObjectMD(otherCanonicalId); + const authInfo = makeAuthInfo(otherCanonicalId, 'arn:aws:iam::999999999999:/account/'); + isObjAuthorized(bucket, objectMD, 'objectGet', otherCanonicalId, authInfo, stubLog, + request, { objectGet: false }); + assert.strictEqual(request.serverAccessLog.aclRequired, undefined); + }); + + it('should set aclRequired to Yes for non-owner with no bucket policy', () => { + const request = makeRequest(); + const bucket = makeBucket(ownerCanonicalId); + const objectMD = makeObjectMD(ownerCanonicalId); + objectMD.acl.READ = [otherCanonicalId]; + const authInfo = makeAuthInfo(otherCanonicalId, 'arn:aws:iam::999999999999:user/other'); + isObjAuthorized(bucket, objectMD, 'objectGet', otherCanonicalId, authInfo, stubLog, + request, { objectGet: false }); + assert.strictEqual(request.serverAccessLog.aclRequired, 'Yes'); + }); +}); + +describe('aclRequired field in evaluateBucketPolicyWithIAM', () => { + function makeBucket(owner, bucketPolicy) { + return { + getOwner: () => owner, + getAcl: () => ({ + Canned: '', + FULL_CONTROL: [], + READ: [], + READ_ACP: [], + WRITE: [], + WRITE_ACP: [], + }), + getBucketPolicy: () => bucketPolicy || null, + }; + } + + function makeAuthInfo(canonicalID, arn) { + return { + getCanonicalID: () => canonicalID, + getArn: () => arn, + isRequesterAnIAMUser: () => false, + }; + } + + it('should not set aclRequired (ACLs are not actually consulted)', () => { + const request = { serverAccessLog: {} }; + const bucket = makeBucket(ownerCanonicalId); + const authInfo = makeAuthInfo(otherCanonicalId, 'arn:aws:iam::999999999999:user/other'); + evaluateBucketPolicyWithIAM(bucket, 'objectDelete', otherCanonicalId, authInfo, + { objectDelete: false }, stubLog, request); + assert.strictEqual(request.serverAccessLog.aclRequired, undefined); + }); +}); diff --git a/tests/unit/metadata/metadataUtils.spec.js b/tests/unit/metadata/metadataUtils.spec.js index 50aab8ddb1..4affcf29b8 100644 --- a/tests/unit/metadata/metadataUtils.spec.js +++ b/tests/unit/metadata/metadataUtils.spec.js @@ -18,6 +18,7 @@ const { validateBucket, metadataGetObjects, metadataGetObject, + storeServerAccessLogInfo, } = require('../../../lib/metadata/metadataUtils'); const metadata = require('../../../lib/metadata/wrapper'); @@ -150,3 +151,35 @@ describe('metadataGetObject', () => { }); }); }); + +describe('storeServerAccessLogInfo - copySource aclRequired', () => { + it('should move source aclRequired to sourceServerAccessLog and restore destination value', () => { + // Destination auth set aclRequired='Yes', then source auth ran on the + // same request object and did not set aclRequired (owner on source). + const request = { + serverAccessLog: {}, + }; + const options = { + copySource: true, + savedAclRequired: 'Yes', + }; + storeServerAccessLogInfo(request, null, null, options); + assert.strictEqual(request.sourceServerAccessLog.aclRequired, undefined); + assert.strictEqual(request.serverAccessLog.aclRequired, 'Yes'); + }); + + it('should swap aclRequired when source auth also required ACL check', () => { + // Destination auth did not set aclRequired (owner on dest), then + // source auth set aclRequired='Yes' on the same request object. + const request = { + serverAccessLog: { aclRequired: 'Yes' }, + }; + const options = { + copySource: true, + savedAclRequired: undefined, + }; + storeServerAccessLogInfo(request, null, null, options); + assert.strictEqual(request.sourceServerAccessLog.aclRequired, 'Yes'); + assert.strictEqual(request.serverAccessLog.aclRequired, undefined); + }); +}); diff --git a/tests/unit/utils/serverAccessLogger.js b/tests/unit/utils/serverAccessLogger.js index e41648d565..03e5572d3e 100644 --- a/tests/unit/utils/serverAccessLogger.js +++ b/tests/unit/utils/serverAccessLogger.js @@ -1231,6 +1231,46 @@ describe('serverAccessLogger utility functions', () => { assert.strictEqual('loggingEnabled' in loggedData, true); }); + it('should log aclRequired as Yes when set on request', () => { + setServerAccessLogger(mockLogger); + const req = { + serverAccessLog: { + aclRequired: 'Yes', + }, + headers: {}, + socket: {}, + }; + const res = { + serverAccessLog: {}, + getHeader: () => null, + }; + + logServerAccess(req, res); + + assert.strictEqual(mockLogger.write.callCount, 1); + const loggedData = JSON.parse(mockLogger.write.firstCall.args[0].trim()); + assert.strictEqual(loggedData.aclRequired, 'Yes'); + }); + + it('should omit aclRequired when not set on request', () => { + setServerAccessLogger(mockLogger); + const req = { + serverAccessLog: {}, + headers: {}, + socket: {}, + }; + const res = { + serverAccessLog: {}, + getHeader: () => null, + }; + + logServerAccess(req, res); + + assert.strictEqual(mockLogger.write.callCount, 1); + const loggedData = JSON.parse(mockLogger.write.firstCall.args[0].trim()); + assert.strictEqual('aclRequired' in loggedData, false); + }); + it('should include error code when present', () => { setServerAccessLogger(mockLogger); const req = {