From 02bfc9413322e3558e79cc8e9a78315e30ba7f2b Mon Sep 17 00:00:00 2001 From: Utkarsh Srivastava Date: Mon, 5 May 2025 14:40:06 +0530 Subject: [PATCH] fix anonymous user access with PAB defined and fix PAB tests Signed-off-by: Utkarsh Srivastava --- src/api/bucket_api.js | 3 + src/endpoint/s3/s3_rest.js | 20 ++++--- src/sdk/object_sdk.js | 1 + src/server/system_services/bucket_server.js | 1 + .../unit_tests/test_public_access_block.js | 60 +++++++++---------- 5 files changed, 47 insertions(+), 38 deletions(-) diff --git a/src/api/bucket_api.js b/src/api/bucket_api.js index 5816cf5c76..5634313fc4 100644 --- a/src/api/bucket_api.js +++ b/src/api/bucket_api.js @@ -1321,6 +1321,9 @@ module.exports = { }, cors_configuration_rules: { $ref: 'common_api#/definitions/bucket_cors_configuration', + }, + public_access_block: { + $ref: 'common_api#/definitions/public_access_block', } } }, diff --git a/src/endpoint/s3/s3_rest.js b/src/endpoint/s3/s3_rest.js index a99635bc4b..eff8e68923 100755 --- a/src/endpoint/s3/s3_rest.js +++ b/src/endpoint/s3/s3_rest.js @@ -230,14 +230,21 @@ async function authorize_request_policy(req) { if (req.op_name === 'put_bucket') return; // owner_account is { id: bucket.owner_account, email: bucket.bucket_owner }; - const { s3_policy, system_owner, bucket_owner, owner_account } = await req.object_sdk.read_bucket_sdk_policy_info(req.params.bucket); + const { + s3_policy, + system_owner, + bucket_owner, + owner_account, + public_access_block, + } = await req.object_sdk.read_bucket_sdk_policy_info(req.params.bucket); + const auth_token = req.object_sdk.get_auth_token(); const arn_path = _get_arn_from_req_path(req); const method = _get_method_from_req(req); const is_anon = !(auth_token && auth_token.access_key); if (is_anon) { - await authorize_anonymous_access(s3_policy, method, arn_path, req); + await authorize_anonymous_access(s3_policy, method, arn_path, req, public_access_block); return; } @@ -283,21 +290,20 @@ async function authorize_request_policy(req) { let permission_by_id; let permission_by_name; - const public_access_block_cfg = await req.object_sdk.get_public_access_block({ name: req.params.bucket }); // In NC, we allow principal to be: // 1. account name (for backwards compatibility) // 2. account id // we start the permission check on account identifier intentionally if (account_identifier_id) { permission_by_id = await s3_bucket_policy_utils.has_bucket_policy_permission( - s3_policy, account_identifier_id, method, arn_path, req, public_access_block_cfg?.public_access_block?.restrict_public_buckets); + s3_policy, account_identifier_id, method, arn_path, req, public_access_block?.restrict_public_buckets); dbg.log3('authorize_request_policy: permission_by_id', permission_by_id); } if (permission_by_id === "DENY") throw new S3Error(S3Error.AccessDenied); if ((!account_identifier_id || permission_by_id !== "DENY") && account.owner === undefined) { permission_by_name = await s3_bucket_policy_utils.has_bucket_policy_permission( - s3_policy, account_identifier_name, method, arn_path, req, public_access_block_cfg?.public_access_block?.restrict_public_buckets + s3_policy, account_identifier_name, method, arn_path, req, public_access_block?.restrict_public_buckets ); dbg.log3('authorize_request_policy: permission_by_name', permission_by_name); } @@ -307,11 +313,11 @@ async function authorize_request_policy(req) { throw new S3Error(S3Error.AccessDenied); } -async function authorize_anonymous_access(s3_policy, method, arn_path, req) { +async function authorize_anonymous_access(s3_policy, method, arn_path, req, public_access_block) { if (!s3_policy) throw new S3Error(S3Error.AccessDenied); const permission = await s3_bucket_policy_utils.has_bucket_policy_permission( - s3_policy, undefined, method, arn_path, req); + s3_policy, undefined, method, arn_path, req, public_access_block?.restrict_public_buckets); if (permission === "ALLOW") return; throw new S3Error(S3Error.AccessDenied); diff --git a/src/sdk/object_sdk.js b/src/sdk/object_sdk.js index ae2bd7430b..a471d56582 100644 --- a/src/sdk/object_sdk.js +++ b/src/sdk/object_sdk.js @@ -219,6 +219,7 @@ class ObjectSDK { system_owner: bucket.system_owner, // note that bucketspace_fs currently doesn't return system_owner bucket_owner: bucket.bucket_owner, owner_account: bucket.owner_account, // in NC NSFS this is the account id that owns the bucket + public_access_block: bucket.public_access_block, }; return policy_info; } diff --git a/src/server/system_services/bucket_server.js b/src/server/system_services/bucket_server.js index cade1b7eab..268006beff 100644 --- a/src/server/system_services/bucket_server.js +++ b/src/server/system_services/bucket_server.js @@ -716,6 +716,7 @@ async function read_bucket_sdk_info(req) { .then(get_bucket_info), notifications: bucket.notifications, cors_configuration_rules: bucket.cors_configuration_rules, + public_access_block: bucket.public_access_block, }; if (bucket.namespace) { diff --git a/src/test/unit_tests/test_public_access_block.js b/src/test/unit_tests/test_public_access_block.js index e4aa64ec47..6a52dfd301 100644 --- a/src/test/unit_tests/test_public_access_block.js +++ b/src/test/unit_tests/test_public_access_block.js @@ -46,7 +46,7 @@ async function run_on_random_bucket(s3, bucket_prefix, cb) { try { await cb(bucket); } finally { - await s3.deleteBucket({ Bucket: bucket }); + await s3.deleteBucket({ Bucket: bucket }).catch(() => { /* nothing */ }); } } @@ -110,29 +110,33 @@ mocha.describe('noobaa public access block test', function() { mocha.it('put_public_access_block must throw when acls are used', async function() { await run_on_random_bucket(s3_owner, bucket_prefix, async bucket => { - try { - await s3_owner.putPublicAccessBlock({ + await assert.rejects( + s3_owner.putPublicAccessBlock({ Bucket: bucket, PublicAccessBlockConfiguration: { BlockPublicAcls: true, } - }); - assert.fail("expected to fail"); - } catch (error) { - assert(error.Code === S3Error.AccessControlListNotSupported.code); - } + }), + error => { + // @ts-ignore + assert(error.Code === S3Error.AccessControlListNotSupported.code); + return true; + } + ); - try { - await s3_owner.putPublicAccessBlock({ + await assert.rejects( + s3_owner.putPublicAccessBlock({ Bucket: bucket, PublicAccessBlockConfiguration: { IgnorePublicAcls: true, } - }); - assert.fail("expected to fail"); - } catch (error) { - assert(error.Code === S3Error.AccessControlListNotSupported.code); - } + }), + error => { + // @ts-ignore + assert(error.Code === S3Error.AccessControlListNotSupported.code); + return true; + } + ); }); }); @@ -146,15 +150,12 @@ mocha.describe('noobaa public access block test', function() { }); // Ensure we cannot put a public policy on this bucket - try { - await s3_owner.putBucketPolicy({ + await assert.rejects( + s3_owner.putBucketPolicy({ Bucket: bucket, Policy: generate_public_policy(bucket), - }); - assert.fail("expected to fail"); - } catch { - assert.ok(true); - } + }) + ); await s3_owner.deletePublicAccessBlock({ Bucket: bucket, @@ -163,7 +164,7 @@ mocha.describe('noobaa public access block test', function() { }); mocha.it('public_access_block should work when restrict public buckets is used', async function() { - await run_on_random_bucket(s3_owner, bucket_prefix, async bucket => { + await run_on_random_bucket(s3_owner, bucket_prefix, async function(bucket) { const KEY = "key"; await s3_owner.putObject({ @@ -191,16 +192,13 @@ mocha.describe('noobaa public access block test', function() { } }); - try { - // Ensure anon can no longer access - await s3_anon.getObject({ + // Ensure anon can no longer access + await assert.rejects( + s3_anon.getObject({ Bucket: bucket, Key: KEY, - }); - assert.fail("expected to fail after PublicAccessBlock"); - } catch { - assert.ok(true); - } + }), + ); await s3_owner.deletePublicAccessBlock({ Bucket: bucket,