Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cert validation: use Android cert validation APIs #2525

Merged
merged 50 commits into from
Oct 18, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
f0a846a
build cert validator
danzh1989 Aug 18, 2022
464f365
remove allow expired cert
danzh1989 Aug 19, 2022
7edc2ef
add http2 test
danzh1989 Aug 23, 2022
d8e9c6d
host name override
danzh1989 Aug 23, 2022
c9894a1
platform_name str
danzh1989 Aug 25, 2022
b1f9a04
Merge branch 'main' into certvalidator
danzh1989 Aug 26, 2022
79443ee
verify leaf in thread
danzh1989 Aug 29, 2022
b40b230
return value
danzh1989 Aug 30, 2022
93ab2ed
globalref
danzh1989 Aug 30, 2022
1e4f295
all tests pass
danzh1989 Sep 2, 2022
e13b72e
EnvoyConfigurationTest pass
danzh1989 Sep 6, 2022
c8fdf91
release note
danzh1989 Sep 6, 2022
ffa0c1b
Merge branch 'main' into certvalidator
danzh1989 Sep 6, 2022
0a8a474
remove unrelated chagne
danzh1989 Sep 6, 2022
9012dbb
fix tests
danzh1989 Sep 6, 2022
c6cea9d
Merge branch 'main' into certvalidator
danzh1989 Sep 6, 2022
27129eb
fix object-c builder
danzh1989 Sep 7, 2022
8384259
format
danzh1989 Sep 7, 2022
3de8d89
use FakeX509Util in CronetTestRule
danzh1989 Sep 9, 2022
658a340
address comments
danzh1989 Sep 9, 2022
403eca1
Update submodule
danzh1989 Sep 9, 2022
3a02268
fix build
danzh1989 Sep 12, 2022
2154876
revert wrong format change
danzh1989 Sep 12, 2022
2f1e156
fix build again
danzh1989 Sep 12, 2022
e5a6bcc
fix hostname usage
danzh1989 Sep 12, 2022
47ea600
fix doc
danzh1989 Sep 12, 2022
0e3d5ef
Merge branch 'main' into certvalidator
danzh1989 Sep 20, 2022
76bd20e
refine threading model
danzh1989 Sep 20, 2022
26b754f
use absl::string_view
danzh1989 Sep 21, 2022
c680e44
Merge branch 'main' into certvalidator
danzh1989 Sep 21, 2022
5fc2770
address comments
danzh1989 Sep 23, 2022
d9c4c0f
change config template
danzh1989 Sep 27, 2022
d485003
use trace logging
danzh1989 Sep 28, 2022
853667f
Merge branch 'main' into certvalidator
danzh1989 Sep 28, 2022
b76a1ad
fix c++ and object-C engines
danzh1989 Sep 28, 2022
09433c4
revert unrelated format change
danzh1989 Sep 29, 2022
a481c44
fix c++ engine
danzh1989 Oct 3, 2022
406c24f
check platform
danzh1989 Oct 4, 2022
00ecfa4
fix build
danzh1989 Oct 4, 2022
66580a3
Merge branch 'main' into certvalidator
danzh1989 Oct 4, 2022
1a2cc8b
fix envoy_config_test
danzh1989 Oct 4, 2022
cebcea3
fix cpp engine and swift engine config
danzh1989 Oct 6, 2022
82f87c4
register platform api during initialize
danzh1989 Oct 12, 2022
ed215f0
Merge branch 'main' into certvalidator
danzh1989 Oct 12, 2022
c4b6198
unit test
danzh1989 Oct 12, 2022
7fd388b
revert unrelated change
danzh1989 Oct 12, 2022
61ab5c8
deps boringssl
danzh1989 Oct 12, 2022
3e9418e
split out cert verify calls
danzh1989 Oct 12, 2022
34526b3
add NOLINT
danzh1989 Oct 12, 2022
e815550
fix build
danzh1989 Oct 13, 2022
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
12 changes: 12 additions & 0 deletions docs/root/api/starting_envoy.rst
Original file line number Diff line number Diff line change
Expand Up @@ -514,6 +514,18 @@ to use IPv6. Note this is an experimental option and should be enabled with caut
builder.forceIPv6(true)


~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
``enablePlatformCertificatesValidation``
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Specify whether to use platform provided certificate validation interfaces. Currently only supported on Android. Defaults to false.

**Example**::

// Kotlin
builder.enablePlatformCertificatesValidation(true)


~~~~~~~~~~~~~~~~~~~~~~~~~~
``enableProxying``
~~~~~~~~~~~~~~~~~~~~~~~~~~
Expand Down
1 change: 1 addition & 0 deletions docs/root/intro/version_history.rst
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ Bugfixes:

Features:

