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

Expose a high level NTAuth Negotiate API #29270

Closed
Tratcher opened this issue Apr 15, 2019 · 21 comments · Fixed by #70720
Closed

Expose a high level NTAuth Negotiate API #29270

Tratcher opened this issue Apr 15, 2019 · 21 comments · Fixed by #70720
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Security
Milestone

Comments

@Tratcher
Copy link
Member

Please expose a high level API for managing Negotiate/NTLM/Kerberos authentication handshakes. This is not the same as https://github.com/dotnet/corefx/issues/32291, which wants a low level Kerberos specific API.

There is an internal API today called NTAuthentication that is used by HttpClient, HttpListener, and NegotiateStream to exchange opaque auth blobs, negotiate the authentication protocol, and identify the client to the server. ASP.NET Core also wants to expose this functionality for our cross platform server Kestrel. It could use the same NTAuthentication APIs and handle the HTTP aspect itself.

This is related to https://github.com/dotnet/corefx/issues/8221 for cross platform server support.

A general purpose API that could be used by NegotiateStream, SocketsHttpHandler, and HttpListener would be like this:

// System.Net.Security assembly and namespace
public class NegotiateAuthState : IDisposable
{
  public NegotiateAuthState(bool isServer, string package, NetworkCredential credential, string spn, ContextFlagsPal requestedContextFlags, ChannelBinding channelBinding);
  public bool IsCompleted { get; }
  public string Package { get; }
  public string ClientSpecifiedSpn { get; }
  public IIdentity GetIdentity();
  public string GetOutgoingBlob(string incomingBlob);
  // SmtpOnly:
  public int VerifySignature(byte[] buffer, int offset, int count);
  public int MakeSignature(byte[] buffer, int offset, int count, ref byte[] output);
  // NegotiateStream only:
  public ContextFlagsPal NegotiatedFlags { get; }
  public byte[] GetOutgoingBlob(byte[] incomingBlob, bool thrownOnError);
  public int Encrypt(byte[] buffer, int offset, int count, ref byte[] output, uint sequenceNumber)
  public int Decrypt(byte[] payload, int offset, int count, out int newOffset, uint expectedSeqNumber)
}

Note this does expose ContextFlagsPal. I only see the constructor parameter used once or twice and it could likely be abstracted to a bool or similar.

@Tratcher Tratcher self-assigned this Apr 15, 2019
@Tratcher Tratcher removed their assignment Jun 4, 2019
@davidfowl
Copy link
Member

Those APIs need more Span<byte>

davidsh referenced this issue in davidsh/corefx Jun 9, 2019
The latest changes to the System.Net.Security.Native shim layer fixed a lot of important
bugs for Linux Kerberos usage. But this created a new problem since SqlClient ships
in out-of-band NuGet packages separate from the .NET Core runtime. SqlClient builds
out of the CoreFx repo and uses the common source includes for Kerberos authentication.
This created an unexpected dependency on the System.Net.Security.Native shim layer.
The recent changes to these API signatures caused problems with different combinations
of SqlClient NuGet packages and .NET Core 2.x versus .NET Core 3.0.

After discussion with the SqlClient team, we decided to rework the changes to these
native APIs so that they would remain compatible across all .NET Core versions.

Long-term, the plan is to implement #36896 to expose a Kerberos API in .NET Core which
could be used by SqlClient and other consumers.

Closes #37183
Closes #25205
davidsh referenced this issue in dotnet/corefx Jun 11, 2019
The latest changes to the System.Net.Security.Native shim layer fixed a lot of important
bugs for Linux Kerberos usage. But this created a new problem since SqlClient ships
in out-of-band NuGet packages separate from the .NET Core runtime. SqlClient builds
out of the CoreFx repo and uses the common source includes for Kerberos authentication.
This created an unexpected dependency on the System.Net.Security.Native shim layer.
The recent changes to these API signatures caused problems with different combinations
of SqlClient NuGet packages and .NET Core 2.x versus .NET Core 3.0.

After discussion with the SqlClient team, we decided to rework the changes to these
native APIs so that they would remain compatible across all .NET Core versions.

Long-term, the plan is to implement #36896 to expose a Kerberos API in .NET Core which
could be used by SqlClient and other consumers.

Closes #37183
Closes #25205
@mconnew
Copy link
Member

mconnew commented Sep 10, 2019

I believe this api would also enable more WCF scenarios. We have a scenario enabled by the api SecurityBindingElement.CreateSspiNegotiationOverTransportBindingElement which makes native calls to Sspi. By exposing the NTAuth Negotiate API's, I believe WCF would be able to support this important enterprise scenario.

@davidsh davidsh self-assigned this Nov 1, 2019
davidsh referenced this issue in davidsh/runtime Dec 3, 2019
This is the first of several PRs that add Enterprise Scenarios Testing capability to
the repo. This PR focusses on Linux which allows for docker containers to be used
in an enterprise network configuration.

