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

ambuc · 2020-07-14T17:50:11Z

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

Commit Message: [tls] Add a custom listener handshaker for TLS.
Additional Description: This CL introduces a mechanism for modifying the behavior a TLS socket uses to handshake. It moves the existing implementation into a default HandshakerImpl and introduces an extension for injecting custom handshaker behavior.
Risk Level: Low, the default handshaking behavior in unmodified Envoy is unchanged.
Testing: All existing unit/integration tests use the default handshaking behavior (as before). The existing handshaker implementation also gains unit tests.
Docs Changes: None -- I think the API comments will generate documentation.
Release Notes: n/a

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

repokitteh-read-only · 2020-07-14T17:50:16Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #12075 was opened by ambuc.

see: more, trace.

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

rojkov · 2020-07-15T07:07:04Z

What's the use case for a custom handshaker?

ambuc · 2020-07-15T13:35:23Z

@rojkov We want to be able to modify TLS termination to perform crypto operations outside of the Envoy binary.

rojkov · 2020-07-15T15:19:53Z

We want to be able to modify TLS termination to perform crypto operations outside of the Envoy binary.

Probably a custom PrivateKeyMethodProvider could be suitable for your needs.

ambuc · 2020-07-15T15:45:01Z

I got the impression PrivateKeyMethodProvider lets you inject behavior before a handshake. My motivation for this change is to override behavior within a handshake.

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

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

htuch · 2020-07-15T19:10:41Z

@ggreenway @PiotrSikora @lizan could you folks take a look at this? Thanks.

source/extensions/transport_sockets/tls/handshaker_impl.cc

source/extensions/transport_sockets/tls/ssl_socket.cc

include/envoy/ssl/handshaker.h

PiotrSikora · 2020-07-17T19:19:46Z

Probably a custom PrivateKeyMethodProvider could be suitable for your needs.

For some use cases, yes. But custom handshaker allows you to offload more than just private key operations.

include/envoy/ssl/handshaker.h

source/extensions/transport_sockets/tls/context_config_impl.cc

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

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

ggreenway

I think the overall design is fine.

I'd like to see a reference implementation of calling another handshaker out of process, or at least in another thread in a test. This can't be adequately tested without it I don't think. Also, I think should be clear how to implement this interface, ie an implementation needs to get some data from the SSL, send it to the thing that does the privatekey operations, do those operations, send the result back, and apply the result to the SSL. The private-key operation server-like thing doesn't need to be production-grade, but the envoy implementation of a handshaker probably should be.

include/envoy/ssl/handshaker.h

source/extensions/transport_sockets/tls/context_config_impl.cc

source/extensions/transport_sockets/tls/context_config_impl.h

source/extensions/transport_sockets/tls/handshaker_impl.cc

include/envoy/ssl/handshaker.h

source/extensions/transport_sockets/tls/handshaker_impl.h

test/extensions/transport_sockets/tls/handshaker_test.cc

ggreenway · 2020-07-20T17:12:02Z

One option for a Handshaker implementation would be to write an integration with https://github.com/cloudflare/gokeyless. Although I don't know if the license actually allows that (it's not a standard open-source license).

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

ggreenway

The overall pattern looks good.

source/extensions/transport_sockets/tls/context_config_impl.cc

include/envoy/ssl/handshaker.h

ggreenway

If you put names like this (bssl, rbio, etc) in backticks in the comment the spellchecker won't check them, and then they don't need to be in the dictionary.

include/envoy/ssl/handshaker.h

source/extensions/transport_sockets/tls/ssl_socket.h

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

… to a non-dangling SSL*. Signed-off-by: James Buckland <jbuckland@google.com>

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

include/envoy/ssl/handshaker.h

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

include/envoy/ssl/handshaker.h

antoniovicente · 2020-07-29T20:52:45Z

source/extensions/transport_sockets/tls/handshaker_impl.h

+    handshaker_callbacks_ = &handshaker_callbacks;
+  }
+
+  SSL* ssl() override { return ssl_.get(); }


I think we're missing an important restriction on the use of this method: access to the SSL* object before handshake is complete should be forbidden.

I think this restriction clashes with use of this method in SslSocketInfo's constructor and possibly in a few other places.

antoniovicente · 2020-07-29T20:53:55Z

test/extensions/transport_sockets/tls/handshaker_test.cc

+  }
+
+  Network::PostIoAction doHandshake(Ssl::SocketState& state) override {
+    ASSERT(state != Ssl::SocketState::HandshakeComplete && state != Ssl::SocketState::ShutdownSent);


This for test doHandshake implementation looks too real. it seems like a copy of HandshakeImpl::doHandshake.

It basically is. I specifically wanted to demonstrate special-case error handling under switch(SSL_get_error(...)). Should I add more comments to call out that only the SSL_ERROR_WANT_X509_LOOKUP block is different?

ggreenway · 2020-07-30T16:31:45Z

include/envoy/ssl/handshaker.h

+  virtual SSL* ssl() PURE;
+};
+
+using HandshakerSharedPtr = std::shared_ptr<Handshaker>;


It seems like we should statically know the lifetime of this, and should be able to use a unique_ptr + references.

What's the alternative here? @lizan pointed out (#12075 (comment)) that the SslSocketInfo struct expects to have access to the SSL object after the connection has been destroyed, for logging purposes. What do you think of explicitly std::move()ing the SSL object from the handshaker to the SslSocketInfo just before ~SslSocket(), perhaps during SslSocket::shutdown()? @antoniovicente

It might make sense for Handshaker to implement Ssl::ConnectionInfo, as the socket info class is basically represent the result of handshake.

Handshaker providing Ssl::ConnectionInfo makes sense.

PTAL @ #12571, I am attempting to move the handshaker behavior into SslSocketInfo before adding an extension point.

ggreenway · 2020-08-03T16:32:56Z

/wait

ambuc · 2020-08-10T14:36:02Z

Closing this PR. Will continue work in #12571 and a follow-up PR. PTAL at #12571 instead.

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] Add a custom listener handshaker for TLS.

647e23c

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

repokitteh-read-only bot added the api label Jul 14, 2020

[misc] run fix_format

a497d44

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

lizan self-assigned this Jul 14, 2020

ambuc added 2 commits July 15, 2020 14:03

Merge branch 'master' of github.com:envoyproxy/envoy into custom-hand…

df9dfb2

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

[misc] run fix_format

22f2003

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

htuch requested review from lizan, PiotrSikora and ggreenway July 15, 2020 19:10

PiotrSikora reviewed Jul 17, 2020

View reviewed changes

ambuc marked this pull request as ready for review July 17, 2020 19:24

lizan suggested changes Jul 18, 2020

View reviewed changes

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

source/extensions/transport_sockets/tls/context_config_impl.cc Outdated Show resolved Hide resolved

lizan added the waiting label Jul 18, 2020

ambuc added 2 commits July 20, 2020 09:32

[ssl] Rename some HandshakerCallbacks methods.

a403972

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

[tls] Rename HandOff/HandBack.

59d5905

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

repokitteh-read-only bot removed the waiting label Jul 20, 2020

ambuc requested review from lizan and PiotrSikora July 20, 2020 13:55

ambuc added 2 commits July 20, 2020 10:49

[misc] run fix_format

2168d8b

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

[misc] remove comment formatting on PemPasswordCallback, breaking che…

44b31b8

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

ggreenway requested changes Jul 20, 2020

View reviewed changes

[misc] fix typo in handshaker_test

78656cc

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

[misc] Run fix_format.

cb39b1a

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

ggreenway requested changes Jul 28, 2020

View reviewed changes

source/extensions/transport_sockets/tls/context_config_impl.cc Outdated Show resolved Hide resolved

source/extensions/transport_sockets/tls/context_config_impl.cc Outdated Show resolved Hide resolved

