Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/endpoint/s3/s3_rest.js
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,8 @@ async function authorize_request_policy(req) {
public_access_block,
} = await req.object_sdk.read_bucket_sdk_policy_info(req.params.bucket);

console.log("authorize_request_policy ====>>>", bucket_owner, owner_account);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use console.log and not dbg.log0 ?
Is it a leftover, and we want to remove it completely?

Suggested change
console.log("authorize_request_policy ====>>>", bucket_owner, owner_account);
dbg.log1("authorize_request_policy ====>>>", bucket_owner, owner_account);


Comment on lines +244 to +245
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Remove debug console.log or convert to proper logging.

This console.log statement should be removed before merging, or if needed for debugging, converted to use the dbg module (e.g., dbg.log0 or dbg.log1) for consistency with the rest of the codebase. Additionally, be cautious about logging bucket_owner and owner_account as they may contain sensitive information.

Apply this diff to remove the debug statement:

-    console.log("authorize_request_policy ====>>>", bucket_owner, owner_account);
-

Or, if logging is necessary, convert it to:

-    console.log("authorize_request_policy ====>>>", bucket_owner, owner_account);
-
+    dbg.log1('authorize_request_policy:', { bucket_owner, owner_account });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
console.log("authorize_request_policy ====>>>", bucket_owner, owner_account);
🤖 Prompt for AI Agents
In src/endpoint/s3/s3_rest.js around lines 244 to 245, remove the stray debug
console.log statement or replace it with the project dbg logging (e.g., dbg.log0
or dbg.log1) to match existing logging conventions; if logging is required,
redact or avoid printing sensitive fields like bucket_owner and owner_account
(log only non-sensitive identifiers or an existence flag) and ensure the call
uses the dbg module rather than console.log.

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);
Expand Down
78 changes: 77 additions & 1 deletion src/test/integration_tests/internal/test_upgrade_scripts.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,15 @@
// setup coretest first to prepare the env
const _ = require('lodash');
const coretest = require('../../utils/coretest/coretest');
const { rpc_client, EMAIL } = coretest;
const { rpc_client, EMAIL, POOL_LIST } = coretest;
coretest.setup({ pools_to_create: [coretest.POOL_LIST[1]] });
const { S3 } = require('@aws-sdk/client-s3');
const { NodeHttpHandler } = require("@smithy/node-http-handler");
const http = require('http');
const SensitiveString = require('../../../util/sensitive_string');
const system_store = require('../../../server/system_services/system_store').get_instance();
const upgrade_bucket_policy = require('../../../upgrade/upgrade_scripts/5.15.6/upgrade_bucket_policy');
const upgrade_bucket_policy_principal = require('../../../upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal');
const upgrade_bucket_cors = require('../../../upgrade/upgrade_scripts/5.19.0/upgrade_bucket_cors');
const remove_mongo_pool = require('../../../upgrade/upgrade_scripts/5.20.0/remove_mongo_pool');
const dbg = require('../../../util/debug_module')(__filename);
Expand Down Expand Up @@ -199,6 +201,80 @@ mocha.describe('test upgrade scripts', async function() {
assert.strictEqual(updated_bucket.tiering.tiers[0].tier.mirrors[0].spread_pools[0].name, default_pool_name);
});

