Conversation
Adds an explicit flag to enabled DN mapping in the LDAP Proxy. This fixes a consistency issue where attributes are mapped to the Proxy base DN by the entry DNs were not. The code was fixed so that either the entry DN and attribute DNs are all mapped or none mapped. The LDAPRolesLookupInterceptor is modified to differentiate between group and user entries. If the entry is a group entry, then the entry dn is replaced with the role dn. Adds new workflow test with knox configured to use role lookup. A new docker-compose.role-lookup.yml file is added to run the tests under the new configuration. The roles.json file is used for file-based role lookup so no external server is needed. The workflow test copies the test_knox_ldap_proxy_search.py tests and replaces the expected values from role lookup. The tests are also run with the bypass control. docker compose -f ./.github/workflows/compose/docker-compose.yml -f ./.github/workflows/compose/docker-compose.roles-lookup.yml up --exit-code-from tests tests
Test Results 4 files 4 suites 42s ⏱️ Results for commit 19936e9. |
| } | ||
|
|
||
| // Map the dn-valued attributes from the proxy base dn to remote base dn | ||
| if (DN_VALUED_ATTRIBUTES.contains(currentAttribute.toLowerCase(Locale.ROOT))) { |
There was a problem hiding this comment.
Possible NPE is currentAttribute is null.
| public static final int DEFAULT_LDAP_MAX_SIZE_LIMIT = 1000; | ||
| /* The default max time for LDAP search in milliseconds */ | ||
| public static final int DEFAULT_LDAP_MAX_TIME_LIMIT = 60 * 1000; | ||
| public static final boolean DEFAULT_LDAP_DN_MAPPING_ENABLED = true; |
There was a problem hiding this comment.
This is just a comment to remember: flips returned-DN behavior for every existing LDAP-proxy deployment on upgrade with no config change.
| return dedup.values(); | ||
| } | ||
|
|
||
| private void combineGroupEntry(Entry entry1, Entry entry2) throws LdapException { |
There was a problem hiding this comment.
Attribute entry1Member = entry1.get("member");
Attribute entry2Member = entry2.get("member");
if (entry1Member == null && entry2Member != null) { ... }
else if (entry1Member != null && entry2Member != null) { ... }
And LdapUtils.isGroupEntry (line 35) routes both groupOfNames and groupOfUniqueNames into this path. For groupOfUniqueNames, membership lives in uniqueMember, so both get("member") calls return null → neither branch runs → the second group's members are gone. The collision is real: translateGroupEntry renames each group's DN to cn=<role>,<same parent> (line 147), so two groups mapping to one role produce the identical DN and hit combineGroupEntry at line 113. Silent, no log, defeats the PR's own purpose.
This branch has not been deployed
2 participants
KNOX-3485 - Roles lookup replaces group entries with role entries
What changes were proposed in this pull request?
Adds an explicit flag to enabled DN mapping in the LDAP Proxy. This fixes a consistency issue where attributes are mapped to the Proxy base DN by the entry DNs were not. The code was fixed so that either the entry DN and attribute DNs are all mapped or none mapped.
The LDAPRolesLookupInterceptor is modified to differentiate between group and user entries. If the entry is a group entry, then the entry dn is replaced with the role dn.
How was this patch tested?
Added unit and integration tests
Integration Tests
Adds new workflow test with knox configured to use role lookup. A new docker-compose.role-lookup.yml file is added to run the tests under the new configuration. The roles.json file is used for file-based role lookup so no external server is needed. The workflow test copies the test_knox_ldap_proxy_search.py tests and replaces the expected values from role lookup. The tests are also run with the bypass control.
docker compose -f ./.github/workflows/compose/docker-compose.yml -f ./.github/workflows/compose/docker-compose.roles-lookup.yml up --exit-code-from tests tests
UI changes
No UI changes