ggreenway reviewed Jul 28, 2020

View reviewed changes

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

ggreenway reviewed Jul 28, 2020

View reviewed changes

ggreenway self-assigned this Jul 28, 2020

lizan reviewed Jul 28, 2020

View reviewed changes

ambuc added 6 commits July 29, 2020 10:15

[tls] Clean up initialization order inside context_config_impl

ed5f209

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

[tls] Rename methods to onSuccessCb/onFailureCb

2c7d3c8

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

[tls] dedup HandshakerMaker with HandshakerFactoryCb

2d5ec2f

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

[misc] Remove wbio, rbio, bssl from dictionary

f32acbc

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

[tls] HandshakerPtr is now a shared_ptr; SslSocketInfo retains access…

18dc2c6

… to a non-dangling SSL*. Signed-off-by: James Buckland <jbuckland@google.com>

Move HandshakerCallbacks into setCallbacks()

c7c40b5

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

ambuc requested review from lizan and ggreenway July 29, 2020 17:11

lizan reviewed Jul 29, 2020

View reviewed changes

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

[misc] Rename HandshakerPtr to HandshakerSharedPtr

f711e26

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

ambuc requested a review from lizan July 29, 2020 20:49

antoniovicente reviewed Jul 29, 2020

View reviewed changes

ggreenway reviewed Jul 30, 2020

View reviewed changes

ambuc requested review from ggreenway and antoniovicente July 30, 2020 17:18

repokitteh-read-only bot added the waiting label Aug 3, 2020

ambuc mentioned this pull request Aug 10, 2020

[tls] Move handshaking behavior into SslSocketInfo. #12571

Merged

ambuc closed this Aug 10, 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] Add a custom listener handshaker for TLS. #12075

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

ambuc commented Jul 14, 2020

repokitteh-read-only bot commented Jul 14, 2020

rojkov commented Jul 15, 2020

ambuc commented Jul 15, 2020

rojkov commented Jul 15, 2020

ambuc commented Jul 15, 2020

htuch commented Jul 15, 2020

PiotrSikora commented Jul 17, 2020

ggreenway left a comment

ggreenway commented Jul 20, 2020

ggreenway left a comment

ggreenway left a comment

antoniovicente Jul 29, 2020

antoniovicente Jul 29, 2020

ambuc Jul 30, 2020

ggreenway Jul 30, 2020

ambuc Jul 30, 2020

lizan Jul 30, 2020

antoniovicente Aug 3, 2020 •

edited

Loading

ambuc Aug 10, 2020

ggreenway commented Aug 3, 2020

ambuc commented Aug 10, 2020

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

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

Conversation

ambuc commented Jul 14, 2020

repokitteh-read-only bot commented Jul 14, 2020

rojkov commented Jul 15, 2020

ambuc commented Jul 15, 2020

rojkov commented Jul 15, 2020

ambuc commented Jul 15, 2020

htuch commented Jul 15, 2020

PiotrSikora commented Jul 17, 2020

ggreenway left a comment

Choose a reason for hiding this comment

ggreenway commented Jul 20, 2020

ggreenway left a comment

Choose a reason for hiding this comment

ggreenway left a comment

Choose a reason for hiding this comment

antoniovicente Jul 29, 2020

Choose a reason for hiding this comment

antoniovicente Jul 29, 2020

Choose a reason for hiding this comment

ambuc Jul 30, 2020

Choose a reason for hiding this comment

ggreenway Jul 30, 2020

Choose a reason for hiding this comment

ambuc Jul 30, 2020

Choose a reason for hiding this comment

lizan Jul 30, 2020

Choose a reason for hiding this comment

antoniovicente Aug 3, 2020 • edited Loading

Choose a reason for hiding this comment

ambuc Aug 10, 2020

Choose a reason for hiding this comment

ggreenway commented Aug 3, 2020

ambuc commented Aug 10, 2020

antoniovicente Aug 3, 2020 •

edited

Loading