-
Notifications
You must be signed in to change notification settings - Fork 169
Implement TLS client authentication #193
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
base: master
Are you sure you want to change the base?
Conversation
|
Shouldn't the rest-server also extract the username from the client certificate similar to #191? Or is the idea to only use the client certificate to allow access to the rest-server and then require an additional htpasswd verification (I'm not particularly sure how useful that is)? |
|
@MichaelEischer IMO, what you request should be optional. I use TLS with my own CA to prevent anyone inside my home network from using the rest server. I need neither htaccess nor TLS username verification though since I'm the only user. |
|
I tested this branch at 517d9cbcad3a52965f73d36fe0fd8cc61e8f1300 and it works just fine and as expected. Notes:
@M1cha If you are still interested to work on this feature, I would offer to test this further and help with documentation or unit tests. |
|
I updated the PR from my personal fork.
Good catch, fixed.
That sounds reasonable. I didn't implement that in my push, yet though.
I've been using the code in this PR for my main backup since I've opened the PR. Unfortunately it looks like the maintainers of rest-server are very inactive. |
|
FWIW, this PR when rebased with For reference, I'm able to spin up rest-server with the command: # Builds the server
CGO_ENABLED=0 go build -o rest-server ./cmd/rest-server
# (In separate terminal window 1) Manually running to test that this works as expected
# (Ctrl + C to stop the server)
rest-server \
--path /foo/bar/path/to/repositories \
--append-only \
--private-repos \
--prometheus \
--listen ":10000" \
--tls --tls-min-ver "1.3" \
--tls-cacert /foo/bar/path/to/all_allowed_clients.combined.public.pemWhere the One note for anybody following, if your client cert is signed by a root or intermediate, you'll need to also include whatever root/intermediate cert that was used to sign the client as well, e.g. cat intermediate.public.crt \
client_a.public.crt \
client_b_signed_by_intermediate.public.crt > all_allowed_clients.combined.public.pemNote that from testing, you just need the immediate signing cert for your client- in this example, intermediate.public.crt was signed by a root.crt, but there wasn't a need to include that here to authenticate |
What is the purpose of this change? What does it change?
Add support for authenticating clients using a CA certificate.
Was the change discussed in an issue or in the forum before?
Closes #73
Checklist
changelog/unreleased/that describes the changes for our users (template here)gofmton the code in all commits