ssl: support wildcard matching for verify_subject_alt_name by crazytan · Pull Request #1298 · envoyproxy/envoy

crazytan · 2017-07-20T21:23:37Z

As discussed in #1272 , I made changes so that an optional character * is allowed at the end of string in verify_subject_alt_name option. For example, spiffe://foo.com/* will match all names starting with spiffe://foo.com/. The wildcard is only allowed in the end of path, not host.

mattklein123

Beyond doc/format failures, small perf comments. @myidpt could you take a quick look?

mattklein123 · 2017-07-21T00:23:59Z

source/common/ssl/context_impl.cc

perf nit: If you are going to call strlen() you might as well just walk both strings at the same time char by char. Also, probably don't need to do the direct compare first and then do this (could just do direct compare in same code path probably) but that's not as big of a deal to me.

agree that the code is not very efficient. Will rewrite so that it only takes one walk through both strings.

mattklein123 · 2017-07-21T18:40:04Z

source/common/ssl/context_impl.cc

I don't think you can always safely dereference *(p - 1). Please add test for this while fixing.

PiotrSikora · 2017-07-24T17:26:51Z

source/common/ssl/context_impl.cc

This whole function is low-level C instead of C++11.

Please consider using std::string::back() and std::string::compare(0, uriPattern.length() - 1, uri)

Yes that would be much better. :)

I'm confused but std::string::compare is what I was using. So should I use pointer walk through or string::compare?

You should use std::string::back() - O(1), std::string::length() - O(1) and std::string::compare() - O(n).

Previously, you were using strlen() - O(n) and std::string::compare() - O(n), i.e. walking the same string twice, which resulted in suboptimal performance, although the difference is probably negligible.

got it. Since my original strlen() was unnecessary, I'll change back to string::compare() to match c++11 style so that future change will be easier.

mattklein123 · 2017-07-24T22:58:19Z

@crazytan looks like you fixed the bug. Anyway, this impl is OK with me, but also happy to have it redone the way @PiotrSikora mentioned. I will let the two of you sort it out.

mattklein123 · 2017-07-25T15:17:06Z

source/common/ssl/context_impl.cc

