Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvement/cldsrv 426 acl imp deny #5323

Closed
wants to merge 2 commits into from
Closed

Improvement/cldsrv 426 acl imp deny #5323

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
849c926
CLDSRV-424: api call updated with implicit deny logic
Sep 11, 2023
8cebedf
CLDSRV-426: update ACL permission checks for implicitDeny logic
Sep 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 37 additions & 12 deletions lib/api/apiUtils/authorization/permissionChecks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,32 @@ const { allAuthedUsersId, bucketOwnerActions, logId, publicId,
const publicReadBuckets = process.env.ALLOW_PUBLIC_READ_BUCKETS ?
process.env.ALLOW_PUBLIC_READ_BUCKETS.split(',') : [];

function checkBucketAcls(bucket, requestType, canonicalID) {
function checkBucketAcls(bucket, requestType, canonicalID, mainApiCall) {
// Same logic applies on the Versioned APIs, so let's simplify it.
const requestTypeParsed = requestType.endsWith('Version') ?
requestType.slice(0, -7) : requestType;
if (bucket.getOwner() === canonicalID) {
return true;
}
// Backward compatibility
const arrayOfAllowed = [
'objectPutTagging',
'objectPutLegalHold',
'objectPutRetention',
];
if (mainApiCall === 'objectGet') {
if (requestTypeParsed === 'objectGetTagging') {
return true;
}
}
if (mainApiCall === 'objectPut') {
if (arrayOfAllowed.includes(requestTypeParsed)) {
return true;
}
}

const bucketAcl = bucket.getAcl();
if (requestType === 'bucketGet' || requestType === 'bucketHead') {
if (requestTypeParsed === 'bucketGet' || requestTypeParsed === 'bucketHead') {
if (bucketAcl.Canned === 'public-read'
|| bucketAcl.Canned === 'public-read-write'
|| (bucketAcl.Canned === 'authenticated-read'
Expand All @@ -32,7 +51,7 @@ function checkBucketAcls(bucket, requestType, canonicalID) {
return true;
}
}
if (requestType === 'bucketGetACL') {
if (requestTypeParsed === 'bucketGetACL') {
if ((bucketAcl.Canned === 'log-delivery-write'
&& canonicalID === logId)
|| bucketAcl.FULL_CONTROL.indexOf(canonicalID) > -1
Expand All @@ -48,7 +67,7 @@ function checkBucketAcls(bucket, requestType, canonicalID) {
}
}

if (requestType === 'bucketPutACL') {
if (requestTypeParsed === 'bucketPutACL') {
if (bucketAcl.FULL_CONTROL.indexOf(canonicalID) > -1
|| bucketAcl.WRITE_ACP.indexOf(canonicalID) > -1) {
return true;
Expand All @@ -62,11 +81,7 @@ function checkBucketAcls(bucket, requestType, canonicalID) {
}
}

if (requestType === 'bucketDelete' && bucket.getOwner() === canonicalID) {
return true;
}

if (requestType === 'objectDelete' || requestType === 'objectPut') {
if (requestTypeParsed === 'objectDelete' || requestTypeParsed === 'objectPut') {
if (bucketAcl.Canned === 'public-read-write'
|| bucketAcl.FULL_CONTROL.indexOf(canonicalID) > -1
|| bucketAcl.WRITE.indexOf(canonicalID) > -1) {
Expand All @@ -86,11 +101,12 @@ function checkBucketAcls(bucket, requestType, canonicalID) {
// objectPutACL, objectGetACL, objectHead or objectGet, the bucket
// authorization check should just return true so can move on to check
// rights at the object level.
return (requestType === 'objectPutACL' || requestType === 'objectGetACL' ||
requestType === 'objectGet' || requestType === 'objectHead');
return (requestTypeParsed === 'objectPutACL' || requestTypeParsed === 'objectGetACL' ||
requestTypeParsed === 'objectGet' || requestTypeParsed === 'objectHead');
}

function checkObjectAcls(bucket, objectMD, requestType, canonicalID) {
function checkObjectAcls(bucket, objectMD, requestType, canonicalID, requesterIsNotUser,
isUserUnauthenticated, mainApiCall) {
const bucketOwner = bucket.getOwner();
// acls don't distinguish between users and accounts, so both should be allowed
if (bucketOwnerActions.includes(requestType)
Expand All @@ -100,6 +116,15 @@ function checkObjectAcls(bucket, objectMD, requestType, canonicalID) {
if (objectMD['owner-id'] === canonicalID) {
return true;
}

// Backward compatibility
if (mainApiCall === 'objectGet') {
if ((isUserUnauthenticated || (requesterIsNotUser && bucketOwner === objectMD['owner-id']))
&& requestType === 'objectGetTagging') {
return true;
}
}

if (!objectMD.acl) {
return false;
}
Expand Down
269 changes: 269 additions & 0 deletions tests/unit/auth/permissionChecks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,269 @@
const assert = require('assert');
const { checkBucketAcls, checkObjectAcls } = require('../../../lib/api/apiUtils/authorization/permissionChecks');
const constants = require('../../../constants');

const { bucketOwnerActions } = constants;

describe('checkBucketAcls', () => {
const mockBucket = {
getOwner: () => 'ownerId',
getAcl: () => ({
Canned: '',
FULL_CONTROL: [],
READ: [],
READ_ACP: [],
WRITE: [],
WRITE_ACP: [],
}),
};

const testScenarios = [
{
description: 'should return true if bucket owner matches canonicalID',
input: {
bucketAcl: {}, requestType: 'anyType', canonicalID: 'ownerId', mainApiCall: 'anyApiCall',
},
expected: true,
},
{
description: 'should return true for objectGetTagging when mainApiCall is objectGet',
input: {
bucketAcl: {}, requestType: 'objectGetTagging', canonicalID: 'anyId', mainApiCall: 'objectGet',
},
expected: true,
},
{
description: 'should return true for objectPutTagging when mainApiCall is objectPut',
input: {
bucketAcl: {}, requestType: 'objectPutTagging', canonicalID: 'anyId', mainApiCall: 'objectPut',
},
expected: true,
},
{
description: 'should return true for objectPutLegalHold when mainApiCall is objectPut',
input: {
bucketAcl: {}, requestType: 'objectPutLegalHold', canonicalID: 'anyId', mainApiCall: 'objectPut',
},
expected: true,
},
{
description: 'should return true for objectPutRetention when mainApiCall is objectPut',
input: {
bucketAcl: {}, requestType: 'objectPutRetention', canonicalID: 'anyId', mainApiCall: 'objectPut',
},
expected: true,
},
{
description: 'should return true for bucketGet if canned acl is public-read-write',
input: {
bucketAcl: { Canned: 'public-read-write' },
requestType: 'bucketGet',
canonicalID: 'anyId',
mainApiCall: 'anyApiCall',
},
expected: true,
},
{
description: 'should return true for bucketGet if canned acl is authenticated-read and id is not publicId',
input: {
bucketAcl: { Canned: 'authenticated-read' },
requestType: 'bucketGet',
canonicalID: 'anyIdNotPublic',
mainApiCall: 'anyApiCall',
},
expected: true,
},
{
description: 'should return true for bucketHead if canned acl is public-read',
input: {
bucketAcl: { Canned: 'public-read' },
requestType: 'bucketHead',
canonicalID: 'anyId',
mainApiCall: 'anyApiCall',
},
expected: true,
},
{
description: 'should return true for bucketGet if canonicalID has FULL_CONTROL access',
input: {
bucketAcl: { FULL_CONTROL: ['anyId'], READ: [] },
requestType: 'bucketGet',
canonicalID: 'anyId',
mainApiCall: 'anyApiCall',
},
expected: true,
},
{
description: 'should return true for bucketGetACL if canonicalID has FULL_CONTROL',
input: {
bucketAcl: { FULL_CONTROL: ['anyId'], READ_ACP: [] },
requestType: 'bucketGetACL',
canonicalID: 'anyId',
mainApiCall: 'anyApiCall',
},
expected: true,
},
{
description: 'should return true for objectDelete if bucketAcl.Canned is public-read-write',
input: {
bucketAcl: { Canned: 'public-read-write' },
requestType: 'objectDelete',
canonicalID: 'anyId',
mainApiCall: 'anyApiCall',
},
expected: true,
},
{
description: 'should return true for requestType ending with "Version"',
input: {
bucketAcl: {},
requestType: 'objectGetVersion',
canonicalID: 'anyId',
mainApiCall: 'objectGet',
},
expected: true,
},
{
description: 'should return true for objectPutACL',
input: {
bucketAcl: {},
requestType: 'objectPutACL',
canonicalID: 'anyId',
mainApiCall: 'anyApiCall',
},
expected: true,
},
{
description: 'should return true for objectGetACL',
input: {
bucketAcl: {},
requestType: 'objectGetACL',
canonicalID: 'anyId',
mainApiCall: 'anyApiCall',
},
expected: true,
},
{
description: 'should return false for unmatched scenarios',
input: {
bucketAcl: {},
requestType: 'unmatchedRequest',
canonicalID: 'anyId',
mainApiCall: 'anyApiCall',
},
expected: false,
},
];

testScenarios.forEach(scenario => {
it(scenario.description, () => {
// Mock the bucket based on the test scenario's input
mockBucket.getAcl = () => scenario.input.bucketAcl;

const result = checkBucketAcls(mockBucket,
scenario.input.requestType, scenario.input.canonicalID, scenario.input.mainApiCall);
assert.strictEqual(result, scenario.expected);
});
});
});

describe('checkObjectAcls', () => {
const mockBucket = {
getOwner: () => 'bucketOwnerId',
getName: () => 'bucketName',
getAcl: () => ({ Canned: '' }),
};
const mockObjectMD = {
'owner-id': 'objectOwnerId',
'acl': {
Canned: '',
FULL_CONTROL: [],
READ: [],
READ_ACP: [],
WRITE: [],
WRITE_ACP: [],
},
};

it('should return true if request type is in bucketOwnerActions and bucket owner matches canonicalID', () => {
assert.strictEqual(checkObjectAcls(mockBucket, mockObjectMD, bucketOwnerActions[0],
'bucketOwnerId', false, false, 'anyApiCall'), true);
});

it('should return true if objectMD owner matches canonicalID', () => {
assert.strictEqual(checkObjectAcls(mockBucket, mockObjectMD, 'anyType',
'objectOwnerId', false, false, 'anyApiCall'), true);
});

it('should return true for objectGetTagging when mainApiCall is objectGet and conditions met', () => {
assert.strictEqual(checkObjectAcls(mockBucket, mockObjectMD, 'objectGetTagging',
'anyIdNotPublic', true, true, 'objectGet'), true);
});

it('should return false if no acl provided in objectMD', () => {
const objMDWithoutAcl = Object.assign({}, mockObjectMD);
delete objMDWithoutAcl.acl;
assert.strictEqual(checkObjectAcls(mockBucket, objMDWithoutAcl, 'anyType',
'anyId', false, false, 'anyApiCall'), false);
});

const tests = [
{
acl: 'public-read', reqType: 'objectGet', id: 'anyIdNotPublic', expected: true,
},
{
acl: 'public-read-write', reqType: 'objectGet', id: 'anyIdNotPublic', expected: true,
},
{
acl: 'authenticated-read', reqType: 'objectGet', id: 'anyIdNotPublic', expected: true,
},
{
acl: 'bucket-owner-read', reqType: 'objectGet', id: 'bucketOwnerId', expected: true,
},
{
acl: 'bucket-owner-full-control', reqType: 'objectGet', id: 'bucketOwnerId', expected: true,
},
{
aclList: ['someId', 'anyIdNotPublic'],
aclField: 'FULL_CONTROL',
reqType: 'objectGet',
id: 'anyIdNotPublic',
expected: true,
},
{
aclList: ['someId', 'anyIdNotPublic'],
aclField: 'READ',
reqType: 'objectGet',
id: 'anyIdNotPublic',
expected: true,
},
{ reqType: 'objectPut', id: 'anyId', expected: true },
{ reqType: 'objectDelete', id: 'anyId', expected: true },
{
aclList: ['anyId'], aclField: 'FULL_CONTROL', reqType: 'objectPutACL', id: 'anyId', expected: true,
},
{
aclList: ['anyId'], aclField: 'FULL_CONTROL', reqType: 'objectGetACL', id: 'anyId', expected: true,
},
{
acl: '', reqType: 'objectGet', id: 'randomId', expected: false,
},
];

tests.forEach(test => {
it(`should return ${test.expected} for ${test.reqType} with ACL as ${test.acl
|| (`${test.aclField}:${JSON.stringify(test.aclList)}`)}`, () => {
if (test.acl) {
mockObjectMD.acl.Canned = test.acl;
} else if (test.aclList && test.aclField) {
mockObjectMD.acl[test.aclField] = test.aclList;
}

assert.strictEqual(
checkObjectAcls(mockBucket, mockObjectMD, test.reqType, test.id, false, false, 'anyApiCall'),
test.expected,
);
});
});
});

Loading