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 5 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
6 changes: 5 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,11 @@ 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 client certificate's subject alt name matches one of the specified values. You can specify
two types of subject alt names: DNS (``www.foo.com``) and URI (``foo.com/bar``). Both types also
support optional wildcard matching. For DNS, ``*.foo.com`` will match subject alt names like
```bar.foo.com`` and ``baz.foo.com``. For URI, ``foo.com/*`` will match names like ``foo.com/bar``
and ``foo.com/baz``.

cipher_suites
*(optional, string)* If specified, the TLS connection will only support the specified `cipher list
Expand Down
6 changes: 5 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,11 @@ 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. You can specify
two types of subject alt names: DNS (``www.foo.com``) and URI (``foo.com/bar``). Both types also
support optional wildcard matching. For DNS, ``*.foo.com`` will match subject alt names like
```bar.foo.com`` and ``baz.foo.com``. For URI, ``foo.com/*`` will match names like ``foo.com/bar``
and ``foo.com/baz``.

cipher_suites
*(optional, string)* If specified, the TLS listener will only support the specified `cipher list
Expand Down
31 changes: 20 additions & 11 deletions source/common/ssl/context_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ bool ContextImpl::verifySubjectAltName(X509* cert,
ASN1_STRING* str = altname->d.dNSName;
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)))) {

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

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

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

why?

@PiotrSikora PiotrSikora Jul 27, 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.

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

verified = true;
break;
}
Expand All @@ -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, static_cast<size_t>(ASN1_STRING_length(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.

Same here.

verified = true;
break;
}
Expand All @@ -223,20 +223,29 @@ bool ContextImpl::verifySubjectAltName(X509* cert,
return verified;
}

bool ContextImpl::dNSNameMatch(const std::string& dNSName, const char* pattern) {
if (dNSName == pattern) {
return true;
bool ContextImpl::dNSNameMatch(const std::string& dns_pattern, const char* dns, size_t dns_len) {
size_t pattern_len = dns_pattern.length();
if (pattern_len > 1 && dns_pattern.substr(0, 2) == "*.") {
if (dns_len >= pattern_len) {
size_t off = dns_len - pattern_len + 1;
return dns_pattern.compare(1, pattern_len - 1, dns + off, pattern_len - 1) == 0;
}
return false;
}

size_t pattern_len = strlen(pattern);
if (pattern_len > 1 && pattern[0] == '*' && pattern[1] == '.') {
if (dNSName.length() > pattern_len - 1) {
size_t off = dNSName.length() - pattern_len + 1;
return dNSName.compare(off, pattern_len - 1, pattern + 1) == 0;
return dns_pattern == dns;
}

bool ContextImpl::uriMatch(const std::string& uri_pattern, const char* uri, size_t uri_len) {
size_t pattern_len = uri_pattern.length();
if (pattern_len > 1 && uri_pattern.substr(pattern_len - 2) == "/*") {
if (uri_len >= pattern_len) {
return uri_pattern.compare(0, pattern_len - 1, uri, pattern_len - 1) == 0;
}
return false;
}

return false;
return uri_pattern == uri;
}

bool ContextImpl::verifyCertificateHash(X509* cert, const std::vector<uint8_t>& expected_hash) {
Expand Down
20 changes: 15 additions & 5 deletions source/common/ssl/context_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,13 +60,23 @@ class ContextImpl : public virtual Context {
static bool verifySubjectAltName(X509* cert, const std::vector<std::string>& subject_alt_names);

/**
* Determines whether the given name matches 'pattern' which may optionally begin with a wildcard.
* Determines whether the given DNS matches 'pattern' which may begin with a wildcard.
* NOTE: public for testing
* @param san the subjectAltName to match
* @param pattern the pattern to match against (*.example.com)
* @return true if the san matches pattern
* @param dns_pattern the pattern to match against (*.example.com)
* @param dns the DNS to match
* @param dns_len length of DNS string
* @return true if the dns matches pattern
*/
static bool dNSNameMatch(const std::string& dnsName, const char* pattern);
static bool dNSNameMatch(const std::string& dns_pattern, const char* dns, size_t dns_len);

/**
* Determines whether the given URI matches 'pattern' which may end with a wildcard.
* @param uri_pattern the pattern to match against
* @param uri the URI to match
* @param uri_len length of URI string
* @return true if uri matches pattern
*/
static bool uriMatch(const std::string& uri_pattern, const char* uri, size_t uri_len);

SslStats& stats() { return stats_; }

Expand Down
40 changes: 30 additions & 10 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 @@ -18,16 +18,31 @@ namespace Ssl {
class SslContextImplTest : public SslCertsTest {};

TEST_F(SslContextImplTest, TestdNSNameMatching) {
EXPECT_TRUE(ContextImpl::dNSNameMatch("lyft.com", "lyft.com"));
EXPECT_TRUE(ContextImpl::dNSNameMatch("a.lyft.com", "*.lyft.com"));
EXPECT_TRUE(ContextImpl::dNSNameMatch("a.b.lyft.com", "*.lyft.com"));
EXPECT_FALSE(ContextImpl::dNSNameMatch("foo.test.com", "*.lyft.com"));
EXPECT_FALSE(ContextImpl::dNSNameMatch("lyft.com", "*.lyft.com"));
EXPECT_FALSE(ContextImpl::dNSNameMatch("alyft.com", "*.lyft.com"));
EXPECT_FALSE(ContextImpl::dNSNameMatch("alyft.com", "*lyft.com"));
EXPECT_FALSE(ContextImpl::dNSNameMatch("lyft.com", "*lyft.com"));
EXPECT_FALSE(ContextImpl::dNSNameMatch("", "*lyft.com"));
EXPECT_FALSE(ContextImpl::dNSNameMatch("lyft.com", ""));
EXPECT_TRUE(ContextImpl::dNSNameMatch("lyft.com", "lyft.com", 8));
EXPECT_TRUE(ContextImpl::dNSNameMatch("*.lyft.com", "a.lyft.com", 10));
EXPECT_TRUE(ContextImpl::dNSNameMatch("*.lyft.com", "a.b.lyft.com", 12));
EXPECT_FALSE(ContextImpl::dNSNameMatch("*.lyft.com", "foo.test.com", 12));
EXPECT_FALSE(ContextImpl::dNSNameMatch("*.lyft.com", "lyft.com", 8));
EXPECT_FALSE(ContextImpl::dNSNameMatch("*.lyft.com", "alyft.com", 9));
EXPECT_FALSE(ContextImpl::dNSNameMatch("*lyft.com", "alyft.com", 9));
EXPECT_FALSE(ContextImpl::dNSNameMatch("*lyft.com", "lyft.com", 8));
EXPECT_FALSE(ContextImpl::dNSNameMatch("*lyft.com", "", 0));
EXPECT_FALSE(ContextImpl::dNSNameMatch("", "lyft.com", 8));
}

TEST_F(SslContextImplTest, TestURIMatch) {
EXPECT_TRUE(ContextImpl::uriMatch("spiffe://lyft.com/foo", "spiffe://lyft.com/foo", 21));
EXPECT_TRUE(ContextImpl::uriMatch("spiffe://lyft.com/*", "spiffe://lyft.com/foo", 21));
EXPECT_TRUE(ContextImpl::uriMatch("spiffe://lyft.com/foo/*", "spiffe://lyft.com/foo/bar", 25));
EXPECT_TRUE(ContextImpl::uriMatch("spiffe://lyft.com/*", "spiffe://lyft.com/foo/bar", 25));
EXPECT_FALSE(ContextImpl::uriMatch("spiffe://lyft.com/*", "spiffe://lyft.com/", 18));
EXPECT_FALSE(ContextImpl::uriMatch("spiffe://lyft.com/foo", "spiffe://lyft.com/foo/bar", 25));
EXPECT_FALSE(ContextImpl::uriMatch("spiffe://lyft.com/*", "", 0));
EXPECT_FALSE(ContextImpl::uriMatch("spiffe://lyft.com/*", "spiffe://lyft.net/foo", 21));
EXPECT_FALSE(ContextImpl::uriMatch("spiffe://lyft.com*", "spiffe://lyft.com/foo", 21));
EXPECT_FALSE(ContextImpl::uriMatch("spiffe://lyft.com/*", "spiffe://lyft.comfoo", 20));
EXPECT_FALSE(ContextImpl::uriMatch("*", "foo", 3));
EXPECT_FALSE(ContextImpl::uriMatch("f", "foo", 3));
}

TEST_F(SslContextImplTest, TestVerifySubjectAltNameDNSMatched) {
Expand All @@ -52,6 +67,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