+bool ContextImpl::uriMatch(const std::string& uriPattern, const char* uri) {
+  size_t pattern_len = uriPattern.length();
+  if (pattern_len > 1 && uriPattern[pattern_len - 1] == '*' && uriPattern[pattern_len - 2] == '/') {
+    return uriPattern.compare(0, pattern_len - 1, uri, pattern_len - 1) == 0;


I think you will need to use strncmp() here since IIRC compare will go off end of string. (See ASAN failure).

That's because the wrong method is used here. It should be:

uriPattern.compare(0, pattern_len - 1, uri)

for NULL-terminated string, instead of:

uriPattern.compare(0, pattern_len - 1, uri, pattern_len - 1)

which specifies length of the uri string.

the problem here is, if uriPattern="foo.bar/*" and uri="foo.bar/baz", uriPattern.compare(0, pattern_len - 1, uri) would give negative result -3. And this cannot be distinguished from the case as uriPattern="foo.aar" and uri="foo.bar", for which compare gives result -1. So I'll have to use strncmp() or pointer walk but again it's not C++11 style.

from doc:

value <0: Either the value of the first character that does not match is lower in the compared string, or all compared characters match but the compared string is shorter.

Sigh, indeed. What a terrible API.

I'll use strncmp() then.

PiotrSikora · 2017-07-25T16:04:45Z

test/common/ssl/context_impl_test.cc

+TEST_F(SslContextImplTest, TestURIMatch) {
+  EXPECT_TRUE(ContextImpl::uriMatch("spiffe://lyft.com/foo", "spiffe://lyft.com/foo"));
+  EXPECT_TRUE(ContextImpl::uriMatch("spiffe://lyft.com/*", "spiffe://lyft.com/foo"));
+  EXPECT_TRUE(ContextImpl::uriMatch("spiffe://lyft.com/*", "spiffe://lyft.com/"));


I'm not sure if this should pass (logically, not according to the code).

why not? ;) the first case fall back to uriPattern == uri. For the rest two uri is compared as a buffer and the prefixes all agree.

I only meant the last one, i.e. spiffe://lyft.com/ matches spiffe://lyft.com/*, which doesn't feel right.

I see... I'll make this a false case

PiotrSikora · 2017-07-25T16:07:23Z

test/common/ssl/context_impl_test.cc

+  EXPECT_TRUE(ContextImpl::uriMatch("spiffe://lyft.com/foo", "spiffe://lyft.com/foo"));
+  EXPECT_TRUE(ContextImpl::uriMatch("spiffe://lyft.com/*", "spiffe://lyft.com/foo"));
+  EXPECT_TRUE(ContextImpl::uriMatch("spiffe://lyft.com/*", "spiffe://lyft.com/"));
+  EXPECT_TRUE(ContextImpl::uriMatch("spiffe://lyft.com/foo/*", "spiffe://lyft.com/foo/bar"));


Could you also add test for:

EXPECT_TRUE(ContextImpl::uriMatch("spiffe://lyft.com/*", "spiffe://lyft.com/foo/bar"));

PiotrSikora · 2017-07-25T16:12:54Z

source/common/ssl/context_impl.cc


+bool ContextImpl::uriMatch(const std::string& uriPattern, const char* uri) {
+  size_t pattern_len = uriPattern.length();
+  if (pattern_len > 1 && uriPattern[pattern_len - 1] == '*' && uriPattern[pattern_len - 2] == '/') {


Perhaps

if (pattern_len > 1 && uriPattern.substr(pattern_len - 2) == '/*') {

would be more readable?

PiotrSikora · 2017-07-25T18:20:05Z

docs/configuration/cluster_manager/cluster_ssl.rst

s/in the end/at the end/.

PiotrSikora · 2017-07-25T18:20:13Z

docs/configuration/listeners/ssl.rst

s/in the end/at the end/.

PiotrSikora · 2017-07-25T18:21:34Z

source/common/ssl/context_impl.h

Please remove space and perhaps replace this with the same example as in docs (i.e. foo.com/*).

I tried foo.com/* but compiler gave me this weird error:

error: "/*" within comment [-Werror=comment]

Just remove the example, then.

mattklein123

Small nit. LGTM pending @PiotrSikora +1

mattklein123 · 2017-07-26T15:35:39Z

source/common/ssl/context_impl.cc

 }

+bool ContextImpl::uriMatch(const std::string& uriPattern, const char* uri) {
+  size_t pattern_len = uriPattern.length();


nit: const size_t

myidpt · 2017-07-26T17:07:29Z

docs/configuration/cluster_manager/cluster_ssl.rst

  *(optional, array)* An optional list of subject alt names. If specified, Envoy will verify
-  that the server certificate's subject alt name matches one of the specified values.
+  that the server certificate's subject alt name matches one of the specified values. The names
+  support wildcard at the end. For example, ``foo.com/*`` will match certificate with subject alt


Could you make it clear that this wildcard support is only for URI type SAN. And also for DNS type SAN, there's a wildcard support for "*.foo.com" style?

myidpt · 2017-07-26T17:11:24Z

source/common/ssl/context_impl.cc

  return verified;
 }

+bool ContextImpl::uriMatch(const std::string& uriPattern, const char* uri) {


nit: Maybe move this function after dNSNameMatch, to be consistent with the order in the .h file.

PiotrSikora · 2017-07-26T17:12:37Z

source/common/ssl/context_impl.cc

@@ -209,7 +209,7 @@ bool ContextImpl::verifySubjectAltName(X509* cert,
        ASN1_STRING* str = altname->d.uniformResourceIdentifier;
        char* crt_san = reinterpret_cast<char*>(ASN1_STRING_data(str));


It just occurred to me that there is ASN1_STRING_length(), which is O(1), so this could be changed to:

std::string crt_san(reinterpret_cast<char *>(ASN1_STRING_data(str)), ASN1_STRING_length(str));

and then we could compare strings only if crt_san.length() >= uriPattern.length() and use std::string::compare() again.

Sorry for not catching this yesterday!

I'll fix this along with the problem mentioned below.

a side question though, if we use string (const char* s, size_t n) then isn't it also O(n)? Since the constructor always copies the characters

Yes, that is correct. Optimally, You would just pass char* and length to the function, then still use strncmp (or compare if you can). At this point though let's just get something working that everyone agrees with. :)

cool, I'll stick to the current way then

...it is. Feel free to ignore this.

Alternatively, you could use ASN1_STRING_length() and pass size_t to do the crt_san_len >= uriPattern.length() check and use std::string::compare().

right. I'll pass size_t since it also makes DNS match easier.

myidpt · 2017-07-26T17:47:23Z

source/common/ssl/context_impl.cc

+  return uriPattern == uri;
+}
+
 bool ContextImpl::dNSNameMatch(const std::string& dNSName, const char* pattern) {


Sorry to bring this up ... I feel like how this function is called (line 203 in context_impl.cc) is not right. It should be:
dNSNameMatch(dns_name, config_san)
config_san should be the pattern, not dns_name from the cert.

It makes sense to me that we support wildcard in config, not in certificates. Unfortunately I didn't notice this when I modified this function. This was unnoticed because we don't have integration tests, nor did we tell users this wildcard is supported.
@mattklein123 Could you confirm?

@myidpt yes that make sense to me. Can we fix?

@myidpt @mattklein123 I can fix this and update the doc. So the SAN field, whether it's URI or DNS, should never contain a wildcard; and we allow configs to optionally use wildcard to match multiple SAN values.

Thank you very much. Please also make sure the function signature (parameter naming&order) aligns with the uriMatch function :)

PiotrSikora · 2017-07-26T21:47:20Z

source/common/ssl/context_impl.cc

        char* dns_name = reinterpret_cast<char*>(ASN1_STRING_data(str));
        for (auto& config_san : subject_alt_names) {
-          if (dNSNameMatch(config_san, dns_name)) {
+          if (dNSNameMatch(config_san, dns_name, static_cast<size_t>(ASN1_STRING_length(str)))) {


You probably want to save this to a local variable before the for loop.

To avoid extra calls to ASN1_STRING_length() (since I don't think it will get inlined) and dereference of str.

PiotrSikora · 2017-07-26T21:47:43Z

source/common/ssl/context_impl.cc

        char* crt_san = reinterpret_cast<char*>(ASN1_STRING_data(str));
        for (auto& config_san : subject_alt_names) {
-          if (config_san.compare(crt_san) == 0) {
+          if (uriMatch(config_san, crt_san, static_cast<size_t>(ASN1_STRING_length(str)))) {


PiotrSikora · 2017-07-27T18:55:28Z

test/config/integration/server_ssl.json

      "ssl_context": {
        "ca_cert_file": "{{ test_rundir }}/test/config/integration/certs/upstreamcacert.pem",
-        "verify_subject_alt_name": [ "foo.lyft.com" ]
+        "verify_subject_alt_name": [ "*.lyft.com" ]


I'd prefer if you add new test cases, so that we have both exact match and wildcard covered.

I think unit tests already cover both cases. The purpose of integration tests should just be making sure that the overall flow works?

I disagree, especially when most of the verification and SAN extraction is actually happening in 3rd-party library, which isn't involved at all in those unit tests.

FWIW, I disagree also. Please keep the existing test and add a new one. Changing this code almost always breaks something / someone so the more coverage we have the better. (Hence why we are now at like 60 comments on a tiny PR).

mattklein123 · 2017-08-02T23:47:24Z

@crazytan friendly ping to make the requested changes so that we can close this out. Thank you.

mattklein123 · 2017-08-04T18:52:47Z

Closing this for now. Please reopen when ready to finish.

Resolves #1292. Handles the iOS portion of #1291, and the Android changes are being tracked there. Risk Level: Medium, new filter being added to the core chain Testing: Added in PR Docs Changes: N/A Signed-off-by: Michael Rebello <me@michaelrebello.com> Co-authored-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

**Description** Fix response_format schema type with raw byte to avoid sorting behavior by `json/encode` Go library. Fixes #1298 --------- Signed-off-by: Xiaolin Lin <xlin158@bloomberg.net>

mattklein123 reviewed Jul 21, 2017

View reviewed changes

mattklein123 requested a review from myidpt July 21, 2017 00:24

ssl: support wildcard matching for verify_subject_alt_name

3ed769d

mattklein123 reviewed Jul 21, 2017

View reviewed changes

PiotrSikora reviewed Jul 24, 2017

View reviewed changes

rewrite uriMatch for better performance

bef14f1

change back to using std::string::compare()

e4b97c7

mattklein123 reviewed Jul 25, 2017

View reviewed changes

PiotrSikora suggested changes Jul 25, 2017

View reviewed changes

change back to use strncmp()

ad7d440

mattklein123 reviewed Jul 26, 2017

View reviewed changes

myidpt reviewed Jul 26, 2017

View reviewed changes

PiotrSikora suggested changes Jul 26, 2017

View reviewed changes

myidpt reviewed Jul 26, 2017

View reviewed changes

fix dNSNameMatch and update the doc

c878f8b

PiotrSikora suggested changes Jul 26, 2017

View reviewed changes

cert and config changes for ssl integration tests

9347913

PiotrSikora reviewed Jul 27, 2017

View reviewed changes

mattklein123 closed this Aug 4, 2017

PiotrSikora mentioned this pull request Jul 16, 2018

ssl: subject alternate name regex validation support #3858

Closed

		@@ -209,7 +209,7 @@ bool ContextImpl::verifySubjectAltName(X509* cert,
		ASN1_STRING* str = altname->d.uniformResourceIdentifier;
		char* crt_san = reinterpret_cast<char*>(ASN1_STRING_data(str));

Conversation

crazytan commented Jul 20, 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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 Jul 24, 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!

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Jul 24, 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!

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!

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!

crazytan Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

mattklein123 Jul 24, 2017 •

edited

Loading

crazytan Jul 26, 2017 •

edited

Loading

crazytan Jul 26, 2017 •

edited

Loading