[tls] Move handshaking behavior into SslSocketInfo. by ambuc · Pull Request #12571 · envoyproxy/envoy

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

  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

  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 PiotrSikora, alyssawilk, htuch, lizan and mattklein123 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 antoniovicente and lizan 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
mattklein123 merged 11 commits intoenvoyproxy:masterfrom
ambuc:custom-handshaker-socketinfo

ambuc commented Aug 10, 2020

Uh oh!

antoniovicente Aug 10, 2020

Uh oh!

ambuc Aug 10, 2020

Uh oh!

antoniovicente Aug 10, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

antoniovicente Aug 10, 2020

Uh oh!

Uh oh!

Uh oh!

lizan left a comment

Uh oh!

Uh oh!

lizan left a comment

Uh oh!

Uh oh!

Uh oh!

lizan commented Aug 12, 2020

Uh oh!

repokitteh-read-only bot commented Aug 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

ambuc commented Aug 10, 2020

Uh oh!

antoniovicente Aug 10, 2020

Choose a reason for hiding this comment

Uh oh!

ambuc Aug 10, 2020

Choose a reason for hiding this comment

Uh oh!

antoniovicente Aug 10, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

antoniovicente Aug 10, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lizan commented Aug 12, 2020

Uh oh!

repokitteh-read-only bot commented Aug 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants