Fixed LDAP nested group claims by y4r9 · Pull Request #55707 · dotnet/aspnetcore

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

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

Co-authored-by: Günther Foidl <gue@korporal.at>

Correct code formatting Co-authored-by: Günther Foidl <gue@korporal.at>

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.

I was looking at this again, but apart from memberof never being null I fail to see the advantage of using a static readonly empty DirectoryAttribute here. However, I might be missing something obvious, so please let me know if I do and I will change it right away.

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

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

        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?

Well this does not actually test much. It just adds the claims to the cache which is then directly copied to the claims (regardless of the claim types).

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.

I think I figured out how to add primarygroupsid by constructing it from the RID, but I could not find much useful information about denyonlysid (unfortunately my time to look into this is quite restricted so if anyone could point me in the right direction I would be very grateful).

I added primarygroupsidbut as mentioned before I am stuck with denyonlysid.

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 <halter73@gmail.com>

Incorporated suggestions from the review process: - Instantiating `uniqueGroups` only when resolving nested groups - Using target-typed new for key value pair allocations - Using simplified `if (groupSid is { Count: 1 })` instead of two nested if's

dotnet-policy-service · 2024-06-08T19:09:22Z

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

Deriving the full SID from the RID stored in the primarygroupid field retrieved by LDAP using the SID of the user.

Added the option EnableLdapSIDClaimResolution to the public API LdapSetting to make the SID resolution on Linux using LDAP optional.

…mResolution Using the added EnableLdapSIDClaimResolution to make the SID claim resolution optional.

ghost 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 <gue@korporal.at>

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

b5438ec

Co-authored-by: Günther Foidl <gue@korporal.at>

Apply suggestions from code review

c93fbcc

Correct code formatting Co-authored-by: Günther Foidl <gue@korporal.at>

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

y4r9 and others added 2 commits May 23, 2024 10:25

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 <halter73@gmail.com>

Incorporated suggestions and comments

9b583c0

Incorporated suggestions from the review process: - Instantiating `uniqueGroups` only when resolving nested groups - Using target-typed new for key value pair allocations - Using simplified `if (groupSid is { Count: 1 })` instead of two nested if's

dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jun 8, 2024

Added primaryGroupID as claim

7feee25

Deriving the full SID from the RID stored in the primarygroupid field retrieved by LDAP using the SID of the user.

y4r9 mentioned this pull request Jun 26, 2024

Group claim duplication when using Negotiate authentication on Linux AD domain member with LDAP #55705

Open

1 task

y4r9 added 2 commits June 27, 2024 12:49

Added EnableLdapSIDClaimResolution to LdapSettings

8e56675

Added the option EnableLdapSIDClaimResolution to the public API LdapSetting to make the SID resolution on Linux using LDAP optional.

Added EnableLdapSIDClaimResolution to the public API of LdapSettings

c75536b

Added the option EnableLdapSIDClaimResolution to the public API LdapSetting to make the SID resolution on Linux using LDAP optional.

y4r9 requested a review from a team as a code owner June 27, 2024 10:50

Made LDAP based SID claim resolution optional using EnableLdapSIDClai…

f2cfebd

…mResolution Using the added EnableLdapSIDClaimResolution to make the SID claim resolution optional.

This was referenced Apr 4, 2026

Quarantine Re-quarantine WebWorkerTemplateE2ETest tests (CanInvokeMethods, HandlesErrors, CanDisposeWorker) #66138

Closed

Quarantine ServerVirtualizationTest.DynamicContent_PrependItemsWhileScrolledToMiddle_VisibleItemsStayInPlace #66139

Open

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

Conversation

y4r9 commented May 14, 2024

Fixed LDAP based claims

Description

Uh oh!

y4r9 commented May 14, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dotnet-policy-service bot commented Jun 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants