Skip to content

Commit d0b91f2

Browse files
committed
CLDSRV-616: Fix bucket policy check for anonymous req
When checking bucket policies and the following conditions are true: - The request is anonymous (`--no-sign-request`) - There is a bucket policy with AWS principal Then `_getAccountId` is called in arn === undefined and causes an exception to be thrown. The reason is that vault return the following authInfo with anonymous requests: { arn: undefined, canonicalID: 'http://acs.amazonaws.com/groups/global/AllUsers', shortid: undefined, email: undefined, accountDisplayName: undefined, IAMdisplayName: undefined } The fix is to check is to check is arn === undefined and fail the check if the policy principal is not '*' (cherry picked from commit d57e3a9)
1 parent 162bdd6 commit d0b91f2

2 files changed

Lines changed: 20 additions & 2 deletions

File tree

lib/api/apiUtils/authorization/permissionChecks.js

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -269,6 +269,10 @@ function _checkPrincipal(requester, principal) {
269269
if (principal === '*') {
270270
return true;
271271
}
272+
// User in unauthenticated (anonymous request)
273+
if (requester === undefined) {
274+
return false;
275+
}
272276
if (principal === requester) {
273277
return true;
274278
}

tests/unit/api/bucketPolicyAuth.js

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
const assert = require('assert');
22
const { BucketInfo, BucketPolicy } = require('arsenal').models;
3+
const AuthInfo = require('arsenal').auth.AuthInfo;
34
const constants = require('../../../constants');
45
const { isBucketAuthorized, isObjAuthorized, validatePolicyResource }
56
= require('../../../lib/api/apiUtils/authorization/permissionChecks');
@@ -35,6 +36,9 @@ const basePolicyObj = {
3536
};
3637
const bucketName = 'matchme';
3738
const log = new DummyRequestLogger();
39+
const publicUserAuthInfo = new AuthInfo({
40+
canonicalID: constants.publicId,
41+
});
3842

3943
const authTests = [
4044
{
@@ -292,11 +296,21 @@ describe('bucket policy authorization', () => {
292296
it('should allow access to public user if principal is set to "*"',
293297
done => {
294298
const allowed = isBucketAuthorized(bucket, bucAction,
295-
constants.publicId, null, log);
299+
constants.publicId, publicUserAuthInfo, log);
296300
assert.equal(allowed, true);
297301
done();
298302
});
299303

304+
it('should deny access to public user if principal is not set to "*"', function itFn(done) {
305+
const newPolicy = this.test.basePolicy;
306+
newPolicy.Statement[0].Principal = { AWS: authInfo.getArn() };
307+
bucket.setBucketPolicy(newPolicy);
308+
const allowed = isBucketAuthorized(bucket, bucAction,
309+
constants.publicId, publicUserAuthInfo, log);
310+
assert.equal(allowed, false);
311+
done();
312+
});
313+
300314
authTests.forEach(t => {
301315
it(`${t.name}bucket owner`, function itFn(done) {
302316
const newPolicy = this.test.basePolicy;
@@ -376,7 +390,7 @@ describe('bucket policy authorization', () => {
376390
it('should allow access to public user if principal is set to "*"',
377391
done => {
378392
const allowed = isObjAuthorized(bucket, object, objAction,
379-
constants.publicId, null, log);
393+
constants.publicId, publicUserAuthInfo, log);
380394
assert.equal(allowed, true);
381395
done();
382396
});

0 commit comments

Comments
 (0)