-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Update bundled JDK to 25 #134910
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
Update bundled JDK to 25 #134910
Conversation
Pinging @elastic/es-delivery (Team:Delivery) |
Pending on #134719 to silence some noisy timezone id warnings at startup due to a log4j2 bug. |
@breskeby does updating the bundled jdk still require updating verification metadata? |
It looks like it's not necessary anymore due to below: elasticsearch/gradle/verification-metadata.xml Lines 6 to 19 in 8be273a
|
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.
LGTM
Might have to have someone from @elastic/es-analytical-engine take a look at the ES|QL failures. The root cause seems to be for some reason we're throwing an |
Are we talking about It may be that the behaviour has changed in JDK 25, but this method could always have returned a |
task.systemProperty('java.security.policy', String.format(Locale.ROOT, "=%s", fipsPolicy)) | ||
task.systemProperty('javax.net.ssl.trustStore', fipsTrustStore) | ||
task.systemProperty('org.bouncycastle.fips.approved_only', 'true') | ||
task.systemProperty('jdk.includeInExceptions', 'hostInfo') |
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.
@rjernst this fixes a category of failures on FIPS, mostly around URI parsing, where the returned exception messages lack specifics that might leak sensitive information (i.e. "illegal character at index n"). This behavior seems to be enabled in "FIPS mode", but can be overriden via a system property (see code change). Is this actually going to be problematic? Do we need to set this system property by default in ES so that we get the same behavior even when running on FIPS, since we do our own redaction of URIs?
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.
Here's an example failure: https://gradle-enterprise.elastic.co/s/z75k5utsjqnxu/tests/task/:x-pack:plugin:sql:sql-client:test/details/org.elasticsearch.xpack.sql.client.UriUtilsTests/testUriRedactionInvalidFragment?top-execution=1
Essentially, in the FIPS mode we cannot parse where the problematic characters are from the thrown Exception (the JDK omits this for security reasons) so we return a generic <REDACTED>
instead. It's worth noting that we only hit this a) in FIPS mode (or at least as we configure it for testing) and b) when we are parsing an invalid URI. So presumably this is so we don't leak credentials with logs and error messages. I think the end result is when running on FIPS if folks provide bad URIs in configuration or other requests, they might get a slightly less useful error message unless we pass the system property above. Thoughts?
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.
Perhaps @tvernum can weigh in here. I think it would be ok to be consistent at the expense of less useful error messages, but I could go either way.
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.
Sorry, I don't quite follow how "fips mode" changes the behaviour (that is, what about our fips tests triggers the change)
From the JDK 25 docs it sounds like the JDK never includes this information without the system property, but your description seem to imply otherwise.
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.
OK, I think I worked it out.
The set of possibly values for the jdk.includeInExceptions
property has expanded between JDK24 and JDK25
- In JDK 24, the options were
hostInfo
andjar
and the default was empty. - In JDK 25, the options are
hostInfo
,hostInfoExclSocket
,jar
anduserInfo
and the "default" ishostInfoExclSocket
but the default is actually configured inconf/security/java.properties
Our FIPS tests run with a custom security config (that's how you enable FIPS, so we need to copy that new default into our FIPS setup
diff --git i/build-tools-internal/src/main/resources/fips_java.security w/build-tools-internal/src/main/resources/fips_java.security
index 1018b1b96ac..6bed997bfe8 100644
--- i/build-tools-internal/src/main/resources/fips_java.security
+++ w/build-tools-internal/src/main/resources/fips_java.security
@@ -49,3 +49,7 @@ jdk.xml.dsig.secureValidationPolicy=\
noRetrievalMethodLoops
jceks.key.serialFilter = java.base/java.lang.Enum;java.base/java.security.KeyRep;\
java.base/java.security.KeyRep$Type;java.base/javax.crypto.spec.SecretKeySpec;!*
+
+# Needed since JDK25 to match the default config in the out-of-the-box java.security
+jdk.includeInExceptions=hostInfoExclSocket
+
diff --git i/build-tools-internal/src/main/resources/fips_java_oracle.security w/build-tools-internal/src/main/resources/fips_java_oracle.security
index 875151a058e..c50f85202a1 100644
--- i/build-tools-internal/src/main/resources/fips_java_oracle.security
+++ w/build-tools-internal/src/main/resources/fips_java_oracle.security
@@ -50,3 +50,7 @@ jdk.xml.dsig.secureValidationPolicy=\
noRetrievalMethodLoops
jceks.key.serialFilter = java.base/java.lang.Enum;java.base/java.security.KeyRep;\
java.base/java.security.KeyRep$Type;java.base/javax.crypto.spec.SecretKeySpec;!*
+
+# Needed since JDK25 to match the default config in the out-of-the-box java.security
+jdk.includeInExceptions=hostInfoExclSocket
+
(From my testing that doesn't break older JDKs, and it shouldn't, they just ignore unrecognized options)
It's possible we need to also add it to the versions in test/test-clusters/src/main/resources/fips/
as well.
But if we do that, then we don't need this system-property change.
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.
For reference this is the effective set of differences for java.security
between Oracle's OpenJDK v24 and v25
$ diff <(grep '^[^#]' ~/.gradle/jdks/oracle_corporation-24-aarch64-os_x.2/jdk-24.jdk/Contents/Home/conf/security/java.security) <(grep '^[^#]' ~/.gradle/jdks/oracle_corporation-25-aarch64-os_x.2/jdk-25.jdk/Contents/Home/conf/security/java.security)
40c40,41
< ECDH, TLS_RSA_*
---
> ECDH, TLS_RSA_*, rsa_pkcs1_sha1 usage HandshakeSignature, \
> ecdsa_sha1 usage HandshakeSignature, dsa_sha1 usage HandshakeSignature
64a66
> jdk.includeInExceptions=hostInfoExclSocket
66c68
< jdk.security.caDistrustPolicies=SYMANTEC_TLS,ENTRUST_TLS
---
> jdk.security.caDistrustPolicies=SYMANTEC_TLS,ENTRUST_TLS,CAMERFIRMA_TLS
68a71
> jdk.epkcs8.defaultAlgorithm=PBEWithHmacSHA256AndAES_128
I don't think any other other changes are going to be a problem between FIPS/non-FIPS.
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.
Thanks @tvernum for having a look 👍 I'll push the suggested changes
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.
@tvernum Actually, wondering, are you ok doing this as a follow up? I don't want to risk further delays on this PR and would rather isolate the change in a follow up if you're ok.
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.
Ok, as discussed I've included above change, see 63fce46
💔 Backport failed
You can use sqren/backport to manually backport by running |
Co-authored-by: Mark Vieira <[email protected]> (cherry picked from commit 4aa0f16) # Conflicts: # x-pack/plugin/esql/build.gradle
Co-authored-by: Mark Vieira <[email protected]> (cherry picked from commit 4aa0f16) # Conflicts: # x-pack/plugin/esql/build.gradle # x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/custom/CustomRequestManagerTests.java # x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/custom/request/CustomRequestTests.java
Co-authored-by: Mark Vieira <[email protected]> (cherry picked from commit 4aa0f16) # Conflicts: # x-pack/plugin/esql/build.gradle
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
Co-authored-by: Mark Vieira <[email protected]> (cherry picked from commit 4aa0f16) # Conflicts: # x-pack/plugin/esql/build.gradle # x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/custom/CustomRequestManagerTests.java # x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/custom/request/CustomRequestTests.java
Co-authored-by: Mark Vieira <[email protected]> (cherry picked from commit 4aa0f16)
Co-authored-by: Mark Vieira <[email protected]> (cherry picked from commit 4aa0f16) # Conflicts: # x-pack/plugin/esql/build.gradle
Co-authored-by: Mark Vieira <[email protected]> (cherry picked from commit 4aa0f16) # Conflicts: # x-pack/plugin/esql/build.gradle
* Update bundled JDK to 25 (#134910) Co-authored-by: Mark Vieira <[email protected]> (cherry picked from commit 4aa0f16) # Conflicts: # x-pack/plugin/esql/build.gradle # x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/custom/CustomRequestManagerTests.java # x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/custom/request/CustomRequestTests.java * fix asm version
Upgrade the bundled JDK to 25.
No need to update verification data anymore for openjdk.
Relates to ES-12404