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

Add SASL SCRAM-SHA-512 and SCRAM-SHA-256 mechanismes #1295

Merged
merged 1 commit into from
Mar 5, 2019

Conversation

iyedbennour
Copy link
Contributor

Hi ! I would like to add SCRAM-SHA-512 and SCRAM-SHA-256 SASL mechanismes. We are already using this fork in production.

@ghost ghost added the cla-needed label Feb 27, 2019
@bai bai requested a review from varun06 February 27, 2019 17:00
@bai
Copy link
Contributor

bai commented Feb 27, 2019

Thanks for contributing! Could you please sign CLA to unblock this? 🙏

@iyedbennour
Copy link
Contributor Author

Thanks for contributing! Could you please sign CLA to unblock this?

Hi ! I signed the CLA but I could not get the recheck to run.

@ghost ghost removed the cla-needed label Feb 27, 2019
Copy link
Contributor

@varun06 varun06 left a comment

Choose a reason for hiding this comment

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

you seems to have committed a binary file with this push, is that intentional?

broker.go Outdated Show resolved Hide resolved
broker.go Outdated Show resolved Hide resolved
broker.go Outdated Show resolved Hide resolved
broker.go Outdated Show resolved Hide resolved
broker.go Outdated
if err := c.Begin(b.conf.Net.SASL.User, b.conf.Net.SASL.Password, b.conf.Net.SASL.SCRAMAuthzID); err != nil {
return fmt.Errorf("failed to start SCRAM exchange with the server: %s", err.Error())
}
msg, err := c.Step("")
Copy link
Contributor

Choose a reason for hiding this comment

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

[nitpicky] It helps readability to add a newline after a block even inside a function.

broker.go Outdated Show resolved Hide resolved
examples/sasl_scram_client/main.go Outdated Show resolved Hide resolved
@iyedbennour
Copy link
Contributor Author

I have fixed according to you comments. I have also remove the example which I will be submitting in another PR. Thank you :)

@iyedbennour
Copy link
Contributor Author

you seems to have committed a binary file with this push, is that intentional?

This is a mistake. My bad.

@varun06
Copy link
Contributor

varun06 commented Feb 28, 2019

You might have to write more tests too. Also is there anyone who can look at SCRAM part of PR?

@bai bai requested review from pt2pham and sam-obeid February 28, 2019 13:32
@bai
Copy link
Contributor

bai commented Mar 5, 2019

Thanks for contributing this! And thanks Varun for reviewing 👍

@bai bai merged commit 5894b46 into IBM:master Mar 5, 2019
@iyedbennour
Copy link
Contributor Author

Thank you very much!

@giautm
Copy link

giautm commented Mar 13, 2019

@bai : Hi, any release plan for this feature?

@bai
Copy link
Contributor

bai commented Mar 13, 2019

I'll cut one early next week.

@tsrikanth06
Copy link

@bai when can you cut a release?

@zhipcui
Copy link

zhipcui commented Apr 2, 2019

looking forward to a new release

@iyedbennour
Copy link
Contributor Author

@bai @varun06 Please guys make sure #1349 is merged before cutting the release with SCRAM auth.

@varun06
Copy link
Contributor

varun06 commented Apr 9, 2019

Will do sir.

@dbodtke
Copy link

dbodtke commented Apr 22, 2019

Is this ready to be used?

@iyedbennour
Copy link
Contributor Author

Yes It has been released but you might want to wait for this #1349 to be in a release as it introduces a backward incompatible change.

@yurishkuro
Copy link

@bai question: could you elaborate on the reasoning for using SCRAMClientGeneratorFunc abstraction rather than implementing a SCRAM client directly in this library?

@iyedb
Copy link

iyedb commented Jan 29, 2020

The scram client is stateful and cannot be shared by multiple goroutines/connections. The function creates a new instance for each connection.

@yurishkuro
Copy link

Understood, but is there an actual implementation of the client in this library that can be used? The examples import another 3rd party lib "github.com/xdg/scram".

@iyedb
Copy link

iyedb commented Jan 29, 2020 via email

@liangxj8
Copy link

liangxj8 commented Dec 3, 2021

@bai question: I'm wondering why we don't make the client generator in the example used as the default SCRAMClientGeneratorFunc automatically when the user set the SASL.Mechanism as sarama.SASLTypeSCRAMSHA256 or sarama.SASLTypeSCRAMSHA512 in config.go. When I am using the config, I think the client should be automatically generated according to my configuration but not telling me kafka: invalid configuration (A SCRAMClientGeneratorFunc function must be provided to Net.SASL.SCRAMClientGeneratorFunc). When I'm stepping into the imported source code, it is a little bit hard for me to target the useful GeneratorFunc because golang import without the examples.

@Neustradamus
Copy link

@iyedbennour and others: Thanks a lot for all about SCRAM.

Linked to:

@iyedbennour iyedbennour deleted the sasl_v1 branch January 11, 2022 15:10
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.