tls: add functionality to override requested server name in the upstream cluster by vadimeisenbergibm · Pull Request #4973 · envoyproxy/envoy

vadimeisenbergibm · 2018-11-06T13:06:49Z

Description: add functionality to override requested server name in the upstream cluster
Risk Level: medium
Testing: unit tests
Docs Changes: yes
Release Notes: yes
Related to #4076

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

…luster Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

vadimeisenbergibm · 2018-11-06T15:34:33Z

@lizan I have extracted the override server name functionality from the extension PR #4334. Could you please review it? Are the tests I wrote sufficient?

The test on mac fails for some reason, could you please rerun it?

lizan · 2018-11-06T23:40:26Z

include/envoy/network/transport_socket.h

   */
-  virtual TransportSocketPtr createTransportSocket() const PURE;
+  virtual TransportSocketPtr
+  createTransportSocket(absl::optional<std::string> override_server_name) const PURE;


This should be const reference everywhere.

Thinking more, I think we would like some sort of TransportSocketOptions to passed into here, like an interface:

class TransportSocketOptions { public: virtual ~TransportSocketOptions(); virtual absl::optional<string> overrideServerName() const PURE; virtual void hashKey(std::vector<uint8_t>& key) const PURE; } typedef std::shared_ptr<TransportSocketOptions> TransportSocketOptionsSharedPtr;

and pass TransportSocketOptionsSharedPtr around. This will help when we want to have more than override_server_name, and you can put this into FilterState as something called UpstreamTransportSocketOption.

@lizan OK, thank you for the feedback, let me implement it.

lizan · 2018-11-06T23:43:03Z

Thanks @vadimeisenbergibm for extracting this. The structure looks nice though I think it can be more future proof. I re-kicked mac CI as it seems unrelated.

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

lizan · 2018-11-08T00:38:05Z

source/common/ssl/context_impl.h

 class ContextImpl : public virtual Context {
 public:
-  virtual bssl::UniquePtr<SSL> newSsl() const;
+  virtual bssl::UniquePtr<SSL> newSsl(absl::optional<std::string> override_server_name) const;


const ref for parameter

lizan · 2018-11-08T00:38:13Z

source/common/ssl/context_impl.h

                    TimeSource& time_source);

-  bssl::UniquePtr<SSL> newSsl() const override;
+  bssl::UniquePtr<SSL> newSsl(absl::optional<std::string> override_server_name) const override;


const ref for parameter (everywhere)

lizan · 2018-11-08T00:38:38Z

include/envoy/network/transport_socket.h

+class TransportSocketOptions {
+public:
+  virtual ~TransportSocketOptions() {}
+  virtual absl::optional<std::string> overrideServerName() const PURE;


return a const reference for this.

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

lizan

The transport socket part LGTM modulo test.

lizan · 2018-11-08T07:53:30Z

test/common/tcp_proxy/tcp_proxy_test.cc

+        EXPECT_TRUE(transport_socket_options->overrideServerName().has_value());
+        EXPECT_EQ(transport_socket_options->overrideServerName().value(), "www.example.com");
+
+        (void)priority; // suppress unused warning


Just make the parameter unnamed. or UNREFERENCED_PARAMETER macro is for this.

@lizan It seems lambda requires the parameters to be named. I will use UNREFERENCED_PARAMETER.

I don't think so, lambda with unnamed parameters appears many times in the code base.

You are right, I was probably confused by some other error before. Done.

lizan · 2018-11-08T07:58:54Z

source/common/stream_info/forward_requested_server_name.cc

@@ -0,0 +1,10 @@
+#include "common/stream_info/forward_requested_server_name.h"


(cont from #4334 (comment))

I don't think this should be here and named like this. Name it "upstream_server_name" or "upstream_transport_socket_options" (if you put the class here).

The filter is not related directly to the TCP proxy. The filter specifies the intention that the SNI must be forwarded. Other filters, such as TCP proxy, can extract that information and forward the SNI.

The consumer of this is tcp_proxy, or you should make this into network namespace, as stream_info is for both L4/L7, while this is specific to transport socket / connection.

So there are three different names here:

Forward Requested Server Name in the Stream Info, means that there is some requested server name, not necessarrily SNI, maybe requested server name from some other protocol, that must be forwarded to the upstream, not necessarily by tcp_proxy.

"forward" (i.e. propagating downstream server name to upstream) is decided by the filter setting this, this filter state doesn't imply that. Another filter can set this to arbitrary value.

The filter is not related directly to the TCP proxy. The filter specifies the intention that the SNI must be forwarded. Other filters, such as TCP proxy, can extract that information and forward the SNI.

Override Server Name in tcp_proxy, cluster manager etc. it means that the server name of the upstream cluster must be overridden by that value. It does not specify that it is SNI and does not specify where this server name comes from.

Yes, this is right, now it is a struct named TransportSocketOptions.

@lizan So, making that struct into the network namespace, and calling it upstream_server_name will be OK?

Should I make this upstream_transport_socket_options to hold TransportSocketOptions? My intention was to decouple Filter state objects from TransportSocketOptions. We can have different Filter State objects holding basic types as strings and different filters like tcp_proxy can compose TransportSocketOptions differently, according to their logic.

I am OK with both solutions.

Yeah I think upstream_server_name should be OK, then we can keep every filter state small.

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

…rk/upstream_server_name Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

vadimeisenbergibm · 2018-11-08T16:03:53Z

@lizan I have implemented your comments, the tests pass.

lizan

Great, thanks for working on this.

@htuch @zuercher PTAL?

lizan · 2018-11-08T21:35:13Z

include/envoy/network/transport_socket.h

+class TransportSocketOptions {
+public:
+  virtual ~TransportSocketOptions() {}
+  virtual const absl::optional<std::string>& overrideServerName() const PURE;


comment for each method

Rename to serverNameOverride or just simply serverName; otherwise, it sounds like a mutating method vs. an option.

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

required for clang_tidy Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

htuch

LGTM modulo remaining comments.

htuch · 2018-11-20T17:28:42Z

source/common/network/upstream_server_name.cc

+
+absl::string_view UpstreamServerName::key() {
+  // Construct On First Use Idiom: https://isocpp.org/wiki/faq/ctors#static-init-order-on-first-use
+  static const char* cstring_key = "envoy.network.upstream_server_name";


You can use the existing macro for this,

envoy/source/common/common/version.cc

Line 13 in 1570696

CONSTRUCT_ON_FIRST_USE(std::string, build_scm_revision);

, here.

Done in b31e6f6

htuch · 2018-11-20T17:29:14Z

source/common/common/scalar_to_byte_vector.h

+template <typename T> void pushScalarToByteVector(T val, std::vector<uint8_t>& bytes) {
+  uint8_t* byte_ptr = reinterpret_cast<uint8_t*>(&val);
+  for (unsigned int byte_index = 0; byte_index < sizeof val; byte_index++) {
+    bytes.push_back(*byte_ptr);


Nit: collapse two lines to bytes.push_back(*byte_ptr++);

Done in d84b467

htuch · 2018-11-20T17:29:40Z

source/common/common/scalar_to_byte_vector.h

+namespace Envoy {
+template <typename T> void pushScalarToByteVector(T val, std::vector<uint8_t>& bytes) {
+  uint8_t* byte_ptr = reinterpret_cast<uint8_t*>(&val);
+  for (unsigned int byte_index = 0; byte_index < sizeof val; byte_index++) {


Nit: Envoy style prefers fixed type sizes, e.g. uint32_t here.

Done in d6583b9

htuch · 2018-11-20T17:42:07Z

source/common/ssl/context_impl.h

 public:
-  virtual bssl::UniquePtr<SSL> newSsl() const;
+  virtual bssl::UniquePtr<SSL>
+  newSsl(const absl::optional<std::string>& override_server_name) const;


Nit: making this argument an absl::optional<absl::string_view> (without reference) might be strictly more flexible.

Done in 505cce2

source/common/network/upstream_server_name.h

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

This reverts commit 6c9128d. Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

vadimeisenbergibm · 2018-11-21T03:01:33Z

@htuch I have applied your comments, all the tests pass.

htuch

LGTM, one last test fix and we can merge, thanks.

htuch · 2018-11-21T16:15:00Z

test/common/upstream/cluster_manager_impl_test.cc

+  Network::MockActiveDnsQuery active_dns_query;
+  EXPECT_CALL(*dns_resolver, resolve(_, _, _))
+      .WillRepeatedly(DoAll(SaveArg<2>(&dns_callback), Return(&active_dns_query)));
+  create(parseBootstrapFromJson(json));


Can you switch this to v2 YAML config? v1 is deprecated and will be deleted real soon now.

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

htuch

Thanks! Appreciate your patience while working through the final nits, I think this PR looks great.

vadimeisenbergibm · 2018-11-21T17:46:51Z

@htuch My pleasure! Thank you for your patient reviewing.

vadimeisenbergibm · 2018-11-21T18:24:09Z

@lizan Could you please approve this PR? @htuch has approved it.

I think the earlier review indicated this was fine modulo test, and these LGTM.

htuch · 2018-11-21T22:13:48Z

@lizan if you have any further feedback, I'm thinking this will only be test related, so we can punt that to a further PR.

* no need to submit git to google cloud; document how to invoke cloud build on local state * update latest envoy that * fix envoy compilation. see envoyproxy/envoy#4973

…sted server name, in HTTP filters similar to envoyproxy#4973 Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

…eam cluster (envoyproxy#4973) Add functionality to override requested server name in the upstream cluster Risk Level: medium Testing: unit tests Docs Changes: yes Release Notes: yes Related to envoyproxy#4076 Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com> Signed-off-by: Fred Douglas <fredlas@google.com>

add ForwardRequestedServerName FilterState Object

2fa0949

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

vadimeisenbergibm mentioned this pull request Nov 6, 2018

Extensions: Network filter to forward SNI from the downstream TLS connection to the upstream TLS connection #4334

Closed

add functionality to override requested server name in the upstream c…

9701ce6

…luster Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

vadimeisenbergibm requested review from htuch and zuercher as code owners November 6, 2018 14:29

lizan self-assigned this Nov 6, 2018

lizan reviewed Nov 6, 2018

View reviewed changes

vadimeisenbergibm added 3 commits November 7, 2018 23:58

replace override_server_name with TransportSocketOptions

ab66f42

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

fix compilation errors in tests

5fe6e3e

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

fix format

b6f32d7

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

lizan reviewed Nov 8, 2018

View reviewed changes

vadimeisenbergibm added 4 commits November 8, 2018 07:24

fix compilation errors

6aff2ea

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

make overrideServerName() return const&

0b2a296

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

make the parameter of newSSL() const&

6c9128d

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

fix a compilation error

dedd511

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

lizan previously requested changes Nov 8, 2018

View reviewed changes

vadimeisenbergibm added 9 commits November 8, 2018 10:42

fixed missed refactoring

27557de

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

"refactor" a comment

b019a99

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

fix a typo

09af682

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

move stream_info/forward_requested_server_name to source/common/netwo…

8ff3b25

…rk/upstream_server_name Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

StreamInfo::ForwardRequestedServerName -> Network::UpstreamServerName

cfbb753

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

append missing StreamInfo namespace

566570f

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

remove a leftover from the PR this PR was extracted from

173849b

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

fix format

41cf2c7

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

remove names of the unused parameters

995d15e

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

lizan reviewed Nov 8, 2018

View reviewed changes

add comments

c8afa18

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

vadimeisenbergibm added 2 commits November 20, 2018 06:19

uint -> unsigned int

7109894

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

add missing includes

b8f0d3c

required for clang_tidy Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

vadimeisenbergibm mentioned this pull request Nov 20, 2018

TLS origination doesn't seem to work with wildcard domains istio/istio#10041

Closed

vadimeisenbergibm added 3 commits November 20, 2018 11:20

fix static initialization problem

dddd8e3

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

fix format

2532513

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

PerConnectionCluster::Key -> PerConnectionCluster::key() in tests

588fa98

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

htuch suggested changes Nov 20, 2018

View reviewed changes

vadimeisenbergibm added 6 commits November 20, 2018 22:29

add TODO replace long parameter lists with options objects

2f23141

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

use CONSTRUCT_ON_FIRST_USE macro

b31e6f6

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

combine pointer dereferencing and increment into one line

d84b467

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

unsigned int -> uint32_t

d6583b9

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

Revert "make the parameter of newSSL() const&"

505cce2

This reverts commit 6c9128d. Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

Merge branch 'master' into override_server_name

58efa36

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

htuch reviewed Nov 21, 2018

View reviewed changes

replace v1 json with v2 yaml in DynamicHostRemoveWithTls test

7080014

Signed-off-by: Vadim Eisenberg <vadime@il.ibm.com>

htuch approved these changes Nov 21, 2018

View reviewed changes

htuch merged commit 48b161e into envoyproxy:master Nov 21, 2018

lizan mentioned this pull request Dec 13, 2018

http: add ip src transparency #5255

Closed

vadimeisenbergibm mentioned this pull request Dec 23, 2018

tls: Refactor the ssl socket test #5402

Merged

vadimeisenbergibm mentioned this pull request Dec 25, 2018

add support for passing TransportSocketOptions in HTTP filters #5415

Closed

vadimeisenbergibm mentioned this pull request Jan 21, 2019

[filters] original source listener filter beginnings #5337

Merged

		@@ -0,0 +1,10 @@
		#include "common/stream_info/forward_requested_server_name.h"

Conversation

vadimeisenbergibm commented Nov 6, 2018

Uh oh!

vadimeisenbergibm commented Nov 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lizan commented Nov 6, 2018

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!

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!

lizan Nov 8, 2018 • 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!

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!

vadimeisenbergibm commented Nov 8, 2018

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!

htuch 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!

vadimeisenbergibm Nov 21, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadimeisenbergibm Nov 21, 2018 • 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!

vadimeisenbergibm commented Nov 21, 2018

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

lizan Nov 8, 2018 •

edited

Loading

vadimeisenbergibm Nov 21, 2018 •

edited

Loading

vadimeisenbergibm Nov 21, 2018 •

edited

Loading