transport socket: api and implementation for startTls transport socket by cpakulski · Pull Request #13112 · envoyproxy/envoy

cpakulski · 2020-09-15T20:01:16Z

Commit Message:
PR defines configuration API and implementation for StartTls transport socket. StartTls is programmable transport socket which starts in clear-text but may programatically be switched to TLS.

Original discussion about use cases is here: #9577

Design document is here: https://docs.google.com/document/d/1ajrQpOEuup04V95xDDY8hSigW4B-J3iuhwn8QQEl-q4/edit?usp=sharing

Risk Level: Low.
Testing: Yes. Added integration tests.

Docs Changes: Some doc sections are automatically produced, but they contain minimum info.

Release Notes: No.

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

repokitteh-read-only · 2020-09-15T20:01:22Z

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: #13112 was opened by cpakulski.

see: more, trace.

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

mattklein123

Thanks, at a high level this LGTM. This is small enough that I'm not sure it's worth merging as is. Do you want to provide the implementation (even if without tests) and we can review the whole thing together?

/wait

cpakulski · 2020-09-16T14:54:43Z

Thanks for reviewing. It makes total sense to add socket implementation to this PR.

stale · 2020-10-04T20:16:17Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

cpakulski · 2020-10-07T22:28:02Z

WIP.

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Consolidated fixture classes. Renamed data members to indicate if they are used by cleartext or tls. Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Changed passthrough_ to oper_socket_. Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

cpakulski · 2020-11-04T15:24:28Z

Originally this PR contained only api definition. Since then I added starttls socket implementation plus integration test.

The integration test simulates the intended usage of the starttls transport socket. A client opens a clear-text connection and as protocol exchange progresses, a filter understanding the protocol may instruct the transport socket to start using tls. Switching from clear-text to tls must happen without closing the socket: tls handshake starts over the same socket where clear-text exchange happened.

Testing/coverage should still be expanded, but I believe that there is enough content right now to see if this is going in the right direction.

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

lizan

Sorry for the delay, in general this is in good structure, starting from high level comments.

lizan · 2020-12-02T11:53:35Z

+  /**
+   * @return std::string type of the underlying transport socket.
+   */
+  virtual std::string transportProtocol() const PURE;


The only meaningful use of this one is to check whether transport socket can startSecureTransport, but I don't think you really need as you can call startSecureTransport and check its return value?

You are probably right. I was originally thinking that a filter could check underlying transport socket when it receives a configuration and could verify that it sits on top of starttls transport socket. But it has to wait until connection is formed and only then can check if the stack contains starttls transport socket. Therefore, as you suggest, it can just try to call startSecureTransport and complain when it returns false.

Yeah if you want to reject at configuration level this should be part of transport socket factory but not connection.

lizan · 2020-12-04T22:33:20Z

+  /**
+   * @return std::string type of the underlying transport socket.
+   */
+  virtual std::string transportProtocol() const PURE;


Yeah if you want to reject at configuration level this should be part of transport socket factory but not connection.

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

lizan

LGTM, @mattklein123 can you take a look?

mattklein123

Thanks generally LGTM. Flushing out a few comments. Thank you!

/wait

mattklein123 · 2020-12-10T23:56:05Z

+  // Socket used in all transport socket operations.
+  // initially it is set to use raw buffer socket but
+  // can be converted to use ssl.
+  Network::TransportSocketPtr oper_socket_;


nit: I don't know what oper_socket means. Can you name this something a bit better?

Renamed to active_socket_

I imagined oper_ to mean operating

mattklein123 · 2020-12-10T23:57:12Z

