Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use managed NTLM/SPNEGO on Apple platforms by default #89267

Merged
merged 1 commit into from
Aug 15, 2023

Conversation

filipnavara
Copy link
Member

Apple implementation of NTLM has two major compatibility issues:

Let's try to use the managed NTLM implementation on Apple platforms now that we can support it with Kerberos at the same time. There's still an opt-out through setting UseManagedNtlm app context switch to false.

Fixes #65678
Fixes #82547

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 20, 2023
@ghost
Copy link

ghost commented Jul 20, 2023

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

Issue Details

Apple implementation of NTLM has two major compatibility issues:

Let's try to use the managed NTLM implementation on Apple platforms now that we can support it with Kerberos at the same time. There's still an opt-out through setting UseManagedNtlm app context switch to false.

Fixes #65678
Fixes #82547

Author: filipnavara
Assignees: -
Labels:

area-System.Net.Security

Milestone: -

@karelz
Copy link
Member

karelz commented Aug 3, 2023

@filipnavara we discussed it and we fear that it might cause regressions. What is your confidence?
Given that we are close to fork of 8.0 into RC branch (around 8/14), perhaps we might wait with this for 9.0 (when main becomes 9.0 branch). Would you be ok with it?

@filipnavara
Copy link
Member Author

@filipnavara we discussed it and we fear that it might cause regressions. What is your confidence? Given that we are close to fork of 8.0 into RC branch (around 8/14), perhaps we might wait with this for 9.0 (when main becomes 9.0 branch). Would you be ok with it?

Sure. There's already opt-in for .NET 8 so I am fine with doing this in .NET 9 with a bit more of a leeway.

@wfurt
Copy link
Member

wfurt commented Aug 3, 2023

we can always pull it into servicing if we get decent verification and there is need.

@wfurt wfurt added this to the 9.0.0 milestone Aug 3, 2023
@filipnavara
Copy link
Member Author

What is your confidence?

There are few things to balance here.

Apple's NTLM implementation has both compatibility issues and known buffer overflows that are relatively easy to trigger. Apple has shown no interest in fixing the buffer overflows. Since they don't happen in the core authentication flow in HTTP and/or SMTP I don't expect people to run into it though. The compatibility issues are difficult to diagnose and several people run into it already. So, as far as NTLM itself is concerned I am quite confident that the managed implementation is better choice.

Unfortunately, to offer consistent experience we have to use managed SPNEGO as well, and that received very little testing so far. I expect us to switch our application to it in the opt-in mode once .NET 8 is released, and we should get enough exposure throughout the .NET 9 timeline.

@karelz karelz added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 4, 2023
Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wfurt wfurt merged commit 9d53816 into dotnet:main Aug 15, 2023
100 of 103 checks passed
@filipnavara filipnavara deleted the managedntlm-macos branch August 15, 2023 19:50
@am11 am11 removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 26, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Security community-contribution Indicates that the PR has been added by a community member
Projects
None yet
4 participants