Specify type for matching Subject Alternative Name. #18628
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ import "envoy/type/matcher/v3/string.proto"; | |
| import "google/protobuf/any.proto"; | ||
| import "google/protobuf/wrappers.proto"; | ||
|
|
||
| import "envoy/annotations/deprecation.proto"; | ||
| import "udpa/annotations/migrate.proto"; | ||
| import "udpa/annotations/sensitive.proto"; | ||
| import "udpa/annotations/status.proto"; | ||
|
|
@@ -253,7 +254,29 @@ message CertificateProviderPluginInstance { | |
| string certificate_name = 2; | ||
| } | ||
|
|
||
| // String matcher for subject alternative names, to match both type and value of the SAN. | ||
| message StringSanMatcher { | ||
| enum SanType { | ||
| EMAIL_ID = 0; | ||
| DNS_ID = 1; | ||
| URI_ID = 2; | ||
|
mathetake marked this conversation as resolved.
Outdated
|
||
| IP_ADD = 3; | ||
|
|
||
| } | ||
|
|
||
| SanType san_type = 1 [(validate.rules).enum.defined_only = true]; | ||
|
|
||
| type.matcher.v3.StringMatcher matcher = 2; | ||
|
Contributor
There was a problem hiding this comment. I think this can be annotated with
Contributor
Author
There was a problem hiding this comment. Modified existing rule to exclude default value. Added comments.
Contributor
There was a problem hiding this comment. IIUC this allows a config with a defined |
||
| } | ||
|
|
||
| message SubjectAltNameMatcher { | ||
|
Contributor
There was a problem hiding this comment. Please add comments describing the message and field. |
||
| oneof matcher { | ||
| StringSanMatcher string_matcher = 1; | ||
|
|
||
| config.core.v3.TypedExtensionConfig typed_config = 2; | ||
|
Member
There was a problem hiding this comment. Do we need this extension point? If someone needs to extend, can they just do it as
Contributor
Author
There was a problem hiding this comment. To me, it makes more sense to have the extension point, as the string matcher does not work for all san types, and it is specifically only the san matcher that needs special handling in those cases, and not the entire validator config. Does that sound like a reasonable argument to you?
Member
There was a problem hiding this comment. Is anyone actually going to write an extension for this extension point? Or is it just hypothetical need?
Contributor
Author
There was a problem hiding this comment. The extension is needed to support OtherName as defined on page 38 here https://www.rfc-editor.org/rfc/pdfrfc/rfc5280.txt.pdf, which can be any user-defined type. As of now Envoy does not support this, but we will need to add it in the future if we would want to support SRV-ID or UPN (User Principle Name) which are othernames.
Member
There was a problem hiding this comment. Unless someone plans to add that support in the immediate future, I'd rather not add it until it's used. If it is added and never used, it creates extra maintenance burden for no gain.
Contributor
Author
There was a problem hiding this comment. Sounds good to me. |
||
| } | ||
| } | ||
|
|
||
| // [#next-free-field: 15] | ||
| message CertificateValidationContext { | ||
| option (udpa.annotations.versioning).previous_message_type = | ||
| "envoy.api.v2.auth.CertificateValidationContext"; | ||
|
|
@@ -388,6 +411,8 @@ message CertificateValidationContext { | |
|
|
||
| // An optional list of Subject Alternative name matchers. If specified, Envoy will verify that the | ||
| // Subject Alternative Name of the presented certificate matches one of the specified matchers. | ||
| // The matching uses "any" semantics, that is to say, the SAN is verified if at least one matcher is | ||
| // matched. | ||
| // | ||
| // When a certificate has wildcard DNS SAN entries, to match a specific client, it should be | ||
| // configured with exact match type in the :ref:`string matcher <envoy_v3_api_msg_type.matcher.v3.StringMatcher>`. | ||
|
|
@@ -396,15 +421,23 @@ message CertificateValidationContext { | |
| // | ||
| // .. code-block:: yaml | ||
| // | ||
| // match_subject_alt_names_with_type: | ||
| // string_matcher: | ||
| // san_type: DNS_ID | ||
|
|
||
| // matcher: | ||
| // 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>`. | ||
|
Contributor
There was a problem hiding this comment. 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) |
||
| repeated SubjectAltNameMatcher match_subject_alt_names_with_type = 14; | ||
|
Member
There was a problem hiding this comment. An alternative API that I think is simpler is to have a message of StringMatcher and an enum for the type of match. Thoughts?
Contributor
Author
There was a problem hiding this comment. 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.
Contributor
There was a problem hiding this comment. 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
Contributor
There was a problem hiding this comment. 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 cc: @howardjohn
Contributor
Author
There was a problem hiding this comment. Hi Rama, Howard,
Member
There was a problem hiding this comment. Consider renaming to 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. |
||
|
|
||
| // This field is deprecated in favor of ref:`match_subject_alt_names_with_type | ||
| // <envoy_v3_api_field_extensions.transport_sockets.tls.v3.CertificateValidationContext.match_subject_alt_names_with_type>` | ||
| repeated type.matcher.v3.StringMatcher match_subject_alt_names = 9 | ||
| [deprecated = true, (envoy.annotations.deprecated_at_minor_version) = "3.0"]; | ||
|
|
||
| // [#not-implemented-hide:] Must present signed certificate time-stamp. | ||
| google.protobuf.BoolValue require_signed_certificate_timestamp = 6; | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -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" | ||||||
|
Contributor
There was a problem hiding this comment. I think this is what causing your format error.
Suggested change
Contributor
Author
There was a problem hiding this comment. Thanks! Great catch! |
||||||
| #include "envoy/extensions/transport_sockets/tls/v3/cert.pb.h" | ||||||
|
|
||||||
| #include "source/common/common/empty_string.h" | ||||||
| #include "source/common/common/fmt.h" | ||||||
| #include "source/common/common/logger.h" | ||||||
| #include "source/common/config/datasource.h" | ||||||
|
|
||||||
| namespace Envoy { | ||||||
|
|
@@ -22,8 +25,7 @@ CertificateValidationContextConfigImpl::CertificateValidationContextConfigImpl( | |||||
| certificate_revocation_list_path_( | ||||||
| Config::DataSource::getPath(config.crl()) | ||||||
| .value_or(certificate_revocation_list_.empty() ? EMPTY_STRING : INLINE_STRING)), | ||||||
| subject_alt_name_matchers_(getSubjectAltNameMatchers(config)), | ||||||
| verify_certificate_hash_list_(config.verify_certificate_hash().begin(), | ||||||
| config.verify_certificate_hash().end()), | ||||||
| verify_certificate_spki_list_(config.verify_certificate_spki().begin(), | ||||||
|
|
@@ -36,6 +38,7 @@ CertificateValidationContextConfigImpl::CertificateValidationContextConfigImpl( | |||||
| config.custom_validator_config()) | ||||||
| : absl::nullopt), | ||||||
| api_(api) { | ||||||
|
|
||||||
| if (ca_cert_.empty() && custom_validator_config_ == absl::nullopt) { | ||||||
| if (!certificate_revocation_list_.empty()) { | ||||||
| throw EnvoyException(fmt::format("Failed to load CRL from {} without trusted CA", | ||||||
|
|
@@ -51,5 +54,32 @@ CertificateValidationContextConfigImpl::CertificateValidationContextConfigImpl( | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| std::vector<envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher> | ||||||
| CertificateValidationContextConfigImpl::getSubjectAltNameMatchers( | ||||||
| const envoy::extensions::transport_sockets::tls::v3::CertificateValidationContext& config) { | ||||||
| if (!config.match_subject_alt_names_with_type().empty() && | ||||||
| !config.match_subject_alt_names().empty()) { | ||||||
| throw EnvoyException("SAN-based verification using both match_subject_alt_names_with_type and " | ||||||
| "the deprecated match_subject_alt_names is not allowed"); | ||||||
| } | ||||||
| std::vector<envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher> | ||||||
| subject_alt_name_matchers(config.match_subject_alt_names_with_type().begin(), | ||||||
| config.match_subject_alt_names_with_type().end()); | ||||||
| // Handle deprecated string type san matchers without san type specified, by | ||||||
| // creating backwards compatible san matcher configs. | ||||||
| for (auto& matcher : config.match_subject_alt_names()) { | ||||||
| subject_alt_name_matchers.emplace_back(); | ||||||
|
Contributor
There was a problem hiding this comment. You may be able to iterate over the values [
Contributor
Author
There was a problem hiding this comment. I'd rather be explicit here. If new enum types are added in the future, those should NOT be iterated over in this loop, as this is for backwards compatibility ONLY.
Contributor
There was a problem hiding this comment. So in this case I suggest to refactor the code to iterate over the specific set of enum values, and add them (decrease code duplication). |
||||||
| subject_alt_name_matchers.back().set_allocated_typed_config( | ||||||
| new ::envoy::config::core::v3::TypedExtensionConfig()); | ||||||
| subject_alt_name_matchers.back().mutable_typed_config()->set_allocated_typed_config( | ||||||
| new ProtobufWkt::Any()); | ||||||
| subject_alt_name_matchers.back().mutable_typed_config()->mutable_typed_config()->PackFrom( | ||||||
| matcher); | ||||||
| subject_alt_name_matchers.back().mutable_typed_config()->mutable_name()->assign( | ||||||
| "envoy.san_matchers.backward_compatible_san_matcher"); | ||||||
|
Contributor
There was a problem hiding this comment. Instead of allocating new objects you should be able to create the TypedExtensionConfig object on the stack, and pack it into the
Contributor
Author
There was a problem hiding this comment. Aah, didn't realise that |
||||||
| } | ||||||
| return subject_alt_name_matchers; | ||||||
| } | ||||||
|
|
||||||
| } // namespace Ssl | ||||||
| } // namespace Envoy | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment here with the link to the relevant RFC section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comment.