add Kerberos auth support #702

chandanjainn · 2021-11-29T15:48:15Z

Have added support for kerberos authentication for go-mssqldb using the GOKRB5 package.

To enable the support for krb the users need to pass the connection string as the following

To use the keytab file

server=server;user id=user;port=1345;realm=domain;trustservercertificate=true;krb5conffile=/etc/krb5.conf;keytabfile=/path/to/keytabFile.keytab

To use kerberos cache file

"server=server;port=1345;realm=domain;trustservercertificate=true;krb5conffile=/etc/krb5.conf;krbcache=/tmp/krb5cc_1000"

krb5conffile        : path to the krb5 configuration file, defaults to "/etc/krb5.conf"
krbcache            : path to the kerberos cache file, required if initkrbwithkeytab = false
keytabfile          : path to the keytab file, required if initkrbwithkeytab = true
realm               : domain name, required when using kerberos cache

kardianos · 2021-11-29T17:21:31Z

Thank you for doing this.

Is there a reason not to use the latest version "v8" with a module import address of "github.com/jcmturner/gokrb5/v8" ?

I have not done a full review yet, and a few things jump out to me, for which I'll send a review for in a bit.

-Daniel

kardianos

There are currently many items that prevent this from being merged in. I need to look into the design a little closer as well. We might need to talk a bit about how the Connection structure should be represented; is it possible to have a single unified *Kerberos field in the connection structure that contains the data used to connect? Then on connection string parse, that information would be pulled from either the krbCache or keytabfile which I think would simplify some of the logic in the actual connection.

As mentioned in a prior comment, I would like to know why "github.com/jcmturner/gokrb5/v8" isn't used, and if it just relates to when you started using it, we should change it to use v8 before being merged.

I do appreciate that you have submitted this.

kerbauth.go

msdsn/conn_str.go

tds.go

chandanjainn · 2021-11-30T15:20:28Z

Thank you for doing this.

Is there a reason not to use the latest version "v8" with a module import address of "github.com/jcmturner/gokrb5/v8" ?

I have not done a full review yet, and a few things jump out to me, for which I'll send a review for in a bit.

-Daniel

@kardianos Working on migrating this code to v8. Will push the changes along with other code review comments.

kerbauth.go

kerbauth_test.go

msdsn/conn_str.go

types.go

go.mod

kardianos

Just focused on some error handling in this review.

kerbauth.go

msdsn/conn_str.go

chandanjainn · 2022-01-04T06:04:58Z

@kardianos could you please review this PR again? have made the changes as per the review comments

kardianos

This change is in much better shape then before. I think it is close.

Also, how does this operate on windows? Does windows support kerberose? Where is a CCache or keytab file?

tds.go

msdsn/conn_str_test.go

msdsn/conn_str.go

kerbauth.go

msdsn/conn_str_test.go

README.md

kardianos

A few final changes.

msdsn/conn_str_test.go

tds.go

msdsn/conn_str.go

bmanosh-sf · 2022-03-09T20:06:16Z

Please look at all line item changes, then also investigate why the CI is failing.

As we have gokrb v8 as a dependency which in turn uses go modules for dependency management ,it requires go version 1.13 and above, that's why CI jobs which are using go version less than 1.13 are failing. @kardianos

@kardianos, any chance we could get this merged in? It sounds like requirement for v1.13 is the only blocker and according to the Go Developer survey, it's likely that 84%< of teams are already using 1.13 or above.

Or maybe @denisenkom could help us get this merged?

vipulwad · 2022-03-14T07:10:06Z

rb v8 as a dependency which in turn us

@denisenkom - This is a good feature which we are also looking for. Can this be merged asap?

chandanjainn · 2022-03-14T13:49:02Z

@kardianos any updates please?

@shueybubbles could you please review this PR, as this has been pending for about 2 months now?

shueybubbles · 2022-03-14T14:11:45Z

@chandanjainn I can look but I don't have merge privileges.
FYI I am considering forking this driver over to the Microsoft organization to have a version with some active maintainers and a corporate budget for maintenance. I think it'd be good to the SQL Server team at MSFT to have a Go driver version it backs. Would you and other members of the community welcome such a version? Is there a better forum to present this idea to the community?

shueybubbles · 2022-03-14T14:17:55Z

msdsn/conn_str.go

