Skip to content

Commit 29d705f

Browse files
authored
Merge pull request #8280 from shirady/nsfs-nc-bucket-policy-by-id
NC | NSFS | Bucket Policy With Principal as account ID
2 parents f004fd3 + 4c60720 commit 29d705f

File tree

8 files changed

+347
-33
lines changed

8 files changed

+347
-33
lines changed

docs/NooBaaNonContainerized/S3Ops.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,7 @@ A bucket policy defines which principals can perform actions on the bucket. The
9797
Currently we support a couple of options:
9898
1. Grant anonymous permissions (all principals): either `"Principal": { "AWS": "*" }` or `"Principal": { "*" }`.
9999
2. Principal by account name: `"Principal": { "AWS": [ "<account-name-1>", "<account-name-2>", ... ,"<account-name-n>"] }`
100+
3. Principal by account ID: `"Principal": { "AWS": [ "<account-ID-1>", "<account-ID-2>", ... ,"<account-ID-n>"] }`
100101

101102
### Anonymous Requests Support
102103

src/endpoint/s3/s3_rest.js

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -228,16 +228,17 @@ async function authorize_request_policy(req) {
228228
}
229229

230230
const account = req.object_sdk.requesting_account;
231-
const account_identifier = req.object_sdk.nsfs_config_root ? account.name.unwrap() : account.email.unwrap();
232-
const is_system_owner = Boolean(system_owner) && system_owner.unwrap() === account_identifier;
231+
const account_identifier_name = req.object_sdk.nsfs_config_root ? account.name.unwrap() : account.email.unwrap();
232+
const account_identifier_id = req.object_sdk.nsfs_config_root ? account._id : undefined;
233+
const is_system_owner = Boolean(system_owner) && system_owner.unwrap() === account_identifier_name;
233234

234235
// @TODO: System owner as a construct should be removed - Temporary
235236
if (is_system_owner) return;
236237

237238
const is_owner = (function() {
238239
if (account.bucket_claim_owner && account.bucket_claim_owner.unwrap() === req.params.bucket) return true;
239240
if (owner_account && owner_account.id === account._id) return true;
240-
if (account_identifier === bucket_owner.unwrap()) return true;
241+
if (account_identifier_name === bucket_owner.unwrap()) return true; // TODO: change it to root accounts after we will have the /users structure
241242
return false;
242243
}());
243244

@@ -249,8 +250,21 @@ async function authorize_request_policy(req) {
249250
if (is_owner || is_iam_account_and_same_root_account_owner) return;
250251
throw new S3Error(S3Error.AccessDenied);
251252
}
252-
const permission = await s3_bucket_policy_utils.has_bucket_policy_permission(
253-
s3_policy, account_identifier, method, arn_path, req);
253+
let permission;
254+
// In NC, we allow principal to be:
255+
// 1. account name (for backwards compatibility)
256+
// 2. account id
257+
// we start the permission check on account identifier intentionally
258+
if (account_identifier_id) {
259+
permission = await s3_bucket_policy_utils.has_bucket_policy_permission(
260+
s3_policy, account_identifier_id, method, arn_path, req);
261+
}
262+
263+
if (!account_identifier_id || permission === "IMPLICIT_DENY") {
264+
permission = await s3_bucket_policy_utils.has_bucket_policy_permission(
265+
s3_policy, account_identifier_name, method, arn_path, req);
266+
}
267+
254268
if (permission === "DENY") throw new S3Error(S3Error.AccessDenied);
255269
if (permission === "ALLOW" || is_owner) return;
256270

