Added support for second access key s3 access #9290
Conversation
WalkthroughReplaces first-access-key assumptions: account lookup and secret resolution now call Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthServer
participant SystemStore
opt create_access_key_auth / _authorize_signature_token flow
Client->>AuthServer: request with access_key (+ token/signature)
AuthServer->>SystemStore: get_account_by_access_key(access_key)
Note right of SystemStore `#D6EAF8`: returns { account, key_pair }
SystemStore-->>AuthServer: account + matching key_pair
AuthServer->>AuthServer: use key_pair.secret_key to verify signature / derive auth
AuthServer-->>Client: auth result (success / failure)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/server/system_services/account_server.js (1)
118-124: Multi-key lookup inread_account_by_access_keyis correct; consider more robust key unwrappingThe change to use
acc.access_keys.some(...)fixes the previous “first key only” limitation and aligns with the new multi-key behavior. To be more robust against any legacy or non-standard data, you might mirror theSensitiveString/plain-string handling used inaccount_util._check_specific_access_key_exists(unwrap wheninstanceof SensitiveString, otherwise compare as-is) so this lookup doesn’t assume all stored keys are wrapped.src/util/account_util.js (1)
287-291: Encrypting all access keys is right, but current map drops any extra per-key fieldsMapping
account.access_keysto{ access_key, secret_key }ensures every secret gets encrypted, which fixes the single-key assumption. However, if callers ever pass additional per-key properties (e.g. future metadata), they’ll be silently discarded here. You can future‑proof this by spreading the original object and only overridingsecret_key:- account.access_keys = account.access_keys.map(key => ({ - access_key: key.access_key, - secret_key: system_store.master_key_manager.encrypt_sensitive_string_with_master_key_id( - key.secret_key, account_mkey._id) - })); + account.access_keys = account.access_keys.map(key => ({ + ...key, + secret_key: system_store.master_key_manager.encrypt_sensitive_string_with_master_key_id( + key.secret_key, account_mkey._id) + }));src/server/common_services/auth_server.js (1)
189-209: Multi-key + deactivation handling increate_access_key_authlooks correct; could be shared helperThe function now correctly searches all
acc.access_keysfor a matching entry and rejects requests signed with a deactivated key before computing the signature, which addresses the second-access-key gap. Since the same “find account + key_pair by access key and enforce deactivation” pattern is also needed in_authorize_signature_token(and similarly inread_account_by_access_key), consider extracting a small helper (e.g. onsystem_storeor a local utility) to centralize this behavior and avoid future drift.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/server/common_services/auth_server.js(2 hunks)src/server/system_services/account_server.js(1 hunks)src/util/account_util.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-13T07:56:23.620Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9281
File: src/server/system_services/account_server.js:1053-1058
Timestamp: 2025-11-13T07:56:23.620Z
Learning: In noobaa-core, account_server.js is only used in containerized deployments, not in NSFS/NC deployments. NSFS/NC deployments have separate account management code in src/manage_nsfs/ directory. Therefore, account_server.js only processes accounts from account_schema.js where owner is an objectid reference, never from nsfs_account_schema.js where owner is a string.
Applied to files:
src/server/system_services/account_server.jssrc/server/common_services/auth_server.js
🧬 Code graph analysis (3)
src/server/system_services/account_server.js (2)
src/server/common_services/auth_server.js (1)
access_key(181-181)src/util/account_util.js (1)
access_key(507-508)
src/server/common_services/auth_server.js (3)
src/util/account_util.js (7)
access_key(507-508)account(31-43)account(197-197)account(305-305)account(326-326)account(365-365)account(640-640)src/test/external_tests/external_tests_utils.js (2)
access_key(67-67)secret_key(68-68)src/server/system_services/account_server.js (9)
account(54-54)account(102-102)account(121-124)account(146-146)account(194-194)account(317-317)account(680-680)account(944-944)secret(567-567)
src/util/account_util.js (2)
src/server/common_services/auth_server.js (4)
account(104-104)account(189-194)account(325-330)system_store(9-9)src/server/system_services/account_server.js (9)
account(54-54)account(102-102)account(121-124)account(146-146)account(194-194)account(317-317)account(680-680)account(944-944)system_store(17-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
- GitHub Check: run-package-lock-validation
🔇 Additional comments (1)
src/server/common_services/auth_server.js (1)
325-344:_authorize_signature_tokencorrectly honors per-key deactivationThis path now mirrors
create_access_key_auth: it finds the matching key withinaccount.access_keys, rejects deactivated keys, and then uses that key’s secret for signature verification. That ensures signature tokens can’t be used once their underlying access key is deactivated, which is consistent with the access-key status semantics.
aayushchouhan09
force-pushed
the
sec-key
branch
from
96bd7a3 to
c3bf10b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/server/common_services/auth_server.js (2)
189-209: Multi-key lookup and deactivation check look correct; consider small refactor to avoid duplicate scansThe logic to:
- locate the owning account via
acc.access_keys.some(...), and- then find the concrete
key_pairand enforcedeactivatedis consistent with the patterns in
account_server.jsand correctly enables using any access key on the account while blocking deactivated ones.Since you run essentially the same predicate twice (once in
some(), once infind()), you could slightly simplify and avoid repeatedtoString()calls by normalizing the access key once and reusing it:function create_access_key_auth(req) { - const access_key = req.rpc_params.access_key.unwrap(); + const access_key = req.rpc_params.access_key.unwrap(); + const access_key_str = access_key.toString(); @@ - const account = _.find(system_store.data.accounts, function(acc) { - return acc.access_keys && acc.access_keys.length > 0 && - acc.access_keys.some(key => - key.access_key.unwrap().toString() === access_key.toString() - ); - }); + const account = _.find(system_store.data.accounts, function(acc) { + return acc.access_keys && acc.access_keys.length > 0 && + acc.access_keys.some(key => + key.access_key.unwrap().toString() === access_key_str + ); + }); @@ - const key_pair = account.access_keys.find(key => - key.access_key.unwrap().toString() === access_key.toString() - ); + const key_pair = account.access_keys.find(key => + key.access_key.unwrap().toString() === access_key_str + );This keeps behavior identical while making the comparison cheaper and a bit clearer.
325-343: Unify access-key comparison semantics withcreate_access_key_authHere you compare using
key.access_key.unwrap() === auth_token_obj.access_key, whilecreate_access_key_authnormalizes both sides via.toString(). For consistency and to avoid subtle type/format mismatches between these two call sites, it would be safer to normalize both sides the same way in_authorize_signature_token:function _authorize_signature_token(req) { const auth_token_obj = req.auth_token; + const access_key_str = String(auth_token_obj.access_key); @@ - const account = _.find(system_store.data.accounts, function(acc) { - return acc.access_keys && acc.access_keys.length > 0 && - acc.access_keys.some(key => - key.access_key.unwrap() === auth_token_obj.access_key - ); - }); + const account = _.find(system_store.data.accounts, function(acc) { + return acc.access_keys && acc.access_keys.length > 0 && + acc.access_keys.some(key => + String(key.access_key.unwrap()) === access_key_str + ); + }); @@ - const key_pair = account.access_keys.find(key => - key.access_key.unwrap() === auth_token_obj.access_key - ); + const key_pair = account.access_keys.find(key => + String(key.access_key.unwrap()) === access_key_str + );Behavior stays the same for well-formed tokens but removes any discrepancy between the two access-key auth flows.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/server/common_services/auth_server.js(2 hunks)src/server/system_services/account_server.js(1 hunks)src/util/account_util.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/server/system_services/account_server.js
- src/util/account_util.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-19T15:03:42.231Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9291
File: src/server/common_services/auth_server.js:548-554
Timestamp: 2025-11-19T15:03:42.231Z
Learning: In src/server/common_services/auth_server.js, account objects are loaded directly from system_store (e.g., system_store.data.get_by_id()), so account.owner is an object ID reference with an ._id property, not a string. This differs from s3_rest.js where account.owner is a string due to RPC serialization.
Applied to files:
src/server/common_services/auth_server.js
🧬 Code graph analysis (1)
src/server/common_services/auth_server.js (3)
src/util/account_util.js (7)
access_key(496-497)account(30-42)account(196-196)account(304-304)account(325-325)account(362-362)account(628-628)src/test/external_tests/external_tests_utils.js (2)
access_key(67-67)secret_key(68-68)src/server/system_services/account_server.js (9)
account(69-69)account(117-117)account(135-138)account(160-160)account(208-208)account(331-331)account(694-694)account(958-958)secret(581-581)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-package-lock-validation
- GitHub Check: run-jest-unit-tests
@aayushchouhan09 could you mention what S3 operations you tested in the PR description? |
aayushchouhan09
force-pushed
the
sec-key
branch
from
c3bf10b to
a1dc25d
Compare
Tried operations like make bucket, upload object, download object and list objects --> working as expected |
aayushchouhan09
force-pushed
the
sec-key
branch
from
a1dc25d to
a622d73
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
-
src/server/common_services/auth_server.js(2 hunks) -
src/server/system_services/account_server.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server/system_services/account_server.js
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-19T15:03:42.260Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9291
File: src/server/common_services/auth_server.js:548-554
Timestamp: 2025-11-19T15:03:42.260Z
Learning: In src/server/common_services/auth_server.js, account objects are loaded directly from system_store (e.g., system_store.data.get_by_id()), so account.owner is an object ID reference with an ._id property, not a string. This differs from s3_rest.js where account.owner is a string due to RPC serialization.
Applied to files:
src/server/common_services/auth_server.js
📚 Learning: 2025-11-13T07:56:23.620Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9281
File: src/server/system_services/account_server.js:1053-1058
Timestamp: 2025-11-13T07:56:23.620Z
Learning: In noobaa-core, account_server.js is only used in containerized deployments, not in NSFS/NC deployments. NSFS/NC deployments have separate account management code in src/manage_nsfs/ directory. Therefore, account_server.js only processes accounts from account_schema.js where owner is an objectid reference, never from nsfs_account_schema.js where owner is a string.
Applied to files:
src/server/common_services/auth_server.js
📚 Learning: 2025-11-12T04:55:42.193Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
Applied to files:
src/server/common_services/auth_server.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run-package-lock-validation
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (2)
src/server/common_services/auth_server.js (2)
181-199: Approve the multi-key authentication approach.The refactoring to use
system_store.get_account_by_access_key()and iterate through all access keys correctly implements multi-key support. The centralized lookup and per-key secret resolution align well with the PR objective.
195-197: I'll help you verify the handling of deactivated access keys in the authentication flow. Let me start by running the verification script to examine the codebase.
<function_calls>
#!/bin/bashDescription: Check if system_store.get_account_by_access_key filters deactivated keys
and verify if additional checks are needed in the authentication flow
Search for get_account_by_access_key implementation
echo "=== Searching for get_account_by_access_key implementation ==="
ast-grep --pattern$'get_account_by_access_key($ $$) {
$$$
}'Search for deactivated key checks in authentication flows
echo -e "\n=== Searching for deactivated key handling ==="
rg -n -C5 'deactivated.*access.*key|access.*key.*deactivated' --type=jsSearch for key status validation
echo -e "\n=== Searching for key status checks ==="
rg -n -C3 '.deactivated\b' --type=js
</function_calls>
aayushchouhan09
force-pushed
the
sec-key
branch
from
a622d73 to
838088c
Compare
Signed-off-by: Aayush Chouhan <achouhan@redhat.com>
aayushchouhan09
force-pushed
the
sec-key
branch
from
838088c to
8a7f955
Compare
3 participants
Describe the Problem
Currently, we do not support second access key for s3 access.
Explain the Changes
access_keys[0]), preventing IAM users from using their second access key for S3 operations. This fix updatesauth_server.js,account_server.jsandaccount_util.jsto iterate through all access keys in the array.Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
nb account create <account-name>Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.