rbac: add support for SNI based permissions by venilnoronha · Pull Request #4662 · envoyproxy/envoy

venilnoronha · 2018-10-09T22:33:57Z

Description: This PR adds support for SNI based permissions by matching over a
connection's requested server name.
Risk Level: Low
Testing: Added 4 additional tests
Docs Changes: N/A
Release Notes: Added a note about this feature
Fixes #4515

rshriram · 2018-10-09T22:38:58Z

shouldn't this be a reference as well?

rshriram

Thanks for the quick turn around. We need exact/prefix/regex matches for the SNI, similar to the string match constructs supported by the header matchers in rbac.

rshriram · 2018-10-09T22:42:59Z

is this a string equals function? For SNI specifically, we need prefix matches at lest. (like *.example.com) that matches foo.example.com and so on.. Didn't RBAC have some exact/prefix/regex match construct for strings/

mattklein123 · 2018-10-09T23:26:23Z

Can we potentially call this "requested_server_name_indication" to be super clear about what this is (or similar)? (With more detail in the comment about TLS, etc.)? WDYT?

How about requested_sni_server_name?

how about just SNI instead of all the workarounds?

End users understand SNI much more than "requested server name". And in this PR, this requested server name is spilling into end user config, where we are better of using commonly known terms than the "technically" right term.

I think we made name change from SNI to server names in FilterChainMatch, so it is not TLS specific? If here we're going to match same thing it shouldn't refer SNI directly. ref: #3454
cc @ggreenway @PiotrSikora

That's correct. It was named this way so that it could also apply to HTTP Host header, or any other similar field in another protocol.

Eh? RBAC already has dedicated fields for HTTP host. The fact that this specific thing applies to the SNI value presented in the ClientHello message means we need to be very explicit about what server name this refers to. If consistency is needed, then we need to stick with requested_Server_name. But as @mattklein123 points out, its still confusing, because there are one too many server names :).
How about tls_server_name ? Very little scope for confusion. I really don't want to give people the impression that this could also be a HTTP host header, as it receives separate treatment in RBAC.

If we keep requested_server_name, my ask is that we make the documentation much more explicit. Basically call out that this is typically set via TLS SNI, and then explain how it might be set some other way (Listener filter, other?). Thank you.

I don't think it could apply to HTTP Host header, but definitely similar field in another protocol. Any listener filter extension may set that. TLS inspector is the only one setting that here, but we may have other. +1 to https://github.com/envoyproxy/envoy/pull/4662/files#r224160788 make documentation explicit.

Addressed in 92e54f4.

ggreenway · 2018-10-10T16:58:17Z

If the expectation is that this only applies to SNI, then this code could do the wrong thing, depending on your configured filter chain:

If you don't explicitly add the tls_inspector filter, and you don't have any FilterChainMatch matching on servername, this field will be empty

A different listener filter could set this to something other than the SNI value

This could be a non-TLS connection, in which case there is no SNI value.

Given all this, I think you should document exactly what this is filtering on. Link to the docs on how requestedServerName() is set, give an example for how to set this up for TLS, and note that some filter chain configurations could lead to different results.

Addressed in 92e54f4.

lizan · 2018-10-10T17:15:58Z

I don't think it could apply to HTTP Host header, but definitely similar field in another protocol. Any listener filter extension may set that. TLS inspector is the only one setting that here, but we may have other. +1 to https://github.com/envoyproxy/envoy/pull/4662/files#r224160788 make documentation explicit.

This commit adds support for SNI based permissions by matching over a connection's requested server name. Signed-off-by: Venil Noronha <veniln@vmware.com>

This commit replaces the existing string based SNI field with StringMatcher which provides much more flexibility for matching patterns. Signed-off-by: Venil Noronha <veniln@vmware.com>

lizan · 2018-10-11T18:35:31Z

 #include "common/http/header_utility.h"
 #include "common/network/cidr_range.h"

+#include "absl/strings/str_replace.h"


venilnoronha · 2018-10-11T18:53:55Z

Docs are on their way...

This commit adds notes about how the requested_server_name field behaves in different situations. Signed-off-by: Venil Noronha <veniln@vmware.com>

rshriram · 2018-10-11T20:21:12Z

+                                         const Envoy::Http::HeaderMap&,
+                                         const envoy::api::v2::core::Metadata&) const {
+  absl::string_view requested_server_name = connection.requestedServerName();
+  if (!requested_server_name.empty()) { // test for null string_view


is this needed? isn't "" a string?

A couple of tests fail if this isn't in place.

That's because you only passing the data() to StringMatcher::match, but this is still wrong. string_view doesn't have to be null terminated, so the string you passed in to Matcher could be longer than the string_view.

Can you change StringMatcher::match to take absl::string_view instead? That would avoid a copy too.

This commit adds tests cases to verify the RBAC filter behavior when the configured and a connection's requested server name is non/empty. Signed-off-by: Venil Noronha <veniln@vmware.com>

lizan · 2018-10-11T21:03:27Z

+                                         const Envoy::Http::HeaderMap&,
+                                         const envoy::api::v2::core::Metadata&) const {
+  absl::string_view requested_server_name = connection.requestedServerName();
+  if (!requested_server_name.empty()) { // test for null string_view


That's because you only passing the data() to StringMatcher::match, but this is still wrong. string_view doesn't have to be null terminated, so the string you passed in to Matcher could be longer than the string_view.

Can you change StringMatcher::match to take absl::string_view instead? That would avoid a copy too.

This commit updates the StringMatcher implementation to take absl::string_view inputs in place of the previous std::string& input for no-copy matching. Signed-off-by: Venil Noronha <veniln@vmware.com>

venilnoronha · 2018-10-11T21:27:06Z

@lizan I've addressed the StringMatcher::match change in cce67a7.

mattklein123

LGTM. Mostly comments on comments. @ggreenway can you also take another pass?

mattklein123 · 2018-10-11T23:25:40Z

+    //   * A listener filter may overwrite a connection's requested server name
+    //     within Envoy.
+    //
+    // Please refer to `this FAQ <https://www.envoyproxy.io/docs/envoy/latest/faq/sni>`_


Instead of using an absolute URL here can you ref link to the faq page?

Addressed all comments in 0e2ad38.

mattklein123 · 2018-10-11T23:26:19Z

    // match, this permission would match.
    Permission not_rule = 8;
+
+    // The requested TLS SNI server name from the client's connection request.


Per below this is not necessarily TLS SNI. Should it be something like:

"The request server from the client's connection request. This is typically TLS SNI." (Or something like that?)

mattklein123 · 2018-10-11T23:26:31Z

+    //     a TLS connection's requested SNI server name will be treated as if it
+    //     wasn't present.
+    //
+    //   * A listener filter may overwrite a connection's requested server name


Link to listener filter arch docs?

mattklein123 · 2018-10-11T23:26:58Z

 * http: no longer adding whitespace when appending X-Forwarded-For headers. **Warning**: this is not
  compatible with 1.7.0 builds prior to `9d3a4eb4ac44be9f0651fcc7f87ad98c538b01ee <https://github.com/envoyproxy/envoy/pull/3610>`_.
  See `#3611 <https://github.com/envoyproxy/envoy/issues/3611>`_ for details.
+* rbac: added support for :ref:`TLS SNI based <envoy_api_field_config.rbac.v2alpha.Permission.requested_server_name>` permissions.


TLS SNI might be too specific per other comment? Make more general/clear?

mattklein123 · 2018-10-11T23:27:26Z

 };

+/**
+ * Perform a match against the requested TLS SNI server name.


Same comment about specificity.

This commit updates the comments related to the newly introduced requested_server_name configuration to more closely match its behavior. Signed-off-by: Venil Noronha <veniln@vmware.com>

mattklein123

Thank you!

rshriram reviewed Oct 9, 2018

View reviewed changes

Comment thread docs/root/intro/version_history.rst Outdated

rshriram reviewed Oct 9, 2018

View reviewed changes

Comment thread source/extensions/filters/common/rbac/matchers.h Outdated

Copy link
Copy Markdown

Member

rshriram Oct 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be a reference as well?

rshriram reviewed Oct 9, 2018

View reviewed changes

mattklein123 reviewed Oct 9, 2018

View reviewed changes

alyssawilk assigned ggreenway Oct 10, 2018

ggreenway reviewed Oct 10, 2018

View reviewed changes

lizan reviewed Oct 10, 2018

View reviewed changes

rbac: add support for SNI based permissions

74642f0

This commit adds support for SNI based permissions by matching over a connection's requested server name. Signed-off-by: Venil Noronha <veniln@vmware.com>

venilnoronha force-pushed the rbac-sni branch from 527170f to 74642f0 Compare October 10, 2018 22:56

Use StringMatcher for configuring the SNI field

7f2a5ef

This commit replaces the existing string based SNI field with StringMatcher which provides much more flexibility for matching patterns. Signed-off-by: Venil Noronha <veniln@vmware.com>

lizan reviewed Oct 11, 2018

View reviewed changes

Add documentation about configuration behavior

92e54f4

This commit adds notes about how the requested_server_name field behaves in different situations. Signed-off-by: Venil Noronha <veniln@vmware.com>

rshriram reviewed Oct 11, 2018

View reviewed changes

Comment thread test/extensions/filters/common/rbac/matchers_test.cc

rshriram reviewed Oct 11, 2018

View reviewed changes

Comment thread test/extensions/filters/common/rbac/matchers_test.cc

Add tests with non/empty SNI requested server name

4e67b08

This commit adds tests cases to verify the RBAC filter behavior when the configured and a connection's requested server name is non/empty. Signed-off-by: Venil Noronha <veniln@vmware.com>

lizan suggested changes Oct 11, 2018

View reviewed changes

Update StringMatcher to take absl::string_view

cce67a7

This commit updates the StringMatcher implementation to take absl::string_view inputs in place of the previous std::string& input for no-copy matching. Signed-off-by: Venil Noronha <veniln@vmware.com>

lizan previously approved these changes Oct 11, 2018

View reviewed changes

mattklein123 requested changes Oct 11, 2018

View reviewed changes

venilnoronha dismissed lizan’s stale review via 0e2ad38 October 11, 2018 23:45

venilnoronha force-pushed the rbac-sni branch from 0e2ad38 to 9057cc3 Compare October 12, 2018 00:12

Update comments related to config behavior

5fe6e19

This commit updates the comments related to the newly introduced requested_server_name configuration to more closely match its behavior. Signed-off-by: Venil Noronha <veniln@vmware.com>

venilnoronha force-pushed the rbac-sni branch from 9057cc3 to 5fe6e19 Compare October 12, 2018 16:40

mattklein123 approved these changes Oct 12, 2018

View reviewed changes

mattklein123 merged commit 811ee0d into envoyproxy:master Oct 12, 2018

lizan mentioned this pull request Oct 13, 2018

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

Closed

Conversation

venilnoronha commented Oct 9, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rshriram left a comment

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

venilnoronha commented Oct 11, 2018

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

venilnoronha commented Oct 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

venilnoronha commented Oct 11, 2018 •

edited

Loading