Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
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
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)))) {
Copy link
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.

Copy link
Author

Choose a reason for hiding this comment

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

why?

Copy link
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));
Copy link
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!

Copy link
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.

Copy link
Author

@crazytan crazytan Jul 26, 2017

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

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

Copy link
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

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

Copy link
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)))) {
Copy link
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
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
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
14 changes: 7 additions & 7 deletions test/config/integration/certs/upstreamcacert.pem
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
-----BEGIN CERTIFICATE-----
MIIC3zCCAkigAwIBAgIJAIP3FexxDZw/MA0GCSqGSIb3DQEBCwUAMH8xCzAJBgNV
MIIC3zCCAkigAwIBAgIJAOqvogwSzSGmMA0GCSqGSIb3DQEBCwUAMH8xCzAJBgNV
BAYTAlVTMRMwEQYDVQQIEwpDYWxpZm9ybmlhMRYwFAYDVQQHEw1TYW4gRnJhbmNp
c2NvMQ0wCwYDVQQKEwRMeWZ0MRkwFwYDVQQLExBMeWZ0IEVuZ2luZWVyaW5nMRkw
FwYDVQQDExBUZXN0IFVwc3RyZWFtIENBMB4XDTE3MDcwOTAxMzkzMloXDTE5MDcw
OTAxMzkzMlowfzELMAkGA1UEBhMCVVMxEzARBgNVBAgTCkNhbGlmb3JuaWExFjAU
FwYDVQQDExBUZXN0IFVwc3RyZWFtIENBMB4XDTE3MDcyNzE3NTcxNFoXDTE5MDcy
NzE3NTcxNFowfzELMAkGA1UEBhMCVVMxEzARBgNVBAgTCkNhbGlmb3JuaWExFjAU
BgNVBAcTDVNhbiBGcmFuY2lzY28xDTALBgNVBAoTBEx5ZnQxGTAXBgNVBAsTEEx5
ZnQgRW5naW5lZXJpbmcxGTAXBgNVBAMTEFRlc3QgVXBzdHJlYW0gQ0EwgZ8wDQYJ
KoZIhvcNAQEBBQADgY0AMIGJAoGBANbxYY3hK35w0cSDReeoEJtqoegs+v3wo3B7
Uaki2AWXcdQK4kEyz1zesRcwUgT3gTqNdQJ+WiN0UtZpEgqvNDvSRYj1ONLIrnP7
en1Uc2ld5KHjzZfSUnlDSlHURGad2N/V9fsT8HUUrAFSNnyRRmA54zuuQP8cQBLl
YXgisaADAgMBAAGjYzBhMA8GA1UdEwEB/wQFMAMBAf8wDgYDVR0PAQH/BAQDAgEG
MB0GA1UdDgQWBBQ3dX2uHV9GclTYCEwe0vUNWBQwaTAfBgNVHSMEGDAWgBQ3dX2u
HV9GclTYCEwe0vUNWBQwaTANBgkqhkiG9w0BAQsFAAOBgQB6tJ+WGcHcAwc148ca
+TtjxKB1GI6LoQusMXmF5tZlFi/Prk8+pHqcNZdFeNxYSIQnZvIQijTHUFKrBfbn
1KGxrivXNGQj1/ivAAofzduLHK0k8g8koejwyM5aqkMXUucGHyp28+75D5paTnEZ
v+yOm2g0grB52deXB97mj5yYlQ==
HV9GclTYCEwe0vUNWBQwaTANBgkqhkiG9w0BAQsFAAOBgQBEuF8bxTKs/ZxjKCkV
vL/1VFl//Y0iXpYl7D+fH6R1/jC0FQ5x1esvJkmS4vVqWCkpz+u8PFi4LhgIIx5V
6IhP4fvJtBaGkSE7Ct8zFL2mZX4198Bm+XbuUXvdehOAs6GX3XIVI+Pcu7ioaXPP
MTFQHMB4XlhWsUudCZ7VVln5Ew==
-----END CERTIFICATE-----
2 changes: 1 addition & 1 deletion test/config/integration/certs/upstreamcert.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,4 @@ subjectKeyIdentifier = hash
authorityKeyIdentifier = keyid:always

[alt_names]
DNS.1 = *.lyft.com
DNS.1 = foo.lyft.com
20 changes: 10 additions & 10 deletions test/config/integration/certs/upstreamcert.pem
Original file line number Diff line number Diff line change
@@ -1,19 +1,19 @@
-----BEGIN CERTIFICATE-----
MIIDFjCCAn+gAwIBAgIJAIEoeL+knpI0MA0GCSqGSIb3DQEBCwUAMH8xCzAJBgNV
MIIDGDCCAoGgAwIBAgIJAJPDaoHAmb5AMA0GCSqGSIb3DQEBCwUAMH8xCzAJBgNV
BAYTAlVTMRMwEQYDVQQIEwpDYWxpZm9ybmlhMRYwFAYDVQQHEw1TYW4gRnJhbmNp
c2NvMQ0wCwYDVQQKEwRMeWZ0MRkwFwYDVQQLExBMeWZ0IEVuZ2luZWVyaW5nMRkw
FwYDVQQDExBUZXN0IFVwc3RyZWFtIENBMB4XDTE3MDcwOTAxMzkzMloXDTE5MDcw
OTAxMzkzMlowgYMxCzAJBgNVBAYTAlVTMRMwEQYDVQQIEwpDYWxpZm9ybmlhMRYw
FwYDVQQDExBUZXN0IFVwc3RyZWFtIENBMB4XDTE3MDcyNzE3NTcxNFoXDTE5MDcy
NzE3NTcxNFowgYMxCzAJBgNVBAYTAlVTMRMwEQYDVQQIEwpDYWxpZm9ybmlhMRYw
FAYDVQQHEw1TYW4gRnJhbmNpc2NvMQ0wCwYDVQQKEwRMeWZ0MRkwFwYDVQQLExBM
eWZ0IEVuZ2luZWVyaW5nMR0wGwYDVQQDExRUZXN0IFVwc3RyZWFtIFNlcnZlcjCB
nzANBgkqhkiG9w0BAQEFAAOBjQAwgYkCgYEAu7BZ855UTUGLBbSIG7NbmOHnit0k
qhVaS7qf56wuGybRyoS82vUKKF+HG84DJZGjPCCzr9sjgPuDI2ebWf28YScS/K/v
VVVk4mdJcpL8LSKF9j2wWjD1fk/soLAfeMgs3GvXBXk1GsJ+PzCr6jKJ+EqghKpV
3snf6/leRi0gBrMCAwEAAaOBlDCBkTAMBgNVHRMBAf8EAjAAMAsGA1UdDwQEAwIF
4DAdBgNVHSUEFjAUBggrBgEFBQcDAgYIKwYBBQUHAwEwFQYDVR0RBA4wDIIKKi5s
eWZ0LmNvbTAdBgNVHQ4EFgQUL4L/GnkQef+2KrADZK0zecZe84MwHwYDVR0jBBgw
FoAUN3V9rh1fRnJU2AhMHtL1DVgUMGkwDQYJKoZIhvcNAQELBQADgYEAvE6Hoguo
ImLP3WPBzW5WuvWwheG2QOlCEo9gP3V/C4ptAr92znG4Jrsp72S86LFdFYR+ZKgK
4QRwtoPyVIb6cGK+N70TE/21Z3jWuA5gvezKT3XnGy+RhPSapLAjjafuvYsVUPv9
aGPmBCsMiipz9dWjLxVTF9nUNZeRLnxbLSk=
3snf6/leRi0gBrMCAwEAAaOBljCBkzAMBgNVHRMBAf8EAjAAMAsGA1UdDwQEAwIF
4DAdBgNVHSUEFjAUBggrBgEFBQcDAgYIKwYBBQUHAwEwFwYDVR0RBBAwDoIMZm9v
Lmx5ZnQuY29tMB0GA1UdDgQWBBQvgv8aeRB5/7YqsANkrTN5xl7zgzAfBgNVHSME
GDAWgBQ3dX2uHV9GclTYCEwe0vUNWBQwaTANBgkqhkiG9w0BAQsFAAOBgQAHX0fA
YGUQrS5/+k+lnmcJo6j0dSwGZAjemIjbFUXqMPOK72G4vveZKGJvYQ5bLXiRdc5I
XKcxpdF1h9DNnVUdCJtb4UEHds1s167uACX/fLW8IhWiN+V6scI4oBzS0fe4qKn6
MhcCbQFkIVgCFBesM30elX8yPHIYTSBeeA/OgQ==
-----END CERTIFICATE-----
4 changes: 2 additions & 2 deletions test/config/integration/server_ssl.json
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@
"connect_timeout_ms": 5000,
"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" ]
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

},
"type": "static",
"lb_type": "round_robin",
Expand All @@ -110,7 +110,7 @@
"connect_timeout_ms": 5000,
"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" ]
},
"type": "strict_dns",
"lb_type": "round_robin",
Expand Down
4 changes: 2 additions & 2 deletions test/config/integration/server_xfcc.json
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@
"connect_timeout_ms": 5000,
"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" ]
},
"type": "static",
"lb_type": "round_robin",
Expand All @@ -190,7 +190,7 @@
"connect_timeout_ms": 5000,
"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" ]
},
"type": "strict_dns",
"lb_type": "round_robin",
Expand Down