cert validation: use Android cert validation APIs#2525
cert validation: use Android cert validation APIs#2525Augustyniak merged 50 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
|
/retest |
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
|
/retest |
|
/retest |
RyanTheOptimist
left a comment
There was a problem hiding this comment.
Wow, this looks awesome. A number of mostly small comments. We'll definitely need to get some other folks on the review since it's touching a lot of EM stuff that I'm not an expert in, but this looks great!
| "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", | ||
|
|
There was a problem hiding this comment.
nit: Remove newline?
| */ | ||
| extern const char* route_cache_reset_filter_insert; | ||
|
|
||
| extern const char* default_cert_validation_context_template; |
There was a problem hiding this comment.
nit: comments, please
|
|
||
| // NOLINT(namespace-envoy) | ||
|
|
||
| typedef struct { |
There was a problem hiding this comment.
Comments on all the structs/functions, please
| CertValidatorPtr PlatformBridgeCertValidatorFactory::createCertValidator( | ||
| const Envoy::Ssl::CertificateValidationContextConfig* config, SslStats& stats, | ||
| TimeSource& /*time_source*/) { | ||
| #if defined(__APPLE__) |
There was a problem hiding this comment.
Does the platform verifier work on non-Android platforms? If so, perhaps we should make this check "if not android"
There was a problem hiding this comment.
Though we don't have non-Android platform supporting this, the current cronet tests do mock up the API calls so at least those cronet tests worked on non-Android platform. So I'm not going to enforce platform checking here.
Non-cronet enginer builders don't provide the interface to choose platform based cert validator after all.
| } | ||
|
|
||
| private: | ||
| const envoy_cert_validator* platform_bridge_api_; |
There was a problem hiding this comment.
nit: since this member is actually the validator, I'd be inclined to name the member validator_, but your call.
library/common/extensions/cert_validator/platform_bridge/platform_bridge_cert_validator.h
Show resolved
Hide resolved
| // Return empty string | ||
| std::string getCaFileName() const override { return ""; } | ||
|
|
||
| void verifyCertChainByPlatform( |
There was a problem hiding this comment.
nit: comment, please.
| private: | ||
| const Envoy::Ssl::CertificateValidationContextConfig* config_; | ||
| SslStats& stats_; | ||
| bool allow_untrusted_certificate_{false}; |
There was a problem hiding this comment.
nit: does = false work here? If so, let's use it.
| */ | ||
| public static native String brotliConfigInsert(); | ||
|
|
||
| public static native String certValidationTemplate(boolean use_platform); |
There was a problem hiding this comment.
nit: Comments here, please.
There was a problem hiding this comment.
Since it's java I think that the convention is to use the camel case naming (see logLevel argument in one of the methods in this file). So let's rename to usePlatform (and update the corresponding doc string)
|
|
||
| @Test | ||
| public void testGetRequest() throws Exception { | ||
| System.out.println("TEST_testGetRequest"); |
There was a problem hiding this comment.
nit: presumably we should remove these printlns?
There was a problem hiding this comment.
done. Though I really feel like it will always be needed. Hopefully there will be a better way to distinguish tests.
|
/retest |
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
|
/retest |
Augustyniak
left a comment
There was a problem hiding this comment.
Thank you very much for simplifying the config template logic. It looks great now!
Left a few comments - mostly related to the registerAPI part and the logic responsible for the interpolation of strings in builders.
library/cc/engine_builder.cc
Outdated
| const std::string& cert_validation_template = | ||
| (this->platform_certificates_validation_on_ ? platform_cert_validation_context_template | ||
| : default_cert_validation_context_template); | ||
| config_builder << "- &validation_context" << cert_validation_template << std::endl; |
There was a problem hiding this comment.
Nice! I personally think that it is much simpler now - thank you for working on this!
library/common/jni/jni_interface.cc
Outdated
| // TODO(danzh) this object leaks, but it's tied to the life of the engine. | ||
| envoy_cert_validator* api = (envoy_cert_validator*)safe_malloc(sizeof(envoy_cert_validator)); | ||
| api->validate_cert = verify_x509_cert_chain; | ||
| api->release_validator = jvm_detach_thread; |
There was a problem hiding this comment.
In theory we could release the api in here too (as part of release_validator). Optional comment as you've already added TODO
There was a problem hiding this comment.
release_validator is called at the end of each validation to detach thread. We shouldn't release the api during the life time of the engine.
There was a problem hiding this comment.
Things might change if we stop creating a new thread for each validation, but coalesce validations to a few dedicated threads.
| bssl::UniquePtr<X509> leaf_cert(d2i_X509( | ||
| nullptr, const_cast<const unsigned char**>(&leaf_cert_der.bytes), leaf_cert_der.length)); | ||
| envoy_cert_validation_result result = | ||
| platform_validator_->validate_cert(cert_chain.data(), cert_chain.size(), host_name.c_str()); |
There was a problem hiding this comment.
I remember that last week (in the community meeting) there was an idea to remove the use of registerAPI calls at all and just make platform_bridge_cert_validator call into verify_x509_cert_chain directly. I think that socket tagging example was brought up https://github.com/envoyproxy/envoy-mobile/pull/2423/files#diff-bf08f22bd707e11bdfa9ac4d50f12976176e2722ad82d3e66f5b88457b98f9a0R30 as it does call into platform specific stuff directly from c++ code without going through a register.
Would that be possible to stop using registerApi and call verify_x509_cert_chain directly from here? If yes, maybe we should do it to simplify the logic - we are doing additional work to register the API and I am not sure whether we need to. Do you think that using register is helping us in some way?
There was a problem hiding this comment.
I tried to remove registerApi, but ran into a very subtle difficulty in tests where we run cronet engine in MacOS, i.e. https://github.com/envoyproxy/envoy-mobile/actions/runs/3003635334/jobs/4822128405. If the platform_validator_ API gets loaded based on platform, those tests will not be able to load verify_x509_cert_chain. And to make things more complex, one day we will have IOS validation API. Those tests will be very confused about which API to load if the engine doesn't register the right one during initialization. I think java_tests_mac CI is the only blocker for me to get rid of registerApi stuff in this PR. Any thoughts?
There was a problem hiding this comment.
For the record, not planning to keep blocking on this - just want to ask some questions so that I understand the current situation better. Thank you :)
The logs from https://github.com/envoyproxy/envoy-mobile/actions/runs/3003635334/jobs/4822128405 are expired - I cannot see them :( .
If the platform_validator_ API gets loaded based on platform, those tests will not be able to load verify_x509_cert_chain
Since I do not have access to the logs of the failing tests - could you elaborate on this? Any chance that you remember which tests were failing specifically? Some of our existing platform specific JNI utility methods branch on #if(_ANDROID_API_) check (example here) but not sure whether this would be useful in your case.
one day we will have IOS validation API
yes, that's when the API register will come in handy. I just wonder whether for now we could avoid that additional complexity as between now and then (when we have iOS platform cert validation) a lot can change.
There was a problem hiding this comment.
Since I do not have access to the logs of the failing tests - could you elaborate on this? Any chance that you remember which tests were failing specifically?
Some tests under https://github.com/envoyproxy/envoy-mobile/tree/main/test/java/org/chromium/net/testing runs by android_tests / java_tests_mac CI, and they uses cronet engines which will always use platform API for cert validation.
Some of our existing platform specific JNI utility methods branch on
#if(_ANDROID_API_)check (example here) but not sure whether this would be useful in your case.
The AndroidNetworkLibrary APIs has mock interfaces, so that even on non-Android platform we can still run Java tests which call into verify_x509_cert_chain as long as those tests setup the mock interface. And there is no need of #if(_ANDROID_API_) for all the verification related JNI calls. But if we want to call validation interfaces based on #if not defined(__APPLE__) or #if(_ANDROID_API_), the cronet engine tests running on MacOS will not be able to make the right JNI calls.
That being said, today we are not supporting iOS cert validation APIs, so a temporary solution to make java tests on MacOS happy could be hard coding those JNI calls instead of calling them in #ifdef block. As a result, any engine enables platform based cert validation will call into those JNI functions. But this PR doesn't enable swift engine to use platform based cert validation, and in C++ engine we check platform before enabling it. This won't work once we support iOS cert validation API but still want to run tests under https://github.com/envoyproxy/envoy-mobile/tree/main/test/java/org/chromium/net/testing on MacOS.
one day we will have IOS validation API
yes, that's when the API register will come in handy. I just wonder whether for now we could avoid that additional complexity as between now and then (when we have iOS platform cert validation) a lot can change.
Will there be any chance not to run tests under https://github.com/envoyproxy/envoy-mobile/tree/main/test/java/org/chromium/net/testing on MacOS in the future? If that's the case, we won't even need platform registry after we support iOS cert validation API. Those CIs are helpful for Android developer using Mac, but not very valuable in real device scenario.
There was a problem hiding this comment.
Thank you for a detailed answer(s). I think that the current approach of registering filters makes sense if it helps us to have a better test coverage in here. I wish there was an easier way to make all of this works (actual implementation + tests) - maybe that's something that we will be able to make easier in the future.
The one remaining comment that I have left is the one with regard to moving the registration of the filter from its current place to ...initialize method instead? I left more details in #2525 (comment). What do you think about this one?
Will there be any chance not to run tests under https://github.com/envoyproxy/envoy-mobile/tree/main/test/java/org/chromium/net/testing on MacOS in the future?
If we decided that this is the way to go I think that there is nothing that should prevent us from doing so from the technical point of view. Not entirely sure why we run some of Android tests on macOS and some on Linux but I am guessing that we just wanted to cover testing on more platforms - not sure how useful is that, we can discuss in the community meeting if needed.
library/common/jni/jni_interface.cc
Outdated
|
|
||
| set_vm(vm); | ||
| // At this point, we know Android APIs are available. Register cert chain validation JNI calls. | ||
| envoy_status_t result = jvm_register_cert_validator(); |
There was a problem hiding this comment.
If we decided that we need to keep registerAPI logic in I think that we should move the registration to AndoridJNILibrary.initialize in here because:
- The
initializemethod is all about the initialization so registering API sounds like a good fit for it. - The
initializemethod is Android specific and so isAndroidJNILibrary.initialize find_classdoes not work beforeAndroidJNILibrary.initializegets called andAndroidJNILibrary.initializegets called afterJNI_onLoadso in general if your implementation ofjvm_register_cert_validatorever needs to accessfind_classit will fail.
There was a problem hiding this comment.
As we decided to keep using registerAPI, doing that during AndoridJNILibrary.initialize makes sense. In order do to so, I have to move the relevant JNI calls from jni_interface.cc to android_jni_utility.cc. PTAL!
|
|
||
| NSString *cert_validator_template = | ||
| [[NSString alloc] initWithUTF8String:default_cert_validation_context_template]; | ||
| [definitions appendFormat:@"- &validation_context_config_trust_chain%@\n" |
There was a problem hiding this comment.
This looks different compared to what we have in Java file https://github.com/envoyproxy/envoy-mobile/pull/2525/files#diff-33c52309a2e0d09eab91df02e32e8f571149afbe5a23f69509550d0f328622aeR291 and what we have in c++ builder https://github.com/envoyproxy/envoy-mobile/pull/2525/files#diff-204f30e7778ed8d1f3e6008076942d0cd78f18df61826d44ef7f2d54b2b9d2fbR198
Shouldn't they be more 1 to 1?
There was a problem hiding this comment.
I think that the c++ builder is what we want other builders to look like.
| configBuilder.append("] \n"); | ||
| } | ||
|
|
||
| // Add a new anchor to override the default anchors in config header. |
There was a problem hiding this comment.
Don't we need to prepend it with &validation_context or similar?
There was a problem hiding this comment.
Sorry, I myself got confused about what was included in the template and what wasn't. &validation_context is already included in the template string. So in the engine implementations, there is no need to append it.
The Java engine here is doing the right thing. The C++ engine shouldn't have it appended again, though it seems still to be a valid YAML as the unit test passed. And the swift engine was out-dated.
Signed-off-by: Dan Zhang <danzh@google.com>
test/cc/unit/envoy_config_test.cc
Outdated
| } | ||
| } | ||
|
|
||
| #if not defined(__APPLE__) |
There was a problem hiding this comment.
nit: can we add a doc string explaining that this test is effectively not run if being executed on a mac (which is the case currently)?
There was a problem hiding this comment.
Instead, I added #else block where calling enablePlatformCertificatesValidation(true) causes death.
| bssl::UniquePtr<X509> leaf_cert(d2i_X509( | ||
| nullptr, const_cast<const unsigned char**>(&leaf_cert_der.bytes), leaf_cert_der.length)); | ||
| envoy_cert_validation_result result = | ||
| platform_validator_->validate_cert(cert_chain.data(), cert_chain.size(), host_name.c_str()); |
There was a problem hiding this comment.
Thank you for a detailed answer(s). I think that the current approach of registering filters makes sense if it helps us to have a better test coverage in here. I wish there was an easier way to make all of this works (actual implementation + tests) - maybe that's something that we will be able to make easier in the future.
The one remaining comment that I have left is the one with regard to moving the registration of the filter from its current place to ...initialize method instead? I left more details in #2525 (comment). What do you think about this one?
Will there be any chance not to run tests under https://github.com/envoyproxy/envoy-mobile/tree/main/test/java/org/chromium/net/testing on MacOS in the future?
If we decided that this is the way to go I think that there is nothing that should prevent us from doing so from the technical point of view. Not entirely sure why we run some of Android tests on macOS and some on Linux but I am guessing that we just wanted to cover testing on more platforms - not sure how useful is that, we can discuss in the community meeting if needed.
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
| HasSubstr("envoy_mobile.cert_validator.platform_bridge_cert_validator")); | ||
| ASSERT_THAT(bootstrap.DebugString(), Not(HasSubstr("trusted_ca"))); | ||
| #else | ||
| EXPECT_DEATH(engine_builder.enablePlatformCertificatesValidation(true), |
There was a problem hiding this comment.
this is great - thank you
Augustyniak
left a comment
There was a problem hiding this comment.
Tests are red.
At this point I am fine with merging this PR as is but I left a few tiny comments. It would be great if you could address/respond to them.
Thank you for working with me on this PR - I've learnt a lot about the JIN and c++ engine builder while working/talking with you.
| envoy_status_t result = | ||
| register_platform_api(cert_validator_name, get_android_cert_validator_api()); | ||
| if (result == ENVOY_FAILURE) { | ||
| return -1; |
There was a problem hiding this comment.
I wonder whether we should just return register_platform_api... in here? ENVOY_SUCCESS is equal to 1 which is different than -1 that we return in here but I guess we decided to use ENVOY_SUCCESS/ENVOY_FAILURE constants so shiould be probably fine to return to simplify this and do return register_platform_api...
Thoughts?
There was a problem hiding this comment.
discussed offline - looks good the way it is.
There was a problem hiding this comment.
Changed to return ENVOY_FAILURE here and made android_test_jni_inteface.cc return directly.
| // At this point, we know Android APIs are available. Register cert chain validation JNI calls. | ||
| envoy_status_t result = | ||
| register_platform_api(cert_validator_name, get_android_cert_validator_api()); | ||
| if (result == ENVOY_FAILURE) { |
There was a problem hiding this comment.
same comment with regard to a returns value as in my comment above.
|
/retest |
1 similar comment
|
/retest |
|
@goaway can you re-review/approve this PR? I think that your concerns have been addressed |
|
Friendly ping on this :) |
|
@goaway I am going to merge this PR as I think that your concerns with regard to configuration template have been addressed. |
Description: add engine API to allow user config to use Android cert validation APIs. Risk Level: high Testing: added tests in Http2TestServerTest.java Docs Changes: Release Notes: Fixes envoyproxy#1575 Part of envoyproxy#2144 Signed-off-by: danzh <danzh2010@users.noreply.github.com>
Description: add engine API to allow user config to use Android cert validation APIs.
Risk Level: high
Testing: added tests in Http2TestServerTest.java
Docs Changes:
Release Notes:
Fixes #1575
Part of #2144