dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.97k stars 4.66k forks source link

PrincipalContext.ValidateCredentials against the local SAM store fails with a PrincipalOperationException after any successful call to ValidateCredentials against the local SAM store #106905

Open KaizerT opened 3 weeks ago

KaizerT commented 3 weeks ago

Description

In summary, when validating a local user, PrincipalContext.ValidateCredentials throws an exception after a single local user is verified. It doesn't matter if the credentials are correct or not, the exception is thrown. The same code works when validating against an Active Directory.

This is a duplicate of the issue below. https://github.com/dotnet/runtime/issues/83269

In the issue it is said to have been broken for version 7.0 and 8.0 and fixed last year. However we are running with 8.0 and are experiencing the same issue.

I've tried downgrading System.DirectoryServices and System.DirectoryServices.AccountManagement to 7.0 and 6.0 with the same issue.

Reproduction Steps

PrincipalContextIssueRepro.zip Here's a small sample project that duplicates the behavior

Expected behavior

Unlimited amount of local user credentials can be verified

Actual behavior

Exception is thrown after 1 user is successfully verified. After that only that user can be verified. Exception is thrown for other users or when the first user is verified with the wrong password.

Regression?

This is a duplicate issue https://github.com/dotnet/runtime/issues/83269

Known Workarounds

Workaround is restarting the application (recycle for web apps) but would break right after.

Configuration

.net 8.0.6 Windows 10 Enterprise Build 19045.4651 x64 Windows 11 Pro Build 22621.4037 x64 Vistual Studio Enterprise 2022 17.10.4

Other information

No response

dotnet-policy-service[bot] commented 3 weeks ago

Tagging subscribers to this area: @dotnet/area-system-directoryservices, @jay98014 See info in area-owners.md if you want to be subscribed.

ericstj commented 3 weeks ago

@jkoritzinsky can you have a look? It seems that this was fixed by you last year, https://github.com/dotnet/runtime/commit/b039eab2303355bc3a306cfb6f234b9a63a180e2

I confirm that fix shipped in 8.0, and was backported to 7.0 in System.DirectoryServices.AccountManagement 7.0.1. The repro is using package version 8.0.0.

buyaa-n commented 1 week ago

@jkoritzinsky looks the finally block that releases the pointer in UnsafeNativeMethods.cs supposed to added to the outer try/catch block as it calls Interop.Activeds.ADsOpenObject in outer block https://github.com/dotnet/runtime/blob/a3365e4c442a728bc82e59d5583cf83bc6d31d72/src/libraries/System.DirectoryServices/src/Interop/UnsafeNativeMethods.cs#L35-L50

ericstj commented 3 days ago

@buyaa-n were you able to verify that's the problem here? It seems unusual since the outer try/catch is handling an EntryPointNotFoundException which I would expect to only be hit if there was an issue invoking ADsOpenObject - therefor nothing to release.

I gave the repro a try, first by adapting to a console app, then trying the original winforms application. Validating local user accounts I created for the purpose of repro. I could not reproduce this issue.

@KaizerT - I'm not sure what you're hitting but your repro worked for me. One thing I did notice about your repro code is that you leak the PrincipalContext and UserPrincipal types - but I don't think that should matter here. Here's a screen cap of what I see: Recording-20240916_114150.webm

KaizerT commented 3 days ago

Hello @ericstj ,

I just tried it again and I'm getting the same error, this is on the same repro and I even fixed the context leak on the project. Here's a recording I made to show the behavior.

https://github.com/user-attachments/assets/c6c950f8-a81a-4187-9440-d35adb714f49

Here's the full exception

System.DirectoryServices.AccountManagement.PrincipalOperationException: An extended error has occurred.

---> System.Runtime.InteropServices.COMException (0x800704B8): An extended error has occurred.

at System.DirectoryServices.AccountManagement.UnsafeNativeMethods.IADs.Get(String bstrName) at System.DirectoryServices.AccountManagement.CredentialValidator.BindSam(String target, String userName, String password) --- End of inner exception stack trace --- at System.DirectoryServices.AccountManagement.CredentialValidator.BindSam(String target, String userName, String password) at System.DirectoryServices.AccountManagement.CredentialValidator.Validate(String userName, String password) at System.DirectoryServices.AccountManagement.PrincipalContext.ValidateCredentials(String userName, String password) at PrincipalContextIssueRepro.Form1.cmdLogin_Click(Object sender, EventArgs e) in C:\Users\MDumaraos\source\repos\PrincipalContextIssueRepro\PrincipalContextIssueRepro\Form1.cs:line 51

KaizerT commented 3 days ago

I was able to reproduce this in separate environments, one a physical laptop, and one on a Windows 11 VM. We're just pretty lost on what could've caused this since it's not doing domain verification at this point, and even then the Windows 11 VM was unjoined from any domain. A point in the right direction would be appreciated, thank you. @ericstj

ericstj commented 3 days ago

The callstack helps.

ericstj commented 3 days ago

This looks different than #83269. The call to ADsOpenObject was successful, but the object returned is failing when the Get method is called. https://github.com/dotnet/runtime/blob/76f10f9f3c995c13c53cd9be743fdac918fbf61a/src/libraries/System.DirectoryServices.AccountManagement/src/System/DirectoryServices/AccountManagement/Context.cs#L126 It's failing with 0x800704B8 which is ERROR_EXTENDED_ERROR or one of over dozen LDAP specific error codes: https://learn.microsoft.com/en-us/windows/win32/adsi/win32-error-codes-for-adsi-2-0 I can't tell what that might mean from https://learn.microsoft.com/en-us/windows/win32/api/iads/nf-iads-iads-get as I'm no AD expert. This library is actually owned by @BRDPM @grubioe @jay98014 but we try to maintain it and triage any issues.

Just to double-check @KaizerT you mentioned this doesn't work in 6.0, 7.0, and 8.0 for you, right? Did it ever work? Have you tried package version 5.0.0 or 4.7.0 or System.DirectoryServices.AccountManagement? Did it work on .NETFramework? Just trying to understand where the problem might be.

In the previous issue we were able to isolate the problem to a change made to interop. In this case we don't have anything that shows this ever working, nor do we have a repro of your problem to debug.

KaizerT commented 3 days ago

Hello eric, We've confirmed that it works on .Net Framework 4.8 since this code is basically copied form the UI part of the system and was just updated to .Net 8, this was confirmed in both of the environments where it failed during our tests. I haven't tried any packages below 6.0 but I can try with the repro project. I only tried the lower version of the package in our actual microservice.

KaizerT commented 3 days ago

@ericstj I've uploaded the repro to a public repo and added branches for all the lower versions to 4.7.0. All versions had the same issue with my windows 10 machine.

Also, regarding System.DirectoryServices.AccountManagement, if you check the repro it's already being used by the application. image

Do I need to create this ticket in another repo for the right team to work on it and investigate or is this fine in this project?

Thanks for the help so far.

ericstj commented 3 days ago

If I understand correctly, you mention this has always been broken, even using package version 4.7.0 - but it works on .NETFramework? (apologies for closing this again , that was unintentional)

KaizerT commented 3 days ago

Yes, this has been broken for local authentication since we created the project for .net 8.0. It works fine when authenticated against a domain.

Local and domain authentication works fine on our web UI project using .Net Framework. The package is not explicitly referenced in the .Net Framework project due to the nature of .Net Framework containing everything it can by default though.

Would it help if I create another project but in .Net Framework?

ericstj commented 3 days ago

I'm not sure your repro will help us in any case as right now, it's no-repo for us.

If every version of those packages is broken in the same way for you, then we also don't have a change to look to as a possible source of regression. It could be a problem with the initial port of this code to .NETCore if you're saying this exact same app works on .NET Framework, but not on any version of .NETCore.

KaizerT commented 3 days ago

ok, thanks for investigating it so far and looking forward when the rest of the package owners can have a look at it.

ericstj commented 3 days ago

I gave your repro a try on two different machines here. One domain joined and one not. Both cases I created new users with net user name password /add to run the test. In both cases the app worked correctly for me. I'm using Windows 11 23H2, x64.

One thing I noticed that's somewhat unusual is what this method actually does to check the user name. It's fetching the server computer object: https://github.com/dotnet/runtime/blob/657f18dbd8ac9ffae3abb79104d038155387df31/src/libraries/System.DirectoryServices.AccountManagement/src/System/DirectoryServices/AccountManagement/Context.cs#L88

Then accessing the name property of that. Is there anything unusual about the computer name of the systems you're testing?

KaizerT commented 3 days ago

For the computer names, I don't think we have weird ones. The environments are as follows:

  1. Windows 10 Enterprise, 22H2 x64, Name: USLKZ220C9Y3
  2. Windows 11 Pro 22H2 x64, Name: Win11Pro-DXT31
ericstj commented 2 days ago

On the off-chance that there is some runtime / interop bug we're missing that's causing this behavior -- could you please share your .NET 8.0 runtime version? You can get this from dotnet --info. I'm still perplexed that we can't reproduce your problem.

KaizerT commented 2 days ago

Here's for my dev environment, the windows 10 laptop:

image

And here's the windows 11 VM

image