add Kerberos auth #35

chandanjainn · 2022-07-08T12:05:56Z

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

ghost · 2022-07-08T12:06:07Z

All CLA requirements met.

shueybubbles · 2022-07-14T15:10:47Z

Can you separate the krb5 code into non-Windows .go files and stub the implementation in a Windows .go file so Windows apps don't need to pick up this code?

In reply to: 1184565905

shueybubbles · 2022-07-20T16:06:15Z

@dzsquared @chandanjainn @stuartpa the krb code requires go 1.13 and higher.
Should Chandan put the new code in .go files with a 1.13 and higher build directive (and linux only), or should we go ahead and remove 1.12 and prior versions from the appveyor tests?

In reply to: 1190471174

chandanjainn · 2022-07-21T06:30:11Z

Can you separate the krb5 code into non-Windows .go files and stub the implementation in a Windows .go file so Windows apps don't need to pick up this code?

apologies for the late response @shueybubbles working on this. the only issue at my end is lack of a windows system to test out the changes.

In reply to: 1191093407

* add windows stub for krb * add import for msdsn * fix ci lint issue Co-authored-by: chandan jain <Chandan.Jain@ocean.ibm.com>

chandanjainn · 2022-08-12T12:55:28Z

@shueybubbles have added the stub file for windows so that krb is not added as a dependency for windows based systems.
Please review the approach.

In reply to: 1213081795

kerbauth.go

types.go

msdsn/conn_str.go

shueybubbles · 2022-08-14T20:10:59Z

you can also add a go113 directive to the files with !windows directives so pre-go 1.13 won't try to use it.

In reply to: 1214441216

* remove krb dep for windows * remove krb dep * unit test coverage Co-authored-by: chandan jain <Chandan.Jain@ocean.ibm.com>

chandanjainn · 2022-08-16T19:38:26Z

@shueybubbles done with the suggested changes. please review

PeteBassettBet365 · 2022-08-17T19:07:59Z

Hi. There appear to be some logic issues and opportunities for more thorough error checking here.

For instance, I'm attempting to connect using a keytab. I dont supply a KrbCache param in the connection string, which I believe is valid and correct. However the code panics inside auth.go func getKrbParams in the dereference of :

krb["KrbCache"].(string)
in
krbParams["Cache"], err = setupKerbCache(krb["KrbCache"].(string))

Aside from the unchecked type assertion, the function as a whole enforces the connection string contain both a KrbCache and KeytabFile parameter. Their use should be mutually exclusive.

The conn string is user supplied and as such can be mangled entirely, a sensible error message should tell the caller of the issue.

Also I dont believe there is currently a way to connect using specified username and password on the connection string.

e.g. something like
server=localhost;user id=MyUsername;password=MyPassword;database=DBName;krb5conffile=path/to/file;realm=domain.co.uk

We use the methodology in certain scenarios where distribution of key keytab files across a server estate would prohibitive. When initially investigating use of keytabs we found that if a domain account password is somehow locked out or the password becomes changed, even changing the password back to it's original value will not make the currently existing keytabs valid again. New keytabs must the created and distributed to all servers. Some services are sufficiently critical that they must be able to re connect in the face of malicious action or similar, even if the acc in question is set to 1) Password never expires 2) User can not change his own password

chandanjainn · 2022-08-17T21:36:45Z

@PeteBassettBet365 have fixed all unchecked assertion based issues by using structs everywhere and made cache and keytab mutually exclusive with proper error messages. All of this was was there in the earlier commits but got lost while removing the dependencies for windows system in the last couple of commits.

Regarding the ability to connect using username and password, am not sure what you are expecting here? Why would we use password in the connection string along with kerberos config?

if you see the code at https://github.com/microsoft/go-mssqldb/pull/35/files#diff-1f48700829d7e0cdba7f3729d6a7c1c5df3aa11f2aeeed583c7e1e39aa7b66d1R21 , it falls back to username/password based authN given that the user is not sending any krb related config in the conn string and if the user does we'll get relevant errors thrown.

PeteBassettBet365 · 2022-08-24T17:02:38Z

Hi again @chandanjainn thanks for sorting the few issues!

