Skip to content

Commit 1613783

Browse files
Sakshi MunjalSakshi Munjal
Sakshi Munjal
authored and
Sakshi Munjal
committed
add ldap changes
Signed-off-by: Sakshi Munjal <sakshimunjal@Sakshis-MacBook-Pro.local>
1 parent 45ca4a4 commit 1613783

4 files changed

Lines changed: 335 additions & 2 deletions

File tree

‎src/endpoint/sts/sts_rest.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ async function authenticate_request(req) {
117117
signature_utils.authenticate_request_by_service(req, req.sts_sdk);
118118
if (req.op_name === 'post_assume_role_with_web_identity') {
119119
const web_identity_info = access_policy_utils.fetch_web_identity_info(req);
120-
const is_ldap_request = web_identity_info.username;
120+
const is_ldap_request = web_identity_info.type === 'ldap' || web_identity_info.user;
121121
if (is_ldap_request) {
122122
// fetch LDAP identity info
123123
req.sts_sdk.identity_info = await req.sts_sdk.authenticate_web_identity(req);

‎src/sdk/sts_sdk.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -205,6 +205,7 @@ class StsSDK {
205205
return {
206206
access_key: role_config.access_key,
207207
role_config,
208+
account_id: role_config.account_id,
208209
dn: ldap_auth_result.dn,
209210
};
210211
}

‎src/test/unit_tests/util_functions_tests/test_ldap_assume_role_trust_policy.test.js‎

Lines changed: 307 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,307 @@
1+
/* Copyright (C) 2026 NooBaa */
2+
'use strict';
3+
4+
/**
5+
* LDAP AssumeRoleWithWebIdentity trust-policy unit tests.
6+
*
7+
* Asserts intended behavior (Federated LDAP URI + ldap: Conditions on bind attrs).
8+
* Expectations must NOT be adapted to current bugs — if production wrongly ALLOWs
9+
* when Condition should deny (or the reverse), these tests should fail.
10+
* Does not cover Keycloak/OIDC, JWT/bind/STS wiring (those belong in STS suites).
11+
*/
12+
13+
jest.mock('jwks-rsa', () => jest.fn().mockImplementation(() => ({
14+
getSigningKey: jest.fn((kid, cb) => cb(null, {
15+
getPublicKey: () => '-----BEGIN PUBLIC KEY-----\nMOCK\n-----END PUBLIC KEY-----'
16+
})),
17+
})), { virtual: true });
18+
19+
const LDAP_URI = 'ldaps://127.0.0.1:1636';
20+
21+
jest.mock('../../../util/ldap_client', () => ({
22+
instance: jest.fn(() => ({
23+
ldap_params: { uri: LDAP_URI },
24+
})),
25+
is_ldap_configured: jest.fn(() => true),
26+
}));
27+
28+
const jwt = require('jsonwebtoken');
29+
const access_policy_utils = require('../../../util/access_policy_utils');
30+
const ldap_client = require('../../../util/ldap_client');
31+
32+
function ldap_jwt(claims = { user: 'fry', password: 'fry' }) {
33+
return jwt.sign(claims, 'test-secret');
34+
}
35+
36+
function trust_policy({ principal, action = 'sts:AssumeRoleWithWebIdentity', condition } = {}) {
37+
const statement = {
38+
Effect: 'Allow',
39+
Principal: principal,
40+
Action: action,
41+
};
42+
if (condition) statement.Condition = condition;
43+
return {
44+
Version: '2012-10-17',
45+
Statement: [statement],
46+
};
47+
}
48+
49+
function ldap_req(claims, identity_info) {
50+
const req = { body: { web_identity_token: ldap_jwt(claims) } };
51+
if (identity_info) {
52+
req.sts_sdk = { identity_info };
53+
}
54+
return req;
55+
}
56+
57+
describe('LDAP AssumeRoleWithWebIdentity trust policy', () => {
58+
59+
beforeEach(() => {
60+
ldap_client.instance.mockReturnValue({
61+
ldap_params: { uri: LDAP_URI },
62+
});
63+
});
64+
65+
describe('Principal.Federated (LDAP URI)', () => {
66+
67+
it('should ALLOW when Federated URI matches configured LDAP URI (ldaps)', async () => {
68+
const policy = trust_policy({
69+
principal: { Federated: LDAP_URI },
70+
});
71+
const result = await access_policy_utils.has_access_policy_permission(
72+
policy,
73+
[],
74+
'sts:AssumeRoleWithWebIdentity',
75+
undefined,
76+
ldap_req({ user: 'fry', password: 'fry' }),
77+
{ is_trust_policy: true }
78+
);
79+
expect(result).toBe('ALLOW');
80+
});
81+
82+
it('should ALLOW when Federated uses ldap:// and config uses ldaps:// (scheme-stripped match)', async () => {
83+
const policy = trust_policy({
84+
principal: { Federated: 'ldap://127.0.0.1:1636' },
85+
});
86+
const result = await access_policy_utils.has_access_policy_permission(
87+
policy,
88+
[],
89+
'sts:AssumeRoleWithWebIdentity',
90+
undefined,
91+
ldap_req({ user: 'fry', password: 'fry' }),
92+
{ is_trust_policy: true }
93+
);
94+
expect(result).toBe('ALLOW');
95+
});
96+
97+
it('should ALLOW when Federated uses ldaps:// and config uses ldap:// (scheme-stripped match)', async () => {
98+
ldap_client.instance.mockReturnValue({
99+
ldap_params: { uri: 'ldap://127.0.0.1:1636' },
100+
});
101+
const policy = trust_policy({
102+
principal: { Federated: LDAP_URI },
103+
});
104+
const result = await access_policy_utils.has_access_policy_permission(
105+
policy,
106+
[],
107+
'sts:AssumeRoleWithWebIdentity',
108+
undefined,
109+
ldap_req({ user: 'fry', password: 'fry' }),
110+
{ is_trust_policy: true }
111+
);
112+
expect(result).toBe('ALLOW');
113+
});
114+
115+
it('should IMPLICIT_DENY when Federated URI does not match configured LDAP URI', async () => {
116+
const policy = trust_policy({
117+
principal: { Federated: 'ldaps://wrong-host:1636' },
118+
});
119+
const result = await access_policy_utils.has_access_policy_permission(
120+
policy,
121+
[],
122+
'sts:AssumeRoleWithWebIdentity',
123+
undefined,
124+
ldap_req({ user: 'fry', password: 'fry' }),
125+
{ is_trust_policy: true }
126+
);
127+
expect(result).toBe('IMPLICIT_DENY');
128+
});
129+
130+
it('should ALLOW anonymous LDAP caller with Principal AWS *', async () => {
131+
const policy = trust_policy({
132+
principal: { AWS: '*' },
133+
});
134+
const result = await access_policy_utils.has_access_policy_permission(
135+
policy,
136+
[],
137+
'sts:AssumeRoleWithWebIdentity',
138+
undefined,
139+
ldap_req({ user: 'fry', password: 'fry' }),
140+
{ is_trust_policy: true }
141+
);
142+
expect(result).toBe('ALLOW');
143+
});
144+
145+
it('should IMPLICIT_DENY when Action is sts:AssumeRole only (not WebIdentity)', async () => {
146+
const policy = trust_policy({
147+
principal: { Federated: LDAP_URI },
148+
action: 'sts:AssumeRole',
149+
});
150+
const result = await access_policy_utils.has_access_policy_permission(
151+
policy,
152+
[],
153+
'sts:AssumeRoleWithWebIdentity',
154+
undefined,
155+
ldap_req({ user: 'fry', password: 'fry' }),
156+
{ is_trust_policy: true }
157+
);
158+
expect(result).toBe('IMPLICIT_DENY');
159+
});
160+
161+
it('should ALLOW when Action is sts:* and Federated URI matches', async () => {
162+
const policy = trust_policy({
163+
principal: { Federated: LDAP_URI },
164+
action: 'sts:*',
165+
});
166+
const result = await access_policy_utils.has_access_policy_permission(
167+
policy,
168+
[],
169+
'sts:AssumeRoleWithWebIdentity',
170+
undefined,
171+
ldap_req({ user: 'fry', password: 'fry' }),
172+
{ is_trust_policy: true }
173+
);
174+
expect(result).toBe('ALLOW');
175+
});
176+
177+
it('should ALLOW when Federated is an array and one URI matches', async () => {
178+
const policy = trust_policy({
179+
principal: { Federated: ['ldaps://other:636', LDAP_URI] },
180+
});
181+
const result = await access_policy_utils.has_access_policy_permission(
182+
policy,
183+
[],
184+
'sts:AssumeRoleWithWebIdentity',
185+
undefined,
186+
ldap_req({ user: 'fry', password: 'fry' }),
187+
{ is_trust_policy: true }
188+
);
189+
expect(result).toBe('ALLOW');
190+
});
191+
});
192+
193+
describe('ldap: Condition keys (bind attributes)', () => {
194+
195+
// Intended behavior (not "whatever the code currently returns"):
196+
// - ldap:* Conditions evaluate against LDAP bind attrs on req.sts_sdk.identity_info
197+
// - JWT claims alone (user/password) are NOT sufficient for ldap: Conditions
198+
// These go through has_access_policy_permission so a missing merge fails the suite.
199+
200+
const fry_bind = {
201+
dn: 'cn=Philip J. Fry,ou=people,dc=planetexpress,dc=com',
202+
ou: 'Delivering Crew',
203+
memberOf: 'cn=ship_crew,ou=people,dc=planetexpress,dc=com',
204+
uid: 'fry',
205+
cn: 'Philip J. Fry',
206+
mail: 'fry@planetexpress.com',
207+
};
208+
209+
async function eval_trust(policy, identity_info) {
210+
return access_policy_utils.has_access_policy_permission(
211+
policy,
212+
[],
213+
'sts:AssumeRoleWithWebIdentity',
214+
undefined,
215+
ldap_req({ user: 'fry', password: 'fry' }, identity_info),
216+
{ is_trust_policy: true }
217+
);
218+
}
219+
220+
it('should ALLOW when StringEquals ldap:ou matches bind attr', async () => {
221+
const policy = trust_policy({
222+
principal: { Federated: LDAP_URI },
223+
condition: { StringEquals: { 'ldap:ou': 'Delivering Crew' } },
224+
});
225+
expect(await eval_trust(policy, fry_bind)).toBe('ALLOW');
226+
});
227+
228+
it('should IMPLICIT_DENY when StringEquals ldap:ou mismatches bind attr', async () => {
229+
const policy = trust_policy({
230+
principal: { Federated: LDAP_URI },
231+
condition: { StringEquals: { 'ldap:ou': 'Wrong OU' } },
232+
});
233+
expect(await eval_trust(policy, fry_bind)).toBe('IMPLICIT_DENY');
234+
});
235+
236+
it('should IMPLICIT_DENY when ldap: condition attr is missing from bind result', async () => {
237+
const policy = trust_policy({
238+
principal: { Federated: LDAP_URI },
239+
condition: { StringEquals: { 'ldap:ou': 'Delivering Crew' } },
240+
});
241+
expect(await eval_trust(policy, { uid: 'fry', dn: fry_bind.dn })).toBe('IMPLICIT_DENY');
242+
});
243+
244+
it('should ALLOW ForAnyValue:StringEquals when multi-valued ldap attr overlaps', async () => {
245+
const policy = trust_policy({
246+
principal: { Federated: LDAP_URI },
247+
condition: {
248+
'ForAnyValue:StringEquals': {
249+
'ldap:ou': ['Delivering Crew', 'Service Staff'],
250+
},
251+
},
252+
});
253+
expect(await eval_trust(policy, {
254+
...fry_bind,
255+
ou: ['Delivering Crew', 'Ship Crew'],
256+
})).toBe('ALLOW');
257+
});
258+
259+
it('should IMPLICIT_DENY ForAnyValue:StringEquals when no values overlap', async () => {
260+
const policy = trust_policy({
261+
principal: { Federated: LDAP_URI },
262+
condition: {
263+
'ForAnyValue:StringEquals': {
264+
'ldap:ou': ['Service Staff', 'Office'],
265+
},
266+
},
267+
});
268+
expect(await eval_trust(policy, {
269+
...fry_bind,
270+
ou: ['Delivering Crew'],
271+
})).toBe('IMPLICIT_DENY');
272+
});
273+
274+
it('should ALLOW Principal AWS * + matching ldap:ou (LDAPRole shape)', async () => {
275+
const policy = trust_policy({
276+
principal: { AWS: '*' },
277+
condition: { StringEquals: { 'ldap:ou': 'Delivering Crew' } },
278+
});
279+
expect(await eval_trust(policy, fry_bind)).toBe('ALLOW');
280+
});
281+
282+
it('should IMPLICIT_DENY ldap:ou when only JWT is present (bind attrs required)', async () => {
283+
// Production bug class: Principal fits from JWT, Condition wrongly ignored/used JWT.
284+
// Correct behavior: deny — ldap:ou is not on the JWT.
285+
const policy = trust_policy({
286+
principal: { AWS: '*' },
287+
condition: { StringEquals: { 'ldap:ou': 'Delivering Crew' } },
288+
});
289+
expect(await eval_trust(policy, undefined)).toBe('IMPLICIT_DENY');
290+
});
291+
292+
it('should ALLOW when Principal/Action fit and no Condition block', async () => {
293+
const policy = trust_policy({
294+
principal: { Federated: LDAP_URI },
295+
});
296+
expect(await eval_trust(policy, fry_bind)).toBe('ALLOW');
297+
});
298+
299+
it('should IMPLICIT_DENY unsupported condition operator on ldap path', async () => {
300+
const policy = trust_policy({
301+
principal: { Federated: LDAP_URI },
302+
condition: { StringLike: { 'ldap:ou': 'Delivering*' } },
303+
});
304+
expect(await eval_trust(policy, fry_bind)).toBe('IMPLICIT_DENY');
305+
});
306+
});
307+
});

‎src/util/access_policy_utils.js‎

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ const dbg = require('./debug_module')(__filename);
66
const s3_utils = require('../endpoint/s3/s3_utils');
77
const RpcError = require('../rpc/rpc_error');
88
const jwt = require('jsonwebtoken');
9+
const ldap_client = require('./ldap_client');
910

1011
const OP_NAME_TO_ACTION = Object.freeze({
1112
delete_bucket_analytics: { regular: "s3:PutAnalyticsConfiguration" },
@@ -314,6 +315,23 @@ function _is_principal_fit(account_arr, statement,
314315
}
315316
}
316317

318+
// --- LDAP principal ---
319+
const is_ldap = web_identity_info.type === 'ldap' || (web_identity_info.user !== undefined && web_identity_info.password !== undefined);
320+
if(!principal_fit && statement_principal.Federated && is_ldap) {
321+
const ldap_uri = ldap_client.instance()?.ldap_params?.uri;
322+
if (ldap_uri) {
323+
const ldap_address = uri => String(uri).replace(/^ldaps?:\/\//i, '');
324+
const configured = ldap_address(ldap_uri);
325+
for (const federated of _.flatten([statement_principal.Federated])) {
326+
const federated_url = typeof federated === 'string' ? federated : federated.unwrap();
327+
if (ldap_address(federated_url) === configured) {
328+
principal_fit = true;
329+
break;
330+
}
331+
}
332+
}
333+
}
334+
317335
return statement.Principal ? principal_fit : !principal_fit;
318336
}
319337

@@ -734,7 +752,7 @@ function _is_identity_condition_fit(is_keycloak_request, condition, web_identity
734752
return false;
735753
}
736754
} else if (expected_key.startsWith('ldap:')) { // LDAP identity condition
737-
if (!_is_ldap_identity_fit(condition_key, expected_value, web_identity_info, predicate)) return false;
755+
if (!_is_ldap_identity_fit(expected_key, expected_value, web_identity_info, predicate)) return false;
738756
}
739757
}
740758
}
@@ -945,6 +963,12 @@ function fetch_web_identity_info(req) {
945963
if (req?.body?.web_identity_token) {
946964
web_identity_info = jwt.decode(req.body.web_identity_token, { json: true });
947965
}
966+
// LDAP: JWT only carries user/password. Bind attributes (ou, memberOf, uid, ...)
967+
// are set on req.sts_sdk.identity_info during authenticate_request and must be
968+
// merged so trust-policy Conditions like StringEquals ldap:ou can evaluate.
969+
if (req?.sts_sdk?.identity_info) {
970+
web_identity_info = { ...(web_identity_info || {}), ...req.sts_sdk.identity_info };
971+
}
948972
return web_identity_info || {};
949973
}
950974

@@ -958,6 +982,7 @@ exports.get_policy_principal_arn = get_policy_principal_arn;
958982
exports.create_arn_for_root = create_arn_for_root;
959983
exports.get_account_identifier_id = get_account_identifier_id;
960984
exports._is_wildcard_match = _is_wildcard_match;
985+
exports._is_principal_fit = _is_principal_fit;
961986
exports._is_identity_condition_fit = _is_identity_condition_fit;
962987
exports.keycloak_predicate_map = keycloak_predicate_map;
963988
exports.extract_tag_key_from_condition = extract_tag_key_from_condition;

0 commit comments

Comments
 (0)