I focussed on 2 workflows: 1) The 'dev' workflow, and 2) The PR/CI workflow. The dev
workflow works well since it's using containers in a docker-compose environment along
with volume mounting your current dev's repo enlistment. The PR/CI workflow gives us
an Azure DevOps pipeline to automate verification.

I still need to work with the infra team to add a real pipeline that will run. I can't
do that until this is merged. In the meantime, I have my own DevOps pipeline that verified this PR.

See: https://dev.azure.com/systemnetncl/Enterprise%20Testing/_build/results?buildId=141

I will be linking a follow-up GitHub issue describing the roadmap for building on this system
including adding Windows environments, NTLM protocol, proxies, and other libraries such as
System.Net.Mail and System.Data.SqlClient. Those libraries also use Negotiate/Kerberos/NTLM
enterprise-oriented protocols.

Contributes to:
https://github.com/dotnet/corefx/issues/41652
https://github.com/dotnet/corefx/issues/41489
https://github.com/dotnet/corefx/issues/36896
https://github.com/dotnet/corefx/issues/30150
https://github.com/dotnet/corefx/issues/24707
https://github.com/dotnet/corefx/issues/10041
https://github.com/dotnet/corefx/issues/6606
https://github.com/dotnet/corefx/issues/6161
davidsh referenced this issue Dec 5, 2019
This is the first of several PRs that add Enterprise Scenarios Testing capability to
the repo. This PR focusses on Linux which allows for docker containers to be used
in an enterprise network configuration.

I focussed on 2 workflows: 1) The 'dev' workflow, and 2) The PR/CI workflow. The dev
workflow works well since it's using containers in a docker-compose environment along
with volume mounting your current dev's repo enlistment. The PR/CI workflow gives us
an Azure DevOps pipeline to automate verification.

I still need to work with the infra team to add a real pipeline that will run. I can't
do that until this is merged. In the meantime, I have my own DevOps pipeline that verified this PR.

See: https://dev.azure.com/systemnetncl/Enterprise%20Testing/_build/results?buildId=141

I will be linking a follow-up GitHub issue describing the roadmap for building on this system
including adding Windows environments, NTLM protocol, proxies, and other libraries such as
System.Net.Mail and System.Data.SqlClient. Those libraries also use Negotiate/Kerberos/NTLM
enterprise-oriented protocols.

Contributes to:
https://github.com/dotnet/corefx/issues/41652
https://github.com/dotnet/corefx/issues/41489
https://github.com/dotnet/corefx/issues/36896
https://github.com/dotnet/corefx/issues/30150
https://github.com/dotnet/corefx/issues/24707
https://github.com/dotnet/corefx/issues/10041
https://github.com/dotnet/corefx/issues/6606
https://github.com/dotnet/corefx/issues/6161

* Address PR feedback

* Change pipeline *.yml to only run on selected filepaths for PRs
* Change kdc container Dockerfile to be based on ubuntu:18.04
* Fix typo in README.md

* Update .yml file

* Link (instead of copy) apache kerb module to the right place
@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the 5.0 milestone Feb 1, 2020
@karelz karelz modified the milestones: 5.0, Future Jun 5, 2020
@hameedshk
Copy link

@joperezr thank you for linking the issue. Because of lack of kerberos authentication , we are on windows servers which is more costly in terms of operation, hoping to find or implement a solution as soon as possible.

@filipnavara
Copy link
Member

filipnavara commented Feb 18, 2022

This is mirroring the current NTAuthentication class. Due to #62264 I would like to move forward with this a bit.

Issues I see with the proposal:

  • Let's use Span and ReadOnlySpan instead of the offset/count.
  • Error handling and expected exceptions are not defined.
  • VerifySignature should take message and signature, should it not?
  • NTLM also uses sequence numbers in signatures, so perhaps they should be part of the VerifySignature and MakeSignature API since they are explicit in Encrypt/Decrypt?
  • Either we make the caller responsible for the sequence numbers (enables both datagram connection-less and connection-based scenarios) or they should be counted internally (enables only connection-based scenarios)

@wfurt wfurt modified the milestones: Future, 7.0.0 Feb 18, 2022
@wfurt
Copy link
Member

wfurt commented Feb 18, 2022

lets give it try

@filipnavara
Copy link
Member

filipnavara commented Feb 18, 2022

I gave this a closer look, so let's iterate:

public NegotiateAuthState(bool isServer, string package, NetworkCredential credential, string spn, ContextFlagsPal requestedContextFlags, ChannelBinding channelBinding);

There's a common part of the interface for both a client and a server once the mutual authentication is established but also a large part of the surface is different. credential and spn are provided here only for the client-side. The ClientSpecifiedSpn and GetIdentity members are equivalent on the server-side.

Ideally, I would like to see two classes with a common parent. Keeping with the current naming scheme that would be NegotiateServerAuthState : NegotiateAuthState and NegotiateClientAuthState : NegotiateAuthState with some common methods.

Alternative proposal: Keep it as one class but provide different constructors for the server-side and client-side scenario. The downside is that ClientSpecifiedSpn property and GetIdentity are exposed for client-side scenario and need to provide a failure path.

public bool IsCompleted { get; }

The name is not quite descriptive though - what is completed? It's the initial authentication exchange. It essentially means that now we have established a session where VerifySignature, MakeSignature, Encrypt and Decrypt can be called. We should choose a more descriptive name for the property.

Java seems to call the equivalent IsEstablished

Alternative proposal: Maybe there should be a separate class for an established session with the appropriate methods. Then we could expose bool TryGetAuthenticatedSession(out NegotiateAuthSession). In that case my previous proposal about class hierarchy could ditch the common NegotiateAuthState parent.

public string Package { get; }

Looks reasonable. It's not strictly necessary since the caller has to provide it in the constructor though. (Returns underlying mechanism if SPNEGO is used for negotiation.)

public string ClientSpecifiedSpn { get; }
public IIdentity GetIdentity();
public ContextFlagsPal NegotiatedFlags { get; }

I am not familiar with the server-side mechanics here. IIdentity is public and looks alright in this context. As stated above, we either should not expose them for the client-side scenario or specify how they behave in that case.

public string GetOutgoingBlob(string incomingBlob);
public byte[] GetOutgoingBlob(byte[] incomingBlob, bool thrownOnError);

The error handling is grossly underspecified here. Essentially, on the implementation side this calls into GSSAPI which returns an error code. On the managed side this is exposed as SecurityStatusPalErrorCode enum. Most users of this API on the client-side don't care about the error and only differentiate between success and failure. On the server-side, however, the error handling is more complex. The current implementation offers variations of this method which differ in the way how the errors are propagated (exception / status code). This needs to be addressed. Should be throw exception? If yes, which one? Should the current SecurityStatusPalErrorCode be exposed? If not, how do we cover the current usage on the ASP.NET side?

Also, only the byte-array methods may be exposed. The fact that the high level protocols wrap the blobs in Base64 is quite a specific detail. It may warranty a convenience method but at the same time string is immutable and the authentication data may be sensitive. This would possibly encourage usage that doesn't clear the buffers.

public int VerifySignature(byte[] buffer, int offset, int count);
public int MakeSignature(byte[] buffer, int offset, int count, ref byte[] output);

I am not sure where to even start with these... My initial assumption was that they map to the VerifyMIC and GetMIC calls on the GSSAPI side. That's not what they do, they end up calling Wrap/Unwrap on the GSSAPI side, just like Encrypt/Decrypt. That seems downright nuts and a misguided attempt at implementing something. The only current usage is in SMTP code. See the check for byte sequence 01 00 00 00 in there? That's NTLM signature header, the same NTLM signature that can be processed with VerifyMIC and GetMIC. So, how does this even work in the first place you may ask?! Well, NTLM signatures are sealed (if the negotiation is done correctly) and the sealing uses the exact same algorithm that Encrypt/Decrypt (or Wrap/Unwrap) does. So it doesn't verify the signature with the actual data, it merely unseals it and checks if that fails. Well, for NTLM it can fail only if the negotiation didn't result in exchange of the session key. MakeSignature looks to be implemented correctly on Windows (ie. call GetMIC equivalent) but at first sight looks incorrect on Unix. I may be missing something though.

My take is that these methods should wrap the VerifyMIC and GetMIC GSSAPI methods and we should spanify them:

public bool VerifySignature(ReadOnlySpan<byte> message, ReadOnlySpan<byte> signature);
public byte[] MakeSignature(ReadOnlySpan<byte> message);
public int TryMakeSignature(ReadOnlySpan<byte> message, Span<byte> signature);

The SMTP usage would obviously need to be fixed.

On re-reading I realized that this supposed to be an API that calls into Wrap/Unwrap like Encrypt/Decrypt below. The difference seems to be the value of the confidentiality flag passed to the GSSAPI. Should the APIs be merged? Renamed? The parameter shape should match in any case.

public int Encrypt(byte[] buffer, int offset, int count, ref byte[] output, uint sequenceNumber)
public int Decrypt(byte[] payload, int offset, int count, out int newOffset, uint expectedSeqNumber)

Let's spanify those. What is the return value? Do we have any current usage that exposes the sequence numbers as datagram protocol? If not, do we want to drop it? If yes, should we add it to signature APIs and can the underlying SSPI/GSSAPI do that?

@filipnavara
Copy link
Member

filipnavara commented Feb 18, 2022

Looking again at the SMTP code:

                        // If auth completed and still have a challenge then
                        // server may be doing "correct" form of GSSAPI SASL.
                        // Validate incoming and produce outgoing SASL security
                        // layer negotiate message.
It actually handles a case that I called out as unhandled in `SocketsHttpHandler` in another issue. The Negotiate (SPNEGO) authentication does additional exchange of message integrity of the mechanism list (`mechListMIC`). This is not something that should be handled by manually doing the verification/signing. It should be handled as part of the `IsCompleted`/`GetOutgoingBlob` since it's mechanism specific thing. In case of SPNEGO the signed thing is the mechanism list which is an internal part of the first message in the authentication exchange. High-level code should not deal with that at all.

In HTTP this manifests as Authorization: Negotiate <base64 challenge> header on non-401 message following the authentication exchange. Since the HTTP authentication code bails out either on IsCompleted or a non-401 status it never processes it.

After rereading the code, comments and the specification I realized it's actually implementing part of the SASL GSSAPI mechanism that is defined in terms of the GssWrap/GssUnwrap operations with confidentiality flag turned off.

@Tratcher
Copy link
Member Author

Note AspNetCore ended up consuming the existing implementation via reflection. We'd love to be rid of that 😁.
https://github.com/dotnet/aspnetcore/blob/main/src/Security/Authentication/Negotiate/src/Internal/ReflectedNegotiateState.cs

@filipnavara
Copy link
Member

Note AspNetCore ended up consuming the existing implementation via reflection. We'd love to be rid of that 😁.

Thanks for pointing that out. I already linked that code in previous comment and on issue #62264 but I didn't quite emphasise it.

I'm slowly moving forward with some experiments and unit tests but I'd welcome any feedback on the API shape discussed here.

@wfurt
Copy link
Member

wfurt commented Feb 18, 2022

Note that Kestrel is not only code that depends on runtime internals via reflection. https://github.com/dotnet/SqlClient/blob/main/src/Microsoft.Data.SqlClient/netcore/src/Common/src/Interop/Unix/System.Net.Security.Native/Interop.NetSecurityNative.cs and related #62202

I'm not quite sure why we need Encrypt/Decrypt here but it can perhaps help with some other scenarios.

@wfurt
Copy link
Member

wfurt commented Mar 9, 2022

Few more thoughts:
I would personally prefer one Class with multiple constructors if it comes to it or follow the SslStream pattern where you create the object with AuthenticateAsClient and AuthenticateAsServer that can replace the ambiguous GetOutgoingBlob.

The IsCompleted probably follows the Task naming pattern in lack of return code. The GetOutgoingBlob gives you blob but it cannot tell you the state. As far as I can tell, we use this wrong in HttpClient. We will start authentication but we finish as soon as the server gives us back 200. We would never finish processing the last chunk and get the MIC properly verified. This may be probably replaced with 3-state status of success, fail, continue. (or more if we want to incorporate GSSAPI codes)

package, spn and channelBinding should probably be nullable.

I would prefer if this does not throw on authentication errors. I would push that to the caller. We may expose low lever GSSAPI codes but since that is not portable we should probably add status enum with errors we care about. And the runtime PAL should map to it as best as it can. We can still expose the platform value if somebody feels it would be useful.

I would be supportive to do optionally base64. Since this is what goes on on wire, I would not expect it to contain any sensitive data. We may be able to use ArrayPool for intermediates. (or stackalloc if small enough)

I would abandon arrays where possible. Since this does no do any IO, I would expect ReadOnlySpan<byte> for input would be sufficient everywhere. For the output, I would probably standardize on ref byte[] output. That gives caller option to provide buffer and if sufficient than we can use it and if not we would allocate what ever is needed.

On the same note, I would only support Span for *crypt and *Signature.

For now, I think connection-based scenarios would be sufficient. I don't see need for datagrams any time soon but universe can prove me wrong.

we had separate chat with @filipnavara and we agreed that we may proceed with minimal set needed for HTTP while sorting out SMTP/SASL, SqlClient(s) or WCF. This would allow us to (finally!) make some progress and cleanup Kestrel (and runtime) and unblock #62264. If we get enough feedback we can finish rest in 7.0 and if not perhaps shortly(?) after.

@mconnew
Copy link
Member

mconnew commented Mar 10, 2022

CoreWCF is using an implementation based off of the asp.net core version. You can find it here: https://github.com/CoreWCF/CoreWCF/blob/main/src/CoreWCF.Primitives/src/CoreWCF/Security/NegotiateInternal/NegotiateInternalState.cs

