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

brod supports sasl credentials from file #135

Merged
merged 4 commits into from
Jan 3, 2024

Conversation

rewritten
Copy link
Contributor

Hi folks! This PR adds support for file-based sasl credentials. From brod readme (https://github.com/kafka4beam/brod/blob/master/README.md?plain=1#L425-L428) (emphasis mine):

brod supports SASL PLAIN, SCRAM-SHA-256 and SCRAM-SHA-512 authentication mechanisms out of the box. To use it, add {sasl, {Mechanism, Username, Password}} or {sasl, {Mechanism, File}} to client config. Where Mechanism is plain | scram_sha_256 | scram_sha_512, and File is the path to a text file
which contains two lines, first line for username and second line for password

`brod` supports having credentials in a file (which is good since they do not stay in the producer state and they are not exposed upon inspection).

https://github.com/kafka4beam/brod/blob/master/README.md?plain=1#L425-L428
@rewritten
Copy link
Contributor Author

rewritten commented Jan 3, 2024

I'll squash commits before merging if the PR is accepted, since we are pointing to my branch HEAD in our systems at the moment.


defp validate_option(:sasl, {mechanism, path} = value)
when mechanism in [:plain, :scram_sha_256, :scram_sha_512] and
is_binary(path) do
Copy link
Member

Choose a reason for hiding this comment

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

To they actually allow the path to be a binary or must it be an Erlang string (i.e. Elixir charlist?)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have it working as binary, I'm not sure if it allows a charlist, do you want me to check? I'm following the same typing approach as the existing username/password

Copy link
Member

Choose a reason for hiding this comment

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

If it works as a binary, that's all we need.

@josevalim
Copy link
Member

@rewritten please update the tests too :)

@josevalim josevalim merged commit 3f2ab90 into dashbitco:main Jan 3, 2024
4 checks passed
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

@rewritten rewritten deleted the brod-sasl-file branch January 3, 2024 17:39
@Neustradamus
Copy link

@rewritten: Nice!

Linked to:

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

3 participants