[tls] Add an extension point for TLS handshaker behavior. by ambuc · Pull Request #12658 · envoyproxy/envoy

ambuc · 2020-08-14T20:06:10Z

Commit Message: Add an extension point to allow overriding TLS handshaker behavior.

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

repokitteh-read-only · 2020-08-14T20:06:17Z

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: #12658 was opened by ambuc.

see: more, trace.

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

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

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

lizan

Other than style nits LGTM

test/extensions/transport_sockets/tls/handshaker_test.cc

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

PiotrSikora

I think a lot more configuration settings should be guarded, since depending on what's handled by the custom handshaker, a lot of features in the context is going to be a dead code:

Does the custom handshaker provide certificates and/or private key operations?
Does the custom handshaker handle session resumption (Session IDs, Session Tickets, PSK)? Notably, in TLS 1.3, NewSessionTicket message is sent after the handshake is completed.
Does the custom handshaker handle ALPN negotiation?
Does the custom handshaker verify client certificates?
Is the custom handshaker FIPS compliant? Should we reject configuration if non-compliant handshaker is provided in --define=boringssl=fips builds?

api/envoy/extensions/transport_sockets/tls/v3/tls.proto

source/extensions/transport_sockets/tls/context_config_impl.cc

source/extensions/transport_sockets/tls/context_impl.cc

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

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

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

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

ambuc · 2020-08-21T20:25:53Z

I think a lot more configuration settings should be guarded, since depending on what's handled by the custom handshaker, a lot of features in the context is going to be a dead code:

Does the custom handshaker provide certificates and/or private key operations?

Does the custom handshaker handle session resumption (Session IDs, Session Tickets, PSK)? Notably, in TLS 1.3, NewSessionTicket message is sent after the handshake is completed.

Does the custom handshaker handle ALPN negotiation?

Does the custom handshaker verify client certificates?

Is the custom handshaker FIPS compliant? Should we reject configuration if non-compliant handshaker is provided in --define=boringssl=fips builds?

This makes sense to me. I've introduced a struct enumerating these requirements (HandshakerRequirements) and propagated it down to ContextConfigImpl. I've gated several SSL_CTX_foo calls behind these bools, and the bools are configurable from the extended factory impl. I think this provides the appropriate controls for an implementation of this handshaker to switch off SSL calls when required.

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

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

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

ambuc · 2020-09-08T19:03:51Z

/retest

repokitteh-read-only · 2020-09-08T19:03:55Z

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Cannot retry non-completed check: envoy-presubmit (Linux-x64 compile_time_options), please wait.

🐱

Caused by: a #12658 (comment) was created by @ambuc.

see: more, trace.

ambuc · 2020-09-08T21:06:30Z

/retest

repokitteh-read-only · 2020-09-08T21:06:35Z

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

🐱

Caused by: a #12658 (comment) was created by @ambuc.

see: more, trace.

antoniovicente · 2020-09-09T00:54:16Z

@PiotrSikora PTAL. Any other comments?

ambuc · 2020-09-09T14:01:18Z

/retest

repokitteh-read-only · 2020-09-09T14:01:22Z

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Check envoy-presubmit didn't fail.

🐱

Caused by: a #12658 (comment) was created by @ambuc.

see: more, trace.

PiotrSikora

You need to go through all code of TLS extension and add capability guards where appropriate, e.g. the certificate loading block is not guarded by capabilities right now, etc.

Also, it would be helpful if you were consistent and always added them either as the first or as the last condition, right now it's mixed.

source/extensions/transport_sockets/tls/context_impl.cc

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

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

ambuc · 2020-09-10T14:40:56Z

You need to go through all code of TLS extension and add capability guards where appropriate, e.g. the certificate loading block is not guarded by capabilities right now, etc.

Also, it would be helpful if you were consistent and always added them either as the first or as the last condition, right now it's mixed.

The capability guards are now either outside, or the last argument to a conditional. I added some capability guards around the cert loading mechanisms as well. Thanks for catching this :)

ambuc · 2020-09-10T16:03:40Z

/retest

repokitteh-read-only · 2020-09-10T16:03:57Z

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Cannot retry non-completed check: envoy-presubmit (Linux multi-arch docker), please wait.

🐱

Caused by: a #12658 (comment) was created by @ambuc.

see: more, trace.

ambuc · 2020-09-10T17:35:37Z

/retest

repokitteh-read-only · 2020-09-10T17:35:41Z

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

🐱

Caused by: a #12658 (comment) was created by @ambuc.

see: more, trace.

addressed

mattklein123 · 2020-09-11T16:02:51Z

@ambuc post merge drive by: can you please add a release note and some documentation for this feature? For docs I'm thinking here? https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/security/ssl

In particular it would be good to briefly discuss how this feature is different from the existing private key methods support. Thank you!

mattklein123 · 2020-09-11T16:54:50Z

source/extensions/transport_sockets/tls/ssl_socket.cc