+		}
+	}
+	if c["krbcache"] == "" && c["keytabfile"] == "" {
+		missingParam = "atleast krbcache or keytab is required"


shueybubbles · 2022-03-14T14:23:22Z

msdsn/conn_str.go

@@ -37,6 +43,21 @@ const (
 	LogRetries     Log = 128
 )

+type Kerberos struct {


What is the value of coupling msdsn to gokrb5 types? Can the config store some standard types and have kerbauth do the conversion to gokrb5 at auth time?

and will Windows apps using SSPI still pull in these krb packages in their compilation? Might it be useful to have the krb code be non-Windows-only?

shueybubbles · 2022-03-14T14:29:47Z

kerbauth_test.go

+	}
+
+	_, err := krbObj.InitialBytes()
+	if err == nil {


If the test is expecting a nil err please change the t.ErrorF messages to indicate that. Otherwise the code reads like err == nil should be changed to err != nil

shueybubbles · 2022-03-14T14:31:15Z

kerbauth_test.go

+	}
+}
+
+func TestFree(t *testing.T) {


a test should have at least one assertion

shueybubbles · 2022-03-14T14:42:13Z

kerbauth.go

@@ -0,0 +1,123 @@
+package mssql


I agree 1.13 is probably a reasonable minimum requirement at this point.
That said - it may be worthwhile to decouple specific auth implementations like this one from msdsn in general and instead have each auth implementation inject itself into a discovery mechanism such as with some visitor pattern on the connection properties. I'm new enough to Go not to know the best way to have such dependency injection.
With such dependency injection you could mark the kerbauth files for 1.13+ only.

How could this PR align with #718 ?

@shueybubbles with 718, anyone using go-mssql driver for kerberos will have to implement their own custom logic and/or maintain their own repos to handle the entire kerberos authN process.
Moreover my PR provides with flexibility to use either krb cache or keytab file which I believe is missing from 718?

My other comment re: coupling the config to the specific krb implementation is my main concern. The other PR at least avoids such coupling. I think there should be a way for the backend of the driver to manage krb more transparently than having the client use krb-specific packages to even define the connection.

shueybubbles · 2022-03-14T14:45:40Z

tds.go

-	auth, authOk := getAuth(p.User, p.Password, p.ServerSPN, p.Workstation)
+	var auth auth
+	var authOk bool
+	if p.Kerberos != nil && p.Kerberos.Config != nil {


This is where I think having a generic way to discover an auth implementation from the config properties could be useful to make it simpler to add new ones in the future. The msdsn could have simple types and this code could call registered delegates from each auth provider until it finds one that handles it.

keith6014 · 2022-03-14T21:28:23Z

@chandanjainn I can look but I don't have merge privileges.
FYI I am considering forking this driver over to the Microsoft organization to have a version with some active maintainers and a corporate budget for maintenance. I think it'd be good to the SQL Server team at MSFT to have a Go driver version it backs. Would you and other members of the community welcome such a version? Is there a better forum to present this idea to the community?

You should absolutly do this. I have been waiting few years for this feature. golang-reddit is a good place to have such ideas.

bmanosh-sf · 2022-03-14T23:47:53Z

@chandanjainn I can look but I don't have merge privileges. FYI I am considering forking this driver over to the Microsoft organization to have a version with some active maintainers and a corporate budget for maintenance. I think it'd be good to the SQL Server team at MSFT to have a Go driver version it backs. Would you and other members of the community welcome such a version? Is there a better forum to present this idea to the community?

Yes please.

bmanosh-sf · 2022-03-17T18:16:47Z

@chandanjainn I can look but I don't have merge privileges. FYI I am considering forking this driver over to the Microsoft organization to have a version with some active maintainers and a corporate budget for maintenance. I think it'd be good to the SQL Server team at MSFT to have a Go driver version it backs. Would you and other members of the community welcome such a version? Is there a better forum to present this idea to the community?

Yes please.

It kinda seems like this repo has become stale / unsupported... @chandanjainn any further thoughts or timeline on forking the repo? I know multiple teams & companies who rely on this so the lack of support is blocking them at the moment. It would be great to get some enterprise-level backing for it.

FYI @kardianos @denisenkom

chandanjainn · 2022-03-19T05:20:08Z

@chandanjainn I can look but I don't have merge privileges. FYI I am considering forking this driver over to the Microsoft organization to have a version with some active maintainers and a corporate budget for maintenance. I think it'd be good to the SQL Server team at MSFT to have a Go driver version it backs. Would you and other members of the community welcome such a version? Is there a better forum to present this idea to the community?

Yes please.

It kinda seems like this repo has become stale / unsupported... @chandanjainn any further thoughts or timeline on forking the repo? I know multiple teams & companies who rely on this so the lack of support is blocking them at the moment. It would be great to get some enterprise-level backing for it.

FYI @kardianos @denisenkom

@bmanosh-salesforce i believe your comment was directed at @shueybubbles

bmanosh-sf · 2022-03-21T14:58:13Z

@chandanjainn I can look but I don't have merge privileges. FYI I am considering forking this driver over to the Microsoft organization to have a version with some active maintainers and a corporate budget for maintenance. I think it'd be good to the SQL Server team at MSFT to have a Go driver version it backs. Would you and other members of the community welcome such a version? Is there a better forum to present this idea to the community?

Yes please.

It kinda seems like this repo has become stale / unsupported... @chandanjainn any further thoughts or timeline on forking the repo? I know multiple teams & companies who rely on this so the lack of support is blocking them at the moment. It would be great to get some enterprise-level backing for it.
FYI @kardianos @denisenkom

@bmanosh-salesforce i believe your comment was directed at @shueybubbles

Oops, yes. Thank you :)

shueybubbles · 2022-03-21T15:20:46Z

@bmanosh-salesforce I am making the pitch to create this fork this week. There aren't many precedents for such an "adoption" at Microsoft so I don't have a good time estimate. I think if our open source folks agree it's a good idea I'll need to put together some standard stuff like a code of conduct and contribution guidelines. We'll also need to create a set of Github actions or ADO pipelines to replace the appveyor-based test suite for PR validation. We can readily replace some of the SQL VMs with Linux containers for SQL 2017+ but testing against Windows instances of SQL 2012+ will need a fair amount of investment.
I'd prefer not supporting Go versions older than 1.13, to reduce the test matrix considerably.
We can certainly open up the fork for PRs without having a full test suite in place with the assumption that early adopters are going to rigorously test their scenarios anyway.

keith6014 · 2022-03-22T01:09:20Z

@bmanosh-salesforce I am making the pitch to create this fork this week. There aren't many precedents for such an "adoption" at Microsoft so I don't have a good time estimate. I think if our open source folks agree it's a good idea I'll need to put together some standard stuff like a code of conduct and contribution guidelines. We'll also need to create a set of Github actions or ADO pipelines to replace the appveyor-based test suite for PR validation. We can readily replace some of the SQL VMs with Linux containers for SQL 2017+ but testing against Windows instances of SQL 2012+ will need a fair amount of investment. I'd prefer not supporting Go versions older than 1.13, to reduce the test matrix considerably. We can certainly open up the fork for PRs without having a full test suite in place with the assumption that early adopters are going to rigorously test their scenarios anyway.

Hi.
I am curious. How does Microsoft decide what languages to write drivers for? I can understand for C#, C++,and visual basic but for a language like Go? What is the correct way to get them engaged?

shueybubbles · 2022-03-22T12:41:31Z

@keith6014 if by "them" you mean "Microsoft", it's my team that manages most in-house driver and client application development for SQL Server. We have seen steady growth in Go usage with SQL both inside and outside Microsoft, so we started participating in changes for this repo last year. Go looks like a great open source foundation for creating more command line tools for cross platform CI/CD infrastructure so we plan some investment in that area too, starting with github.com/microsoft/go-sqlcmd, which is built using go-mssqldb

keith6014 · 2022-03-24T17:43:01Z

@keith6014 if by "them" you mean "Microsoft", it's my team that manages most in-house driver and client application development for SQL Server. We have seen steady growth in Go usage with SQL both inside and outside Microsoft, so we started participating in changes for this repo last year. Go looks like a great open source foundation for creating more command line tools for cross platform CI/CD infrastructure so we plan some investment in that area too, starting with github.com/microsoft/go-sqlcmd, which is built using go-mssqldb

Understood. I hope you get the funding and attention it deserves. 🤞

bmanosh-sf · 2022-03-24T21:51:00Z

bump @kardianos @denisenkom could you please merge this?

shueybubbles · 2022-03-28T14:27:58Z

I opened #729 to solicit input on a Microsoft fork. We very much appreciate your insights!

bmanosh-sf · 2022-03-30T23:37:39Z

@dimdin, could you help us get this merged?

shueybubbles · 2022-04-29T21:31:15Z

the microsoft fork is open now, you can clone the issue and the PR there.
I'd appreciate some more community input on the structure of this PR to merge it there.
github.com/microsoft/go-mssqldb

dimdin · 2022-04-29T22:40:42Z

A modular approach can be applied to load authentication providers (e.g. kerberos)

Each authentication provider should be able to extend the configuration parameters parsing and read the parsed parameter values when getAuth is called.
An authentication provider module import statement is also required to register the module.

Using this approach allows authentication providers to be implemented without modifying go-mssqldb and reduces dependencies imported by these providers when not in use.

@kardianos @denisenkom Do you have any concerns about the modular loading of authentication providers?

bmanosh-sf · 2022-06-10T19:32:14Z

the microsoft fork is open now, you can clone the issue and the PR there. I'd appreciate some more community input on the structure of this PR to merge it there. github.com/microsoft/go-mssqldb

@shueybubbles @chandanjainn would we want to move this PR work over to the Microsoft fork?

chandanjainn · 2022-08-12T12:57:10Z

@bmanosh-sf @keith6014 have moved this PR to the MS repo here microsoft#35
Requesting your feedbacks and help with verifying the changes.

chandanjainn · 2022-08-16T20:10:01Z

Moved to microsoft#35

add krb auth support

1854c5c

kardianos requested changes Nov 29, 2021

View reviewed changes

chandanjainn added 2 commits November 30, 2021 19:39

worked on review comments

f2e846d

Rename kerberos variables.

b46e98e

Removed leading space

1f85924

chandanjainn requested a review from kardianos December 3, 2021 14:31

kardianos requested changes Dec 4, 2021

View reviewed changes

chandanjainn added 2 commits December 6, 2021 12:54

unexport initiator states

2fe0bca

removed the global vars in kerbauth.go

d361509

chandanjainn requested a review from kardianos December 7, 2021 11:13

chandanjainn added 2 commits December 8, 2021 17:25

migrated gokrb from v7 to v8.

86e0074

worked on error message

eca4758

kardianos requested changes Dec 9, 2021

View reviewed changes

worked on error messages.

c47a35b

chandanjainn requested a review from kardianos December 13, 2021 04:43

updated readme.

27f50f1

kardianos requested changes Jan 11, 2022

View reviewed changes

chandanjainn and others added 4 commits January 19, 2022 21:50

Merge branch 'master' into kerberos_auth

f792281

Wroked on review comments

ca67c06

fixed the changes for readme.

6e619be

fix: whitespace

d4f52ce

manawasp mentioned this pull request Jan 20, 2022

DSN parameter in connection string #532

Closed

chandanjainn added 2 commits January 20, 2022 16:55

worked on review comments

530eb45

fix for unit testing

85c5bb1

chandanjainn requested a review from kardianos January 20, 2022 12:05

kardianos requested changes Jan 20, 2022

View reviewed changes

msdsn/conn_str_test.go Outdated Show resolved Hide resolved

msdsn/conn_str_test.go Outdated Show resolved Hide resolved

msdsn/conn_str_test.go Outdated Show resolved Hide resolved

tds.go Outdated Show resolved Hide resolved

msdsn/conn_str.go Outdated Show resolved Hide resolved

Merge branch 'master' into kerberos_auth

04c18be

shueybubbles reviewed Mar 14, 2022

View reviewed changes

kerbauth_test.go

}

}

