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

No way to specify CA/client certificate for SSL connections. #168

Open
grapland0 opened this issue Nov 29, 2023 · 10 comments · Fixed by #173
Open

No way to specify CA/client certificate for SSL connections. #168

grapland0 opened this issue Nov 29, 2023 · 10 comments · Fixed by #173

Comments

@grapland0
Copy link

It always constuct a system-default ssl context which uses system common CA set and no client certificate (for mutual authentation at server side).

@criatura2
Copy link

You can pass the context method to the connection constructor:

asio::ssl::context::method method = asio::ssl::context::tls_client,

@grapland0
Copy link
Author

It is just the method. Can I pass the whole context in? I need to specify CA certificate, as well as client cert/key for mutual authentication.

@criatura2
Copy link

At the moment you can't pass the context. I agree that passing the whole context would have been more flexible, so I might change that in a future release. But I fail to see why you can't first pass the method and then set whatever you have to set. There is only one non-trivial constructor in ssl::context, which is the one you will be using anyway https://www.boost.org/doc/libs/1_83_0/doc/html/boost_asio/reference/ssl__context/context.html

@grapland0
Copy link
Author

Is there any API I can use to directly access the SSL context of current connection from boost::redis?

The get_ssl_context doesn't exist in executor-erased connection (https://github.com/boostorg/redis/blob/7caea928affde85146b738eb456d4772a410f721/include/boost/redis/connection.hpp#L337C7-L337C17)

@mzimbres
Copy link
Collaborator

mzimbres commented Dec 1, 2023

Yeah it is missing. I will add one.

@grapland0
Copy link
Author

I think just adding a constructor with ssl context passed in is sufficient, like Beast's secure websocket. Letting end users play with existing ssl context may interfere with ongoing async ops (or you'll need to document when they can/cannot make change safely)

@mzimbres
Copy link
Collaborator

Did not have the time to look at this yet. Perhaps in the forthcoming weekends.

@mzimbres mzimbres linked a pull request Dec 30, 2023 that will close this issue
@mzimbres
Copy link
Collaborator

@grapland0 It was not my intention to close the ticket, so please reopen if necessary. The PR adds the getters you were missing to the ssl-context. I am still unsure whether and how I will allow passing a custom ssl context to the connection. This is what Boost.MySql does (from a conversation with @anarthal)

The way mysql solves it (on the newer any_connection)

If you want a custom SSL context, you pass a referene

If you don't need a custom context but need TLS, you pass nullptr, and you receive a singleton reference with appropriate values

If you don't need a context because you're not using TLS, you pass nullptr and nothing gets ever created

If you're using a connection pool, SSL contexts are passed by value. A single context object is shared between all connections

SSL contexts are unique pointers to openssl SSL_CTX objects. It's not documented how heavyweight they are, but they have an internal refcount mechanism, so they introduce some overhead, at least.

I understand that using custom TLS connections without using a connection pool is something advanced and not too frequent

@anarthal
Copy link
Contributor

Please note that it is UB to set properties of SSL contexts once a ssl::stream is created from them. If connection creates internally a ssl::stream when constructed, the ssl::context must not be modified after construction.

@mzimbres mzimbres reopened this Dec 30, 2023
@mzimbres
Copy link
Collaborator

mzimbres commented Jan 3, 2024

Fix is merged now. Please have a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants