Restructure NegotiateAuthentication implementation #87930

filipnavara · 2023-06-22T15:08:20Z

Change NegotiateAuthentication implementation to use indirection through NegotiateAuthenticationPal to the actual implementation.

The PAL implementations are merged from different sources to follow the same structure:

NTAuthentication.Managed.cs -> NegotiateAuthenticationPal.Managed.cs + NegotiateAuthenticationPal.ManagedNtlm.cs + NegotiateAuthenticationPal.ManagedSpnego.cs
NTAuthentication.Common.cs + NegotiateStreamPal.Windows.cs -> NegotiateAuthenticationPal.Windows.cs
NTAuthentication.Common.cs + NegotiateStreamPal.Unix.cs -> NegotiateAuthenticationPal.Unix.cs

This split allows to delete ContextFlagsPal, SafeDeleteNegoContext, and SafeFreeNegoCredentials abstractions that were used in NegotiateStreamPal.

Additionally, a new app context option is added (System.Net.Security.UseManagedNtlm) to unconditionally turn on managed implementation of NTLM and SPNEGO on Unix-like platforms. On Linux systems without NTLM GSSAPI provider the managed NTLM and SPNEGO implementations are turned on by default, they can be turned off by setting the System.Net.Security.UseManagedNtlm option to false.

Contributes to #87665, #80846

…ugh NegotiateAuthenticationPal to the actual implementation. The PAL implementation are merged from different sources to follow the same structure: - NTAuthentication.Managed.cs -> NegotiateAuthenticationPal.Managed.cs - NTAuthentication.Common.cs + NegotiateStreamPal.Windows.cs -> NegotiateAuthenticationPal.Windows.cs - NTAuthentication.Common.cs + NegotiateStreamPal.Unix.cs -> NegotiateAuthenticationPal.Unix.cs This split allows to delete ContextFlagsPal, SafeDeleteNegoContext, and SafeFreeNegoCredentials abstractions that were used in NegotiateStreamPal.

ghost · 2023-06-22T15:08:32Z

Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

[NOT FOR REVIEW YET, JUST WANT TO RUN THROUGH CI]

Change NegotiateAuthentication implementation to use indirection through NegotiateAuthenticationPal to the actual implementation.

The PAL implementation are merged from different sources to follow the same structure:

NTAuthentication.Managed.cs -> NegotiateAuthenticationPal.Managed.cs
NTAuthentication.Common.cs + NegotiateStreamPal.Windows.cs -> NegotiateAuthenticationPal.Windows.cs
NTAuthentication.Common.cs + NegotiateStreamPal.Unix.cs -> NegotiateAuthenticationPal.Unix.cs

This split allows to delete ContextFlagsPal, SafeDeleteNegoContext, and SafeFreeNegoCredentials abstractions that were used in NegotiateStreamPal.

Author:	filipnavara
Assignees:	-
Labels:	`area-System.Net.Security`, `community-contribution`
Milestone:	-

…Ntlm switch on Unix platforms

filipnavara · 2023-06-24T10:04:32Z

@wfurt While not quite polished and ready for review yet, this implements the changes for UseManagedNtlm switch on Unix-like platforms (Linux and macOS). ~~It doesn't contain all the fallbacks to handle missing NTLM SSP, or missing GSSAPI (Linux Bionic).~~ It, however, implements SPNEGO over managed NTLM + native Kerberos, which passes the loopback test.

Please let me know if the general direction makes sense to you. I'm still undecided whether it would make sense to have two switches (UseManagedNtlm and UseManagedSpnego) or just one (managed NTLM implying that SPNEGO is managed too and uses managed NTLM).

This reverts commit 87d0c56.

This reverts commit 91d7ce2.

…onPal

filipnavara · 2023-07-06T20:50:42Z

src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.Unix.cs

