harbor icon indicating copy to clipboard operation
harbor copied to clipboard

feat: Add nested LDAP group Support in Active Directory

Open TMHBOFH opened this issue 7 months ago • 11 comments

Comprehensive Summary of your change

Currently, Harbor does not support the use of nested LDAP groups. Especially in larger Active Directory environments, nested LDAP groups are commonly used. This pull request enables the use of nested LDAP groups and thereby helps to fully utilize the capabilities of Harbor.

Note

This change is additive and does not break existing LDAP config setups.

Screenshots

image

Issue being fixed

Fixes Support nested LDAP groups

Please indicate you've done the following:

  • [x] Well Written Title and Summary of the PR
  • [x] Label the PR as needed. "release-note/ignore-for-release, release-note/new-feature, release-note/update, release-note/enhancement, release-note/community, release-note/breaking-change, release-note/docs, release-note/infra, release-note/deprecation"
  • [x] Accepted the DCO. Commits without the DCO will delay acceptance.
  • [x] Made sure tests are passing and test coverage is added if needed.
  • [x] Considered the docs impact and opened a new docs issue or PR with docs changes if needed in website repository.

TMHBOFH avatar May 21 '25 08:05 TMHBOFH

@TMHBOFH DCO is missing

Vad1mo avatar May 21 '25 08:05 Vad1mo

  • Correct me if I am wrong, but I think you can accomplish the same thing by adding member:1.2.840.113556.1.4.1941 into the group filter. This way, there is no need to add MS-AD specific options.
  • Unfortunately, it’s not working. I’ve already tried various approaches. According to the specification, the filter should look like this (&(objectClass=group)(member:1.2.840.113556.1.4.1941:=[UserDN])), but from my point of view, the current code doesn’t allow for that.
  • This is AD-specific, so it needs to be marked as such. Currently its not clear
  • You mean accordingly in the labeling and documentation?

TMHBOFH avatar May 21 '25 08:05 TMHBOFH

  • Correct me if I am wrong, but I think you can accomplish the same thing by adding member:1.2.840.113556.1.4.1941 into the group filter. This way, there is no need to add MS-AD specific options.
  • Unfortunately, it’s not working. I’ve already tried various approaches. According to the specification, the filter should look like this (&(objectClass=group)(member:1.2.840.113556.1.4.1941:=[UserDN])), but from my point of view, the current code doesn’t allow for that.

I see, Do you think we can expose variables like $UserDN in the query: e.g. so you can write (&(objectClass=group)(member:1.2.840.113556.1.4.1941:=[$UserDN]))

  • This is AD-specific, so it needs to be marked as such. Currently its not clear
  • You mean accordingly in the labeling and documentation?

Yes, there is no indication this feature only works in the AD context.


My goal here is to make it possible but, at the same time, not adding AD specifics into it.

Vad1mo avatar May 21 '25 12:05 Vad1mo

My goal here is to make it possible but, at the same time, not adding AD specifics into it.

I understand your goal and want to support you as best I can.

From my point of view, authorization is currently handled based on the user object. That means, during login, the system checks which LDAP groups the user is currently a member of, and then Harbor maps the user accordingly internally. This is where my PR comes in: it extends the group resolution to include the handling of nested groups in AD.

If we now want to switch to a group-based filter (as in your example), a large part of the current LDAP authorization logic would likely need to be reworked. That’s because the permission mapping currently relies on the LDAP user object, not on the LDAP group.

Here's how I could imagine a possible flow: After the user successfully authenticates via LDAP, the group memberships of all LDAP groups known to Harbor would need to be resolved and compared against the authenticated user.

I hope I’ve understood the code correctly and haven’t misrepresented anything here.

TMHBOFH avatar May 21 '25 15:05 TMHBOFH

@Vad1mo I did some more research on how others handle this topic. My suggestion would be to use the group filter as you described and check it for the relevant content.

If the filter contains the variable {{.Username}} or {{.UserDN}}, group membership can be resolved using the corresponding filter. This is similar to what I tried in this PR, except that the filter is now not specific to AD.

For example:

  • (&(objectClass=group)(member:1.2.840.113556.1.4.1941:={{.UserDN}}))
  • (|(memberUid={{.Username}})(member={{.UserDN}})

With this approach, not much would need to be changed in the code. The functionality would just be an addition and would not alter the existing processes.

What do you think of the idea?

TMHBOFH avatar May 22 '25 06:05 TMHBOFH

I’ve implemented the alternative I described earlier. My tests was been successful so far. I believe this version is much more flexible.

If you agree, I’ll go ahead and close the PR and open a new one. Thanks a lot for your support

TMHBOFH avatar May 22 '25 19:05 TMHBOFH

@TMHBOFH thank you for looking into alternatives, let's see what other maintainer (@wy65701436, @stonezdj , @MinerYang ) think about the two options..

Vad1mo avatar May 24 '25 11:05 Vad1mo

I prefer the current option, the alternative impl lets the user configure the magic number in the filter. rare user can understand the meaning. and the code is more complex than the current.

Suggest to change the option name from LDAP Nested Group to Support Nested Group in Active Directory, because the implementation only support Active Directory, not other LDAP types.

stonezdj avatar May 27 '25 06:05 stonezdj

Alright, I've updated the PR accordingly and I hope everything is fine now. I’d be happy if it could make it into 2.14.

Thanks a lot to all of you!

TMHBOFH avatar May 27 '25 16:05 TMHBOFH

@TMHBOFH, can outline how those two might be related? https://github.com/goharbor/harbor/pull/21806

Vad1mo avatar Jun 13 '25 09:06 Vad1mo

an outline how those two might be related?

I hope I understood your question correctly. My PR enables the use of nested groups in an Active Directory (AD) environment. This applies to both admin groups and non-admin groups in Harbor.

The other PR 21806, from my perspective, mainly adds the ability to specify more than one admin group. It may also allow to support nested groups outside of an AD environment, but this is limited to admin groups only. Non-admin groups are still not allowed to be nested.

So, with regard to an AD environment, the other PR only provides the additional benefit of being able to configure more than one admin group.

TMHBOFH avatar Jun 14 '25 21:06 TMHBOFH

I am closing this pull request because I think you can solve this problem the following way, by adding the following to your LDAP Group filter

((member:1.2.840.113556.1.4.1941:="userDN")(userDN))...

Vad1mo avatar Jun 30 '25 12:06 Vad1mo

((member:1.2.840.113556.1.4.1941:="userDN")(userDN))

@Vad1mo Hello, I’ve tried various versions using the filter you suggested, but none of them worked. From my understanding, it can't really work this way, because the userDN variable isn’t being replaced in the LDAP query.If you’d like I can share the debug logs.

Since nested groups are used exclusively in our AD, it would be great if we could also use them with Harbor. Whether this is implemented via the PR or another solution doesn’t matter to me. The issue with nested groups has been brought up since mid-2019, even if it might not affect a large user group.

Thank you very much.

TMHBOFH avatar Jul 01 '25 06:07 TMHBOFH

Best is we set up a call and walk through this together... ping me on slack regarding appointment

Vad1mo avatar Jul 09 '25 12:07 Vad1mo

This PR is being marked stale due to a period of inactivty. If this PR is still relevant, please comment or remove the stale label. Otherwise, this PR will close in 30 days.

github-actions[bot] avatar Oct 20 '25 09:10 github-actions[bot]

This PR was closed because it has been stalled for 30 days with no activity. If this PR is still relevant, please re-open a new PR against main.

github-actions[bot] avatar Nov 20 '25 09:11 github-actions[bot]