Skip to content

Commit 09256e2

Browse files
committed
Merge branch 'w/9.3/improvement/CLDSRV-774' into tmp/octopus/w/9.4/improvement/CLDSRV-774
2 parents 87be88a + b27136c commit 09256e2

7 files changed

Lines changed: 322 additions & 9 deletions

File tree

lib/api/apiUtils/authorization/permissionChecks.js

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -509,8 +509,10 @@ function processBucketPolicy(requestType, bucket, canonicalID, arn, bucketOwner,
509509
request, aclPermission, results, actionImplicitDenies) {
510510
const bucketPolicy = bucket.getBucketPolicy();
511511
let processedResult = results[requestType];
512+
let aclRequired = false;
512513
if (!bucketPolicy || request?.bypassUserBucketPolicies) {
513514
processedResult = actionImplicitDenies[requestType] === false && aclPermission;
515+
aclRequired = true;
514516
} else {
515517
const bucketPolicyPermission = checkBucketPolicy(bucketPolicy, requestType, canonicalID, arn,
516518
bucketOwner, log, request, actionImplicitDenies);
@@ -525,9 +527,10 @@ function processBucketPolicy(requestType, bucket, canonicalID, arn, bucketOwner,
525527
processedResult = true;
526528
} else {
527529
processedResult = actionImplicitDenies[requestType] === false && aclPermission;
530+
aclRequired = true;
528531
}
529532
}
530-
return processedResult;
533+
return { allowed: processedResult, aclRequired };
531534
}
532535

533536
function isBucketAuthorized(bucket, requestTypesInput, canonicalID, authInfo, log, request,
@@ -564,8 +567,13 @@ function isBucketAuthorized(bucket, requestTypesInput, canonicalID, authInfo, lo
564567
_requestType = 'objectGet';
565568
actionImplicitDenies.objectGet = actionImplicitDenies.objectGet || false;
566569
}
567-
return processBucketPolicy(_requestType, bucket, canonicalID, arn, bucket.getOwner(), log,
568-
request, aclPermission, results, actionImplicitDenies);
570+
const { allowed, aclRequired } = processBucketPolicy(_requestType, bucket, canonicalID, arn,
571+
bucket.getOwner(), log, request, aclPermission, results, actionImplicitDenies);
572+
if (aclRequired && request?.serverAccessLog) {
573+
// eslint-disable-next-line no-param-reassign
574+
request.serverAccessLog.aclRequired = 'Yes';
575+
}
576+
return allowed;
569577
});
570578
}
571579

@@ -582,8 +590,9 @@ function evaluateBucketPolicyWithIAM(bucket, requestTypesInput, canonicalID, aut
582590
if (authInfo) {
583591
arn = authInfo.getArn();
584592
}
585-
return processBucketPolicy(_requestType, bucket, canonicalID, arn, bucket.getOwner(), log,
593+
const { allowed } = processBucketPolicy(_requestType, bucket, canonicalID, arn, bucket.getOwner(), log,
586594
request, true, results, actionImplicitDenies);
595+
return allowed;
587596
});
588597
}
589598

@@ -641,8 +650,13 @@ function isObjAuthorized(bucket, objectMD, requestTypesInput, canonicalID, authI
641650
}
642651
const aclPermission = checkObjectAcls(bucket, objectMD, parsedMethodName,
643652
canonicalID, requesterIsNotUser, isUserUnauthenticated, mainApiCall);
644-
return processBucketPolicy(_requestType, bucket, canonicalID, arn, bucketOwner,
653+
const { allowed, aclRequired } = processBucketPolicy(_requestType, bucket, canonicalID, arn, bucketOwner,
645654
log, request, aclPermission, results, actionImplicitDenies);
655+
if (aclRequired && request?.serverAccessLog) {
656+
// eslint-disable-next-line no-param-reassign
657+
request.serverAccessLog.aclRequired = 'Yes';
658+
}
659+
return allowed;
646660
});
647661
}
648662

