Specify type for matching Subject Alternative Name. #18628

pradeepcrao · 2021-10-14T15:13:19Z

Signed-off-by: Pradeep Rao [email protected]

Commit Message:
Additional Description:
Risk Level: Low
Testing: Added test
Docs Changes:
Release Notes:
Platform Specific Features:

Signed-off-by: Pradeep Rao <[email protected]>

repokitteh-read-only · 2021-10-14T15:13:28Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #18628 was opened by pradeepcrao.

see: more, trace.

ggreenway · 2021-10-14T16:47:21Z

api/envoy/extensions/transport_sockets/tls/v3/common.proto

  //
  // .. attention::
  //
  //   Subject Alternative Names are easily spoofable and verifying only them is insecure,
  //   therefore this option must be used together with :ref:`trusted_ca
  //   <envoy_v3_api_field_extensions.transport_sockets.tls.v3.CertificateValidationContext.trusted_ca>`.
-  repeated type.matcher.v3.StringMatcher match_subject_alt_names = 9;
+  repeated SubjectAltNameMatcher match_subject_alt_names_with_type = 14;


An alternative API that I think is simpler is to have a message of StringMatcher and an enum for the type of match. Thoughts?

That was exactly my first pass implementation. However, I could not find a simple way to extend it to support OtherName (eg. Microsoft UPN), the value for which could be a string or a complex message.

Jus thinking out loud here: if the typical use-case is using on of the types noted below, then it might make sense to have a oneof that includes either a typed-extension or a message that includes the enum+StringMatcher (example).

I think this would need lot of control planes to know about the type of SAN - which forces all the existing APIs to add the type (like DestintationRule API) in Istio which treats SANs as string. IMO this leads to lot of churn. Can we also accept ANY in enum so that if some one specifies ANY it does not mandate the type check? or leave both fields (do not deprecate the match_subject_alt_names)

cc: @howardjohn

Hi Rama, Howard,
the current change is needed to fix #18259 and having ANY in the enum or not deprecating match_subject_alt_names breaks the fix. Istio, etc. can keep utilizing the deprecated san matcher field for now, and use the deprecation window to modify their apis for correctness with respect to RFC 6125.

Consider renaming to match_typed_subject_alt_names.

But neither of these names are really correct, because this type allows either a typed-matcher, or a typed_config which could be anything. But I'm failing to think of a good name for this.

Signed-off-by: Pradeep Rao <[email protected]>

adisuissa

Thanks for fixing this!
Left a few API comments

adisuissa · 2021-10-15T14:52:41Z

api/envoy/extensions/transport_sockets/tls/v3/common.proto

@@ -253,7 +254,11 @@ message CertificateProviderPluginInstance {
  string certificate_name = 2;
 }

-// [#next-free-field: 14]
+message SubjectAltNameMatcher {


Please add comments describing the message and field.

adisuissa · 2021-10-15T14:55:54Z

api/envoy/extensions/transport_sockets/tls/v3/common.proto

@@ -253,7 +254,11 @@ message CertificateProviderPluginInstance {
  string certificate_name = 2;
 }

-// [#next-free-field: 14]
+message SubjectAltNameMatcher {
+  google.protobuf.Any typed_config = 1;


This type should be TypedExtensionConfig, as it is more strict.

Actually, is there an added value to use the wrapper message (SubjectAltNameMatcher) rather than just repeated core.v3.TypedExtensionConfig?

My hope was that the wrapper provides some "type enforcing" in the certificate validation context C++ classes, and helps map back to the right message in the config.

I'll switch to TypeExtensionConfig.

adisuissa · 2021-10-15T15:09:27Z

api/envoy/extensions/transport_sockets/tls/v3/common.proto

  //
  // .. attention::
  //
  //   Subject Alternative Names are easily spoofable and verifying only them is insecure,
  //   therefore this option must be used together with :ref:`trusted_ca
  //   <envoy_v3_api_field_extensions.transport_sockets.tls.v3.CertificateValidationContext.trusted_ca>`.
-  repeated type.matcher.v3.StringMatcher match_subject_alt_names = 9;
+  repeated SubjectAltNameMatcher match_subject_alt_names_with_type = 14;


Jus thinking out loud here: if the typical use-case is using on of the types noted below, then it might make sense to have a oneof that includes either a typed-extension or a message that includes the enum+StringMatcher (example).

pradeepcrao · 2021-10-15T15:42:26Z

With regards to having enum + StringMatcher, the enums would need to be one of the pound defines here: https://boringssl.googlesource.com/boringssl/+/refs/heads/master/include/openssl/x509v3.h#174 or an identifier type from here https://datatracker.ietf.org/doc/html/rfc6125#section-1.8

The issue is that a Stringmatcher would not make sense for every enum value, just the ones we support today.

My initial implementation was exactly that : enum + Stringmatcher instead of a typed config. I changed it when the above was pointed out to me.

Does that sound reasonable?

adisuissa · 2021-10-15T17:44:02Z

With regards to having enum + StringMatcher, the enums would need to be one of the pound defines here: https://boringssl.googlesource.com/boringssl/+/refs/heads/master/include/openssl/x509v3.h#174 or an identifier type from here https://datatracker.ietf.org/doc/html/rfc6125#section-1.8

The issue is that a Stringmatcher would not make sense for every enum value, just the ones we support today.

My initial implementation was exactly that : enum + Stringmatcher instead of a typed config. I changed it when the above was pointed out to me.

Does that sound reasonable?

I guess that if most use just a string matcher, then an enum + StringMatcher should be used to simplify the code, and a TypedExtension can be used to define the Other or some alternative that we don't support out of the box.
Specifically I suggest to have a generic string matcher message:

message StringSanMatcher {
  MatcherTypeEnum type = 1;
  type.matcher.v3.StringMatcher matcher = 2;
}

and the wrapper message message will use either the string matcher or the TypedExtension:

message SubjectAltNameMatcher {
  oneof {
    StringSanMatcher string_matcher = 1;
    core.v3.TypedExtensionConfig typed_config = 2;
  }
}

Note that this holds as long as we don't expect additional fields to the derived types (e.g., if UriSanMatcher will probably have an additional field later, then the current design may be better).

adisuissa · 2021-10-15T18:10:20Z

api/envoy/extensions/transport_sockets/tls/v3/common.proto

+  //  match_subject_alt_names_with_type:
+  //    typed_config:
+  //      "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.common.DnsSanMatcher
+  //      exact: "api.example.com"
  //
  // .. attention::
  //
  //   Subject Alternative Names are easily spoofable and verifying only them is insecure,
  //   therefore this option must be used together with :ref:`trusted_ca
  //   <envoy_v3_api_field_extensions.transport_sockets.tls.v3.CertificateValidationContext.trusted_ca>`.


Can you improve the comment by mentioning that this has an Any semantics (the SAN is verified if at least one of the matchers is matched)

StringSanMatchers or TypedExtensionConfigs for san matchers. Made test regexes stricter. Signed-off-by: Pradeep Rao <[email protected]>

Signed-off-by: Pradeep Rao <[email protected]>

adisuissa · 2021-10-18T20:45:40Z

source/common/ssl/certificate_validation_context_config_impl.cc

@@ -1,10 +1,13 @@
 #include "source/common/ssl/certificate_validation_context_config_impl.h"

 #include "envoy/common/exception.h"
+#include "envoy/config/core/v3/extension.pb.h"
+#include "envoy/extensions//transport_sockets/tls/v3/common.pb.h"


I think this is what causing your format error.

Suggested change

#include "envoy/extensions//transport_sockets/tls/v3/common.pb.h"

#include "envoy/extensions/transport_sockets/tls/v3/common.pb.h"

Thanks! Great catch!

Signed-off-by: Pradeep Rao <[email protected]>

pradeepcrao · 2021-11-17T14:03:54Z

/retest

repokitteh-read-only · 2021-11-17T14:03:57Z

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

Caused by: a #18628 (comment) was created by @pradeepcrao.

see: more, trace.

Signed-off-by: Pradeep Rao <[email protected]>

ggreenway

This is looking pretty good. A few more tasks:

Please convert all tests that use match_subject_alt_names to the new config (so that removing the deprecated config in the future is painless)
Add a small amount of test coverage to ensure the now-deprecated match_subject_alt_names works as expected, eg it matches on any of the SAN types.

/wait

ggreenway · 2021-11-17T23:00:11Z

api/envoy/extensions/transport_sockets/tls/v3/common.proto

+  }
+
+  // Specification of type of SAN. Note that the default enum value is an invalid choice.
+  SanType san_type = 1 [(validate.rules).enum = {defined_only: true not_in: 0}];


I apologize; I totally missed that while reading this. Looks good; sorry for the hassle.

source/common/ssl/certificate_validation_context_config_impl.cc

ggreenway · 2021-11-17T23:38:49Z

source/extensions/transport_sockets/tls/cert_validator/san_matcher_config.cc

@@ -0,0 +1,47 @@
+#include "source/extensions/transport_sockets/tls/cert_validator/san_matcher_config.h"


Rename files: san_matcher.cc/h

ggreenway · 2021-11-17T23:42:28Z

source/extensions/transport_sockets/tls/cert_validator/default_validator.cc

@@ -290,24 +290,28 @@ bool DefaultCertValidator::verifySubjectAltName(X509* cert,
  return false;
 }

+bool DefaultCertValidator::verifySubjectAltName(


Should this function by part of SanMatcher? It's only called from there.

ggreenway · 2021-11-17T23:57:14Z

test/extensions/transport_sockets/tls/cert_validator/san_matcher_config_test.cc

+  san_matcher.set_san_type(
+      envoy::extensions::transport_sockets::tls::v3::
+          SubjectAltNameMatcher_SanType_SubjectAltNameMatcher_SanType_INT_MIN_SENTINEL_DO_NOT_USE_);
+  const SanMatcherPtr matcher = createStringSanMatcher(san_matcher);


The invalid enum shouldn't be possible due to proto validation. And even if it were possible, shouldn't this hit the RELEASE_ASSERT in this function? I don't think this test case is needed.

I added the test case because CI failed due to of coverage numbers reducing (as NOT_REACHED_GCOVR_EXCL_LINE does not work as advertised anymore).

ggreenway · 2021-11-17T23:59:08Z

source/extensions/transport_sockets/tls/cert_validator/san_matcher_config.cc

+  case envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::IP_ADDRESS:
+    return Envoy::Ssl::SanMatcherPtr{std::make_unique<IpAddSanMatcher>(matcher.matcher())};
+  default:
+    RELEASE_ASSERT(true, "Invalid san type for "


This should be RELEASE_ASSERT(false, ......)

Signed-off-by: Pradeep Rao <[email protected]>

pradeepcrao · 2021-11-18T19:47:15Z

source/extensions/transport_sockets/tls/cert_validator/san_matcher.cc

+  case envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::IP_ADDRESS:
+    return SanMatcherPtr{std::make_unique<StringSanMatcher>(GEN_IPADD, matcher.matcher())};
+  default:
+    NOT_REACHED_GCOVR_EXCL_LINE;


As expected this leads to a reduction in coverage by 0.1% and causes CI to fail.
https://storage.googleapis.com/envoy-pr/fd2b974/coverage/source/extensions/transport_sockets/tls/cert_validator/san_matcher.cc.gcov.html

@ggreenway do you know of a way I can overcome this?

You can modify test/per_file_coverage.sh to lower the required coverage.

Signed-off-by: Pradeep Rao <[email protected]>

adisuissa

/lgtm api

pradeepcrao · 2021-11-19T14:13:06Z

/retest

repokitteh-read-only · 2021-11-19T14:13:10Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18628 (comment) was created by @pradeepcrao.

see: more, trace.

pradeepcrao · 2021-11-23T14:02:49Z

Hi @ggreenway, does this look good now? Do you have any pending concerns?

All requested changes have been addressed

* main: (77 commits) Fix verify_and_print_latest_release logic (envoyproxy#19111) http2: drain only once when reached max_requests_per_connection (envoyproxy#19078) Overload: Reset H2 server stream only use codec level reset mechanism (envoyproxy#18895) Update QUICHE from c2ddf95dc to 7f2d442e3 (envoyproxy#19095) tools: Fix dependency checker release dates bug (envoyproxy#19109) cve_scan: Use `envoy.dependency.cve_scan` (envoyproxy#19047) tcp: fix overenthusiastic bounds on the new pool (envoyproxy#19036) dep: update Proxy-Wasm C++ host (2021-11-18). (envoyproxy#19074) build(deps): bump frozendict from 2.0.7 to 2.1.0 in /tools/base (envoyproxy#19080) kafka: dependency upgrades (envoyproxy#18995) build(deps): bump charset-normalizer in /tools/dependency (envoyproxy#19105) build(deps): bump slack-sdk in /.github/actions/pr_notifier (envoyproxy#19093) dep: Remove dependency - six (envoyproxy#19085) Remove requested_server_name_ field from StreamInfo (envoyproxy#19102) broken link path fix for items http_filters/grpc_json_transcoder_filter (envoyproxy#19101) quic: turn off GRO (envoyproxy#19088) Listener: Add global conn limit opt out. (envoyproxy#18876) Specify type for matching Subject Alternative Name. (envoyproxy#18628) Fix a broken example in Lua filter docs (envoyproxy#19086) Fix a small typo (envoyproxy#19058) ... Signed-off-by: Michael Puncel <[email protected]>

Specify type for matching Subject Alternative Name.

3c47ebb

Signed-off-by: Pradeep Rao <[email protected]>

pradeepcrao requested review from asraa, ggreenway and lizan as code owners October 14, 2021 15:13

repokitteh-read-only bot added the api label Oct 14, 2021

repokitteh-read-only bot assigned adisuissa Oct 14, 2021

ggreenway reviewed Oct 14, 2021

View reviewed changes

pradeepcrao added 4 commits October 14, 2021 17:18

Fix build and presubmit issues

a554236

Signed-off-by: Pradeep Rao <[email protected]>

Merge remote-tracking branch 'upstream/main' into san_matcher

9d3a7e9

Signed-off-by: Pradeep Rao <[email protected]>

fix build

3d6272e

Signed-off-by: Pradeep Rao <[email protected]>

fix whitespace

f3c87a6

Signed-off-by: Pradeep Rao <[email protected]>

adisuissa requested changes Oct 15, 2021

View reviewed changes

adisuissa reviewed Oct 15, 2021

View reviewed changes

davinci26 added the waiting label Oct 15, 2021

pradeepcrao added 2 commits October 17, 2021 23:51

Added StringSanMatcher message, and the ability to provide either

ff9cd5f

StringSanMatchers or TypedExtensionConfigs for san matchers. Made test regexes stricter. Signed-off-by: Pradeep Rao <[email protected]>

Merge remote-tracking branch 'upstream/main' into san_matcher

7287a34

Signed-off-by: Pradeep Rao <[email protected]>

repokitteh-read-only bot removed the waiting label Oct 17, 2021

pradeepcrao added 3 commits October 18, 2021 13:41

Fix formatting

3aae6db

Signed-off-by: Pradeep Rao <[email protected]>

Fix formatting.

ab5cd11

Signed-off-by: Pradeep Rao <[email protected]>

Fix formatting

135391e

Signed-off-by: Pradeep Rao <[email protected]>

adisuissa reviewed Oct 18, 2021

View reviewed changes

pradeepcrao added 2 commits October 18, 2021 20:49

Fix include

2a65d30

Signed-off-by: Pradeep Rao <[email protected]>

Merge remote-tracking branch 'upstream/main' into san_matcher

086ece5

Signed-off-by: Pradeep Rao <[email protected]>

yanavlasov self-assigned this Oct 19, 2021

pradeepcrao added 3 commits October 19, 2021 18:30

Merge remote-tracking branch 'upstream/main' into san_matcher

42c2f75

Signed-off-by: Pradeep Rao <[email protected]>

Fix merge issue

e028388

Signed-off-by: Pradeep Rao <[email protected]>

Fix formatting.

cebac65

Signed-off-by: Pradeep Rao <[email protected]>

repokitteh-read-only bot added the waiting label Nov 15, 2021

pradeepcrao added 2 commits November 16, 2021 20:53

Address feedback.

36226ea

Signed-off-by: Pradeep Rao <[email protected]>

Merge remote-tracking branch 'upstream/main' into san_matcher

133a6a8

Signed-off-by: Pradeep Rao <[email protected]>

pradeepcrao dismissed adisuissa’s stale review via 133a6a8 November 16, 2021 21:02

repokitteh-read-only bot added api and removed waiting labels Nov 16, 2021

Merge remote-tracking branch 'upstream/main' into san_matcher

c5b05ab

Signed-off-by: Pradeep Rao <[email protected]>

ggreenway previously requested changes Nov 18, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Nov 18, 2021

pradeepcrao added 2 commits November 18, 2021 17:17

Address feedback.

05fb9b0

Signed-off-by: Pradeep Rao <[email protected]>

Merge remote-tracking branch 'upstream/main' into san_matcher

d97b59f

Signed-off-by: Pradeep Rao <[email protected]>

repokitteh-read-only bot removed the waiting label Nov 18, 2021

pradeepcrao commented Nov 18, 2021

View reviewed changes

Modify coverage to account for unreached line.

6ceb50d

Signed-off-by: Pradeep Rao <[email protected]>

adisuissa reviewed Nov 19, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Nov 19, 2021

yanavlasov approved these changes Nov 24, 2021

View reviewed changes

yanavlasov merged commit bb95af8 into envoyproxy:main Nov 24, 2021

adisuissa mentioned this pull request Jan 10, 2022

TLS: allow specification of both type and value for SAN matcher #18259

Closed

AmaliMatharaarachchi mentioned this pull request Mar 26, 2022

Remove deprecated messages wso2/product-microgateway#2775

Closed

adisuissa mentioned this pull request Mar 27, 2022

TLS: Allow specification of both typed and non-typed san matchers in config #20529

Merged

arulthileeban mentioned this pull request May 24, 2024

Allow OtherName type in SAN matching #34358

Closed

ggreenway mentioned this pull request Jun 18, 2024

tls: Add support for matching against OtherName SAN type #34471

Merged

	#include "envoy/extensions//transport_sockets/tls/v3/common.pb.h"
	#include "envoy/extensions/transport_sockets/tls/v3/common.pb.h"

		@@ -0,0 +1,47 @@
		#include "source/extensions/transport_sockets/tls/cert_validator/san_matcher_config.h"

Specify type for matching Subject Alternative Name. #18628

Specify type for matching Subject Alternative Name. #18628

Conversation

pradeepcrao commented Oct 14, 2021 • edited Loading

repokitteh-read-only bot commented Oct 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adisuissa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradeepcrao commented Oct 15, 2021

adisuissa commented Oct 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradeepcrao commented Nov 17, 2021

repokitteh-read-only bot commented Nov 17, 2021

ggreenway left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradeepcrao Nov 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adisuissa left a comment

Choose a reason for hiding this comment

pradeepcrao commented Nov 19, 2021

repokitteh-read-only bot commented Nov 19, 2021

pradeepcrao commented Nov 23, 2021

pradeepcrao commented Oct 14, 2021 •

edited

Loading

pradeepcrao Nov 18, 2021 •

edited

Loading