Reimplement NegotiateStream using (mostly) public NegotiateAuthenticaton API #86948

filipnavara · 2023-05-31T11:39:17Z

Adds support for tvOS and Android as a side-effect.

…tion API

ghost · 2023-05-31T11:39:24Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2023-05-31T11:39:28Z

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

Issue Details

Adds support for tvOS and Android as a side-effect.

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

filipnavara · 2023-05-31T11:40:28Z

src/libraries/apicompat/ApiCompatBaseline.NetCoreAppLatestStable.xml

-  <Suppression>
-    <DiagnosticId>CP0015</DiagnosticId>
-    <Target>M:System.Type.GetEnumValues:[T:System.Diagnostics.CodeAnalysis.RequiresDynamicCodeAttribute]</Target>
-    <Left>net7.0/mscorlib.dll</Left>
-    <Right>net8.0/mscorlib.dll</Right>
-  </Suppression>
-  <Suppression>
-    <DiagnosticId>CP0015</DiagnosticId>
-    <Target>M:System.Type.GetEnumValues:[T:System.Diagnostics.CodeAnalysis.RequiresDynamicCodeAttribute]</Target>
-    <Left>net7.0/netstandard.dll</Left>
-    <Right>net8.0/netstandard.dll</Right>
-  </Suppression>
-  <Suppression>
-    <DiagnosticId>CP0015</DiagnosticId>
-    <Target>M:System.Type.GetEnumValues:[T:System.Diagnostics.CodeAnalysis.RequiresDynamicCodeAttribute]</Target>
-    <Left>net7.0/System.Runtime.dll</Left>
-    <Right>net8.0/System.Runtime.dll</Right>
-  </Suppression>


This is side-effect of rebuilding the API compat file.

…iateStream to work

wfurt · 2023-06-07T20:20:18Z

src/libraries/Common/src/System/Net/Security/NegotiateStreamPal.Windows.cs

        }

-        private static unsafe int DecryptNtlm(
+        internal static unsafe void GetMIC(


Would this produce same output e.g. would it be compatible with other versions of NegotiateStream?

Yes, the NTLM code should be compatible.

(There's a part of code where I fixed the on-wire transmission of error codes. It was broken ever since Unix support was added and didn't follow the spec or the .NET Framework version.)

it is Kerberos what worries me more. And SqlClient uses fragments of this as well. (dotnet/SqlClient#303)

cc: @JRahnama @DavoudEshtehari for visibility

There should not be any change of the on-wire data with this PR with two exceptions:

The fixed error codes that now match the spec and .NET Framework

Write sends the frame length separately from the data to the underlying transport stream (valid according to Stream contract but may be problematic if someone was using NegotiateStream to implement Negotiate algorithm, and stripping the frame headers)

There's no modification of the native shim beyond what was already done in .NET 7.

wfurt · 2023-06-07T20:48:37Z

src/libraries/System.Net.Security/src/System/Net/Security/NegotiateStream.cs

-                    exception = new AuthenticationException(SR.Format(SR.net_auth_context_expectation, result.ToString(), _expectedProtectionLevel.ToString()));
-                    int statusCode = ERROR_TRUST_FAILURE;
-                    message = new byte[sizeof(long)];
+                message = new byte[sizeof(long)];


can we make it static constant?

I suppose we can but I am not sure it's worth it. It's an error condition that can happen only once per connection. Is it really worth optimizing an error code path?

while the impact is small it also seem super easy...

It was not done previously. The code basically stayed the same. I felt the diff was hard enough to read as-is.

wfurt

thanks @filipnavara. generally looks good to me but it is big cleanup. I wish we can test agains previous implementation but our tests are not set up for that.

filipnavara · 2023-06-08T01:42:21Z

I wish we can test against previous implementation but our tests are not set up for that.

I think I can test that manually. I'll report back on the results.

wfurt · 2023-06-08T06:23:10Z

I wish we can test against previous implementation but our tests are not set up for that.

I think I can test that manually. I'll report back on the results.

That would be great. Possibly also cover dotnet/SqlClient#1390 - probably does not need any real database.

filipnavara · 2023-06-08T11:52:00Z

Test case

using System;
using System.IO.Pipes;
using System.Net.Security;
using System.Text;

class Program
{
    public static void Main(string[] args)
    {
        if (args.Length == 0)
            return;

        bool isClient = args[0] == "client";

        if (isClient)
        {
            using var pipeStream = new NamedPipeClientStream(".", "negtest", PipeDirection.InOut, PipeOptions.None);
            pipeStream.Connect();
            using var negStream = new NegotiateStream(pipeStream);
            negStream.AuthenticateAsClient();
            negStream.Write("hello"u8.ToArray(), 0, 5);
            negStream.Flush();
        }
        else
        {
            using var pipeStream = new NamedPipeServerStream("negtest", PipeDirection.InOut);
            pipeStream.WaitForConnection();
            using var negStream = new NegotiateStream(pipeStream);
            negStream.AuthenticateAsServer();
            byte[] buffer = new byte[5];
            negStream.Read(buffer, 0, 5);
            Console.WriteLine(Encoding.UTF8.GetString(buffer));
        }
    }
}

I tested the basic communication between all the variations of .NET 4.8, .NET 8 Preview 4 and runtime build from this PR (on Windows). In all cases the communication was successfully established and the data were transferred.

I modified the code above to test both Signed-only and Encrypted-and-Signed scenarios for NTLM (on Windows) since that's the part that had the largest level of refactoring.

filipnavara · 2023-06-09T15:15:47Z

I checked that Linux-Linux communication also works. I tried Windows-Linux too but run into W11 rejecting the NTLM exchange from Linux (unrelated to the changes in this PR) but at least it showed that the transmission of error codes over the wire works as well.

wfurt

thanks @filipnavara

wfurt · 2023-06-16T07:12:56Z

can you please resolve the conflict @filipnavara? Ill merge it.

filipnavara · 2023-06-16T07:13:45Z

Sure, will do it in few minutes.

src/libraries/apicompat/ApiCompatBaseline.NetCoreAppLatestStable.xml

suppression changes

filipnavara · 2023-06-16T07:29:32Z

can you please resolve the conflict @filipnavara? Ill merge it.

done

Reimplement NegotiateStream using (mostly) public NegotiateAuthentica…

e373666

…tion API

ghost added community-contribution Indicates that the PR has been added by a community member area-System.Net.Security new-api-needs-documentation labels May 31, 2023

filipnavara requested a review from wfurt May 31, 2023 11:39

filipnavara commented May 31, 2023

View reviewed changes

filipnavara mentioned this pull request May 31, 2023

[API Proposal]: Add public [Compute/Verify]IntegrityCheck methods to NegotiateAuthentication #86950

Closed

filipnavara marked this pull request as ready for review May 31, 2023 13:01

Minor adjustment to NegotiateStreamPal on Android/tvOS to allow Negot…

b6338cb

…iateStream to work

This was referenced May 31, 2023

Tracking issue for CI build timeouts #76454

Closed

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

Closed

wfurt reviewed Jun 7, 2023

View reviewed changes

wfurt approved these changes Jun 12, 2023

View reviewed changes

wfurt mentioned this pull request Jun 16, 2023

Things needing System.Net.Security.Native may not work on Bionic #87665

Closed

Merge branch 'main' into negotiatestream-cleanup

45825f6

filipnavara commented Jun 16, 2023

View reviewed changes

src/libraries/apicompat/ApiCompatBaseline.NetCoreAppLatestStable.xml Show resolved Hide resolved

Remove unrelated ApiCompat

55034c8

suppression changes

wfurt merged commit 77ad806 into dotnet:main Jun 16, 2023

filipnavara deleted the negotiatestream-cleanup branch June 16, 2023 10:50

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

System.Text.Json failing some large file tests #59678

Closed

karelz added this to the 8.0.0 milestone Jul 3, 2023

ghost locked as resolved and limited conversation to collaborators Aug 2, 2023

Reimplement NegotiateStream using (mostly) public NegotiateAuthenticaton API #86948

Reimplement NegotiateStream using (mostly) public NegotiateAuthenticaton API #86948

Uh oh!

Conversation

filipnavara commented May 31, 2023

Uh oh!

ghost commented May 31, 2023

Uh oh!

ghost commented May 31, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filipnavara Jun 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wfurt left a comment

Choose a reason for hiding this comment

Uh oh!

filipnavara commented Jun 8, 2023

Uh oh!

wfurt commented Jun 8, 2023

Uh oh!

filipnavara commented Jun 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

filipnavara commented Jun 9, 2023

Uh oh!

wfurt left a comment

Choose a reason for hiding this comment

Uh oh!

wfurt commented Jun 16, 2023

Uh oh!

filipnavara commented Jun 16, 2023

Uh oh!

Uh oh!

filipnavara commented Jun 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

filipnavara Jun 8, 2023 •

edited

Loading

filipnavara commented Jun 8, 2023 •

edited

Loading