src/manage_nsfs/manage_nsfs_validations.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -354,7 +354,7 @@ async function validate_bucket_args(config_fs, data, action) {
354354
if (data.s3_policy) {
355355
try {
356356
await bucket_policy_utils.validate_s3_policy(data.s3_policy, data.name,
357-
async principal => config_fs.is_account_exists_by_name(principal, owner_account_data.owner)
357+
async principal => config_fs.is_account_exists_by_principal(principal, { silent_if_missing: true })
358358
);
359359
} catch (err) {
360360
dbg.error('validate_bucket_args invalid bucket policy err:', err);

src/sdk/bucketspace_fs.js

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -217,6 +217,7 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
217217
}
218218
if (!bucket) return;
219219
const bucket_policy_accessible = await this.has_bucket_action_permission(bucket, account, 's3:ListBucket');
220+
dbg.log2(`list_buckets: bucket_name ${bucket_name} bucket_policy_accessible`, bucket_policy_accessible);
220221
if (!bucket_policy_accessible) return;
221222
const fs_accessible = await this.validate_fs_bucket_access(bucket, object_sdk);
222223
if (!fs_accessible) return;
@@ -653,7 +654,7 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
653654
dbg.log2('put_bucket_policy: bucket properties before validate_bucket_schema', bucket_to_validate);
654655
nsfs_schema_utils.validate_bucket_schema(bucket_to_validate);
655656
await bucket_policy_utils.validate_s3_policy(bucket.s3_policy, bucket.name, async principal =>
656-
this.config_fs.get_account_by_name(principal, { silent_if_missing: true }));
657+
this.config_fs.is_account_exists_by_principal(principal, { silent_if_missing: true }));
657658
const update_bucket = JSON.stringify(bucket);
658659
await this.config_fs.update_bucket_config_file(name, update_bucket);
659660
} catch (err) {
@@ -708,10 +709,9 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
708709
// TODO: move the following 3 functions - has_bucket_action_permission(), validate_fs_bucket_access(), _has_access_to_nsfs_dir()
709710
// so they can be re-used
710711
async has_bucket_action_permission(bucket, account, action, bucket_path = "") {
711-
const account_identifier = account.name.unwrap();
712-
dbg.log1('has_bucket_action_permission:', bucket.name.unwrap(), account_identifier, bucket.owner_account);
712+
dbg.log1('has_bucket_action_permission:', bucket.name.unwrap(), account.name.unwrap(), account._id, bucket.bucket_owner.unwrap());
713713

714-
const is_system_owner = Boolean(bucket.system_owner) && bucket.system_owner.unwrap() === account_identifier;
714+
const is_system_owner = Boolean(bucket.system_owner) && bucket.system_owner.unwrap() === account.name.unwrap();
715715

716716
// If the system owner account wants to access the bucket, allow it
717717
if (is_system_owner) return true;
@@ -729,16 +729,28 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
729729
throw new Error('has_bucket_action_permission: action is required');
730730
}
731731

732-
const result = await bucket_policy_utils.has_bucket_policy_permission(
732+
let permission;
733+
permission = await bucket_policy_utils.has_bucket_policy_permission(
733734
bucket_policy,
734-
account_identifier,
735+
account._id,
735736
action,
736737
`arn:aws:s3:::${bucket.name.unwrap()}${bucket_path}`,
737738
undefined
738739
);
740+
// we (currently) allow account identified to be both id and name,
741+
// so if by-id failed, try also name
742+
if (permission === 'IMPLICIT_DENY') {
743+
permission = await bucket_policy_utils.has_bucket_policy_permission(
744+
bucket_policy,
745+
account.name.unwrap(),
746+
action,
747+
`arn:aws:s3:::${bucket.name.unwrap()}${bucket_path}`,
748+
undefined
749+
);
750+
}
739751

740-
if (result === 'DENY') return false;
741-
return is_owner || result === 'ALLOW';
752+
if (permission === 'DENY') return false;
753+
return is_owner || permission === 'ALLOW';
742754
}
743755

744756
async validate_fs_bucket_access(bucket, object_sdk) {

src/sdk/config_fs.js

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ const dbg = require('../util/debug_module')(__filename);
66
const _ = require('lodash');
77
const path = require('path');
88
const os_utils = require('../util/os_utils');
9+
const SensitiveString = require('../util/sensitive_string');
910
const nb_native = require('../util/nb_native');
1011
const native_fs_utils = require('../util/native_fs_utils');
1112
const nc_mkm = require('../manage_nsfs/nc_master_key_manager').get_instance();
@@ -464,6 +465,34 @@ class ConfigFS {
464465
return account;
465466
}
466467

468+
/**
469+
* is_account_exists_by_principal checks if we can get the account in multiple ways:
470+
* 1. name
471+
* 2. id
472+
* (in the future using ARN - currently it is a GAP)
473+
*
474+
* @param {string|SensitiveString} principal
475+
* @param {object} options
476+
* @returns {Promise<Boolean>}
477+
*/
478+
async is_account_exists_by_principal(principal, options = { silent_if_missing: true }) {
479+
if (principal === undefined) return undefined;
480+
481+
const principal_as_string = principal instanceof SensitiveString ? principal.unwrap() : principal;
482+
const arn_prefix = 'arn:aws:iam::';
483+
dbg.log2('is_account_exists_by_principal:', principal, options);
484+
if (principal_as_string.includes(arn_prefix)) {
485+
return false; // GAP
486+
}
487+
const principal_by_id = await this.is_identity_exists(principal_as_string, undefined, options);
488+
dbg.log2('is_account_exists_by_principal: principal_by_id', principal_by_id);
489+
if (principal_by_id) return true;
490+
const principal_by_name = await this.is_account_exists_by_name(principal_as_string, undefined);
491+
dbg.log2('is_account_exists_by_principal: principal_by_name', principal_by_name);
492+
if (principal_by_name) return true;
493+
return false;
494+
}
495+
467496
/**
468497
* list_accounts returns the account names array -
469498
* 1. get new accounts names

src/test/unit_tests/jest_tests/test_nc_nsfs_bucket_cli.test.js

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,15 @@ describe('manage nsfs cli bucket flow', () => {
3939
secret_key: 'G2AYaMpU3zRDcRFWmvzgQr9MoHIAsD+3oEXAMPLE',
4040
};
4141

42+
const account_defaults2 = {
43+
name: 'account_test2',
44+
// without new_buckets_path
45+
uid: 1002,
46+
gid: 1002,
47+
access_key: 'GIGiFAnjaaE7OKD5N8hY',
48+
secret_key: 'G3BYaMpU3zRDcRFWmvzgQr9MoHIAsD+3oEXAMPLE',
49+
};
50+
4251
const bucket_defaults = {
4352
name: 'bucket1',
4453
owner: account_defaults.name,
@@ -56,6 +65,9 @@ describe('manage nsfs cli bucket flow', () => {
5665
await fs_utils.file_must_exist(account_path);
5766
await set_path_permissions_and_owner(account_path, account_options, 0o700);
5867
await exec_manage_cli(TYPES.ACCOUNT, action, account_options);
68+
// account add (account 2 without new_buckets_path)
69+
const account_options2 = { config_root, ...account_defaults2 };
70+
await exec_manage_cli(TYPES.ACCOUNT, action, account_options2);
5971
});
6072

6173
afterEach(async () => {
@@ -196,6 +208,37 @@ describe('manage nsfs cli bucket flow', () => {
196208
const res = await exec_manage_cli(TYPES.BUCKET, action, bucket_options);
197209
expect(JSON.parse(res.stdout).error.code).toBe(ManageCLIError.BucketSetForbiddenBucketOwnerIsIAMAccount.code);
198210
});
211+
212+
it('cli create bucket - with bucket policy (principal by name)', async () => {
213+
const action = ACTIONS.ADD;
214+
const principal_by_name = account_defaults2.name;
215+
const bucket_policy = generate_s3_policy(principal_by_name, bucket_defaults.name, ['s3:*']).policy;
216+
const bucket_policy_string = JSON.stringify(bucket_policy);
217+
const bucket_options = { config_root, ...bucket_defaults, bucket_policy: `'${bucket_policy_string}'`}; // notice bucket_policy with quotes ('')
218+
await fs_utils.create_fresh_path(bucket_options.path);
219+
await fs_utils.file_must_exist(bucket_options.path);
220+
await set_path_permissions_and_owner(bucket_options.path, account_defaults, 0o700);
221+
await exec_manage_cli(TYPES.BUCKET, action, bucket_options);
222+
const bucket = await config_fs.get_bucket_by_name(bucket_defaults.name);
223+
await assert_bucket(bucket, bucket_options, config_fs);
224+
expect(bucket.s3_policy).toStrictEqual(bucket_policy);
225+
});
226+
227+
it('cli create bucket - with bucket policy (principal by id)', async () => {
228+
const action = ACTIONS.ADD;
229+
const account2 = await config_fs.get_account_by_name(account_defaults2.name);
230+
const principal_by_id = account2._id;
231+
const bucket_policy = generate_s3_policy(principal_by_id, bucket_defaults.name, ['s3:*']).policy;
232+
const bucket_policy_string = JSON.stringify(bucket_policy);
233+
const bucket_options = { config_root, ...bucket_defaults, bucket_policy: `'${bucket_policy_string}'`}; // notice bucket_policy with quotes ('')
234+
await fs_utils.create_fresh_path(bucket_options.path);
235+
await fs_utils.file_must_exist(bucket_options.path);
236+
await set_path_permissions_and_owner(bucket_options.path, account_defaults, 0o700);
237+
await exec_manage_cli(TYPES.BUCKET, action, bucket_options);
238+
const bucket = await config_fs.get_bucket_by_name(bucket_defaults.name);
239+
await assert_bucket(bucket, bucket_options, config_fs);
240+
expect(bucket.s3_policy).toStrictEqual(bucket_policy);
241+
});
199242
});
200243

201244
describe('cli create bucket using from_file', () => {
@@ -566,6 +609,37 @@ describe('manage nsfs cli bucket flow', () => {
566609
const res = await exec_manage_cli(TYPES.BUCKET, action, bucket_options);
567610
expect(JSON.parse(res.stdout).error.code).toBe(ManageCLIError.BucketSetForbiddenBucketOwnerIsIAMAccount.code);
568611
});
612+
613+
it('cli update bucket - with bucket policy (principal by name)', async () => {
614+
const action = ACTIONS.UPDATE;
615+
const principal_by_name = account_defaults2.name;
616+
const bucket_policy = generate_s3_policy(principal_by_name, bucket_defaults.name, ['s3:*']).policy;
617+
const bucket_policy_string = JSON.stringify(bucket_policy);
618+
const bucket_options = { config_root, ...bucket_defaults, bucket_policy: `'${bucket_policy_string}'`}; // notice bucket_policy with quotes ('')
619+
await fs_utils.create_fresh_path(bucket_options.path);
620+
await fs_utils.file_must_exist(bucket_options.path);
621+
await set_path_permissions_and_owner(bucket_options.path, account_defaults, 0o700);
622+
await exec_manage_cli(TYPES.BUCKET, action, bucket_options);
623+
const bucket = await config_fs.get_bucket_by_name(bucket_defaults.name);
624+
await assert_bucket(bucket, bucket_options, config_fs);
625+
expect(bucket.s3_policy).toStrictEqual(bucket_policy);
626+
});
627+
628+
it('cli update bucket - with bucket policy (principal by id)', async () => {
629+
const action = ACTIONS.UPDATE;
630+
const account2 = await config_fs.get_account_by_name(account_defaults2.name);
631+
const principal_by_id = account2._id;
632+
const bucket_policy = generate_s3_policy(principal_by_id, bucket_defaults.name, ['s3:*']).policy;
633+
const bucket_policy_string = JSON.stringify(bucket_policy);
634+
const bucket_options = { config_root, ...bucket_defaults, bucket_policy: `'${bucket_policy_string}'`}; // notice bucket_policy with quotes ('')
635+
await fs_utils.create_fresh_path(bucket_options.path);
636+
await fs_utils.file_must_exist(bucket_options.path);
637+
await set_path_permissions_and_owner(bucket_options.path, account_defaults, 0o700);
638+
await exec_manage_cli(TYPES.BUCKET, action, bucket_options);
639+
const bucket = await config_fs.get_bucket_by_name(bucket_defaults.name);
640+
await assert_bucket(bucket, bucket_options, config_fs);
641+
expect(bucket.s3_policy).toStrictEqual(bucket_policy);
642+
});
569643
});
570644

571645
describe('cli delete bucket', () => {

0 commit comments

Comments
 (0)