+  if (!using_ssl_) {
+    ssl_socket_->setTransportSocketCallbacks(*callbacks_);
+    ssl_socket_->onConnected();
+    oper_socket_ = std::move(ssl_socket_);


Can transport sockets internally buffer? Is this guaranteed to be correct? Can we drop buffered data if there is flow control?

I think that in this case we are safe. RawBuffer::doWrite/doRead do not buffer.
In general case, where any type of transport socket could be plugged in, it would be nice to have ::flush() method to call before switching.

Please add a TODO/comment about this.

Added TODO.

mattklein123 · 2020-12-10T23:58:06Z

  const std::string Tap = "envoy.transport_sockets.tap";
  const std::string Tls = "envoy.transport_sockets.tls";
  const std::string UpstreamProxyProtocol = "envoy.transport_sockets.upstream_proxy_protocol";
+  const std::string StartTls = "envoy.transport_sockets.starttls";


We are trying to get rid of these files so just inline the strings where they are needed. Same below.

What would you say if I leave it as is and create a PR which removes well_known_names.h and inlines strings instead of variables across all files?

OK that's fine.

… sockets except starttls. Renamed data members to better reflect their meaning. Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

mattklein123

Thanks!

antoniovicente · 2020-12-14T21:52:21Z

+        ":starttls_integration_proto_cc_proto",
+        "//source/extensions/filters/network/tcp_proxy:config",
+        "//source/extensions/transport_sockets/raw_buffer:config",
+        "//source/extensions/transport_sockets/starttls:config",


We introduced a new direct dependency between core tests and //source/extensions.
Could we move this test to //test/extensions/transport_sockets ?

Oops sorry I missed this. Yes @cpakulski please move.

No problem - will move it.

Moved the test: #14425

antoniovicente · 2021-02-18T00:44:52Z

  IoResult doRead(Buffer::Instance& buffer) override;
  IoResult doWrite(Buffer::Instance& buffer, bool end_stream) override;
  Ssl::ConnectionInfoConstSharedPtr ssl() const override { return nullptr; }
+  bool startSecureTransport() override { return false; }


Test coverage for this line seems to be flaky. Some runs seem to be hitting it but some aren't.

hit: https://storage.googleapis.com/envoy-pr/2de21bf/coverage/source/common/network/raw_buffer_socket.h.gcov.html
not hit: https://storage.googleapis.com/envoy-pr/48c0687/coverage/source/common/network/raw_buffer_socket.h.gcov.html

Thanks @antoniovicente. I will investigate it.

Config API for StartTls transport socket.

61f604d

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

repokitteh-read-only Bot added the api label Sep 15, 2020

asraa reviewed Sep 15, 2020

View reviewed changes

Comment thread source/extensions/transport_sockets/starttls/BUILD Outdated

asraa assigned mattklein123 Sep 15, 2020

cpakulski added 2 commits September 15, 2020 20:49

Updated CODEOWNERS file.

ba3fb12

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Updated comments in proto file.

36d9db0

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

mattklein123 requested changes Sep 16, 2020

View reviewed changes

repokitteh-read-only Bot added the waiting label Sep 16, 2020

stale Bot added the stale stalebot believes this issue/PR has not been touched recently label Oct 4, 2020

jsbucy mentioned this pull request Oct 6, 2020

STARTTLS support #9577

Closed

stale Bot removed the stale stalebot believes this issue/PR has not been touched recently label Oct 7, 2020

cpakulski added 8 commits November 2, 2020 15:19

Added integration tests.

2516160

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Removed disabled tests.

aecbbf2

Consolidated fixture classes. Renamed data members to indicate if they are used by cleartext or tls. Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Added list of allowed messages to StartTlsSwitchFilter.

dc9de2d

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Corrected format and spelling in integration test file.

686b3d7

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Cleaunp of starttls transport socket code.

e3252d5

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Removed unused connection related methods.

9731870

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Removed unused server_names_ variable.

86154ef

Changed passthrough_ to oper_socket_. Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Code formatting.

4870c9a

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

cpakulski requested review from PiotrSikora, alyssawilk, ggreenway and htuch as code owners November 4, 2020 13:54

repokitteh-read-only Bot removed the waiting label Nov 4, 2020

Merge branch 'master' into starttls/api

c3d6d8d

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Added unit tests to quic to increase coverage.

72282ea

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

lizan suggested changes Dec 2, 2020

View reviewed changes

lizan reviewed Dec 4, 2020

View reviewed changes

cpakulski added 3 commits December 8, 2020 21:53

Merge branch 'master' into starttls/api

2127ba7

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Removed transportProtocol method from connection and transport socket.

d7530b7

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Added release note.

bfbdea8

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

cpakulski changed the title ~~api: config API for StartTls transport socket~~ transport socket: api and implementation for startTls transport socket Dec 8, 2020

cpakulski added 3 commits December 9, 2020 02:37

Trying to fix integration test.

40c9eb6

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Fix for integration test.

403117a

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Merge branch 'master' into starttls/api

b44ebd3

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

mattklein123 added the waiting label Dec 9, 2020

Updated buffer mock to reflect that it is called twice.

2ce8217

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

repokitteh-read-only Bot removed the waiting label Dec 9, 2020

cpakulski requested a review from lizan December 9, 2020 22:15

lizan reviewed Dec 9, 2020

View reviewed changes

mattklein123 requested changes Dec 10, 2020

View reviewed changes

repokitteh-read-only Bot added the waiting label Dec 10, 2020

Changed startSecureTransport method to return false for all transport…

1b9889e

… sockets except starttls. Renamed data members to better reflect their meaning. Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

repokitteh-read-only Bot removed the waiting label Dec 11, 2020

Fixed formatting in tests.

298f70e

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

mattklein123 approved these changes Dec 11, 2020

View reviewed changes

repokitteh-read-only Bot removed the api label Dec 11, 2020

mattklein123 merged commit 98e8bf3 into envoyproxy:master Dec 13, 2020

antoniovicente reviewed Dec 14, 2020

View reviewed changes

cpakulski mentioned this pull request Jan 19, 2021

transport_sockets: removed well_known_names.h file. #14751

Closed

antoniovicente reviewed Feb 18, 2021

View reviewed changes

ceastman-ibm mentioned this pull request Jun 9, 2021

startTLS support in service entry istio/istio#33345

Closed

alyssawilk mentioned this pull request Apr 10, 2023

support for starttls inner socket in UpstreamProxyProtocolSocket #26593

Closed

AnindyaKrDas mentioned this pull request Oct 29, 2025

Envoy tls_inspector: parseClientHello failed (WRONG_VERSION_NUMBER) for PostgreSQL clients using native SSLRequest; Gateway TLS listener rejects connection (no matching filter chain) #41760

Closed

Licht-T mentioned this pull request Mar 23, 2026

mysql_proxy: MySQL SSL termination #44089

Closed

Conversation

cpakulski commented Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only Bot commented Sep 15, 2020

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cpakulski commented Sep 16, 2020

Uh oh!

stale Bot commented Oct 4, 2020

Uh oh!

cpakulski commented Oct 7, 2020

Uh oh!

cpakulski commented Nov 4, 2020

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpakulski Dec 11, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpakulski commented Sep 15, 2020 •

edited

Loading

cpakulski Dec 11, 2020 •

edited

Loading