mocha.it('test upgrade bucket policy to ARN version 5.21.0', async function() {
const iam_username = 'iam_username';
const old_policy = {
Version: '2012-10-17',
Statement: [{
Sid: 'id-1',
Effect: 'Allow',
Principal: {
"AWS": [new SensitiveString(EMAIL)],
},
Action: ['s3:GetObject', 's3:*'],
Resource: [`arn:aws:s3:::*`]
},
{
Effect: 'Deny',
Principal: {
"AWS": [new SensitiveString(iam_username)],
},
Action: ['s3:PutObject'],
Resource: [`arn:aws:s3:::*`]
},
]
};
// clean all leftover bucket policies as upgrade script doesn't work on updated policies
await _clean_all_bucket_policies();

const bucket = system_store.data.buckets.find(bucket_obj => bucket_obj.name.unwrap() === BKT);
await system_store.make_changes({
update: {
buckets: [{
_id: bucket._id,
s3_policy: old_policy
}]
}
});
const account = system_store.data.accounts.find(acc => acc.email.unwrap() === EMAIL);
const nsr = 's3_bucket_policy_nsr';
const iam_acc = {
name: iam_username,
email: iam_username,
has_login: false,
s3_access: true,
default_resource: process.env.NC_CORETEST ? nsr : POOL_LIST[1].name,
};
await rpc_client.account.create_account(iam_acc);

const iam_account = system_store.data.accounts.find(acc => acc.email.unwrap() === iam_username);
await system_store.make_changes({
update: {
accounts: [{
_id: iam_account._id,
owner: account._id.toString(),
}]
}
});

await upgrade_bucket_policy_principal.run({ dbg, system_store, system_server: null });
const res = await s3.getBucketPolicy({ // should work - bucket policy should fit current schema
Bucket: BKT,
});
const new_policy = JSON.parse(res.Policy);

assert.strictEqual(new_policy.Statement.length, old_policy.Statement.length);
assert.strictEqual(new_policy.Version, old_policy.Version);
assert.strictEqual(new_policy.Statement[0].Sid, old_policy.Statement[0].Sid);
assert.strictEqual(new_policy.Statement[0].Effect, 'Allow');
assert.strictEqual(new_policy.Statement[0].Action[0], 's3:GetObject');
assert.strictEqual(new_policy.Statement[0].Action[1], 's3:*');
assert.strictEqual(new_policy.Statement[0].Resource[0], old_policy.Statement[0].Resource[0]);

assert.strictEqual(new_policy.Statement[0].Principal.AWS[0], `aws:arn:${account._id.toString()}:root`);
assert.strictEqual(new_policy.Statement[1].Principal.AWS[0], `aws:arn:${iam_account._id.toString()}:user/${iam_account.email.unwrap()}`);
});