I agree with @wfurt about not throwing exceptions. Exceptions are expensive to throw and handle, and a malicious client can cause a server to throw at will by sending a specially crafted authentication packet. Failed authentication is a regular occurrence and not an exceptional event that you can take the cost of an Exception every time it happens.

Ideally this functionality will be in a nuget package which targets netstandard2.0. The reason for this is that we would use it in the WCF client, which only targets netstandard2.0. When we release new features, so far we've been able to make them available to every supported version of .NET Core/.NET. There's no technical reason to require adopting of .NET 7+ to use this.

@filipnavara
Copy link
Member

I refactored the proposal significantly and I'll write one more post later with rationale behind the changes:

public class NegotiateAuthenticationClientOptions
{
    // Specifies the GSSAPI authentication package used for the authentication.
    // Common values are Negotiate, NTLM or Kerberos. Default value is Negotiate.
    public string Package { get; set; }

    // The NetworkCredential that is used to establish the identity of the client.
    // Default value is CrendentialCache.DefaultCredential.
    public NetworkCredential Credential { get; set; }

    // The Service Principal Name (SPN) that uniquely identifies the server to authenticate.
    public string? TargetName { get; set; }

    // The ChannelBinding that is used for extended protection.
    public ChannelBinding? Binding { get; set; }

    // Indicates the requires level of protection of the authentication exchange
    // and any further data exchange.
    // Valid values: None, Sign, EncryptAndSign
    // Default value: None
    System.Net.Security.ProtectionLevel RequiredProtectionLevel { get; set; }
}

public class NegotiateAuthenticationServerOptions
{
    // Specifies the GSSAPI authentication package used for the authentication.
    // Common values are Negotiate, NTLM or Kerberos. Default value is Negotiate.
    public string Package { get; set; }

    // The NetworkCredential that is used to establish the identity of the client.
    // Default value is CrendentialCache.DefaultCredential.
    // Note: I'm not quite sure of the meaning of this on the server side but
    // it exists on GSSAPI side.
    public NetworkCredential Credential { get; set; }

    // The ChannelBinding that is used for extended protection.
    public ChannelBinding? Binding { get; set; }

    // Indicates the requires level of protection of the authentication exchange
    // and any further data exchange.
    // Valid values: None, Sign, EncryptAndSign
    // Default value: None
    System.Net.Security.ProtectionLevel RequiredProtectionLevel { get; set; }
}

public class NegotiateAuthentication : IDisposable
{
    // Create client-side authentication
    public NegotiateAuthentication(NegotiateAuthenticationClientOptions clientOptions);

    // Create server-side authentication
    public NegotiateAuthentication(NegotiateAuthenticationServerOptions serverOptions);

    // Indicates whether the initial authentication finished.
    // NOTE: Original it was named IsCompleted in the proposal but I renamed
    // it to match the property on NegotiateStream.
    public bool IsAuthenticated { get; set; }

    // Indicates negotiated protection level (can be higher than the required one).
    // Returns None if authentication was not finished yet.
    public System.Net.Security.ProtectionLevel ProtectionLevel { get; set; }

    // Indicates whether signing was negotiated.
    // Returns false if authentication was not finished yet.
    public bool IsSigned { get; set; }

    // Indicates whether signing was negotiated.
    // Returns false if authentication was not finished yet.
    public bool IsEncrypted { get; set; }

    // Indicates whether the client and server are mutually authenticated.
    // Returns false if authentication was not finished yet.
    public bool IsMutuallyAuthenticated { get; set; }

    // Indicates whether this is server-side authentication context.
    // Returns value based on the constructor used, provided for parity with
    // NegoatiateStream.
    public bool IsServer { get; set; }

    // The GSSAPI package name that was used for the authentiation. Initially it's
    // the name specified in the options in constructor. After successful authentication
    // it should return the actual mechanism used. For example, if Negotiate is
    // specified as the requested GSSAPI package this may return NTLM or Kerberos once
    // authentication exchange is complete (eg. IsAuthenticated == true).
    //
    // NOTE: This is partly duplicate with RemoteIdentity so it may not be necessary
    // on the public API.
    public string Package { get; }

    // For server context it returns the SPN target name of the client after successful
    // authentication. For client context it returns the target name specified in the
    // constructor options.
    public string? TargetName { get; }

    // Gets information about the identity of the remote party.
    //
    // When accessed by the client, this property returns a GenericIdentity containing
    // the Service Principal Name (SPN) of the server and the authentication protocol used.
    //
    // Throws InvalidOperationException if authentication was not finished yet
    // (IsAuthenticated == false) for server-side.
    public System.Security.Principal.IIdentity RemoteIdentity { get };

    public byte[] GetOutgoingBlob(ReadOnlySpan<byte> incomingBlob, out NegotiateAuthenticationStatusCode statusCode);

    // Base64 version of GetOutgoingBlob
    public string GetOutgoingBlob(string incomingBlob, out NegotiateAuthenticationStatusCode statusCode);

