conduit-lwt-unix: allow openssl clients to customize the ssl context and the verification #417
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
psafont
changed the title
psafont
force-pushed
the
openssl-ctx
branch
2 times, most recently
from
7e349c4
to
4de8786
Compare
edwintorok
reviewed
Nov 29, 2022
The ssl context may be used for connect_with_ssl. When tls_own_key is not configured the configured ssl_ctx is used, by default this is the default client ssl_ctx, just as before. Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
|
@hannesm Would you mind reviewing these changes? I've integrated them into a xenserver build and all related tests are passing which gives me confidence it's working. I've also manually tested the defaults for the client and it's verifying the domain name of certificate as previously |
|
I guess this is fine, although it makes the ocaml-tls and openssl versions slightly different. The API is only changed in a minor way (adding optional arguments) which allows old programs to use the new API as well. |
hannesm
reviewed
Dec 2, 2022
hannesm
reviewed
Dec 2, 2022
hannesm
reviewed
Dec 2, 2022
hannesm
reviewed
Dec 2, 2022
psafont
force-pushed
the
openssl-ctx
branch
3 times, most recently
from
c2d9e06
to
ddd44e1
Compare
When a valid hostname is not available it's better to fail early with a useful error message rather than letting the connection go on and letting OpenSSL fail with an undecipherable message. Note that the "hostname" parameters are strings and don't have to be hostnames, they can be IPs as well when using cohttp. Ideally these should be a union type of domain names and ip addresses for better clarity, but this would be a breaking change. Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
This is not exposed currently to the user, so there is no change in functionality. This allows clients to turn on and off hostname and ip verification in the remote cert independently in the unusual case where it's needed by changing the default in the library. Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
The only options allowed are whether the hostname or the IP are used to validate the remote host's certificate Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
|
I've tried to clarify the confusing situation of "hostname"s by changing some the names of some values and explaining it into a commit message. Not sure what more can I do there without breaking compatibility |
Now the SNIs is only sent when there's a domain name, as this is the only type of server names allowed by the RFC Additionally IP verification for the peer certificate can be enabled if needed Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
|
looks good to me. does the latest commit still work in your environment? |
|
unit tests look good. I'll do a whole build and let you know tomorrow whether the tests went well |
|
All tests passed successfully :) |
hannesm
added a commit
to hannesm/opam-repository
that referenced
this pull request
Dec 14, 2022
…and conduit-async (6.0.2) CHANGES: * conduit-lwt-unix-ssl: allow users to create a client ssl_context and use it for any connections. This allows users to manage the lifecycle of the context. * conduit-lwt-unix-ssl: domain name verification can be disabled by users, it's enabled by default. The library returns an error when the hostname verification is turned on but it cannot be performed, this follows the TLS implementation. * conduit-lwt-unix-ssl: IP verification can be enabled by users, it's disabled by default. * conduit-lwt-unix-ssl: SNI is not sent when there isn't a domain name available all done by @psafont in mirage/ocaml-conduit#417
|
thanks for your patience, merged and released as 6.0.2 in ocaml/opam-repository#22665 |
|
Sorry, but in the case that I suspect we'll have to define |
|
Ah, sorry about that. Yes the solution makes sense. |
|
@psafont great, would you have time to provide a PR? |
|
Yes, of course.here it is: #418 |
hannesm
added a commit
to hannesm/opam-repository
that referenced
this pull request
Dec 15, 2022
…and conduit-async (6.1.0) CHANGES: done by @psafont in mirage/ocaml-conduit#417: * conduit-lwt-unix-ssl: allow users to create a client ssl_context and use it for any connections. This allows users to manage the lifecycle of the context. * conduit-lwt-unix-ssl: domain name verification can be disabled by users, it's enabled by default. The library returns an error when the hostname verification is turned on but it cannot be performed, this follows the TLS implementation. * conduit-lwt-unix-ssl: IP verification can be enabled by users, it's disabled by default. * conduit-lwt-unix-ssl: SNI is not sent when there isn't a domain name available * conduit-lwt-unix: avoid direct use of Ssl in conduit_lwt_unix (mirage/ocaml-conduit#418 @psafont)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Opening the PR to gather comments and discussion around it, supersedes #390
This PR allows clients to have complete control of the ssl context used by conduit, as the previous PR intended. Unlike it, this one integrates into the conduit context creation, like the client-side authenticator. It doesn't allow to control the hostname that verifies the server. xapi doesn't use it anymore and doubt it's going to be useful for anybody else.
It similarly includes into the context a verification control to be able to set whether to verify the hostname and/or the IP
Issues:
ssl_ctxdoes not make it explicit that it will be used only for client connections, not server ones. If clients want to change the ssl context for server connections a new parameter will be added and might make things confusing. Do we want to useclient_ssl_ctxhere? I can even addserver_ssl_ctxparameter even if I don't have a need for it.