Skip to content

Commit c096f08

Browse files
committed
NC | NSFS | Fix Issue DFBUGS-1307 | Bucket Policy With Principal as ID
1. Change the condition in rest_s3 and bucketspace_fs to check the permission by the principal on account name when the previous check was only not DENY, so in case someone puts several statements using both account ID and account name, we won't skip it after the checks on the account ID. 2. Add logs in level 3 to help investigate bucket policy issues. Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>
1 parent dacfe8e commit c096f08

File tree

3 files changed

+305
-15
lines changed

3 files changed

+305
-15
lines changed

src/endpoint/s3/s3_rest.js

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -277,23 +277,27 @@ async function authorize_request_policy(req) {
277277
if (is_owner || is_iam_account_and_same_root_account_owner) return;
278278
throw new S3Error(S3Error.AccessDenied);
279279
}
280-
let permission;
280+
// in case we have bucket policy
281+
let permission_by_id;
282+
let permission_by_name;
281283
// In NC, we allow principal to be:
282284
// 1. account name (for backwards compatibility)
283285
// 2. account id
284286
// we start the permission check on account identifier intentionally
285287
if (account_identifier_id) {
286-
permission = await s3_bucket_policy_utils.has_bucket_policy_permission(
288+
permission_by_id = await s3_bucket_policy_utils.has_bucket_policy_permission(
287289
s3_policy, account_identifier_id, method, arn_path, req);
290+
dbg.log3('authorize_request_policy: permission_by_id', permission_by_id);
288291
}
292+
if (permission_by_id === "DENY") throw new S3Error(S3Error.AccessDenied);
289293

290-
if ((!account_identifier_id || permission === "IMPLICIT_DENY") && account.owner === undefined) {
291-
permission = await s3_bucket_policy_utils.has_bucket_policy_permission(
294+
if ((!account_identifier_id || permission_by_id !== "DENY") && account.owner === undefined) {
295+
permission_by_name = await s3_bucket_policy_utils.has_bucket_policy_permission(
292296
s3_policy, account_identifier_name, method, arn_path, req);
297+
dbg.log3('authorize_request_policy: permission_by_name', permission_by_name);
293298
}
294-
295-
if (permission === "DENY") throw new S3Error(S3Error.AccessDenied);
296-
if (permission === "ALLOW" || is_owner) return;
299+
if (permission_by_name === "DENY") throw new S3Error(S3Error.AccessDenied);
300+
if ((permission_by_id === "ALLOW" || permission_by_name === "ALLOW") || is_owner) return;
297301

298302
throw new S3Error(S3Error.AccessDenied);
299303
}

src/sdk/bucketspace_fs.js

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -805,28 +805,28 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
805805
throw new Error('has_bucket_action_permission: action is required');
806806
}
807807

808-
let permission;
809-
permission = await bucket_policy_utils.has_bucket_policy_permission(
808+
let permission_by_name;
809+
const permission_by_id = await bucket_policy_utils.has_bucket_policy_permission(
810810
bucket_policy,
811811
account._id,
812812
action,
813813
`arn:aws:s3:::${bucket.name.unwrap()}${bucket_path}`,
814814
undefined
815815
);
816+
if (permission_by_id === "DENY") return false;
816817
// we (currently) allow account identified to be both id and name,
817818
// so if by-id failed, try also name
818-
if (account.owner === undefined && permission === 'IMPLICIT_DENY') {
819-
permission = await bucket_policy_utils.has_bucket_policy_permission(
819+
if (account.owner === undefined) {
820+
permission_by_name = await bucket_policy_utils.has_bucket_policy_permission(
820821
bucket_policy,
821822
account.name.unwrap(),
822823
action,
823824
`arn:aws:s3:::${bucket.name.unwrap()}${bucket_path}`,
824825
undefined
825826
);
826827
}
827-
828-
if (permission === 'DENY') return false;
829-
return is_owner || permission === 'ALLOW';
828+
if (permission_by_name === 'DENY') return false;
829+
return is_owner || (permission_by_id === 'ALLOW' || permission_by_name === 'ALLOW');
830830
}
831831

832832
async validate_fs_bucket_access(bucket, object_sdk) {

src/test/unit_tests/test_s3_bucket_policy.js

Lines changed: 287 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ const { rpc_client, EMAIL, POOL_LIST, anon_rpc_client } = coretest;
1010
const MDStore = require('../../server/object_services/md_store').MDStore;
1111
coretest.setup({ pools_to_create: process.env.NC_CORETEST ? undefined : [POOL_LIST[1]] });
1212
const path = require('path');
13+
const _ = require('lodash');
1314
const fs_utils = require('../../util/fs_utils');
1415

1516
const { S3 } = require('@aws-sdk/client-s3');
@@ -33,6 +34,7 @@ async function assert_throws_async(promise, expected_message = 'Access Denied')
3334
const BKT = 'test2-bucket-policy-ops';
3435
const BKT_B = 'test2-bucket-policy-ops-1';
3536
const BKT_C = 'test2-bucket-policy-ops-2';
37+
const BKT_D = 'test2-bucket-policy-ops-3';
3638
const KEY = 'file1.txt';
3739
const user_a = 'alice';
3840
const user_b = 'bob';
@@ -134,6 +136,7 @@ async function setup() {
134136
s3_owner = new S3(s3_creds);
135137
await s3_owner.createBucket({ Bucket: BKT });
136138
await s3_owner.createBucket({ Bucket: BKT_C });
139+
await s3_owner.createBucket({ Bucket: BKT_D });
137140
s3_anon = new S3({
138141
...s3_creds,
139142
credentials: {
@@ -147,7 +150,7 @@ async function setup() {
147150
});
148151
}
149152

150-
/*eslint max-lines-per-function: ["error", 1600]*/
153+
/*eslint max-lines-per-function: ["error", 2000]*/
151154
mocha.describe('s3_bucket_policy', function() {
152155
mocha.before(setup);
153156
mocha.it('should fail setting bucket policy when user doesn\'t exist', async function() {
@@ -335,6 +338,289 @@ mocha.describe('s3_bucket_policy', function() {
335338
}));
336339
});
337340

341+
mocha.describe('s3_bucket_policy with more complex policies (conflict statements)', function() {
342+
mocha.after(async function() {
343+
await s3_owner.deleteBucketPolicy({
344+
Bucket: BKT_D,
345+
});
346+
});
347+
348+
const allow_all_principals_all_s3_actions_statement = {
349+
Sid: `Allow all s3 actions on bucket ${BKT_D} to all principals`,
350+
Effect: 'Allow',
351+
Principal: { AWS: "*" },
352+
Action: ['s3:*'],
353+
Resource: [`arn:aws:s3:::${BKT_D}`, `arn:aws:s3:::${BKT_D}/*`]
354+
};
355+
356+
const deny_all_principals_get_object_action_statement = {
357+
Sid: `Deny all GetObject on bucket ${BKT_D} to all principals`,
358+
Effect: 'Deny',
359+
Principal: { AWS: "*" },
360+
Action: 's3:GetObject',
361+
Resource: [`arn:aws:s3:::${BKT_D}/*`]
362+
};
363+
364+
function get_deny_account_by_id_all_s3_actions_statement(_id) {
365+
return {
366+
Sid: `Do not allow user ${_id} any s3 action`,
367+
Effect: 'Deny',
368+
Principal: { AWS: [_id] },
369+
Action: ['s3:*'],
370+
Resource: [`arn:aws:s3:::${BKT_D}/*`]
371+
};
372+
}
373+
374+
const deny_account_by_name_all_s3_actions_statement = {
375+
Sid: `Do not allow user ${user_a} any s3 action`,
376+
Effect: 'Deny',
377+
Principal: { AWS: [user_a] },
378+
Action: ['s3:*'],
379+
Resource: [`arn:aws:s3:::${BKT_D}/*`]
380+
};
381+
382+
mocha.it('should not allow principal get object bucket policy with 2 statements: ' +
383+
'(1) DENY principal by account ID (2) ALLOW all principals as *', async function() {
384+
// in NC we allow principal to be also IDs
385+
if (!is_nc_coretest) this.skip(); // eslint-disable-line no-invalid-this
386+
const deny_account_by_id_all_s3_actions_statement =
387+
get_deny_account_by_id_all_s3_actions_statement(user_a_account_details._id);
388+
const policy = {
389+
Statement: [
390+
allow_all_principals_all_s3_actions_statement,
391+
deny_account_by_id_all_s3_actions_statement
392+
]
393+
};
394+
await s3_owner.putBucketPolicy({
395+
Bucket: BKT_D,
396+
Policy: JSON.stringify(policy)
397+
});
398+
// prepare - put the object to get
399+
const key2 = 'file2.txt';
400+
const res_put_object = await s3_owner.putObject({
401+
Body: BODY,
402+
Bucket: BKT_D,
403+
Key: key2
404+
});
405+
assert.equal(res_put_object.$metadata.httpStatusCode, 200);
406+
// should fail - user a has a DENY statement
407+
await assert_throws_async(s3_a.getObject({
408+
Body: BODY,
409+
Bucket: BKT_D,
410+
Key: key2
411+
}));
412+
// should fail - user b does not have a DENY statement (uses the general ALLOW statement)
413+
const res_get_object = await s3_b.getObject({
414+
Body: BODY,
415+
Bucket: BKT_D,
416+
Key: key2
417+
});
418+
assert.equal(res_get_object.$metadata.httpStatusCode, 200);
419+
});
420+
421+
mocha.it('should not allow principal get object bucket policy with 2 statements: ' +
422+
'(1) DENY principal by account name (2) ALLOW all principals as *', async function() {
423+
const policy = {
424+
Statement: [
425+
allow_all_principals_all_s3_actions_statement,
426+
deny_account_by_name_all_s3_actions_statement
427+
]
428+
};
429+
await s3_owner.putBucketPolicy({
430+
Bucket: BKT_D,
431+
Policy: JSON.stringify(policy)
432+
});
433+
// prepare - put the object to get
434+
const key3 = 'file3.txt';
435+
const res_put_object = await s3_owner.putObject({
436+
Body: BODY,
437+
Bucket: BKT_D,
438+
Key: key3
439+
});
440+
assert.equal(res_put_object.$metadata.httpStatusCode, 200);
441+
// should fail - user a has a DENY statement
442+
await assert_throws_async(s3_a.getObject({
443+
Body: BODY,
444+
Bucket: BKT_D,
445+
Key: key3
446+
}));
447+
// should fail - user b does not have a DENY statement (uses the general ALLOW statement)
448+
const res_get_object = await s3_b.getObject({
449+
Body: BODY,
450+
Bucket: BKT_D,
451+
Key: key3
452+
});
453+
assert.equal(res_get_object.$metadata.httpStatusCode, 200);
454+
});
455+
456+
mocha.it('should not allow principal get object bucket policy with 2 statements: ' +
457+
'(1) DENY principal by account ID (2) ALLOW by account name', async function() {
458+
// in NC we allow principal to be also IDs
459+
if (!is_nc_coretest) this.skip(); // eslint-disable-line no-invalid-this
460+
const deny_account_by_id_all_s3_actions_statement =
461+
get_deny_account_by_id_all_s3_actions_statement(user_a_account_details._id);
462+
const allow_account_by_name_all_s3_actions_statement = _.cloneDeep(deny_account_by_name_all_s3_actions_statement);
463+
allow_account_by_name_all_s3_actions_statement.Effect = 'Allow';
464+
allow_account_by_name_all_s3_actions_statement.Sid = `Allow user ${user_a} any s3 action`;
465+
const policy = {
466+
Statement: [
467+
deny_account_by_id_all_s3_actions_statement,
468+
allow_account_by_name_all_s3_actions_statement
469+
]
470+
};
471+
await s3_owner.putBucketPolicy({
472+
Bucket: BKT_D,
473+
Policy: JSON.stringify(policy)
474+
});
475+
// prepare - put the object to get
476+
const key4 = 'file4.txt';
477+
const res_put_object = await s3_owner.putObject({
478+
Body: BODY,
479+
Bucket: BKT_D,
480+
Key: key4
481+
});
482+
assert.equal(res_put_object.$metadata.httpStatusCode, 200);
483+
// should fail - user a has a DENY statement
484+
await assert_throws_async(s3_a.getObject({
485+
Body: BODY,
486+
Bucket: BKT_D,
487+
Key: key4
488+
}));
489+
});
490+
491+
mocha.it('should not allow principal get object bucket policy with 2 statements: ' +
492+
'(1) DENY principal by account name (2) ALLOW by account ID', async function() {
493+
// in NC we allow principal to be also IDs
494+
if (!is_nc_coretest) this.skip(); // eslint-disable-line no-invalid-this
495+
const deny_account_by_id_all_s3_actions_statement =
496+
get_deny_account_by_id_all_s3_actions_statement(user_a_account_details._id);
497+
const allow_account_by_id_all_s3_actions_statement = _.cloneDeep(deny_account_by_id_all_s3_actions_statement);
498+
allow_account_by_id_all_s3_actions_statement.Effect = 'Allow';
499+
allow_account_by_id_all_s3_actions_statement.Sid = `Allow user ${user_a_account_details._id} any s3 action`;
500+
const policy = {
501+
Statement: [
502+
deny_account_by_name_all_s3_actions_statement,
503+
allow_account_by_id_all_s3_actions_statement
504+
]
505+
};
506+
await s3_owner.putBucketPolicy({
507+
Bucket: BKT_D,
508+
Policy: JSON.stringify(policy)
509+
});
510+
// prepare - put the object to get
511+
const key5 = 'file5.txt';
512+
const res_put_object = await s3_owner.putObject({
513+
Body: BODY,
514+
Bucket: BKT_D,
515+
Key: key5
516+
});
517+
assert.equal(res_put_object.$metadata.httpStatusCode, 200);
518+
// should fail - user a has a DENY statement
519+
await assert_throws_async(s3_a.getObject({
520+
Body: BODY,
521+
Bucket: BKT_D,
522+
Key: key5
523+
}));
524+
});
525+
526+
mocha.it('should not allow principal get object bucket policy with 2 statements: ' +
527+
'(1) ALLOW principal by account name (2) DENY all principals as * (specific action only)', async function() {
528+
const allow_account_by_name_all_s3_actions_statement = _.cloneDeep(deny_account_by_name_all_s3_actions_statement);
529+
allow_account_by_name_all_s3_actions_statement.Effect = 'Allow';
530+
allow_account_by_name_all_s3_actions_statement.Sid = `Allow user ${user_a} any s3 action`;
531+
const policy = {
532+
Statement: [
533+
allow_account_by_name_all_s3_actions_statement,
534+
deny_all_principals_get_object_action_statement
535+
]
536+
};
537+
await s3_owner.putBucketPolicy({
538+
Bucket: BKT_D,
539+
Policy: JSON.stringify(policy)
540+
});
541+
// prepare - put the object to get
542+
const key6 = 'file6.txt';
543+
const res_put_object = await s3_owner.putObject({
544+
Body: BODY,
545+
Bucket: BKT_D,
546+
Key: key6
547+
});
548+
assert.equal(res_put_object.$metadata.httpStatusCode, 200);
549+
// should fail - user a has a DENY statement
550+
await assert_throws_async(s3_a.getObject({
551+
Body: BODY,
552+
Bucket: BKT_D,
553+
Key: key6
554+
}));
555+
});
556+
557+
mocha.it('should not allow principal get object bucket policy with 2 statements: ' +
558+
'(1) ALLOW principal by account ID (2) DENY all principals as * (specific action only)', async function() {
559+
// in NC we allow principal to be also IDs
560+
if (!is_nc_coretest) this.skip(); // eslint-disable-line no-invalid-this
561+
const deny_account_by_id_all_s3_actions_statement =
562+
get_deny_account_by_id_all_s3_actions_statement(user_a_account_details._id);
563+
const allow_account_by_id_all_s3_actions_statement = _.cloneDeep(deny_account_by_id_all_s3_actions_statement);
564+
allow_account_by_id_all_s3_actions_statement.Effect = 'Allow';
565+
allow_account_by_id_all_s3_actions_statement.Sid = `Allow user ${user_a_account_details._id} any s3 action`;
566+
const policy = {
567+
Statement: [
568+
allow_account_by_id_all_s3_actions_statement,
569+
deny_all_principals_get_object_action_statement
570+
]
571+
};
572+
await s3_owner.putBucketPolicy({
573+
Bucket: BKT_D,
574+
Policy: JSON.stringify(policy)
575+
});
576+
// prepare - put the object to get
577+
const key7 = 'file7.txt';
578+
const res_put_object = await s3_owner.putObject({
579+
Body: BODY,
580+
Bucket: BKT_D,
581+
Key: key7
582+
});
583+
assert.equal(res_put_object.$metadata.httpStatusCode, 200);
584+
// should fail - user a has a DENY statement
585+
await assert_throws_async(s3_a.getObject({
586+
Body: BODY,
587+
Bucket: BKT_D,
588+
Key: key7
589+
}));
590+
});
591+
592+
mocha.it('should not allow principal get object bucket policy with 2 statements: ' +
593+
'(1) ALLOW principal by account name (2) DENY all principals as * (specific action only)', async function() {
594+
const allow_account_by_name_all_s3_actions_statement = _.cloneDeep(deny_account_by_name_all_s3_actions_statement);
595+
allow_account_by_name_all_s3_actions_statement.Effect = 'Allow';
596+
allow_account_by_name_all_s3_actions_statement.Sid = `Allow user ${user_a} any s3 action`;
597+
const policy = {
598+
Statement: [
599+
allow_account_by_name_all_s3_actions_statement,
600+
deny_all_principals_get_object_action_statement
601+
]
602+
};
603+
await s3_owner.putBucketPolicy({
604+
Bucket: BKT_D,
605+
Policy: JSON.stringify(policy)
606+
});
607+
// prepare - put the object to get
608+
const key6 = 'file6.txt';
609+
const res_put_object = await s3_owner.putObject({
610+
Body: BODY,
611+
Bucket: BKT_D,
612+
Key: key6
613+
});
614+
assert.equal(res_put_object.$metadata.httpStatusCode, 200);
615+
// should fail - user a has a DENY statement
616+
await assert_throws_async(s3_a.getObject({
617+
Body: BODY,
618+
Bucket: BKT_D,
619+
Key: key6
620+
}));
621+
});
622+
});
623+
338624
mocha.it('should be able to set bucket policy when none set', async function() {
339625
const self = this; // eslint-disable-line no-invalid-this
340626
self.timeout(15000);

0 commit comments

Comments
 (0)