Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
base: master
Are you sure you want to change the base?
add Kerberos auth support #702
Changes from 4 commits
1854c5c
f2e846d
b46e98e
1f85924
2fe0bca
d361509
86e0074
eca4758
c47a35b
27f50f1
f792281
ca67c06
6e619be
d4f52ce
530eb45
85c5bb1
04c18be
8059af5
79d6641
b4e96e3
729e190
dc1a816
73ae20f
bf01def
dd22d87
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.