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 Kerberos auth support #702

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

chandanjainn
Copy link

@chandanjainn chandanjainn commented Nov 29, 2021

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

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

  1. 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
Copy link
Collaborator

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

Copy link
Collaborator

@kardianos kardianos left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
kerbauth.go Outdated Show resolved Hide resolved
kerbauth.go Outdated Show resolved Hide resolved
kerbauth.go Outdated Show resolved Hide resolved
kerbauth.go Outdated Show resolved Hide resolved
msdsn/conn_str.go Outdated Show resolved Hide resolved
msdsn/conn_str.go Outdated Show resolved Hide resolved
msdsn/conn_str.go Outdated Show resolved Hide resolved
msdsn/conn_str.go Outdated Show resolved Hide resolved
tds.go Outdated Show resolved Hide resolved
@chandanjainn
Copy link
Author

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 Outdated Show resolved Hide resolved
kerbauth.go Outdated Show resolved Hide resolved
kerbauth_test.go Outdated Show resolved Hide resolved
msdsn/conn_str.go Outdated Show resolved Hide resolved
types.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
Copy link
Collaborator

@kardianos kardianos left a comment

Choose a reason for hiding this comment

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

Just focused on some error handling in this review.

kerbauth.go Outdated Show resolved Hide resolved
msdsn/conn_str.go Outdated Show resolved Hide resolved
msdsn/conn_str.go Outdated Show resolved Hide resolved
msdsn/conn_str.go Outdated Show resolved Hide resolved
msdsn/conn_str.go Outdated Show resolved Hide resolved
msdsn/conn_str.go Outdated Show resolved Hide resolved
@chandanjainn
Copy link
Author

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

Copy link
Collaborator

@kardianos kardianos left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
msdsn/conn_str_test.go Outdated Show resolved Hide resolved
msdsn/conn_str.go Outdated Show resolved Hide resolved
kerbauth.go Outdated Show resolved Hide resolved
msdsn/conn_str_test.go Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@kardianos kardianos left a comment

Choose a reason for hiding this comment

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

A few final 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
@bmanosh-sf
Copy link

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

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

@kardianos any updates please?

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

@shueybubbles
Copy link
Contributor

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

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

Choose a reason for hiding this comment

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

at least

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

type Kerberos struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

}

_, err := krbObj.InitialBytes()
if err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}
}

func TestFree(t *testing.T) {
Copy link
Contributor

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

@@ -0,0 +1,123 @@
package mssql
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

How could this PR align with #718 ?

Copy link
Author

@chandanjainn chandanjainn Mar 25, 2022

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

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

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

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

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

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

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

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

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

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

bump @kardianos @denisenkom could you please merge this?

@shueybubbles
Copy link
Contributor

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

@bmanosh-sf
Copy link

@dimdin, could you help us get this merged?

@shueybubbles
Copy link
Contributor

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

dimdin commented Apr 29, 2022

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

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

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

@chandanjainn
Copy link
Author

Moved to microsoft#35

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants