Export auth interface to allow external implementations e.g. krb5 - V1-Candidate

[PeteBassettBet365]

The original driver auth interface and implementations have been moved into their own packages and exported. There are two implementations within the driver, NTLM and Windows SSPI.
As before the default on windows is the winsspi package and NTLM on Linux.

The consuming application can now override this at runtime by calling mssql.SetIntegratedAuthenticationProvider(authProvider) e.g.

// create a new auth.Provider around the kerberos client
provider := krb5.NewAuthProvider(krbClient)

// pass the provider to mssql to override the default authentication mechanism
mssql.SetIntegratedAuthenticationProvider(provider)
We have created an integratedauth.Provider implementation here https://github.com/bet365/go-mssqldb-auth-krb5 which uses http://github.com/jcmturner/gokrb5 to authenticate with active directory.

This keep the main driver free from the krb5 dependency and opens the possibility for other implementations in future.

If this pull request is accepted we will update our implementation to the main repo master. Currently it is using a replacement to point at a branch.

[shueybubbles]

I like this modular approach. By contrast there was competing krb5 implementation at denisenkom#702
Would it make sense to try to combine these a bit and put a krb5-based auth package in this repo for one-stop shopping?
It'd be great to get the folks from that other PR in a conversation to get the community aligned here.

[PeteBassettBet365]

Hi David. Sorry for the lateness of this reply, I've been on annual leave.

The 702 PR is quite different from our approach here but I think the base question is "Do you want to take a dependency on github.com/jcmturner/gokrb5 in go-mssqldb itself?"

As you say, ours is reasonably modular, the changes were only just enough to allow external packages to supply the required functionality at runtime, windows was already handled, we added linux. We had in mind a second repo which held the actual reference implementation which users could depend on if they required kerberos on linux. Our example implementation is here https://github.com/bet365/go-mssqldb-auth-krb5 but we dont expect it being the defacto implementation or repo. MS could have it if you like. We could extend it to show other common patterns quite easily, flesh it out.

Have you got a set of requirements in mind for this? Or any comments about the PR as it stands?

[dergyitheron]

Hi everyone, I've been occasionally watching deniskom/#702 and now it seems like this fork is on good way to finally make the krb5-based auth possible.

From someone who simply uses the go-mssqldb module for many use cases, I like the modular approach as well but I think having working krb5 auth in this repo would help many much more, the mentioned PR has quite a lot of people supporting it.

In this particular need to have krb5-based auth, adding only auth interface to allow external implementations could lead to having different implementations in different packages or developers would have their own implementations that could later get outdated or loose maintenance workforce.

I would support both the option to have external auth implementations and also having built-in krb5-based auth that would cover most of the use cases.

[shueybubbles]

thx for the input.
This comment from 702 seems relevant:
denisenkom#702 (comment)

We are trying to move the azure identity dependency out of the core module altogether with #30, which makes the azuread package a separate module. That use case is a little different, as the app as to create an AAD-specific connector.

For krb, it seems like it's not a big change for consumers of the driver to import two modules - the core mssql and a krb5 implementation. We'd be happy to host a krb5-based module in this repo, and it could be a version of the code from denisenkom#702

With that in mind - what would it take to modify this PR to allow a module to automatically register itself as the krb auth implementation when an app imports it? Is it just a matter of the driver adding an init function that calls mssql.SetIntegratedAuthenticationProvider with an instance of itself?

[chandanjainn]

@shueybubbles I echo what @dergyitheron said. Having krb5 auth logic in the same repo would help and prevent dependencies on external implementations getting outdated and losing relevance ?

Should we move ahead and raise a PR cloning changes from denisenkom#702 ?

As a side note, would it be possible to take the changes for the modular approach as an enhancement later on if required at all? We have multiple teams depending on this feature but the PR has been in a limbo for more than 6 months now :)

[shueybubbles]

@chandanjainn @PeteBassettBet365 @stuartpa I think for what we are calling V1 of this driver, taking Chandan's change instead of this one is probably fine. There seems to be more people wanting a standard krb5 implementation in the driver that we can tweak instead of enabling multiple implementations floating around.

There are changes coming for hardened SQL Server 2022 support (and eventually Azure SQL DB etc) that will involve refactoring of a fair amount of the authentication stack to support it. At that time we can assess whether the modularity proposed here would help.

My main request for these krb5 implementations is they only compile for non-Windows builds, possibly adding a stub go file for Windows if needed.

Any objections to taking Chandan's change instead of this one?

[PeteBassettBet365]

Hi everyone.

We're still working on the more modular approach here that was previously mentioned.

