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

Add hook for custom SSPI context for SqlConnection instances #2253

Open
3 of 6 tasks
twsouthwick opened this issue Dec 1, 2023 · 12 comments · May be fixed by #2494
Open
3 of 6 tasks

Add hook for custom SSPI context for SqlConnection instances #2253

twsouthwick opened this issue Dec 1, 2023 · 12 comments · May be fixed by #2494
Labels
💡 Enhancement Issues that are feature requests for the drivers we maintain. 🆕 Public API Issues/PRs that introduce new APIs to the driver. 🙌 Up-for-Grabs Issues that are ready to be picked up for anyone interested. Please self-assign and remove the label

Comments

@twsouthwick
Copy link
Member

twsouthwick commented Dec 1, 2023

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>
+     /// Gets the authentication parameters for the current connection.
+     /// </summary>
+     protected SqlAuthenticationParameters AuthenticationParameters { get; }

+     /// <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 void GenerateContext(ReadOnlySpan<byte> incomingBlob, IBufferWriter<byte> outgoingBlobWriter);
+ }

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.

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.

@JRahnama JRahnama added the 🆕 Triage Needed For new issues, not triaged yet. label Dec 4, 2023
@kf-gonzalez kf-gonzalez added this to the Future milestone Dec 5, 2023
@kf-gonzalez kf-gonzalez added 💡 Enhancement Issues that are feature requests for the drivers we maintain. 🙌 Up-for-Grabs Issues that are ready to be picked up for anyone interested. Please self-assign and remove the label 🆕 Public API Issues/PRs that introduce new APIs to the driver. and removed 🆕 Triage Needed For new issues, not triaged yet. labels Dec 5, 2023
@arontsang
Copy link

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
Copy link
Member Author

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
Copy link
Contributor

@roji For his thoughts.

@roji
Copy link
Member

roji commented Jan 23, 2024

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
Copy link
Contributor

@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
Copy link
Member

roji commented Jan 23, 2024

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
Copy link
Member Author

@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.

@nhart12
Copy link

nhart12 commented Oct 14, 2024

It seems like part of this has been merged already. Would anyone be able to provide an ETA on this?

@brianlagunas
Copy link

Would love to see a sample for this too. Right now this is my work-around: #31 (comment)

@nhart12
Copy link

nhart12 commented Oct 18, 2024

Would love to see a sample for this too. Right now this is my work-around: #31 (comment)

He has some samples here which I tested in our environment early on with success.
https://github.com/twsouthwick/swick.sqlclient.sspi
My use case is I need to do a manual kerberos negotiation inside of containers and don't want to deploy side-cars etc to support the ticket refresh etc that is typically required #305 (comment)

@brianlagunas
Copy link

@twsouthwick the sample you provided works like a charm for us for our NTLM needs. Is there a timeline of when we can expect the related PRs to be merged so we can start using these extension points officially?

@swigerb
Copy link

swigerb commented Jan 31, 2025

Any update on the this issue to integrate into SqlClient @twsouthwick ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💡 Enhancement Issues that are feature requests for the drivers we maintain. 🆕 Public API Issues/PRs that introduce new APIs to the driver. 🙌 Up-for-Grabs Issues that are ready to be picked up for anyone interested. Please self-assign and remove the label
Projects
Status: Needs Investigation
Development

Successfully merging a pull request may close this issue.

10 participants