From 947878627201727c54d2c8ca23d4639c8468d6d5 Mon Sep 17 00:00:00 2001 From: Dimitrios Vasilas Date: Mon, 6 Apr 2026 14:52:46 +0300 Subject: [PATCH 1/3] CLDSRV-774: Add aclRequired field to server access logs aclRequired indicates whether the request required an ACL for authorization. It is "Yes" when no bucket policy exists or when the bucket policy returns DEFAULT_DENY, falling back to ACL evaluation. It is "-" for owner requests, service accounts, and requests decided by IAM or bucket policy alone. For copy operations, the source bucket auth runs on the same request object and would overwrite the destination's aclRequired. The value is saved before source auth, then moved to sourceServerAccessLog and the destination's value is restored. --- .../authorization/permissionChecks.js | 24 +++++++++++++++---- lib/metadata/metadataUtils.js | 9 +++++++ lib/utilities/serverAccessLogger.js | 6 +++-- 3 files changed, 32 insertions(+), 7 deletions(-) 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`); From cdfe3cc956cfc4f64ed5d3f7549ac770dca26e3f Mon Sep 17 00:00:00 2001 From: Dimitrios Vasilas Date: Mon, 6 Apr 2026 14:53:20 +0300 Subject: [PATCH 2/3] CLDSRV-774: Add aclRequired field tests --- .../testServerAccessLogFile.js | 2 +- tests/unit/api/apiUtils/permissionChecks.js | 217 +++++++++++++++++- tests/unit/metadata/metadataUtils.spec.js | 33 +++ tests/unit/utils/serverAccessLogger.js | 40 ++++ 4 files changed, 290 insertions(+), 2 deletions(-) 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 = { From 9d6061af1b3e3844b2a328e7198b1a0a76e32a60 Mon Sep 17 00:00:00 2001 From: Dimitrios Vasilas Date: Tue, 7 Apr 2026 16:45:09 +0300 Subject: [PATCH 3/3] CLDSRV-774: Bump version to 9.2.34 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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": {