Essentially it gives a default kerberos auth provider, on linux, but allows overriding. We are going to give the auth providers access to the parsed connection string so other implementations could make use of them.

This would still keep krb5 out of the base driver, and would be an additional package used only on linux via an additional import.

One question I would have is if the 702 PR is restricted to just keytabs, or can raw credentials be used?

[chandanjainn]

@shueybubbles we can definitely look into the suggestion i.e to compile the krb logic for non windows builds.

@PeteBassettBet365 yes, the PR 702 does indeed support the raw credentials as well.

[PeteBassettBet365]

Hi.

I've pushed a new set of changes for this that add, I believe, everything we wanted.

We keep the krb5 packages separate from the main driver, expose a modular registration approach for authentication, change the interface into authentication to expose the whole connection string

The reference kerberos authentication implementation can be found here, as before.
https://github.com/bet365/go-mssqldb-auth-krb5

It now supports three modes of auth. KeyTabs, Cached Credential Files and Direct Username & Password.

All the values are specified on the connection string, including authentication provider.

I very much look forward to hearing any feedback you might have.

I have seen the issue building one of the azure tests for 1.18 and will resolve that in the morning.

[shueybubbles]

@chandanjainn @stuartpa @PeteBassettBet365 Chandan is trying to implement kerb5 in this repo at #35
I think Chandan's change could be adapted here by converting his additions to an integratedAuth provider. The question is - whose change should go in first? The second person would need to adapt their code to the first.
Chandan is already isolating most of his code in linux-specific files so moving that code around a bit shouldn't be too tricky.

[PeteBassettBet365]

With the modular approach, you can pick any kerberos implementation you like, but I do like not having to drag in all the other packages when they wont be needed. I'll have a look at his in the morning.

[codecov-commenter]

Codecov Report

Merging #15 (7fd8e3b) into main (2d408c3) will increase coverage by 0.66%.
The diff coverage is 57.14%.

@@            Coverage Diff             @@
##             main      #15      +/-   ##
==========================================
+ Coverage   72.44%   73.11%   +0.66%     
==========================================
  Files          25       25              
  Lines        5596     5456     -140     
==========================================
- Hits         4054     3989      -65     
+ Misses       1302     1229      -73     
+ Partials      240      238       -2     

Impacted Files	Coverage Δ
tds.go	67.35% <37.50%> (-0.50%)	⬇️
mssql.go	86.24% <75.00%> (ø)
auth_windows.go	100.00% <100.00%> (ø)
token.go	64.29% <0.00%> (-0.59%)	⬇️
integratedauth/winsspi/winsspi.go

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[shueybubbles]

pls address the lint issues.

[PeteBassettBet365]

Hi. I've commit for the linting issues. Is there a problem with the pr-validation job? It seems to fail intermittently. I committed a whitespace change earlier and it flew through, while the previous push failed. It does it on other prs too.

[shwetamoharil]

@shueybubbles any update please? Have you reached a decision which PR would go in first and if our PR - #35 requires any more changes?

[PeteBassettBet365]

I expect we would have to go first. Then a chosen kerberos implementation could slot into that framework.

[shueybubbles]

Some of the tests are timing dependent and can fail intermittently, unfortunately.

[shueybubbles]

[shueybubbles]

@stuartpa I'd appreciate your review, and then I'd merge this one first.
thx all. This experience has been very illuminating for me to issues faced in the field.

In the end I think we'll be setting a pretty good example of driver modularity, in contrast with certain other Microsoft-provided drivers for other languages that pull in a gazillion dependencies.

[PeteBassettBet365]

hi, I should reiterate that we are fine with MS having the reference kerberos provider, which is currently in our repo. Fork it, or similar, and keep it going forward. The import paths for the driver and provider would then be nice and clean.

We'd be fine to continue contributing in either case.

I've reread the comments above. Do you want the reference krb5 implementation to be part of this repo, alongside the ntlm and winsspi packages? If so I have a commit ready with that. Two additional files in the integratedauth folder. Obviously that brings github.com/jcmturner/gokrb5/v8 into the driver via go.mod.

[PeteBassettBet365]

Hi. I've dropped a new commit here with brings our reference kerberos implementation (https://github.com/bet365/go-mssqldb-auth-krb5) into the driver as the integratedauth/krb5 package.

It supports three modes of auth. KeyTabs, Cached Credential Files and Direct Username & Password. All the values are specified on the connection string, including authentication provider. I've updated the read me to detail this.

This does mean that the driver now has a dependency on github.com/jcmturner/gokrb5/v8, meaning it will be fetched regardless of the users need. We can revert this back very simply if that is what you would like. Then it would be a two repo solution again. I feel that this auth provider would be better as it's own repo, so the user only pays for it if they need to use it. We are fine with that being in your org, here on github if you so wish.

Please let me know how you want to play this.

[chandanjainn]

@PeteBassettBet365 @shueybubbles we are good with the modular approach suggested by Pete and will adapt to it, but it's a humble request can we not disregard our PR #35 here ? A lot of effort and time went into the PR since we raised it with the other repo and would like to iterate it came in way before this ?

[shueybubbles]

i think the dependency only has an effect on apps that import the package, and you haven't added the import by default to auth_unix.go.

---

In reply to: 1219704721

[shueybubbles]

I appreciate the community enthusiasm behind getting a working kerberos implementation in the driver and ultimately I think everyone will get benefit if can reframe this discussion. Rather than considering #35 as competition with this one, can we look at having 2 PRs as a chance to collaborate on a shared solution both groups will be happy with? I am not an expert on Linux kerberos so I'm not qualified to say whether one implementation proposed is more suitable for inclusion in the core driver than the other. Can this PR evolve as a merger of the two by incorporating components of #35 ? What can I do to facilitate it? Would creating a new branch for interim PRs to merge help?

---

In reply to: 1219720436

[chandanjainn]

Reply to #15 (comment)

@shueybubbles yes, definitely. As I previously mentioned we are good with the modular approach suggested by Pete for modularity, all we ask for is to keep the kerberos implementation from #35 and we are happy to adapt to changes according to Pete's PR (once it's merged in) and with any more suggestions the community has for us.

[shueybubbles]

does anyone have docker scripts for setting up a test system with a SQL server image and a Linux container with the kerb client installed to test the kerb auth implementation? I'd like to automate it.

[PeteBassettBet365]

ok, I have reverted that last commit and we are back to having just the authentication provider system in this PR.

A small point but can I ask for everyone's opinions on the Provider interface?

type Provider interface {
	// GetIntegratedAuthenticator is responsible for returning an instance of the required IntegratedAuthenticator interface
	GetIntegratedAuthenticator(config msdsn.Config) (IntegratedAuthenticator, error)
}

In the previous commit I changed it to take a pointer to msdsn.Config as the config is growing. It wont make a massive difference as these functions dont get called very often, but it would make some difference. The other side of that coin is that the providers shouldn't really be able to make changes to the config and as such passing by value is right. Just a thought. @chandanjainn you and I need to agree on this :-)

If no further changes are suggested I think we can move ahead with this PR.

On the second point of an actual krb5 implementation, I have no problem working with @chandanjainn toward that. It was not my intention to marginalise your effort. We all want to see the driver evolve in the best way possible which is why we got involved after having to implement the kerberos auth ourselves internally. We've been running a version of this auth for 18 months and in production for over a year. It now sits behind a very substantial, and growing, percentage of our business. I'm sure we can ensure the final version meets all our needs.

Thirdly @shueybubbles In reference to #15 (comment) I believe any package referenced in go-mssqldb go.mod will be downloaded onto all developers machines, including all their deps etc. You are right that not importing the krb5 packages on windows will cause them to not form part of the output binary, but the source code will still be on their disk. That's why I was suggesting a second repo for these. The krb5 packages would not be part of the go-mssqldb go.mod file at all and it would then be the developers choice to add both the repos for go-mssqldb driver and the krb5 packages to the go.mod file of their client application. If dev disk space is not a concern (it'll likely only be a few mb) then the krb5 auth package can be part of this repo.

[shueybubbles]

thx all
I think passing the config by value provides the right interface and clear expectations that the config is immutable. I suppose an alternative would be to define a config interface so you could pass the pointer but protect the data by only using the interface.

As for disk space consumption from unused dependencies, I think that ship sailed when I added the azuread support. Also, a fair number of folks like having a default implementation in the same repo as the driver where it's easy to find.

If everyone agrees on the pass by value config we can approve and merge this, then Chandan can adapt his PR to it.

---

In reply to: 1220750763

[chandanjainn]

Reply to 1220768497 and 1220750763

@PeteBassettBet365 @shueybubbles I agree with passing by value for the config. Even I had switched pass by value from pass by reference in my recent commits considering this.

@shueybubbles are we looking for a script just for Krb client or the KDC server too?

[shueybubbles]

Preferably a script for both, since kerb auth doesn't need a full Windows domain (afaik). Given such a script I can integrate it into the Github action to run driver tests, and I'll reuse it for go-sqlcmd testing.

---

In reply to: 1221897805