Fixed LDAP nested group claims #55707

y4r9 · 2024-05-14T10:54:56Z

Fixed LDAP based claims

Claims retrieved from LDAP return unique group memberships with SIDs.

Description

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 in LDAP)

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.

Fixes #55705

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>

y4r9 · 2024-05-14T16:39:06Z

@dotnet-policy-service agree

src/Security/Authentication/Negotiate/src/Internal/LdapAdapter.cs

gfoidl · 2024-05-14T17:26:22Z

src/Security/Authentication/Negotiate/src/Internal/LdapAdapter.cs

+ 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];


Same comments as above.

src/Security/Authentication/Negotiate/src/Internal/LdapAdapter.cs

src/Security/Authentication/Negotiate/test/Negotiate.Test/NegotiateHandlerTests.cs

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.

halter73 · 2024-05-21T20:45:17Z

src/Security/Authentication/Negotiate/src/Internal/LdapAdapter.cs

+ }
+
+ var uniqueGroups = new HashSet<string>();
+ if (memberof is not null)


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.

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.

halter73 · 2024-05-21T20:47:14Z

src/Security/Authentication/Negotiate/src/Internal/LdapAdapter.cs

- else
+ }
+
+ var uniqueGroups = new HashSet<string>();


Can we lazily allocate this only when !settings.IgnoreNestedGroups and the group membership is non-empty?

Yes, thank you for pointing this out.

src/Security/Authentication/Negotiate/src/Internal/LdapAdapter.cs

halter73 · 2024-05-21T20:50:29Z

src/Security/Authentication/Negotiate/src/Internal/LdapAdapter.cs

+ }
+ else
+ {
+ retrievedClaims.Add(new KeyValuePair<string, string>(identity.RoleClaimType, groupCN));


Nit: These KVPs are a great candidate for using target-typed new.

Suggested change

retrievedClaims.Add(new KeyValuePair<string, string>(identity.RoleClaimType, groupCN));

retrievedClaims.Add(new(identity.RoleClaimType, groupCN));

Using a value tuple would make it even shorter:

Suggested change

retrievedClaims.Add(new KeyValuePair<string, string>(identity.RoleClaimType, groupCN));

retrievedClaims.Add((identity.RoleClaimType, groupCN));

halter73 · 2024-05-21T21:09:17Z

src/Security/Authentication/Negotiate/src/Internal/LdapAdapter.cs

+ return;
+ }
+
+ retrievedClaims.Add(new KeyValuePair<string, string>(principal.RoleClaimType, groupCN));


Why not add this whether or not there's any SearchResponse.Entries like before?

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?

halter73 · 2024-05-21T21:25:21Z

src/Security/Authentication/Negotiate/src/Internal/LdapAdapter.cs

@@ -129,12 +202,145 @@ private static void GetNestedGroups(LdapConnection connection, ClaimsIdentity pr
 if (processedGroups.Contains(nestedGroupDN))


Is this still necessary given we have the same check at the top of the method?

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.

halter73 · 2024-05-21T21:26:08Z

src/Security/Authentication/Negotiate/src/Internal/LdapAdapter.cs

+ {
+ 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);


This seems mostly copied from GetNestedGroups. Can we share the logic?

halter73 · 2024-05-21T21:39:21Z

src/Security/Authentication/Negotiate/src/Internal/LdapAdapter.cs

+ 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


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

halter73 · 2024-05-21T21:44:33Z

src/Security/Authentication/Negotiate/test/Negotiate.Test/NegotiateHandlerTests.cs

@@ -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"));


Can we test caching the Sid and GroupSid claims now?

halter73 · 2024-05-21T21:54:26Z

src/Security/Authentication/Negotiate/src/Internal/LdapAdapter.cs

+ var usid = ParseSID((byte[])userSID[0]);
+ if (usid is not null)
+ {
+ retrievedClaims.Add(new KeyValuePair<string, string>(ClaimTypes.Sid, usid.ToString()));


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]>

dotnet-issue-labeler bot added the area-security label May 14, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label May 14, 2024

y4r9 added 2 commits May 14, 2024 18:33

Update NegotiateHandlerTests.cs

a4148a7

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>

Merge branch 'dotnet:main' into y4r9/linux-negotiate-claims-fix

9f07e32

gfoidl reviewed May 14, 2024

View reviewed changes

y4r9 and others added 8 commits May 14, 2024 19:38

Update src/Security/Authentication/Negotiate/src/Internal/LdapAdapter.cs

bad1397

Co-authored-by: Günther Foidl <[email protected]>

Update src/Security/Authentication/Negotiate/src/Internal/LdapAdapter.cs

b5438ec

Co-authored-by: Günther Foidl <[email protected]>

Apply suggestions from code review

c93fbcc

Correct code formatting Co-authored-by: Günther Foidl <[email protected]>

Update LdapAdapter.cs

6eb4e18

More code formatting

Update NegotiateHandlerTests.cs

1b8623b

Fix missing new

Update LdapAdapter.cs

33b1fde

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.

Update LdapAdapter.cs

cbce3fa

Required to change from simple if statement to pattern matching

Update NegotiateHandlerTests.cs

ea3bab0

Fixed use of the wrong schema for the claim.

y4r9 changed the title ~~Update LdapAdapter.cs~~ Fixed LDAP nested group claims May 15, 2024

mkArtakMSFT assigned halter73 May 15, 2024

halter73 reviewed May 21, 2024

View reviewed changes

Fixed cache size calculation for claims cache

0f58291

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]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed LDAP nested group claims #55707

Fixed LDAP nested group claims #55707

y4r9 commented May 14, 2024

y4r9 commented May 14, 2024

gfoidl May 14, 2024

halter73 May 21, 2024

y4r9 May 22, 2024

halter73 May 21, 2024

y4r9 May 27, 2024

halter73 May 21, 2024

halter73 May 21, 2024

y4r9 May 22, 2024

halter73 May 21, 2024

y4r9 May 23, 2024

halter73 May 21, 2024

halter73 May 21, 2024

halter73 May 21, 2024

halter73 May 21, 2024

	retrievedClaims.Add(new KeyValuePair<string, string>(identity.RoleClaimType, groupCN));
	retrievedClaims.Add(new(identity.RoleClaimType, groupCN));

	retrievedClaims.Add(new KeyValuePair<string, string>(identity.RoleClaimType, groupCN));
	retrievedClaims.Add((identity.RoleClaimType, groupCN));

		@@ -129,12 +202,145 @@ private static void GetNestedGroups(LdapConnection connection, ClaimsIdentity pr
		if (processedGroups.Contains(nestedGroupDN))

Fixed LDAP nested group claims #55707

Are you sure you want to change the base?

Fixed LDAP nested group claims #55707

Conversation

y4r9 commented May 14, 2024

Fixed LDAP based claims

Description

y4r9 commented May 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment