network: add timeout for transport connect #13610

akonradi · 2020-10-16T16:05:55Z

Commit Message: Add a timeout for how long an incoming connection has to establish a transport-level connection
Additional Description:
Adds a configurable timeout for the amount of time a downstream client is allowed to finish the transport-level connect before the connection is forcefully terminated. This can be used to require that a client finishes the TLS handshake in a bounded amount of time.

Risk Level: low
Testing: added unit tests
Docs Changes: document new field
Release Notes: document new feature
Platform Specific Features: none

Addresses #11426

Add a new interface and impl class for incoming connections to the server. This class currently adds no methods on top of the existing Connection class, but will be used to add new functionality in the future. Signed-off-by: Alex Konradi <akonradi@google.com>

Add code to the new ServerConnection class to support a timeout for the transport socket connect event. This will be used to require TLS connections to complete their handshake within a specified period of time. Signed-off-by: Alex Konradi <akonradi@google.com>

Add a config field to the filter chain proto to specify the transport socket timeout and use that to set the value on the server socket. Signed-off-by: Alex Konradi <akonradi@google.com>

Signed-off-by: Alex Konradi <akonradi@google.com>

repokitteh-read-only · 2020-10-16T16:06:02Z

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: #13610 was opened by akonradi.

see: more, trace.

akonradi · 2020-10-16T16:06:15Z

/assign @antoniovicente

Signed-off-by: Alex Konradi <akonradi@google.com>

htuch · 2020-10-16T21:15:45Z

/lgtm api

antoniovicente

Very elegant implementation on top of raiseEvent. Thanks for getting this done.

docs/root/version_history/current.rst

source/common/network/connection_impl.cc

source/common/network/connection_impl.h

include/envoy/network/connection.h

test/mocks/network/connection.h

test/common/network/connection_impl_test.cc

Signed-off-by: Alex Konradi <akonradi@google.com>

source/common/network/connection_impl.cc

test/common/network/connection_impl_test.cc

docs/root/faq/configuration/timeouts.rst

antoniovicente · 2020-10-19T19:01:13Z

docs/root/faq/configuration/timeouts.rst

+
+* The :ref:`transport_socket_connect_timeout <envoy_v3_api_field_config.listener.v3.FilterChain.transport_socket_connect_timeout>`
+  specifies the amount of time Envoy will wait for a downstream client to complete transport-level
+  negotiations. This can be used to limit the amount of time allowed to finish a TLS handshake


Worth generalizing this description further? The timeout covers transport socket establishment, which includes the handshake step when establishing TLS or ALPN sockets.

Done, PTAL. I'm open to suggestions for optimizing for searchability for "tls timeout" and such.

Sorry, I hadn't looked at the rest of this doc until now.

It makes some sense to me to move this timeout closer to the top. Alternatively, you could add references to the HTTP connection level timeouts and TCP timeouts sections: "See :ref:`Transport Socket` for other connection timeouts."

Moved above the TCP section.

I think that now it is even more hidden than before.

Signed-off-by: Alex Konradi <akonradi@google.com>

docs/root/faq/configuration/timeouts.rst

antoniovicente · 2020-10-20T23:12:13Z

docs/root/faq/configuration/timeouts.rst

+
+* The :ref:`transport_socket_connect_timeout <envoy_v3_api_field_config.listener.v3.FilterChain.transport_socket_connect_timeout>`
+  specifies the amount of time Envoy will wait for a downstream client to complete transport-level
+  negotiations. This can be used to limit the amount of time allowed to finish a TLS handshake


Sorry, I hadn't looked at the rest of this doc until now.

It makes some sense to me to move this timeout closer to the top. Alternatively, you could add references to the HTTP connection level timeouts and TCP timeouts sections: "See :ref:`Transport Socket` for other connection timeouts."

docs/root/version_history/current.rst

source/server/listener_manager_impl.cc

Signed-off-by: Alex Konradi <akonradi@google.com>

antoniovicente

Thanks for implementing this important timeout.

Signed-off-by: Alex Konradi <akonradi@google.com>

antoniovicente · 2020-10-22T20:20:01Z

/retest

repokitteh-read-only · 2020-10-22T20:20:05Z

Retrying Azure Pipelines.

🐱

Caused by: a #13610 (comment) was created by @antoniovicente.

see: more, trace.

…eout Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi · 2020-10-23T20:09:51Z

/retest

repokitteh-read-only · 2020-10-23T20:09:55Z

Retrying Azure Pipelines.
Cannot retry non-completed check: envoy-presubmit (check linux_x64 compile_time_options), please wait.

🐱

Caused by: a #13610 (comment) was created by @akonradi.

see: more, trace.

akonradi · 2020-10-26T14:21:48Z

The clang_tidy check looks to be the same GOAWAY issue seen elsewhere. Would merging master help, or should I just kick off a retest?

akonradi · 2020-10-27T14:54:26Z

/retest

repokitteh-read-only · 2020-10-27T14:54:30Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #13610 (comment) was created by @akonradi.

see: more, trace.

mattklein123

Thanks LGTM with some small comments. Very nice to have this timout.

Is it possible to add a simple integration test of this behavior? I think it should be pretty straightforward.

/wait

mattklein123 · 2020-10-27T16:39:54Z

source/common/network/connection_impl.cc

+
+void ServerConnectionImpl::onTransportSocketConnectTimeout() {
+  closeConnectionImmediately();
+  stream_info_.setConnectionTerminationDetails("transport socket timeout was reached");


nit: can you make this a const string_view somewhere to avoid a probable string_view temporary? Also, I would set the stream info details before closing just for safety in case something looks up the info in the immediate close path.

mattklein123 · 2020-10-27T16:41:59Z

source/common/network/connection_impl.cc

+  case ConnectionEvent::Connected:
+  case ConnectionEvent::RemoteClose:
+  case ConnectionEvent::LocalClose:


Do you envision other events in the future in which we wouldn't do this? Seems like you can just remove the switch and do this always?

I don't know, but this seemed like a good way of 1) demonstrating that this is supposed to handle all cases identically (i.e. I didn't consider one and forget about the others) and 2) ensuring that if someone ever adds an event type, they have to consider how to modify this since it'll cause a compile error otherwise.

OK fair enough. Fine by me.

Signed-off-by: Alex Konradi <akonradi@google.com>

mattklein123

Thanks!

* master: (83 commits) tls: Typesafe tls slots (envoyproxy#13789) docs(example): Correct URL for caching example page (envoyproxy#13810) [fuzz] Made health check fuzz more efficient (envoyproxy#13747) rtds: properly scope rtds stats (envoyproxy#13764) http: fixing a bug with IPv6 hosts (envoyproxy#13798) connection: Remember transport socket read resumption requests and replay them when re-enabling read. (envoyproxy#13772) network: adding some accessors for ALPN work. (envoyproxy#13785) docs: added a step about how to handle platform specific extensions (envoyproxy#13759) Fix identation in ip transparency code snippet (envoyproxy#13743) wasm: enable WAVM's stack unwinding feature (envoyproxy#13792) log: set route name for direct response (envoyproxy#13683) Use nghttp2 as external dependsncy in protocol_constraints_lib (envoyproxy#13763) [Windows] Update windows dev docs (envoyproxy#13741) cel: patch thread safety issue (envoyproxy#13739) Windows: Fix ssl_socket_test (envoyproxy#13264) apple dns: add fake api test suite (envoyproxy#13780) overload: scale selected timers in response to load (envoyproxy#13475) examples: Add dynamic configuration (control plane) sandbox (envoyproxy#13746) Removed exception in getResponseStatus() (envoyproxy#13314) network: add timeout for transport connect (envoyproxy#13610) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

passthrough functionality for "TLS handshake timeout". Envoy has supported this [for a bit](envoyproxy/envoy#13610), and this PR will allow gloo-edge to support the functionality as well. [Referenced Documentation Here](https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/listener/v3/listener_components.proto#config-listener-v3-filterchain) closes solo-io#5438

* feat(ssl): added timeout option passthrough functionality for "TLS handshake timeout". Envoy has supported this [for a bit](envoyproxy/envoy#13610), and this PR will allow gloo-edge to support the functionality as well. [Referenced Documentation Here](https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/listener/v3/listener_components.proto#config-listener-v3-filterchain) closes solo-io#5438

akonradi added 4 commits October 16, 2020 11:51

Configure filter chain transport socket timeout

de4d2f9

Add a config field to the filter chain proto to specify the transport socket timeout and use that to set the value on the server socket. Signed-off-by: Alex Konradi <akonradi@google.com>

Add release notes

5de8f0e

Signed-off-by: Alex Konradi <akonradi@google.com>

repokitteh-read-only bot added the api label Oct 16, 2020

repokitteh-read-only bot assigned antoniovicente Oct 16, 2020

Fix spelling

c5c64c8

Signed-off-by: Alex Konradi <akonradi@google.com>

repokitteh-read-only bot removed the api label Oct 16, 2020

antoniovicente reviewed Oct 16, 2020

View reviewed changes

Address review feedback

aad8bee

Signed-off-by: Alex Konradi <akonradi@google.com>

antoniovicente reviewed Oct 19, 2020

View reviewed changes

akonradi added 2 commits October 19, 2020 16:07

Address feedback

7e7a2fc

Signed-off-by: Alex Konradi <akonradi@google.com>

Fix build

2498d53

Signed-off-by: Alex Konradi <akonradi@google.com>

antoniovicente reviewed Oct 20, 2020

View reviewed changes

Address feedback

78fcb30

Signed-off-by: Alex Konradi <akonradi@google.com>

antoniovicente previously approved these changes Oct 21, 2020

View reviewed changes

Add references to Transport Socket section

a8daa82

Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi dismissed antoniovicente’s stale review via a8daa82 October 22, 2020 16:51

antoniovicente previously approved these changes Oct 22, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into ssl-handshake-tim…

6b84941

…eout Signed-off-by: Alex Konradi <akonradi@google.com>

mattklein123 self-assigned this Oct 24, 2020

mattklein123 requested changes Oct 27, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Oct 27, 2020

Address feedback

902d92e

Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi dismissed antoniovicente’s stale review via 902d92e October 27, 2020 19:35

repokitteh-read-only bot removed the waiting label Oct 27, 2020

mattklein123 approved these changes Oct 27, 2020

View reviewed changes

mattklein123 merged commit 2d0a2f6 into envoyproxy:master Oct 27, 2020

akonradi mentioned this pull request Dec 16, 2020

[tls] timeout for TLS handshakes #11426

Closed

bcollard mentioned this pull request Oct 6, 2021

make TLS handshake timeout configurable kgateway-dev/kgateway#5438

Closed

gunnar-solo mentioned this pull request Oct 15, 2021

feat(ssl): added timeout option kgateway-dev/kgateway#5475

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

network: add timeout for transport connect #13610

network: add timeout for transport connect #13610

akonradi commented Oct 16, 2020

repokitteh-read-only bot commented Oct 16, 2020

akonradi commented Oct 16, 2020

htuch commented Oct 16, 2020

antoniovicente left a comment

antoniovicente Oct 19, 2020

akonradi Oct 19, 2020

antoniovicente Oct 20, 2020

akonradi Oct 21, 2020

antoniovicente Oct 21, 2020

antoniovicente Oct 20, 2020

antoniovicente left a comment

antoniovicente commented Oct 22, 2020

repokitteh-read-only bot commented Oct 22, 2020

akonradi commented Oct 23, 2020

repokitteh-read-only bot commented Oct 23, 2020

akonradi commented Oct 26, 2020

akonradi commented Oct 27, 2020

repokitteh-read-only bot commented Oct 27, 2020

mattklein123 left a comment

mattklein123 Oct 27, 2020

akonradi Oct 27, 2020

mattklein123 Oct 27, 2020

akonradi Oct 27, 2020

mattklein123 Oct 27, 2020

mattklein123 left a comment

network: add timeout for transport connect #13610

network: add timeout for transport connect #13610

Conversation

akonradi commented Oct 16, 2020

repokitteh-read-only bot commented Oct 16, 2020

akonradi commented Oct 16, 2020

htuch commented Oct 16, 2020

antoniovicente left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoniovicente left a comment

Choose a reason for hiding this comment

antoniovicente commented Oct 22, 2020

repokitteh-read-only bot commented Oct 22, 2020

akonradi commented Oct 23, 2020

repokitteh-read-only bot commented Oct 23, 2020

akonradi commented Oct 26, 2020

akonradi commented Oct 27, 2020

repokitteh-read-only bot commented Oct 27, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment