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

Jump to bottom

Expose kerberos fast negotiation configuration #1466

Merged
merged 2 commits into from May 22, 2020
Merged

Expose kerberos fast negotiation configuration #1466

merged 2 commits into from May 22, 2020

Conversation

rubenvp8510
Copy link
Contributor

@rubenvp8510 rubenvp8510 commented Aug 22, 2019 •
edited

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

d1egoaz
d1egoaz reviewed Aug 22, 2019
View reviewed changes
gssapi_kerberos.go
@@ -33,6 +33,7 @@ type GSSAPIConfig struct {
Username string
Password string
Realm string
DisablePAFXFAST bool
Copy link
Contributor

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

d1egoaz
d1egoaz reviewed Aug 22, 2019
View reviewed changes
kerberos_client.go
@@ -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

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

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

@ghost
Copy link

ghost 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.

@ghost ghost added the stale Issues and pull requests without any recent activity label Feb 21, 2020
@camdencheek
Copy link

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

@ghost ghost removed the stale Issues and pull requests without any recent activity label Mar 18, 2020
@rubenvp8510 rubenvp8510 requested a review from bai as a code owner April 20, 2020 21:48
@ghost ghost added the cla-needed label Apr 20, 2020
@ghost ghost removed the cla-needed label Apr 20, 2020
@rubenvp8510 rubenvp8510 force-pushed the disable-fast-kerberos branch 2 times, most recently from 549c3d7 to 36d31d4 Compare April 20, 2020 22:02
@rubenvp8510
Expose kerberos fast negotiation configuration
01ca5be 
Signed-off-by: Ruben <ruben.vp8510@gmail.com>
@rubenvp8510
Copy link
Contributor Author

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.

@rubenvp8510
Formar NewKerberosClient comments acording to golang standad, added t…
dd2a9df 
…est for DisablePAFXFast

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

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 02:46
@rubenvp8510
Copy link
Contributor Author

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

d1egoaz
d1egoaz approved these changes May 22, 2020
View reviewed changes
Copy link
Contributor

@d1egoaz d1egoaz left a comment •
edited

Choose a reason for hiding this comment

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

LGTM thanks @rubenvp8510

@d1egoaz d1egoaz merged commit fb465ba into IBM:master May 22, 2020
@kaz-zahed kaz-zahed mentioned this pull request Jan 27, 2021
Open
@swythan swythan mentioned this pull request Aug 31, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@d1egoaz d1egoaz d1egoaz approved these changes

@bai bai Awaiting requested review from bai

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@rubenvp8510 @d1egoaz @camdencheek @timjonesdev