Sorry, I'm not talking about sqlserver auth fallback, this is kerberos auth. The reason I'd like to support supplying the kerberos username and password is because, for certain use cases as I said previously, it is easier/faster than distribution of new keytabs to servers. github.com/jcmturner/gokrb5/v8/client supports this via NewWithPassword

msdsn/conn_str.go

tds.go

codecov-commenter · 2022-08-30T05:20:37Z

Codecov Report

Merging #35 (cde874d) into main (ba5a4a0) will increase coverage by 0.16%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main      #35      +/-   ##
==========================================
+ Coverage   73.08%   73.25%   +0.16%     
==========================================
  Files          25       25              
  Lines        5458     5458              
==========================================
+ Hits         3989     3998       +9     
+ Misses       1230     1224       -6     
+ Partials      239      236       -3

Impacted Files	Coverage Δ
tds.go	`68.08% <0.00%> (+1.09%)`	⬆️

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

README.md

shueybubbles · 2022-09-06T17:09:27Z

integratedauth/krb5/krb5.go

+	AuthProviderFunc integratedauth.Provider                = integratedauth.ProviderFunc(getAuth)
+)
+
+func init() {


If my app includes this package, and my connection string doesn't specify a specific authenticator for integrated auth, will it use this authenticator with some default setting ?

IE, I'd like my connection string to be the same on Windows as Linux.

Should this init also do:
integratedAuth.DefaultProviderName = "krb5"
?

@shueybubbles this comment by Pete should answer your question https://github.com/microsoft/go-mssqldb/blob/main/integratedauth/auth.go#L23

yes, it will fallback to default provider i.e ntlm for unix and winsspi for windows if the authenticator is not provided in the connection string.

refer https://github.com/microsoft/go-mssqldb/blob/main/auth_windows.go#L17 and https://github.com/microsoft/go-mssqldb/blob/main/auth_unix.go#L14

And for your second question, no I dont think so we should do integratedAuth.DefaultProviderName = "krb5" as this is meant to be a fallback.

@PeteBassettBet365 this is correct right?

shueybubbles · 2022-09-06T17:11:22Z

README.md

@@ -59,6 +60,31 @@ Other supported formats are listed below.
 * `Workstation ID` - The workstation name (default is the host name)
 * `ApplicationIntent` - Can be given the value `ReadOnly` to initiate a read-only connection to an Availability Group listener. The `database` must be specified when connecting with `Application Intent` set to `ReadOnly`.

+### Kerberos Active Directory authentication outside Windows
+The package supports authentication via 3 methods.


what's the default approach used if the connection string doesn't specify one explicitly? If a keytab file exists in some default location will it be used without having to specify it in the connection string?

@shueybubbles as of now, no. The user needs to explicitly specify the locations for keytab/cache.

A default location can be added. Should we?

Refer this comment

the MIT doc itself says to put it in /etc by default, or set environment variable KRB5_CONFIG. Couldn't we just rely on those too? I'm a bit surprised the jcmturner/gokrb5 module isn't already doing that

I'd expect someone to be able to set the kerberos environment variables https://web.mit.edu/kerberos/krb5-1.12/doc/admin/env_variables.html and not have to put them all explicitly in the connection string here.

I read https://github.com/jcmturner/gokrb5/blob/master/config/krb5conf.go and have a recommendation for a fallback model for the kr5b.conf file location.

Specified in the connection string

Check KRB5_CONFIG environment variable

Check for /etc/krb5.conf

just use config.NewConfig()

If the user doesn't specify paths to keytab or cache, they probably can rely on the defaults defined in krb5.conf.
If there's no krb5.conf file in a default location or the environment variables aren't set, the authentication will fail but that's by design.

Does this seem reasonable?

@shueybubbles yes, it does. will make the changes.

thx I think trying to follow the MIT spec will help developers. If jcmturner/go-krb5 isn't following the spec for environment variables and defaults, you should open some issues against that module to do so.

shueybubbles · 2022-09-06T17:22:01Z

README.md

@@ -28,6 +28,7 @@ Other supported formats are listed below.
  * `false` - Data sent between client and server is not encrypted beyond the login packet. (Default)
  * `true` - Data sent between client and server is encrypted.
 * `app name` - The application name (default is go-mssqldb)
+* `authenticator` - Can be used to specify use of a registered authentication provider. (e.g. ntlm, winsspi (on windows) or krb5 (on linux))



if I don't set a specific authenticator, can the integratedAuth code in the core try to connect using the default provider first, then fall back to any other registered providers ?

This is the scenario:

My service has a single connection string of a server name with no user name.

The client apps run on both Linux and Windows hosts

My preproduction environment Linux only has NTLM while production has krb5

If my client app imports the krb5 package will the runtime behavior correctly use SSPI on Windows, krb5 in prod, and ntlm in preprod?

If my connection string does have an authenticator tag then I'd expect it to fail to connect if that method isn't available or is mis-configured on the host.
If the connection string doesn't have an authenticator tag, I'd expect to have a predictable behavior based on what is available on the host at runtime and not have to create separate connection strings for every possible environment.

shueybubbles · 2022-09-06T17:46:26Z

integratedauth/krb5/krb5.go

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


is there no default location for these files or a default behavior that can be determined based on looking for krb5.conf in a well-known location ?

By contrast it seems most of these parameters are optional in the JDBC SQL driver.
https://docs.microsoft.com/en-us/sql/connect/jdbc/using-kerberos-integrated-authentication-to-connect-to-sql-server?view=sql-server-ver16

yes, there's a default location for keytab and krbcache at /etc/krb5/ and /tmp/krb5cc_%{uid} respectively.

the reason we didn't look there in the first place was so that the user will be aware of the exact keytab file being used just in case their system has multiple keytabs.

do we want to check this location in case the user hasnt provided one ? can add this support.

shueybubbles · 2022-09-06T17:49:23Z

README.md

+### Kerberos Parameters
+
+* `krb5conffile` - path to kerberos configuration file.
+* `realm` - Domain name for kerberos authentication.


realm` - Domain name for kerberos authentication.

The JDBC driver docs have "realm" as optional

https://docs.microsoft.com/sql/connect/jdbc/setting-the-connection-properties?view=sql-server-ver16

(Version 9.4+) The realm for Kerberos authentication. Set this value to override the Kerberos authentication realm the driver autodetects from the server's realm.

Do you know what it means by "autodetects from the server's realm"? Is that a feature in JDBC we are missing here? Or is it something handled in the krb module itself?

will check up on the JDBC part and revert. I hope, I have answered rest of your questions?

shueybubbles · 2022-09-06T17:55:16Z

I'd like to see my questions about how apps will need to manage integrated auth connection strings, protocol fallback behavior, etc answered in the README.md at least so it's clear what the limitations are about connection string reuse across Windows and Linux for integrated auth.
Maybe a single connection string for all environments isn't achievable, but let's document it appropriately. If there are potentially ways to enable protocol negotiation in the future, please open an Issue to discuss what that would look like.
thx!

LessTravelledWay · 2022-10-10T08:47:53Z

Activity on this has stalled - what do we need to do to get this moving again ?

@shueybubbles - Have all your points been addressed ?

@chandanjainn - @PeteBassettBet365 is available to assist with any blockers.

chandanjainn · 2022-10-10T10:58:24Z

@LessTravelledWay @shueybubbles apologies for the late response here. I have been on a medical leave for some time and will need some more time to recover fully.

Can we get the current PR merged as a V1 and then take up the suggestions as enhancement in future releases?

shueybubbles

PeteBassettBet365 · 2022-10-11T16:26:27Z

Hi. I would add one point, I think the server name needs to be canonicalized with a call to net.LookupCNAME. I'll confirm in the morning. @shueybubbles I'll go back and answer your questions where I can regarding the auth system behaviour.

shueybubbles · 2022-10-11T16:32:52Z

Should we merge this as-is then @PeteBassettBet365 could follow up with any tweaks after testing? I don't have a working kerberos Linux setup to test with.
It shouldn't break anybody to merge it, and I don't plan to add a tag until some others confirm it is suitable for production.

In reply to: 1274962421

PeteBassettBet365 · 2022-10-11T16:39:34Z

Understood. I can give it a test this end.

PeteBassettBet365 · 2022-10-14T15:57:52Z

Reply to 1225995716

@PeteBassettBet365 got it. thanks for the feedback. will incorporate the changes for NewWithPassword in the upcoming commits.

@shueybubbles will add the changes after integration with Pete's changes by today or tomorrow EOD max. Have the changes on a separate branch, testing them out.

@chandanjainn @shueybubbles Hi. There are some bugs to be addressed here.

Raw Credentials dont work as the validateKerbConfig function https://github.com/microsoft/go-mssqldb/blob/main/integratedauth/krb5/krb5.go#L224
which returns an error of "atleast krbcache or keytab is required" when using Raw credentials as per the readme example. That error also needs to be fixed to add a space in "at least".

Example conn string:
authenticator=krb5;server=DatabaseServerName;database=DBName;user id=MyUserName;password=foo;realm=comani.com;krb5conffile=/etc/krb5.conf;

If I bypass that validation issue then the ServerSpn parsing code fails to handle .co.uk domains e.g. "server.domain.co.uk" will result in a domain of "server.domain.co"

Also, the Realm field
https://github.com/microsoft/go-mssqldb/blob/main/integratedauth/krb5/krb5.go#L54
is read from the connection string here
https://github.com/microsoft/go-mssqldb/blob/main/integratedauth/krb5/krb5.go#L203
but is then never referenced again and instead the ServerSPN is parsed to get the realm, which then fails as it doesnt handle .co.uk etc

If I bypass that I get further errors regarding KDC issues. I will check this further.

This is not ready for a tag.

PeteBassettBet365 · 2022-10-17T15:26:18Z

@chandanjainn @shueybubbles Further issues.

DisablePAFXFAST is required in the Password and CredentialCache paths
https://github.com/microsoft/go-mssqldb/pull/35/files#diff-043475ad621581173830d43c2fd895aa7455acd099e156fb045eda996146f6f9R140
and
https://github.com/microsoft/go-mssqldb/pull/35/files#diff-043475ad621581173830d43c2fd895aa7455acd099e156fb045eda996146f6f9R145

The serverSPN needs to be canonicalized before the call to GetServiceTicket https://github.com/microsoft/go-mssqldb/pull/35/files#diff-043475ad621581173830d43c2fd895aa7455acd099e156fb045eda996146f6f9R152

chandan jain and others added 2 commits July 8, 2022 15:13

add kerberos auth

80db7fd

added kerberos auth

85c33fc

shueybubbles requested review from gambtho and stuartpa July 14, 2022 15:08

shueybubbles requested a review from apoorvdeshmukh July 20, 2022 16:07

chandan jain and others added 2 commits August 12, 2022 17:59

Merge branch 'main' into kerberos_auth

4986010

Krb auth (#1)

7ffe6ce

* add windows stub for krb * add import for msdsn * fix ci lint issue Co-authored-by: chandan jain <Chandan.Jain@ocean.ibm.com>

chandanjainn mentioned this pull request Aug 12, 2022

add Kerberos auth support denisenkom/go-mssqldb#702

Open

shueybubbles reviewed Aug 14, 2022

View reviewed changes

kerbauth.go Outdated Show resolved Hide resolved

shueybubbles reviewed Aug 14, 2022

View reviewed changes

types.go Outdated Show resolved Hide resolved

shueybubbles reviewed Aug 14, 2022

View reviewed changes

msdsn/conn_str.go Outdated Show resolved Hide resolved

build tag for go 1.13

bb2bb99

shueybubbles mentioned this pull request Aug 16, 2022

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

Merged

Remove krb dep (#2)

d842b56

* remove krb dep for windows * remove krb dep * unit test coverage Co-authored-by: chandan jain <Chandan.Jain@ocean.ibm.com>

chandanjainn mentioned this pull request Aug 16, 2022

Use go-mssqldb's Kerberos auth gravitational/teleport#10483

Open

chandanjainn requested a review from shueybubbles August 17, 2022 17:45

chandan jain added 2 commits August 18, 2022 02:43

add Kerberos and KerberosConfig structs

ef2e8b2

make krb cache and keyta mutually excusive

d92f2a8

shueybubbles reviewed Aug 24, 2022

View reviewed changes

msdsn/conn_str.go Outdated Show resolved Hide resolved

shueybubbles reviewed Aug 24, 2022

View reviewed changes

tds.go Outdated Show resolved Hide resolved

add krb5 provider

cde874d

chandanjainn requested review from shueybubbles and removed request for apoorvdeshmukh, stuartpa and gambtho August 30, 2022 10:39

add krb conn str examples

73d65f1

shueybubbles requested review from apoorvdeshmukh and stuartpa September 6, 2022 17:02

shueybubbles reviewed Sep 6, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

shueybubbles reviewed Sep 6, 2022

View reviewed changes

chandan jain added 2 commits September 8, 2022 12:11

update krb con str example

770b7da

update krb params info

47f4981

shueybubbles approved these changes Oct 11, 2022

View reviewed changes

shueybubbles merged commit 537893a into microsoft:main Oct 11, 2022

shueybubbles mentioned this pull request Oct 26, 2022

Kerberos on MacOS #62

Closed

sliekens mentioned this pull request Oct 10, 2023

How to use Kerberos ticket cache on Linux? microsoft/go-sqlcmd#468

Open

cwegener mentioned this pull request Dec 15, 2023

refactor: swap mssql driver to 'microsoft' driver open-telemetry/opentelemetry-collector-contrib#29694

Merged

add Kerberos auth #35

add Kerberos auth #35

Conversation

chandanjainn commented Jul 8, 2022 • edited Loading

ghost commented Jul 8, 2022 • edited by ghost Loading

shueybubbles commented Jul 14, 2022 • edited Loading

shueybubbles commented Jul 20, 2022 • edited Loading

chandanjainn commented Jul 21, 2022 • edited by shueybubbles Loading

chandanjainn commented Aug 12, 2022 • edited by shueybubbles Loading

shueybubbles commented Aug 14, 2022 • edited Loading

chandanjainn commented Aug 16, 2022

PeteBassettBet365 commented Aug 17, 2022 • edited Loading

chandanjainn commented Aug 17, 2022 • edited Loading

PeteBassettBet365 commented Aug 24, 2022

codecov-commenter commented Aug 30, 2022 • edited Loading

Codecov Report

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 Sep 8, 2022 • edited Loading

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

Choose a reason for hiding this comment

chandanjainn Sep 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shueybubbles commented Sep 6, 2022

LessTravelledWay commented Oct 10, 2022

chandanjainn commented Oct 10, 2022

shueybubbles left a comment

Choose a reason for hiding this comment

PeteBassettBet365 commented Oct 11, 2022 • edited Loading

shueybubbles commented Oct 11, 2022

PeteBassettBet365 commented Oct 11, 2022

PeteBassettBet365 commented Oct 14, 2022 • edited Loading

PeteBassettBet365 commented Oct 17, 2022 • edited Loading

chandanjainn commented Jul 8, 2022 •

edited

Loading

ghost commented Jul 8, 2022 •

edited by ghost

Loading

shueybubbles commented Jul 14, 2022 •

edited

Loading

shueybubbles commented Jul 20, 2022 •

edited

Loading

chandanjainn commented Jul 21, 2022 •

edited by shueybubbles

Loading

chandanjainn commented Aug 12, 2022 •

edited by shueybubbles

Loading

shueybubbles commented Aug 14, 2022 •

edited

Loading

PeteBassettBet365 commented Aug 17, 2022 •

edited

Loading

chandanjainn commented Aug 17, 2022 •

edited

Loading

codecov-commenter commented Aug 30, 2022 •

edited

Loading

chandanjainn Sep 8, 2022 •

edited

Loading

chandanjainn Sep 8, 2022 •

edited

Loading

PeteBassettBet365 commented Oct 11, 2022 •

edited

Loading

PeteBassettBet365 commented Oct 14, 2022 •

edited

Loading

PeteBassettBet365 commented Oct 17, 2022 •

edited

Loading