[tls] Move handshaking behavior into SslSocketInfo. #12571

ambuc · 2020-08-10T14:33:24Z

Signed-off-by: James Buckland jbuckland@google.com

Commit Message: Move handshaking behavior into SslSocketInfo.
Additional Description: This change makes possible (and simpler) a later change in which we allow users to modify the behavior of the handshaker (i.e. to add new branches for handing SSL_ERRORs) by using an extension point. See discussion here: (#12075 (review)).

This PR does not add that extension point, but it does move all handshaking behavior into the SslSocketInfo class (which already has SSL* ownership) to make that change simpler.

However, since doing a handshake can modify the SSL object, SslSocketInfo::doHandshake must be non-const, and this change swaps ConnectionInfoConstSharedPtr for ConnectionInfoSharedPtr everywhere.

This change also necessitated pulling out Ssl::SocketState and Network::PostIoAction into their own build targets to break dependency cycles.

Risk Level: Low, no behavior change.
Testing: N/A, the suite of //test/extensions/transport_sockets/tls/... did not test SslSocketInfo and SslSocket independently before and does not now.
Docs Changes: N/a
Release Notes: N/a

Signed-off-by: James Buckland <jbuckland@google.com>

antoniovicente · 2020-08-10T19:38:46Z

source/extensions/transport_sockets/tls/ssl_socket.h

@@ -77,11 +86,13 @@ class SslSocketInfo : public Envoy::Ssl::ConnectionInfo {
  std::string ciphersuiteString() const override;
  const std::string& tlsVersion() const override;
  absl::optional<std::string> x509Extension(absl::string_view extension_name) const override;
+  Network::PostIoAction doHandshake(Ssl::SocketState& state) override;
  SSL* ssl() const { return ssl_.get(); }


Should we restrict access to the SSL* prior to handshake completing?

Yes, I think so. It seems like a matter for the next PR, but my plan is (a) the state accessor method returns HandshakeOngoing, a new state enum value indicating that the handshake is ongoing, and (b) when state == HandshakeOngoing, ssl() might return nullptr.

Sounds good. The move of state_ to this object should make that future change possible.

source/extensions/transport_sockets/tls/ssl_socket.cc

include/envoy/network/transport_socket.h

Signed-off-by: James Buckland <jbuckland@google.com>

source/extensions/transport_sockets/tls/ssl_socket.cc

antoniovicente · 2020-08-10T21:44:23Z

source/extensions/transport_sockets/tls/ssl_socket.h

@@ -77,11 +86,13 @@ class SslSocketInfo : public Envoy::Ssl::ConnectionInfo {
  std::string ciphersuiteString() const override;
  const std::string& tlsVersion() const override;
  absl::optional<std::string> x509Extension(absl::string_view extension_name) const override;
+  Network::PostIoAction doHandshake(Ssl::SocketState& state) override;
  SSL* ssl() const { return ssl_.get(); }


Sounds good. The move of state_ to this object should make that future change possible.

include/envoy/network/transport_socket.h

include/envoy/ssl/connection.h

Signed-off-by: James Buckland <jbuckland@google.com>

lizan

Thanks, the structure LGTM.

source/extensions/transport_sockets/tls/ssl_socket.h

Signed-off-by: James Buckland <jbuckland@google.com>

lizan

comment bits, lgtm otherwise

include/envoy/network/transport_socket.h

include/envoy/ssl/handshaker.h

Signed-off-by: James Buckland <jbuckland@google.com>

lizan · 2020-08-12T23:52:37Z

/retest

repokitteh-read-only · 2020-08-12T23:52:42Z

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #12571 (comment) was created by @lizan.

see: more, trace.

* master: (67 commits) logger: support log control in admin interface and command line option for Fancy Logger (envoyproxy#12369) test: fix http_timeout_integration_test flake (envoyproxy#12654) [fuzz]added an input check in writefilter fuzzer and added test cases (envoyproxy#12628) add 'explicit' restriction. (envoyproxy#12643) scoped_rds_integration_test migrate from api v2 to api v3. (envoyproxy#12633) fuzz: added fuzz test for listener filter tls_inspector (envoyproxy#12617) testing: fix multiple race conditions in simulated time tests (envoyproxy#12527) [tls] Move handshaking behavior into SslSocketInfo. (envoyproxy#12571) header: getting rid of exception-throwing behaviors in header files [the rest] (envoyproxy#12611) router: add new ratelimited retry backoff strategy (envoyproxy#12202) [redis_proxy] added a constraint for route.prefix().size() (envoyproxy#12637) network: add tcp listener backlog config (envoyproxy#12625) runtime: debug log that condition is always true when fractionalPercent numerator > denominator (envoyproxy#12068) WatchDog Extension hook (envoyproxy#12416) router: add dynamic metadata header formatter (envoyproxy#11858) statsd: revert visibility to public (envoyproxy#12621) Fix regression of /build_* in gitignore (envoyproxy#12630) Added a missing extension point to documentation. (envoyproxy#12620) Reverts proxy protocol test on windows (envoyproxy#12619) caching: Improved the tests and coverage of the CacheFilter tree (envoyproxy#12544) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

Additional Description: This PR necessitated decoupling SslHandshakerImpl from ContextConfig a bit. We now pass an int representing the index of the extended_info struct rather than the ContextConfig. This PR moves SslHandshakerImpl to its own build target, moves SslHandshaker construction into the ContextConfig, and adds a HandshakerFactoryContext and HandshakerFactory for modifying the ContextConfig's behavior when constructing a Handshaker. This PR also adds a control (requireCertificates) to turn off the release asserts that a context must have certificates. This PR builds off work in #12571 and refines work done (and abandoned) in #12075. For more discussion please see the comments section of #12075. Risk Level: Low. This PR does not modify existing handshaking behavior, it just adds an extension point for modifying it. Testing: A representative alternative implementation was added under :handshaker_test. Docs Changes: N/a Release Notes: N/a Signed-off-by: James Buckland <jbuckland@google.com>

Additional Description: This PR necessitated decoupling SslHandshakerImpl from ContextConfig a bit. We now pass an int representing the index of the extended_info struct rather than the ContextConfig. This PR moves SslHandshakerImpl to its own build target, moves SslHandshaker construction into the ContextConfig, and adds a HandshakerFactoryContext and HandshakerFactory for modifying the ContextConfig's behavior when constructing a Handshaker. This PR also adds a control (requireCertificates) to turn off the release asserts that a context must have certificates. This PR builds off work in envoyproxy/envoy#12571 and refines work done (and abandoned) in envoyproxy/envoy#12075. For more discussion please see the comments section of envoyproxy/envoy#12075. Risk Level: Low. This PR does not modify existing handshaking behavior, it just adds an extension point for modifying it. Testing: A representative alternative implementation was added under :handshaker_test. Docs Changes: N/a Release Notes: N/a Signed-off-by: James Buckland <jbuckland@google.com> Mirrored from https://github.com/envoyproxy/envoy @ 7d6e7a4e559bdf0346687f7f404412e2412ea6fb

[tls] Move handshaking behavior into SslSocketInfo.

da23a7e

Signed-off-by: James Buckland <jbuckland@google.com>

ambuc requested review from alyssawilk, htuch, lizan, mattklein123 and PiotrSikora as code owners August 10, 2020 14:33

ambuc mentioned this pull request Aug 10, 2020

[tls] Add a custom listener handshaker for TLS. #12075

Closed

ambuc added 2 commits August 10, 2020 10:50

[misc] Run fix_format.

b0f8414

Signed-off-by: James Buckland <jbuckland@google.com>

[misc] fix typo in connection.h

393b621

Signed-off-by: James Buckland <jbuckland@google.com>

jmarantz assigned PiotrSikora Aug 10, 2020

htuch assigned lizan Aug 10, 2020

antoniovicente reviewed Aug 10, 2020

View reviewed changes

lizan reviewed Aug 10, 2020

View reviewed changes

include/envoy/network/transport_socket.h Outdated Show resolved Hide resolved

[tls] Move state_ into SslSocketInfo.

3d56dc8

Signed-off-by: James Buckland <jbuckland@google.com>

ambuc requested a review from antoniovicente August 10, 2020 20:29

[tls] Mark some ConnectionInfoSharedPtrs as const.

1568afc

Signed-off-by: James Buckland <jbuckland@google.com>

ambuc requested a review from lizan August 10, 2020 21:39

antoniovicente reviewed Aug 10, 2020

View reviewed changes

lizan suggested changes Aug 11, 2020

View reviewed changes

include/envoy/network/transport_socket.h Outdated Show resolved Hide resolved

include/envoy/ssl/connection.h Outdated Show resolved Hide resolved

ambuc added 2 commits August 11, 2020 09:04

[tls] Separate interface for handshaking

bdd8d1b

Signed-off-by: James Buckland <jbuckland@google.com>

[tls] Remove non-const ConnectionInfoSharedPtr.

93e2508

Signed-off-by: James Buckland <jbuckland@google.com>

ambuc requested review from lizan and antoniovicente August 11, 2020 13:29

lizan reviewed Aug 11, 2020

View reviewed changes

source/extensions/transport_sockets/tls/ssl_socket.h Outdated Show resolved Hide resolved

ambuc added 2 commits August 12, 2020 09:17

[tls] Fix invalid case style

ad678ff

Signed-off-by: James Buckland <jbuckland@google.com>

[tls] Rename SslSocketInfo to SslHandshakerImpl

0d59a86

Signed-off-by: James Buckland <jbuckland@google.com>

ambuc requested a review from snowp as a code owner August 12, 2020 13:24

ambuc requested a review from lizan August 12, 2020 13:24

[tls] Remove redundant doHandshake method from Connection interface.

ed87486

Signed-off-by: James Buckland <jbuckland@google.com>

lizan reviewed Aug 12, 2020

View reviewed changes

include/envoy/network/transport_socket.h Outdated Show resolved Hide resolved

include/envoy/ssl/handshaker.h Show resolved Hide resolved

[misc] Add documentation for new handshaker interface.

26f3c0d

Signed-off-by: James Buckland <jbuckland@google.com>

ambuc requested a review from lizan August 12, 2020 20:52

lizan approved these changes Aug 12, 2020

View reviewed changes

mattklein123 merged commit b09971d into envoyproxy:master Aug 14, 2020

ambuc mentioned this pull request Aug 14, 2020

[tls] Add an extension point for TLS handshaker behavior. #12658

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tls] Move handshaking behavior into SslSocketInfo. #12571

[tls] Move handshaking behavior into SslSocketInfo. #12571

ambuc commented Aug 10, 2020

antoniovicente Aug 10, 2020

ambuc Aug 10, 2020

antoniovicente Aug 10, 2020

antoniovicente Aug 10, 2020

lizan left a comment

lizan left a comment

lizan commented Aug 12, 2020

repokitteh-read-only bot commented Aug 12, 2020

[tls] Move handshaking behavior into SslSocketInfo. #12571

[tls] Move handshaking behavior into SslSocketInfo. #12571

Conversation

ambuc commented Aug 10, 2020

antoniovicente Aug 10, 2020

Choose a reason for hiding this comment

ambuc Aug 10, 2020

Choose a reason for hiding this comment

antoniovicente Aug 10, 2020

Choose a reason for hiding this comment

antoniovicente Aug 10, 2020

Choose a reason for hiding this comment

lizan left a comment

Choose a reason for hiding this comment

lizan left a comment

Choose a reason for hiding this comment

lizan commented Aug 12, 2020

repokitteh-read-only bot commented Aug 12, 2020