- kotlin/c++: add option to support platform provided certificates validation interfaces on Android. (:issue `#2144 <2144>`)
- api: Add a ``setPerTryIdleTimeoutSeconds()`` method to C++ EngineBuilder.
- kotlin: add a way to tell Envoy Mobile to respect system proxy settings by calling an ``enableProxying(true)`` method on the engine builder. (:issue:`#2416 <2416>`)
- kotlin: add a ``enableSkipDNSLookupForProxiedRequests(true)`` knob for controlling whether Envoy waits on DNS response in the dynamic forward proxy filter for proxied requests. (:issue:`#2602 <2602>`)
Expand Down
1 change: 1 addition & 0 deletions envoy_build_config/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ envoy_cc_library(
"@envoy//source/extensions/transport_sockets/tls:config",
"@envoy//source/extensions/transport_sockets/tls/cert_validator:cert_validator_lib",
"@envoy//source/extensions/upstreams/http/generic:config",
"@envoy_mobile//library/common/extensions/cert_validator/platform_bridge:config",
"@envoy_mobile//library/common/extensions/filters/http/assertion:config",
"@envoy_mobile//library/common/extensions/filters/http/local_error:config",
"@envoy_mobile//library/common/extensions/filters/http/network_configuration:config",
Expand Down
2 changes: 2 additions & 0 deletions envoy_build_config/extension_registry.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include "source/extensions/upstreams/http/generic/config.h"

#include "extension_registry_platform_additions.h"
#include "library/common/extensions/cert_validator/platform_bridge/config.h"
#include "library/common/extensions/filters/http/assertion/config.h"
#include "library/common/extensions/filters/http/local_error/config.h"
#include "library/common/extensions/filters/http/network_configuration/config.h"
Expand Down Expand Up @@ -63,6 +64,7 @@ void ExtensionRegistry::registerFactories() {
Envoy::Extensions::TransportSockets::Http11Connect::
forceRegisterUpstreamHttp11ConnectSocketConfigFactory();
Envoy::Extensions::TransportSockets::Tls::forceRegisterDefaultCertValidatorFactory();
Envoy::Extensions::TransportSockets::Tls::forceRegisterPlatformBridgeCertValidatorFactory();
Envoy::Extensions::Upstreams::Http::Generic::forceRegisterGenericGenericConnPoolFactory();
Envoy::Upstream::forceRegisterLogicalDnsClusterFactory();
ExtensionRegistryPlatformAdditions::registerFactories();
Expand Down
1 change: 1 addition & 0 deletions envoy_build_config/extensions_build_config.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -25,5 +25,6 @@ EXTENSIONS = {
"envoy.transport_sockets.raw_buffer": "//source/extensions/transport_sockets/raw_buffer:config",
"envoy.transport_sockets.tls": "//source/extensions/transport_sockets/tls:config",
"envoy.http.stateful_header_formatters.preserve_case": "//source/extensions/http/header_formatters/preserve_case:config",
"envoy_mobile.cert_validator.platform_bridge_cert_validator": "@envoy_mobile//library/common/extensions/cert_validator/platform_bridge:config",
}
WINDOWS_EXTENSIONS = {}
1 change: 1 addition & 0 deletions library/cc/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ envoy_cc_library(
repository = "@envoy",
deps = [
":envoy_engine_cc_lib_no_stamp",
"@envoy//source/common/common:assert_lib",
],
)

Expand Down
19 changes: 19 additions & 0 deletions library/cc/engine_builder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

#include <sstream>

#include "source/common/common/assert.h"

#include "absl/strings/str_replace.h"
#include "fmt/core.h"
#include "library/common/main_interface.h"
Expand Down Expand Up @@ -129,6 +131,17 @@ EngineBuilder& EngineBuilder::enableSocketTagging(bool socket_tagging_on) {
return *this;
}

EngineBuilder&
EngineBuilder::enablePlatformCertificatesValidation(bool platform_certificates_validation_on) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What will happen if we enable this feature on iOS? Will Envoy work?

Copy link
Contributor

Choose a reason for hiding this comment

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

#if defined(__APPLE__)
    // Assert to make it clear that it's not supported
#elif 
   // this->platform_certificates_validation_on_ = platform_certificates_validation_on;
#endif

Please note that I did not suggest the use of if defined(__ANDROID_API__) even thought that would be probably more correct technically speaking. That's because it would make testing of this code harder than necessary (see #2450).

If you agree with a proposed change, we can make it and drop a TODO (you can assign to me) for changing #if defined(__APPLE__) to defined(__ANDROID_API__) once is addressed #2450

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realize that I didn't even register the platform API in this engine. Adding it right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I moved the API registration from JniLibrary interface to JNI_OnLoad() which is also called when using C++ engine.

#if defined(__APPLE__)
if (platform_certificates_validation_on) {
PANIC("Certificates validation using platform provided APIs is not supported in IOS.");
}
#endif
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";
Expand Down Expand Up @@ -185,6 +198,12 @@ std::string EngineBuilder::generateConfigStr() {
for (const auto& [key, value] : replacements) {
config_builder << "- &" << key << " " << value << std::endl;
}

const std::string& cert_validation_template =
(this->platform_certificates_validation_on_ ? platform_cert_validation_context_template
: default_cert_validation_context_template);
config_builder << cert_validation_template << std::endl;

if (this->gzip_filter_) {
absl::StrReplaceAll(
{{"#{custom_filters}", absl::StrCat("#{custom_filters}\n", gzip_config_insert)}},
Expand Down
2 changes: 2 additions & 0 deletions library/cc/engine_builder.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ class EngineBuilder {
EngineBuilder& enableGzip(bool gzip_on);
EngineBuilder& enableBrotli(bool brotli_on);
EngineBuilder& enableSocketTagging(bool socket_tagging_on);
EngineBuilder& enablePlatformCertificatesValidation(bool platform_certificates_validation_on);

// this is separated from build() for the sake of testability
std::string generateConfigStr();
Expand Down Expand Up @@ -84,6 +85,7 @@ class EngineBuilder {
bool gzip_filter_ = true;
bool brotli_filter_ = false;
bool socket_tagging_filter_ = false;
bool platform_certificates_validation_on_ = false;

absl::flat_hash_map<std::string, KeyValueStoreSharedPtr> key_value_stores_{};

Expand Down
107 changes: 60 additions & 47 deletions library/common/config/config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,20 @@ const char* brotli_config_insert = R"(
ignore_no_transform_header: true
)";

const char* default_cert_validation_context_template = R"(
- &validation_context
trusted_ca:
inline_string: *tls_root_certs
trust_chain_verification: *trust_chain_verification
)";

const char* platform_cert_validation_context_template = R"(
- &validation_context
custom_validator_config:
name: "envoy_mobile.cert_validator.platform_bridge_cert_validator"
trust_chain_verification: *trust_chain_verification
)";

const char* socket_tag_config_insert = R"(
- name: envoy.filters.http.socket_tag
typed_config:
Expand Down Expand Up @@ -143,21 +157,11 @@ R"(- &enable_drain_post_dns_refresh false
#include "certificates.inc"
R"(

!ignore tls_socket_defs:
Augustyniak marked this conversation as resolved.
Show resolved Hide resolved
- &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:
trusted_ca:
inline_string: *tls_root_certs
!ignore validation_context_defs:
- &validation_context
trusted_ca:
inline_string: *tls_root_certs
trust_chain_verification: *trust_chain_verification
)";

const char* config_template = R"(
Expand Down Expand Up @@ -272,6 +276,45 @@ const char* config_template = R"(
typed_config:
"@type": type.googleapis.com/envoy.extensions.filters.http.router.v3.Router

!ignore tls_socket_defs:
- &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
- &base_h3_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.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
!ignore custom_cluster_defs:
stats_cluster: &stats_cluster
name: stats
Expand Down Expand Up @@ -409,45 +452,15 @@ R"(
connect_timeout: *connect_timeout
lb_policy: CLUSTER_PROVIDED
cluster_type: *base_cluster_type
transport_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:
trusted_ca:
inline_string: *tls_root_certs
trust_chain_verification: *trust_chain_verification
transport_socket: *base_h2_socket
Copy link
Contributor

Choose a reason for hiding this comment

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

nice clean up 👍

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:
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.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:
trusted_ca:
inline_string: *tls_root_certs
trust_chain_verification: *trust_chain_verification
transport_socket: *base_h3_socket
upstream_connection_options: *upstream_opts
circuit_breakers: *circuit_breakers_settings
typed_extension_protocol_options: *h3_protocol_options
Expand Down
12 changes: 12 additions & 0 deletions library/common/config/templates.h
Original file line number Diff line number Diff line change
Expand Up @@ -85,3 +85,15 @@ extern const char* socket_tag_config_insert;
* direct responses to mutate headers which are then later used for routing.
*/
extern const char* route_cache_reset_filter_insert;

/**
* Config template which uses Envoy's built-in certificates validator to verify
* certificate chain.
*/
extern const char* default_cert_validation_context_template;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: comments, please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


/**
* Config template which uses platform's certificates APIs to verify certificate
* chain.
*/
extern const char* platform_cert_validation_context_template;
48 changes: 48 additions & 0 deletions library/common/extensions/cert_validator/platform_bridge/BUILD
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",
],
)
42 changes: 42 additions & 0 deletions library/common/extensions/cert_validator/platform_bridge/c_types.h
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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Comments on all the structs/functions, please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

envoy_status_t result;
uint8_t tls_alert;
const char* error_details;
} envoy_cert_validation_result;

#ifdef __cplusplus
Copy link
Contributor

Choose a reason for hiding this comment

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

when would we expect this to be false? Asking mostly so that I understand why it's needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually don't know, but I saw this has been used in all c_types.h to wrap the C interface.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's necessary because the public headers are both valid C++ and pure C headers. extern "C" is needed when compiling for C++ to prevent the compiler from munging the function names in publicly-exposed interface.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you @goaway 👍

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;
25 changes: 25 additions & 0 deletions library/common/extensions/cert_validator/platform_bridge/config.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
#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*/) {
if (platform_validator_ == nullptr) {
platform_validator_ =
static_cast<envoy_cert_validator*>(Api::External::retrieveApi("platform_cert_validator"));
}
return std::make_unique<PlatformBridgeCertValidator>(config, stats, platform_validator_);
}

REGISTER_FACTORY(PlatformBridgeCertValidatorFactory, CertValidatorFactory);

} // namespace Tls
} // namespace TransportSockets
} // namespace Extensions
} // namespace Envoy
Loading