Compare commits

...

6 Commits

Author SHA1 Message Date
Dora Korpar 29b0791a16 [squash] fix tests 3 2020-02-19 15:06:42 -08:00
Dora Korpar 8a01af66b8 [squash] fix tests 2 2020-02-19 14:14:18 -08:00
Dora Korpar da7b6799c3 [squash] fix tests 1 2020-02-19 13:56:11 -08:00
Dora Korpar 1b19798ddf [squash] fix negation 2020-02-18 15:44:24 -08:00
Dora Korpar be09313051 [squash] fix isObjAuthorized func 2020-02-18 14:16:01 -08:00
Dora Korpar 29912f79d8 bf: S3C 2597 user bucket policy 2020-02-18 10:34:37 -08:00
13 changed files with 111 additions and 72 deletions

View File

@ -229,11 +229,17 @@ function checkBucketPolicy(policy, requestType, canonicalID, arn, log) {
return permission;
}
function isBucketAuthorized(bucket, requestType, canonicalID, arn, log) {
function isBucketAuthorized(bucket, requestType, canonicalID, authInfo, log) {
// Check to see if user is authorized to perform a
// particular action on bucket based on ACLs.
// TODO: Add IAM checks
if (bucket.getOwner() === canonicalID) {
let requesterIsNotUser = true;
let arn = null;
if (authInfo) {
requesterIsNotUser = !authInfo.isRequesterAnIAMUser();
arn = authInfo.getArn();
}
if ((bucket.getOwner() === canonicalID) && requesterIsNotUser) {
return true;
}
const aclPermission = checkBucketAcls(bucket, requestType, canonicalID);
@ -252,19 +258,28 @@ function isBucketAuthorized(bucket, requestType, canonicalID, arn, log) {
return (aclPermission || (bucketPolicyPermission === 'allow'));
}
function isObjAuthorized(bucket, objectMD, requestType, canonicalID, arn, log) {
function isObjAuthorized(bucket, objectMD, requestType, canonicalID, authInfo,
log) {
const bucketOwner = bucket.getOwner();
if (!objectMD) {
return false;
}
if (objectMD['owner-id'] === canonicalID) {
let requesterIsNotUser = true;
let arn = null;
if (authInfo) {
requesterIsNotUser = !authInfo.isRequesterAnIAMUser();
arn = authInfo.getArn();
}
if (objectMD['owner-id'] === canonicalID && requesterIsNotUser) {
return true;
}
// account is authorized if:
// - requesttype is included in bucketOwnerActions and
// - account is the bucket owner
// - requester is account, not user
if (constants.bucketOwnerActions.includes(requestType)
&& bucketOwner === canonicalID) {
&& bucketOwner === canonicalID
&& requesterIsNotUser) {
return true;
}
const aclPermission = checkObjectAcls(bucket, objectMD, requestType,

View File

@ -20,7 +20,6 @@ const requestType = 'bucketDeleteCors';
function bucketDeleteCors(authInfo, request, log, callback) {
const bucketName = request.bucketName;
const canonicalID = authInfo.getCanonicalID();
const requestArn = authInfo.getArn();
return metadata.getBucket(bucketName, log, (err, bucket) => {
const corsHeaders = collectCorsHeaders(request.headers.origin,
@ -34,7 +33,7 @@ function bucketDeleteCors(authInfo, request, log, callback) {
}
log.trace('found bucket in metadata');
if (!isBucketAuthorized(bucket, requestType, canonicalID, requestArn,
if (!isBucketAuthorized(bucket, requestType, canonicalID, authInfo,
log)) {
log.debug('access denied for user on bucket', {
requestType,

View File

@ -12,7 +12,6 @@ const requestType = 'bucketDeleteWebsite';
function bucketDeleteWebsite(authInfo, request, log, callback) {
const bucketName = request.bucketName;
const canonicalID = authInfo.getCanonicalID();
const requestArn = authInfo.getArn();
return metadata.getBucket(bucketName, log, (err, bucket) => {
const corsHeaders = collectCorsHeaders(request.headers.origin,
@ -26,7 +25,7 @@ function bucketDeleteWebsite(authInfo, request, log, callback) {
}
log.trace('found bucket in metadata');
if (!isBucketAuthorized(bucket, requestType, canonicalID, requestArn,
if (!isBucketAuthorized(bucket, requestType, canonicalID, authInfo,
log)) {
log.debug('access denied for user on bucket', {
requestType,

View File

@ -21,7 +21,6 @@ const requestType = 'bucketGetCors';
function bucketGetCors(authInfo, request, log, callback) {
const bucketName = request.bucketName;
const canonicalID = authInfo.getCanonicalID();
const requestArn = authInfo.getArn();
metadata.getBucket(bucketName, log, (err, bucket) => {
if (err) {
@ -35,7 +34,7 @@ function bucketGetCors(authInfo, request, log, callback) {
const corsHeaders = collectCorsHeaders(request.headers.origin,
request.method, bucket);
if (!isBucketAuthorized(bucket, requestType, canonicalID, requestArn,
if (!isBucketAuthorized(bucket, requestType, canonicalID, authInfo,
log)) {
log.debug('access denied for user on bucket', {
requestType,

View File

@ -22,7 +22,6 @@ const requestType = 'bucketGetLocation';
function bucketGetLocation(authInfo, request, log, callback) {
const bucketName = request.bucketName;
const canonicalID = authInfo.getCanonicalID();
const requestArn = authInfo.getArn();
return metadata.getBucket(bucketName, log, (err, bucket) => {
if (err) {
@ -37,7 +36,7 @@ function bucketGetLocation(authInfo, request, log, callback) {
const corsHeaders = collectCorsHeaders(request.headers.origin,
request.method, bucket);
if (!isBucketAuthorized(bucket, requestType, canonicalID, requestArn,
if (!isBucketAuthorized(bucket, requestType, canonicalID, authInfo,
log)) {
log.debug('access denied for account on bucket', {
requestType,

View File

@ -21,7 +21,6 @@ const requestType = 'bucketGetWebsite';
function bucketGetWebsite(authInfo, request, log, callback) {
const bucketName = request.bucketName;
const canonicalID = authInfo.getCanonicalID();
const requestArn = authInfo.getArn();
metadata.getBucket(bucketName, log, (err, bucket) => {
if (err) {
@ -35,7 +34,7 @@ function bucketGetWebsite(authInfo, request, log, callback) {
const corsHeaders = collectCorsHeaders(request.headers.origin,
request.method, bucket);
if (!isBucketAuthorized(bucket, requestType, canonicalID, requestArn,
if (!isBucketAuthorized(bucket, requestType, canonicalID, authInfo,
log)) {
log.debug('access denied for user on bucket', {
requestType,

View File

@ -24,7 +24,6 @@ function bucketPutCors(authInfo, request, log, callback) {
log.debug('processing request', { method: 'bucketPutCors' });
const bucketName = request.bucketName;
const canonicalID = authInfo.getCanonicalID();
const requestArn = authInfo.getArn();
if (!request.post) {
log.debug('CORS xml body is missing',
@ -67,8 +66,8 @@ function bucketPutCors(authInfo, request, log, callback) {
});
},
function validateBucketAuthorization(bucket, rules, corsHeaders, next) {
if (!isBucketAuthorized(bucket, requestType, canonicalID,
requestArn, log)) {
if (!isBucketAuthorized(bucket, requestType, canonicalID, authInfo,
log)) {
log.debug('access denied for account on bucket', {
requestType,
});

View File

@ -23,7 +23,6 @@ function bucketPutWebsite(authInfo, request, log, callback) {
log.debug('processing request', { method: 'bucketPutWebsite' });
const bucketName = request.bucketName;
const canonicalID = authInfo.getCanonicalID();
const requestArn = authInfo.getArn();
if (!request.post) {
return callback(errors.MissingRequestBodyError);
@ -47,8 +46,8 @@ function bucketPutWebsite(authInfo, request, log, callback) {
});
},
function validateBucketAuthorization(bucket, config, next) {
if (!isBucketAuthorized(bucket, requestType, canonicalID,
requestArn, log)) {
if (!isBucketAuthorized(bucket, requestType, canonicalID, authInfo,
log)) {
log.debug('access denied for user on bucket', {
requestType,
method: 'bucketPutWebsite',

View File

@ -317,7 +317,6 @@ function multiObjectDelete(authInfo, request, log, callback) {
const bucketName = request.bucketName;
const canonicalID = authInfo.getCanonicalID();
const requestArn = authInfo.getArn();
return async.waterfall([
function parseXML(next) {
@ -449,8 +448,8 @@ function multiObjectDelete(authInfo, request, log, callback) {
return next(null, quietSetting, errorResults, inPlay,
bucketMD);
}
if (!isBucketAuthorized(bucketMD, 'objectDelete',
canonicalID, requestArn, log)) {
if (!isBucketAuthorized(bucketMD, 'objectDelete', canonicalID,
authInfo, log)) {
log.trace("access denied due to bucket acl's");
// if access denied at the bucket level, no access for
// any of the objects so all results will be error results

View File

@ -83,7 +83,6 @@ function objectPutPart(authInfo, request, streamingV4Params, log,
log.trace('owner canonicalid to send to data', {
canonicalID: authInfo.getCanonicalID,
});
const requestArn = authInfo.getArn();
// Note that keys in the query object retain their case, so
// `request.query.uploadId` must be called with that exact capitalization.
const uploadId = request.query.uploadId;
@ -111,8 +110,8 @@ function objectPutPart(authInfo, request, streamingV4Params, log,
// For validating the request at the destinationBucket level the
// `requestType` is the general 'objectPut'.
const requestType = 'objectPut';
if (!isBucketAuthorized(destinationBucket, requestType,
canonicalID, requestArn, log)) {
if (!isBucketAuthorized(destinationBucket, requestType, canonicalID,
authInfo, log)) {
log.debug('access denied for user on bucket', { requestType });
return next(errors.AccessDenied, destinationBucket);
}

View File

@ -176,7 +176,6 @@ function metadataGetObject(bucketName, objectKey, versionId, log, cb) {
function metadataValidateBucketAndObj(params, log, callback) {
const { authInfo, bucketName, objectKey, versionId, requestType } = params;
const canonicalID = authInfo.getCanonicalID();
const requestArn = authInfo.getArn();
async.waterfall([
function getBucketAndObjectMD(next) {
return metadataGetBucketAndObject(requestType, bucketName,
@ -184,7 +183,7 @@ function metadataValidateBucketAndObj(params, log, callback) {
},
function checkBucketAuth(bucket, objMD, next) {
if (!isBucketAuthorized(bucket, requestType, canonicalID,
requestArn, log)) {
authInfo, log)) {
log.debug('access denied for user on bucket', { requestType });
return next(errors.AccessDenied, bucket);
}
@ -202,7 +201,7 @@ function metadataValidateBucketAndObj(params, log, callback) {
return next(null, bucket);
}
if (!isObjAuthorized(bucket, objMD, requestType, canonicalID,
requestArn, log)) {
authInfo, log)) {
log.debug('access denied for user on object', { requestType });
return next(errors.AccessDenied, bucket);
}
@ -256,13 +255,12 @@ function metadataGetBucket(requestType, bucketName, log, cb) {
function metadataValidateBucket(params, log, callback) {
const { authInfo, bucketName, requestType } = params;
const canonicalID = authInfo.getCanonicalID();
const requestArn = authInfo.getArn();
return metadataGetBucket(requestType, bucketName, log, (err, bucket) => {
if (err) {
return callback(err);
}
// still return bucket for cors headers
if (!isBucketAuthorized(bucket, requestType, canonicalID, requestArn,
if (!isBucketAuthorized(bucket, requestType, canonicalID, authInfo,
log)) {
log.debug('access denied for user on bucket', { requestType });
return callback(errors.AccessDenied, bucket);

View File

@ -3,20 +3,23 @@ const { BucketInfo, BucketPolicy } = require('arsenal').models;
const constants = require('../../../constants');
const { isBucketAuthorized, isObjAuthorized, validatePolicyResource }
= require('../../../lib/api/apiUtils/authorization/permissionChecks');
const { DummyRequestLogger } = require('../helpers');
const { DummyRequestLogger, makeAuthInfo } = require('../helpers');
const bucketOwnerCanonicalId = 'bucketOwnerCanonicalId';
const accessKey = 'accessKey1';
const altAccessKey = 'accessKey2';
const authInfo = makeAuthInfo(accessKey);
const userAuthInfo = makeAuthInfo(accessKey, 'user');
const altUserAuthInfo = makeAuthInfo(accessKey, 'other');
const altAcctAuthInfo = makeAuthInfo(altAccessKey);
const altAcctUserAuthInfo = makeAuthInfo(altAccessKey, 'altUser');
const bucketOwnerCanonicalId = authInfo.getCanonicalID();
const objectOwnerCanonicalId = userAuthInfo.getCanonicalID();
const canonicalIdToVet = altAcctAuthInfo.getCanonicalID();
const creationDate = new Date().toJSON();
const bucket = new BucketInfo('policyBucketAuthTester', bucketOwnerCanonicalId,
'iAmTheOwnerDisplayName', creationDate);
const objectOwnerCanonicalId = 'objectOwnerCanonicalId';
authInfo.getAccountDisplayName(), creationDate);
const object = { 'owner-id': objectOwnerCanonicalId };
const canonicalIdToVet = 'canonicalIdToVet';
const userArn = 'arn:aws:iam::123456789012:user/user';
const otherUserArn = 'arn:aws:iam::123456789012:user/other';
const otherAccountUserArn = 'arn:aws:iam:987654321098:user/other';
const accountArn = 'arn:aws:iam::123456789012:root';
const accountId = '123456789012';
const accountId = authInfo.getShortid();
const bucAction = 'bucketPut';
const objAction = 'objectPut';
const basePolicyObj = {
@ -35,7 +38,9 @@ const authTests = [
{
name: 'should allow access if canonical user principal matches non-',
bucketId: canonicalIdToVet,
bucketAuthInfo: altAcctAuthInfo,
objectId: canonicalIdToVet,
objectAuthInfo: altAcctAuthInfo,
keyToChange: 'Principal',
bucketValue: { CanonicalUser: [canonicalIdToVet] },
objectValue: { CanonicalUser: [canonicalIdToVet] },
@ -43,26 +48,32 @@ const authTests = [
},
{
name: 'should allow access if user arn principal matches non-',
bucketId: userArn,
objectId: userArn,
bucketId: objectOwnerCanonicalId,
bucketAuthInfo: userAuthInfo,
objectId: objectOwnerCanonicalId,
objectAuthInfo: userAuthInfo,
keyToChange: 'Principal',
bucketValue: { AWS: userArn },
objectValue: { AWS: userArn },
bucketValue: { AWS: userAuthInfo.getArn() },
objectValue: { AWS: userAuthInfo.getArn() },
expected: true,
},
{
name: 'should allow access if account arn principal matches non-',
bucketId: accountArn,
objectId: accountArn,
bucketId: bucketOwnerCanonicalId,
bucketAuthInfo: authInfo,
objectId: bucketOwnerCanonicalId,
objectAuthInfo: authInfo,
keyToChange: 'Principal',
bucketValue: { AWS: accountArn },
objectValue: { AWS: accountArn },
bucketValue: { AWS: authInfo.getArn() },
objectValue: { AWS: authInfo.getArn() },
expected: true,
},
{
name: 'should allow access if account id principal matches non-',
bucketId: accountId,
bucketAuthInfo: authInfo,
objectId: accountId,
objectAuthInfo: authInfo,
keyToChange: 'Principal',
bucketValue: { AWS: accountId },
objectValue: { AWS: accountId },
@ -71,8 +82,10 @@ const authTests = [
{
name: 'should allow access if account id principal is contained in ' +
'user arn of non-',
bucketId: userArn,
objectId: userArn,
bucketId: objectOwnerCanonicalId,
bucketAuthInfo: userAuthInfo.getArn(),
objectId: objectOwnerCanonicalId,
objectAuthInfo: userAuthInfo.getArn(),
keyToChange: 'Principal',
bucketValue: { AWS: accountId },
objectValue: { AWS: accountId },
@ -81,8 +94,10 @@ const authTests = [
{
name: 'should allow access if account id principal is contained in ' +
'account arn of non-',
bucketId: accountArn,
objectId: accountArn,
bucketId: bucketOwnerCanonicalId,
bucketAuthInfo: authInfo,
objectId: bucketOwnerCanonicalId,
objectAuthInfo: authInfo,
keyToChange: 'Principal',
bucketValue: { AWS: accountId },
objectValue: { AWS: accountId },
@ -91,37 +106,45 @@ const authTests = [
{
name: 'should allow access if account arn principal is contained in ' +
'user arn of non-',
bucketId: userArn,
objectId: userArn,
bucketId: objectOwnerCanonicalId,
bucketAuthInfo: userAuthInfo,
objectId: objectOwnerCanonicalId,
objectAuthInfo: userAuthInfo,
keyToChange: 'Principal',
bucketValue: { AWS: accountArn },
objectValue: { AWS: accountArn },
bucketValue: { AWS: authInfo.getArn() },
objectValue: { AWS: authInfo.getArn() },
expected: true,
},
{
name: 'should deny access if account arn principal doesn\'t match ' +
'user arn of non-',
bucketId: otherAccountUserArn,
objectId: otherAccountUserArn,
bucketId: altAcctUserAuthInfo.getCanonicalID(),
bucketAuthInfo: altAcctUserAuthInfo,
objectId: altAcctUserAuthInfo.getCanonicalID(),
objectAuthInfo: altAcctUserAuthInfo,
keyToChange: 'Principal',
bucketValue: { AWS: accountArn },
objectValue: { AWS: accountArn },
bucketValue: { AWS: authInfo.getArn() },
objectValue: { AWS: authInfo.getArn() },
expected: false,
},
{
name: 'should deny access if user arn principal doesn\'t match ' +
'user arn of non-',
bucketId: userArn,
objectId: userArn,
bucketId: objectOwnerCanonicalId,
bucketAuthInfo: userAuthInfo,
objectId: objectOwnerCanonicalId,
objectAuthInfo: userAuthInfo,
keyToChange: 'Principal',
bucketValue: { AWS: otherUserArn },
objectValue: { AWS: otherUserArn },
bucketValue: { AWS: altUserAuthInfo.getArn() },
objectValue: { AWS: altUserAuthInfo.getArn() },
expected: false,
},
{
name: 'should deny access if principal doesn\'t match non-',
bucketId: canonicalIdToVet,
bucketAuthInfo: altAcctAuthInfo,
objectId: canonicalIdToVet,
objectAuthInfo: altAcctAuthInfo,
keyToChange: 'Principal',
bucketValue: { CanonicalUser: [bucketOwnerCanonicalId] },
objectValue: { CanonicalUser: [objectOwnerCanonicalId] },
@ -131,7 +154,9 @@ const authTests = [
name: 'should allow access if principal and action match policy for ' +
'non-',
bucketId: canonicalIdToVet,
bucketAuthInfo: altAcctAuthInfo,
objectId: canonicalIdToVet,
objectAuthInfo: altAcctAuthInfo,
keyToChange: 'Action',
bucketValue: ['s3:CreateBucket'],
objectValue: ['s3:PutObject'],
@ -141,7 +166,9 @@ const authTests = [
name: 'should deny access if principal matches but action does not ' +
'match policy for non-',
bucketId: canonicalIdToVet,
bucketAuthInfo: altAcctAuthInfo,
objectId: canonicalIdToVet,
objectAuthInfo: altAcctAuthInfo,
keyToChange: 'Action',
bucketValue: ['s3:GetBucketLocation'],
objectValue: ['s3:GetObject'],
@ -150,7 +177,9 @@ const authTests = [
{
name: 'should allow access even if bucket policy denies for ',
bucketId: bucketOwnerCanonicalId,
bucketAuthInfo: authInfo,
objectId: objectOwnerCanonicalId,
objectAuthInfo: userAuthInfo,
keyToChange: 'Effect',
bucketValue: 'Deny',
objectValue: 'Deny',
@ -187,7 +216,7 @@ const resourceTests = [
},
];
describe('bucket policy authorization', () => {
describe.only('bucket policy authorization', () => {
describe('isBucketAuthorized with no policy set', () => {
it('should allow access to bucket owner', done => {
const allowed = isBucketAuthorized(bucket, 'bucketPut',
@ -234,7 +263,7 @@ describe('bucket policy authorization', () => {
newPolicy.Statement[0][t.keyToChange] = t.bucketValue;
bucket.setBucketPolicy(newPolicy);
const allowed = isBucketAuthorized(bucket, bucAction,
t.bucketId, t.bucketId, log);
t.bucketId, t.bucketAuthInfo, log);
assert.equal(allowed, t.expected);
done();
});
@ -310,7 +339,7 @@ describe('bucket policy authorization', () => {
newPolicy.Statement[0][t.keyToChange] = t.objectValue;
bucket.setBucketPolicy(newPolicy);
const allowed = isObjAuthorized(bucket, object, objAction,
t.objectId, t.objectId, log);
t.objectId, t.objectAuthInfo, log);
assert.equal(allowed, t.expected);
done();
});

View File

@ -63,7 +63,7 @@ function timeDiff(startTime) {
return milliseconds;
}
function makeAuthInfo(accessKey) {
function makeAuthInfo(accessKey, userName) {
const canIdMap = {
accessKey1: '79a59df900b949e55d96a1e698fbacedfd6e09d98eacf8f8d5218e7'
+ 'cd47ef2be',
@ -73,12 +73,18 @@ function makeAuthInfo(accessKey) {
};
canIdMap[constants.publicId] = constants.publicId;
return new AuthInfo({
const params = {
canonicalID: canIdMap[accessKey] || canIdMap.default,
shortid: 'shortid',
email: `${accessKey}@l.com`,
accountDisplayName: `${accessKey}displayName`,
});
arn: 'arn:aws:iam::shortid:root',
};
if (userName) {
params.IAMdisplayName = `${accessKey}-${userName}-UserDisplayName`;
params.arn = `arn:aws:iam::shortid:user/${userName}`;
}
return new AuthInfo(params);
}
class WebsiteConfig {