lib/metadata/metadataUtils.js

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,11 @@ function storeServerAccessLogInfo(request, bucket, raftSessionId, options = {})
5151
target.enabled = true;
5252
target.loggingEnabled = bucket.getBucketLoggingStatus().getLoggingEnabled();
5353
}
54+
// Source auth runs on the same request object and overwrites the
55+
// destination's aclRequired. Move the source value to its log entry
56+
// and restore the destination value that was saved before source auth.
57+
target.aclRequired = request.serverAccessLog.aclRequired;
58+
request.serverAccessLog.aclRequired = options.savedAclRequired;
5459
} else {
5560
// Default behavior: store in main serverAccessLog
5661
request.serverAccessLog.raftSessionID = raftSessionId;
@@ -327,6 +332,10 @@ function standardMetadataValidateBucketAndObj(params, actionImplicitDenies, log,
327332
if (!Array.isArray(requestType)) {
328333
requestType = [requestType];
329334
}
335+
if (params.serverAccessLogOptions?.copySource) {
336+
// eslint-disable-next-line no-param-reassign
337+
params.serverAccessLogOptions.savedAclRequired = request?.serverAccessLog?.aclRequired;
338+
}
330339
async.waterfall([
331340
next => {
332341
// versionId may be 'null', which asks metadata to fetch the null key specifically

lib/utilities/serverAccessLogger.js

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -496,7 +496,7 @@ function buildLogEntry(req, params, options) {
496496
tlsVersion: req.socket?.encrypted
497497
? req.socket.getCipher()['version']
498498
: req.headers?.['x-ssl-protocol'] ?? undefined,
499-
aclRequired: options.aclRequired ?? undefined, // TODO: CLDSRV-774
499+
aclRequired: options.aclRequired ?? undefined,
500500
// hostID: undefined, // NOT IMPLEMENTED
501501
// accessPointARN: undefined, // NOT IMPLEMENTED
502502

@@ -558,6 +558,7 @@ function logBatchDeleteObject(req, requestID, objectKey, objectSize, error) {
558558
errorCode,
559559
objectKey,
560560
requestID,
561+
aclRequired: req.serverAccessLog?.aclRequired,
561562
});
562563

563564
serverAccessLogger.write(`${JSON.stringify(logEntry)}\n`);
@@ -590,7 +591,7 @@ function logServerAccess(req, res) {
590591
totalTime: calculateTotalTime(params.startTime, params.onFinishEndTime),
591592
turnAroundTime: calculateTurnAroundTime(params.startTurnAroundTime, endTurnAroundTime),
592593
versionId: req.query?.versionId,
593-
aclRequired: undefined, // TODO: CLDSRV-774
594+
aclRequired: req.serverAccessLog.aclRequired,
594595
referer: req.headers?.referer,
595596
userAgent: req.headers?.['user-agent'],
596597
requestID,
@@ -660,6 +661,7 @@ function logCopySourceAccess(req, requestID, operation, sourceBucket, sourceObje
660661
httpCode,
661662
errorCode,
662663
requestID,
664+
aclRequired: params.aclRequired,
663665
});
664666

665667
serverAccessLogger.write(`${JSON.stringify(logEntry)}\n`);

tests/functional/aws-node-sdk/test/serverAccessLogs/testServerAccessLogFile.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,7 @@ describe('Server Access Logs - File Output', async () => {
177177
'authenticationType': 'AuthHeader', // STATIC
178178
// 'hostHeader': '', // UNKNOWN
179179
'tlsVersion': null, // TODO: Add https tests.
180-
'aclRequired': null, // TODO: Add https tests.
180+
'aclRequired': null, // DYNAMIC (absent for owner, "Yes" when ACL is consulted)
181181
'bucketOwner': '79a59df900b949e55d96a1e698fbacedfd6e09d98eacf8f8d5218e7cd47ef2be', // DYNAMIC
182182
bucketName, // DYNAMIC
183183
// 'req_id': '', // UNKNOWN

tests/unit/api/apiUtils/permissionChecks.js

Lines changed: 216 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,15 @@
11
const assert = require('assert');
22

3-
const { isLifecycleSession, checkBucketPolicyResult, checkBucketPolicy } =
3+
const { isLifecycleSession, checkBucketPolicyResult, checkBucketPolicy,
4+
isBucketAuthorized, isObjAuthorized, evaluateBucketPolicyWithIAM } =
45
require('../../../../lib/api/apiUtils/authorization/permissionChecks.js');
56
const { DummyRequestLogger } = require('../../helpers');
67

78
const stubLog = new DummyRequestLogger();
89

10+
const ownerCanonicalId = '79a59df900b949e55d96a1e698fbacedfd6e09d98eacf8f8d5218e7cd47ef2be';
11+
const otherCanonicalId = 'aaaaaaaabbbbbbbbccccccccddddddddeeeeeeeeffffffffgggggggghhhhhhh1';
12+
913
describe('authInfoHelper', () => {
1014
const tests = [
1115
{
@@ -708,3 +712,214 @@ describe('checkBucketPolicy Principal logic', () => {
708712
});
709713
});
710714
});
715+
716+
describe('aclRequired field in isBucketAuthorized', () => {
717+
function makeBucket(owner, acl, bucketPolicy) {
718+
return {
719+
getOwner: () => owner,
720+
getAcl: () => ({
721+
Canned: acl?.Canned || '',
722+
FULL_CONTROL: acl?.FULL_CONTROL || [],
723+
READ: acl?.READ || [],
724+
READ_ACP: acl?.READ_ACP || [],
725+
WRITE: acl?.WRITE || [],
726+
WRITE_ACP: acl?.WRITE_ACP || [],
727+
}),
728+
getBucketPolicy: () => bucketPolicy || null,
729+
};
730+
}
731+
732+
function makeRequest() {
733+
return { serverAccessLog: {}, socket: { remoteAddress: '127.0.0.1' }, headers: {} };
734+
}
735+
736+
function makeAuthInfo(canonicalID, arn, isIAMUser = false) {
737+
return {
738+
getCanonicalID: () => canonicalID,
739+
getArn: () => arn,
740+
isRequesterAnIAMUser: () => isIAMUser,
741+
};
742+
}
743+
744+
it('should not set aclRequired when requester is bucket owner', () => {
745+
const request = makeRequest();
746+
const bucket = makeBucket(ownerCanonicalId);
747+
const authInfo = makeAuthInfo(ownerCanonicalId, 'arn:aws:iam::123456789012:/owner/');
748+
isBucketAuthorized(bucket, 'bucketGet', ownerCanonicalId, authInfo, stubLog,
749+
request, { bucketGet: false });
750+
assert.strictEqual(request.serverAccessLog.aclRequired, undefined);
751+
});
752+
753+
it('should set aclRequired to Yes for non-owner with no bucket policy', () => {
754+
const request = makeRequest();
755+
const bucket = makeBucket(ownerCanonicalId, { READ: [otherCanonicalId] });
756+
const authInfo = makeAuthInfo(otherCanonicalId, 'arn:aws:iam::999999999999:user/other');
757+
isBucketAuthorized(bucket, 'bucketGet', otherCanonicalId, authInfo, stubLog,
758+
request, { bucketGet: false });
759+
assert.strictEqual(request.serverAccessLog.aclRequired, 'Yes');
760+
});
761+
762+
it('should not set aclRequired when bucket policy explicitly allows', () => {
763+
const request = makeRequest();
764+
// Use an IAM user in the bucket owner's account so principal match is OK (not CROSS_ACCOUNT)
765+
// and the owner early-return doesn't fire (requester is IAM user, not account root)
766+
const iamUserCanonicalId = ownerCanonicalId;
767+
const bucket = makeBucket(ownerCanonicalId, {}, {
768+
Statement: [{
769+
Effect: 'Allow',
770+
Principal: { AWS: 'arn:aws:iam::123456789012:user/iamuser' },
771+
Action: ['s3:ListBucket'],
772+
Resource: ['arn:aws:s3:::test-bucket'],
773+
}],
774+
});
775+
const authInfo = makeAuthInfo(iamUserCanonicalId, 'arn:aws:iam::123456789012:user/iamuser', true);
776+
isBucketAuthorized(bucket, 'bucketGet', iamUserCanonicalId, authInfo, stubLog,
777+
request, { bucketGet: false });
778+
assert.strictEqual(request.serverAccessLog.aclRequired, undefined);
779+
});
780+
781+
it('should not set aclRequired when bucket policy explicitly denies', () => {
782+
const request = makeRequest();
783+
const iamUserCanonicalId = ownerCanonicalId;
784+
const bucket = makeBucket(ownerCanonicalId, { READ: [iamUserCanonicalId] }, {
785+
Statement: [{
786+
Effect: 'Deny',
787+
Principal: { AWS: 'arn:aws:iam::123456789012:user/iamuser' },
788+
Action: ['s3:ListBucket'],
789+
Resource: ['arn:aws:s3:::test-bucket'],
790+
}],
791+
});
792+
const authInfo = makeAuthInfo(iamUserCanonicalId, 'arn:aws:iam::123456789012:user/iamuser', true);
793+
isBucketAuthorized(bucket, 'bucketGet', iamUserCanonicalId, authInfo, stubLog,
794+
request, { bucketGet: false });
795+
assert.strictEqual(request.serverAccessLog.aclRequired, undefined);
796+
});
797+
798+
it('should set aclRequired to Yes when bucket policy returns DEFAULT_DENY', () => {
799+
const request = makeRequest();
800+
// Policy grants PutObject to a different principal — nothing matches
801+
// the bucketGet request from otherCanonicalId, so checkBucketPolicy
802+
// returns DEFAULT_DENY and falls back to ACL evaluation.
803+
const bucket = makeBucket(ownerCanonicalId, { READ: [otherCanonicalId] }, {
804+
Statement: [{
805+
Effect: 'Allow',
806+
Principal: { AWS: 'arn:aws:iam::111111111111:root' },
807+
Action: ['s3:PutObject'],
808+
Resource: ['arn:aws:s3:::test-bucket/*'],
809+
}],
810+
});
811+
const authInfo = makeAuthInfo(otherCanonicalId, 'arn:aws:iam::999999999999:user/other');
812+
isBucketAuthorized(bucket, 'bucketGet', otherCanonicalId, authInfo, stubLog,
813+
request, { bucketGet: false });
814+
assert.strictEqual(request.serverAccessLog.aclRequired, 'Yes');
815+
});
816+
817+
it('should handle missing serverAccessLog gracefully', () => {
818+
const request = {};
819+
const bucket = makeBucket(ownerCanonicalId, { READ: [otherCanonicalId] });
820+
const authInfo = makeAuthInfo(otherCanonicalId, 'arn:aws:iam::999999999999:user/other');
821+
assert.doesNotThrow(() => {
822+
isBucketAuthorized(bucket, 'bucketGet', otherCanonicalId, authInfo, stubLog,
823+
request, { bucketGet: false });
824+
});
825+
});
826+
});
827+
828+
describe('aclRequired field in isObjAuthorized', () => {
829+
function makeBucket(owner, acl, bucketPolicy) {
830+
return {
831+
getOwner: () => owner,
832+
getName: () => 'test-bucket',
833+
getAcl: () => ({
834+
Canned: acl?.Canned || '',
835+
FULL_CONTROL: acl?.FULL_CONTROL || [],
836+
READ: acl?.READ || [],
837+
READ_ACP: acl?.READ_ACP || [],
838+
WRITE: acl?.WRITE || [],
839+
WRITE_ACP: acl?.WRITE_ACP || [],
840+
}),
841+
getBucketPolicy: () => bucketPolicy || null,
842+
};
843+
}
844+
845+
function makeRequest() {
846+
return { serverAccessLog: {} };
847+
}
848+
849+
function makeAuthInfo(canonicalID, arn) {
850+
return {
851+
getCanonicalID: () => canonicalID,
852+
getArn: () => arn,
853+
isRequesterAnIAMUser: () => false,
854+
};
855+
}
856+
857+
function makeObjectMD(ownerId) {
858+
return {
859+
'owner-id': ownerId,
860+
acl: {
861+
Canned: '',
862+
FULL_CONTROL: [],
863+
READ: [],
864+
READ_ACP: [],
865+
WRITE: [],
866+
WRITE_ACP: [],
867+
},
868+
};
869+
}
870+
871+
it('should not set aclRequired when requester is object owner', () => {
872+
const request = makeRequest();
873+
const bucket = makeBucket(ownerCanonicalId);
874+
const objectMD = makeObjectMD(otherCanonicalId);
875+
const authInfo = makeAuthInfo(otherCanonicalId, 'arn:aws:iam::999999999999:/account/');
876+
isObjAuthorized(bucket, objectMD, 'objectGet', otherCanonicalId, authInfo, stubLog,
877+
request, { objectGet: false });
878+
assert.strictEqual(request.serverAccessLog.aclRequired, undefined);
879+
});
880+
881+
it('should set aclRequired to Yes for non-owner with no bucket policy', () => {
882+
const request = makeRequest();
883+
const bucket = makeBucket(ownerCanonicalId);
884+
const objectMD = makeObjectMD(ownerCanonicalId);
885+
objectMD.acl.READ = [otherCanonicalId];
886+
const authInfo = makeAuthInfo(otherCanonicalId, 'arn:aws:iam::999999999999:user/other');
887+
isObjAuthorized(bucket, objectMD, 'objectGet', otherCanonicalId, authInfo, stubLog,
888+
request, { objectGet: false });
889+
assert.strictEqual(request.serverAccessLog.aclRequired, 'Yes');
890+
});
891+
});
892+
893+
describe('aclRequired field in evaluateBucketPolicyWithIAM', () => {
894+
function makeBucket(owner, bucketPolicy) {
895+
return {
896+
getOwner: () => owner,
897+
getAcl: () => ({
898+
Canned: '',
899+
FULL_CONTROL: [],
900+
READ: [],
901+
READ_ACP: [],
902+
WRITE: [],
903+
WRITE_ACP: [],
904+
}),
905+
getBucketPolicy: () => bucketPolicy || null,
906+
};
907+
}
908+
909+
function makeAuthInfo(canonicalID, arn) {
910+
return {
911+
getCanonicalID: () => canonicalID,
912+
getArn: () => arn,
913+
isRequesterAnIAMUser: () => false,
914+
};
915+
}
916+
917+
it('should not set aclRequired (ACLs are not actually consulted)', () => {
918+
const request = { serverAccessLog: {} };
919+
const bucket = makeBucket(ownerCanonicalId);
920+
const authInfo = makeAuthInfo(otherCanonicalId, 'arn:aws:iam::999999999999:user/other');
921+
evaluateBucketPolicyWithIAM(bucket, 'objectDelete', otherCanonicalId, authInfo,
922+
{ objectDelete: false }, stubLog, request);
923+
assert.strictEqual(request.serverAccessLog.aclRequired, undefined);
924+
});
925+
});

0 commit comments

Comments
 (0)