mocha.after(async function() {
await s3.deleteBucket({ Bucket: BKT });
await s3.deleteBucket({ Bucket: BKT1 });
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
/* Copyright (C) 2023 NooBaa */
"use strict";

const util = require('util');
const _ = require('lodash');

const iam_constants = require('../../../endpoint/iam/iam_constants');

function _create_arn(dbg, principals, system_store) {
if (!principals.AWS) return;
const principal_arns = [];
for (const principal of principals.AWS) {
if (principal === '*') continue;
const account = system_store.data.accounts.find(acc => acc.email.unwrap() === principal.unwrap());
if (!account) {
dbg.log0(`Could not found the account with email: ${principal}`);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Avoid logging sensitive principal information.

Logging the principal email directly may expose sensitive account information. Consider redacting or using a safer logging format.

Apply this diff:

-            dbg.log0(`Could not found the account with email: ${principal}`);
+            dbg.log0(`Could not find account for principal in bucket policy`);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
dbg.log0(`Could not found the account with email: ${principal}`);
dbg.log0(`Could not find account for principal in bucket policy`);
🤖 Prompt for AI Agents
In src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js around
line 16, the code currently logs the full principal email; change the log to
avoid printing sensitive principal information by either redacting or
hashing/truncating the principal before logging (e.g., replace the local-part
with asterisks or log only a UUID/hash of the principal), and update the message
to something like "Could not find the account for principal: [REDACTED]" while
preserving enough context for debugging; implement a small helper or inline
masking (do not log the raw principal) and use that in the processLogger/dbg
call.

continue;
}
let arn;
if (account.owner) {
const iam_path = account.iam_path || iam_constants.IAM_DEFAULT_PATH;
arn = `aws:arn:${account._id.toString()}:user${iam_path}${account.email.unwrap()}`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the places there are aws:arn...
We should use arn:aws: instead, and maybe it's worth setting a prefix const and not rewriting that every time. The same goes for the suffix.

Suggested change
arn = `aws:arn:${account._id.toString()}:user${iam_path}${account.email.unwrap()}`;
arn = `arn:aws:${account._id.toString()}:user${iam_path}${account.email.unwrap()}`;

} else {
arn = `aws:arn:${account._id.toString()}:root`;
}
principal_arns.push(arn);
}
return { AWS: principal_arns };
}
Comment on lines +9 to +29
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: Wildcard principals are lost during upgrade.

The function skips wildcard ('*') principals at line 13 but never adds them to principal_arns, causing wildcard principals to be dropped from the upgraded policy. This breaks bucket policies that grant anonymous access.

Apply this diff to preserve wildcard principals:

 function _create_arn(dbg, principals, system_store) {
     if (!principals.AWS) return;
     const principal_arns = [];
     for (const principal of principals.AWS) {
-        if (principal === '*') continue;
+        if (principal === '*') {
+            principal_arns.push('*');
+            continue;
+        }
         const account = system_store.data.accounts.find(acc => acc.email.unwrap() === principal.unwrap());
         if (!account) {
             dbg.log0(`Could not found the account with email: ${principal}`);
             continue;
         }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
function _create_arn(dbg, principals, system_store) {
if (!principals.AWS) return;
const principal_arns = [];
for (const principal of principals.AWS) {
if (principal === '*') continue;
const account = system_store.data.accounts.find(acc => acc.email.unwrap() === principal.unwrap());
if (!account) {
dbg.log0(`Could not found the account with email: ${principal}`);
continue;
}
let arn;
if (account.owner) {
const iam_path = account.iam_path || iam_constants.IAM_DEFAULT_PATH;
arn = `aws:arn:${account._id.toString()}:user${iam_path}${account.email.unwrap()}`;
} else {
arn = `aws:arn:${account._id.toString()}:root`;
}
principal_arns.push(arn);
}
return { AWS: principal_arns };
}
function _create_arn(dbg, principals, system_store) {
if (!principals.AWS) return;
const principal_arns = [];
for (const principal of principals.AWS) {
if (principal === '*') {
principal_arns.push('*');
continue;
}
const account = system_store.data.accounts.find(acc => acc.email.unwrap() === principal.unwrap());
if (!account) {
dbg.log0(`Could not found the account with email: ${principal}`);
continue;
}
let arn;
if (account.owner) {
const iam_path = account.iam_path || iam_constants.IAM_DEFAULT_PATH;
arn = `aws:arn:${account._id.toString()}:user${iam_path}${account.email.unwrap()}`;
} else {
arn = `aws:arn:${account._id.toString()}:root`;
}
principal_arns.push(arn);
}
return { AWS: principal_arns };
}
🤖 Prompt for AI Agents
In src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js around
lines 9 to 29, the upgrade currently skips wildcard principals ('*') and thus
drops anonymous access from upgraded policies; modify the loop to preserve
wildcards by adding '*' to principal_arns instead of continuing (e.g., when
principal === '*' push '*' into principal_arns), keeping other logic for
resolving account ARNs unchanged so the returned object can include both
resolved ARNs and the wildcard.


async function run({ dbg, system_store, system_server }) {
try {
dbg.log0('Starting bucket policy Principal upgrade...');
const buckets = [];
for (const bucket of system_store.data.buckets) {
// Do not update if there are no bucket policy.
if (!bucket.s3_policy) continue;
if (!_.isUndefined(bucket.s3_policy.Statement)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need loadsh?

Suggested change
if (!_.isUndefined(bucket.s3_policy.Statement)) {
if (bucket.s3_policy.Statement !== undefined) {

const new_policy = bucket.s3_policy;
new_policy.Statement = bucket.s3_policy.Statement.map(statement => ({
Sid: statement.Sid,
Effect: statement.Effect,
Action: statement.Action,
Resource: statement.Resource,
Principal: _create_arn(dbg, statement.Principal, system_store),
}));
Comment on lines +38 to +46
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Essential: Statement fields may be lost during upgrade.

The upgrade script only copies Sid, Effect, Action, Resource, and Principal fields from each statement. This will drop other important fields such as Condition, NotPrincipal, NotAction, and NotResource if they exist in the original policy, breaking the policy semantics.

Apply this diff to preserve all statement fields:

                 const new_policy = bucket.s3_policy;
                 new_policy.Statement = bucket.s3_policy.Statement.map(statement => ({
-                    Sid: statement.Sid,
-                    Effect: statement.Effect,
-                    Action: statement.Action,
-                    Resource: statement.Resource,
+                    ...statement,
                     Principal: _create_arn(dbg, statement.Principal, system_store),
                 }));

buckets.push({
_id: bucket._id,
s3_policy: new_policy,
});
}
}

if (buckets.length > 0) {
dbg.log0(`Replacing bucket policy Principal : ${buckets.map(bucket => util.inspect(bucket)).join(', ')}`);
await system_store.make_changes({ update: { buckets } });
} else {
dbg.log0('Upgrading buckets policy Principal: no upgrade needed...');
}

} catch (err) {
dbg.error('Got error while upgrading buckets policy Principal:', err);
throw err;
}
}


module.exports = {
run,
description: 'Update bucket policy Principal to ARN formate'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Fix typo in description.

The word "formate" should be "format".

Apply this diff:

-    description: 'Update bucket policy Principal to ARN formate'
+    description: 'Update bucket policy Principal to ARN format'
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
description: 'Update bucket policy Principal to ARN formate'
description: 'Update bucket policy Principal to ARN format'
🤖 Prompt for AI Agents
In src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js around
line 70, fix the typo in the description by replacing "formate" with "format" so
the description reads 'Update bucket policy Principal to ARN format'.

};