    // TODO (APIs not necessary for HTTP authentication but necessary for high-level protocols
    // like SASL and NegotiateStream):
    // Wrap, Unwrap as replacement for Encrypt, Decrypt, MakeSignature and VerifySignature
    // GetMIC and VerifyMIC if we find a use for that
}

// NOTE: Mirrors SecurityStatusPalErrorCode at the moment but it should mostly map to GSSAPI error
// codes.
public enum NegotiateAuthenticationStatusCode
{
    NotSet = 0,
    OK,
    ContinueNeeded,
    CompleteNeeded,
    CompleteAndContinue,
    ContextExpired,
    CredentialsNeeded,
    Renegotiate,
    TryAgain,

    // Errors
    OutOfMemory,
    InvalidHandle,
    Unsupported,
    TargetUnknown,
    InternalError,
    PackageNotFound,
    NotOwner,
    CannotInstall,
    InvalidToken,
    CannotPack,
    QopNotSupported,
    NoImpersonation,
    LogonDenied,
    UnknownCredentials,
    NoCredentials,
    MessageAltered,
    OutOfSequence,
    NoAuthenticatingAuthority,
    IncompleteMessage,
    IncompleteCredentials,
    BufferNotEnough,
    WrongPrincipal,
    TimeSkew,
    UntrustedRoot,
    IllegalMessage,
    CertUnknown,
    CertExpired,
    DecryptFailure,
    AlgorithmMismatch,
    SecurityQosFailed,
    SmartcardLogonRequired,
    UnsupportedPreauth,
    BadBinding,
    DowngradeDetected,
    ApplicationProtocolMismatch,
    NoRenegotiation
}

@filipnavara
Copy link
Member

Few more thoughts: I would personally prefer one Class with multiple constructors if it comes to it or follow the SslStream pattern where you create the object with AuthenticateAsClient and AuthenticateAsServer that can replace the ambiguous GetOutgoingBlob.

The AuthenticateAsClient/AuthenticateAsServer pattern doesn't work well here because the whole operation is split into multiple calls. Different constructors do work though and it seems reasonable to follow the approach with extensible options classes like the new overrides on SslStream do. This makes is immediately obvious whether a server-side or client-side context is created and it allows easily adding new options.

The IsCompleted probably follows the Task naming pattern in lack of return code.

I renamed the property to IsAuthenticated to match NegotiateStream. I believe that expresses the intent better. Similarly I added various Is* properties present on NegotiateStream and currently also present on the NTAuthentication class in System.Net.Security.

The GetOutgoingBlob gives you blob but it cannot tell you the state. As far as I can tell, we use this wrong in HttpClient. We will start authentication but we finish as soon as the server gives us back 200. We would never finish processing the last chunk and get the MIC properly verified. This may be probably replaced with 3-state status of success, fail, continue. (or more if we want to incorporate GSSAPI codes)

You are right, SocketsHttpHandler doesn't verify the MIC in Negotiate authentication and it should be fixed.

package, spn and channelBinding should probably be nullable.

Yep for spn and channelBinding. For package I'd go with Negotiate default value. I've also renamed spn to TargetName for consistency with NegotiateStream. I don't have particular preference for either.

I would prefer if this does not throw on authentication errors. I would push that to the caller. We may expose low lever GSSAPI codes but since that is not portable we should probably add status enum with errors we care about. And the runtime PAL should map to it as best as it can. We can still expose the platform value if somebody feels it would be useful.

Agreed that the API should not throw. I feel that the GSSAPI status codes would be ideal here. SSPI and GSSAPI basically produce nearly identical set of errors. For now I followed the SecurityStatusPalErrorCode enum.

I would be supportive to do optionally base64. Since this is what goes on on wire, I would not expect it to contain any sensitive data. We may be able to use ArrayPool for intermediates. (or stackalloc if small enough)

No particular opinion on this. I added the base64 string overload.

I would abandon arrays where possible. Since this does no do any IO, I would expect ReadOnlySpan<byte> for input would be sufficient everywhere. For the output, I would probably standardize on ref byte[] output. That gives caller option to provide buffer and if sufficient than we can use it and if not we would allocate what ever is needed.

I'm not sure whether the caller can reasonably predict the buffer sizes for GetOutgoingBlob and whether allowing custom buffer is beneficial.

On the same note, I would only support Span for *crypt and *Signature.

For encrypt/decrypt/sign/verify it would likely be possible to expose API to return the correct size beforehand and thus work only with ReadOnlySpan<byte> and Span<byte>. However, for the moment we leave that out of scope for the initial API proposal.

For now, I think connection-based scenarios would be sufficient. I don't see need for datagrams any time soon but universe can prove me wrong.

👍

