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 bcrypt hashes support to basic_auth filter #36278

Open
vixns opened this issue Sep 22, 2024 · 22 comments · May be fixed by #36882
Open

Add bcrypt hashes support to basic_auth filter #36278

vixns opened this issue Sep 22, 2024 · 22 comments · May be fixed by #36882
Labels
enhancement Feature requests. Not bugs or questions. help wanted Needs help!

Comments

@vixns
Copy link

vixns commented Sep 22, 2024

Currently the auth_basic filter only supports SHA hashed passwords.

ExternalSecrets provide a htpasswd templating function from sprig that generates only bcrypt hashes

Supporting brcypt hashes for basic_auth will improve security and simplify the workflow I use -> hashicorp vault>external secret>k8s secret>envoy gateway securitypolicy>envoy basic_auth filter

https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_filters/basic_auth_filter#configuration
https://masterminds.github.io/sprig/crypto.html
https://external-secrets.io/latest/guides/templating/

@vixns vixns added enhancement Feature requests. Not bugs or questions. triage Issue requires triage labels Sep 22, 2024
@jmarantz jmarantz added help wanted Needs help! and removed triage Issue requires triage labels Sep 23, 2024
@Athishpranav2003
Copy link
Contributor

I guess i can work on this feature

@Athishpranav2003
Copy link
Contributor

Athishpranav2003 commented Oct 7, 2024

@vixns So openssl doesnt support bcrypt. How do u suggest to proceed with this issue?
we would either need to ship the implementation along with code here
or use some other library

@jmarantz Could please help me with this

https://httpd.apache.org/docs/2.4/misc/password_encryptions.html
Here we can see the docs refer a C implementation

https://github.com/openbsd/src/blob/master/lib/libc/crypt/bcrypt.c I prefer the OpenBSD implementation due to the interface being more user friendly

@vixns
Copy link
Author

vixns commented Oct 8, 2024

Hi @Athishpranav2003, the OpenBSD implementation seems fine, thanks for working on this

@Athishpranav2003
Copy link
Contributor

So @vixns do we indeed ship the openbsd implementation along with this?

I am not sure how does dependencies work in envoy. I can implement it but not sure how to include the dependency

@vixns
Copy link
Author

vixns commented Oct 8, 2024

@Athishpranav2003
Copy link
Contributor

This also seems to solve the problem, except that I didn't know if it's the standard bcrypt implementation. If it's fine for maintainer's I will use it i guess

@Athishpranav2003
Copy link
Contributor

@vixns so for external dependencies there are many more details that need to be provided and i feel that libbcrypt will not be acceptable by the maintainers. I guess we need someone from maintainer team to step in to approve one of the choices we have here

@Athishpranav2003
Copy link
Contributor

@jmarantz can u help us proceed with the dependency part?
I have the code ready for the PR except that need to accomodate the external dependency

@Athishpranav2003
Copy link
Contributor

@KBaichoo could you please help me with the dependency part? or redirect me to someone who is apt for this query?
I have the PR ready, only thing needed is to include the openBSD dependency

@Athishpranav2003
Copy link
Contributor

@alyssawilk Idk whom to tag for this. No one came forward to help me resolve the dependency part. Could you please help me

@alyssawilk
Copy link
Contributor

Sorry is the problem that you don't know how to set up the external dependency or that you don't know how to get approval for the external dependency?

@Athishpranav2003
Copy link
Contributor

Athishpranav2003 commented Oct 28, 2024

@alyssawilk The problem i am not aware of setting up the external dependency in this case since here the dependency is in OpenBSD repo(not a library)

@alyssawilk
Copy link
Contributor

my intuition is you're going to have to find a way to pull the code that isn't pulling all of OpenBSD. I'll poke around internally to see if relevant folks know of a trusted standalone library but if you can't find one I suspect you're going to have to find a way to make an extension point and do this in your own repo rather than upstream

@Athishpranav2003
Copy link
Contributor

@alyssawilk so the code part for this is relatively stable(doesn't change unless there is a CVE) so I presume if I can properly add the dependency in the losted dependency and directly pull the code part alone should be fine.

But I want a confirmation from you/any maintainer for this.

Plus how do I add the dependency for pulling the code part?

@alyssawilk
Copy link
Contributor

When I want an example PR of adding a dependency to Envoy I usually use the blame view on
https://github.com/envoyproxy/envoy/blame/main/bazel/repository_locations.bzl
to get example PRs of adding new deps. Again I don't think depending directly on OpenBSD files is plausible in a way that aligns with envoy security guidelines so again I'm not sure that's plausible without converting it to a standalone module/library first.

@Athishpranav2003
Copy link
Contributor

@alyssawilk thanks for letting me know your opinion.

I am going ahead with adding this repo as a external dependency https://github.com/hilch/Bcrypt.cpp

I hope this works in our case
Will make the pr tomorrow

@jmarantz
Copy link
Contributor

Just to clarify; is the plan to make this an extension? I would assume wouldn't want to add this dependency in core?

@Athishpranav2003
Copy link
Contributor

@jmarantz this should be an dependency for the basic auth extension
Do you suggest any other method?

@jmarantz
Copy link
Contributor

I don't know the details of how this should work, but I'm guessing that we wouldn't want enabling auth to imply enabling bcrypt; we'd want to use a separate extension point for that feature. I'll defer to Alyssa though.

@Athishpranav2003
Copy link
Contributor

Technically we store hashed passwords during config and use it to verify it on run
The verification will be based on the format of hash(no enabling)

If the hash prefix is SHA then we use SHA, if its $2y$ then its bcrypt.

In this way there is no difference infact(its not affecting existing workflow)

@Athishpranav2003 Athishpranav2003 linked a pull request Oct 29, 2024 that will close this issue
@alyssawilk
Copy link
Contributor

obviously we don't want to change the existing auth defaults. I figure it'd be a configuration option and/or a compile time option depending on how onerous the dependencies ended up being

@Athishpranav2003
Copy link
Contributor

@alyssawilk
I guess this is not changing the defaults but just extends right?
Anyway, please check the PR and we can discuss further on the draft

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests. Not bugs or questions. help wanted Needs help!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants