Skip to content

Basic TLS Encrypted ClientHello (ECH) support (native layer) #1374

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
tweksteen merged 9 commits into from
Sep 29, 2025
Merged

Basic TLS Encrypted ClientHello (ECH) support (native layer) #1374

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
37b0729
Native classes updated to provide access to ECH parameters in BoringS…
mnbogner Aug 14, 2025
c54c80c
First pass of updates based on PR feedback (includes temp changes to …
mnbogner Sep 2, 2025
0b033db
implemented additional changes requested in the pr
mnbogner Sep 3, 2025
e88b5c9
remove redundant methods and postpone retry flow testing
mnbogner Sep 4, 2025
32f5387
fixed formatting for clang ci failure
mnbogner Sep 5, 2025
df0a9e9
revised several methods and corresponding tests to handle queued errors
mnbogner Sep 11, 2025
82b5bad
Removed several redundant "else" blocks. Commented out non-ech chang…
mnbogner Sep 23, 2025
171e31e
fixed additional CI errors
mnbogner Sep 28, 2025
49d0dc9
Merge branch 'master' into ech_native_part
tweksteen Sep 28, 2025
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
47 changes: 40 additions & 7 deletions common/src/jni/main/cpp/conscrypt/native_crypto.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1167,19 +1167,30 @@ static jint NativeCrypto_EVP_PKEY_cmp(JNIEnv* env, jclass, jobject pkey1Ref, job
JNI_TRACE("EVP_PKEY_cmp(%p, %p)", pkey1Ref, pkey2Ref);
EVP_PKEY* pkey1 = fromContextObject<EVP_PKEY>(env, pkey1Ref);
if (pkey1 == nullptr) {
conscrypt::jniutil::throwNullPointerException(env, "Null pointer, key 1");
ERR_clear_error();
JNI_TRACE("EVP_PKEY_cmp => pkey1 == null");
return 0;
}
EVP_PKEY* pkey2 = fromContextObject<EVP_PKEY>(env, pkey2Ref);
if (pkey2 == nullptr) {
conscrypt::jniutil::throwNullPointerException(env, "Null pointer, key 2");
ERR_clear_error();
JNI_TRACE("EVP_PKEY_cmp => pkey2 == null");
return 0;
}
JNI_TRACE("EVP_PKEY_cmp(%p, %p) <- ptr", pkey1, pkey2);

int result = EVP_PKEY_cmp(pkey1, pkey2);
JNI_TRACE("EVP_PKEY_cmp(%p, %p) => %d", pkey1, pkey2, result);
return result;
if (result < 0) {
conscrypt::jniutil::throwInvalidKeyException(env, "Decoding error");
ERR_clear_error();
Copy link
Contributor

@prbprbprb prbprbprb Sep 12, 2025

Choose a reason for hiding this comment

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

Drive-by comment: If you're going to throw a checked exception, you should reflect that in the Java method signature and add a regression test for it. Same for the parsing exception below.

Copy link
Member

@tweksteen tweksteen Sep 15, 2025

Choose a reason for hiding this comment

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

+1, but more generally let's not change the non-ECH part of the code in this pull request. If you wanted to, please follow up with a new PR.

Could you please revert the changes here that are not related to ECH? Thanks

Copy link
Contributor Author

@mnbogner mnbogner Sep 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I ended up making these changes because one of the failing tests was the one that tested this method:

org.conscrypt.ConscryptOpenJdkSuite > org.conscrypt.NativeCryptoTest.EVP_PKEY_cmp_BothNullParameters FAILED
    java.lang.AssertionError: unexpected exception type thrown; expected:<java.lang.NullPointerException> but was:<java.lang.AssertionError>
        at org.junit.Assert.assertThrows(Assert.java:1020)
        at org.junit.Assert.assertThrows(Assert.java:981)
        at org.conscrypt.NativeCryptoTest.EVP_PKEY_cmp_BothNullParameters(NativeCryptoTest.java:244)
        Caused by:
        java.lang.AssertionError: Error queue should have been empty but was (/home/mnbogner/Software/Repository/boringssl/ssl/encrypted_client_hello.cc:1039) error:10000089:SSL routines:OPENSSL_internal:DECODE_ERROR
            at org.conscrypt.NativeCrypto.EVP_PKEY_cmp(Native Method)
            at org.conscrypt.NativeCryptoTest.lambda$EVP_PKEY_cmp_BothNullParameters$0(NativeCryptoTest.java:244)
            at org.junit.Assert.assertThrows(Assert.java:1001)
            ... 2 more

Could this just be failing locally for some reason? I suppose I can revert it and see what happens with the CI tests.

(UPDATE) Commented this part out for now and pushed it. If there's no CI failures I'll pull out all the extra stuff and we can wrap this up.

Copy link
Contributor Author

@mnbogner mnbogner Sep 24, 2025

Choose a reason for hiding this comment

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

+1, but more generally let's not change the non-ECH part of the code in this pull request. If you wanted to, please follow up with a new PR.

Could you please revert the changes here that are not related to ECH? Thanks

Unfortunately it looks like EVP_PKEY_cmp_BothNullParameters still fails (and test_get_RSA_public_params, which doesn't fail locally). I assume this isn't failing elsewhere, but I'm not sure how my changes could have caused this. Could the failure be caused by errors placed in the queue by a prior test? I notice that the failing test is right after one of the new ECH tests.

Copy link
Member

@tweksteen tweksteen Sep 25, 2025

Choose a reason for hiding this comment

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

Thanks. I think your hunch is correct. If you look at the error in the CI, the complete error is:

2025-09-23T20:09:49.2214351Z org.conscrypt.ConscryptOpenJdkSuite > org.conscrypt.NativeCryptoTest.EVP_PKEY_cmp_BothNullParameters FAILED
2025-09-23T20:09:49.2215827Z     java.lang.AssertionError: unexpected exception type thrown; expected:<java.lang.NullPointerException> but was:<java.lang.AssertionError>
2025-09-23T20:09:49.2229754Z         at org.junit.Assert.assertThrows(Assert.java:1020)
2025-09-23T20:09:49.2230440Z         at org.junit.Assert.assertThrows(Assert.java:981)
2025-09-23T20:09:49.2231640Z         at org.conscrypt.NativeCryptoTest.EVP_PKEY_cmp_BothNullParameters(NativeCryptoTest.java:248)
2025-09-23T20:09:49.2232278Z 
2025-09-23T20:09:49.2232411Z         Caused by:
2025-09-23T20:09:49.2233793Z         java.lang.AssertionError: Error queue should have been empty but was (/home/runner/work/_temp/boringssl/ssl/encrypted_client_hello.cc:1039) error:10000089:SSL routines:OPENSSL_internal:DECODE_ERROR
2025-09-23T20:09:49.2235956Z             at org.conscrypt.NativeCrypto.EVP_PKEY_cmp(Native Method)
2025-09-23T20:09:49.2236915Z             at org.conscrypt.NativeCryptoTest.lambda$EVP_PKEY_cmp_BothNullParameters$0(NativeCryptoTest.java:248)
2025-09-23T20:09:49.2237827Z             at org.junit.Assert.assertThrows(Assert.java:1001)
2025-09-23T20:09:49.2238310Z             ... 2 more

Note how the error queue is populated in encrypted_client_hello.cc:1039. Looking at NativeCrypto_SSL_CTX_ech_enable_server, I think you would need to clear the queue on line 1193, if SSL_ECH_KEYS_add failed.

JNI_TRACE("VP_PKEY_cmp(%p, %p) => threw exception", pkey1, pkey2);
return result;
} else {
JNI_TRACE("EVP_PKEY_cmp(%p, %p) => %d", pkey1, pkey2, result);
return result;
}
}

/*
Expand Down Expand Up @@ -8289,6 +8300,7 @@ static jlong NativeCrypto_SSL_CTX_new(JNIEnv* env, jclass) {
bssl::UniquePtr<SSL_CTX> sslCtx(SSL_CTX_new(TLS_with_buffers_method()));
if (sslCtx.get() == nullptr) {
conscrypt::jniutil::throwExceptionFromBoringSSLError(env, "SSL_CTX_new");
ERR_clear_error();
return 0;
}
SSL_CTX_set_options(
Expand Down Expand Up @@ -11816,17 +11828,28 @@ static jboolean NativeCrypto_SSL_set1_ech_config_list(JNIEnv* env, jclass, jlong
SSL* ssl = to_SSL(env, ssl_address, true);
JNI_TRACE("ssl=%p NativeCrypto_SSL_set1_ech_config_list(%p)", ssl, configJavaBytes);
if (ssl == nullptr) {
conscrypt::jniutil::throwNullPointerException(env, "Null pointer, ssl address");
ERR_clear_error();
return JNI_FALSE;
}
ScopedByteArrayRO configBytes(env, configJavaBytes);
if (configBytes.get() == nullptr) {
conscrypt::jniutil::throwNullPointerException(env, "Null pointer, ech config");
ERR_clear_error();
JNI_TRACE("NativeCrypto_SSL_set1_ech_config_list => could not read config bytes");
return JNI_FALSE;
}
int ret = SSL_set1_ech_config_list(ssl, reinterpret_cast<const uint8_t*>(configBytes.get()),
configBytes.size());
JNI_TRACE("ssl=%p NativeCrypto_SSL_set1_ech_config_list(%p) => %d", ssl, configJavaBytes, ret);
return ret;
if (!ret) {
conscrypt::jniutil::throwParsingException(env, "Error parsing ECH config");
ERR_clear_error();
JNI_TRACE("ssl=%p NativeCrypto_SSL_set1_ech_config_list(%p) => threw exception", ssl, configJavaBytes);
return JNI_FALSE;
} else {
JNI_TRACE("ssl=%p NativeCrypto_SSL_set1_ech_config_list(%p) => %d", ssl, configJavaBytes, ret);
return ret;
}
}

static jstring NativeCrypto_SSL_get0_ech_name_override(JNIEnv* env, jclass, jlong ssl_address,
Expand Down Expand Up @@ -11912,11 +11935,21 @@ static jboolean NativeCrypto_SSL_ech_accepted(JNIEnv* env, jclass, jlong ssl_add
SSL* ssl = to_SSL(env, ssl_address, true);
JNI_TRACE("ssl=%p NativeCrypto_SSL_ech_accepted", ssl);
if (ssl == nullptr) {
return 0;
conscrypt::jniutil::throwNullPointerException(env, "Null pointer, ssl address");
ERR_clear_error();
return JNI_FALSE;
}
jboolean accepted = SSL_ech_accepted(ssl);
JNI_TRACE("ssl=%p NativeCrypto_SSL_ech_accepted => %d", ssl, accepted);
return accepted;

if (!accepted) {
conscrypt::jniutil::throwParsingException(env, "Invalid ECH config list");
ERR_clear_error();
JNI_TRACE("ssl=%p NativeCrypto_SSL_ech_accepted => threw exception", ssl);
return JNI_FALSE;
} else {
JNI_TRACE("ssl=%p NativeCrypto_SSL_ech_accepted => %d", ssl, accepted);
return accepted;
}
}

static jboolean NativeCrypto_SSL_CTX_ech_enable_server(JNIEnv* env, jclass, jlong ssl_ctx_address,
Expand Down
17 changes: 5 additions & 12 deletions openjdk/src/test/java/org/conscrypt/NativeCryptoTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -636,14 +636,9 @@ public void test_SSL_set1_ech_invalid_config_list() throws Exception {
byte[] badConfigList = {
0x00, 0x05, (byte) 0xfe, 0x0d, (byte) 0xff, (byte) 0xff, (byte) 0xff};
boolean set = false;
try {
set = NativeCrypto.SSL_set1_ech_config_list(s, null, badConfigList);
NativeCrypto.SSL_free(s, null);
NativeCrypto.SSL_CTX_free(c, null);
} catch (AssertionError e) {
// ignored when running with checkErrorQueue
}
assertFalse(set);
assertThrows(ParsingException.class, () -> NativeCrypto.SSL_set1_ech_config_list(s, null, badConfigList));
NativeCrypto.SSL_free(s, null);
NativeCrypto.SSL_CTX_free(c, null);
}

@Test
Expand Down Expand Up @@ -671,7 +666,7 @@ public void test_SSL_ech_accepted() throws Exception {
long c = NativeCrypto.SSL_CTX_new();
long s = NativeCrypto.SSL_new(c, null);

assertFalse(NativeCrypto.SSL_ech_accepted(s, null));
assertThrows(ParsingException.class, () -> assertFalse(NativeCrypto.SSL_ech_accepted(s, null)));

NativeCrypto.SSL_free(s, null);
NativeCrypto.SSL_CTX_free(c, null);
Expand Down Expand Up @@ -3194,9 +3189,7 @@ public void test_get_RSA_public_params() throws Exception {
assertNotEquals(NULL, groupCtx);
NativeRef.EC_GROUP group = new NativeRef.EC_GROUP(groupCtx);
NativeRef.EVP_PKEY ctx = new NativeRef.EVP_PKEY(NativeCrypto.EC_KEY_generate_key(group));
// Test originally asserted a RuntimeException but actually threw InvalidKeyException
// If this tests how the wrong keys are handled, I assume InvalidKeyException is correct
assertThrows(InvalidKeyException.class, () -> NativeCrypto.get_RSA_public_params(ctx));
assertThrows(RuntimeException.class, () -> NativeCrypto.get_RSA_public_params(ctx));
}

@Test
Expand Down
Loading