ssl: Add an SslCtxCb hook to Ssl::HandshakerFactory. #15179
Conversation
Signed-off-by: James Buckland <[email protected]>
ambuc
requested review from
asraa,
ggreenway,
lizan and
PiotrSikora
as code owners
There was a problem hiding this comment.
What is this PR trying to fix, exactly? This is the issue with exposing interfaces, but not having consumers for them in public. It's impossiible to review this PR without context.
Also, why are you quoting OpenSSL documentation? We don't use OpenSSL, and I cannot find such statements in BoringSSL documenation.
|
Re the second point: sorry for the link to the wrong docs. I updated the description with the BoringSSL docs' mention of the same notion, that it is invalid to modify SSL_CTX's configuration after it is shared. Re the first point: I have a private implementation of the custom handshaker interface which needs to call SSL_CTX_* methods on the SSL_CTX object. I'm going to write a test which exercises this callsite to regression-proof it and re-request review on this PR. |
Signed-off-by: James Buckland <[email protected]>
Signed-off-by: James Buckland <[email protected]>
Signed-off-by: James Buckland <[email protected]>
Signed-off-by: James Buckland <[email protected]>
Signed-off-by: James Buckland <[email protected]>
Signed-off-by: James Buckland <[email protected]>
| @@ -72,6 +72,7 @@ class SslSocket : public Network::TransportSocket, | |||
| Network::TransportSocketCallbacks* transportSocketCallbacks() override { return callbacks_; } | |||
|
|
|||
| protected: | |||
| friend class SslSocketFriend; | |||
There was a problem hiding this comment.
This class doesn't exist, I don't think? Delete.
There was a problem hiding this comment.
It's used in handshaker_factory_test.cc as a way to peek at an SslSocket's underlying SSL*. I thought briefly about just making the rawSsl() accessor public instead of protected but decided that it would expose the internal SSL* too much.
There was a problem hiding this comment.
The pattern used elsewhere for this is to create a method rawSslForTest().
Signed-off-by: James Buckland <[email protected]>
Signed-off-by: James Buckland <[email protected]>
Signed-off-by: James Buckland <[email protected]>
Signed-off-by: James Buckland <[email protected]>
test/extensions/quic_listeners/quiche/envoy_quic_proof_source_test.cc
Outdated
Show resolved
Hide resolved
test/extensions/quic_listeners/quiche/envoy_quic_proof_verifier_test.cc
Outdated
Show resolved
Hide resolved
| @@ -72,6 +72,7 @@ class SslSocket : public Network::TransportSocket, | |||
| Network::TransportSocketCallbacks* transportSocketCallbacks() override { return callbacks_; } | |||
|
|
|||
| protected: | |||
| friend class SslSocketFriend; | |||
There was a problem hiding this comment.
The pattern used elsewhere for this is to create a method rawSslForTest().
…fig. Signed-off-by: James Buckland <[email protected]>
Signed-off-by: James Buckland <[email protected]>
|
Re #15179 (comment): I changed this PR to introduce |
|
/retest |
|
Retrying Azure Pipelines: |
Commit Message: Add an SslCtxCb hook to Ssl::HandshakerFactory.
Additional Description: This lets a custom handshaker configure the SSL_CTX object after creation, but before it is used to create any SSL objects.
Per the BoringSSL docs, "|SSL_CTX| are reference-counted and may be shared by connections across multiple threads. Once shared, functions which change the |SSL_CTX|'s configuration may not be used." Additionally, the SSL_CTX object is complex enough and has enough methods that, rather than expose each of them via another method on some wrapper class, it is convenient to just allow mutating access via a callback during socket creation (as in tls/context_impl.cc` in this PR).
This PR also provides a reference to the HandshakerFactoryContext to this sslctx_cb callback at runtime so that a custom handshaker can further inject SSL_CTX configuration behavior.
Risk Level: Low
Testing: n/a
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a