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.

Sign up for GitHub

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

feat: support SASL SCRAM mechanism #247

Merged
merged 3 commits into from
Aug 22, 2024
Merged

feat: support SASL SCRAM mechanism #247

merged 3 commits into from
Aug 22, 2024

Conversation

WenyXu
Copy link
Contributor

@WenyXu WenyXu commented Aug 10, 2024 •
edited
Loading

#239

support SASL SCRAM-SHA-256 and SCRAM-SHA-512

  • I've read the contributing section of the project CONTRIBUTING.md.
  • Signed CLA (if not already signed).

@WenyXu WenyXu force-pushed the feat/add-sasl-scram branch 2 times, most recently from ca59cde to ef7b953 Compare August 10, 2024 15:39
This was referenced Aug 11, 2024
Merged
Merged
@WenyXu WenyXu force-pushed the feat/add-sasl-scram branch 2 times, most recently from 2665a03 to 096bdca Compare August 11, 2024 11:46
@WenyXu WenyXu force-pushed the feat/add-sasl-scram branch from 096bdca to 79d1a86 Compare August 11, 2024 11:48
@WenyXu WenyXu marked this pull request as ready for review August 11, 2024 11:49
@crepererum
Copy link
Collaborator

I would like to wait for upstream to merge and release this feature. I agree with the general approach of using a dedicated crate instead of implementing all these algorithms within rskafka. 👍

dequbed added a commit to dequbed/rsasl that referenced this pull request Aug 17, 2024
@dequbed
feat: support SCRAM-SHA-512 and SCRAM-SHA-512-PLUS (#48)
a6719cf 
Add support for `SCRAM-SHA-512` and `SCRAM-SHA-512-PLUS`. Tested with
Kafka.

See also: #26, influxdata/rskafka#247
@WenyXu
Copy link
Contributor Author

WenyXu commented Aug 22, 2024

I would like to wait for upstream to merge and release this feature. I agree with the general approach of using a dedicated crate instead of implementing all these algorithms within rskafka. 👍

Now, we can push forward.

crepererum
crepererum requested changes Aug 22, 2024
View reviewed changes
Copy link
Collaborator

@crepererum crepererum left a comment

Choose a reason for hiding this comment

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

Some nitpicks:

  • structured logging (which is esp. helpful if you never yeet through production logs trying to debug why your auth suddenly broke)
  • some general recommendations for the the auth stepping

src/messenger.rs Outdated Show resolved Hide resolved
src/messenger.rs Outdated Show resolved Hide resolved
src/messenger.rs Outdated Show resolved Hide resolved
@crepererum crepererum added the automerge Instruct kodiak to merge the PR label Aug 22, 2024
@crepererum crepererum merged commit 75535b5 into influxdata:main Aug 22, 2024
11 checks passed
@crepererum crepererum mentioned this pull request Jan 7, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crepererum crepererum crepererum approved these changes

Assignees
No one assigned
Labels
automerge Instruct kodiak to merge the PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@WenyXu @crepererum