Fix UseManagedNtlm linker substitutions

[filipnavara]

Depends on dotnet/sdk#34903.

Tested with the following code:

using System.Net.Security;
var negAuth = new NegotiateAuthentication(new NegotiateAuthenticationClientOptions { });
negAuth.GetOutgoingBlob("", out _);

It saves around 100Kb when this code is published with dotnet publish -p:PublishAot=true.

Fixes #90898

[ghost]

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

Issue Details

---

• Specify the default value (false) for the feature on Linux so the linker runs the substitution when no value was specified by the user.
• Make the UseManagedNtlm property public because the linker and IL compiler doesn't support substitution of private properties.
• Add --ignore-substitutions switch to ILLink during library build to prevent the substitution with default value taking place.

Tested with the following code:

using System.Net.Security;
var negAuth = new NegotiateAuthentication(new NegotiateAuthenticationClientOptions { });
negAuth.GetOutgoingBlob("", out _);

It saves around 100Kb when this code is published with dotnet publish -p:PublishAot=true.

Fixes #90898

Author:	filipnavara
Assignees:	-
Labels:	area-System.Net.Security
Milestone:	-

[filipnavara]

cc @sbomer @vitek-karas ... I find it odd that there was no other use of --ignore-substitutions in the code base. Maybe I am doing something wrong?

[teo-tsirpanis]

How about you call it ILLink.Substitutions.Apple.xml? OSXLike does not exist in any other file.

(misclicked Approve 😅)

[filipnavara]

I have no strong preference on this, but Apple sounds good. There's at least one other case where OSX is used for both macOS and iOS.

[vitek-karas]

This feature switch is a bit weird currently:

• It's not documented in https://github.com/dotnet/runtime/blob/main/docs/workflow/trimming/feature-switches.md.
• There's no corresponding MSBuild property - even if "internal" only (underscore in front)

I would recommend:

• Adding an MSBuild property - if we don't want people to really change its value, keep it "internal", but this PR suggests that there are use cases for developers to change the value, so why not make it public. Without the property, the item group is the only way to modify this and that one is very cumbersome (it's easy to introduce duplicates and it's hard to test for the "current value", and os on)
• If we want this to be off by default on Linux, set the default value of the property to false on linux - in the SDK (not in the runtime's build, but in the app's build when this is used). That is assuming the default value everywhere else is true. It's OK if we leave the property undefined everywhere else - Linux is special in this case. It also seems we might want to default this to true for some other platforms (Mac, iOS, ...) - that would not change anything, but it would make the MSBuild property consistent with the actual runtime's behavior.
• Have the substitution file present on all platforms - by default it would not substitute anything (the feature switch value is undefined), and in library build it would not do anything either.

Make the UseManagedNtlm property public because the linker and IL compiler doesn't support substitution of private properties.

That should not be the case - we have internal properties which are substituted - for example

runtime/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.cs

Line 25 in 23b2ea3

internal static bool Invariant => Settings.Invariant;

is substituted here

runtime/src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.Shared.xml

Line 12 in 23b2ea3

<method signature="System.Boolean get_Invariant()" body="stub" value="true" feature="System.Globalization.Invariant" featurevalue="true" />

[vitek-karas]