I agree. The design above could easily be extended by adding option into NegotiateAuthenticationClientOptions or NegotiateAuthenticationServerOptions.

@filipnavara
Copy link
Member

filipnavara commented Mar 20, 2022

System.Net.Security.ProtectionLevel RequiredProtectionLevel

The original proposal used ContextFlagsPal requestedContextFlags. Unfortunately that would require exposing internal enum and there didn't seem to be a valid reason to do that.

Aside from NegotiateStream the only values used are ContextFlagsPal.Connection and ContextFlagsPal.InitIdentify. We established above that we don't need the initial API to handle datagram scenario so ContextFlagsPal.Connection is implied. ContextFlagsPal.InitIdentify value corresponds to ProtectionLevel.Sign.

For NegotiateStream the public API also uses ProtectionLevel enum. It sets additional options based on TokenImpersonationLevel allowedImpersonationLevel (both client-side and server-side) and ExtendedProtectionPolicy? policy (server-side). These types are already public and we can add the options if necessary. They are not needed for the HTTP scenario though.

public ChannelBinding? Binding { get; set; }

I put this into NegotiateAuthenticationClientOptions and NegotiateAuthenticationServerOptions. However, it seems that if it was passed separately to the constructor then NegotiateAuthentication*Options could be reused between requests. That may be useful for reducing allocations, especially on server side. This is something to think about.

@JamesNK
Copy link
Member

JamesNK commented May 15, 2022

What's the status of this for .NET 7? ASP.NET Core is being annotated for trimming and the current reflection approach isn't trim safe. A public API is needed.