func TestFree(t *testing.T) {

Copy link

Contributor

shueybubbles Mar 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a test should have at least one assertion

shueybubbles reviewed Mar 14, 2022

View reviewed changes

shueybubbles approved these changes Mar 14, 2022

View reviewed changes

shueybubbles mentioned this pull request May 11, 2022

Export auth interface to allow external implementations e.g. krb5 - V1-Candidate microsoft/go-mssqldb#15

Merged

Merge branch 'master' into kerberos_auth

dd22d87

add Kerberos auth support #702

Are you sure you want to change the base?

add Kerberos auth support #702

Conversation

chandanjainn commented Nov 29, 2021 • edited

kardianos commented Nov 29, 2021

kardianos left a comment

Choose a reason for hiding this comment

chandanjainn commented Nov 30, 2021

kardianos left a comment

Choose a reason for hiding this comment

chandanjainn commented Jan 4, 2022

kardianos left a comment

Choose a reason for hiding this comment

kardianos left a comment

Choose a reason for hiding this comment

bmanosh-sf commented Mar 9, 2022

vipulwad commented Mar 14, 2022

chandanjainn commented Mar 14, 2022

shueybubbles commented Mar 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chandanjainn Mar 25, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keith6014 commented Mar 14, 2022

bmanosh-sf commented Mar 14, 2022

bmanosh-sf commented Mar 17, 2022

chandanjainn commented Mar 19, 2022

bmanosh-sf commented Mar 21, 2022

shueybubbles commented Mar 21, 2022

keith6014 commented Mar 22, 2022

shueybubbles commented Mar 22, 2022

keith6014 commented Mar 24, 2022

bmanosh-sf commented Mar 24, 2022

shueybubbles commented Mar 28, 2022

bmanosh-sf commented Mar 30, 2022

shueybubbles commented Apr 29, 2022

dimdin commented Apr 29, 2022

bmanosh-sf commented Jun 10, 2022

chandanjainn commented Aug 12, 2022

chandanjainn commented Aug 16, 2022

chandanjainn commented Nov 29, 2021 •

edited

chandanjainn Mar 25, 2022 •

edited