cert validation: use Android cert validation APIs #2525
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -124,6 +124,12 @@ EngineBuilder& EngineBuilder::enableSocketTagging(bool socket_tagging_on) { | |
| return *this; | ||
| } | ||
|
|
||
| EngineBuilder& | ||
| EngineBuilder::enablePlatformCertificatesValidation(bool platform_certificates_validation_on) { | ||
|
There was a problem hiding this comment. What will happen if we enable this feature on iOS? Will Envoy work? There was a problem hiding this comment. Please note that I did not suggest the use of If you agree with a proposed change, we can make it and drop a TODO (you can assign to me) for changing There was a problem hiding this comment. I realize that I didn't even register the platform API in this engine. Adding it right now. There was a problem hiding this comment. Okay, I moved the API registration from JniLibrary interface to JNI_OnLoad() which is also called when using C++ engine. |
||
| this->platform_certificates_validation_on_ = platform_certificates_validation_on; | ||
| return *this; | ||
| } | ||
|
|
||
| std::string EngineBuilder::generateConfigStr() { | ||
| #if defined(__APPLE__) | ||
| std::string dns_resolver_name = "envoy.network.dns_resolver.apple"; | ||
|
|
@@ -200,6 +206,14 @@ std::string EngineBuilder::generateConfigStr() { | |
| config_builder << config_template_; | ||
|
|
||
| auto config_str = config_builder.str(); | ||
|
|
||
| // Choose the right certification validator. | ||
| absl::StrReplaceAll( | ||
| {{"{{custom_cert_validation_context}}", | ||
| (this->platform_certificates_validation_on_ ? platform_cert_validation_context_template | ||
| : default_cert_validation_context_template)}}, | ||
| &config_str); | ||
|
|
||
| if (config_str.find("{{") != std::string::npos) { | ||
| throw std::runtime_error("could not resolve all template keys in config"); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,6 +75,14 @@ const char* brotli_config_insert = R"( | |
| ignore_no_transform_header: true | ||
| )"; | ||
|
|
||
| const char* platform_cert_validation_context_template = R"( | ||
| custom_validator_config: | ||
| name: "envoy_mobile.cert_validator.platform_bridge_cert_validator")"; | ||
|
|
||
| const char* default_cert_validation_context_template = R"( | ||
| trusted_ca: | ||
| inline_string: *tls_root_certs)"; | ||
|
|
||
| const char* socket_tag_config_insert = R"( | ||
| - name: envoy.filters.http.socket_tag | ||
| typed_config: | ||
|
|
@@ -149,24 +157,7 @@ R"(- &enable_drain_post_dns_refresh false | |
| !ignore tls_root_ca_defs: &tls_root_certs | | ||
| )" | ||
| #include "certificates.inc" | ||
Augustyniak marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ; | ||
|
|
||
| const char* config_template = R"( | ||
| !ignore local_error_defs: &local_error_config | ||
|
|
@@ -280,6 +271,46 @@ const char* config_template = R"( | |
| typed_config: | ||
| "@type": type.googleapis.com/envoy.extensions.filters.http.router.v3.Router | ||
|
|
||
| !ignore tls_socket_defs: | ||
| - &validation_context {{custom_cert_validation_context}} | ||
|
There was a problem hiding this comment. The main config ( There was a problem hiding this comment. Hmm, if I change it to use I see that There was a problem hiding this comment. I think that there is a value in keeping the config to be a valid YAML at all times (even if we do not replace some substrings that are intended to be replaces). I think that we should not resign from the current status quo without having strong reasons to do so since I am quite positive that the current set up makes it easier to work with multiple separate builder (Swift, Kotlin and c++). If we learn that it's not possible to do so we can change our approach in here. With that being said, would it be possible to have and make builders (Kotlin, c++) to conditionally add the following line(s): I think that for this to work you would need to move Does it make sense? Could it work for what you a re trying to accomplish? There was a problem hiding this comment. Can you explain a bit more about this? Are you suggesting to have 2 There was a problem hiding this comment. Sure, let me try to explain my understanding of our YAML setup. So when you have more than one 'definition' (YAML anchor) such as
Now about the It is fine that that engine builder does not modify the So looking at ^ you can provide a default value in 1): override it in 2) as needed (conditionally) - by either adding or not adding the following string: and this is what will be visible to YAML in 3). There was a problem hiding this comment. Thanks for the suggestion! It basically worked. One small wrinkle here is that I had to also append when we don't config platform cert validator, even though it's duplicated with config_header. This is because &trust_chain_verification gets overridden. There was a problem hiding this comment.
Ah, I see - so it's a little bit more tricky here. Nice catch 👍 There was a problem hiding this comment. After a little bit more of thinking. Should we just move (That would allow you to avoid having the prepend There was a problem hiding this comment. I deprecated |
||
| - &validation_context_config_trust_chain {{custom_cert_validation_context}} | ||
|
There was a problem hiding this comment. Could we replace There was a problem hiding this comment. Basically, I wonder whether we can remove the number of places where we reference |
||
| trust_chain_verification: *trust_chain_verification | ||
|
|
||
| - &base_tls_socket | ||
| name: envoy.transport_sockets.http_11_proxy | ||
| typed_config: | ||
| "@type": type.googleapis.com/envoy.extensions.transport_sockets.http_11_proxy.v3.Http11ProxyUpstreamTransport | ||
| transport_socket: | ||
| name: envoy.transport_sockets.tls | ||
| typed_config: | ||
| "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.UpstreamTlsContext | ||
| common_tls_context: | ||
| tls_params: | ||
| tls_maximum_protocol_version: TLSv1_3 | ||
| validation_context: *validation_context | ||
| - &base_h2_socket | ||
| name: envoy.transport_sockets.http_11_proxy | ||
| typed_config: | ||
| "@type": type.googleapis.com/envoy.extensions.transport_sockets.http_11_proxy.v3.Http11ProxyUpstreamTransport | ||
| transport_socket: | ||
| name: envoy.transport_sockets.tls | ||
| typed_config: | ||
| "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.UpstreamTlsContext | ||
| common_tls_context: | ||
| alpn_protocols: [h2] | ||
| tls_params: | ||
| tls_maximum_protocol_version: TLSv1_3 | ||
| validation_context: *validation_context_config_trust_chain | ||
| - &base_h3_socket | ||
| name: envoy.transport_sockets.quic | ||
| typed_config: | ||
| "@type": type.googleapis.com/envoy.extensions.transport_sockets.quic.v3.QuicUpstreamTransport | ||
| upstream_tls_context: | ||
| common_tls_context: | ||
| tls_params: | ||
| tls_maximum_protocol_version: TLSv1_3 | ||
| validation_context: *validation_context_config_trust_chain | ||
|
|
||
| !ignore custom_cluster_defs: | ||
| stats_cluster: &stats_cluster | ||
| name: stats | ||
|
|
@@ -429,41 +460,15 @@ const char* config_template = R"( | |
| connect_timeout: *connect_timeout | ||
| lb_policy: CLUSTER_PROVIDED | ||
| cluster_type: *base_cluster_type | ||
| transport_socket: *base_h2_socket | ||
|
|
||
| upstream_connection_options: *upstream_opts | ||
| circuit_breakers: *circuit_breakers_settings | ||
| typed_extension_protocol_options: *h2_protocol_options | ||
| - name: base_h3 | ||
| connect_timeout: *connect_timeout | ||
| lb_policy: CLUSTER_PROVIDED | ||
| cluster_type: *base_cluster_type | ||
| transport_socket: *base_h3_socket | ||
| upstream_connection_options: *upstream_opts | ||
| circuit_breakers: *circuit_breakers_settings | ||
| typed_extension_protocol_options: *h3_protocol_options | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| load( | ||
| "@envoy//bazel:envoy_build_system.bzl", | ||
| "envoy_cc_extension", | ||
| "envoy_cc_library", | ||
| "envoy_extension_package", | ||
| ) | ||
|
|
||
| licenses(["notice"]) # Apache 2 | ||
|
|
||
| envoy_extension_package() | ||
|
|
||
| envoy_cc_library( | ||
| name = "c_types_lib", | ||
| hdrs = ["c_types.h"], | ||
| repository = "@envoy", | ||
| deps = [ | ||
| "//library/common/data:utility_lib", | ||
| ], | ||
| ) | ||
|
|
||
| envoy_cc_library( | ||
| name = "platform_bridge_cert_validator_lib", | ||
| srcs = ["platform_bridge_cert_validator.cc"], | ||
| hdrs = [ | ||
| "platform_bridge_cert_validator.h", | ||
| ], | ||
| repository = "@envoy", | ||
| deps = [ | ||
| ":c_types_lib", | ||
| "@envoy//source/extensions/transport_sockets/tls/cert_validator:cert_validator_lib", | ||
| ], | ||
| ) | ||
|
|
||
| envoy_cc_extension( | ||
| name = "config", | ||
| srcs = ["config.cc"], | ||
| hdrs = [ | ||
| "c_types.h", | ||
| "config.h", | ||
| ], | ||
| repository = "@envoy", | ||
| deps = [ | ||
| ":platform_bridge_cert_validator_lib", | ||
| "//library/common/api:external_api_lib", | ||
| "//library/common/data:utility_lib", | ||
| "@envoy//envoy/registry", | ||
| ], | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| #pragma once | ||
|
|
||
| #include "library/common/types/c_types.h" | ||
|
|
||
| // NOLINT(namespace-envoy) | ||
|
|
||
| /** | ||
| * Certification validation binary result with corresponding boring SSL alert | ||
| * and error details if the result indicates failure. | ||
| */ | ||
| typedef struct { | ||
|
There was a problem hiding this comment. Comments on all the structs/functions, please |
||
| envoy_status_t result; | ||
| uint8_t tls_alert; | ||
| const char* error_details; | ||
| } envoy_cert_validation_result; | ||
|
|
||
| #ifdef __cplusplus | ||
|
There was a problem hiding this comment. when would we expect this to be false? Asking mostly so that I understand why it's needed. There was a problem hiding this comment. I actually don't know, but I saw this has been used in all c_types.h to wrap the C interface. There was a problem hiding this comment. It's necessary because the public headers are both valid C++ and pure C headers. When called publicly (e.g. from the built-in iOS and Android platform layers), they're bound as C functions. When the core layer is compiled though, everything is C++. |
||
| extern "C" { // function pointers | ||
| #endif | ||
|
|
||
| /** | ||
| * Function signature for calling into platform APIs to validate certificates. | ||
| */ | ||
| typedef envoy_cert_validation_result (*envoy_validate_cert_f)(const envoy_data* certs, uint8_t size, | ||
| const char* host_name); | ||
|
|
||
| /** | ||
| * Function signature for calling into platform APIs to clean up after validation completion. | ||
| */ | ||
| typedef void (*envoy_release_validator_f)(); | ||
|
|
||
| #ifdef __cplusplus | ||
| } // function pointers | ||
| #endif | ||
|
|
||
| /** | ||
| * A bag of function pointers to be registered in the platform registry. | ||
| */ | ||
| typedef struct { | ||
| envoy_validate_cert_f validate_cert; | ||
| envoy_release_validator_f release_validator; | ||
| } envoy_cert_validator; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| #include "library/common/extensions/cert_validator/platform_bridge/config.h" | ||
|
|
||
| #include "library/common/api/external.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Extensions { | ||
| namespace TransportSockets { | ||
| namespace Tls { | ||
|
|
||
| CertValidatorPtr PlatformBridgeCertValidatorFactory::createCertValidator( | ||
| const Envoy::Ssl::CertificateValidationContextConfig* config, SslStats& stats, | ||
| TimeSource& /*time_source*/) { | ||
| platform_validator_ = | ||
| static_cast<envoy_cert_validator*>(Api::External::retrieveApi("platform_cert_validator")); | ||
|
There was a problem hiding this comment. Considering you may want your own guard here to ensure that the implementation is found, just for safety.There was a problem hiding this comment. This would be the bad config case. Would it make sense to throw or PANIC as this is still during start up? |
||
| return std::make_unique<PlatformBridgeCertValidator>(config, stats, platform_validator_); | ||
| } | ||
|
|
||
| REGISTER_FACTORY(PlatformBridgeCertValidatorFactory, CertValidatorFactory); | ||
|
|
||
| } // namespace Tls | ||
| } // namespace TransportSockets | ||
| } // namespace Extensions | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| #include "envoy/registry/registry.h" | ||
|
|
||
| #include "source/extensions/transport_sockets/tls/cert_validator/factory.h" | ||
|
|
||
| #include "library/common/extensions/cert_validator/platform_bridge/platform_bridge_cert_validator.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Extensions { | ||
| namespace TransportSockets { | ||
| namespace Tls { | ||
|
|
||
| class PlatformBridgeCertValidatorFactory : public CertValidatorFactory { | ||
| public: | ||
| CertValidatorPtr createCertValidator(const Envoy::Ssl::CertificateValidationContextConfig* config, | ||
| SslStats& stats, TimeSource& time_source) override; | ||
|
|
||
| std::string name() const override { | ||
| return "envoy_mobile.cert_validator.platform_bridge_cert_validator"; | ||
| } | ||
|
|
||
| private: | ||
| const envoy_cert_validator* platform_validator_; | ||
| }; | ||
|
|
||
| DECLARE_FACTORY(PlatformBridgeCertValidatorFactory); | ||
|
|
||
| } // namespace Tls | ||
| } // namespace TransportSockets | ||
| } // namespace Extensions | ||
| } // 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.
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.
done