83>C:\Development\Source\aspnetcore\src\Security\Authentication\Negotiate\src\Internal\ReflectedNegotiateState.cs(37,9): Trim analysis warning IL2026: Microsoft.AspNetCore.Authentication.Negotiate.ReflectedNegotiateState..cctor(): Using member 'System.Reflection.Assembly.GetType(String, Boolean)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. Types might be removed.
83>C:\Development\Source\aspnetcore\src\Security\Authentication\Negotiate\src\Internal\ReflectedNegotiateState.cs(38,9): Trim analysis warning IL2075: Microsoft.AspNetCore.Authentication.Negotiate.ReflectedNegotiateState..cctor(): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.NonPublicConstructors' in call to 'System.Type.GetConstructors(BindingFlags)'. The return value of method 'System.Reflection.Assembly.GetType(String, Boolean)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
83>C:\Development\Source\aspnetcore\src\Security\Authentication\Negotiate\src\Internal\ReflectedNegotiateState.cs(39,9): Trim analysis warning IL2075: Microsoft.AspNetCore.Authentication.Negotiate.ReflectedNegotiateState..cctor(): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.NonPublicMethods' in call to 'System.Type.GetMethods(BindingFlags)'. The return value of method 'System.Reflection.Assembly.GetType(String, Boolean)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
83>C:\Development\Source\aspnetcore\src\Security\Authentication\Negotiate\src\Internal\ReflectedNegotiateState.cs(41,9): Trim analysis warning IL2075: Microsoft.AspNetCore.Authentication.Negotiate.ReflectedNegotiateState..cctor(): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.NonPublicMethods' in call to 'System.Type.GetMethods(BindingFlags)'. The return value of method 'System.Reflection.Assembly.GetType(String, Boolean)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
83>C:\Development\Source\aspnetcore\src\Security\Authentication\Negotiate\src\Internal\ReflectedNegotiateState.cs(43,9): Trim analysis warning IL2075: Microsoft.AspNetCore.Authentication.Negotiate.ReflectedNegotiateState..cctor(): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.NonPublicMethods' in call to 'System.Type.GetMethods(BindingFlags)'. The return value of method 'System.Reflection.Assembly.GetType(String, Boolean)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
83>C:\Development\Source\aspnetcore\src\Security\Authentication\Negotiate\src\Internal\ReflectedNegotiateState.cs(45,9): Trim analysis warning IL2075: Microsoft.AspNetCore.Authentication.Negotiate.ReflectedNegotiateState..cctor(): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.NonPublicMethods' in call to 'System.Type.GetMethods(BindingFlags)'. The return value of method 'System.Reflection.Assembly.GetType(String, Boolean)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
83>C:\Development\Source\aspnetcore\src\Security\Authentication\Negotiate\src\Internal\ReflectedNegotiateState.cs(48,9): Trim analysis warning IL2026: Microsoft.AspNetCore.Authentication.Negotiate.ReflectedNegotiateState..cctor(): Using member 'System.Reflection.Assembly.GetType(String, Boolean)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. Types might be removed.
83>C:\Development\Source\aspnetcore\src\Security\Authentication\Negotiate\src\Internal\ReflectedNegotiateState.cs(49,9): Trim analysis warning IL2075: Microsoft.AspNetCore.Authentication.Negotiate.ReflectedNegotiateState..cctor(): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicFields' in call to 'System.Type.GetField(String)'. The return value of method 'System.Reflection.Assembly.GetType(String, Boolean)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
83>C:\Development\Source\aspnetcore\src\Security\Authentication\Negotiate\src\Internal\ReflectedNegotiateState.cs(50,9): Trim analysis warning IL2075: Microsoft.AspNetCore.Authentication.Negotiate.ReflectedNegotiateState..cctor(): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicFields' in call to 'System.Type.GetField(String)'. The return value of method 'System.Reflection.Assembly.GetType(String, Boolean)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
83>C:\Development\Source\aspnetcore\src\Security\Authentication\Negotiate\src\Internal\ReflectedNegotiateState.cs(54,13): Trim analysis warning IL2026: Microsoft.AspNetCore.Authentication.Negotiate.ReflectedNegotiateState..cctor(): Using member 'System.Reflection.Assembly.GetType(String, Boolean)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. Types might be removed.
83>C:\Development\Source\aspnetcore\src\Security\Authentication\Negotiate\src\Internal\ReflectedNegotiateState.cs(55,13): Trim analysis warning IL2075: Microsoft.AspNetCore.Authentication.Negotiate.ReflectedNegotiateState..cctor(): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.NonPublicNestedTypes' in call to 'System.Type.GetNestedType(String, BindingFlags)'. The return value of method 'System.Reflection.Assembly.GetType(String, Boolean)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
83>C:\Development\Source\aspnetcore\src\Security\Authentication\Negotiate\src\Internal\ReflectedNegotiateState.cs(56,13): Trim analysis warning IL2075: Microsoft.AspNetCore.Authentication.Negotiate.ReflectedNegotiateState..cctor(): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.NonPublicNestedTypes' in call to 'System.Type.GetNestedType(String, BindingFlags)'. The return value of method 'System.Type.GetNestedType(String, BindingFlags)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
83>C:\Development\Source\aspnetcore\src\Security\Authentication\Negotiate\src\Internal\ReflectedNegotiateState.cs(57,13): Trim analysis warning IL2080: Microsoft.AspNetCore.Authentication.Negotiate.ReflectedNegotiateState..cctor(): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.NonPublicFields' in call to 'System.Type.GetField(String, BindingFlags)'. The field 'Microsoft.AspNetCore.Authentication.Negotiate.ReflectedNegotiateState._gssExceptionType' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
83>C:\Development\Source\aspnetcore\src\Security\Authentication\Negotiate\src\Internal\ReflectedNegotiateState.cs(60,9): Trim analysis warning IL2026: Microsoft.AspNetCore.Authentication.Negotiate.ReflectedNegotiateState..cctor(): Using member 'System.Reflection.Assembly.GetType(String, Boolean)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. Types might be removed.
83>C:\Development\Source\aspnetcore\src\Security\Authentication\Negotiate\src\Internal\ReflectedNegotiateState.cs(61,9): Trim analysis warning IL2075: Microsoft.AspNetCore.Authentication.Negotiate.ReflectedNegotiateState..cctor(): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.NonPublicMethods' in call to 'System.Type.GetMethods(BindingFlags)'. The return value of method 'System.Reflection.Assembly.GetType(String, Boolean)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
83>C:\Development\Source\aspnetcore\src\Security\Authentication\Negotiate\src\Internal\ReflectedNegotiateState.cs(63,9): Trim analysis warning IL2075: Microsoft.AspNetCore.Authentication.Negotiate.ReflectedNegotiateState..cctor(): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.NonPublicMethods' in call to 'System.Type.GetMethods(BindingFlags)'. The return value of method 'System.Reflection.Assembly.GetType(String, Boolean)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.

There is no way to know whether the internal API is trimmed or not so it can't be safely suppressed. ASP.NET Core negotiate auth will need to be marked as unsafe until there is a new API.

@filipnavara
Copy link
Member

@JamesNK Discussing it with the stakeholders now to see if we can move forward with API review.

@wfurt
Copy link
Member

wfurt commented May 27, 2022

I extracted latest revision to #69920 @filipnavara. edit it as you see fit.

@filipnavara
Copy link
Member

Maybe we can close this issue in favor of #69920 (API approved) and #70909, #70982 (API suggestions)? Together it should cover the full functionality surface that was proposed here.

@wfurt
Copy link
Member

wfurt commented Jun 21, 2022

We can. My plan was to close it when #70720 is merged. (perhaps mention it this there as well so it linked and resolved)

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jun 21, 2022
@filipnavara
Copy link
Member

We can. My plan was to close it when #70720 is merged. (perhaps mention it this there as well so it linked and resolved)

I added the mention so it auto-closes.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 21, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jul 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Security
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants