Skip to content

Commit 873a00c

Browse files
shiradyshirady
authored
Merge pull request #8680 from shirady/nsfs-nc-bucket-policy-issue
NC | NSFS | Fix Issue DFBUGS-1307 | Bucket Policy With Principal as ID
2 parents dacfe8e + c096f08 commit 873a00c

File tree

3 files changed

+305
-15
lines changed

3 files changed

+305
-15
lines changed

‎src/endpoint/s3/s3_rest.js

+11-7
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

+7-7
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

+287-1
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)