[API Proposal]: Add support in NegotiateStream.AuthenticateAs for packageName and cancellationToken

Charlieface commented 1 year ago

Background and motivation

NegotiateStream currently attempts Authentication using only the Negotiate protocol i.e. it will only (at least on Windows) select between Kerberos and NTLM, with no way to force only one, or to use any other package.

The underlying SSPI provider does allow it though, and the internal System.Net.NTAuthentication class is set up to do so, so it would be trivial to add such functionality by simply passing through another parameter from AuthenticateAsClient and AuthenticateAsServer.

Forcing Kerberos is something that may be essential for some applications, as NTLM has many known issues, such as Pass-the-Hash, replay attacks, and low hash cracking time. Many applications may want to disallow NTLM altogether.

We may want to also add a static function that can retrieve a list of possible security packages, as well as making the DefaultPackage a public readonly property rather than a private const.

Most of the work for this is done already.

Another parameter that should be added is CancellationToken. There is currently no way to pass one through when authenticating, which means an authentication could block indefinitely. But the async work on NegotiateStream related to issue #36572 has now enabled this functionality on the internal A/SyncReadWriterAdapter classes. So again, this could be trivially solved by passing through an extra parameter.

API Proposal

For brevity I have not added the old APM Begin... functions.

namespace System.Net.Security

public partial class  NegotiateStream : AuthenticatedStream
{
    public string DefaultPackage => NegotiationInfoClass.Negotiate;

    public virtual void AuthenticateAsServer(NetworkCredential credential, ExtendedProtectionPolicy? policy, ProtectionLevel requiredProtectionLevel, TokenImpersonationLevel requiredImpersonationLevel) =>
        AuthenticateAsServer(DefaultPackage, credential, policy, requiredProtectionLevel, requiredImpersonationLevel);

    public virtual void AuthenticateAsServer(string packageName, NetworkCredential credential, ExtendedProtectionPolicy? policy, ProtectionLevel requiredProtectionLevel, TokenImpersonationLevel requiredImpersonationLevel, CancellationToken cancellationToken = default)
    {
        ValidateCreateContext(packageName, credential, string.Empty, policy, requiredProtectionLevel, requiredImpersonationLevel);
        AuthenticateAsync<SyncReadWriteAdapter>(cancellationToken).GetAwaiter().GetResult();
        // Is GetResult() safe, or is there a deadlock risk? What other options are there for sync-over-async?
    }

    public virtual void AuthenticateAsServerAsync(NetworkCredential credential, ExtendedProtectionPolicy? policy, ProtectionLevel requiredProtectionLevel, TokenImpersonationLevel requiredImpersonationLevel) =>
        AuthenticateAsServerAsync(DefaultPackage, credential, policy, requiredProtectionLevel, requiredImpersonationLevel);

    public virtual async Task AuthenticateAsServerAsync(string packageName, NetworkCredential credential, ExtendedProtectionPolicy? policy, ProtectionLevel requiredProtectionLevel, TokenImpersonationLevel requiredImpersonationLevel, CancellationToken cancellationToken = default)
    {
        ValidateCreateContext(packageName, credential, string.Empty, policy, requiredProtectionLevel, requiredImpersonationLevel);
        return AuthenticateAsync<AsyncReadWriteAdapter>(cancellationToken);
    }

    public virtual void AuthenticateAsClient(
        NetworkCredential credential, ChannelBinding? binding, string targetName, ProtectionLevel requiredProtectionLevel, TokenImpersonationLevel allowedImpersonationLevel) =>
        AuthenticateAsClient(DefaultPackage, credential, binding, targetName, requiredProtectionLevel, allowedImpersonationLevel);

    public virtual void AuthenticateAsClient(
        string packageName, NetworkCredential credential, ChannelBinding? binding, string targetName, ProtectionLevel requiredProtectionLevel, TokenImpersonationLevel allowedImpersonationLevel, CancellationToken cancellationToken = default)
    {
        ValidateCreateContext(packageName, isServer: false, credential, targetName, binding, requiredProtectionLevel, allowedImpersonationLevel);
        AuthenticateAsync<SyncReadWriteAdapter>(cancellationToken).GetAwaiter().GetResult();
        // Is GetResult() safe, or is there a deadlock risk? What other options are there for sync-over-async?
    }

    public virtual void AuthenticateAsClientAsync(
        NetworkCredential credential, ChannelBinding? binding, string targetName, ProtectionLevel requiredProtectionLevel, TokenImpersonationLevel allowedImpersonationLevel) =>
        AuthenticateAsClientAsync(DefaultPackage, credential, binding, targetName, requiredProtectionLevel, allowedImpersonationLevel);

    public virtual void AuthenticateAsClientAsync(
        string packageName, NetworkCredential credential, ChannelBinding? binding, string targetName, ProtectionLevel requiredProtectionLevel, TokenImpersonationLevel allowedImpersonationLevel, CancellationToken cancellationToken = default)
    {
        ValidateCreateContext(packageName, isServer: false, credential, targetName, binding, requiredProtectionLevel, allowedImpersonationLevel);
        return AuthenticateAsync<AsyncReadWriteAdapter>(cancellationToken);
    }
}

For listing available packages, most of the work has already been done in Interop.SSPIWrapper. It's primarily a matter of exposing it, as well as the SecurityPackageInfoClass, in a public API. Do we want to just expose the names, or do we want to expose the other data also?

API Usage

Usage should be pretty obvious.

using var ns = new NegotiateStream(someStream);
await ns.AuthenticateAsServerAsync(
    "Kerberos",
    CredentialCache.DefaultNetworkCredentials,
    null,
    ProtectionLevel.EncryptAndSign,
    TokenImpersonationLevel.Identification,
    myCancelToken);

Alternative Designs

The number of overloads has now increased significantly, because both server and client have sync, async Task and async APM functions, and we have added two parameters.

I have narrowed this down by using an optional parameter for cancellationToken, as this covers all cases in the smallest extra code. We could alternately do:

Optional parameter for packageName instead. Needs thinking about which of them is more likely to be used.
Non-optional parameters only.
Overload for packageName, and overload for packageName and cancellationToken, alternately v.v.
Overloads for each possible combination (an extra three overloads), which seems excessive.
Cannot have two optional parameters, otherwise it will clash with existing oevrloads.
Break binary compatibility by refactoring into one big optional parameter list (best not because of breaking changes).

Risks

Do we want to expose such functionality at all? Is a NegotiateStream by definition only able to use Negotiate?

It seems all of the code should be identical otherwise, so there is little risk in adding this. Perhaps we only want to allow Kerberos, NTLM and Negotiate as possible options.

Needs investigating: are there any packages we want to specifically disallow, because the responses they give or the way their state machine runs are not compatible with current code?

dotnet-issue-labeler[bot] commented 1 year ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

ghost commented 1 year ago

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

Issue Details

