Skip to content

ssl: support wildcard matching for verify_subject_alt_name #1298

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
crazytan wants to merge 6 commits into from
Closed

ssl: support wildcard matching for verify_subject_alt_name #1298

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3ed769d
ssl: support wildcard matching for verify_subject_alt_name
crazytan Jul 20, 2017
bef14f1
rewrite uriMatch for better performance
crazytan Jul 21, 2017
e4b97c7
change back to using std::string::compare()
crazytan Jul 25, 2017
ad7d440
change back to use strncmp()
crazytan Jul 25, 2017
c878f8b
fix dNSNameMatch and update the doc
crazytan Jul 26, 2017
9347913
cert and config changes for ssl integration tests
crazytan Jul 27, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion docs/configuration/cluster_manager/cluster_ssl.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,9 @@ verify_certificate_hash

verify_subject_alt_name
*(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 in the end. For example, ``foo.com/*`` will match certificate with subject alt

@PiotrSikora PiotrSikora Jul 25, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

s/in the end/at the end/.

name ``foo.com/bar``.

cipher_suites
*(optional, string)* If specified, the TLS connection will only support the specified `cipher list
Expand Down
4 changes: 3 additions & 1 deletion docs/configuration/listeners/ssl.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,9 @@ verify_certificate_hash

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

@PiotrSikora PiotrSikora Jul 25, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

s/in the end/at the end/.

name ``foo.com/bar``.

cipher_suites
*(optional, string)* If specified, the TLS listener will only support the specified `cipher list
Expand Down
11 changes: 10 additions & 1 deletion source/common/ssl/context_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));

@PiotrSikora PiotrSikora Jul 26, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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!

@crazytan crazytan Jul 26, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

@crazytan crazytan Jul 26, 2017 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

@mattklein123 mattklein123 Jul 26, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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. :)

@crazytan crazytan Jul 26, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

cool, I'll stick to the current way then

@PiotrSikora PiotrSikora Jul 26, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

...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().

@crazytan crazytan Jul 26, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

for (auto& config_san : subject_alt_names) {
if (config_san.compare(crt_san) == 0) {
if (uriMatch(config_san, crt_san)) {
verified = true;
break;
}
Expand All @@ -223,6 +223,15 @@ bool ContextImpl::verifySubjectAltName(X509* cert,
return verified;
}

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

@PiotrSikora PiotrSikora Jul 24, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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)

@mattklein123 mattklein123 Jul 24, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes that would be much better. :)

@crazytan crazytan Jul 24, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

@PiotrSikora PiotrSikora Jul 24, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@crazytan crazytan Jul 25, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

@myidpt myidpt Jul 26, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

size_t pattern_len = uriPattern.length();

@mattklein123 mattklein123 Jul 26, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: const size_t

if (pattern_len > 1 && uriPattern[pattern_len - 1] == '*' && uriPattern[pattern_len - 2] == '/') {

@PiotrSikora PiotrSikora Jul 25, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps

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

would be more readable?

@crazytan crazytan Jul 25, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

agree

return uriPattern.compare(0, pattern_len - 1, uri, pattern_len - 1) == 0;

@mattklein123 mattklein123 Jul 25, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@PiotrSikora PiotrSikora Jul 25, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@crazytan crazytan Jul 25, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

@crazytan crazytan Jul 25, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

@PiotrSikora PiotrSikora Jul 25, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sigh, indeed. What a terrible API.

@crazytan crazytan Jul 25, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I'll use strncmp() then.

}

return uriPattern == uri;
}

bool ContextImpl::dNSNameMatch(const std::string& dNSName, const char* pattern) {

@myidpt myidpt Jul 26, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

@mattklein123 mattklein123 Jul 26, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@crazytan crazytan Jul 26, 2017 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@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.

@myidpt myidpt Jul 26, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

if (dNSName == pattern) {
return true;
Expand Down
8 changes: 8 additions & 0 deletions source/common/ssl/context_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,14 @@ class ContextImpl : public virtual Context {
*/
static bool dNSNameMatch(const std::string& dnsName, const char* pattern);

/**
* Determines whether the given URI matches 'pattern' which may end with a wildcard.
* @param uriPattern the pattern to match against (foo.bar/baz/ *)

@PiotrSikora PiotrSikora Jul 25, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@crazytan crazytan Jul 25, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

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

@PiotrSikora PiotrSikora Jul 25, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just remove the example, then.

* @param uri the URI to match
* @return true if uri matches pattern
*/
static bool uriMatch(const std::string& uriPattern, const char* uri);

SslStats& stats() { return stats_; }

// Ssl::Context
Expand Down
19 changes: 19 additions & 0 deletions test/common/ssl/context_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,20 @@ TEST_F(SslContextImplTest, TestdNSNameMatching) {
EXPECT_FALSE(ContextImpl::dNSNameMatch("lyft.com", ""));
}

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/"));

@PiotrSikora PiotrSikora Jul 25, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@crazytan crazytan Jul 25, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

@PiotrSikora PiotrSikora Jul 25, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@crazytan crazytan Jul 25, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

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

@PiotrSikora PiotrSikora Jul 25, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you also add test for:

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

@crazytan crazytan Jul 25, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

will add

EXPECT_FALSE(ContextImpl::uriMatch("spiffe://lyft.com/foo", "spiffe://lyft.com/foo/bar"));
EXPECT_FALSE(ContextImpl::uriMatch("spiffe://lyft.com/*", ""));
EXPECT_FALSE(ContextImpl::uriMatch("spiffe://lyft.com/*", "spiffe://lyft.net/foo"));
EXPECT_FALSE(ContextImpl::uriMatch("spiffe://lyft.com*", "spiffe://lyft.com/foo"));
EXPECT_FALSE(ContextImpl::uriMatch("spiffe://lyft.com/*", "spiffe://lyft.comfoo"));
EXPECT_FALSE(ContextImpl::uriMatch("*", "foo"));
EXPECT_FALSE(ContextImpl::uriMatch("f", "foo"));
}

TEST_F(SslContextImplTest, TestVerifySubjectAltNameDNSMatched) {
FILE* fp = fopen(
TestEnvironment::runfilesPath("test/common/ssl/test_data/san_dns_cert.pem").c_str(), "r");
Expand All @@ -52,6 +66,11 @@ TEST_F(SslContextImplTest, TestVerifySubjectAltNameURIMatched) {
std::vector<std::string> verify_subject_alt_name_list = {"spiffe://lyft.com/fake-team",
"spiffe://lyft.com/test-team"};
EXPECT_TRUE(ContextImpl::verifySubjectAltName(cert, verify_subject_alt_name_list));

verify_subject_alt_name_list.clear();
verify_subject_alt_name_list.push_back("spiffe://lyft.com/*");
EXPECT_TRUE(ContextImpl::verifySubjectAltName(cert, verify_subject_alt_name_list));

X509_free(cert);
fclose(fp);
}
Expand Down