krb5 authentication provider #65
Conversation
Codecov Report
@@ Coverage Diff @@
## main #65 +/- ##
==========================================
+ Coverage 73.25% 73.30% +0.05%
==========================================
Files 25 25
Lines 5458 5458
==========================================
+ Hits 3998 4001 +3
+ Misses 1224 1223 -1
+ Partials 236 234 -2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| * `keytabfile` - path to Keytab file. | ||
| * `krbcache` - path to Credential cache. | ||
| * For further information on usage: | ||
| * `krb5-configfile` (mandatory) - path to kerberos configuration file. |
There was a problem hiding this comment.
krb5-configfile(mandatory) - path to kerberos configuration file.
please file a followup issue to make these parameters optional. Per the Kerberos specs, there are default locations for these files and those paths can also be overridden by environment variables.
If the krb5 package this driver uses doesn't follow that spec, the driver itself should by looking in the default locations and checking the environment variables.
Also - consider some way to establish a chain of integrated authenticators so the connection string on Windows can be the same connection string on Linux and it will use the most appropriate authenticator .
It seems like it could be reasonably simple to do, since a Windows app would only have the SSPI authenticator registered and a Linux app could have both NTLM and Krb5. The app could set the protocol ordering in the latter case.
There was a problem hiding this comment.
Hi. Just a quick note to say, yes I'll look into it.
There was a problem hiding this comment.
looks good to me.
@shueybubbles @PeteBassettBet365 just a thought, do we really need to rename all the connection parameters?
@PeteBassettBet365 thanks for picking this up again. have been away for medical reasons.
|
@shueybubbles - What is stopping this being merged ? Has been sitting for review for a while now, would be good to get some feedback on this. |
@LessTravelledWay I'd love to get one or two more people with krb5 experience (I am not one) to sign off on this PR . To my eye Pete's change looks like a pretty extensive rewrite of Chandan's change. Are the developers who plan to consume this change ok with it? |
| * For further information on usage: | ||
| * `krb5-configfile` (mandatory) - path to kerberos configuration file. | ||
| * `krb5-realm` (required with keytab and raw credentials) - Domain name for kerberos authentication. | ||
| * `krb5-keytabfile` - path to Keytab file. |
There was a problem hiding this comment.
is it worthwhile to support both the new and old values to avoid the breaking change? This code has been around long enough now that some folks could be using Chandan's strings.
There was a problem hiding this comment.
I added a CHANGELOG.md, please document the fix and the breaking changes there. We're getting some flak from the community now on being better about such documentation per #76
There was a problem hiding this comment.
@PeteBassettBet365 @chandanjainn @LessTravelledWay I would merge this once it includes the CHANGELOG update if the breaking changes are acceptable.
|
thx for the update, is the changelog update the last commit? if so I will merge |
|
Hi yes, that's complete. Thanks |
|
Fantastic, thanks David. |
Implementation of krb5 authentication in linux and windows using the github.com/jcmturner/gokrb5/v8/ package.
Fixes the issues in the previous submitted implementation.