### Background and motivation NegotiateStream currently attempts Authentication using only the Negotiate protocol i.e. it will only (at least on Windows) select between Kerberos and NTLM, with no way to force only one, or to use any other package. The underlying SSPI provider does allow it though, and the internal [`System.Net.NTAuthentication`](https://github.com/dotnet/runtime/blob/7db1c3333302d4d5ac97a5cfb28e88e5c2cde968/src/libraries/Common/src/System/Net/NTAuthentication.Common.cs#L92) class is set up to do so, so it would be trivial to add such functionality by simply passing through another parameter from `AuthenticateAsClient` and `AuthenticateAsServer`. Forcing Kerberos is something that may be essential for some applications, as NTLM has many known issues, such as Pass-the-Hash, replay attacks, and low hash cracking time. Many applications may want to disallow NTLM altogether. ____ We may want to also add a static function that can retrieve a list of possible security packages, as well as making the `DefaultPackage` a `public readonly` property rather than a `private const`. Most of the work for this is done already. ____ Another parameter that should be added is `CancellationToken`. There is currently no way to pass one through when authenticating, which means an authentication could block indefinitely. But the async work on `NegotiateStream` related to issue #36572 has now enabled this functionality on the internal [`A/SyncReadWriterAdapter`](https://github.com/dotnet/runtime/blob/56c84971041ae1debfa5ff360c547392d29f4cb3/src/libraries/System.Net.Security/src/System/Net/Security/ReadWriteAdapter.cs) classes. So again, this could be trivially solved by passing through an extra parameter. ### API Proposal For brevity I have not added the old APM Begin... functions. ```csharp namespace System.Net.Security public partial class NegotiateStream : AuthenticatedStream { public string DefaultPackage => NegotiationInfoClass.Negotiate; public virtual void AuthenticateAsServer(NetworkCredential credential, ExtendedProtectionPolicy? policy, ProtectionLevel requiredProtectionLevel, TokenImpersonationLevel requiredImpersonationLevel) => AuthenticateAsServer(DefaultPackage, credential, policy, requiredProtectionLevel, requiredImpersonationLevel); public virtual void AuthenticateAsServer(string packageName, NetworkCredential credential, ExtendedProtectionPolicy? policy, ProtectionLevel requiredProtectionLevel, TokenImpersonationLevel requiredImpersonationLevel, CancellationToken cancellationToken = default) { ValidateCreateContext(packageName, credential, string.Empty, policy, requiredProtectionLevel, requiredImpersonationLevel); AuthenticateAsync(cancellationToken).GetAwaiter().GetResult(); // Is GetResult() safe, or is there a deadlock risk? What other options are there for sync-over-async? } public virtual void AuthenticateAsServerAsync(NetworkCredential credential, ExtendedProtectionPolicy? policy, ProtectionLevel requiredProtectionLevel, TokenImpersonationLevel requiredImpersonationLevel) => AuthenticateAsServerAsync(DefaultPackage, credential, policy, requiredProtectionLevel, requiredImpersonationLevel); public virtual async Task AuthenticateAsServerAsync(string packageName, NetworkCredential credential, ExtendedProtectionPolicy? policy, ProtectionLevel requiredProtectionLevel, TokenImpersonationLevel requiredImpersonationLevel, CancellationToken cancellationToken = default) { ValidateCreateContext(packageName, credential, string.Empty, policy, requiredProtectionLevel, requiredImpersonationLevel); return AuthenticateAsync(cancellationToken); } public virtual void AuthenticateAsClient( NetworkCredential credential, ChannelBinding? binding, string targetName, ProtectionLevel requiredProtectionLevel, TokenImpersonationLevel allowedImpersonationLevel) => AuthenticateAsClient(DefaultPackage, credential, binding, targetName, requiredProtectionLevel, allowedImpersonationLevel); public virtual void AuthenticateAsClient( string packageName, NetworkCredential credential, ChannelBinding? binding, string targetName, ProtectionLevel requiredProtectionLevel, TokenImpersonationLevel allowedImpersonationLevel, CancellationToken cancellationToken = default) { ValidateCreateContext(packageName, isServer: false, credential, targetName, binding, requiredProtectionLevel, allowedImpersonationLevel); AuthenticateAsync(cancellationToken).GetAwaiter().GetResult(); // Is GetResult() safe, or is there a deadlock risk? What other options are there for sync-over-async? } public virtual void AuthenticateAsClientAsync( NetworkCredential credential, ChannelBinding? binding, string targetName, ProtectionLevel requiredProtectionLevel, TokenImpersonationLevel allowedImpersonationLevel) => AuthenticateAsClientAsync(DefaultPackage, credential, binding, targetName, requiredProtectionLevel, allowedImpersonationLevel); public virtual void AuthenticateAsClientAsync( string packageName, NetworkCredential credential, ChannelBinding? binding, string targetName, ProtectionLevel requiredProtectionLevel, TokenImpersonationLevel allowedImpersonationLevel, CancellationToken cancellationToken = default) { ValidateCreateContext(packageName, isServer: false, credential, targetName, binding, requiredProtectionLevel, allowedImpersonationLevel); return AuthenticateAsync(cancellationToken); } } ``` ____ For listing available packages, most of the work has already been done in [`Interop.SSPIWrapper`](https://github.com/dotnet/runtime/blob/cdb9f5f179db22745fbb2e878460662c0b21643e/src/libraries/Common/src/Interop/Windows/SspiCli/SSPIWrapper.cs). It's primarily a matter of exposing it, as well as the [`SecurityPackageInfoClass`](https://github.com/dotnet/runtime/blob/dfd618dc648ba9b11dd0f8034f78113d69f223cd/src/libraries/Common/src/Interop/Windows/SspiCli/SecurityPackageInfoClass.cs), in a public API. Do we want to just expose the names, or do we want to expose the other data also? ### API Usage Usage should be pretty obvious. ```csharp using var ns = new NegotiateStream(someStream); await ns.AuthenticateAsServerAsync( "Kerberos", CredentialCache.DefaultNetworkCredentials, null, ProtectionLevel.EncryptAndSign, TokenImpersonationLevel.Identification, myCancelToken); ``` ### Alternative Designs The number of overloads has now increased significantly, because both server and client have sync, async Task and async APM functions, and we have added two parameters. I have narrowed this down by using an optional parameter for `cancellationToken`, as this covers all cases in the smallest extra code. We could alternately do: * Optional parameter for `packageName` instead. Needs thinking about which of them is more likely to be used. * Non-optional parameters only. * Overload for `packageName`, and overload for `packageName` and `cancellationToken`, alternately v.v. * Overloads for each possible combination (an extra three overloads), which seems excessive. * Cannot have two optional parameters, otherwise it will clash with existing oevrloads. * Break binary compatibility by refactoring into one big optional parameter list (best not because of breaking changes). ### Risks Do we want to expose such functionality at all? Is a `NegotiateStream` by definition only able to use Negotiate? It seems all of the code should be identical otherwise, so there is little risk in adding this. Perhaps we only want to allow Kerberos, NTLM and Negotiate as possible options. Needs investigating: are there any packages we want to specifically disallow, because the responses they give or the way their state machine runs are not compatible with current code?

Author:	Charlieface
Assignees:	-
Labels:	`api-suggestion`, `area-System.Net.Security`, `untriaged`
Milestone:	-

wfurt commented 1 year ago

ability to pin Kerberso seems reasonable to me. Any thoughts on this @filipnavara ?

wfurt commented 1 year ago

One the cancellation, I'm not sure if that is really doable. While waiting in GSSAPI call, I'm not sure we have any good way how to interrupt it. So even if we add it it may do nothing as cancellation is best effort.

filipnavara commented 1 year ago

What would you expect on the wire? Plain Kerberos or Kerberos wrapped in Negotiate? IIRC the first one would violate the specification for the wire format, in addition to being incompatible with older clients.

We currently don't have API to specify that you want Negotiate with specific algorithms. It may be a useful API request on its own.

If you want to use Kerberos directly and thus change the wire format then I would argue that using NegotiateStream in new software may not be the best option.

Charlieface commented 1 year ago

True you can't at the moment do an async Kerberos request but the actual negotiation between the parties is just a standard WriteAsync and ReadAsync, fully cancellable. Is it wrong to have a CancellationToken which only works partly? I recall Dns.GetHostEntry had a similar issue for a long time until it was fixed.

Looking at the code, it seems it's using InitializeSecurityContextAsyncW instead of SspiInitializeSecurityContextAsyncW, the latter appears to be asynchronous, although there does not appear to be any cancellation function

Charlieface commented 1 year ago

What would you expect on the wire? Plain Kerberos or Kerberos wrapped in Negotiate? IIRC the first one would violate the specification for the wire format, in addition to being incompatible with older clients.

We currently don't have API to specify that you want Negotiate with specific algorithms. It may be a useful API request on its own.

If you want to use Kerberos directly and thus change the wire format then I would argue that using NegotiateStream in new software may not be the best option.

Does it actually change the wire format? Does it not just end up with Kerberos being negotiated as the only protocol?

My testing (using some rather nasty reflection to change the packageName) seems to work when one side specifies Kerberos and the other Negotiate. I can maybe put up a dotnetfiddle to show it.

filipnavara commented 1 year ago

In my opinion, supporting Negotiate w/ Kerberos (ie. explicitly disabling NTLM) would make sense but we currently lack the underlying API for that. That said, there is already system policy for disabling NTLM, and given how Windows Authentication is tied to deployment it seems that for most scenarios it may be good enough to disable NTLM through global policies.

Does it actually change the wire format? Does it not just end up with Kerberos being negotiated as the only protocol?

Yes, it does. The old NTAuthentication and the new public NegotiateAuthentication can do Negotiate, raw Kerberos, or raw NTLM. It currently doesn't allow Negotiate with specific algorithm.

My testing (using some rather nasty reflection to change the packageName) seems to work when one side specifies Kerberos and the other Negotiate.

It may work due to some compatibility hacks. For example, raw NTLM is often accepted in Negotiate contexts. It's still not ideal and probably violates the on-the-wire specification.

filipnavara commented 1 year ago

One the cancellation, I'm not sure if that is really doable. While waiting in GSSAPI call, I'm not sure we have any good way how to interrupt it.

Correct. The GSSAPI ticket acquisition is not really cancellable and can block for few seconds (depending on system settings).

Looking at the code, it seems it's using InitializeSecurityContextAsyncW instead of SspiInitializeSecurityContextAsyncW, the latter appears to be asynchronous, although there does not appear to be any cancellation function

SSPI (Windows) has async APIs, GSSAPI (Linux, macOS) does not. I never tried to work with them though.

Charlieface commented 1 year ago

The GSSAPI spec is actually quite vague as I recall. All of the packages are GSSAPI-compatible. Whereas SPNEGO is a specific package, which internally negotiates which one to use. So is there a blocking reason why not to make available a raw GSSAPI stream (i.e. where you can specify the package)? Putting it another way: NegotiateStream should have originally been designed to inherit from a generic GssApiStream anyway, as it's an outgrowth of that.

As far as the async APIs are concerned: currently a caller will expect ticket acquisition to happen asynchronously, as part of the whole AuthenticateAs but it does not. So essentially it's a bug. That we cannnot fix it in Linux shouldn't prevent fixing it in Windows to use the async APIs.

SteveSyfuhs commented 1 year ago

I'm supportive of exposing a way to allow callers to choose which package they want to use for authentication, but I would caution you on using this as a mechanism for excluding packages like NTLM by doing something like explicitly forcing Kerberos. The correct way to do that is to inform the negotiate package to behave as normal but exclude specific packages. This is done through the use of parameters on the credential when setting up the context, specifically with structures like SEC_WINNT_AUTH_IDENTITY_EX2: https://learn.microsoft.com/en-us/windows/win32/api/sspi/ns-sspi-sec_winnt_auth_identity_ex2

If you want to continue using negotiate (as one should in 95% of all cases) but want to exclude NTLM, you can pass !NTLM in the PackageList** fields.

I would also recommend having a different class name. If you're specifying a package other than negotiate, by definition you aren't negotiating.

SteveSyfuhs commented 1 year ago

Sorry for the double post, but also the usage of something like SEC_WINNT_AUTH_IDENTITY_EX2 is not dependent on the usage of supplied credentials. You can pass this struct with null username/password/domain fields and it'll treat that as instructions to use the current SSO creds.

Charlieface commented 1 year ago

I admit it's much more appealing to have an exclusion rather than inclusion list, but the amount of engineering is going to be immense. We'd need to change the following classes and interfaces to feed that data down to the PInvoke calls: NTAuthentication, NegotiateStreamPal, SSPIAuthType, ISSPIInterface and SafeFreeCredentials. It may be worth it or it may not, Note that some of those classes have separate Windows, Unix and Managed implementations, complicating it even further.

If this is going to be too much effort then perhaps it would make sense to derive another class KerberosStream, with the only change being the package name. Maybe even split off a base abstract class below NegotiateStream and have both derive from that (I understand that shouldn't be a problem as far as breaking changes are concerned).

As far as the CancellationToken is concerned, are there any other concerns beyond what is mentioned? I hadn't originally considered using the async SSPI functions, I was only considering the handshake between the parties, which is much more likely to be over a poor/unstable connection than a link to a DC, which is normally a short round-trip to a beefy server. But adding support for async SSPI sounds like a good idea also.

Charlieface commented 1 year ago

I'm thinking something like this:

// rename current NegotiateStream class
public abstract class GssStream : AuthenticatedStream
{
    public abstract string DefaultPackage { get; }
    // etc
}

public class NegotiateStream : GssStream
{
    public override string DefaultPackage => NegotiationInfoClass.Negotiate;
}

public class KerberosStream : GssStream
{
    public override string DefaultPackage => NegotiationInfoClass.Kerberos;
}

Charlieface commented 1 year ago

An alternative might be to allow setting the value in the base class, and override it in NegotiateStream

// rename current NegotiateStream class
public class GssStream : AuthenticatedStream
{
    public string DefaultPackage { get; set; } = NegotiationInfoClass.Negotiate;
    // etc
}

public class NegotiateStream : GssStream
{
    public override string DefaultPackage
    {
        get => return NegotiationInfoClass.Negotiate;
        set => throw new InvalidOperationException("Changing package not allowed");
    }
}

rzikm commented 1 year ago

Triage: not critical for 8.0, putting to future for now.

Charlieface commented 1 year ago

Triage: not critical for 8.0, putting to future for now.

The cancellationToken part seems to be pretty trivial, does it not make sense to prioritize just that part?

rzikm commented 1 year ago

The networking team is a bit short-staffed atm. We might get to this in 8.0, but we are not committing to it. This may change depending on the demand for the feature.

wfurt commented 1 year ago

Adding cancellation that only work on Windows seems weird to me. I feel that would send wrong message to developers who consume this x-plat. I feel we should investigate more before making decision and proposal for public API.

Charlieface commented 1 year ago

Adding cancellation that only work on Windows seems weird to me. I feel that would send wrong message to developers who consume this x-plat. I feel we should investigate more before making decision and proposal for public API.

Like I said upthread: cancellation would work as far as cancelling the handshake is concerned, even on Linux. It only doesn't work for acquiring the token, which is less of a concern (because of fast Kerberos responses), and would need quite a bit of extra work even for Windows. Cancelling the handshake is a very easy win here because the token can be passed straight in, all the underlying code is there. Whether that is unexpected I don't know: we've certainly had many async APIs in the past that have blocking sections, and we still do (File.ReadAllBytesAsync for example, which blocks on opening the file).

Any case, a simple hack for cancelling token acquisition until a better option is available: just hand off the acquisition to the threadpool, and do a cancellable wait on it. Ugly but works.

dotnet / runtime