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

basic auth: add option to exclude bcrypt at build time #258

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jan--f
Copy link

@jan--f jan--f commented Oct 2, 2024

This add the option to exclude bcrypt during the build process by passing -tags nobcrypt. Currently this disables user authentication via basic_auth. If authorized users are configured but the server is build with nobcrypt the endpoint will respond with Unauthorized, even if the correct credentials are send.
The goal is to be able to exclude bcrypt for compliance reasons. In the future we could add basic_auth using other hash functions.

This add the option to exclude bcrypt during the build process by
passing `-tags nobcrypt`. Currently this disables user authentication
via basic_auth. If authorized users are configured but the server is
build with `nobcrypt` the endpoint will respond with Unauthorized, even
if the correct credentials are send.
The goal is to be able to exclude bcrypt for compliance reasons. In the
future we could add basic_auth using other hash functions.

Signed-off-by: Jan Fajerski <[email protected]>

func Validate(users map[string]config_util.Secret) error {
if len(users) > 0 {
slog.Info("basic auth via bcrypt hashes not implemented")
Copy link
Author

Choose a reason for hiding this comment

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

This cloud also return an error so a server without bcrypt support can not start with a config that includes authorized users.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that would be better, if we know at startup time that a configuration cannot work, failing explicitly lets the operator figure out what they want to do about it.

@matthiasr
Copy link
Contributor

Just as a note, this will have merge conflicts with #151 which also refactors how authentication happens. Can we get that one in before finishing up here?

Copy link
Contributor

@matthiasr matthiasr left a comment

Choose a reason for hiding this comment

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

Now I am curious what compliance regime forbids bcrypt 😄 and at least this is not as difficult as FIPS mode would be …

@jan--f
Copy link
Author

jan--f commented Oct 25, 2024

Just as a note, this will have merge conflicts with #151 which also refactors how authentication happens. Can we get that one in before finishing up here?

Sure no problem, I'm not in a rush. This might not be the best way anyway.

@jan--f
Copy link
Author

jan--f commented Oct 25, 2024

Now I am curious what compliance regime forbids bcrypt 😄 and at least this is not as difficult as FIPS mode would be …

This does actually target FIPS. Afaiu bcrypt is at least not in the list of approved crypto algos. But most certain depending on x/crypto is a problem. 🤷

@matthiasr
Copy link
Contributor

Hmm, yeah, TLS won't be an option then either. I guess in our case FIPS-mode would entail no encryption at all 😅 I think #151 will actually help in this because it adds the notion of chained authenticators, which adds a well contained point to control what is and isn't active, and then we can remove the relevant authenticators from the build altogether.

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.

2 participants