-
Notifications
You must be signed in to change notification settings - Fork 21
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
Improve certificate authentication #93
Conversation
Whoa! I really like this. Thanks for putting in the work. Give me some time to review it. What are some blockers for you right now? |
Well mostly getting the changes to ocaml-ssl in and updating the dependency |
These commits fix the outstanding test issues |
Thanks, I'm gonna take a look at this soon. I pushed a commit bumping the Nix package sources to a commit that includes your openssl PR. Let's see what CI says. |
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.
This looks great!
I left a couple comments that are mostly nits, some questions, and a couple code suggestions. Let me know what you think.
Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
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.
Alright, 2nd round! Everything looks great, there's only the change in bin/carl.ml
that I'd rather revert.
I have added the possibility of passing the cert/key pair as files, and added to carl an implementation resembling curl of client certificates |
Thank you! |
let cert = "./certificates/server.pem" in | ||
let priv_key = "./certificates/server.key" in | ||
Lwt_io.establish_server_with_client_socket | ||
listen_address | ||
(fun client_addr fd -> | ||
let server_ctx = Ssl.create_context Ssl.TLSv1_3 Ssl.Server_context in | ||
Ssl.disable_protocols server_ctx [ Ssl.SSLv23; Ssl.TLSv1_1 ]; | ||
Ssl.load_verify_locations server_ctx ca ""; |
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.
@Firgeis why did you add this here? Trying to run the tests locally, it seems like openssl is not happy with it.
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.
That loads the ca certificate so it can verify the client cert properly. What error are you getting?
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.
It was my bad. dune runtest
works but dune exec lib_test/test_client.exe
couldn't find the certificates.
I'm exploring how to make both work with dune-sites
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.
Probably related to the relative paths
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.
I fixed the paths and now I'm getting an infinite loop running the client certs test. Similar to what I see in #94 (CI just hangs)
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.
Retrieving the client certs here also uses relative paths
https://github.com/Firgeis/piaf/blob/fd5e07ceea0ef9d072abc1b39d39229c1300b85f/lib_test/test_client.ml#L225
We should probably add a try block there
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.
That's not the issue. The problem is that the server never returns if it can't verify the peer. So the try / catch around the client request is useless because the server never really returns a response.
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.
Strange, because when I run the following test:
Line 278 in 6e1b433
(* No client certificate provided *) |
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.
My suggestion was a try block around the reading of the certs not the client request, sorry if I was unclear
CHANGES: - Improve certificate checking and authentication ([anmonteiro/piaf#93](anmonteiro/piaf#93)) - [@Firgeis](https://github.com/Firgeis) - Check certificate SAN IP address when appropriate ([anmonteiro/piaf#96](anmonteiro/piaf#96)) - [@Firgeis](https://github.com/Firgeis) - Close the file descriptor when failing to open a connection ([anmonteiro/piaf#97](anmonteiro/piaf#97)) - [@EduardoRFS](https://github.com/EduardoRFS) - Yield to other threads when reading a message body. This improves fairness for large message bodies ([anmonteiro/piaf#100](anmonteiro/piaf#100)) - Add error handling to `Response.of_file` ([anmonteiro/piaf#103](anmonteiro/piaf#103)) - Add `Client.send` which sends a `Request.t` ([anmonteiro/piaf#110](anmonteiro/piaf#110)) - openssl: set the client verify callback ([anmonteiro/piaf#112](anmonteiro/piaf#112)) - Piaf.Response: add `or_internal_error` ([anmonteiro/piaf#120](anmonteiro/piaf#120)) - Piaf.Response: Add `Body.sendfile` and `Response.sendfile` ([anmonteiro/piaf#124](anmonteiro/piaf#124)) - Piaf.Config: Add `config.flush_headers_immediately` ([anmonteiro/piaf#125](anmonteiro/piaf#125)) - Piaf.Server: Add `config.shutdown_timeout` to wait before shutting down the Piaf server ([anmonteiro/piaf#174](anmonteiro/piaf#174)) - Websocket support ([anmonteiro/piaf#139](anmonteiro/piaf#139)) - Multicore support ([anmonteiro/piaf#151](anmonteiro/piaf#151)) - Allow binding to UNIX domain socket ([anmonteiro/piaf#161](anmonteiro/piaf#161)) - Don't send invalid HTTP/2 headers ([anmonteiro/piaf#197](anmonteiro/piaf#197))
This PR improves client certificate handling in piaf with the following:
Outstanding issues: