Verify SAN with multiple values, support verifying URI type in SAN by myidpt · Pull Request #559 · envoyproxy/envoy

myidpt · 2017-03-10T05:23:12Z

Enable Envoy to verify certificate SAN field against multiple values (set through config).
In config, modify the type of verify_subject_alt_name field to an array.
Support verifying URI type in certificate SAN. The exact match is checked.

This is part of Istio work.

fixes #515

Email SAN types.

mattklein123

generally looks good, a few comments/questions.

mattklein123 · 2017-03-10T17:34:35Z

source/common/ssl/context_config_impl.h

  std::string cert_chain_file_;
  std::string private_key_file_;
-  std::string verify_subject_alt_name_;
+  std::vector<std::string> verify_subject_alt_name_;


nit: verify_subject_alt_names_ or verify_subject_alt_name_list_

verify_subject_alt_name_list_ sounds good :)

Good point.

mattklein123 · 2017-03-10T17:35:44Z

source/common/ssl/context_impl.h


  ContextManagerImpl& parent_;
  SslCtxPtr ctx_;
-  std::string verify_subject_alt_name_;


same nit above

mattklein123 · 2017-03-10T17:54:59Z

test/common/ssl/context_impl_test.cc

+}
+
+TEST(SslContextImplTest, TestVerifySubjectAltNameDNSMatched) {
+  FILE* fp = fopen("test/common/ssl/test_data/san_dns.crt", "r");


close fp here and in other test functions.

mattklein123 · 2017-03-10T17:56:01Z

source/common/ssl/context_impl.cc

 }

-bool ContextImpl::verifySubjectAltName(X509* cert, const std::string& subject_alt_name) {
+bool ContextImpl::verifySubjectAltName(X509* cert,


I assume we have 100% coverage on this function now? Can you please get someone else who is familiar with TLS to give a +1 on the TLS part. I'm really not familiar with this stuff and don't feel comfortable approving the actual crypto aspect.

Yes, 100% unit test coverage. I can get Tao Li in the loop as well.

wattli · 2017-03-10T23:24:26Z

source/common/ssl/context_config_impl.h

  std::string cert_chain_file_;
  std::string private_key_file_;
-  std::string verify_subject_alt_name_;
+  std::vector<std::string> verify_subject_alt_name_;


Good point.

wattli · 2017-03-10T23:30:15Z

source/common/ssl/context_impl.cc

+        for (auto& config_san : subject_alt_names) {
+          if (dNSNameMatch(config_san, dns_name)) {
+            verified = true;
+            break;


This break will only jump out the for loop in line 266, but not the one in line 260, which will change the existing behavior.

Maybe you can free altnames here and return true.

Good catch. Thanks!

@wattli yes, great catch. @myidpt can we make sure to have a test case that would have caught this bug.

@mattklein123 , this is not a bug, it's just about efficiency since 'verified' will never be set to 'false' explicitly.

ah ok, cool, thanks. Didn't look carefully.

Adding "&& !verified" in the loop condition should do the work.

wattli · 2017-03-10T23:31:32Z

source/common/ssl/context_impl.cc

+        for (auto& config_san : subject_alt_names) {
+          if (config_san.compare(crt_san) == 0) {
+            verified = true;
+            break;


Same thing here. Should return immediately once a match found.

This commit cleans up a few leftovers from envoyproxy#559 .

This commit cleans up a few leftovers from #559 .

Signed-off-by: Pengyuan Bian <[email protected]>

This archive is actually a zip containing the produced aar, so this makes that a bit less confusing. Signed-off-by: Keith Smiley <[email protected]> Signed-off-by: JP Simard <[email protected]>

Oliver Liu added 2 commits March 9, 2017 20:47

Change verify_subject_alt_name to array. Support verifying URI and

d87aaf7

Email SAN types.

Merge branch 'master' into verify_san

010e534

myidpt changed the title ~~Verify SAN with multiple values, support verifying SAN type in URI~~ Verify SAN with multiple values, support verifying URI type in SAN Mar 10, 2017

myidpt closed this Mar 10, 2017

myidpt reopened this Mar 10, 2017

mattklein123 reviewed Mar 10, 2017

View reviewed changes

Merge branch 'master' of https://github.com/lyft/envoy into verify_san

f5a0cc4

wattli approved these changes Mar 10, 2017

View reviewed changes

Oliver Liu added 2 commits March 10, 2017 16:12

Polish the code for PR#559.

658ef29

Fix some test logic in context_impl_text.

4fc154a

mattklein123 approved these changes Mar 11, 2017

View reviewed changes

mattklein123 merged commit 9dcac8c into envoyproxy:master Mar 11, 2017

myidpt deleted the verify_san branch March 13, 2017 03:58

lookuptable added a commit to lookuptable/envoy that referenced this pull request Mar 14, 2017

"verify_subject_alt_name" should accept an array of strings

c29a442

This commit cleans up a few leftovers from envoyproxy#559 .

lookuptable mentioned this pull request Mar 14, 2017

"verify_subject_alt_name" should accept an array of strings #569

Merged

mattklein123 pushed a commit that referenced this pull request Mar 15, 2017

"verify_subject_alt_name" should accept an array of strings (#569)

fca5cd5

This commit cleans up a few leftovers from #559 .

jplevyak pushed a commit to istio/envoy that referenced this pull request Jul 9, 2020

Prometheus scraper: Opt out of automatic namespacing (envoyproxy#559)

99c7b92

Signed-off-by: Pengyuan Bian <[email protected]>

mathetake pushed a commit that referenced this pull request Mar 3, 2026

build(deps): bump google.golang.org/grpc from 1.71.0 to 1.71.1 (#559)

c50941d

Conversation

myidpt commented Mar 10, 2017 • edited by mattklein123 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

myidpt commented Mar 10, 2017 •

edited by mattklein123

Loading