-
Notifications
You must be signed in to change notification settings - Fork 102
Nc iam roles ldap #9888
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Nc iam roles ldap #9888
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -342,13 +342,13 @@ Used when an external identity provider (LDAP etc.) authenticates the user. NooB | |
| "Version": "2012-10-17", | ||
| "Statement": [{ | ||
| "Effect": "Allow", | ||
| "Principal": { "Federated": "ldap://127.0.0.1:1389" }, | ||
| "Principal": { "Federated": "arn:aws:iam:::ldap-provider/127.0.0.1:1636" }, | ||
| "Action": "sts:AssumeRoleWithWebIdentity" | ||
| }] | ||
| } | ||
| ``` | ||
|
|
||
| The `Federated` value must match the LDAP server URI in `/etc/noobaa-server/ldap_config`. Matching strips the `ldap://` / `ldaps://` prefix. | ||
| The `Federated` value must be arn:aws:iam:::ldap-provider/<host>[:port] and host:port must match the LDAP server URI in `/etc/noobaa-server/ldap_config`. Matching strips the `ldap://` / `ldaps://` prefix. | ||
|
|
||
| ##### LDAP group / attribute conditions | ||
|
|
||
|
|
@@ -366,7 +366,7 @@ Condition keys use the `ldap:<attribute>` format. NooBaa strips the `ldap:` pref | |
| "Version": "2012-10-17", | ||
| "Statement": [{ | ||
| "Effect": "Allow", | ||
| "Principal": { "Federated": "ldap://127.0.0.1:1389" }, | ||
| "Principal": { "Federated": "arn:aws:iam:::ldap-provider/127.0.0.1:1636" }, | ||
| "Action": "sts:AssumeRoleWithWebIdentity", | ||
| "Condition": { | ||
| "StringEquals": { "ldap:ou": "Delivering Crew" } | ||
|
|
@@ -382,7 +382,7 @@ Condition keys use the `ldap:<attribute>` format. NooBaa strips the `ldap:` pref | |
| "Version": "2012-10-17", | ||
| "Statement": [{ | ||
| "Effect": "Allow", | ||
| "Principal": { "Federated": "ldap://127.0.0.1:1389" }, | ||
| "Principal": { "Federated": "arn:aws:iam:::ldap-provider/127.0.0.1:1636" }, | ||
| "Action": "sts:AssumeRoleWithWebIdentity", | ||
| "Condition": { | ||
| "ForAnyValue:StringEquals": { | ||
|
|
@@ -403,7 +403,7 @@ Condition keys use the `ldap:<attribute>` format. NooBaa strips the `ldap:` pref | |
| "Version": "2012-10-17", | ||
| "Statement": [{ | ||
| "Effect": "Allow", | ||
| "Principal": { "Federated": "ldap://127.0.0.1:1389" }, | ||
| "Principal": { "Federated": "arn:aws:iam:::ldap-provider/127.0.0.1:1636" }, | ||
| "Action": "sts:AssumeRoleWithWebIdentity", | ||
| "Condition": { | ||
| "StringEquals": { "ldap:ou": "Delivering Crew" }, | ||
|
|
@@ -423,15 +423,15 @@ Condition keys use the `ldap:<attribute>` format. NooBaa strips the `ldap:` pref | |
| "Statement": [ | ||
| { | ||
| "Effect": "Allow", | ||
| "Principal": { "Federated": "ldap://127.0.0.1:1389" }, | ||
| "Principal": { "Federated": "arn:aws:iam:::ldap-provider/127.0.0.1:1636" }, | ||
| "Action": "sts:AssumeRoleWithWebIdentity", | ||
| "Condition": { | ||
| "StringEquals": { "ldap:ou": "Delivering Crew" } | ||
| } | ||
| }, | ||
| { | ||
| "Effect": "Allow", | ||
| "Principal": { "Federated": "ldap://127.0.0.1:1389" }, | ||
| "Principal": { "Federated": "arn:aws:iam:::ldap-provider/127.0.0.1:1636" }, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sakshimunjal I think maybe we should add a name to the ldap configuration and use it here - it will look closer to what AWS is doing. WDYT? |
||
| "Action": "sts:AssumeRoleWithWebIdentity", | ||
| "Condition": { | ||
| "ForAnyValue:StringEquals": { | ||
|
|
@@ -492,7 +492,7 @@ NooBaa STS | |
| │ └─ look up role entity via config_fs / AccountSpaceFS | ||
| │ [Phase 2: look up standalone role entity by owner_id, role_name] | ||
| ├─► 5. [TODO] Evaluate trust policy | ||
| │ ├─ Principal fit (Federated URI match / "*" / AWS ARN) | ||
| │ ├─ Principal fit (Federated ldap-provider ARN match / "*" / AWS ARN) | ||
| │ ├─ Action fit (sts:AssumeRoleWithWebIdentity) | ||
| │ └─ Condition fit (e.g. ldap:ou == "Engineering") | ||
|
Comment on lines
+495
to
497
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Remove the stale LDAP trust-policy TODO. The flow now lists the LDAP principal, action, and condition checks, but Line 494 still labels trust-policy evaluation as 🤖 Prompt for AI Agents |
||
| └─► 6. Issue temporary credentials (AccessKeyId + SecretAccessKey + SessionToken) | ||
|
|
@@ -599,7 +599,7 @@ Phase 2 — append `iam_role_policies`: | |
| ### Code Changes | ||
|
|
||
| 1. Implement all role CRUD methods in `src/sdk/accountspace_fs.js` and `config_fs.js` | ||
| 2. Add support for `Principal.Federated` in trust-policy evaluation (`_is_principal_fit()` / access policy utils) — LDAP URI match and OIDC/`iss` match | ||
| 2. Add support for `Principal.Federated` in trust-policy evaluation (`_is_principal_fit()` / access policy utils) — LDAP ARN match and OIDC/`iss` match | ||
| 3. Load `assume_role_policy_document` from role entity via `config_fs` / **role cache** (same pattern as account cache) | ||
| 4. Implement LDAP and Keycloak condition evaluation for trust policy (`_is_identity_condition_fit`) — including `ldap:ou` / `ldap:memberOf` | ||
| 5. Schema changes for roles (`name`, `owner`, `assume_role_policy_document`, …) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -117,6 +117,11 @@ module.exports = { | |
| enum: ['DISABLED', 'SUSPENDED', 'ENABLED'] | ||
| }, | ||
|
|
||
| identity_type: { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @aayushchouhan09 aslo have kind of same changes in his PR, can you check
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, have already updated in his PR to take care of it in case my PR gets merged first, else I'll take care of the conflicts before merging. there were some other changes related to this field which need to be added for NC, hence added it here as well |
||
| type: 'string', | ||
| enum: ['ACCOUNT', 'USER', 'ROLE'] | ||
| }, | ||
|
|
||
| assume_role_policy: { | ||
| type: 'object', | ||
| required: ['statement'], | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -659,7 +659,7 @@ async function validate_account_args(config_fs, data, action, is_flag_iam_operat | |
| */ | ||
| async function validate_account_resources_before_deletion(config_fs, data) { | ||
| await validate_account_not_owns_buckets(config_fs, data); | ||
| // If it is root account (not owned by other account) then we check that it doesn't owns IAM accounts | ||
| // If it is root account (not owned by other account) then we check that it doesn't owns IAM users | ||
| if (data.owner === undefined) { | ||
| await check_if_root_account_does_not_have_IAM_users(config_fs, data, ACTIONS.DELETE); | ||
| await validate_account_not_owns_roles(config_fs, data); | ||
|
|
@@ -739,7 +739,7 @@ async function check_if_root_account_does_not_have_IAM_users(config_fs, account_ | |
| if (is_root_account_owns_user) { | ||
| const detail_msg = `Account ${account_to_check.name} has IAM account ${account_data.name}`; | ||
| if (action === ACTIONS.DELETE) { | ||
| throw_cli_error(ManageCLIError.AccountDeleteForbiddenHasIAMAccounts, detail_msg); | ||
| throw_cli_error(ManageCLIError.AccountDeleteForbiddenHasIAMUsers, detail_msg); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Use IAM-user terminology in the deletion detail.
Proposed fix- const detail_msg = `Account ${account_to_check.name} has IAM account ${account_data.name}`;
+ const detail_msg = `Account ${account_to_check.name} has IAM user ${account_data.name}`;🤖 Prompt for AI Agents |
||
| } | ||
| // else it is called with action ACTIONS.UPDATE | ||
| throw_cli_error(ManageCLIError.AccountCannotBeRootAccountsManager, detail_msg); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -942,7 +942,7 @@ class AccountSpaceFS { | |
| _id: generate_id(), | ||
| name: params.role_name, | ||
| email: params.role_name, | ||
| type: 'role', | ||
| identity_type: 'ROLE', | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# Expect role readers and migration paths to use identity_type.
rg -n --glob '*.js' \
"identity_type|role_data\.type|account_data\.type|type[[:space:]]*:[[:space:]]*['\"]role['\"]" \
srcRepository: noobaa/noobaa-core Length of output: 609 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- accountspace_fs.js role-related symbols ---'
rg -n -C 8 --glob 'src/sdk/accountspace_fs.js' \
"_new_role_defaults|create_role_config_file|role.*config|identity_type|\.type" src/sdk/accountspace_fs.js
printf '%s\n' '--- role configuration readers and validators ---'
rg -n -C 5 --glob '*.js' \
"read.*role|role.*config|role_config|identity_type|identity.*type|type[[:space:]]*:[[:space:]]*['\"]role['\"]" \
src/server src/sdk src/test
printf '%s\n' '--- accountspace_fs structure ---'
ast-grep outline src/sdk/accountspace_fs.js --match '_new_role_defaults' --view expanded
ast-grep outline src/sdk/accountspace_fs.js --match 'create_role_config_file' --view expanded
printf '%s\n' '--- relevant schema sections ---'
cat -n src/server/system_services/schemas/nsfs_account_schema.js | sed -n '90,155p'
cat -n src/api/common_api.js | sed -n '95,140p'Repository: noobaa/noobaa-core Length of output: 50375 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- role read and transformation implementations ---'
cat -n src/sdk/config_fs.js | sed -n '720,850p'
cat -n src/sdk/bucketspace_fs.js | sed -n '110,155p'
cat -n src/server/system_services/account_server.js | sed -n '130,175p'
printf '%s\n' '--- identity loading and schema validation ---'
rg -n -C 6 --glob '*.js' \
"get_identity_config_data|validate_account_schema|identity_type|CONFIG_TYPES" \
src/sdk src/server/system_services src/api | head -n 500
printf '%s\n' '--- migration-related code and legacy role fields ---'
rg -n -C 5 --glob '*.js' \
"migrat|legacy|type[[:space:]]*:[[:space:]]*['\"]role['\"]|role_data\.type|identity_type" \
src | head -n 600
printf '%s\n' '--- identity and role schema definitions ---'
rg -n -C 12 --glob '*.js' \
"identity_type:|role_info|role_config|additionalProperties" \
src/api/common_api.js src/server/system_services/schemas/nsfs_account_schema.js src/server/system_services/schemas/account_schema.jsRepository: noobaa/noobaa-core Length of output: 50375 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- complete relevant schema declarations ---'
cat -n src/server/system_services/schemas/nsfs_account_schema.js | sed -n '1,145p'
rg -n -C 10 --glob 'src/manage_nsfs/nsfs_schema_utils.js' \
"validate_account_schema|validate.*schema|additionalProperties|nsfs_account_schema" src/manage_nsfs/nsfs_schema_utils.js
printf '%s\n' '--- direct role identity_type consumers ---'
rg -n --glob '*.js' \
"identity_type|role_data\.type|iam_role\.type|iam_role\.identity_type|type[[:space:]]*:[[:space:]]*['\"]role['\"]" \
src/sdk src/server src/manage_nsfs src/test
printf '%s\n' '--- role data construction and persistence call graph ---'
rg -n -C 4 --glob '*.js' \
"create_role_config_file|update_role_config_file|get_role_by_name|read_role_by_name|_new_role_defaults" \
src/sdk src/server src/manage_nsfs src/test | head -n 700Repository: noobaa/noobaa-core Length of output: 30451 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- strict schema behavior ---'
rg -n -C 12 --glob '*.js' \
"function strictify|strictify[[:space:]]*=|additionalProperties[[:space:]]*:[[:space:]]*false" \
src node_modules 2>/dev/null | head -n 300
printf '%s\n' '--- current change and prior role default ---'
git status --short
git diff -- src/sdk/accountspace_fs.js
git show HEAD:src/sdk/accountspace_fs.js 2>/dev/null | sed -n '935,955p' || true
printf '%s\n' '--- legacy role-shaped fixtures and type fields ---'
rg -n -C 4 --glob '*.js' --glob '*.json' \
"(dummy_role|role_data|role_config_file|identity_type|^[[:space:]]*type[[:space:]]*:)" \
src/test/unit_tests/nsfs src/test/integration_tests/nc/cli | head -n 500Repository: noobaa/noobaa-core Length of output: 20148 Handle legacy role files before persisting When an existing role contains 🤖 Prompt for AI Agents |
||
| owner: requesting_account._id, | ||
| iam_path: params.iam_path || IAM_DEFAULT_PATH, | ||
| creation_date: new Date().toISOString(), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to have
ldap-provider? For keycloack we are usingoidc-providerwhich common for all the identity provider. Not sure each should have different oneThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LDAP matching is different from OIDC matching — we compare
<host>[:port]afterldap-provider/to the configured LDAP URI.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@naveenpaul1 I can see in AWS that a connector to oidc is per account? What is our plan? Do you use the account as part of the Prinicipal? https://docs.aws.amazon.com/IAM/latest/APIReference/API_CreateOpenIDConnectProvider.html#:~:text=The%20URL%20of%20the%20identity,Connect%20ID%20tokens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just see there that we don't suppose to have ports and thing like that... maybe we should align better to this document for both OIDC and LDAP. I agree with @sakshimunjal that we should use LDAP outside of OIDC.