-
Notifications
You must be signed in to change notification settings - Fork 160
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
support mTLS for authentication #360
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR- this looks good, with a few nitpicks.
Could you please also add some more documentation in the README.md file:
- An example in the config file section, after the other TLS options.
- Mention how to use this with bazel (which flags are required, etc).
- Regenerate the --help text.
Thanks for the review! I'll take a bit later today to rework. |
Sorry that took a bit, was preoccupied. @mostynb I've opted for "mTLS (client certificate)" esque wording as I've actually never heard it called "client certificates" only "mTLS". I also believe that it can be helpful to understand that it's a mutual authentication, which I think makes the error message for specifying a TLS CA File but not cert/key more understandable. "if i'm just authenticating client certificates, why do I also need a key/cert?" is a question I imagine coming up. Happy to change again, but let me know what you think, thanks! |
Looks good- thanks for contributing. I also landed some related adjustments to README.md. It seems that mTLS/HTTPS doesn't work, so I skipped over that in the configuring bazel section of the docs for now. |
mTLS/HTTPS doesn't work, I'm confused what did you try? mTLS is https just with extra authentication. |
In one terminal, I ran bazel-remote with Then in another terminal, I tried building bazel remote using bazel 3.2.0, with the following flags: Then I changed the URL to
|
Interesting! Based on my testing above with cURL it should just work, this honestly seems like a problem with bazel not using the client certificate for HTTPS. Will dig in and make any changes here if necessary though. Thanks! |
bazel seems to have tests for mTLS with both https and grpcs: |
fixes #356
this adds in support for specifying a
--tls_ca_file
which when provided will validate client certificates against that particular authority.I originally thought about splitting the
--tls_ca_file
option into two (e.g. having an:--mtls
flag, that actually enforced the client certificates), but I figured if this is the only use case for specifying a--tls_ca_file
(which it seems right now this is true for this project, though not in general) best to not add an extra option. If you'd like though I can always go back, and add it in.This seems to pass all tests locally. I've also run just locally on my machine with self signed certs:
The Endpoint is properly authenticated:
And with no ca certificate, but a cert/key it just accepts any user as it does now: