Skip to content

mobile: Replace direct JNIEnv access with JniHelper #30705

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.

Sign up for GitHub

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

Merged
RyanTheOptimist merged 5 commits into from
Nov 2, 2023
Merged

mobile: Replace direct JNIEnv access with JniHelper #30705

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2b4cf56
mobile: Replace direct JNIEnv access with JniHelper
fredyw Nov 2, 2023
05d6a72
Update missing code
fredyw Nov 2, 2023
90fd1d4
Fix typo
fredyw Nov 2, 2023
3fe4dd3
Update macro name to be more consistent with the rest
fredyw Nov 2, 2023
deb7ac8
Merge branch 'main' into jni_helper_update
fredyw Nov 2, 2023
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
1 change: 1 addition & 0 deletions mobile/library/common/jni/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ cc_library(
"jni_utility.h",
],
deps = [
":jni_helper_lib",
":jni_support_lib",
"//library/common/jni/import:jni_import_lib",
"//library/common/jni/types:jni_env_lib",
Expand Down
19 changes: 10 additions & 9 deletions mobile/library/common/jni/android_jni_utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,15 @@
bool is_cleartext_permitted(absl::string_view hostname) {
#if defined(__ANDROID_API__)
envoy_data host = Envoy::Data::Utility::copyToBridgeData(hostname);
JNIEnv* env = Envoy::JNI::get_env();
jstring java_host = Envoy::JNI::native_data_to_string(env, host);
Envoy::JNI::JniHelper jni_helper(Envoy::JNI::get_env());
Copy link
Copy Markdown
Contributor

@RyanTheOptimist RyanTheOptimist Nov 2, 2023

Choose a reason for hiding this comment

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

Would it make sense to have JniHelper call Envoy::JNI::get_env() in the constructor to simplify usage?

Copy link
Copy Markdown
Member Author

@fredyw fredyw Nov 2, 2023

Choose a reason for hiding this comment

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

Unfortunately the use of Envoy::JNI::get_env() is not consistent. Some will use it and some won't. I believe Envoy::JNI::get_env() is also very Android specific, which means it won't work when running on HotSpot VM (non Android). In the future PR, I plan on fixing this as well, which could possibly be using Envoy::JNI::get_env() everywhere (or removing it).

Copy link
Copy Markdown
Contributor

@RyanTheOptimist RyanTheOptimist Nov 2, 2023

Choose a reason for hiding this comment

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

Oh, I see. How confusing! Thanks for working on this.

jstring java_host = Envoy::JNI::native_data_to_string(jni_helper, host);
jclass jcls_AndroidNetworkLibrary =
Envoy::JNI::find_class("io.envoyproxy.envoymobile.utilities.AndroidNetworkLibrary");
jmethodID jmid_isCleartextTrafficPermitted = env->GetStaticMethodID(
jmethodID jmid_isCleartextTrafficPermitted = jni_helper.getEnv()->GetStaticMethodID(
Copy link
Copy Markdown
Contributor

@RyanTheOptimist RyanTheOptimist Nov 2, 2023

Choose a reason for hiding this comment

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

Your plan is to (incrementally) replace code like this with jni_helper.GetStaticMethodID(...), right?

Copy link
Copy Markdown
Member Author

@fredyw fredyw Nov 2, 2023

Choose a reason for hiding this comment

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

Yup. That's the plan. I will try to make small changes at a time, so it's easier to debug in case something breaks. Once everything has been fully migrated, I'll start removing JniHelper::getEnv().

jcls_AndroidNetworkLibrary, "isCleartextTrafficPermitted", "(Ljava/lang/String;)Z");
jboolean result = env->CallStaticBooleanMethod(jcls_AndroidNetworkLibrary,
jmid_isCleartextTrafficPermitted, java_host);
env->DeleteLocalRef(java_host);
jboolean result = jni_helper.getEnv()->CallStaticBooleanMethod(
jcls_AndroidNetworkLibrary, jmid_isCleartextTrafficPermitted, java_host);
jni_helper.getEnv()->DeleteLocalRef(java_host);
release_envoy_data(host);
return result == JNI_TRUE;
#else
Expand All @@ -36,12 +36,13 @@ bool is_cleartext_permitted(absl::string_view hostname) {

void tag_socket(int ifd, int uid, int tag) {
#if defined(__ANDROID_API__)
JNIEnv* env = Envoy::JNI::get_env();
Envoy::JNI::JniHelper jni_helper(Envoy::JNI::get_env());
jclass jcls_AndroidNetworkLibrary =
Envoy::JNI::find_class("io.envoyproxy.envoymobile.utilities.AndroidNetworkLibrary");
jmethodID jmid_tagSocket =
env->GetStaticMethodID(jcls_AndroidNetworkLibrary, "tagSocket", "(III)V");
env->CallStaticVoidMethod(jcls_AndroidNetworkLibrary, jmid_tagSocket, ifd, uid, tag);
jni_helper.getEnv()->GetStaticMethodID(jcls_AndroidNetworkLibrary, "tagSocket", "(III)V");
jni_helper.getEnv()->CallStaticVoidMethod(jcls_AndroidNetworkLibrary, jmid_tagSocket, ifd, uid,
tag);
#else
UNREFERENCED_PARAMETER(ifd);
UNREFERENCED_PARAMETER(uid);
Expand Down
78 changes: 42 additions & 36 deletions mobile/library/common/jni/android_network_utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,81 +12,88 @@
// Helper functions call into AndroidNetworkLibrary, but they are not platform dependent
// because AndroidNetworkLibray can be called in non-Android platform with mock interfaces.

bool jvm_cert_is_issued_by_known_root(JNIEnv* env, jobject result) {
bool jvm_cert_is_issued_by_known_root(Envoy::JNI::JniHelper& jni_helper, jobject result) {
jclass jcls_AndroidCertVerifyResult =
Envoy::JNI::find_class("io.envoyproxy.envoymobile.utilities.AndroidCertVerifyResult");
jmethodID jmid_isIssuedByKnownRoot =
env->GetMethodID(jcls_AndroidCertVerifyResult, "isIssuedByKnownRoot", "()Z");
jni_helper.getEnv()->GetMethodID(jcls_AndroidCertVerifyResult, "isIssuedByKnownRoot", "()Z");
Envoy::JNI::Exception::checkAndClear("jvm_cert_is_issued_by_known_root:GetMethodID");
ASSERT(jmid_isIssuedByKnownRoot);
bool is_issued_by_known_root = env->CallBooleanMethod(result, jmid_isIssuedByKnownRoot);
bool is_issued_by_known_root =
jni_helper.getEnv()->CallBooleanMethod(result, jmid_isIssuedByKnownRoot);
Envoy::JNI::Exception::checkAndClear("jvm_cert_is_issued_by_known_root:CallBooleanMethod");
env->DeleteLocalRef(jcls_AndroidCertVerifyResult);
jni_helper.getEnv()->DeleteLocalRef(jcls_AndroidCertVerifyResult);
return is_issued_by_known_root;
}

envoy_cert_verify_status_t jvm_cert_get_status(JNIEnv* env, jobject j_result) {
envoy_cert_verify_status_t jvm_cert_get_status(Envoy::JNI::JniHelper& jni_helper,
jobject j_result) {
jclass jcls_AndroidCertVerifyResult =
Envoy::JNI::find_class("io.envoyproxy.envoymobile.utilities.AndroidCertVerifyResult");
jmethodID jmid_getStatus = env->GetMethodID(jcls_AndroidCertVerifyResult, "getStatus", "()I");
jmethodID jmid_getStatus =
jni_helper.getEnv()->GetMethodID(jcls_AndroidCertVerifyResult, "getStatus", "()I");
Envoy::JNI::Exception::checkAndClear("jvm_cert_get_status:GetMethodID");
ASSERT(jmid_getStatus);
envoy_cert_verify_status_t result = CERT_VERIFY_STATUS_FAILED;
result = static_cast<envoy_cert_verify_status_t>(env->CallIntMethod(j_result, jmid_getStatus));
result = static_cast<envoy_cert_verify_status_t>(
jni_helper.getEnv()->CallIntMethod(j_result, jmid_getStatus));
Envoy::JNI::Exception::checkAndClear("jvm_cert_get_status:CallIntMethod");

env->DeleteLocalRef(jcls_AndroidCertVerifyResult);
jni_helper.getEnv()->DeleteLocalRef(jcls_AndroidCertVerifyResult);
return result;
}

jobjectArray jvm_cert_get_certificate_chain_encoded(JNIEnv* env, jobject result) {
jobjectArray jvm_cert_get_certificate_chain_encoded(Envoy::JNI::JniHelper& jni_helper,
jobject result) {
jclass jcls_AndroidCertVerifyResult =
Envoy::JNI::find_class("io.envoyproxy.envoymobile.utilities.AndroidCertVerifyResult");
jmethodID jmid_getCertificateChainEncoded =
env->GetMethodID(jcls_AndroidCertVerifyResult, "getCertificateChainEncoded", "()[[B");
jmethodID jmid_getCertificateChainEncoded = jni_helper.getEnv()->GetMethodID(
jcls_AndroidCertVerifyResult, "getCertificateChainEncoded", "()[[B");
Envoy::JNI::Exception::checkAndClear("jvm_cert_get_certificate_chain_encoded:GetMethodID");
jobjectArray certificate_chain =
static_cast<jobjectArray>(env->CallObjectMethod(result, jmid_getCertificateChainEncoded));
jobjectArray certificate_chain = static_cast<jobjectArray>(
jni_helper.getEnv()->CallObjectMethod(result, jmid_getCertificateChainEncoded));
Envoy::JNI::Exception::checkAndClear("jvm_cert_get_certificate_chain_encoded:CallObjectMethod");
env->DeleteLocalRef(jcls_AndroidCertVerifyResult);
jni_helper.getEnv()->DeleteLocalRef(jcls_AndroidCertVerifyResult);
return certificate_chain;
}

static void ExtractCertVerifyResult(JNIEnv* env, jobject result, envoy_cert_verify_status_t* status,
static void ExtractCertVerifyResult(Envoy::JNI::JniHelper& jni_helper, jobject result,
envoy_cert_verify_status_t* status,
bool* is_issued_by_known_root,
std::vector<std::string>* verified_chain) {
*status = jvm_cert_get_status(env, result);
*status = jvm_cert_get_status(jni_helper, result);
if (*status == CERT_VERIFY_STATUS_OK) {
*is_issued_by_known_root = jvm_cert_is_issued_by_known_root(env, result);
jobjectArray chain_byte_array = jvm_cert_get_certificate_chain_encoded(env, result);
*is_issued_by_known_root = jvm_cert_is_issued_by_known_root(jni_helper, result);
jobjectArray chain_byte_array = jvm_cert_get_certificate_chain_encoded(jni_helper, result);
if (chain_byte_array != nullptr) {
Envoy::JNI::JavaArrayOfByteArrayToStringVector(env, chain_byte_array, verified_chain);
Envoy::JNI::JavaArrayOfByteArrayToStringVector(jni_helper, chain_byte_array, verified_chain);
}
}
}

// `auth_type` and `host` are expected to be UTF-8 encoded.
jobject call_jvm_verify_x509_cert_chain(JNIEnv* env, const std::vector<std::string>& cert_chain,
jobject call_jvm_verify_x509_cert_chain(Envoy::JNI::JniHelper& jni_helper,
const std::vector<std::string>& cert_chain,
std::string auth_type, absl::string_view hostname) {
jni_log("[Envoy]", "jvm_verify_x509_cert_chain");
jclass jcls_AndroidNetworkLibrary =
Envoy::JNI::find_class("io.envoyproxy.envoymobile.utilities.AndroidNetworkLibrary");
jmethodID jmid_verifyServerCertificates = env->GetStaticMethodID(
jmethodID jmid_verifyServerCertificates = jni_helper.getEnv()->GetStaticMethodID(
jcls_AndroidNetworkLibrary, "verifyServerCertificates",
"([[B[B[B)Lio/envoyproxy/envoymobile/utilities/AndroidCertVerifyResult;");
Envoy::JNI::Exception::checkAndClear("call_jvm_verify_x509_cert_chain:GetStaticMethodID");
jobjectArray chain_byte_array = Envoy::JNI::ToJavaArrayOfByteArray(env, cert_chain);
jbyteArray auth_string = Envoy::JNI::ToJavaByteArray(env, auth_type);
jobjectArray chain_byte_array = Envoy::JNI::ToJavaArrayOfByteArray(jni_helper, cert_chain);
jbyteArray auth_string = Envoy::JNI::ToJavaByteArray(jni_helper, auth_type);
jbyteArray host_string = Envoy::JNI::ToJavaByteArray(
env, reinterpret_cast<const uint8_t*>(hostname.data()), hostname.length());
jobject result =
env->CallStaticObjectMethod(jcls_AndroidNetworkLibrary, jmid_verifyServerCertificates,
chain_byte_array, auth_string, host_string);
jni_helper, reinterpret_cast<const uint8_t*>(hostname.data()), hostname.length());
jobject result = jni_helper.getEnv()->CallStaticObjectMethod(
jcls_AndroidNetworkLibrary, jmid_verifyServerCertificates, chain_byte_array, auth_string,
host_string);
Envoy::JNI::Exception::checkAndClear("call_jvm_verify_x509_cert_chain:CallStaticObjectMethod");
env->DeleteLocalRef(chain_byte_array);
env->DeleteLocalRef(auth_string);
env->DeleteLocalRef(host_string);
env->DeleteLocalRef(jcls_AndroidNetworkLibrary);
jni_helper.getEnv()->DeleteLocalRef(chain_byte_array);
jni_helper.getEnv()->DeleteLocalRef(auth_string);
jni_helper.getEnv()->DeleteLocalRef(host_string);
jni_helper.getEnv()->DeleteLocalRef(jcls_AndroidNetworkLibrary);
return result;
}

Expand All @@ -96,18 +103,17 @@ static void jvm_verify_x509_cert_chain(const std::vector<std::string>& cert_chai
envoy_cert_verify_status_t* status,
bool* is_issued_by_known_root,
std::vector<std::string>* verified_chain) {
JNIEnv* env = Envoy::JNI::get_env();
jobject result = call_jvm_verify_x509_cert_chain(env, cert_chain, auth_type, hostname);
Envoy::JNI::JniHelper jni_helper(Envoy::JNI::get_env());
jobject result = call_jvm_verify_x509_cert_chain(jni_helper, cert_chain, auth_type, hostname);
if (Envoy::JNI::Exception::checkAndClear()) {
*status = CERT_VERIFY_STATUS_NOT_YET_VALID;
} else {
ExtractCertVerifyResult(Envoy::JNI::get_env(), result, status, is_issued_by_known_root,
verified_chain);
ExtractCertVerifyResult(jni_helper, result, status, is_issued_by_known_root, verified_chain);
if (Envoy::JNI::Exception::checkAndClear()) {
*status = CERT_VERIFY_STATUS_FAILED;
}
}
env->DeleteLocalRef(result);
jni_helper.getEnv()->DeleteLocalRef(result);
}

envoy_cert_validation_result verify_x509_cert_chain(const std::vector<std::string>& certs,
Expand Down
4 changes: 3 additions & 1 deletion mobile/library/common/jni/android_network_utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,14 @@
#include "library/common/api/c_types.h"
#include "library/common/extensions/cert_validator/platform_bridge/c_types.h"
#include "library/common/jni/import/jni_import.h"
#include "library/common/jni/jni_helper.h"

// NOLINT(namespace-envoy)

/* Calls up through JNI to validate given certificates.
*/
jobject call_jvm_verify_x509_cert_chain(JNIEnv* env, const std::vector<std::string>& cert_chain,
jobject call_jvm_verify_x509_cert_chain(Envoy::JNI::JniHelper& jni_helper,
const std::vector<std::string>& cert_chain,
std::string auth_type, absl::string_view hostname);

envoy_cert_validation_result verify_x509_cert_chain(const std::vector<std::string>& certs,
Expand Down
Loading