-  info_ = std::make_shared<SslHandshakerImpl>(std::move(ssl), ctx_, this);
-
+      ctx_(std::dynamic_pointer_cast<ContextImpl>(ctx)),
+      info_(std::dynamic_pointer_cast<SslHandshakerImpl>(


I'm probably missing something, but how is this dynamic_cast safe if someone is using a custom implementation? Don't you need a virtual method to get the info off of the handshake interface since you don't know the inheritance structure of the implementation?

My custom implementation in the test is fortunate enough to extend SslHandshakerImpl. My thought was that anyone who wanted to write their own custom implementation of Ssl::Handshaker would also want to extend the existing SslHandshakerImpl to avoid reimplementing all the fiddly methods in ConnectionInfo. But you're right, this isn't safe for anyone who writes their own impl which doesn't extend SslHandshakerImpl.

I think the right thing to do here is (a) add the relevant methods to Ssl::Handshaker (the interface) so that it's OK for (b) info_ to be an Ssl::Handshaker*. I'll fix this forward in a new PR.

Cool thanks. Feel free to combine with the docs PR. Up to you.

Addressing this in #13081. Docs PR to follow.

ambuc · 2020-09-14T15:43:43Z

@ambuc post merge drive by: can you please add a release note and some documentation for this feature? For docs I'm thinking here? https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/security/ssl

In particular it would be good to briefly discuss how this feature is different from the existing private key methods support. Thank you!

Addressing this in #13083.

ambuc requested review from PiotrSikora and lizan as code owners August 14, 2020 20:06

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

ambuc added 7 commits August 14, 2020 16:06

[tls] SslHandshakerImpl now takes an int argument

bc111ba

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

[tls] Move ssl_handshaker_impl to its own target

e3e8dbc

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

SslHandshakerImpl construction is now a method of ContextConfig.

5b718f4

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

[api] Add custom_listener_handshaker

779758a

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

SslHandshakerImpl construction is now pluggable

80a4d12

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

Add SslHandshakerImplForTest class and new :handshaker_test

a81686e

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

[tls] Add requireCertificates() restriction

ae2dd4e

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

ambuc force-pushed the custom-handshaker branch from 726ed24 to ae2dd4e Compare August 14, 2020 20:06

jmarantz assigned PiotrSikora Aug 15, 2020

ambuc added 2 commits August 17, 2020 09:20

[misc] fix spelling issue

de92887

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

[tls] Initialize |len| in handshaker_test.

11d01e3

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

ggreenway assigned lizan Aug 18, 2020

ambuc added 2 commits August 20, 2020 15:58

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

cf1ddd0

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

[tls] Expose connection(), not connectionState().

60d94a5

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

lizan reviewed Aug 20, 2020

View reviewed changes

test/extensions/transport_sockets/tls/handshaker_test.cc Outdated Show resolved Hide resolved

test/extensions/transport_sockets/tls/handshaker_test.cc Outdated Show resolved Hide resolved

[tls] lowercase test helper methods

e12eadc

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

ambuc requested a review from lizan August 20, 2020 21:22

PiotrSikora previously requested changes Aug 21, 2020

View reviewed changes

api/envoy/extensions/transport_sockets/tls/v3/tls.proto Outdated Show resolved Hide resolved

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

source/extensions/transport_sockets/tls/context_impl.cc Show resolved Hide resolved

ambuc added 4 commits August 21, 2020 08:51

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

770c5b0

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

[tls] requireCertificates should guard all certificate checks in Serv…

62874c2

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

[misc] Rename custom_listener_handshaker to custom_handshaker

399b473

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

[tls] Add HandshakerRequirements struct to enable/disable SSL calls i…

5637755

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

ambuc requested a review from PiotrSikora August 21, 2020 20:26

ambuc added 2 commits August 24, 2020 11:08

[misc] Put SSL_CTX methods in backticks to pass spell check.

3547b6b

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

[misc] Reword a sentence to pass check_spelling.

a981e60

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

ambuc added 2 commits September 8, 2020 11:49

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

a625798

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

[misc] Run fix_format.

9023817

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

ambuc dismissed lizan’s stale review via 9023817 September 8, 2020 17:16

repokitteh-read-only bot added the api label Sep 8, 2020

ambuc requested a review from lizan September 8, 2020 17:18

PiotrSikora reviewed Sep 9, 2020

View reviewed changes

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

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

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

ambuc added 2 commits September 10, 2020 10:14

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

c28d2fa

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

[tls] Better capability-protect cert loading

16e366a

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

ambuc requested a review from PiotrSikora September 10, 2020 15:32

lizan approved these changes Sep 11, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Sep 11, 2020

lizan merged commit 7d6e7a4 into envoyproxy:master Sep 11, 2020

mattklein123 reviewed Sep 11, 2020

View reviewed changes

ambuc mentioned this pull request Sep 14, 2020

[tls] Introduce HandshakerAndConnectionInfo interface. #13081

Closed

Conversation

ambuc commented Aug 14, 2020

Uh oh!

repokitteh-read-only bot commented Aug 14, 2020

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

PiotrSikora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ambuc commented Aug 21, 2020

Uh oh!

ambuc commented Sep 8, 2020

Uh oh!

repokitteh-read-only bot commented Sep 8, 2020

Uh oh!

ambuc commented Sep 8, 2020

Uh oh!

repokitteh-read-only bot commented Sep 8, 2020

Uh oh!

antoniovicente commented Sep 9, 2020

Uh oh!

ambuc commented Sep 9, 2020

Uh oh!

repokitteh-read-only bot commented Sep 9, 2020

Uh oh!

PiotrSikora left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ambuc commented Sep 10, 2020

Uh oh!

ambuc commented Sep 10, 2020

Uh oh!

repokitteh-read-only bot commented Sep 10, 2020

Uh oh!

ambuc commented Sep 10, 2020

Uh oh!

repokitteh-read-only bot commented Sep 10, 2020

Uh oh!

mattklein123 commented Sep 11, 2020

Uh oh!

mattklein123 Sep 11, 2020

Choose a reason for hiding this comment

Uh oh!

ambuc Sep 11, 2020

Choose a reason for hiding this comment

Uh oh!

mattklein123 Sep 11, 2020

Choose a reason for hiding this comment

Uh oh!

ambuc Sep 14, 2020

Choose a reason for hiding this comment

Uh oh!

ambuc commented Sep 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

PiotrSikora left a comment •

edited

Loading