package.json (1)

103-103: Adding ldapts dependency: confirm suitability and security posture

• Sanity-check ldapts 7.3.1 compatibility with your Node.js/runtime and run a quick vuln/license scan.
• Longer term, consider native OpenLDAP for performance and reduced JS attack surface (kept as a Next Step in the MD).

Use this script to check latest version and known advisories:

#!/bin/bash
set -euo pipefail

echo "Latest ldapts version on npm:"
npm view ldapts version

echo "All ldapts versions in the 7.x line:"
npm view ldapts versions --json | jq -r '.[]' | grep -E '^7\.'

echo "Basic advisory search (GitHub security advisories for ldapts):"
gh api graphql -f query='
{
  securityVulnerabilities(first: 10, ecosystem: NPM, package: "ldapts") {
    nodes {
      advisory { ghsaId summary severity publishedAt }
      vulnerableVersionRange
      firstPatchedVersion { identifier }
    }
  }
}'

docs/design/ldap.md (1)

95-101: “Next steps”: track OpenLDAP evaluation and add system test plan

• Keep the OpenLDAP-native evaluation item (performance and security benefits). This echoes prior review feedback.
• Consider adding a matrix test (OpenLDAP and AD) end-to-end flow using docker-compose with TLS.

I can draft a docker-compose and a system test plan if helpful.

src/util/ldap_client.js (2)

14-19: Await unbind and guard admin_client on disconnect.

Unbinding in ldapts is async. Not awaiting can cause races; also guard against undefined admin_client.

Apply:

 async disconnect() {
     dbg.log0('ldap client disconnect called');
     this._disconnected_state = true;
     this._connect_promise = null;
-    this.admin_client.unbind();
+    if (this.admin_client) {
+        try {
+            await this.admin_client.unbind();
+        } catch (e) {
+            dbg.warn('ldap client unbind failed', e?.message || e);
+        }
+    }
 }

---

21-25: Await disconnect in reconnect to avoid races.

Reconnect should await the disconnect before connecting again.

 async reconnect() {
     dbg.log0(`reconnect called`);
-    this.disconnect();
-    return this.connect();
+    await this.disconnect();
+    return this.connect();
 }

src/endpoint/sts/sts_errors.js (1)

29-36: Enhance STS Error Envelope with Type and Conditional Detail

Refine the STS error reply in src/endpoint/sts/sts_errors.js to align with AWS Query conventions:

– Infer and include Type (“Sender” for 4xx, “Receiver” for 5xx)
– Omit Detail when it’s undefined
– Only attach Resource when non-empty

Apply this patch around the existing reply(resource, request_id):