+        {
+            try
+            {
+                if (!Interop.NetSecurityNative.IsNtlmInstalled())
+                {
+                    _useManagedNtlm = !AppContext.TryGetSwitch("System.Net.Security.UseManagedNtlm", out bool useManagedNtlm) || useManagedNtlm;
+                }
+                else
+                {
+                    _useManagedNtlm = AppContext.TryGetSwitch("System.Net.Security.UseManagedNtlm", out bool useManagedNtlm) && useManagedNtlm;
+                }
+                _isGssApiAvailable = true;
+            }
+            catch (EntryPointNotFoundException)
+            {
+                // GSSAPI shim may not be available on some platforms (Linux Bionic)
+                _isGssApiAvailable = false;
+            }


FWIW I am not entirely keen on shipping the fallbacks this way.

I propose to change it to the following:

Add the optional System.Net.Security.UseManagedNtlm switch and default to false.

Do not fallback to managed NTLM/SPNEGO if the native GSSAPI is missing NTLM-SSP (ie. align behavior with .NET 7 and older).

Add ILLink script that links away ManagedNtlm and ManagedSpnego unless System.Net.Security.UseManagedNtlm is set.

Update NativeAOT linux-bionic builds to set System.Net.Security.UseManagedNtlm=true unconditionally.

Would it make sense to fallback to managed only for NTLM and not Negotiate if NTLM-SSP is missing? We would not interfere with negotiation in any way but it would provide some legacy capability

For reference, this is implementation of the proposal above: aa22b0a
It mostly reverts all the fallback paths to .NET 7 behavior and only enables them with the AppContext switch.

Doing only managed NTLM without managed SPNEGO is feasible but not very practical. HTTP servers almost universally send either Negotiate only, or both Negotiate+NTLM. In both cases this would still fail the authentication because Negotiate gets the preference. There may be some SMTP server that supports only NTLM but that's really niche use case.

That said, if you feel like we should do ManagedNtlm-only and not ManagedSpNego for !Interop.NetSecurityNative.IsNtlmInstalled() I can do it. It will just need a re-run of the whole test matrix to figure out what breaks.

I was thinking about #87568 and perhaps Azure functions were there is no control over installed packages. But I guess the AppContext should be good enough for now.

I pushed the one extra commit mentioned above. It passed CI in #89051 test PR except for few legs that failed due to infra failures. They are now re-running 🤞

…enticationPal.Unix.cs

filipnavara · 2023-07-17T19:34:14Z

I assume at this point we are pretty much out of luck for .NET 8, right @wfurt? I would like to wrap this up soon anyway...

wfurt · 2023-07-17T20:01:40Z

sorry for the delay, I will look today. can you please resolve the conflict. I saw bunch of test failures but I did not go through them.

wfurt · 2023-07-17T20:18:07Z

src/libraries/Common/tests/System/Net/Capability.Security.Unix.cs

-            // GSS on Linux does not work with OpenSSL 3.0. Fix was submitted to gss-ntlm but it will take a while to make to
-            // all supported distributions. The second part of the check should be removed when it does.
-            return Interop.NetSecurityNative.IsNtlmInstalled() && (!PlatformDetection.IsOpenSslSupported || PlatformDetection.OpenSslVersion.Major < 3);
+            return true;


Do we need the property at all? Or keep it for future?

I guess that depends on the answer in #87930 (comment). In any case I would not remove it as part of this PR, it would be separate change. Mainly there's a concern about Windows with NTLM disabled, so there would still be use for it.

…m has to be enabled explicitly; NativeAOT on Linux Bionic does that by default because it doesn't have GSSAPI and native shim

wfurt

LGTM. thanks @filipnavara

wfurt · 2023-07-18T04:05:25Z

test failures seems unrelated.

wfurt · 2023-07-18T04:09:57Z

seems like another merge conflict from #88810 @filipnavara. I can merge in the morning.

…onPal

filipnavara · 2023-07-18T06:35:07Z

seems like another merge conflict from #88810 @filipnavara. I can merge in the morning.

Fixed.

wfurt · 2023-07-18T14:03:00Z

thanks @filipnavara

filipnavara · 2023-07-18T14:11:08Z

Thanks for review!

dotMorten · 2023-08-25T02:29:44Z

@filipnavara @wfurt I just moved from .NET 7 to .NET 8 and my IWA tests on Android are now broken hitting this bit of code this PR added:

runtime/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs

Line 229 in f9b07d4

throw new PlatformNotSupportedException(SR.net_ntlm_not_possible_default_cred);

filipnavara · 2023-08-25T04:00:11Z

@dotMorten Can you file issue with some small repro? That use case was definitely not supported in .NET 7 either.

dotMorten · 2023-08-25T04:27:57Z

Yup working on it. Definitely a regression/change in behavior over 7

wfurt · 2023-08-25T17:03:47Z

I'm wondering if it just failed to authenticate in the past and now it throws...? As @filipnavara said, there is no default identity on Android we can use for NTLM.

filipnavara added 2 commits June 22, 2023 16:56

Unify impersonation level validation between Windows and Unix pllatforms

229b1af

ghost added the community-contribution Indicates that the PR has been added by a community member label Jun 22, 2023

dotnet-issue-labeler bot added the area-System.Net.Security label Jun 22, 2023

build-analysis bot mentioned this pull request Jun 22, 2023

Tracking issue for CI build timeouts #76454

Closed

Split managed NTLM and managed SPNEGO implementations; add UseManaged…

73b143c

…Ntlm switch on Unix platforms

filipnavara added 3 commits June 24, 2023 12:20

Remove debug cruft

b5836a4

Fix couple of errors in managed SPNEGO

4142cfa

Remove debug print

b1a879a

build-analysis bot mentioned this pull request Jun 24, 2023

LibraryImportGenerator.Unit.Tests crashed in CI #87951

Closed

Fix message sequence in managed NTLM; remove unused method

f082fcd

filipnavara force-pushed the NegotiateAuthenticationPal branch from 3851657 to f082fcd Compare June 24, 2023 14:04

build-analysis bot mentioned this pull request Jun 24, 2023

NuGet failing with Response status code does not indicate success: 503 (Service Unavailable) dotnet/arcade#11723

Open

5 tasks

filipnavara added 9 commits June 26, 2023 10:58

Fix fallbacks on macOS GSSAPI

665cb21

Cleanup and fallbacks for missing NTLM, GSSAPI

6c04170

Adjust tests to assume that NTLM is always available on Unix

1324d77

Don't claim NTLM support on Browser

87d0c56

Revert "Don't claim NTLM support on Browser"

0c707c4

This reverts commit 87d0c56.

Attempt to fix the browser tests

91d7ce2

Revert "Attempt to fix the browser tests"

e7fc8b5

This reverts commit 91d7ce2.

Browser test suppression

86d72dc

Respect UseManagedNtlm=false on platforms without NTLM GSSAPI provider

ee6b728

filipnavara marked this pull request as ready for review June 27, 2023 08:44

filipnavara requested a review from wfurt June 27, 2023 08:44

build-analysis bot mentioned this pull request Jun 27, 2023

[browser][MT] Wasm.Browser.Threads.Minimal sample is failing #88057

Closed

karelz assigned wfurt Jul 3, 2023

Merge remote-tracking branch 'origin/main' into NegotiateAuthenticati…

29f19fd

…onPal

filipnavara commented Jul 6, 2023

View reviewed changes

Update src/libraries/System.Net.Security/src/System/Net/NegotiateAuth…

e5ebc07

…enticationPal.Unix.cs

This was referenced Jul 14, 2023

Failed USB connection via port 54050, error 61, in tvOS arm64 Release AllSubsets_Mono #82637

Open

ComInterfaceGenerator.Unit.Tests.CompileFails.VerifyByValueMarshallingAttributeUsage tests are failing #88867

Closed

wfurt reviewed Jul 17, 2023

View reviewed changes

filipnavara and others added 2 commits July 17, 2023 22:40

Merge branch 'main' into NegotiateAuthenticationPal

c58d44e

Revert all the fallback code paths, System.Net.Security.UseManagedNtl…

aa22b0a

…m has to be enabled explicitly; NativeAOT on Linux Bionic does that by default because it doesn't have GSSAPI and native shim

filipnavara mentioned this pull request Jul 17, 2023

Negotiate authentication restructuring (ignore) #89051

Closed

wfurt approved these changes Jul 17, 2023

View reviewed changes

filipnavara requested a review from MichalStrehovsky as a code owner July 18, 2023 00:00

Merge remote-tracking branch 'origin/main' into NegotiateAuthenticati…

a69e95a

…onPal

wfurt merged commit f7ad726 into dotnet:main Jul 18, 2023

filipnavara deleted the NegotiateAuthenticationPal branch July 18, 2023 14:11

karelz added this to the 8.0.0 milestone Aug 1, 2023

NinoFloris mentioned this pull request Aug 21, 2023

NegotiateAuthentication larger code size and also brings in BigInteger #90898

Closed

dotMorten mentioned this pull request Aug 25, 2023

Android NTLM: Empty Credentials now throws PlatformNotSupportedException #91131

Closed

ghost locked as resolved and limited conversation to collaborators Sep 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructure NegotiateAuthentication implementation #87930

Restructure NegotiateAuthentication implementation #87930

filipnavara commented Jun 22, 2023 •

edited

Loading

ghost commented Jun 22, 2023

filipnavara commented Jun 24, 2023 •

edited

Loading

filipnavara Jul 6, 2023 •

edited

Loading

wfurt Jul 17, 2023

filipnavara Jul 17, 2023

filipnavara Jul 17, 2023

wfurt Jul 17, 2023

filipnavara Jul 18, 2023

filipnavara commented Jul 17, 2023

wfurt commented Jul 17, 2023

wfurt Jul 17, 2023

filipnavara Jul 17, 2023

wfurt left a comment

wfurt commented Jul 18, 2023

wfurt commented Jul 18, 2023

filipnavara commented Jul 18, 2023

wfurt commented Jul 18, 2023

filipnavara commented Jul 18, 2023

dotMorten commented Aug 25, 2023 •

edited

Loading

filipnavara commented Aug 25, 2023 •

edited

Loading

dotMorten commented Aug 25, 2023

wfurt commented Aug 25, 2023

Restructure NegotiateAuthentication implementation #87930

Restructure NegotiateAuthentication implementation #87930

Conversation

filipnavara commented Jun 22, 2023 • edited Loading

ghost commented Jun 22, 2023

filipnavara commented Jun 24, 2023 • edited Loading

filipnavara Jul 6, 2023 • edited Loading

Choose a reason for hiding this comment

wfurt Jul 17, 2023

Choose a reason for hiding this comment

filipnavara Jul 17, 2023

Choose a reason for hiding this comment

filipnavara Jul 17, 2023

Choose a reason for hiding this comment

wfurt Jul 17, 2023

Choose a reason for hiding this comment

filipnavara Jul 18, 2023

Choose a reason for hiding this comment

filipnavara commented Jul 17, 2023

wfurt commented Jul 17, 2023

wfurt Jul 17, 2023

Choose a reason for hiding this comment

filipnavara Jul 17, 2023

Choose a reason for hiding this comment

wfurt left a comment

Choose a reason for hiding this comment

wfurt commented Jul 18, 2023

wfurt commented Jul 18, 2023

filipnavara commented Jul 18, 2023

wfurt commented Jul 18, 2023

filipnavara commented Jul 18, 2023

dotMorten commented Aug 25, 2023 • edited Loading

filipnavara commented Aug 25, 2023 • edited Loading

dotMorten commented Aug 25, 2023

wfurt commented Aug 25, 2023

filipnavara commented Jun 22, 2023 •

edited

Loading

filipnavara commented Jun 24, 2023 •

edited

Loading

filipnavara Jul 6, 2023 •

edited

Loading

dotMorten commented Aug 25, 2023 •

edited

Loading

filipnavara commented Aug 25, 2023 •

edited

Loading