@sbomer, @ivanpovazan, @eerhardt what do you think? You all have done something similar in the past (I really haven't) so maybe there's something I overlooked.

[filipnavara]

I am fine with introducing SDK property to control this. There are two reasons why I didn't do it at this point.

Firstly, I am not aware of any precedent where the default value of the property depends on the platform, and the default value was really a moving target (false on .NET 8 everywhere; true on .NET 9 on iOS/macOS; TBD: true on Linux Bionic). It feels really weird to have a default in SDK when we change it to fix platform-specific issues in dotnet/runtime. Moreover, the default has to match for non-linked and linked builds.

Secondly, I was considering a backport of minimal fix of this substitution to .NET 8. Doing that across several repos makes that prohibitively expensive fix.

That should not be the case - we have internal properties which are substituted -

Let me rephrase that, it didn't work for private property when I tested it. ILLink didn't do the substitution during library build (thus I didn't realize that adding featuredefault="true" breaks things), and ILC didn't do it either. I had ILC under debugger and it didn't even enumerate the private method.

[filipnavara]

After trying several different things, I couldn't come up with a solution that I would be happy with...

I submitted PR to dotnet/sdk for adding internal _UseManagedNtlm feature switch. I'll rebuild this PR to remove the changes to ILLink.Substitutions.xml and ILLink options. That should leave use with

• Changing UseManagedNtlm property from private to public
• Documenting the _UseManagedNtlm switch
• Optionally forcing the default for _UseManagedNtlm somewhere (NativeAOT integration?)

[jkotas]

I had ILC under debugger and it didn't even enumerate the private method.

Where is this enumeration? I do not think ILC should be skipping private methods for substitutions.

[filipnavara]

Where is this enumeration? I do not think ILC should be skipping private methods for substitutions.

runtime/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/BodySubstitutionParser.cs

Line 59 in 0c4329b

MethodDesc method = FindMethod(type, signature);

[jkotas]

This does not have any filtering based on the visibility. It should be enumerating all methods.

[filipnavara]

This does not have any filtering based on the visibility. It should be enumerating all methods.

I tried to make a repro but it worked this time. Not sure what I did differently before, maybe the method got trimmed during library trimming and I forgot to reload in ILSpy. Sorry for the false alarm.

[eerhardt]

Why is this only for Native AOT apps? Wouldn't we want this default value set when just PublishTrimmed=true?

[filipnavara]

I am open to placing it on a better place but I didn't really find one. As stated above, it's unfortunate that this logic would have to be in sync between dotnet/sdk (linked build-time check) and dotnet/runtime (non-linked runtime check).

[filipnavara]

runtime/src/tools/illink/src/ILLink.Tasks/build/Microsoft.NET.ILLink.targets

Line 34 in 658bdd0

<PropertyGroup Condition="'$(PublishTrimmed)' == 'true'">

Maybe this is the right place?

[vitek-karas]

As stated above, it's unfortunate that this logic would have to be in sync between dotnet/sdk (linked build-time check) and dotnet/runtime (non-linked runtime check).

I view it as a hierarchy. The runtime check is obviously the ultimate decision maker - so for example, if the given feature doesn't exist on a given platform, runtime will simply not do it, no matter what. On platforms where it's optional, the responsibility is on the SDK to decide what the value should be - which is kind of the same as saying the app's developer should decide for a specific app.

The trimming process itself should not modify the value, it just "hardcodes" it into the IL (and while doing that removes the code which can't be used anymore after the hardcoding happens). Even without trimming SDK writes it into the app (.runtimeconfig.json) it just so happens that it's not completely hardcoded, but in reality nobody edits that JSON after build really.

Inside the SDK we have multiple considerations what to set the default value to - what we think makes sense for the developer. It would make sense to set it to false if the target platform doesn't support it, but not doing so doesn't really change much (other than consistency), unless we need it for size reasons, then we should do it. Other than that, we may for example decide on a different default for trimming/AOT. We typically do that if it's likely people don't need the feature and it saves a lot of size. An example of a similar approach is the globalization support, which is off for NativeAOT. But we can probably find others as well.

[filipnavara]

The UseManagedNtlm feature has the following considerations:

• It's supported on some Unix platforms only (ie. the code is hard-coded on Windows, Android, and tvOS)
• On the platforms where the switch has effect the user may override it to either false (only use GSSAPI, even if it's buggy on particular platform) or true (always use managed NTLM and SPNEGO implementations, use GSSAPI for Kerberos only).
• The default value is false on Linux, FreeBSD (Linux Bionic is odd exception, so let's ignore it for now); true on macOS, iOS / .NET 9+; false on macOS, iOS / .NET 8. This is implemented as fallback in the UseManagedNtlm property if the AppContext switch doesn't exist.
• If we are doing trimming and the default value is false according to the rules above we want to remove the managed NTLM/SPNEGO implementation to save space. It's not expected that the AppContext switch would be modified at runtime.

I updated the PR to set the _UseManagedNtlm property in Microsoft.NET.ILLink.targets. That seems to have effect both for PublishTrimmed and PublishAot. It's also part of the dotnet/runtime repository which lessens my concern about the defaults becoming out-of-sync.

[filipnavara]

To add a bit more context, the reasons why the user may want to change the value of the switch:

• On Linux the NTLM module for GSSAPI is optional, so setting UseManagedNtlm=true provides an alternative to installing GSS-NTLMSSP package. Many distributions also ship very old version of GSS-NTLMSSP, so this also ensure more consistent behaviour across all environments where the application is deployed.
• On macOS there are multiple known bugs in the NTLM implementation. This includes sending the credentials in slightly different way, which may result in failed authentication even for credentials that work on Linux/Windows, and there are buffer overflows in the native code. Changing the default was deemed risky at the late .NET 8 stage, but we switched it for .NET 9. Thus on .NET 8 users may want to opt-in with UseManagedNtlm=true.
• For users that don't rely on NTLM but rely on Kerberos, setting UseManagedNtlm=false may provide size savings.

[sbomer]

I find it odd that there was no other use of --ignore-substitutions in the code base.

This was because there are no other uses of featuredefault in substitutions (only in descriptors) - with the exception of nativeaot corelib, which I believe doesn't get trimmed in the runtime build. We so far only use substitution XMLs to either hard-code values during the library build (no feature or featuredefault attributes), or to add support for substituting when the application is trimmed.

We don't have a way to delay the substitution of featuredefault settings, which would have helped with your scenario (--ignore-substitutions is a bit too big of a hammer since it will ignore even the unconditional substitutions). For now the recommendation is to set the defaults in MSBuild as @vitek-karas suggested.

[eerhardt]

This was because there are no other uses of featuredefault in substitutions (only in descriptors) - with the exception of nativeaot corelib, which I believe doesn't get trimmed in the runtime build. We so far only use substitution XMLs to either hard-code values during the library build (no feature or featuredefault attributes), or to add support for substituting when the application is trimmed.

See #96539. The lack of this is blocking #80398 as well.

[ivanpovazan]

LGTM!

I do have one question. Would it make sense to enable trimming (via substitutions) even when _UseManagedNtlm=true ?

[filipnavara]

Would it make sense to enable trimming (via substitutions) even when _UseManagedNtlm=true ?

Probably not. That remaining code is still used for Kerberos regardless of the value of the switch. It would only save one AppContext.GetData call.

[wfurt]

LGTM