ssl: add support for SNI. by PiotrSikora · Pull Request #1984 · envoyproxy/envoy

PiotrSikora · 2017-11-01T21:19:40Z

This version serves different TLS certificates based on SNI,
but it doesn't select alternative filter chains.

Partially fixes #95.

This version serves different TLS certificates based on SNI, but it doesn't select alternative filter chains. Partially fixes envoyproxy#95. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora · 2017-11-01T21:20:11Z

Tests to follow...

cc @mattklein123 @rshriram

rshriram · 2017-11-01T23:41:58Z

yay!

rshriram · 2017-11-01T23:56:57Z

source/common/ssl/context_manager_impl.cc

+    if (pos > 0) {
+      size_t rpos = server_name.rfind('.');
+      if (rpos > pos + 1 && rpos != server_name.size() - 1) {
+        std::string wildcard = '*' + server_name.substr(pos);


nit: any reason to not use the wildcard matching algorithm that we have for virtual hosts?

+1 to reusing the matching code in config_impl. Can we factor that out into a utility?

Which code do you mean, exactly? ContextImpl::dNSNameMatch?

source/common/router/config_impl.h, RouteMatcher does similar work. See "domains" in https://www.envoyproxy.io/envoy/configuration/http_conn_man/route_config/vhost.html#config-http-conn-man-route-table-vhost. Without a strong argument against, I think it makes sense for the matching semantics to be the same between this code and that.

I'd +1 as well for the reason that we might change the implementation to be smarter one day, e.g. trie-based as per @tschroed 's earlier implementation. Probably not needed right now, but just as future proofing.

rshriram · 2017-11-01T23:59:37Z

source/common/ssl/context_manager_impl.cc

+    }
+  }
+
+  return map_exact_[listener_name][EMPTY_STRING];


this function is never going to return nullptr right? But the comment says it might return nullptr if not found. Is this doing the fall back thing that we discussed a few days ago?

It can, e.g. if all available filter chains have sni_domains configured, but none of them match.

I guess my confusion lies in the assumption that all entries of map_exact_ are non null, which does not seem to be the case.

mattklein123

Thank you @PiotrSikora! Extremely awesome to see this about to land. Few comments to get started.

mattklein123 · 2017-11-02T16:39:04Z

source/common/ssl/context_impl.cc

+  size_t len;
+
+  if (SSL_early_callback_ctx_extension_get(client_hello, TLSEXT_TYPE_server_name, &data, &len)) {
+    /* Based on BoringSSL's ext_sni_parse_clienthello(). Match on empty SNI if we encounter any


nit: // style comments

mattklein123 · 2017-11-02T16:39:49Z

source/common/ssl/context_impl.cc

+}
+
+void ServerContextImpl::updateConnection(SSL* ssl) {
+  RELEASE_ASSERT(ctx_);


nit: I would just do ASSERT here.

mattklein123 · 2017-11-02T16:40:24Z

source/common/ssl/context_manager_impl.cc

+    if (pos > 0) {
+      size_t rpos = server_name.rfind('.');
+      if (rpos > pos + 1 && rpos != server_name.size() - 1) {
+        std::string wildcard = '*' + server_name.substr(pos);


+1 to reusing the matching code in config_impl. Can we factor that out into a utility?

mattklein123 · 2017-11-02T16:47:34Z

source/common/ssl/context_manager_impl.cc


+ServerContext* ContextManagerImpl::findSslServerContext(const std::string& listener_name,
+                                                        const std::string& server_name) {
+  std::unique_lock<std::mutex> lock(contexts_lock_);


It's very non-optimal that we are acquiring a lock here on every handshake, especially regardless of whether SNI is being used or not. In the interest of moving this along, I'm OK living with this for now. At minimum please put in a TODO. The quick fix is that we can make this a shared mutex and only acquire read access when we do a find. We are going to move to C++14 soon and that will become available. I would prefer that this be completely lockless within a single listener, and I can think of ways of doing this, but it would be a larger change. If you want to brainstorm on this let's chat offline or we can just leave as part of the TODO.

for the uninitiated, why do we need locks here? is this shared global context across all threads, where the context can be updated by LDS or future SDS updates? Is that the reason for suggesting a r/w lock?

The lock is required because we print the contexts as part of admin output, and contexts come and go with listeners. C++14 support has merged so this should be changed to a shared_mutex (I think C++14 only has the timed one, but it works in not timed mode). We can fix the need for a lock in this flow at all later and leave a TODO.

ggreenway · 2017-11-03T16:18:43Z

source/common/ssl/context_manager_impl.cc

+ServerContext* ContextManagerImpl::findSslServerContext(const std::string& listener_name,
+                                                        const std::string& server_name) {
+  std::unique_lock<std::mutex> lock(contexts_lock_);
+  if (map_exact_[listener_name][server_name] != nullptr) {


In the case that server_name wasn't in the inner map, this inserts it, based on the user-provided SNI value in the handshake. I think you need to use map_exact[listener_name].find(server_name) != end, and assume there are never nullptr's in the map. Similar change needed in other places map_exact_ is used.

ggreenway · 2017-11-03T16:19:49Z

source/common/ssl/context_manager_impl.cc

+      size_t rpos = server_name.rfind('.');
+      if (rpos > pos + 1 && rpos != server_name.size() - 1) {
+        std::string wildcard = '*' + server_name.substr(pos);
+        if (map_wildcard_[listener_name][wildcard] != nullptr) {


Same as above re: inserting into the map

…ora/sni_p1

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

rshriram · 2017-11-07T14:15:30Z

On Tue, Nov 7, 2017 at 4:12 AM Piotr Sikora ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In source/common/ssl/context_manager_impl.cc <#1984 (comment)>: > return context; } +ServerContext* ContextManagerImpl::findSslServerContext(const std::string& listener_name, + const std::string& server_name) { + std::unique_lock<std::mutex> lock(contexts_lock_); + if (map_exact_[listener_name][ server_name] != nullptr) { + return map_exact_[listener_name][server_name]; + } + + // Try to construct and match wildcard domain. + if (server_name.size() >= 5) { + size_t pos = server_name.find('.'); + if (pos > 0) { + size_t rpos = server_name.rfind('.'); + if (rpos > pos + 1 && rpos != server_name.size() - 1) { + std::string wildcard = '*' + server_name.substr(pos); Which code do you mean, exactly? ContextImpl::dNSNameMatch?

Should be in router/config_impl.cc for wild card virtual host match

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1984 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AH0qd124DxZq4wxVVhcdoDz24U35aIuMks5s0B8DgaJpZM4QO1qQ> .

PiotrSikora · 2017-11-07T15:41:51Z

@rshriram RouteMatcher::findWildcardVirtualHost() is quite different from what I do here (longest match vs exact match).

rshriram · 2017-11-07T16:17:58Z

A lengthy and detailed comment would help a lot. Like https://github.com/envoyproxy/envoy/blob/master/source/common/router/config_impl.cc#L661
Especially what is the intended input/how people would setup wildcard certs, and how we match, the semantics, etc.

Found by ASan. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

ggreenway · 2017-11-07T17:26:21Z

source/common/ssl/context_manager_impl.cc

                                                        const std::string& server_name) {
  std::shared_lock<std::shared_timed_mutex> lock(contexts_lock_);
-  if (map_exact_[listener_name][server_name] != nullptr) {
+  if (map_exact_[listener_name].find(server_name) != map_exact_[listener_name].end()) {


This is functionally correct, but it uses 5 hash table lookups when it only needs to do two. Should be:

auto& listener_map = map_exact_[listener_name];
auto server_name_it = listener_map.find(server_name);
if (server_name_it != listener_map.end()) {
return *server_name_it;
}

Same in other places using this map.

ggreenway · 2017-11-07T17:28:24Z

source/server/listener_manager_impl.cc

  }

-  filter_factories_ = parent_.factory_.createFilterFactoryList(filter_chain.filters(), *this);
+  // TODO(PiotrSikora): allow filter chains with mixed use of Session Ticket Keys.


Can you add a comment on why this won't work as is? I thought this would have worked as-is.

Done.

Note that there are few ways to make it work, but I don't want to block this PR on that.

ggreenway · 2017-11-07T17:38:33Z

source/common/ssl/context_manager_impl.cc

+  // Remove mappings.
+  if (server_names.empty()) {
+    if (map_exact_[listener_name][EMPTY_STRING] == context) {
+      map_exact_[listener_name][EMPTY_STRING] = nullptr;


Setting to nullptr doesn't remove from the map. Need one of the variants of erase: http://en.cppreference.com/w/cpp/container/unordered_map/erase

Sigh, I know that... Good catch, fixed, thanks!

mattklein123 · 2017-11-07T18:16:14Z

RouteMatcher::findWildcardVirtualHost() is quite different from what I do here (longest match vs exact match).

@PiotrSikora quick drive by: you might consider breaking the matching stuff out into a separate utility class. I think it would be easier to unit test various matching cases.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

mattklein123

Few small things then ready to 🚢

mattklein123 · 2017-11-16T20:34:30Z

include/envoy/ssl/context_manager.h


  /**
   * Builds a ServerContext from a ServerContextConfig.
+   * The skip_context_update parameter is used for fast-path (avoiding lock & context looup)


typo "looup"

mattklein123 · 2017-11-16T20:39:37Z

source/common/ssl/context_manager_impl.cc

+  std::shared_lock<std::shared_timed_mutex> lock(contexts_lock_);
+
+  // TODO(PiotrSikora): refactor and combine code with RouteMatcher::findVirtualHost().
+  auto listener_map_exact = map_exact_.find(listener_name);


nit: I think most of the vars in this function can be const (including the auto ones).

mattklein123 · 2017-11-16T20:40:14Z

source/common/ssl/context_manager_impl.h

+  std::unordered_map<std::string, std::unordered_map<std::string, ServerContext*>> map_exact_;
+  std::unordered_map<std::string, std::unordered_map<std::string, ServerContext*>> map_wildcard_;
+
+  static bool isWildcardServerName(const std::string& name);


nit: functions go above variables (move to line 48 followed by newline)

mattklein123 · 2017-11-16T20:41:19Z

source/server/listener_manager_impl.cc

+      (config.filter_chains().size() == 1 &&
+       config.filter_chains()[0].filter_chain_match().sni_domains().empty());
+
+  size_t filters_hash = 0;


technically, the hash can be 0. I would use Optional<uint64_t> for this variable.

mattklein123 · 2017-11-16T20:41:57Z

source/server/listener_manager_impl.cc

+      filters_hash = RepeatedPtrUtil::hash(filter_chain.filters());
+      filter_factories_ = parent_.factory_.createFilterFactoryList(filter_chain.filters(), *this);
+    } else if (filters_hash != RepeatedPtrUtil::hash(filter_chain.filters())) {
+      throw EnvoyException(fmt::format("error adding listener '{}': use of different filter chains "


nit: I would say "use of SNI and different filter chains..."

Technically, existing code allows multiple filter chains, as long as they have the same filters, regardless of use of SNI, so I think that existing text is a bit more correct.

OK that's fine.

mattklein123 · 2017-11-16T20:44:16Z

source/server/listener_manager_impl.cc

+  // callback, which is going to be called iff it's set on the initial SSL_CTX, even if it's not
+  // set on the current SSL_CTX that doesn't have any Session Ticket Keys configured.
+  if (has_stk != 0 && has_stk != has_tls) {
+    throw EnvoyException(fmt::format("error adding listener '{}': filter chains with mixed use of "


I might have missed it but I don't see tests for these error cases. Can we add some listener_manager_impl tests that EXPECT_THROW_WITH_MESSAGE on bad configs for here and the not matching filter chains?

(I'm working on those, but I'm on vacations and traveling this week, so I don't have any ETA)

Cool. I'm not sure if it's also worth it to have a real integration test which would exercise the happy path of the listener manager code. It's up to you but definitely would like to get some coverage on the listener manager and the error cases. Thank you for working on this! So close!

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

mattklein123 · 2017-11-21T00:55:00Z

@PiotrSikora also you probably saw but FYI needs a master merge also.

Morriz · 2017-11-23T13:50:01Z

so close! yess! wringing hands

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

mattklein123

LGTM. Small question.

mattklein123 · 2017-11-28T16:09:47Z

test/server/listener_manager_impl_test.cc

Why are those factory local variables needed? AFAICT they are not used?

In order to auto-register envoy.http_connection_manager, otherwise loading of the configuration will fail because of an unknown filter.

I see. I think the issue is not this variable, but just linking against the translation unit that has the config factory in it. (Since that is where static registration happens). I don't think this variable actually does anything. If you leave the dep and remove the variable and include does it pass?

Good catch, linking against it is enough to fix that. Thanks!

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

See [this PR](envoyproxy/envoy#1984)

envoyproxy/envoy#1984

envoyproxy/envoy#1984 Signed-off-by: Jeff Schroeder <jeffschroeder@computer.org>

LuyaoZhong · 2022-06-13T03:11:15Z

@PiotrSikora @mattklein123

Current Envoy does not support selecting certificate based on SNI. Why do we remove this functionality which was done 5 years ago?

I'm working on a feature called TLS bumping, in which scenario there might have multiple certs for different SNI inside one transport socket.

ggreenway · 2022-06-13T17:18:53Z

@LuyaoZhong Envoy does support selecting a certificate based on SNI, it's just done by selecting a filter-chain. No functionality was removed; it was just changed.

LuyaoZhong · 2022-06-14T10:56:17Z

@ggreenway

Got it. But I still would like to support tls-transport-layer cert selecting based on SNI.

Consider TLS bumping, its initial design has been accepted by @mattklein123. We might have a SNI list (*.google.com, *.cnn.com, etc.) for bumping, which means we would like to inspect the traffic to these websites. We will implement on-demand mimic certificates generating during runtime and attaching them to one single transport socket to do TLS handshake, in this case we need to support cert selecting inside one transport socket similar as this PR did.

mattklein123 · 2022-06-14T17:30:24Z

In general there are cases in which the filter chain based selection model does not work well (massive number of certs, etc.). I think it's reasonable to do a custom plugin for cert selection (I thought we already support this extension point but I haven't looked in a while.)

ggreenway · 2022-06-14T18:04:38Z

Agreed that it makes sense, but let's move discussion to the relevant issue, not a PR from 2017.

Description: add EngineBuilder API to filter unroutable families Risk Level: low - opt in API Testing: upstream tests. Docs Changes: updated Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

ssl: add support for SNI.

76709a0

This version serves different TLS certificates based on SNI, but it doesn't select alternative filter chains. Partially fixes envoyproxy#95. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

rshriram reviewed Nov 1, 2017

View reviewed changes

mattklein123 reviewed Nov 2, 2017

View reviewed changes

ggreenway requested changes Nov 3, 2017

View reviewed changes

htuch requested a review from dnoe November 6, 2017 02:33

PiotrSikora mentioned this pull request Nov 6, 2017

router: return 503 when cluster isn't found. #1971

Merged

PiotrSikora added 12 commits November 7, 2017 00:17

Merge commit '908ddde3adb0634c966d1170319b8c1f571dab95' into piotrsik…

d04afe2

…ora/sni_p1

review: skip context update in case of one filter chain.

0f984ce

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: use ASSERT().

47e3df7

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: fix comment.

227589b

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: use shared lock.

75abf26

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: add tests.

0fecd6b

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: add docs.

68541bf

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: unordered_map find instead of direct access.

33142cc

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

reivew: fix format.

ca9eb0b

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: fix non-debug build.

7d1b1af

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: fix non-debug tests.

fc9705f

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: fix typo.

a2f657c

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora added 3 commits November 7, 2017 08:45

review: fix stack-use-after-scope.

0dc9b85

Found by ASan. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: describe algorithm in findSslServerContext().

348e6e8

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: reject configurations with mixed use of Session Ticket Keys.

7ce1bbb

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

ggreenway requested changes Nov 7, 2017

View reviewed changes

ggreenway reviewed Nov 7, 2017

View reviewed changes

reivew: fix format.

6901dc0

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

mattklein123 reviewed Nov 16, 2017

View reviewed changes

PiotrSikora added 4 commits November 20, 2017 13:40

review: fix typo.

f519595

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: constify vars.

a43e74c

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: move function prototype before vars.

5700d0c

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: use Optional<uint64_t>.

57ed156

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora added 2 commits November 27, 2017 17:23

review: add tests for listener manager.

5520293

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

Merge remote-tracking branch 'origin/master' into piotrsikora/sni.

4ac9fdf

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

mattklein123 reviewed Nov 28, 2017

View reviewed changes

review: remove factory variables.

122f450

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

mattklein123 approved these changes Nov 28, 2017

View reviewed changes

mattklein123 merged commit c26b25e into envoyproxy:master Nov 28, 2017

SEJeff added a commit to SEJeff/contour that referenced this pull request Dec 10, 2017

Envoy now supports TLS SNI

5a4ceb8

See [this PR](envoyproxy/envoy#1984)

SEJeff mentioned this pull request Dec 10, 2017

Envoy now supports TLS SNI SEJeff/contour#1

Closed

SEJeff added a commit to SEJeff/contour that referenced this pull request Dec 10, 2017

Envoy now supports SNI with TLS

11add2e

envoyproxy/envoy#1984

SEJeff mentioned this pull request Dec 10, 2017

Envoy now supports SNI with TLS projectcontour/contour#92

Merged

SEJeff added a commit to SEJeff/contour that referenced this pull request Dec 10, 2017

Envoy now supports SNI with TLS

2a591cf

envoyproxy/envoy#1984 Signed-off-by: Jeff Schroeder <jeffschroeder@computer.org>

ggreenway mentioned this pull request Jun 14, 2022

TLS bumping: decrypting communications between internal and external services #18928

Open

LuyaoZhong mentioned this pull request Jun 16, 2022

SNI-based cert selection in TLS transport socket #21739

Closed

Conversation

PiotrSikora commented Nov 1, 2017

Uh oh!

PiotrSikora commented Nov 1, 2017

Uh oh!

rshriram commented Nov 1, 2017

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!

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!

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!

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!

rshriram commented Nov 7, 2017 via email

Uh oh!

PiotrSikora commented Nov 7, 2017

Uh oh!

rshriram commented Nov 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PiotrSikora Nov 7, 2017 • 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!

mattklein123 commented Nov 7, 2017

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

PiotrSikora Nov 7, 2017 •

edited

Loading