--- a/src/endpoint/sts/sts_errors.js
+++ b/src/endpoint/sts/sts_errors.js
@@ reply(resource, request_id) {
-        const xml = {
-            ErrorResponse: {
-                Error: {
-                    Code: this.code,
-                    Message: this.message,
-                    Resource: resource || '',
-                    Detail: this.detail,
-                },
-                RequestId: request_id || '',
-            }
-        };
+        // Build the Error block per AWS Query API
+        const err = {
+            Code: this.code,
+            Message: this.message,
+            Type: this.http_code >= 500 ? 'Receiver' : 'Sender',
+        };
+        if (resource)    err.Resource = resource;
+        if (this.detail !== undefined) err.Detail   = this.detail;
+
+        const xml = {
+            ErrorResponse: {
+                Error: err,
+                RequestId: request_id || '',
+            },
+        };

• No integration tests reference Detail, and STS error tests only assert on the error code.
• Existing tests that mention Resource apply to successful responses and aren’t affected.

This change is optional but recommended for full AWS STS compatibility.

docs/design/ldap.md (2)

90-90: Replace bare URL with a link label for markdownlint compliance

Wrap the URL in markdown syntax:

-read more here: https://docs.aws.amazon.com/cli/latest/reference/sts/assume-role-with-web-identity.html
+Read more here: [AWS CLI assume-role-with-web-identity](https://docs.aws.amazon.com/cli/latest/reference/sts/assume-role-with-web-identity.html)

---

105-105: Heading trailing colon and minor grammar

Remove trailing colon per MD026 and adjust casing:

-## Appendix A: Creating account w/ role config using NooBaa API:
+## Appendix A: Creating an account with role config using the NooBaa API

src/test/integration_tests/api/sts/test_sts.js (1)

894-952: Typo: “indentity” → “identity” in suite title

Fix the spelling in the describe block.

Apply this diff:

-mocha.describe('Assume role with web indentity tests', function() {
+mocha.describe('Assume role with web identity tests', function() {

Optional: Guard tests when LDAP is not configured

To avoid flakiness in environments without LDAP config/jwt_secret, skip this suite when LDAP isn’t configured/connected.

Apply this diff within the existing before() hook:

 mocha.before(async function() {
   const self = this; // eslint-disable-line no-invalid-this
   self.timeout(60000);
 
-  // const random_access_keys = cloud_utils.generate_access_keys();
+  // Skip if LDAP is not configured/connected
+  try {
+    if (!(await ldap_client.is_ldap_configured()) || !ldap_client.instance().is_connected()) {
+      this.skip(); // eslint-disable-line no-invalid-this
+      return;
+    }
+  } catch (e) {
+    this.skip(); // eslint-disable-line no-invalid-this
+    return;
+  }
+
   anon_sts = new AWS.STS({

src/endpoint/sts/sts_utils.js (2)

16-18: Nit: Debug message refers to old module/function

Update the log prefix to reflect the current module/function name.

Apply this diff:

-    dbg.log1('sts_post_assume_role.make_session_token: ', auth_options, expiry);
+    dbg.log1('sts_utils.generate_session_token: ', auth_options, expiry);

---

20-21: Nit: Outdated TODO

This module already is the generalized utils file. Remove or reword the TODO.

Apply this diff:

-// TODO: Generalize and move to a utils file in the future
+// STS utility helpers for duration parsing and session token generation

src/sdk/sts_sdk.js (3)

98-105: Use err.name to detect expired JWTs

jsonwebtoken sets err.name === 'TokenExpiredError' and err.message is typically “jwt expired”. Checking err.message for “TokenExpiredError” will not trigger correctly.

Apply this diff:

-            } catch (err) {
+            } catch (err) {
                 dbg.error('get_assumed_ldap_user error: JWT token verification failed', err);
-                if (err.message.includes('TokenExpiredError')) {
+                if (err && err.name === 'TokenExpiredError') {
                     throw new RpcError('EXPIRED_WEB_IDENTITY_TOKEN', err.message);
                 } else {
                     throw new RpcError('INVALID_WEB_IDENTITY_TOKEN', err.message);
                 }
             }

---

107-109: Fail closed when JWT secret is not configured

Decoding without verification accepts untrusted tokens. Since LDAP auth still validates user/password, the risk is lower, but best practice is to reject un-verifiable tokens to prevent abuse and ambiguity.

Apply this diff:

-        } else {
-            dbg.warn('get_assumed_ldap_user error: No LDAP JWT secret found');
-            web_token = jwt.decode(req.body.web_identity_token);
-        }
+        } else {
+            dbg.error('get_assumed_ldap_user error: No LDAP JWT secret found');
+            throw new RpcError('INVALID_WEB_IDENTITY_TOKEN', 'JWT secret not configured');
+        }

If you intentionally allow decode-only in dev, consider gating it behind an explicit config flag.

---

56-75: Optional: Avoid re-parsing RoleArn in multiple places

Have _assume_role return access_key and role_name with the account to reduce duplication and subtle parsing inconsistencies.

Apply this refactor:

 async _assume_role(role_arn) {
-        const role_name_idx = role_arn.lastIndexOf('/') + 1;
-        const role_name = role_arn.slice(role_name_idx);
-        const access_key = role_arn.split(':')[4];
+        const role_name_idx = role_arn.lastIndexOf('/') + 1;
+        const role_name = role_arn.slice(role_name_idx);
+        const access_key = role_arn.split(':')[4];
 
         const account = await account_cache.get_with_cache({
             bucketspace: this._get_bucketspace(),
             access_key,
         });
         if (!account) {
             throw new RpcError('NO_SUCH_ACCOUNT', 'No such account with access_key: ' + access_key);
         }
         if (!account.role_config || account.role_config.role_name !== role_name) {
             throw new RpcError('NO_SUCH_ROLE', `Role not found`);
         }
-        dbg.log0('sts_sdk.get_assumed_role res', account,
-            'account.role_config: ', account.role_config);
-
-        return account;
+        return { account, access_key, role_name };
 }
 
 async get_assumed_role(req) {
     dbg.log1('sts_sdk.get_assumed_role body', req.body);
     // arn:aws:sts::access_key:role/role_name
-    const account = await this._assume_role(req.body.role_arn);
+    const { account, access_key } = await this._assume_role(req.body.role_arn);
 
     return {
-        access_key: req.body.role_arn.split(':')[4],
+        access_key,
         role_config: account.role_config
     };
 }
 
 ...
-        const account = await this._assume_role(req.body.role_arn);
+        const { account, access_key } = await this._assume_role(req.body.role_arn);
 ...
     return {
-        access_key: req.body.role_arn.split(':')[4],
+        access_key,
         role_config: account.role_config,
         dn,
     };

Also applies to: 77-86, 131-135

src/endpoint/sts/ops/sts_post_assume_role_with_web_identity.js (2)

36-56: Add defensive error handling around temporary key generation.

If generate_temp_access_keys fails, we’ll return a generic InternalFailure from the outer try/catch. Consider catching and mapping to a clearer STS error (e.g., ServiceFailure) to aid troubleshooting.

Example:

-    const access_keys = await req.sts_sdk.generate_temp_access_keys();
+    let access_keys;
+    try {
+        access_keys = await req.sts_sdk.generate_temp_access_keys();
+    } catch (e) {
+        dbg.error('failed generating temp access keys', e);
+        throw new StsError(StsError.InternalFailure);
+    }

---

38-61: Consider including ResponseMetadata (RequestId) to match STS responses.

Not mandatory if your clients don’t rely on it, but adding ResponseMetadata with a request id aligns with AWS STS and helps debugging.

src/util/ldap_client.js (3)

27-33: Async in constructor: explicitly fire-and-forget and handle errors.

load_ldap_config is async; calling it without handling may hide errors. Either mark intention or restructure.

-        this.load_ldap_config();
+        // fire-and-forget initial load; subsequent connect() will ensure readiness
+        void this.load_ldap_config();

---

73-75: is_connected is a method; ensure it’s invoked consistently.

Current code mixes property/method usage. Keep it as a method for consistency.

---

99-106: Avoid unbind on bind failure for admin_client; it may already be disconnected.

Unbinding on bind failure is fine, but handle errors and don’t assume an open connection.

 async _bind(client, user, password) {
     try {
         await client.bind(user, password);
     } catch (err) {
-        await client.unbind();
+        try { await client.unbind(); } catch (_) { /* ignore */ }
         throw err;
     }
 }

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

src/util/ldap_client.js (7)

85-97: Connect retry loop should respect disconnect state

Make the loop exit when disconnecting to avoid endless retries.

---

35-63: Fix defaults, accept both config key variants, secure TLS, and call is_connected()

• Default URI is inconsistent: ldap:// with 636. Use ldaps://:636 or ldap://:389 (prefer ldaps).
• Accept both new and legacy keys: admin/secret and admin_user/admin_password; search_dn and search_base.
• Default to TLS verification enabled.
• The is_connected check should call the method.

     const params = JSON.parse(fs.readFileSync(config.LDAP_CONFIG_PATH).toString());
     this.ldap_params = {
-        uri: params.uri || 'ldap://127.0.0.1:636',
-        admin: params.admin_user || 'Administrator',
-        secret: params.admin_password || 'Passw0rd',
-        search_dn: params.search_dn || 'ou=people,dc=example,dc=com',
+        uri: params.uri || 'ldaps://127.0.0.1:636',
+        // Accept both legacy and new keys
+        admin: params.admin ?? params.admin_user ?? 'Administrator',
+        secret: params.secret ?? params.admin_password ?? 'Passw0rd',
+        // Accept both search_base and search_dn
+        search_dn: params.search_dn ?? params.search_base ?? 'ou=people,dc=example,dc=com',
         dn_attribute: params.dn_attribute || 'uid', // for LDAP 'sAMAccountName' for AD
         // search_filter: params.search_filter || '(uid=%s)',
         search_scope: params.search_scope || 'sub',
         jwt_secret: params.jwt_secret,
         ...params,
     };
     this.tls_options = this.ldap_params.tls_options || {
-        'rejectUnauthorized': false,
+        rejectUnauthorized: true,
     };
     this.admin_client = new ldap_client.Client({
         url: this.ldap_params.uri,
         tlsOptions: this.tls_options,
     });
-    if (this.is_connected) {
-        await this.reconnect();
-    }
+    if (this.is_connected()) {
+        await this.reconnect();
+    }

---

50-52: Defaulting rejectUnauthorized: false weakens TLS security

Please keep certificate verification on by default and allow explicit override via config only.

---

80-81: Do not log secrets within ldap_params

Avoid logging ldap_params wholesale; it includes admin credentials and jwt_secret.

---

7-7: Fix incorrect config import path (module will fail to load).

From src/util/ldap_client.js, config is one level up.

-const config = require('../../config');
+const config = require('../config');

---

77-83: connect(): ensure config loaded and avoid logging secrets

Initialize config/client if missing and log a redacted subset only.

 async connect() {
     this._disconnected_state = false;
     if (this._connect_promise) return this._connect_promise;
-    dbg.log0('connect called, current url', this.ldap_params);
+    if (!this.admin_client) {
+        await this.load_ldap_config();
+    }
+    const { uri, search_dn, search_scope } = this.ldap_params || {};
+    dbg.log0('connect called', { uri, search_dn, search_scope });
     this._connect_promise = this._connect();
     return this._connect_promise;
 }

---

108-125: authenticate(): prevent LDAP injection, ensure admin connected, and unbind user client

Escape filter values, ensure admin is connected, and always unbind the user client.

 async authenticate(user, password) {
-    /** @type {ldap_client.SearchOptions}*/
-    const search_options = {
-        filter: `(${this.ldap_params.dn_attribute}=${user})`,
-        scope: this.ldap_params.search_scope,
-        attributes: ['dn']
-    };
-    const user_client = new ldap_client.Client({
-        url: this.ldap_params.uri,
-        tlsOptions: this.tls_options,
-    });
-    const { searchEntries } = await this.admin_client.search(this.ldap_params.search_dn, search_options);
-    if (!searchEntries || searchEntries.length === 0) {
-        throw new Error('User not found');
-    }
-    await this._bind(user_client, searchEntries[0].dn, password);
-    return searchEntries[0].dn;
+    // RFC2254-style escaping of filter meta-characters
+    const escape = v => String(v)
+        .replace(/\\/g, '\\5c')
+        .replace(/\*/g, '\\2a')
+        .replace(/\(/g, '\\28')
+        .replace(/\)/g, '\\29')
+        .replace(/\0/g, '\\00')
+        .replace(/\//g, '\\2f');
+    /** @type {ldap_client.SearchOptions}*/
+    const search_options = {
+        filter: `(${this.ldap_params.dn_attribute}=${escape(user)})`,
+        scope: this.ldap_params.search_scope,
+        attributes: ['dn']
+    };
+    if (!this.is_connected()) {
+        await this.connect();
+    }
+    const user_client = new ldap_client.Client({
+        url: this.ldap_params.uri,
+        tlsOptions: this.tls_options,
+    });
+    try {
+        const { searchEntries } = await this.admin_client.search(this.ldap_params.search_dn, search_options);
+        if (!searchEntries || searchEntries.length === 0) {
+            throw new Error('User not found');
+        }
+        await this._bind(user_client, searchEntries[0].dn, password);
+        return searchEntries[0].dn;
+    } finally {
+        try { await user_client.unbind(); } catch (_) { /* ignore */ }
+    }
 }

docs/design/ldap.md (5)

80-80: CLI example: fix typo and read token from file to avoid leaks

Prevent exposure in history/process list; also fix “endpint”.

-aws sts assume-role-with-web-identity --endpoint <endpint> --role-arn [role-arn] --role-session-name [session-name] --web-identity-token eyJhbGciOiJub25lIiwidHlwIjoiSldUIn0.eyJ1c2VyIjoiVGhlVXNlck5hbWUiLCJwYXNzd29yZCI6IlRoZVVzZXJQYXNzd29yZCIsInR5cGUiOiJsZGFwIiwiaWF0IjoxNzU1MTgxOTE3fQ.
+echo '<JWT>' > /tmp/web-identity.jwt
+chmod 600 /tmp/web-identity.jwt
+aws sts assume-role-with-web-identity --endpoint <endpoint> \
+  --role-arn [role-arn] \
+  --role-session-name [session-name] \
+  --web-identity-token file:///tmp/web-identity.jwt

---

9-13: Do not recommend unsigned/constant-signature JWTs; require signed tokens with standard claims

Accepting unsigned tokens or a “constant signature” is unsafe.

-1. The client sends a signed(with a predfined constant signature) or unsigned JWT as the web identity token.
+1. The client sends a signed JWT (HS256/RS256/ES256) as the web identity token. Unsigned tokens (alg="none") are not allowed.
+   The token MUST include exp (short, e.g., <= 5 minutes), iss, aud (NooBaa STS endpoint), and jti (nonce). The server validates signature and claims.

---

41-41: Make jwt_secret mandatory and document secure storage

JWTs must be signed; treat jwt_secret as required and advise 0600 permissions.

-jwt_secret (Optional) - The JWT secret the administrator will use to sign the token sent in AssumeRoleWithWebIdentity requests (default: unsigned). Once this option is set - unsigned tokens or tokens signed with different secret will be dropped as access denied.
+jwt_secret (Required) – Secret (for HS256) or private key (for RS/ES) used to sign JWTs for AssumeRoleWithWebIdentity.
+All tokens without a valid signature are rejected. Store this secret securely (file mode 0600, owned by the NooBaa process user).

---

68-76: Fix JWT examples: remove alg='none', add exp/aud/iss/jti, and avoid real-looking tokens

The current examples are inconsistent and trip secret scanners.

-const jwt = require('jsonwebtoken');
-console.log(jwt.sign({ user: "TheUserName", password: "TheUserPassword", type: "ldap" }, "IAMTHEADMIN123!(SHOULDBE256BITS)"));
-eyJhbGciOiJub25lIiwidHlwIjoiSldUIn0.eyJ1c2VyIjoiVGhlVXNlck5hbWUiLCJwYXNzd29yZCI6IlRoZVVzZXJQYXNzd29yZCIsInR5cGUiOiJsZGFwIiwiaWF0IjoxNzU1MTgxOTE3fQ.
+const jwt = require('jsonwebtoken');
+const payload = {
+  user: "TheUserName",
+  type: "ldap",
+  iss: "noobaa-ldap",
+  aud: "https://<endpoint-host>/sts/",
+  exp: Math.floor(Date.now() / 1000) + 300,
+  jti: "<random-nonce>"
+};
+console.log(jwt.sign(payload, "<HS256-SECRET-256-BIT>", { algorithm: 'HS256' }));
-Or you can use 'none' algorithm if you are not interested with verifying the token
-```js
-const jwt = require('jsonwebtoken');
-console.log(jwt.sign({ user: "TheUserName", password: "TheUserPassword", type: "ldap" }, undefined, { algorithm: 'none' }));
-eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJ1c2VyIjoiVGhlVXNlck5hbWUiLCJwYXNzd29yZCI6IlRoZVVzZXJQYXNzd29yZCIsInR5cGUiOiJsZGFwIiwiaWF0IjoxNzU1MTgyMzQ2fQ.P6WYcdM0kJagNK4D0M8AHiGFcUZ-DhTOKHlC1-AxcT0
-```
+// Example output (redacted): eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.<redacted>.<redacted>

---

60-64: Avoid embedding plaintext passwords in JWT; standardize type casing

Remove password from the JWT payload to reduce leakage risk and use type: "ldap" consistently.

 {
- "user": "TheUserName",
- "password": "TheUserPassword",
- "type": "LDAP"
+ "user": "TheUserName",
+ "type": "ldap"
 }

If transporting credentials is unavoidable, require HTTPS and use JWE (encrypted JWT) with exp/jti. I can provide a jose-based JWE example if desired.

src/endpoint/sts/sts_errors.js (1)

29-37: Add “Type” to STS ErrorResponse and verify consumers

In src/endpoint/sts/sts_errors.js (lines 29–37), inject the same Type field used by IAM/S3:

         const xml = {
             ErrorResponse: {
                 Error: {
+                    Type: this.http_code >= 500 ? 'Receiver' : 'Sender',
                     Code: this.code,
                     Message: this.message,
                     Resource: resource || '',
                     Detail: this.detail,
                 },
                 RequestId: request_id || '',
             }
         };

Please also:

• Review any STS XML parsers or client code (including tests under test/ or integration suites) to ensure they handle the nested ErrorResponse with the new Type field.
• Compare with src/endpoint/iam/iam_errors.js and src/endpoint/s3/s3_errors.js for consistent structure.

src/endpoint/sts/ops/sts_post_assume_role.js (1)

19-26: Map get_assumed_role errors; add server-side logging of root cause

Mapping ACCESS_DENIED is good. Log the original error before returning a generic STS error to aid debugging without exposing details to clients.

     try {
         assumed_role = await req.sts_sdk.get_assumed_role(req);
     } catch (err) {
+        dbg.error('AssumeRole: get_assumed_role failed', err);
         if (err.code === 'ACCESS_DENIED') {
             throw new StsError(StsError.AccessDeniedException);
         }
         throw new StsError(StsError.InternalFailure);
     }

src/util/ldap_client.js (3)

4-11: Use local util imports consistently

Within src/util, prefer local requires to avoid brittle ../util/… paths.

-require('../util/fips');
+require('./fips');
...
-const P = require('../util/promise');
+const P = require('./promise');

---

27-33: Constructor: don’t fire-and-forget async; capture errors

You can’t await in a constructor, but at least attach a catch to avoid unhandled rejections.

 constructor() {
     super();
-    this.load_ldap_config();
+    void this.load_ldap_config().catch(err => dbg.warn('load_ldap_config failed during init', err));
     fs.watchFile(config.LDAP_CONFIG_PATH, {
         interval: config.NC_RELOAD_CONFIG_INTERVAL
-    }, () => this.load_ldap_config()).unref();
+    }, () => { void this.load_ldap_config().catch(err => dbg.warn('load_ldap_config failed on change', err)); }).unref();
 }

---

85-97: _connect(): make retry loop cancellable and robust

Observe _disconnected_state, validate admin_client presence, and avoid tight retrying after disconnect.

 async _connect() {
     let is_connected = false;
-    while (!is_connected) {
+    while (!is_connected && !this._disconnected_state) {
         try {
-            await this._bind(this.admin_client, this.ldap_params.admin, this.ldap_params.secret);
+            if (!this.admin_client) throw new Error('LDAP admin client not initialized');
+            await this._bind(this.admin_client, this.ldap_params.admin, this.ldap_params.secret);
             dbg.log0('_connect: initial connect succeeded');
             is_connected = true;
         } catch (err) {
-            dbg.error('_connect: initial connect failed, will retry', err.message);
-            await P.delay(3000);
+            dbg.error('_connect: initial connect failed, will retry', err);
+            if (this._disconnected_state) break;
+            await P.delay(3000);
         }
     }
 }

docs/design/ldap.md (1)

90-97: Markdown lint: avoid bare URLs

Minor doc polish to satisfy linters.

-read more here: https://docs.aws.amazon.com/cli/latest/reference/sts/assume-role-with-web-identity.html
+Read more here: <https://docs.aws.amazon.com/cli/latest/reference/sts/assume-role-with-web-identity.html>
...
- (https://github.com/samba-in-kubernetes/samba-container)
+ (<https://github.com/samba-in-kubernetes/samba-container>)
...
- (https://github.com/ldapjs/docker-test-openldap/pkgs/container/docker-test-openldap%2Fopenldap)
+ (<https://github.com/ldapjs/docker-test-openldap/pkgs/container/docker-test-openldap%2Fopenldap>)

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9042
File: src/util/cloud_utils.js:183-194
Timestamp: 2025-06-17T12:59:51.543Z
Learning: In the set_noobaa_s3_connection function in src/util/cloud_utils.js, internal NooBaa S3 endpoints (filtered by 'api': 's3', 'kind': 'INTERNAL') will always be non-secure, so tls: false is the correct setting despite the conditional https:// scheme logic.

package.json (1)

103-103: New ldapts runtime dependency: validate necessity, footprint, and maintenance tradeoffs

Adding ldapts looks reasonable for a POC. Before merging to master, evaluate:

• Is ldapts the long-term choice vs. native OpenLDAP C client (perf/stability)?
• Runtime footprint and transitive deps implications for packaging (pkg) and minimal images.

Do you want me to draft a short RFC comparing ldapts vs. OpenLDAP C client (API surface, TLS options, reconnect semantics, perf, bundle size)?

src/endpoint/endpoint.js (1)

253-255: Non-blocking LDAP connect lacks error handling; attach a catch to avoid unhandled rejections

You correctly avoided awaiting the connect. However, if connect() rejects, it will surface as an unhandled rejection. Log-and-continue to keep startup resilient.

Apply this diff:

-        if (await ldap_client.is_ldap_configured()) {
-            ldap_client.instance().connect();
-        }
+        if (await ldap_client.is_ldap_configured()) {
+            // Start LDAP connect in background; don't block startup. Log failures.
+            ldap_client.instance().connect()
+                .catch(e => dbg.warn(
+                    'LDAP configured but failed to connect; continuing without LDAP',
+                    e?.message || e
+                ));
+        }

docs/design/ldap.md (6)

9-13: Do not allow unsigned or “constant signature” JWTs; require signed tokens with standard claims

Allowing unsigned tokens or a “constant signature” is a critical security risk and enables trivial impersonation/replay. Require signed JWTs and validate iss/aud/exp (short), and jti.

Apply:

-1. The client sends a signed(with a predfined constant signature) or unsigned JWT as the web identity token.
+1. The client sends a signed JWT (HS256/RS256/ES256) as the web identity token. Unsigned tokens (alg="none") are not allowed.
+   The token MUST include standard claims (iss, aud, exp<=5m) and jti (nonce). The server validates signature and all claims.

---

41-41: Make jwt_secret mandatory; never default to unsigned tokens

jwt_secret must be required (or a public key when using asymmetric algs). Reject all tokens without a valid signature. Document secure storage/permissions.

-jwt_secret (Optional) - The JWT secret the administrator will use to sign the token sent in AssumeRoleWithWebIdentity requests (default: unsigned). Once this option is set - unsigned tokens or tokens signed with different secret will be dropped as access denied.
+jwt_secret (Required) – Secret (for HS256) or private key (for RS/ES) used to sign JWTs for AssumeRoleWithWebIdentity.
+All tokens without a valid signature are rejected. Store the secret/key securely (file mode 0600, owned by the NooBaa user/service).

---

60-64: Do not embed plaintext passwords in JWT payloads

Placing a user’s password in a bearer token risks leakage via logs, proxies, and browser/shell history. Perform LDAP bind using credentials delivered over TLS at request time, not re-encoded into a token. If you must transport, use JWE and short exp+jti.

-{
- "user": "TheUserName",
- "password": "TheUserPassword",
- "type": "ldap"
-}
+{
+  "user": "TheUserName",
+  "type": "ldap",
+  "iss": "noobaa-ldap",
+  "aud": "https://<endpoint-host>/sts/",
+  "exp": <now+300s>,
+  "jti": "<random-uuid>"
+}

Would you like me to provide a JWE-based example using jose and server-side decryption?

---

68-76: Remove 'alg: none' example; show a single signed example with standard claims

The current examples are inconsistent and encourage insecure practices.

-const jwt = require('jsonwebtoken');
-console.log(jwt.sign({ user: "TheUserName", password: "TheUserPassword", type: "ldap" }, "IAMTHEADMIN123!(SHOULDBE256BITS)"));
-eyJhbGciOiJub25lIiwidHlwIjoiSldUIn0.eyJ1c2VyIjoiVGhlVXNlck5hbWUiLCJwYXNzd29yZCI6IlRoZVVzZXJQYXNzd29yZCIsInR5cGUiOiJsZGFwIiwiaWF0IjoxNzU1MTgxOTE3fQ.
-...
-Or you can use 'none' algorithm if you are not interested with verifying the token
-```js
-const jwt = require('jsonwebtoken');
-console.log(jwt.sign({ user: "TheUserName", password: "TheUserPassword", type: "ldap" }, undefined, { algorithm: 'none' }));
-...
-```
+const jwt = require('jsonwebtoken');
+const payload = {
+  user: "TheUserName",
+  type: "ldap",
+  iss: "noobaa-ldap",
+  aud: "https://<endpoint-host>/sts/",
+  exp: Math.floor(Date.now()/1000) + 300,
+  jti: "<random-uuid>"
+};
+console.log(jwt.sign(payload, "IAMTHEADMIN123!(USE_256_BITS_MIN)", { algorithm: 'HS256' }));

---

80-81: Avoid passing the token inline; use file:// to prevent leaks via shell history/process list

Also fix the “endpoint” typo.

-aws sts assume-role-with-web-identity --endpoint [endpoint] --role-arn [role-arn] --role-session-name [session-name] --web-identity-token eyJhbGciOiJub25lIiwidHlwIjoiSldUIn0.eyJ1c2VyIjoiVGhlVXNlck5hbWUiLCJwYXNzd29yZCI6IlRoZVVzZXJQYXNzd29yZCIsInR5cGUiOiJsZGFwIiwiaWF0IjoxNzU1MTgxOTE3fQ.
+echo '<jwt>' > /tmp/web-identity.jwt
+chmod 600 /tmp/web-identity.jwt
+aws sts assume-role-with-web-identity \
+  --endpoint <endpoint> \
+  --role-arn <ROLE_ARN> \
+  --role-session-name <SESSION_NAME> \
+  --web-identity-token file:///tmp/web-identity.jwt

---

134-141: Redact secrets/tokens in examples to avoid tripping secret scanners

Replace real-looking keys/JWTs with placeholders.

-    "AccessKeyId": "vTFcHUuPqjKdtzTsMULP",
-    "SecretAccessKey": "qBQxFcx99JOlABkZNq2fNRj6qGe18F9IREcHf9DZ",
-    "SessionToken": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9....",
-    "Expiration": "2025-08-17T14:03:18+00:00"
+    "AccessKeyId": "<ACCESS_KEY_ID>",
+    "SecretAccessKey": "<SECRET_ACCESS_KEY>",
+    "SessionToken": "<SESSION_TOKEN>",
+    "Expiration": "<EXPIRATION_ISO8601>"

src/sdk/sts_sdk.js (2)

71-73: Avoid logging full account objects; redact to non-sensitive identifiers

Logging account can leak secrets. Log only access_key and role_name.

-        dbg.log0('sts_sdk.get_assumed_role res', account,
-            'account.role_config: ', account.role_config);
+        dbg.log0('sts_sdk.get_assumed_role res', {
+            access_key,
+            role_name: account.role_config?.role_name
+        });

---

128-130: Avoid logging full account in web identity path

Same concern as above; log only safe identifiers.

-        dbg.log0('sts_sdk.get_assumed_role_with_web_identity res', account,
-            'account.role_config: ', account.role_config);
+        dbg.log0('sts_sdk.get_assumed_role_with_web_identity res', {
+            access_key: req.body.role_arn.split(':')[4],
+            role_name: account.role_config?.role_name
+        });

src/util/ldap_client.js (5)

21-25: reconnect(): await disconnect before connecting

Avoid racing a new connect with an in-flight disconnect.

 async reconnect() {
     dbg.log0(`reconnect called`);
-    this.disconnect();
-    return this.connect();
+    await this.disconnect();
+    return this.connect();
 }

---

84-96: Make retry loop cancellable and guard uninitialized client

Observe _disconnected_state and protect against missing admin_client.

 async _connect() {
     let is_connected = false;
-    while (!is_connected) {
+    while (!is_connected && !this._disconnected_state) {
         try {
-            await this._bind(this.admin_client, this.ldap_params.admin, this.ldap_params.secret);
+            if (!this.admin_client) throw new Error('LDAP admin client not initialized');
+            await this._bind(this.admin_client, this.ldap_params.admin, this.ldap_params.secret);
             dbg.log0('_connect: initial connect succeeded');
             is_connected = true;
         } catch (err) {
-            dbg.error('_connect: initial connect failed, will retry', err.message);
-            await P.delay(3000);
+            dbg.error('_connect: initial connect failed, will retry', err?.message || err);
+            if (this._disconnected_state) break;
+            await P.delay(3000);
         }
     }
 }

---

107-124: authenticate(): prevent LDAP injection, ensure admin is connected, and always unbind user client

Escape filter values, ensure connection, and release the user client in finally.

 async authenticate(user, password) {
-    /** @type {ldap_client.SearchOptions}*/
-    const search_options = {
-        filter: `(${this.ldap_params.dn_attribute}=${user})`,
-        scope: this.ldap_params.search_scope,
-        attributes: ['dn']
-    };
-    const user_client = new ldap_client.Client({
-        url: this.ldap_params.uri,
-        tlsOptions: this.tls_options,
-    });
-    const { searchEntries } = await this.admin_client.search(this.ldap_params.search_dn, search_options);
-    if (!searchEntries || searchEntries.length === 0) {
-        throw new Error('User not found');
-    }
-    await this._bind(user_client, searchEntries[0].dn, password);
-    return searchEntries[0].dn;
+    const escape = v => String(v)
+        .replace(/\\/g, '\\5c')
+        .replace(/\*/g, '\\2a')
+        .replace(/\(/g, '\\28')
+        .replace(/\)/g, '\\29')
+        .replace(/\0/g, '\\00')
+        .replace(/\//g, '\\2f');
+    /** @type {ldap_client.SearchOptions}*/
+    const search_options = {
+        filter: `(${this.ldap_params.dn_attribute}=${escape(user)})`,
+        scope: this.ldap_params.search_scope,
+        attributes: ['dn']
+    };
+    if (!this.is_connected()) {
+        await this.connect();
+    }
+    const user_client = new ldap_client.Client({
+        url: this.ldap_params.uri,
+        tlsOptions: this.tls_options,
+    });
+    try {
+        const { searchEntries } = await this.admin_client.search(this.ldap_params.search_dn, search_options);
+        if (!searchEntries || searchEntries.length === 0) {
+            throw new Error('User not found');
+        }
+        await this._bind(user_client, searchEntries[0].dn, password);
+        return searchEntries[0].dn;
+    } finally {
+        try { await user_client.unbind(); } catch (_) { /* ignore */ }
+    }
 }

---

14-19: disconnect(): null-safety and await unbind to avoid dangling sockets

Guard against missing client and await unbind.

 async disconnect() {
     dbg.log0('ldap client disconnect called');
     this._disconnected_state = true;
     this._connect_promise = null;
-    this.admin_client.unbind();
+    if (this.admin_client) {
+        try {
+            await this.admin_client.unbind();
+        } catch (_) {
+            // ignore
+        }
+    }
 }

---

35-63: Config load: align keys, secure TLS defaults, and fix is_connected() call

• Accept both admin/secret and admin_user/admin_password; search_base alias for search_dn.
• Default rejectUnauthorized to true.
• Call the method is_connected() (not the property) before reconnect.

             const params = JSON.parse(fs.readFileSync(config.LDAP_CONFIG_PATH).toString());
             this.ldap_params = {
-                uri: params.uri || 'ldaps://127.0.0.1:636',
-                admin: params.admin_user || 'Administrator',
-                secret: params.admin_password || 'Passw0rd',
-                search_dn: params.search_dn || 'ou=people,dc=example,dc=com',
+                uri: params.uri || 'ldaps://127.0.0.1:636',
+                // accept both legacy and new keys
+                admin: (params.admin ?? params.admin_user) || 'Administrator',
+                secret: (params.secret ?? params.admin_password) || 'Passw0rd',
+                // accept both search_dn and search_base
+                search_dn: (params.search_dn ?? params.search_base) || 'ou=people,dc=example,dc=com',
                 dn_attribute: params.dn_attribute || 'uid', // for LDAP 'sAMAccountName' for AD
                 search_scope: params.search_scope || 'sub',
                 jwt_secret: params.jwt_secret,
                 ...params,
             };
             this.tls_options = this.ldap_params.tls_options || {
-                'rejectUnauthorized': false,
+                rejectUnauthorized: true,
             };
             this.admin_client = new ldap_client.Client({
                 url: this.ldap_params.uri,
                 tlsOptions: this.tls_options,
             });
-            if (this.is_connected) {
-                await this.reconnect();
-            }
+            if (this.is_connected && typeof this.is_connected === 'function' && this.is_connected()) {
+                await this.reconnect();
+            }

src/endpoint/endpoint.js (1)

253-255: Consider lazy-connect to avoid mass connection storms from all forks

Every fork calls connect() at startup. Prefer connecting on first LDAP-authenticated request (ldap_client can connect-on-demand) or gate the eager connect to a single worker/primary to reduce load on LDAP.

I can wire a connect-on-demand in ldap_client.instance().authenticate() and remove the eager connect here – want me to?

docs/design/ldap.md (1)

9-9: Nit: fix typos and minor grammar

“predfined” → “predefined”; add space “signed (with a predefined…)”; a few other small grammar nits flagged by linter.

I can push a quick cleanup pass across the doc if helpful.

src/sdk/sts_sdk.js (2)

56-60: Role ARN parsing lacks validation; guard malformed inputs

Split-by-colon/slice may throw or yield wrong indices on malformed role_arn. Prefer a strict pattern and error out clearly.

Example guard:

-        const role_name_idx = role_arn.lastIndexOf('/') + 1;
-        const role_name = role_arn.slice(role_name_idx);
-        const access_key = role_arn.split(':')[4];
+        const m = /^arn:aws:sts::([^:]+):role\/(.+)$/.exec(role_arn || '');
+        if (!m) throw new RpcError('INVALID_ARGUMENT', 'Invalid role_arn format');
+        const [ , access_key, role_name ] = m;

---

117-119: Attempt lazy LDAP connect before denying; current check may spuriously fail

If LDAP is configured but not yet connected, try connecting once before throwing ACCESS_DENIED.

-        if (!(await ldap_client.is_ldap_configured()) || !ldap_client.instance().is_connected()) {
-            throw new RpcError('ACCESS_DENIED', 'LDAP is not configured or not connected');
-        }
+        if (!(await ldap_client.is_ldap_configured())) {
+            throw new RpcError('ACCESS_DENIED', 'LDAP is not configured');
+        }
+        if (!ldap_client.instance().is_connected()) {
+            try {
+                await ldap_client.instance().connect();
+            } catch (e) {
+                throw new RpcError('ACCESS_DENIED', 'LDAP is not connected');
+            }
+        }

src/util/ldap_client.js (1)

76-82: connect(): ensure config is loaded and avoid logging secrets

Load config if admin_client is missing; keep logs non-sensitive.

 async connect() {
     this._disconnected_state = false;
     if (this._connect_promise) return this._connect_promise;
-    dbg.log0('connect called, current url:', this.ldap_params.uri);
+    if (!this.admin_client) {
+        await this.load_ldap_config();
+    }
+    dbg.log0('connect called', { uri: this.ldap_params?.uri });
     this._connect_promise = this._connect();
     return this._connect_promise;
 }

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9042
File: src/util/cloud_utils.js:183-194
Timestamp: 2025-06-17T12:59:51.543Z
Learning: In the set_noobaa_s3_connection function in src/util/cloud_utils.js, internal NooBaa S3 endpoints (filtered by 'api': 's3', 'kind': 'INTERNAL') will always be non-secure, so tls: false is the correct setting despite the conditional https:// scheme logic.

docs/design/ldap.md (6)

9-13: Do not allow unsigned JWTs; require signatures and validate standard claims

Accepting unsigned tokens or “constant signature” is a critical auth bypass. Enforce signed tokens and validate iss/aud/exp/jti.

Apply:

-1. The client sends a signed(with a predfined constant signature) or unsigned JWT as the web identity token.
-2. The system parses the JWT to extract the LDAP username and password.
+1. The client sends a signed JWT as the web identity token. Unsigned tokens (alg="none") are rejected.
+   Accepted algorithms: HS256 (shared secret) or RS/ES (public key). The token MUST include: iss, aud, exp (<= 5 minutes), and jti (nonce).
+2. The system validates the JWT signature and claims (iss, aud, exp, jti), and extracts the user identity.

---

41-53: Make jwt_secret mandatory; document secure storage and permissions

JWTs must be signed. Treating jwt_secret as optional is unsafe.

Apply:

-jwt_secret (Optional) - The JWT secret the administrator will use to sign the token sent in AssumeRoleWithWebIdentity requests (default: unsigned). Once this option is set - unsigned tokens or tokens signed with different secret will be dropped as access denied.
+jwt_secret (Required) – Secret (for HS256) or public/private key (for RS/ES) used to verify/sign JWTs for AssumeRoleWithWebIdentity.
+Unsigned tokens are rejected. Store this secret/key securely (file mode 0600, owned by the noobaa service user; consider a secrets manager).

Also update the example to indicate ldaps:

-  "uri": "ldap://ldap.example.com:636",
+  "uri": "ldaps://ldap.example.com:636",

---

68-76: Fix JWT examples and remove alg='none'

The displayed token/header contradicts the signing call and recommending alg='none' is insecure. Show a single HS256 example with standard claims; remove the 'none' example.

Apply:

-const jwt = require('jsonwebtoken');
-console.log(jwt.sign({ user: "TheUserName", password: "TheUserPassword", type: "ldap" }, "IAMTHEADMIN123!(SHOULDBE256BITS)"));
-eyJhbGciOiJub25lIiwidHlwIjoiSldUIn0.eyJ1c2VyIjoiVGhlVXNlck5hbWUiLCJwYXNzd29yZCI6IlRoZVVzZXJQYXNzd29yZCIsInR5cGUiOiJsZGFwIiwiaWF0IjoxNzU1MTgxOTE3fQ.
-```
-Or you can use 'none' algorithm if you are not interested with verifying the token
-```js
-const jwt = require('jsonwebtoken');
-console.log(jwt.sign({ user: "TheUserName", password: "TheUserPassword", type: "ldap" }, undefined, { algorithm: 'none' }));
-eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJ1c2VyIjoiVGhlVXNlck5hbWUiLCJwYXNzd29yZCI6IlRoZVVzZXJQYXNzd29yZCIsInR5cGUiOiJsZGFwIiwiaWF0IjoxNzU1MTgyMzQ2fQ.P6WYcdM0kJagNK4D0M8AHiGFcUZ-DhTOKHlC1-AxcT0
+const jwt = require('jsonwebtoken');
+const payload = {
+  user: "TheUserName",
+  type: "ldap",
+  iss: "noobaa-ldap",
+  aud: "https://<endpoint-host>/sts/",
+  exp: Math.floor(Date.now()/1000) + 300,
+  jti: "<random-uuid>"
+};
+console.log(jwt.sign(payload, "<256-bit-secret>", { algorithm: 'HS256' }));

---

80-81: CLI: avoid leaking tokens; use file://

Inline tokens end up in shell history and process listings. Supply the token via file path.

Apply:

-aws sts assume-role-with-web-identity --endpoint [endpoint] --role-arn [role-arn] --role-session-name [session-name] --web-identity-token eyJhbGciOiJub25lIiwidHlwIjoiSldUIn0.eyJ1c2VyIjoiVGhlVXNlck5hbWUiLCJwYXNzd29yZCI6IlRoZVVzZXJQYXNzd29yZCIsInR5cGUiOiJsZGFwIiwiaWF0IjoxNzU1MTgxOTE3fQ.
+echo '<jwt>' > /tmp/web-identity.jwt
+chmod 600 /tmp/web-identity.jwt
+aws sts assume-role-with-web-identity \
+  --endpoint <endpoint> \
+  --role-arn <ROLE_ARN> \
+  --role-session-name <SESSION_NAME> \
+  --web-identity-token file:///tmp/web-identity.jwt

---

56-64: Avoid embedding plaintext passwords in bearer tokens

Placing the LDAP password in a JWT leaks credentials via logs, proxies, and browser/process lists. Prefer collecting the password over TLS at request time and binding server-side without re-encoding the password.

Apply:

-{
- "user": "TheUserName",
- "password": "TheUserPassword",
- "type": "ldap"
-}
+{
+ "user": "TheUserName",
+ "type": "ldap",
+ "iss": "noobaa-ldap",
+ "aud": "https://<endpoint-host>/sts/",
+ "exp": <now + 300s>,
+ "jti": "<random-uuid>"
+}

If you absolutely must transport credentials, require HTTPS and JWE (encrypted JWT) and include short exp/jti.

---

134-141: Redact real-looking secrets/tokens in example output

These strings will trigger secret scanners and mislead readers.

Apply:

-    "Credentials": {
-        "AccessKeyId": "vTFcHUuPqjKdtzTsMULP",
-        "SecretAccessKey": "qBQxFcx99JOlABkZNq2fNRj6qGe18F9IREcHf9DZ",
-        "SessionToken": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJhY2Nlc3Nfa2V5IjoidlRGY0hVdVBxaktkdHpUc01VTFAiLCJzZWNyZXRfa2V5IjoicUJReEZjeDk5Sk9sQUJrWk5xMmZOUmo2cUdlMThGOUlSRWNIZjlEWiIsImFzc3VtZWRfcm9sZV9hY2Nlc3Nfa2V5IjoicFFJSTFjbTVrRm1wd3FQNmJ6SmgiLCJpYXQiOjE3NTU0MzU3OTgsImV4cCI6MTc1NTQzOTM5OH0.z5Uap_7IPAbWCJyZC5zj8JleNshGxsYuhdbI9hOjr78",
-        "Expiration": "2025-08-17T14:03:18+00:00"
-    },
+    "Credentials": {
+        "AccessKeyId": "<ACCESS_KEY_ID>",
+        "SecretAccessKey": "<SECRET_ACCESS_KEY>",
+        "SessionToken": "<SESSION_TOKEN>",
+        "Expiration": "<EXPIRATION_ISO8601>"
+    },

And in the command snippet above:

-aws sts assume-role-with-web-identity --endpoint https://127.0.0.1:7443 --role-arn arn:aws:sts::pQII1cm5kFmpwqP6bzJh:role/ldap_user --role-session-name fry1 --web-identity-token eyJhbGciOiJub25lIiwidHlwIjoiSldUIn0.eyJ1c2VyIjoiZnJ5IiwicGFzc3dvcmQiOiJmcnkiLCJ0eXBlIjoibGRhcCIsImlhdCI6MTc1NTQzMzkxOX0. --no-verify-ssl
+aws sts assume-role-with-web-identity --endpoint https://127.0.0.1:7443 \
+  --role-arn arn:aws:sts::<ACCESS_KEY_ID>:role/ldap_user \
+  --role-session-name <SESSION_NAME> \
+  --web-identity-token <WEB_IDENTITY_TOKEN> \
+  --no-verify-ssl

src/sdk/sts_sdk.js (3)

71-73: Stop logging full account objects (risk of secret leakage)

Log only safe identifiers (access_key, role_name).

Apply:

-        dbg.log0('sts_sdk.get_assumed_role res', account,
-            'account.role_config: ', account.role_config);
+        dbg.log0('sts_sdk.get_assumed_role res', {
+            access_key,
+            role_name,
+        });

-        dbg.log0('sts_sdk.get_assumed_role_with_web_identity res', account,
-            'account.role_config: ', account.role_config);
+        dbg.log0('sts_sdk.get_assumed_role_with_web_identity res', {
+            access_key: req.body.role_arn.split(':')[4],
+            role_name: account.role_config?.role_name,
+        });

Also applies to: 128-130

---

88-113: Web-identity handling: require signature, restrict algorithms, validate claims, and harden error checks

Allowing decode without a secret enables trivial forgery. Also rely on err.name, not message text.

Apply:

-        let web_token;
-        const jwt_secret = ldap_client.instance().ldap_params?.jwt_secret;
-        if (jwt_secret) {
-            try {
-                web_token = jwt.verify(req.body.web_identity_token, jwt_secret);
-            } catch (err) {
-                dbg.error('get_assumed_ldap_user error: JWT token verification failed', err);
-                if (err.message.includes('TokenExpiredError')) {
-                    throw new RpcError('EXPIRED_WEB_IDENTITY_TOKEN', err.message);
-                } else {
-                    throw new RpcError('INVALID_WEB_IDENTITY_TOKEN', err.message);
-                }
-            }
-        } else {
-            dbg.warn('get_assumed_ldap_user error: No LDAP JWT secret found');
-            web_token = jwt.decode(req.body.web_identity_token);
-        }
+        const token = req.body?.web_identity_token;
+        if (!token) throw new RpcError('INVALID_WEB_IDENTITY_TOKEN', 'Missing web_identity_token');
+        const jwt_secret = ldap_client.instance().ldap_params?.jwt_secret;
+        if (!jwt_secret) {
+            dbg.error('get_assumed_ldap_user: No LDAP JWT secret configured; refusing unsigned tokens');
+            throw new RpcError('INVALID_WEB_IDENTITY_TOKEN', 'Unsigned tokens are not allowed');
+        }
+        let web_token;
+        try {
+            // TODO: extend to RS/ES if public keys are configured
+            web_token = jwt.verify(token, jwt_secret, { algorithms: ['HS256'] });
+        } catch (err) {
+            dbg.error('get_assumed_ldap_user: JWT verification failed', err);
+            if (err?.name === 'TokenExpiredError') throw new RpcError('EXPIRED_WEB_IDENTITY_TOKEN', err.message);
+            throw new RpcError('INVALID_WEB_IDENTITY_TOKEN', err.message || String(err));
+        }

---

107-113: Required claims validation should include null checks and avoid password-in-token (design)

Keep current checks for POC, but add robust guard for non-object payloads. Longer term, remove password from token and bind over TLS.

Apply:

-        if (!web_token.user) {
+        if (!web_token || typeof web_token !== 'object' || !web_token.user) {
             throw new RpcError('INVALID_WEB_IDENTITY_TOKEN', 'Missing a required claim: user');
         }

Follow-up (separate change): drop the password requirement and collect it via HTTPS at request time.

src/endpoint/sts/ops/sts_post_assume_role_with_web_identity.js (1)

14-15: Do not log raw WebIdentityToken/password

Redact sensitive fields before logging.

Apply:

-    dbg.log0('sts_post_assume_role_with_web_identity body: ', req.body);
+    const log_body = { ...req.body };
+    for (const k of Object.keys(log_body)) {
+        if (/(token|password|secret|jwt)/i.test(k)) log_body[k] = '<redacted>';
+    }
+    dbg.log0('sts_post_assume_role_with_web_identity body: ', log_body);

src/util/ldap_client.js (4)

21-25: reconnect(): await disconnect before reconnecting

Prevents racing connect against an in-flight disconnect.

Apply:

 async reconnect() {
     dbg.log0(`reconnect called`);
-    this.disconnect();
-    return this.connect();
+    await this.disconnect();
+    return this.connect();
 }

---

14-19: disconnect(): null-safety and await unbind

Avoid throwing when admin_client is missing and await unbind to close sockets.

Apply:

 async disconnect() {
     dbg.log0('ldap client disconnect called');
     this._disconnected_state = true;
     this._connect_promise = null;
-    this.admin_client.unbind();
+    if (this.admin_client) {
+        try {
+            await this.admin_client.unbind();
+        } catch (_) {
+            // ignore
+        }
+    }
 }

---

107-129: Authenticate: ensure admin is connected and always unbind user client; LDAP injection already mitigated by EqualityFilter

Add a connection check and release sockets on all paths.

Apply:

     const user_client = new ldap.Client({
         url: this.ldap_params.uri,
         tlsOptions: this.tls_options,
     });
-    const { searchEntries } = await this.admin_client.search(this.ldap_params.search_dn, search_options);
-    if (!searchEntries || searchEntries.length === 0) {
-        throw new Error('User not found');
-    }
-    await this._bind(user_client, searchEntries[0].dn, password);
-    return searchEntries[0].dn;
+    if (!this.is_connected()) await this.connect();
+    try {
+        const { searchEntries } = await this.admin_client.search(this.ldap_params.search_dn, search_options);
+        if (!searchEntries || searchEntries.length === 0) {
+            throw new Error('User not found');
+        }
+        await this._bind(user_client, searchEntries[0].dn, password);
+        return searchEntries[0].dn;
+    } finally {
+        try { await user_client.unbind(); } catch (_) { /* ignore */ }
+    }

---

35-62: Fix is_connected() call and default TLS security

Call the method (not property) and default to verifying certs; allow overrides via config.

Apply:

             this.tls_options = this.ldap_params.tls_options || {
-                'rejectUnauthorized': false,
+                rejectUnauthorized: true,
             };
@@
-            if (this.is_connected) {
-                await this.reconnect();
-            }
+            if (this.is_connected()) {
+                await this.reconnect();
+            }

docs/design/ldap.md (1)

90-97: Minor markdown hygiene: linkify bare URLs

Wrap bare URLs in angle brackets or convert to text to satisfy MD034.

Example:

-read more here: https://docs.aws.amazon.com/cli/latest/reference/sts/assume-role-with-web-identity.html
+Read more here: <https://docs.aws.amazon.com/cli/latest/reference/sts/assume-role-with-web-identity.html>

Also applies to: 94-97, 102-102

src/test/integration_tests/api/sts/test_sts.js (3)

894-894: Fix typo in suite name

Use “identity” instead of “indentity”.

-mocha.describe('Assume role with web indentity tests', function() {
+mocha.describe('Assume role with web identity tests', function() {

---

914-915: Avoid global config mutation leaks in tests

Overwriting ldap_client.instance().ldap_params.jwt_secret can leak into other tests. Save/restore around the suite.

Apply:

-        ldap_client.instance().ldap_params.jwt_secret = "TEST_SECRET";
+        const saved_secret = ldap_client.instance().ldap_params?.jwt_secret;
+        ldap_client.instance().ldap_params.jwt_secret = "TEST_SECRET";
+        this.afterAll(() => { ldap_client.instance().ldap_params.jwt_secret = saved_secret; });

---

934-951: Add a positive-path test and “missing secret” case

Currently only negative flows are covered. Consider:

• One test with a valid signed token and a stubbed authenticate() returning a DN → expect success.
• One test where jwt_secret is unset → expect InvalidIdentityToken “Unsigned tokens are not allowed” (after implementing the SDK change).

I can add these tests with a stubbed ldap_client.instance().authenticate and secret save/restore if you want.

src/util/ldap_client.js (2)

76-83: Ensure config/client exists before connecting; avoid logging secrets

Guard against uninitialized admin_client and log only non-sensitive fields.

Apply:

 async connect() {
     this._disconnected_state = false;
     if (this._connect_promise) return this._connect_promise;
-    dbg.log0('connect called, current url:', this.ldap_params.uri);
+    if (!this.admin_client) await this.load_ldap_config();
+    const { uri, search_dn, search_scope } = this.ldap_params || {};
+    dbg.log0('connect called', { uri, search_dn, search_scope });
     this._connect_promise = this._connect();
     return this._connect_promise;
 }

---

84-96: Make the retry loop cancellable and handle missing client

Respect _disconnected_state and verify admin_client before binding.

Apply:

 async _connect() {
     let is_connected = false;
-    while (!is_connected) {
+    while (!is_connected && !this._disconnected_state) {
         try {
-            await this._bind(this.admin_client, this.ldap_params.admin, this.ldap_params.secret);
+            if (!this.admin_client) throw new Error('LDAP admin client not initialized');
+            await this._bind(this.admin_client, this.ldap_params.admin, this.ldap_params.secret);
             dbg.log0('_connect: initial connect succeeded');
             is_connected = true;
         } catch (err) {
             dbg.error('_connect: initial connect failed, will retry', err.message);
-            await P.delay(3000);
+            if (this._disconnected_state) break;
+            await P.delay(3000);
         }
     }
 }

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9042
File: src/util/cloud_utils.js:183-194
Timestamp: 2025-06-17T12:59:51.543Z
Learning: In the set_noobaa_s3_connection function in src/util/cloud_utils.js, internal NooBaa S3 endpoints (filtered by 'api': 's3', 'kind': 'INTERNAL') will always be non-secure, so tls: false is the correct setting despite the conditional https:// scheme logic.

docs/design/ldap.md (4)

80-81: Avoid inline tokens; use file:// to prevent shell history/process list leaks

Also fix “endpoint” placeholder.

Apply:

-aws sts assume-role-with-web-identity --endpoint [endpoint] --role-arn [role-arn] --role-session-name [session-name] --web-identity-token eyJhbGciOiJub25lIiwidHlwIjoiSldUIn0.eyJ1c2VyIjoiVGhlVXNlck5hbWUiLCJwYXNzd29yZCI6IlRoZVVzZXJQYXNzd29yZCIsInR5cGUiOiJsZGFwIiwiaWF0IjoxNzU1MTgxOTE3fQ.
+echo '<jwt>' > /tmp/web-identity.jwt
+chmod 600 /tmp/web-identity.jwt
+aws sts assume-role-with-web-identity --endpoint <endpoint> \
+  --role-arn <role-arn> \
+  --role-session-name <session-name> \
+  --web-identity-token file:///tmp/web-identity.jwt

---

9-13: Do not allow unsigned JWTs; require signature and standard claims

Accepting unsigned tokens or “constant signature” is a critical auth bypass. Require signed JWTs and validate iss/aud/exp/jti.

Apply:

-1. The client sends a signed(with a predfined constant signature) or unsigned JWT as the web identity token.
-2. The system parses the JWT to extract the LDAP username and password.
+1. The client sends a signed JWT (HS256/RS256/ES256) as the web identity token. Unsigned tokens (alg="none") are not allowed. The token MUST include iss, aud (NooBaa STS endpoint), exp (<= 5 minutes), and jti.
+2. The system validates the signature and claims, and extracts the LDAP username (do not include passwords in the token; the password is provided directly to the server over TLS during authentication).
 3. These credentials are validated against a configured external LDAP server.
 4. Upon successful authentication, the system issues a temporary STS token, granting the user access to S3 resources for a limited duration.

---

41-53: Make jwt_secret mandatory; document secure storage and permissions

JWTs must be signed; “unsigned by default” is unsafe. Also advise 0600 permissions and ownership.

Apply:

-jwt_secret (Optional) - The JWT secret the administrator will use to sign the token sent in AssumeRoleWithWebIdentity requests (default: unsigned). Once this option is set - unsigned tokens or tokens signed with different secret will be dropped as access denied.
+jwt_secret (Required) – Verification material used to validate JWT signatures.
+For HS256, set a strong shared secret (>=256 bits). For RS256/ES256, configure the public key used to verify signatures.
+All tokens without a valid signature are rejected. Store this file securely (chmod 600, owned by the NooBaa process user or service account), preferably provisioned via a Kubernetes Secret.

---

60-64: Do not embed passwords in JWT; remove 'none' algorithm and add standard claims

Carrying plaintext passwords in a bearer token is a high risk. Remove password from payload, require exp/iss/aud/jti, and drop the alg='none' example.

Apply:

 {
- "user": "TheUserName",
- "password": "TheUserPassword",
- "type": "ldap"
+ "user": "TheUserName",
+ "type": "ldap"
 }

-const jwt = require('jsonwebtoken');
-console.log(jwt.sign({ user: "TheUserName", password: "TheUserPassword", type: "ldap" }, "IAMTHEADMIN123!(SHOULDBE256BITS)"));
-eyJhbGciOiJub25lIiwidHlwIjoiSldUIn0.eyJ1c2VyIjoiVGhlVXNlck5hbWUiLCJwYXNzd29yZCI6IlRoZVVzZXJQYXNzd29yZCIsInR5cGUiOiJsZGFwIiwiaWF0IjoxNzU1MTgxOTE3fQ.
+const jwt = require('jsonwebtoken');
+const payload = {
+  user: "TheUserName",
+  type: "ldap",
+  iss: "noobaa-ldap",
+  aud: "https://<endpoint-host>/sts/",
+  exp: Math.floor(Date.now() / 1000) + 300, // 5 minutes
+  jti: require('crypto').randomUUID()
+};
+console.log(jwt.sign(payload, "IAMTHEADMIN123!(SHOULDBE256BITS)", { algorithm: 'HS256' }));

-Or you can use 'none' algorithm if you are not interested with verifying the token
-```js
-const jwt = require('jsonwebtoken');
-console.log(jwt.sign({ user: "TheUserName", password: "TheUserPassword", type: "ldap" }, undefined, { algorithm: 'none' }));
-eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJ1c2VyIjoiVGhlVXNlck5hbWUiLCJwYXNzd29yZCI6IlRoZVVzZXJQYXNzd29yZCIsInR5cGUiOiJsZGFwIiwiaWF0IjoxNzU1MTgyMzQ2fQ.P6WYcdM0kJagNK4D0M8AHiGFcUZ-DhTOKHlC1-AxcT0
-```
+<!-- Unsigned tokens are not allowed -->

Also applies to: 68-76

src/util/ldap_client.js (9)

7-7: Likely incorrect config import path

From src/util/ldap_client.js, config is usually one level up (../config), not two (../../config). Please verify and fix.

Apply:

-const config = require('../../config');
+const config = require('../config');

---

21-25: reconnect(): await disconnect before connecting

Prevents racing connect against an in-flight disconnect.

Apply:

 async reconnect() {
     dbg.log0(`reconnect called`);
-    this.disconnect();
-    return this.connect();
+    await this.disconnect();
+    return this.connect();
 }

---

76-82: Ensure config is loaded before connecting; avoid logging secrets

If connect is called early, admin_client may be undefined. Also log only non-sensitive context.

Apply:

 async connect() {
     this._disconnected_state = false;
     if (this._connect_promise) return this._connect_promise;
-    dbg.log0('connect called, current url:', this.ldap_params.uri);
+    if (!this.admin_client) {
+        await this.load_ldap_config();
+    }
+    dbg.log0('connect called', { uri: this.ldap_params?.uri, search_dn: this.ldap_params?.search_dn, search_scope: this.ldap_params?.search_scope });
     this._connect_promise = this._connect();
     return this._connect_promise;
 }

---

14-19: disconnect(): guard and await unbind to avoid dangling sockets

Avoid null deref and ensure the socket is closed before returning.

Apply:

 async disconnect() {
     dbg.log0('ldap client disconnect called');
     this._disconnected_state = true;
     this._connect_promise = null;
-    this.admin_client.unbind();
+    if (this.admin_client) {
+        try {
+            await this.admin_client.unbind();
+        } catch (_) {
+            // ignore
+        }
+    }
 }

---

49-51: Defaulting rejectUnauthorized: false weakens TLS security

Enable cert verification by default; allow override via config if needed.

Apply:

-            this.tls_options = this.ldap_params.tls_options || {
-                'rejectUnauthorized': false,
-            };
+            this.tls_options = this.ldap_params.tls_options || {
+                rejectUnauthorized: true,
+            };

---

84-96: Make retry loop cancellable and robust; verify admin_client exists

Observe _disconnected_state and guard against missing admin_client.

Apply:

 async _connect() {
     let is_connected = false;
-    while (!is_connected) {
+    while (!is_connected && !this._disconnected_state) {
         try {
-            await this._bind(this.admin_client, this.ldap_params.admin, this.ldap_params.secret);
+            if (!this.admin_client) throw new Error('LDAP admin client not initialized');
+            await this._bind(this.admin_client, this.ldap_params.admin, this.ldap_params.secret);
             dbg.log0('_connect: initial connect succeeded');
             is_connected = true;
         } catch (err) {
-            dbg.error('_connect: initial connect failed, will retry', err.message);
-            await P.delay(3000);
+            dbg.error('_connect: initial connect failed, will retry', err);
+            if (this._disconnected_state) break;
+            await P.delay(3000);
         }
     }
 }

---

119-129: Ensure admin connection and always unbind the user client

Avoid LDAP injection is already addressed by EqualityFilter; add connection check and cleanup.

Apply:

-        const user_client = new ldap.Client({
+        if (!this.is_connected()) {
+            await this.connect();
+        }
+        const user_client = new Client({
             url: this.ldap_params.uri,
             tlsOptions: this.tls_options,
         });
-        const { searchEntries } = await this.admin_client.search(this.ldap_params.search_dn, search_options);
-        if (!searchEntries || searchEntries.length === 0) {
-            throw new Error('User not found');
-        }
-        await this._bind(user_client, searchEntries[0].dn, password);
-        return searchEntries[0].dn;
+        try {
+            const { searchEntries } = await this.admin_client.search(this.ldap_params.search_dn, search_options);
+            if (!searchEntries || searchEntries.length === 0) {
+                throw new Error('User not found');
+            }
+            await this._bind(user_client, searchEntries[0].dn, password);
+            return searchEntries[0].dn;
+        } finally {
+            try { await user_client.unbind(); } catch (_) { /* ignore */ }
+        }

---

52-58: Reconnect condition bug: calling the method vs. checking its existence

this.is_connected is a function; current truthy check always triggers reconnect.

Apply:

-            this.admin_client = new ldap.Client({
+            this.admin_client = new Client({
                 url: this.ldap_params.uri,
                 tlsOptions: this.tls_options,
             });
-            if (this.is_connected) {
-                await this.reconnect();
-            }
+            if (this.is_connected()) await this.reconnect();

---

35-48: Load config: accept documented keys and avoid breaking with 'admin'/'secret'

Docs use admin/secret; code only reads admin_user/admin_password. Also accept search_base, not just search_dn.

Apply:

             const params = JSON.parse(fs.readFileSync(config.LDAP_CONFIG_PATH).toString());
             this.ldap_params = {
-                uri: params.uri || 'ldaps://127.0.0.1:636',
-                admin: params.admin_user || 'Administrator',
-                secret: params.admin_password || 'Passw0rd',
-                search_dn: params.search_dn || 'ou=people,dc=example,dc=com',
+                uri: params.uri || 'ldaps://127.0.0.1:636',
+                // accept both "admin" and "admin_user"
+                admin: (params.admin ?? params.admin_user) || 'Administrator',
+                // accept both "secret" and "admin_password"
+                secret: (params.secret ?? params.admin_password) || 'Passw0rd',
+                // accept both "search_dn" and "search_base"
+                search_dn: (params.search_dn ?? params.search_base) || 'ou=people,dc=example,dc=com',
                 dn_attribute: params.dn_attribute || 'uid', // for LDAP 'sAMAccountName' for AD
                 search_scope: params.search_scope || 'sub',
                 jwt_secret: params.jwt_secret,
                 ...params,
             };

src/endpoint/sts/ops/sts_post_assume_role.js (1)

15-17: Duration parsing and rounding alignment

• parse_sts_duration already throws StsError (with InvalidParameterValue or a range‐validation error) on invalid, non‐integer, or out-of-range inputs. These will be handled by the central STS error handler and return the correct 4xx response—no additional try/catch is needed here.

• Since parse_sts_duration returns duration_sec * 1000, the result is always a whole number of seconds. In practice,

  Math.ceil(duration_ms / 1000) === duration_ms / 1000

  so there’s no drift between the token TTL and the XML Expiration.

• Optional: if you’d like to future-proof against any sub-second durations and keep both values derived identically, you can switch to Math.floor and multiply back:

  - const duration_sec = Math.ceil(duration_ms / 1000);
  - const expiration_time = Date.now() + duration_ms;
  + const duration_sec = Math.floor(duration_ms / 1000);
  + const expiration_time = Date.now() + duration_sec * 1000;

docs/design/ldap.md (6)

22-24: Fix LDAPS URI example and port mismatch

Example shows ldap:// with 636. Either use ldaps://:636 or ldap://:389. Recommend LDAPS.

Apply:

-  "uri": "ldap://ldap.example.com:636",
+  "uri": "ldaps://ldap.example.com:636",

Also applies to: 46-46

---

88-90: Replace bare URL with a Markdown link

Minor doc polish per markdownlint.

Apply:

-read more here: https://docs.aws.amazon.com/cli/latest/reference/sts/assume-role-with-web-identity.html
+Read more: [AWS CLI — assume-role-with-web-identity](https://docs.aws.amazon.com/cli/latest/reference/sts/assume-role-with-web-identity.html)

---

92-104: Rephrase next step 5: do not encourage embedding passwords; if transporting, require JWE

Avoid normalizing on encrypted passwords in JWT; mandate JWE if credentials must be transported at all, and still require TLS and exp/jti.

Apply:

-5. See if we want to support encrypted password as part of the JWT token. see here: https://auth0.com/docs/secure/tokens/access-tokens/json-web-encryption
+5. Avoid transporting passwords in tokens. If strictly required, use JWE (encrypted JWT) with TLS, short exp, and jti to mitigate replay: https://auth0.com/docs/secure/tokens/access-tokens/json-web-encryption

---

94-97: Wrap remaining bare URLs in Markdown links

Conforms with MD034 and improves readability.

Apply:

-1. See if we can move to using C open-ldap client as part of our native code instead of ldapts (for better performance and fewer security issues). see here: https://www.openldap.org/software/repo.html (We will mainly need bind and search)
+1. See if we can move to using C open-ldap client as part of our native code instead of ldapts (for better performance and fewer security issues). See: [OpenLDAP repo](https://www.openldap.org/software/repo.html) (We will mainly need bind and search)
-* AD image: `docker run --rm --privileged -p 636:636 quay.io/samba.org/samba-ad-server:latest` (https://github.com/samba-in-kubernetes/samba-container)
+* AD image: `docker run --rm --privileged -p 636:636 quay.io/samba.org/samba-ad-server:latest` ([repo](https://github.com/samba-in-kubernetes/samba-container))
-* LDAP image: `docker run --rm --privileged -p 636:636 ghcr.io/ldapjs/docker-test-openldap/openldap:latest` (https://github.com/ldapjs/docker-test-openldap/pkgs/container/docker-test-openldap%2Fopenldap)
+* LDAP image: `docker run --rm --privileged -p 636:636 ghcr.io/ldapjs/docker-test-openldap/openldap:latest` ([package](https://github.com/ldapjs/docker-test-openldap/pkgs/container/docker-test-openldap%2Fopenldap))
-4. Better align and adapt to the IAM effort also in POC stage
+4. Better align and adapt to the IAM effort also in POC stage

Also applies to: 102-102

---

105-105: Remove trailing punctuation in heading

Minor markdownlint fix (MD026).

Apply:

-## Appendix A: Creating account w/ role config using NooBaa API:
+## Appendix A: Creating account w/ role config using NooBaa API

---

131-131: Style: make the lead-in concise

“In order to” → “To”.

Apply:

-in order to assume this role:
+To assume this role:

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9042
File: src/util/cloud_utils.js:183-194
Timestamp: 2025-06-17T12:59:51.543Z
Learning: In the set_noobaa_s3_connection function in src/util/cloud_utils.js, internal NooBaa S3 endpoints (filtered by 'api': 's3', 'kind': 'INTERNAL') will always be non-secure, so tls: false is the correct setting despite the conditional https:// scheme logic.

src/sdk/sts_sdk.js (2)

71-73: Avoid logging entire account objects

Previous review already flagged this — logging account and role_config risks leaking secrets.

-        dbg.log0('sts_sdk.get_assumed_role res', account,
-            'account.role_config: ', account.role_config);
+        dbg.log0('sts_sdk.get_assumed_role res', {
+            access_key,
+            role_name: account.role_config?.role_name
+        });

Apply similarly to get_assumed_ldap_user logs below.

---

91-107: Reject unsigned tokens; restrict algorithms; handle decode nulls via early validation

Accepting decoded (unsigned) tokens when jwt_secret is missing effectively allows anyone to supply credentials in a bearer token. This is unsafe and contradicts the PR description that tokens are signed with a server secret.

Apply:

-        let web_token;
-        const jwt_secret = ldap_client.instance().ldap_params?.jwt_secret;
-        if (jwt_secret) {
-            try {
-                web_token = jwt.verify(req.body.web_identity_token, jwt_secret);
-            } catch (err) {
-                dbg.error('get_assumed_ldap_user error: JWT token verification failed', err);
-                if (err.message.includes('TokenExpiredError')) {
-                    throw new RpcError('EXPIRED_WEB_IDENTITY_TOKEN', err.message);
-                } else {
-                    throw new RpcError('INVALID_WEB_IDENTITY_TOKEN', err.message);
-                }
-            }
-        } else {
-            dbg.warn('get_assumed_ldap_user: No LDAP JWT secret found, failing back to decoding');
-            web_token = jwt.decode(req.body.web_identity_token);
-        }
+        const token = req.body?.web_identity_token;
+        if (!token) {
+            throw new RpcError('INVALID_WEB_IDENTITY_TOKEN', 'Missing web_identity_token');
+        }
+        const jwt_secret = ldap_client.instance().ldap_params?.jwt_secret;
+        if (!jwt_secret) {
+            dbg.error('get_assumed_ldap_user: No LDAP JWT secret configured; refusing unsigned tokens');
+            throw new RpcError('INVALID_WEB_IDENTITY_TOKEN', 'Unsigned tokens are not allowed');
+        }
+        let web_token;
+        try {
+            // Restrict to expected HMAC algorithms; extend if RS/ES keys are configured
+            web_token = jwt.verify(token, jwt_secret, { algorithms: ['HS256'] });
+        } catch (err) {
+            dbg.error('get_assumed_ldap_user: JWT verification failed', err);
+            if (err?.name === 'TokenExpiredError') {
+                throw new RpcError('EXPIRED_WEB_IDENTITY_TOKEN', err.message);
+            }
+            throw new RpcError('INVALID_WEB_IDENTITY_TOKEN', err.message || String(err));
+        }

Also validate the decoded shape before accessing fields:

-        if (!web_token.user) {
+        if (!web_token || typeof web_token !== 'object' || !web_token.user) {
             throw new RpcError('INVALID_WEB_IDENTITY_TOKEN', 'Missing a required claim: user');
         }

docs/design/ldap.md (6)

79-81: Avoid inline tokens; use file:// to prevent leaking via shell history/ps

Also normalize placeholders and fix bracket style.

-aws sts assume-role-with-web-identity --endpoint [endpoint] --role-arn [role-arn] --role-session-name [session-name] --web-identity-token eyJhbGciOiJub25lIiwidHlwIjoiSldUIn0.eyJ1c2VyIjoiVGhlVXNlck5hbWUiLCJwYXNzd29yZCI6IlRoZVVzZXJQYXNzd29yZCIsInR5cGUiOiJsZGFwIiwiaWF0IjoxNzU1MTgxOTE3fQ.
+echo '<WEB_IDENTITY_TOKEN>' > /tmp/web-identity.jwt
+chmod 600 /tmp/web-identity.jwt
+aws sts assume-role-with-web-identity --endpoint <endpoint> \
+  --role-arn <ROLE_ARN> \
+  --role-session-name <SESSION_NAME> \
+  --web-identity-token file:///tmp/web-identity.jwt

---

134-141: Use placeholders and file-based token in Appendix; avoid JWT-looking strings

Also make --no-verify-ssl clearly dev-only.

-aws sts assume-role-with-web-identity --endpoint https://127.0.0.1:7443 --role-arn arn:aws:sts::pQII1cm5kFmpwqP6bzJh:role/ldap_user --role-session-name fry1 --web-identity-token eyJhbGciOiJub25lIiwidHlwIjoiSldUIn0.eyJ1c2VyIjoiZnJ5IiwicGFzc3dvcmQiOiJmcnkiLCJ0eXBlIjoibGRhcCIsImlhdCI6MTc1NTQzMzkxOX0. --no-verify-ssl
+echo '<WEB_IDENTITY_TOKEN>' > /tmp/web-identity.jwt
+chmod 600 /tmp/web-identity.jwt
+aws sts assume-role-with-web-identity --endpoint https://127.0.0.1:7443 \
+  --role-arn arn:aws:sts::<ACCOUNT_ID>:role/ldap_user \
+  --role-session-name <SESSION_NAME> \
+  --web-identity-token file:///tmp/web-identity.jwt \
+  --no-verify-ssl # for local dev only

---

9-13: Disallow unsigned/constant-signature JWTs; require signed tokens with strict claims

Accepting unsigned tokens or a "constant signature" is an auth bypass. Require HS256/RS256/ES256, and validate iss, aud, exp (<=5m), and jti.

-1. The client sends a signed(with a predfined constant signature) or unsigned JWT as the web identity token.
-2. The system parses the JWT to extract the LDAP username and password.
+1. The client sends a signed JWT (HS256/RS256/ES256) as the web identity token. Unsigned tokens (alg="none") are not allowed.
+2. The server validates the JWT signature and the following claims: iss, aud (NooBaa STS endpoint), exp (<= 5 minutes), and jti (nonce).
+3. The system parses the JWT to extract the LDAP username (and, if present, an encrypted password per the guidance below).
-3. These credentials are validated against a configured external LDAP server.
-4. Upon successful authentication, the system issues a temporary STS token, granting the user access to S3 resources for a limited duration.
+4. These credentials are validated against a configured external LDAP server.
+5. Upon successful authentication, the system issues a temporary STS token, granting the user access to S3 resources for a limited duration.

---

41-53: Make jwt_secret required; document secure storage (0600) and key type

JWTs must be signed; an “unsigned by default” mode is unsafe. Also clarify HS vs RS/ES keying.

-jwt_secret (Optional) - The JWT secret the administrator will use to sign the token sent in AssumeRoleWithWebIdentity requests (default: unsigned). Once this option is set - unsigned tokens or tokens signed with different secret will be dropped as access denied.
+jwt_secret (Required) – Secret (for HS256) or private key (for RS/ES) used to sign JWTs for AssumeRoleWithWebIdentity.
+All tokens without a valid signature are rejected. Store this material securely on disk with file permissions 0600 and owned by the NooBaa process user. Consider using a secrets manager in production.

---

56-64: Avoid embedding plaintext passwords in JWT; if unavoidable, require JWE + TLS

Embedding passwords in a bearer token is risky (logs, proxies, shell history). Prefer server-side bind using a password sent over TLS without re-encoding into a token. If you must carry it, use JWE (encrypted JWT), short exp, and jti.

-{
- "user": "TheUserName",
- "password": "TheUserPassword",
- "type": "ldap"
-}
+{
+  "user": "TheUserName",
+  "type": "ldap",
+  "iss": "noobaa-ldap",
+  "aud": "https://<endpoint-host>/sts/",
+  "exp": <epoch-seconds-now-plus-300>,
+  "jti": "<random-uuid>"
+}

Follow-up: If you decide to keep a password in the token for the POC, explicitly require JWE (encrypted JWT) and HTTPS in this section. I can add a minimal “jose” example if you want.

---

68-76: Fix JWT example and remove 'none' algorithm guidance

Show a single HS256 example with standard claims; remove alg='none'.

-const jwt = require('jsonwebtoken');
-console.log(jwt.sign({ user: "TheUserName", password: "TheUserPassword", type: "ldap" }, "IAMTHEADMIN123!(SHOULDBE256BITS)"));
-eyJhbGciOiJub25lIiwidHlwIjoiSldUIn0.eyJ1c2VyIjoiVGhlVXNlck5hbWUiLCJwYXNzd29yZCI6IlRoZVVzZXJQYXNzd29yZCIsInR5cGUiOiJsZGFwIiwiaWF0IjoxNzU1MTgxOTE3fQ.
-```
-Or you can use 'none' algorithm if you are not interested with verifying the token
-```js
-const jwt = require('jsonwebtoken');
-console.log(jwt.sign({ user: "TheUserName", password: "TheUserPassword", type: "ldap" }, undefined, { algorithm: 'none' }));
-eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJ1c2VyIjoiVGhlVXNlck5hbWUiLCJwYXNzd29yZCI6IlRoZVVzZXJQYXNzd29yZCIsInR5cGUiOiJsZGFwIiwiaWF0IjoxNzU1MTgyMzQ2fQ.P6WYcdM0kJagNK4D0M8AHiGFcUZ-DhTOKHlC1-AxcT0
+const jwt = require('jsonwebtoken');
+const { randomUUID } = require('crypto');
+const payload = {
+  user: "TheUserName",
+  type: "ldap",
+  iss: "noobaa-ldap",
+  aud: "https://<endpoint-host>/sts/",
+  exp: Math.floor(Date.now() / 1000) + 300, // 5 minutes
+  jti: randomUUID(),
+};
+console.log(jwt.sign(payload, "<YOUR_256_BIT_SECRET>", { algorithm: 'HS256' }));

src/util/ldap_client.js (3)

49-51: Defaulting rejectUnauthorized: false weakens TLS security

This enables MITM by default. The earlier diff sets it to true; keep an override for dev/test.

---

39-58: Fix defaults, key mapping, TLS security, and connection check

• Map both admin/secret and admin_user/admin_password; search_dn/search_base.
• Default rejectUnauthorized: true (currently false is insecure).
• Call is_connected() (not the function reference).

             this.ldap_params = {
-                uri: params.uri || 'ldaps://127.0.0.1:636',
-                admin: params.admin_user || 'Administrator',
-                secret: params.admin_password || 'Passw0rd',
-                search_dn: params.search_dn || 'ou=people,dc=example,dc=com',
+                uri: params.uri || 'ldaps://127.0.0.1:636',
+                // Accept both legacy and new keys
+                admin: (params.admin ?? params.admin_user) || 'Administrator',
+                secret: (params.secret ?? params.admin_password) || 'Passw0rd',
+                // Accept both search_base and search_dn
+                search_dn: (params.search_dn ?? params.search_base) || 'ou=people,dc=example,dc=com',
                 dn_attribute: params.dn_attribute || 'uid', // for LDAP 'sAMAccountName' for AD
                 search_scope: params.search_scope || 'sub',
                 jwt_secret: params.jwt_secret,
                 ...params,
             };
             this.tls_options = this.ldap_params.tls_options || {
-                'rejectUnauthorized': false,
+                rejectUnauthorized: true,
             };
             this.admin_client = new ldap.Client({
                 url: this.ldap_params.uri,
                 tlsOptions: this.tls_options,
             });
-            if (this.is_connected) {
-                await this.reconnect();
-            }
+            if (this.is_connected()) {
+                await this.reconnect();
+            }

---

107-129: authenticate(): ensure admin is connected and always unbind user client

Also safe-close user client via finally.

 async authenticate(user, password) {
     // EqualityFilter automatically escapes the value to prevent LDAP injection
-    const eqFilter = new ldap.EqualityFilter({
+    const eqFilter = new ldap.EqualityFilter({
         attribute: this.ldap_params.dn_attribute,
         value: user,
     });
@@
-    const user_client = new ldap.Client({
+    if (!this.is_connected()) {
+        await this.connect();
+    }
+    const user_client = new ldap.Client({
         url: this.ldap_params.uri,
         tlsOptions: this.tls_options,
     });
-    const { searchEntries } = await this.admin_client.search(this.ldap_params.search_dn, search_options);
-    if (!searchEntries || searchEntries.length === 0) {
-        throw new Error('User not found');
-    }
-    await this._bind(user_client, searchEntries[0].dn, password);
-    return searchEntries[0].dn;
+    try {
+        const { searchEntries } = await this.admin_client.search(this.ldap_params.search_dn, search_options);
+        if (!searchEntries || searchEntries.length === 0) {
+            throw new Error('User not found');
+        }
+        await this._bind(user_client, searchEntries[0].dn, password);
+        return searchEntries[0].dn;
+    } finally {
+        try { await user_client.unbind(); } catch (_) { /* ignore */ }
+    }
 }