Add hook for custom SSPI context for SqlConnection instances

[twsouthwick]

Background

There have been a couple of active issues around NTLM/kerberos authentication and customizing it for various reasons (see discussions in #305 and #31), with an attempt of enabling this in #1379. This issue is to track an API proposal and the steps needed to integrate it into SqlClient.

NOTE:
I'm not sure of naming in this area and very open to better names for things. I haven't spent much time within the SSPI/GSS/NTLM/Kerberos/etc space and this proposal is based on my limited understanding of these concepts. If I'm missing anything important, please let me know

Proposal

The proposal is to add a provider that has the ability to generate a context given an incoming blob. There are currently a few ways that this is handled internally:

• A native implementation on all Windows platforms
• An internal managed implementation on .NET Standard/.NET 6
• System.Net.Security.NegotiateAuthentication on .NET 7+

As part of this change, those different implementations should be moved to follow the same pattern as an external implementer would have to ensure the abstraction is sufficient. After attempting to do so, it appears the best design would be to have a class that is used per connection (retrieved via a factory method) as there is some state that may need to be accessed between invocations:

+ /// <summary>
+ /// Defines a provider to supply and operate on an SSPI context as part of integrated authentication.
+ /// </summary>
+ public abstract class SspiClientContextProvider
+ {
+     /// <summary>
+     /// Generates a context given a (possibly empty) previously buffer.
+     /// </summary>
+     /// <param name="incomingBlob">The incoming blob (may be empty).</param>
+     /// <param name="outgoingBlobWriter">A writer that allows us to write data for the outgoing blob.</param>
+     protected abstract bool GenerateContext(ReadOnlySpan<byte> incomingBlob, IBufferWriter<byte> outgoingBlobWriter, SspiAuthenticationParameters authParams);
+ }

public class SqlConnection
{
+   /// <summary>
+   /// Gets or sets a factory to handle the SSPI context
+   /// </summary>
+   public Func<SspiClientContextProvider> SspiContextProviderFactory { get; set; }
}

Alternate designs

• We could just return a byte[] - but this would most likely requrie creating new arrays for any authentication

  public abstract class SspiClientContextProvider
  {
  -     protected abstract void GenerateContext(ReadOnlySpan<byte> incomingBlob, IBufferWriter<byte> outgoingBlobWriter);
  +     protected abstract byte[] GenerateContext(ReadOnlySpan<byte> incomingBlob);
  }

• Or we could use an IMemoryOwner to track the lifetime of potentially pooled memory:

  public abstract class SspiClientContextProvider
  {
  -     protected abstract void GenerateContext(ReadOnlySpan<byte> incomingBlob, IBufferWriter<byte> outgoingBlobWriter);
  +     protected abstract IMemoryOwner<byte> GenerateContext(ReadOnlySpan<byte> incomingBlob);
  }

Implementation/Testing

While prototyping this, I found that as-is, it would be a fairly large change. However, I think it should be broken into the following steps to make sure we don't regress anything.

• Consolidate SSPI context generation to single abstraction #2255
• Use memory/span for SSPI generation #2447
• Use an IBufferWriter<byte> to write the outgoing SSPI blob #2452
• Use string for SNI instead of byte[] #2790
• Expose SspiAuthenticationParameters on SspiContextProvider #2454
• Expose SSPI context provider as public #2494

Some follow up items:

• Ensure correct SPN when calling SspiContextProvider #3347
• Handle IDisposable on SspiContextProvider
• serverSPNs in the .NET Core TdsParser needs to be refactored as it shouldn't be just an array - it's a specific structure and should be a struct (see conversation at Ensure correct SPN when calling SspiContextProvider #3347 (comment))

The first four are mostly refactorings that I'm assuming can use current testing to ensure no regressions (please correct me if that is incorrect). For the final two, the main thing to test will be that a custom implementation is picked up and handled correctly, which can be done with unit tests. I also plan on testing the E2E of this API by enabling NTLM with this enabled hook.

[arontsang]

Currently the closest analogy to SspiClientContextProvider in the current code base is NegotiateAuthentication, inside of TdsParserStateObjectManaged.cs:25.

Given that SspiClientContextProvider is expect to be stateful, I would imagine you would want to make it implement IDisposable.

Additionally, should it not be the responsibility of SspiClientContextProvider to report the state of the SSPI connection (e.g. Error or Complete).

[twsouthwick]

Thanks @arontsang for the feedback.

Given that SspiClientContextProvider is expect to be stateful, I would imagine you would want to make it implement IDisposable.

None of the current implementations seem to need it. However, we can always check if it implements IDisposable to then dispose of it.

Additionally, should it not be the responsibility of SspiClientContextProvider to report the state of the SSPI connection (e.g. Error or Complete).

That's reasonable, but I don't see a place in the code base where that is currently surfaced. Can you point me to that?

btw feel free to assign it to me - I'm working on it in #2255

[David-Engel]

@roji For his thoughts.

[roji]

Thanks @David-Engel.

I'm curious - what would be the point of having a full-blown, public abstraction here, as opposed to just implementing the various needed strategies inside SqlClient? Is the intent actually for users to provide their own custom implementations here, and if so, why and in what scenarios would they want to do that? For example, I agree that there's a bit of variance here across .NET TFMs, but e.g. on .NET 7+ shouldn't NegotiateAuthentication simply be used always?

As a data point, Npgsql (the PostgreSQL driver) has similar authentication requirements, and the need for a public abstraction has never been raised by anyone.

On a related note, I'm no expert in the area but why is it important to support Windows native implementations here, given that we're in a cross-platform context and sufficient cross-platform authentication APIs seem to be available?

[David-Engel]

@roji The main use case is to open up the possibility of authentication across untrusted domains, non-domain, and/or cross platform scenarios where it's difficult or impossible to configure authentication "correctly" on the client. Specifically, NTLM username/password authentication is never going to happen in SqlClient itself as I am not going to try to justify the feature to the security team. Similarly, there have been asks for more control over the Kerberos ticket/negotiation in the same types of environments.

The only reason for the native implementation mention is that's how it's currently done in SqlClient on Windows.

[roji]

OK, thanks for the details @David-Engel. I don't really have much to add - I'd be somewhat wary of introducing such a public abstraction, where making something that fits the requirements across the various implementations may be tricky (though I really don't know much about this area). Of course it's always ideal to just expose the right config switches/APIs (e.g. around Kerberos), but if this makes sense for externalizing NTLM, then sure, I guess...

[twsouthwick]

@David-Engel described the exact situation I've been seeing customers in, mostly as a stop-gap during on-prem to Azure migrations.

making something that fits the requirements across the various implementations

I totally agree with the sentiment. That's why I'm including a refactoring as part of this change to move all the current context generation into this abstraction so it at least works for everything internally :) I'm having a customer try out the completed POC in their environment to make sure it works for custom code outside of the sqlclient library.