dotnet / runtime

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

PrincipalSearcher for machine local users is case sensitive #24887

Open bigbearzhu opened 6 years ago

bigbearzhu commented 6 years ago

Hi all,

When finding users with PrincipalSearcher.FindAll method for machine context, it seems the search is case sensitive. However if the search is for domain users, it becomes case insensitive. The reason for this is because domain search is using LDAP which is case insensitive. But machine local search goes through SAMQuerySet which uses a regex that doesn't take in RegexOptions.IgnoreCase.

Would it be safe to add that option so that the search behavior is consistent across different context types? Thanks

danmoseley commented 6 years ago

Is it the same on desktop framework?

bigbearzhu commented 6 years ago

I actually found this issue in ASP.Net App targeting .Net Framework. However I cannot find public repo contains System.DirectoryServices.AccountManagement source code for .Net Framework. Although decompiled code shows exactly the same code. That’s why I raised it here. Sorry if it is not the right place.

danmoseley commented 6 years ago

@bigbearzhu it's fine, just curious whether it was .NET Core specific. Sounds like not. Fixes we make here are considered for porting to .NET Framework.

bigbearzhu commented 6 years ago

That is awesome! Should I send a pull request or someone else will do? Sounds like a really simple fix but not sure how safe it is. No idea how this would be unit-tested as this is fundamental framework code. Thanks.

On 3 Feb 2018, at 4:31 pm, Dan Moseley notifications@github.com wrote:

@bigbearzhu it's fine, just curious whether it was .NET Core specific. Sounds like not. Fixes we make here are considered for porting to .NET Framework.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

danmoseley commented 6 years ago

@tarekgh sounds comment on your proposed change.

tarekgh commented 6 years ago

@tquerec can comment on this.

ViktorHofer commented 6 years ago

But machine local search goes through SAMQuerySet which uses a regex that doesn't take in RegexOptions.IgnoreCase.

You propose to change the local search behavior from case sensitive to case insensitive, is that right? If so, I'm not sure this isn't a breaking change as you could get more results back than before. We should check if there's documentation that outlines the current behavior.

tarekgh commented 6 years ago

CC @josephisenhour @bongiovimatthew-microsoft @tquerec

sachinpandey122 commented 4 years ago

Hi all, do we have any conclusion on this yet? Thanks,

webatxcent commented 4 years ago

I am not sure why this issue has any push back. The local security authority will not let you create two user accounts with the same name regardless of case. Performing a case insensitive search shouldn't return any more results. Is this a fix that could be forth coming?

danmoseley commented 4 years ago

@joperezr (since you have more context on this area now) -- thoughts? is this simply wrong and an easy fix?

danmoseley commented 4 years ago

Also this comment just above I believe is no longer accurate. We can release DynamicMethods, so RegexOptions.Compiled does not cause unbounded growth. (It does add startup time though so it's not necessarily a good thing, but if throughput matters more then it's a good thing)

            // Ideally, we'd like to use a compiled Regex (RegexOptions.Compiled) for performance,
            // but the CLR cannot release generated MSIL.  Thus, our memory usage would grow without bound
            // each time a query was performed.
joperezr commented 4 years ago

The one I'm very familiar with is System.DirectoryServices.Protocols which in Windows it is just implemented on top of wldap32 which means for all searches it uses the LDAP protocol search, which as this issue points out it is case insensitive. AccountManagement namespace is a bit more complex since it supports more specific types of directories like ADDS, ADLDS, and MSAM which is the one that seems to have the case sensitive issue here. I'm not too familiar with that so as @tarekgh suggested, I would like to know what does @tquerec think about this one as MSAM might have some different characteristics unlike traditional ADDS.

tquerec commented 4 years ago

@joperezr I believe this is just a bug in the API. MSAM ( local account store ) is case insensitive so the regex should be updated to be case insensitive.

simon-biber commented 2 years ago

Is there any workaround to find users in the local machine quickly and with case insensitive usernames?

For AD users we are using:

                return UserPrincipal.FindByIdentity(context, domainBackslashUserName);

but that is very slow for Machine users - I have read it's a NetBIOS timeout per network adapter - so we are using the following which is much faster for the local machine:

                using var user = new UserPrincipal(context);
                user.SamAccountName = username;
                using var searcher = new PrincipalSearcher(user);                
                return searcher.FindOne() as UserPrincipal;