-
Notifications
You must be signed in to change notification settings - Fork 9.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed LDAP nested group claims #55707
base: main
Are you sure you want to change the base?
Conversation
Fix: - Returning group names as claim only once (unique) instead of multiple times when resolving nested groups - Handling users without an explicit group membership without null exception (happens when a user is only an implicit group member without "memberof" attributes) Added: - Adds user and group SIDs as claims which is the default on windows. This should enable writing more portable code. Todo: - For some reason the SID of builtin groups is returned as string. Only used empirical results on local machine (using a samba ad and not a windows server) to check how to interpret the string but should investigate the cause. The code for the SID parsing is derived from SID.cs of the .NET runtime library, because SID parsing is not available on Linux.
Fix problem caused by commit 7193260 where the claims cache was changed from an enumerable of strings to an enumerable of a KeyValuePair<string, string>
@dotnet-policy-service agree |
src/Security/Authentication/Negotiate/src/Internal/LdapAdapter.cs
Outdated
Show resolved
Hide resolved
src/Security/Authentication/Negotiate/src/Internal/LdapAdapter.cs
Outdated
Show resolved
Hide resolved
src/Security/Authentication/Negotiate/src/Internal/LdapAdapter.cs
Outdated
Show resolved
Hide resolved
src/Security/Authentication/Negotiate/src/Internal/LdapAdapter.cs
Outdated
Show resolved
Hide resolved
src/Security/Authentication/Negotiate/src/Internal/LdapAdapter.cs
Outdated
Show resolved
Hide resolved
src/Security/Authentication/Negotiate/src/Internal/LdapAdapter.cs
Outdated
Show resolved
Hide resolved
Span<byte> groupSIDba = stackalloc byte[groupSIDspn.Length]; | ||
for (int i = 0; i < groupSIDspn.Length; ++i) | ||
{ | ||
byte[] bytes = BitConverter.GetBytes(groupSIDspn[i]); | ||
groupSIDba[i] = bytes[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comments as above.
src/Security/Authentication/Negotiate/src/Internal/LdapAdapter.cs
Outdated
Show resolved
Hide resolved
src/Security/Authentication/Negotiate/src/Internal/LdapAdapter.cs
Outdated
Show resolved
Hide resolved
src/Security/Authentication/Negotiate/test/Negotiate.Test/NegotiateHandlerTests.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Günther Foidl <[email protected]>
Co-authored-by: Günther Foidl <[email protected]>
Correct code formatting Co-authored-by: Günther Foidl <[email protected]>
More code formatting
Fix missing new
Addresses comments with regard to the string to SID conversion. The stack allocation is rounded up to the next power of two and the conversion avoids creating temporary arrays.
Required to change from simple if statement to pattern matching
Fixed use of the wrong schema for the claim.
} | ||
|
||
var uniqueGroups = new HashSet<string>(); | ||
if (memberof is not null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When is memberof
null for the top-level user entry? Did you actually see this happen? If it does, it's probably cleaner to do var memberof = userFound.Attributes["memberof"] ?? new();
Or better yet, use a static readonly empty DirectoryAttribute
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this did happen to me. A freshly created unprivileged user does not have any memberof LDAP attributes (user created using windows RSAT tools but with a samba AD as primary DC). This led to a null exception. I will follow your recommendation and switch to using a static readonly empty DirectoryAttribute.
else | ||
} | ||
|
||
var uniqueGroups = new HashSet<string>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we lazily allocate this only when !settings.IgnoreNestedGroups
and the group membership is non-empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thank you for pointing this out.
src/Security/Authentication/Negotiate/src/Internal/LdapAdapter.cs
Outdated
Show resolved
Hide resolved
} | ||
else | ||
{ | ||
retrievedClaims.Add(new KeyValuePair<string, string>(identity.RoleClaimType, groupCN)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: These KVPs are a great candidate for using target-typed new.
retrievedClaims.Add(new KeyValuePair<string, string>(identity.RoleClaimType, groupCN)); | |
retrievedClaims.Add(new(identity.RoleClaimType, groupCN)); |
Using a value tuple would make it even shorter:
retrievedClaims.Add(new KeyValuePair<string, string>(identity.RoleClaimType, groupCN)); | |
retrievedClaims.Add((identity.RoleClaimType, groupCN)); |
return; | ||
} | ||
|
||
retrievedClaims.Add(new KeyValuePair<string, string>(principal.RoleClaimType, groupCN)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not add this whether or not there's any SearchResponse.Entries
like before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be the scenario where a groupCN
should be added to the claims despite not being found in LDAP (=> no SearchResponse.Entries
)? If a trusted LDAP is used this should not be able to happen, but is there any reason you would like the claim being added regardless of finding the group in LDAP?
@@ -129,12 +202,145 @@ private static void GetNestedGroups(LdapConnection connection, ClaimsIdentity pr | |||
if (processedGroups.Contains(nestedGroupDN)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still necessary given we have the same check at the top of the method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell you are right, it would not be necessary. Currently this would only avoid another recursion including one more LDAP search request before returning back to this loop, but it could be removed.
{ | ||
var filter = $"(&(objectClass=group)(sAMAccountName={groupCN}))"; // This is using ldap search query language, it is looking on the server for someUser | ||
var searchRequest = new SearchRequest(distinguishedName, filter, SearchScope.Subtree); | ||
var searchResponse = (SearchResponse)connection.SendRequest(searchRequest); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems mostly copied from GetNestedGroups
. Can we share the logic?
int length = 4; | ||
((ulong)authority).TryFormat(result.Slice(length), out int written, provider: CultureInfo.InvariantCulture); | ||
length += written; | ||
// Might need a check against a stack overflow, but this is directly taken from SID.cs of the .NET runtime library |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SecurityIdentifier.ToString()
includes an explanation in its comments why a stack overflow should be impossible, and I think that logic should hold since we're using the same MaxSubAuthorities
of 15.
We need to be careful not to let the code go out of sync. Ideally, we could make the non-platform-specific aspects of SecurityIdentifier
like its byte[]
-based constructor and ToString()
. That will require a runtime API proposal.
Otherwise, we might want to do some sort of source sharing. Perhaps we can use a github workflow to sync these like we do for some of our HTTP/2 and HTTP/3 logic
@@ -208,7 +208,9 @@ public async Task AuthHeaderAfterNtlmCompleted_ReAuthenticates(bool persist) | |||
public async Task RBACClaimsRetrievedFromCacheAfterKerberosCompleted() | |||
{ | |||
var claimsCache = new MemoryCache(new MemoryCacheOptions()); | |||
claimsCache.Set("name", new string[] { "CN=Domain Admins,CN=Users,DC=domain,DC=net" }); | |||
var claimsList = new List<KeyValuePair<string, string>>(); | |||
claimsList.Add(new KeyValuePair<string, string>("http://schemas.microsoft.com/ws/2008/06/identity/claims/role", "CN=Domain Admins,CN=Users,DC=domain,DC=net")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we test caching the Sid
and GroupSid
claims now?
var usid = ParseSID((byte[])userSID[0]); | ||
if (usid is not null) | ||
{ | ||
retrievedClaims.Add(new KeyValuePair<string, string>(ClaimTypes.Sid, usid.ToString())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is everyone who's using EnableLdap
going to want Sid
and GroupSid
? This was requested by #31959, but it seems like most people have gotten by fine without it. And it does waste space in the cache.
Also, should this include all of the SID claims mentioned by the issue?
primarysid
primarygroupsid
groupsid
denyonlysid
There's even more that aren't listed like denyonlyprimarysid
and denyonlyprimarygroupsid
.
I think this should be opt-in or at the very least opt-out which will require new public API. You can use our API proposal issue template to file a new API proposal issue, or copy the template and fill it out in a new comment on #55705. We can add the api-ready-for-review
label once it's ready.
Since the key of the claim is a reference to a const string the string size does not need to be added to the cache size. Co-authored-by: Stephen Halter <[email protected]>
Fixed LDAP based claims
Claims retrieved from LDAP return unique group memberships with SIDs.
Description
Fix:
Added:
Todo:
The code for the SID parsing is derived from SID.cs of the .NET runtime library, because SID parsing is not available on Linux.
Fixes #55705