denisenkom / go-mssqldb

Microsoft SQL server driver written in go language
BSD 3-Clause "New" or "Revised" License
1.82k stars 495 forks source link

add Kerberos auth support #702

Open chandanjainn opened 2 years ago

chandanjainn commented 2 years ago

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 commented 2 years ago

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

chandanjainn commented 2 years ago

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.

chandanjainn commented 2 years ago

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

chandanjainn commented 2 years ago

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

vasily-kirichenko commented 2 years ago

Oh, God, this is brilliant! No more Scala/Python just because Kerberos was not supported by this lib. Thank you, guys!

vasily-kirichenko commented 2 years ago

I've tested this branch on a real AD environment, everything works as expected.

chandanjainn commented 2 years ago

@kardianos any updates please?

bmanosh-sf commented 2 years ago

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.

keith6014 commented 2 years ago

I've tested this branch on a real AD environment, everything works as expected.

also tested. looks great

bmanosh-sf commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@kardianos any updates please?

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

shueybubbles commented 2 years ago

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

keith6014 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

bump @kardianos @denisenkom could you please merge this?

shueybubbles commented 2 years ago

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

bmanosh-sf commented 2 years ago

@dimdin, could you help us get this merged?

shueybubbles commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@bmanosh-sf @keith6014 have moved this PR to the MS repo here https://github.com/microsoft/go-mssqldb/pull/35 Requesting your feedbacks and help with verifying the changes.

chandanjainn commented 2 years ago

Moved to https://github.com/microsoft/go-mssqldb/pull/35