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

Expose kerberos fast negotiation configuration #1466

Merged
merged 2 commits into from May 22, 2020

Conversation

rubenvp8510
Copy link
Contributor

@rubenvp8510 rubenvp8510 commented Aug 22, 2019

This PR allows Kerberos to be configured with fast negotiation disable, it is required for certain KDCs (i.e Active directory Kerberos server).

@@ -33,6 +33,7 @@ type GSSAPIConfig struct {
Username string
Password string
Realm string
DisablePAFXFAST bool
Copy link
Contributor

@d1egoaz d1egoaz Aug 22, 2019

Choose a reason for hiding this comment

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

I'm seeing some test for this API here sarama/kerberos_client_test.go, might worth to add a test that includes DisablePAFXFAST

@@ -42,10 +42,10 @@ func createClient(config *GSSAPIConfig, cfg *krb5config.Config) (KerberosClient,
if err != nil {
return nil, err
}
client = krb5client.NewClientWithKeytab(config.Username, config.Realm, kt, cfg)
client = krb5client.NewClientWithKeytab(config.Username, config.Realm, kt, cfg, krb5client.DisablePAFXFAST(config.DisablePAFXFAST))
Copy link
Contributor

@d1egoaz d1egoaz Aug 22, 2019

Choose a reason for hiding this comment

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

and now that you're modifying this file, could you please update the NewKerberosClient func comment to make it go friendly

image

Copy link
Contributor Author

@rubenvp8510 rubenvp8510 Aug 22, 2019

Choose a reason for hiding this comment

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

Sure I'll do! I'm new using golang, so thanks for pointing me to the conventions.

@d1egoaz
Copy link
Contributor

d1egoaz commented Aug 22, 2019

@rubenvp8510 could you please add some description to the PR to tell why this is important to have, thanks

@probot-shopify
Copy link

probot-shopify bot commented Feb 21, 2020

Thank you for your contribution! However, this pull request has not had any activity in the past 90 days and will be closed in 30 days if no updates occur.
If you believe the changes are still valid then please verify your branch has no conflicts with master and rebase if needed. If you are awaiting a (re-)review then please let us know.

@probot-shopify probot-shopify bot added the stale label Feb 21, 2020
@camdencheek
Copy link

camdencheek commented Mar 18, 2020

Hi @d1egoaz -- this is required when using some versions of Active Directory. It appears to be the problem blocking my issue #1519 as well.

@probot-shopify probot-shopify bot removed the stale label Mar 18, 2020
@rubenvp8510 rubenvp8510 requested a review from bai as a code owner Apr 20, 2020
@rubenvp8510 rubenvp8510 force-pushed the disable-fast-kerberos branch 2 times, most recently from 549c3d7 to 36d31d4 Compare Apr 20, 2020
Signed-off-by: Ruben <ruben.vp8510@gmail.com>
@rubenvp8510
Copy link
Contributor Author

rubenvp8510 commented Apr 20, 2020

Sorry, I totally forgot this PR,

I've already updated with the requested changes. I think it is ready for another review and/or merge.

…est for DisablePAFXFast

Signed-off-by: Ruben <ruben.vp8510@gmail.com>
@timjonesdev
Copy link

timjonesdev commented May 6, 2020

I'm getting the following error:

KRBMessage_Handling_Error: KDC did not respond appropriately to FAST negotiation

Googling and following Github issue trails leads me to believe it may be fixed by this pull request.

@rubenvp8510 rubenvp8510 requested a review from d1egoaz May 19, 2020
@rubenvp8510
Copy link
Contributor Author

rubenvp8510 commented May 21, 2020

@d1egoaz could you please do another review of this one? Thanks

Copy link
Contributor

@d1egoaz d1egoaz left a comment

LGTM thanks @rubenvp8510

@d1egoaz d1egoaz merged commit fb465ba into Shopify:master May 22, 2020
10 checks passed
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.

None yet

4 participants