Linux: Fix nested group resolution for RBAC claims #35279
Conversation
ghost
added
area-auth
|
|
| { | ||
| var filter = $"(&(objectClass=group)(sAMAccountName={groupCN}))"; // This is using ldap search query language, it is looking on the server for someUser | ||
| retrievedClaims.Add(groupCN); |
There was a problem hiding this comment.
I've moved it here from under if clause, but I'm actually not sure.
Basically what I did was compare the results of this code in Linux vs Windows.
I noticed one difference in their behaviour:
- When using
filterbelow on Windows, the results include all groups, including distribution groups (email distribution lists). - On Linux, distribution groups are not resolved.
There was a problem hiding this comment.
Maybe it warrants a separate issue.
|
Generally we want individual issues rather than PRs first, so we can validate the scenario As you can see with the lack of tests, this is frankly hard to test. We need a suitable ldap server configuration documented that we can reproduce, and then we run manual tests. So can you give us an idea of what setup you have, including some sample users, as well as your docker configuration and how you're authenticating and we can go from there. |
|
Just to update, I plan on documenting the requested stuff here in 1-2 weeks. |
|
Update on 7f72499: |
|
I've filed an API proposal at dotnet/runtime#59900 |
|
Can the recursive issue be isolated as its own PR as it's likely to get merged upstream much faster than proposed API changes? |
|
Something worth mentioning. Kerberos tickets will come with AD group SIDs. If group names to sids mapping can be cached, LDAP queries can be avoided for a large number of requests when authentication is done via Kerberos. See this on how this can be implemented (though in the linked code I preloaded and keep in sync all AD groups as background process so no LDAP queries are ever issued per-request, greatly speeding up request pipeline. The downside of my approach is higher memory consumption if there are a lot of AD groups that need to be cached in memory). |
Yeah, I think that would be a better approach. |
|
@RoadTrain Can this PR be closed now since the recursion fixes were done separately and the LDAP work is being tracked in the dotnet/runtime issue? |
|
@adityamandaleeka Yes, I will close this PR for now. |
This PR fixes three issues with current RBAC claims resolution code. Every issue is fixed in a separate commit, to simplify review.
We have been trying to set up claims resolution for our services running in docker, and it was crashing when we set
IgnoreNestedGroups = true. So I did some research, and found three issues which I managed to fix.c6c4516: Fix nested claims retrieval -- use CN instead of DN
I think this one is simple. Naturally, we should use group canonical name (CN) as a claim value. But for some reason the code used group distinguished name (DN) instead. So instead of
SomeGroup, the claim value would beCN=SomeGroup,DC=domain,DC=COM, which is obviously not what we want.bba75b7: Keep track of already processed group to account for possible circular references
This one fixes a particular corner case which is possible in real-world AD deployments, namely circular group references.
That is, e.g.
GroupA -> GroupB -> GroupC -> GroupA. This is a problematic scenario for any software working with AD.The fix is basically to keep track of groups that we already processed.
Fixes #37225
7f72499: Escape LDAP filter arguments
This one fixes another real-world scenario. It's possible for a group CN to include characters such as
(or), for exampleSome (very) important group. Parentheses in partucular are a part of filter syntax. When we construct the LDAP filter expression and don't escape these symbols, openldap fails withLdapException: The search filter is invalid. So the solution is to simply escape them according to the relevant RFC.This one might be somewhat related to dotnet/runtime#38609.
The caveat here is that on Windows unescaped filter is working, while on Linux it doesn't. There's likely a difference in the way underlying native libraries (Wldap32 vs openldap) operate.
TODO: