Re: RFR: 8349533: Refactor validator tests shell files to java
On Fri, 21 Feb 2025 11:49:26 GMT, Mikhail Yankelevich wrote: > Changed shell files to be java tests: > * ./validator/certreplace.sh > * ./validator/samedn.sh test/jdk/sun/security/validator/CertReplace.java line 117: > 115: final String outputInt = SecurityTools.keytool(ktBaseParameters + > 116:"-export -rfc > -alias int").getOutput(); > 117: Files.write(certPath, outputInt.getBytes(), > StandardOpenOption.APPEND); There are several places that can be enhanced, mainly to reduce `keytool` calling: 1. There is no need to export certs for `user` and `int`. You already created them as `user.cert` and `int.cert`. 2. Since "certreplace.certs" starts with "user.cert", you can directly `keytool -gencert` into this file on line 103. 3. There is no need to import "user.cert" to alias user since we will delete the entry anyway. 4. Consider replacing `keytool -import` and `keytool -delete` calls using `KeyStore` API. You can enhance `KeyStoreUtils` in `/test/lib` if worth doing. - PR Review Comment: https://git.openjdk.org/jdk/pull/23727#discussion_r1965643259
Re: RFR: 8261513: Various BasicConstraintsExtension issues [v4]
> 8261513: Various BasicConstraintsExtension issues Ben Perez has updated the pull request incrementally with one additional commit since the last revision: changed toString wording, no longer set critical to ca - Changes: - all: https://git.openjdk.org/jdk/pull/20224/files - new: https://git.openjdk.org/jdk/pull/20224/files/64398d5c..6ba44821 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20224&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20224&range=02-03 Stats: 3 lines in 1 file changed: 0 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/20224.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20224/head:pull/20224 PR: https://git.openjdk.org/jdk/pull/20224
Re: RFR: 8349759: Add unit test for CertificateBuilder and SimpleOCSPServer test utilities [v4]
On Thu, 20 Feb 2025 01:12:46 GMT, Jamil Nimeh wrote: >> This fix makes some minor changes to the internals of the >> `CertificateBuilder` and `SimpleOCSPServer` test classes. They would break >> when ML-DSA was selected as key and signing algorithms. Also RSASSA-PSS >> works better now with these changes. I've also taken this opportunity to do >> some cleanup on CertificateBuilder and added a method which uses a default >> signing algorithm based on the key, so the `build()` method no longer needs >> to provide that algorithm (though one can if they wish for things like RSA >> signatures if they want a different message digest in the signature). > > Jamil Nimeh has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - Add more descriptive summary to test > - Merge with main > - Remove unnecessary throws declarations > - Fix JBS ID and summary in test > - 8349759: Fix CertificateBuilder and SimpleOCSPServer test utilities to > support PQC algorithms Looks good. - Marked as reviewed by mullan (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/23566#pullrequestreview-2634091549
Re: RFR: 8328914: Document the java.security.debug property in javadoc [v6]
> java.security.debug is a widely used debug system property for JDK security > libs. It's time to capture details about this property via javadoc. > >  > > > NOTE : We are adding a new html file (similar to the Networking Properties > [here](https://download.java.net/java/early_access/jdk25/docs/api/java.base/java/net/doc-files/net-properties.html#networking-properties-heading)) > for documenting security-related properties, and over time, we will add more > properties to this page. Koushik Muthukrishnan Thirupattur has updated the pull request incrementally with one additional commit since the last revision: 8328914: Document the java.security.debug property in javadoc - Changes: - all: https://git.openjdk.org/jdk/pull/23569/files - new: https://git.openjdk.org/jdk/pull/23569/files/dbedca43..46954537 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=23569&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=23569&range=04-05 Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/23569.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23569/head:pull/23569 PR: https://git.openjdk.org/jdk/pull/23569
RFR: 8350456: Test javax/crypto/CryptoPermissions/InconsistentEntries.java crashed: EXCEPTION_ACCESS_VIOLATION
The test javax/crypto/CryptoPermissions/InconsistentEntries.java uses CDSTestUtils.clone to copy the JDK into the scratch dir by creating symbolic links, but this was seen to crash the VM in Windows Server 2025. To ensure test stability, it should hard copy the required files. - Commit messages: - refactoring imports in FileUtils - moved copyDirectory to FileUtils - hard copying jdk folder to scratch Changes: https://git.openjdk.org/jdk/pull/23726/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=23726&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8350456 Stats: 29 lines in 2 files changed: 27 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/23726.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23726/head:pull/23726 PR: https://git.openjdk.org/jdk/pull/23726
Re: RFR: 8346129: Simplify EdDSA & XDH curve name usage
On Fri, 21 Feb 2025 18:36:39 GMT, Sean Mullan wrote:
>> @wangweij is planning on name usage for those. I'm focusing on these older
>> curves.
>
> They are already defined. I think you just want to add something like:
>
>
> If (key.getAlgorithm().equals("ML-KEM") ||
> key.getAlgorithm().equals("ML-DSA")) {
>return ((NamedParameterSpec) key.getParams()).getName();
> }
>
>
> Not urgent, but useful if one of these algorithms were to weaken or be broken
> for some reason.
Or what about this?
if (key instanceof AsymmetricKey ak) {
if (ak.getParams() instanceof NamedParameterSpec nps) {
return nps.getName();
}
}
return key.getAlgorithm();
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23647#discussion_r1966113001
Re: RFR: 8346129: Simplify EdDSA & XDH curve name usage
On Fri, 14 Feb 2025 18:44:38 GMT, Anthony Scarpino wrote: > Hi, > > I need a review for the following change. Naming conventions for EdDSA and > XDH have inconsistencies between DisabledAlgorithms and KeyPairGenerator. > These internal changes help make it more consistent when parsing the actual > curve being used vs the broader algorithm name. > > thanks > > Tony src/java.base/share/classes/sun/security/util/AbstractAlgorithmConstraints.java line 78: > 76: private static List aliasEd25519 = null; > 77: private static List aliasXDH = null; > 78: private static List aliasX25519 = null; I am a little suspicious in this approach. At least this means for each "family" algorithm name like "EdDSA", we need to hardcode all its parameter set names here. Sounds not very sustainable. An EdDSA key always has its `getAlgorithm` being "EdDSA" (at least inside SunEC) and its `getParams()` being the parameter set name. So it looks like it's enough if we do a name comparison on both. Also, why no `aliasEd448` and `aliasX448` here? - PR Review Comment: https://git.openjdk.org/jdk/pull/23647#discussion_r1966129144
Integrated: 8349759: Add unit test for CertificateBuilder and SimpleOCSPServer test utilities
On Tue, 11 Feb 2025 17:50:45 GMT, Jamil Nimeh wrote: > This fix makes some minor changes to the internals of the > `CertificateBuilder` and `SimpleOCSPServer` test classes. They would break > when ML-DSA was selected as key and signing algorithms. Also RSASSA-PSS > works better now with these changes. I've also taken this opportunity to do > some cleanup on CertificateBuilder and added a method which uses a default > signing algorithm based on the key, so the `build()` method no longer needs > to provide that algorithm (though one can if they wish for things like RSA > signatures if they want a different message digest in the signature). This pull request has now been integrated. Changeset: 9d9d7a17 Author:Jamil Nimeh URL: https://git.openjdk.org/jdk/commit/9d9d7a17d3d1a8971712ef1b22e919012350db6f Stats: 329 lines in 3 files changed: 270 ins; 21 del; 38 mod 8349759: Add unit test for CertificateBuilder and SimpleOCSPServer test utilities Reviewed-by: mullan - PR: https://git.openjdk.org/jdk/pull/23566
Re: RFR: 8346129: Simplify EdDSA & XDH curve name usage
On Fri, 21 Feb 2025 20:10:33 GMT, Weijun Wang wrote:
>> They are already defined. I think you just want to add something like:
>>
>>
>> If (key.getAlgorithm().equals("ML-KEM") ||
>> key.getAlgorithm().equals("ML-DSA")) {
>>return ((NamedParameterSpec) key.getParams()).getName();
>> }
>>
>>
>> Not urgent, but useful if one of these algorithms were to weaken or be
>> broken for some reason.
>
> Or what about this?
>
> if (key instanceof AsymmetricKey ak) {
> if (ak.getParams() instanceof NamedParameterSpec nps) {
> return nps.getName();
> }
> }
> return key.getAlgorithm();
>
> `AsymmetricKey` was introduced to make our lives easier.
I stayed away from that because this is likely being backported
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23647#discussion_r1966134944
Re: RFR: 8350456: Test javax/crypto/CryptoPermissions/InconsistentEntries.java crashed: EXCEPTION_ACCESS_VIOLATION
On Fri, 21 Feb 2025 10:31:34 GMT, Fernando Guallini wrote: > The test javax/crypto/CryptoPermissions/InconsistentEntries.java uses > CDSTestUtils.clone to copy the JDK into the scratch dir by creating symbolic > links, but this was seen to crash the VM in Windows Server 2025. To ensure > test stability, it should hard copy the required files. Marked as reviewed by jnimeh (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/23726#pullrequestreview-2634162515
Re: RFR: 8346129: Simplify EdDSA & XDH curve name usage
On Fri, 14 Feb 2025 18:44:38 GMT, Anthony Scarpino
wrote:
> Hi,
>
> I need a review for the following change. Naming conventions for EdDSA and
> XDH have inconsistencies between DisabledAlgorithms and KeyPairGenerator.
> These internal changes help make it more consistent when parsing the actual
> curve being used vs the broader algorithm name.
>
> thanks
>
> Tony
test/jdk/sun/security/util/AlgorithmConstraints/DisabledAlgorithmPermits.java
line 61:
> 59: case "Ed25519" ->
> 60: Arrays.asList(
> 61: new TestCase("EdDSA", false),
As Sean mentioned in another comment, disabling "Ed25519" does not imply all
EdDSA keys are not permitted. This means the result of `permits(primitives,
algorithmName, parameters)` cannot be determined. That said, I noticed you've
used `KeyUtil::getAlgorithm` in a lot of places. Can we guarantee that this
`permits` method is never called on a family algorithm name? If so, we can get
a definitive result.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23647#discussion_r1966138673
Re: RFR: 8328914: Document the java.security.debug property in javadoc [v3]
On Thu, 20 Feb 2025 14:03:37 GMT, Michael McMahon wrote: > As a regular user of the property, this change is a great idea. I think the > text accompanying the table should describe the syntax of the property value. > Is it a comma separated list etc? The syntax has always been a bit loosely specified. So I suggest we don't say anything too specific, but just something like: "The value of the property is one or more options separated by a comma. Some options also have additional sub-options; see the list below for more details on the syntax of each." - PR Comment: https://git.openjdk.org/jdk/pull/23569#issuecomment-2675266655
Re: RFR: 8346129: Simplify EdDSA & XDH curve name usage
On Fri, 21 Feb 2025 17:52:18 GMT, Anthony Scarpino
wrote:
>> src/java.base/share/classes/sun/security/util/KeyUtil.java line 189:
>>
>>> 187: case EdECKey ed -> ed.getParams().getName();
>>> 188: case XECKey xe -> ((NamedParameterSpec)
>>> xe.getParams()).getName();
>>> 189: default -> key.getAlgorithm();
>>
>> Do you also want to add cases for ML-KEM and ML-DSA keys?
>
> @wangweij is planning on name usage for those. I'm focusing on these older
> curves.
They are already defined. I think you just want to add something like:
If (key.getAlgorithm().equals("ML-KEM") || key.getAlgorithm().equals("ML-DSA"))
{
return ((NamedParameterSpec) key.getParams()).getName();
}
Not urgent, but useful if one of these algorithms were to weaken or be broken
for some reason.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23647#discussion_r1966001285
Re: RFR: 8349533: Refactor validator tests shell files to java
On Fri, 21 Feb 2025 15:05:00 GMT, Weijun Wang wrote: >> Changed shell files to be java tests: >> * ./validator/certreplace.sh >> * ./validator/samedn.sh > > test/jdk/sun/security/validator/CertReplace.java line 117: > >> 115: final String outputInt = SecurityTools.keytool(ktBaseParameters >> + >> 116:"-export -rfc >> -alias int").getOutput(); >> 117: Files.write(certPath, outputInt.getBytes(), >> StandardOpenOption.APPEND); > > There are several places that can be enhanced, mainly to reduce `keytool` > calling: > 1. There is no need to export certs for `user` and `int`. You already created > them as `user.cert` and `int.cert`. > 2. Since "certreplace.certs" starts with "user.cert", you can directly > `keytool -gencert` into this file on line 103. > 3. There is no need to import "user.cert" to alias user since we will delete > the entry anyway. > 4. Consider replacing `keytool -import` and `keytool -delete` calls using > `KeyStore` API. You can enhance `KeyStoreUtils` in `/test/lib` if worth doing. done in the next commit - PR Review Comment: https://git.openjdk.org/jdk/pull/23727#discussion_r1965868298
Re: RFR: 8348309: MultiNST tests need more debugging and timing [v2]
On Thu, 20 Feb 2025 00:20:30 GMT, Anthony Scarpino wrote: >> I need a review of this change that adds new timing controls for the initial >> server setup. On rare occasions, more so on certain architectures, the >> server may not fully start before the client tries to connect. Additional >> debugging is added to help identify if there are other timing issues. >> >> Thanks >> >> Tony > > Anthony Scarpino has updated the pull request incrementally with one > additional commit since the last revision: > > review comments Marked as reviewed by hchao (Committer). - PR Review: https://git.openjdk.org/jdk/pull/23407#pullrequestreview-2633821964
Re: RFR: 8347606: Optimize Java implementation of ML-DSA
On Fri, 14 Feb 2025 16:43:32 GMT, Ben Perez wrote: > It turns out that initializing a multidimensional array with `int[][] a = new > int[rows][cols]` is slower than allocating each column in a loop. Since we do > a lot of large multidimensional array allocations in ML-DSA, the optimized > initialization improves performance by roughly 10%. This PR might be relevant: https://github.com/openjdk/jdk/pull/22829 - PR Comment: https://git.openjdk.org/jdk/pull/23642#issuecomment-2675219511
Re: RFR: 8350456: Test javax/crypto/CryptoPermissions/InconsistentEntries.java crashed: EXCEPTION_ACCESS_VIOLATION [v2]
> The test javax/crypto/CryptoPermissions/InconsistentEntries.java uses > CDSTestUtils.clone to copy the JDK into the scratch dir by creating symbolic > links, but this was seen to crash the VM in Windows Server 2025. To ensure > test stability, it should hard copy the required files. Fernando Guallini has updated the pull request incrementally with one additional commit since the last revision: updated comment with runtimeException - Changes: - all: https://git.openjdk.org/jdk/pull/23726/files - new: https://git.openjdk.org/jdk/pull/23726/files/98aee523..fbd35872 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=23726&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=23726&range=00-01 Stats: 5 lines in 1 file changed: 1 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/23726.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23726/head:pull/23726 PR: https://git.openjdk.org/jdk/pull/23726
RFR: 8350459: MontgomeryIntegerPolynomialP256 multiply intrinsic with AVX2 on x86_64
Add AVX2 montgomery multiplication intrinsic. (About 60-80% gain) Also add reduction to existing AVX512 multiplication (this was left-over from https://github.com/openjdk/jdk/pull/19893 where a quick fix was required). This is mostly for cleanup, but there is about 1-2% gain. Before (no AVX512) Benchmark(algorithm) (dataSize) (keyLength) (provider) Mode Cnt Score Error Units SignatureBench.ECDSA.signSHA256withECDSA1024 256 thrpt 40 3720.589 ± 17.879 ops/s SignatureBench.ECDSA.signSHA256withECDSA 16384 256 thrpt 40 3605.940 ± 15.807 ops/s SignatureBench.ECDSA.verify SHA256withECDSA1024 256 thrpt 40 1076.502 ± 4.190 ops/s SignatureBench.ECDSA.verify SHA256withECDSA 16384 256 thrpt 40 1069.624 ± 2.484 ops/s Benchmark (algorithm) (keyLength) (kpgAlgorithm) (provider) Mode Cnt Score Error Units KeyAgreementBench.EC.generateSecret ECDH 256 EC thrpt 40 830.448 ± 2.285 ops/s After (with AVX2) Benchmark(algorithm) (dataSize) (keyLength) (provider) Mode Cnt Score Error Units SignatureBench.ECDSA.signSHA256withECDSA1024 256 thrpt 40 6000.496 ± 39.923 ops/s SignatureBench.ECDSA.signSHA256withECDSA 16384 256 thrpt 40 5739.878 ± 34.838 ops/s SignatureBench.ECDSA.verify SHA256withECDSA1024 256 thrpt 40 1942.437 ± 12.179 ops/s SignatureBench.ECDSA.verify SHA256withECDSA 16384 256 thrpt 40 1921.770 ± 8.992 ops/s Benchmark (algorithm) (keyLength) (kpgAlgorithm) (provider) Mode Cnt Score Error Units KeyAgreementBench.EC.generateSecret ECDH 256 EC thrpt 40 1399.761 ± 6.238 ops/s Before (with AVX512): Benchmark(algorithm) (dataSize) (keyLength) (provider) Mode Cnt Score Error Units SignatureBench.ECDSA.signSHA256withECDSA1024 256 thrpt 409621.950 ± 27.260 ops/s SignatureBench.ECDSA.signSHA256withECDSA 16384 256 thrpt 408975.654 ± 26.707 ops/s SignatureBench.ECDSA.verify SHA256withECDSA1024 256 thrpt 403112.945 ± 12.930 ops/s SignatureBench.ECDSA.verify SHA256withECDSA 16384 256 thrpt 403039.183 ± 12.362 ops/s Benchmark (algorithm) (keyLength) (kpgAlgorithm) (provider) Mode Cnt ScoreError Units KeyAgreementBench.EC.generateSecret ECDH 256 EC thrpt 40 2248.987 ± 7.427 ops/s After (with AVX512): Benchmark(algorithm) (dataSize) (keyLength) (provider) Mode Cnt Score Error Units SignatureBench.ECDSA.signSHA256withECDSA1024 256 thrpt 409815.713 ± 23.455 ops/s SignatureBench.ECDSA.signSHA256withECDSA 16384 256 thrpt 409136.786 ± 27.747 ops/s SignatureBench.ECDSA.verify SHA256withECDSA1024 256 thrpt 403167.702 ± 13.331 ops/s SignatureBench.ECDSA.verify SHA256withECDSA 16384 256 thrpt 403090.053 ± 12.925 ops/s Benchmark (algorithm) (keyLength) (kpgAlgorithm) (provider) Mode Cnt ScoreError Units KeyAgreementBench.EC.generateSecret ECDH 256 EC thrpt 40 2278.031 ± 6.971 ops/s - Commit messages: - whitespace - split up ASM and Math changes Changes: https://git.openjdk.org/jdk/pull/23719/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=23719&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8350459 Stats: 625 lines in 9 files changed: 525 ins; 15 del; 85 mod Patch: https://git.openjdk.org/jdk/pull/23719.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23719/head:pull/23719 PR: https://git.openjdk.org/jdk/pull/23719
Re: RFR: 8347606: Optimize Java implementation of ML-DSA
On Fri, 14 Feb 2025 16:43:32 GMT, Ben Perez wrote: > It turns out that initializing a multidimensional array with `int[][] a = new > int[rows][cols]` is slower than allocating each column in a loop. Since we do > a lot of large multidimensional array allocations in ML-DSA, the optimized > initialization improves performance by roughly 10%. We don't need to bring hotspot in. The same issue has been described in https://bugs.openjdk.org/browse/JDK-8308105. We can proceed with this patch. src/java.base/share/classes/sun/security/provider/ML_DSA.java line 2: > 1: /* > 2: * Copyright (c) 2024, 2025 Oracle and/or its affiliates. All rights > reserved. Suggestion: * Copyright (c) 2024, 2025, Oracle and/or its affiliates. All rights reserved. - PR Comment: https://git.openjdk.org/jdk/pull/23642#issuecomment-2675063845 PR Review Comment: https://git.openjdk.org/jdk/pull/23642#discussion_r1965870968
Re: RFR: 8349533: Refactor validator tests shell files to java [v2]
> Changed shell files to be java tests: > * ./validator/certreplace.sh > * ./validator/samedn.sh Mikhail Yankelevich has updated the pull request incrementally with one additional commit since the last revision: keyStore is not used to delete, cleanup of the calls, minor refactoring - Changes: - all: https://git.openjdk.org/jdk/pull/23727/files - new: https://git.openjdk.org/jdk/pull/23727/files/a2eed266..f333ddff Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=23727&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=23727&range=00-01 Stats: 95 lines in 1 file changed: 43 ins; 26 del; 26 mod Patch: https://git.openjdk.org/jdk/pull/23727.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23727/head:pull/23727 PR: https://git.openjdk.org/jdk/pull/23727
Re: RFR: 8349533: Refactor validator tests shell files to java [v2]
On Fri, 21 Feb 2025 16:57:32 GMT, Mikhail Yankelevich wrote: >> Changed shell files to be java tests: >> * ./validator/certreplace.sh >> * ./validator/samedn.sh > > Mikhail Yankelevich has updated the pull request incrementally with one > additional commit since the last revision: > > keyStore is not used to delete, cleanup of the calls, minor refactoring test/jdk/sun/security/validator/CertReplace.java line 28: > 26: */ > 27: > 28: import java.io.FileInputStream; Remove the "This test is called by certreplace.sh" line above. test/jdk/sun/security/validator/CertReplace.java line 64: > 62: * > 63: * @run main/othervm CertReplace samedn.jks samedn1.certs > 64: * @run main/othervm CertReplace samedn.jks samedn2.certs Must these tests run in `othervm`? They haven't changed any static VM properties. test/jdk/sun/security/validator/CertReplace.java line 82: > 80: final String intAliase = "int"; > 81: final String userAlias = "user"; > 82: final String caAlias = "ca"; Do you really believe creating these variables help the program looking better? There are so many string concatenations in the keytool commands. If it were me, I would remove every variable for a string literal except for `ktBaseParameters`. On the other hand, it's OK to put `"changeit".toCharArray()` into a variable. test/jdk/sun/security/validator/CertReplace.java line 120: > 118: new FileInputStream(intCertFileName) > 119: ) > 120: }; Use `CertUtils.getCertFromStream`. Also, it's better to close the file. Maybe Windows dislikes open files. test/jdk/sun/security/validator/CertReplace.java line 147: > 145: "-selfcert -alias " + caAlias); > 146: keyStore.deleteEntry(intAliase); > 147: keyStore.deleteEntry(userAlias); Call `keyStore.store` to actually remove the 2 entries inside the keystore file. But this needs to be done before the `-selfcert` call, otherwise, the old cert would overwrite the new one. test/jdk/sun/security/validator/CertReplace.java line 197: > 195: > 196: // 3. Remove user for cacerts > 197: keyStore.deleteEntry(userAlias); Call `keyStore.store` to write to the keystore. - PR Review Comment: https://git.openjdk.org/jdk/pull/23727#discussion_r1965994249 PR Review Comment: https://git.openjdk.org/jdk/pull/23727#discussion_r1966022164 PR Review Comment: https://git.openjdk.org/jdk/pull/23727#discussion_r1965999262 PR Review Comment: https://git.openjdk.org/jdk/pull/23727#discussion_r1966008483 PR Review Comment: https://git.openjdk.org/jdk/pull/23727#discussion_r1966005851 PR Review Comment: https://git.openjdk.org/jdk/pull/23727#discussion_r1966016971
Re: RFR: 8346129: Simplify EdDSA & XDH curve name usage
On Thu, 20 Feb 2025 14:20:34 GMT, Sean Mullan wrote: >> Hi, >> >> I need a review for the following change. Naming conventions for EdDSA and >> XDH have inconsistencies between DisabledAlgorithms and KeyPairGenerator. >> These internal changes help make it more consistent when parsing the actual >> curve being used vs the broader algorithm name. >> >> thanks >> >> Tony > > src/java.base/share/classes/sun/security/util/KeyUtil.java line 189: > >> 187: case EdECKey ed -> ed.getParams().getName(); >> 188: case XECKey xe -> ((NamedParameterSpec) >> xe.getParams()).getName(); >> 189: default -> key.getAlgorithm(); > > Do you also want to add cases for ML-KEM and ML-DSA keys? @wangweij is planning on name usage for those. I'm focusing on these older curves. - PR Review Comment: https://git.openjdk.org/jdk/pull/23647#discussion_r1965944775
Re: RFR: 8346129: Simplify EdDSA & XDH curve name usage
On Fri, 14 Feb 2025 18:44:38 GMT, Anthony Scarpino
wrote:
> Hi,
>
> I need a review for the following change. Naming conventions for EdDSA and
> XDH have inconsistencies between DisabledAlgorithms and KeyPairGenerator.
> These internal changes help make it more consistent when parsing the actual
> curve being used vs the broader algorithm name.
>
> thanks
>
> Tony
src/java.base/share/classes/sun/security/util/AbstractAlgorithmConstraints.java
line 95:
> 93: case "Ed25519" -> {
> 94: if (aliasEd25519 == null) {
> 95: aliasEd25519 = List.of("EdDSA", "Ed25519");
Hmm. Should disabling Ed25519 also disable EdDSA? I can see the reverse, but
isn't Ed25519 meant to be a specific curve for EdDSA?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23647#discussion_r1966046628
Re: RFR: 8350456: Test javax/crypto/CryptoPermissions/InconsistentEntries.java crashed: EXCEPTION_ACCESS_VIOLATION [v2]
On Fri, 21 Feb 2025 20:51:11 GMT, Fernando Guallini wrote: >> The test javax/crypto/CryptoPermissions/InconsistentEntries.java uses >> CDSTestUtils.clone to copy the JDK into the scratch dir by creating symbolic >> links, but this was seen to crash the VM in Windows Server 2025. To ensure >> test stability, it should hard copy the required files. > > Fernando Guallini has updated the pull request incrementally with one > additional commit since the last revision: > > updated comment with runtimeException Marked as reviewed by rhalade (Reviewer). test/lib/jdk/test/lib/util/FileUtils.java line 377: > 375: * @param dst the path of the destination directory > 376: * @throws IOException > 377: * if an I/O error occurs Suggestion: * @throws RuntimeException if an I/O error occurs during the copy operation * or if the source or destination paths are invalid - PR Review: https://git.openjdk.org/jdk/pull/23726#pullrequestreview-2634146048 PR Review Comment: https://git.openjdk.org/jdk/pull/23726#discussion_r1966126842
Re: RFR: 8346129: Simplify EdDSA & XDH curve name usage
On Fri, 21 Feb 2025 19:15:21 GMT, Sean Mullan wrote:
>> Hi,
>>
>> I need a review for the following change. Naming conventions for EdDSA and
>> XDH have inconsistencies between DisabledAlgorithms and KeyPairGenerator.
>> These internal changes help make it more consistent when parsing the actual
>> curve being used vs the broader algorithm name.
>>
>> thanks
>>
>> Tony
>
> src/java.base/share/classes/sun/security/util/AbstractAlgorithmConstraints.java
> line 95:
>
>> 93: case "Ed25519" -> {
>> 94: if (aliasEd25519 == null) {
>> 95: aliasEd25519 = List.of("EdDSA", "Ed25519");
>
> Hmm. Should disabling Ed25519 also disable EdDSA? I can see the reverse, but
> isn't Ed25519 meant to be a specific curve for EdDSA?
This is complicated by `KeyPairGenerator.getInstance("EdDSA")` returning an
Ed25519 key
If someone were to check permits() with "EdDSA" the above code recognizes that
"Ed25519" on the disabled algorithm list overlaps with "EdDSA". This is the
first test in the test coded included in the PR.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23647#discussion_r1966171536
Re: RFR: 8346129: Simplify EdDSA & XDH curve name usage
On Fri, 21 Feb 2025 20:35:34 GMT, Weijun Wang wrote:
>> Hi,
>>
>> I need a review for the following change. Naming conventions for EdDSA and
>> XDH have inconsistencies between DisabledAlgorithms and KeyPairGenerator.
>> These internal changes help make it more consistent when parsing the actual
>> curve being used vs the broader algorithm name.
>>
>> thanks
>>
>> Tony
>
> test/jdk/sun/security/util/AlgorithmConstraints/DisabledAlgorithmPermits.java
> line 61:
>
>> 59: case "Ed25519" ->
>> 60: Arrays.asList(
>> 61: new TestCase("EdDSA", false),
>
> As Sean mentioned in another comment, disabling "Ed25519" does not imply all
> EdDSA keys are not permitted. This means the result of `permits(primitives,
> algorithmName, parameters)` cannot be determined. That said, I noticed you've
> used `KeyUtil::getAlgorithm` in a lot of places. Can we guarantee that this
> `permits` method is never called on a family algorithm name? If so, we can
> get a definitive result.
I believe my comment to Sean answers this question, but I'm not sure I
understand the last question in your comment. "never called on a family
algorithm name". The change is to make sure these two families return the
curve name and not the family name (EdDSA & XDH). But on the other side,
someone using the family name of the disabled algorithm list would disable all
curves.
The above test code is checking that this call `
permits(Set.of(CryptoPrimitive.SIGNATURE), "EdDSA", null)` will fail for a
Ed25519 key because of the precedent set by KPG.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23647#discussion_r1966189174
Integrated: 8350456: Test javax/crypto/CryptoPermissions/InconsistentEntries.java crashed: EXCEPTION_ACCESS_VIOLATION
On Fri, 21 Feb 2025 10:31:34 GMT, Fernando Guallini wrote: > The test javax/crypto/CryptoPermissions/InconsistentEntries.java uses > CDSTestUtils.clone to copy the JDK into the scratch dir by creating symbolic > links, but this was seen to crash the VM in Windows Server 2025. To ensure > test stability, it should hard copy the required files. This pull request has now been integrated. Changeset: 825ab20b Author:Fernando Guallini URL: https://git.openjdk.org/jdk/commit/825ab20ba99b1f1127dd94b87ae56020d1831529 Stats: 30 lines in 2 files changed: 28 ins; 1 del; 1 mod 8350456: Test javax/crypto/CryptoPermissions/InconsistentEntries.java crashed: EXCEPTION_ACCESS_VIOLATION Reviewed-by: rhalade, jnimeh - PR: https://git.openjdk.org/jdk/pull/23726
Re: RFR: 8346129: Simplify EdDSA & XDH curve name usage
On Fri, 21 Feb 2025 20:25:59 GMT, Weijun Wang wrote: >> Hi, >> >> I need a review for the following change. Naming conventions for EdDSA and >> XDH have inconsistencies between DisabledAlgorithms and KeyPairGenerator. >> These internal changes help make it more consistent when parsing the actual >> curve being used vs the broader algorithm name. >> >> thanks >> >> Tony > > src/java.base/share/classes/sun/security/util/AbstractAlgorithmConstraints.java > line 78: > >> 76: private static List aliasEd25519 = null; >> 77: private static List aliasXDH = null; >> 78: private static List aliasX25519 = null; > > I am a little suspicious in this approach. At least this means for each > "family" algorithm name like "EdDSA", we need to hardcode all its parameter > set names here. Sounds not very sustainable. > > An EdDSA key always has its `getAlgorithm` being "EdDSA" (at least inside > SunEC) and its `getParams()` being the parameter set name. So it looks like > it's enough if we do a name comparison on both. > > Also, why no `aliasEd448` and `aliasX448` here? I have to give more thought on checking the algorithm and the `getParams()` against the list. That may eliminate the need for the hardcoded list.. As to why 448 curves didn't need an alias, there is no other way to specify those curves other than their given name, like mentioned with the KPG/Ed25519 example in my comment to Sean - PR Review Comment: https://git.openjdk.org/jdk/pull/23647#discussion_r1966173549
Re: RFR: 8346129: Simplify EdDSA & XDH curve name usage
On Fri, 21 Feb 2025 21:09:32 GMT, Anthony Scarpino
wrote:
>> src/java.base/share/classes/sun/security/util/AbstractAlgorithmConstraints.java
>> line 95:
>>
>>> 93: case "Ed25519" -> {
>>> 94: if (aliasEd25519 == null) {
>>> 95: aliasEd25519 = List.of("EdDSA", "Ed25519");
>>
>> Hmm. Should disabling Ed25519 also disable EdDSA? I can see the reverse, but
>> isn't Ed25519 meant to be a specific curve for EdDSA?
>
> This is complicated by `KeyPairGenerator.getInstance("EdDSA")` returning an
> Ed25519 key
>
> If someone were to check permits() with "EdDSA" the above code recognizes
> that "Ed25519" on the disabled algorithm list overlaps with "EdDSA". This is
> the first test in the test coded included in the PR.
Do we call `permits` before instantiating a `KeyPairGenerator`? What if people
call `kpg.initialize(NPS.Ed448)` after the instantiation?
In reality, I think it depends on how many `permits` calls there are. Modern
algorithms have the key same algorithm name and signature algorithm name. When
a signature operation is carried out, do we check on both the signature
algorithm and the key? It seems only checking on the key is enough. It's
actually more precise, since you can get the exact parameter set name there.
This is why I asked if the method is "never called on a family algorithm name".
When checking a key, if we always call `permits` on the parameter set name, we
get the precise result.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23647#discussion_r1966259933
Re: RFR: 8346129: Simplify EdDSA & XDH curve name usage
On Fri, 21 Feb 2025 21:21:24 GMT, Anthony Scarpino
wrote:
>> test/jdk/sun/security/util/AlgorithmConstraints/DisabledAlgorithmPermits.java
>> line 61:
>>
>>> 59: case "Ed25519" ->
>>> 60: Arrays.asList(
>>> 61: new TestCase("EdDSA", false),
>>
>> As Sean mentioned in another comment, disabling "Ed25519" does not imply all
>> EdDSA keys are not permitted. This means the result of `permits(primitives,
>> algorithmName, parameters)` cannot be determined. That said, I noticed
>> you've used `KeyUtil::getAlgorithm` in a lot of places. Can we guarantee
>> that this `permits` method is never called on a family algorithm name? If
>> so, we can get a definitive result.
>
> I believe my comment to Sean answers this question, but I'm not sure I
> understand the last question in your comment. "never called on a family
> algorithm name". The change is to make sure these two families return the
> curve name and not the family name (EdDSA & XDH). But on the other side,
> someone using the family name of the disabled algorithm list would disable
> all curves.
> The above test code is checking that this call `
> permits(Set.of(CryptoPrimitive.SIGNATURE), "EdDSA", null)` will fail for a
> Ed25519 key because of the precedent set by KPG.
We are talking about the same in multiple comments now.
In this case, if both `permits(SIGNATURE, "EdDSA", null)` and
`permits(SIGNATURE, key)` are called, it's safe to bypass the 1st check as long
as the 2nd one blocks the key. So it's not necessary to cover "EdDSA" when only
"Ed25519" is disabled.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23647#discussion_r1966263224
Re: RFR: 8346129: Simplify EdDSA & XDH curve name usage
On Fri, 21 Feb 2025 21:11:54 GMT, Anthony Scarpino wrote: >> src/java.base/share/classes/sun/security/util/AbstractAlgorithmConstraints.java >> line 78: >> >>> 76: private static List aliasEd25519 = null; >>> 77: private static List aliasXDH = null; >>> 78: private static List aliasX25519 = null; >> >> I am a little suspicious in this approach. At least this means for each >> "family" algorithm name like "EdDSA", we need to hardcode all its parameter >> set names here. Sounds not very sustainable. >> >> An EdDSA key always has its `getAlgorithm` being "EdDSA" (at least inside >> SunEC) and its `getParams()` being the parameter set name. So it looks like >> it's enough if we do a name comparison on both. >> >> Also, why no `aliasEd448` and `aliasX448` here? > > I have to give more thought on checking the algorithm and the `getParams()` > against the list. That may eliminate the need for the hardcoded list.. > > As to why 448 curves didn't need an alias, there is no other way to specify > those curves other than their given name, like mentioned with the KPG/Ed25519 > example in my comment to Sean So using the `getAlgorithm()` and `getParams().getName()` work because there is a Key, but not for non-key situation such as `permits(Set.of(CryptoPrimitive.SIGNATURE), "EdDSA", null)`. But this does bring up a point I had not considered. The `permits()` call does not specify any key details other than the family name. Perhaps it's ok for `permits()` to return a passing result with the information given. But for a `permit()` that had more detail, it could return a failing result. However, the KPG example does make me think that the consistency in the current PR is better. - PR Review Comment: https://git.openjdk.org/jdk/pull/23647#discussion_r1966265139
RFR: 8349533: Refactor validator tests shell files to java
Changed shell files to be java tests: * ./validator/certreplace.sh * ./validator/samedn.sh - Commit messages: - changed to 2 different test ids - 8349533: Refactor validator tests shell files to java Changes: https://git.openjdk.org/jdk/pull/23727/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=23727&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8349533 Stats: 326 lines in 3 files changed: 151 ins; 174 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/23727.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23727/head:pull/23727 PR: https://git.openjdk.org/jdk/pull/23727
Re: RFR: 8325766: Review seclibs tests for cert expiry [v2]
On Thu, 20 Feb 2025 20:27:27 GMT, Weijun Wang wrote: > The similarity between the certificate pairs is impressive! Just curious - > why the change in issuer and owner names? Looks like it's something between `keytool` and `openssl x509`. When i print the certificates with openssl, the issuer and owner names are in the opposite order from keytool. - PR Comment: https://git.openjdk.org/jdk/pull/23700#issuecomment-2674599121
Re: RFR: 8348561: Add aarch64 intrinsics for ML-DSA [v5]
On Tue, 18 Feb 2025 13:33:52 GMT, Andrew Dinn wrote: >> Ferenc Rakoczi has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Adding comments + some code reorganization > > src/hotspot/cpu/aarch64/assembler_aarch64.hpp line 2594: > >> 2592: guarantee(T != T1Q && T != T1D, "incorrect arrangement"); >> \ >> 2593: if (!acceptT2D) guarantee(T != T2D, "incorrect arrangement"); >> \ >> 2594: if (strcmp(#NAME, "sqdmulh") == 0) guarantee(T != T8B && T != >> T16B, "incorrect arrangement"); \ > > Suggestion: > > I think it might be better to change this test from a strcmp call to (opc2 == > 0b101101). The strcmp test is clearer to a reader of the code but the call > may not be guaranteed to be compiled out at build time while the latter will. Changed as suggested. - PR Review Comment: https://git.openjdk.org/jdk/pull/23300#discussion_r1965215153
Re: RFR: 8348561: Add aarch64 intrinsics for ML-DSA [v5]
On Tue, 18 Feb 2025 13:43:18 GMT, Andrew Dinn wrote: >> Ferenc Rakoczi has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Adding comments + some code reorganization > > src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 4066: > >> 4064: } >> 4065: >> 4066: // Execute on round of keccak of two computations in parallel. > > Suggestion: > > It would be helpful to add comments that relate the register and instruction > selection to the original Java source code. e.g. change the header as follows > > // Performs 2 keccak round transformations using vector parallelism > // > // Two sets of 25 * 64-bit input states a0[lo:hi]...a24[lo:hi] are passed > in > // the lower/upper halves of registers v0...v24 and the transformed states > // are returned in the same registers. Intermediate 64-bit pairs > // c0...c5 and d0...d5 are computed in registers v25...v30. v31 is > // loaded with the required pair of 64 bit rounding constants. > // During computation of the output states some intermediate results are > // shuffled around registers v0...v30. Comments on each line indicate > // how the values in registers correspond to variables ai, ci, di in > // the Java source code, likewise how the generated machine instructions > // correspond to Java source operations (n.b. rol means rotate left). > > The annotate the generation steps as follows: > > __ eor3(v29, __ T16B, v4, v9, v14); // c4 = a4 ^ a9 ^ a14 > __ eor3(v26, __ T16B, v1, v6, v11); // c1 = a1 ^ a16 ^ a11 > __ eor3(v28, __ T16B, v3, v8, v13); // c3 = a3 ^ a8 ^a13 > __ eor3(v25, __ T16B, v0, v5, v10); // c0 = a0 ^ a5 ^ a10 > __ eor3(v27, __ T16B, v2, v7, v12); // c2 = a2 ^ a7 ^ a12 > __ eor3(v29, __ T16B, v29, v19, v24); // c4 ^= a19 ^ a24 > __ eor3(v26, __ T16B, v26, v16, v21); // c1 ^= a16 ^ a21 > __ eor3(v28, __ T16B, v28, v18, v23); // c3 ^= a18 ^ a23 > __ eor3(v25, __ T16B, v25, v15, v20); // c0 ^= a15 ^ a20 > __ eor3(v27, __ T16B, v27, v17, v22); // c2 ^= a17 ^ a22 > > __ rax1(v30, __ T2D, v29, v26); // d0 = c4 ^ rol(c1, 1) > __ rax1(v26, __ T2D, v26, v28); // d2 = c1 ^ rol(c3, 1) > __ rax1(v28, __ T2D, v28, v25); // d4 = c3 ^ rol(c0, 1) > __ rax1(v25, __ T2D, v25, v27); // d1 = c0 ^ rol(c2, 1) > __ rax1(v27, __ T2D, v27, v29); // d3 = c2 ^ rol(c4, 1) > > __ eor(v0, __ T16B, v0, v30); // a0 = a0 ^ d0 > __ xar(v29, __ T2D, v1, v25, (64 - 1)); // a10' = rol((a1^d1), 1) > __ xar(v1, __ T2D, v6, v25, (64 - 44)); // a1 = rol(a6^d1), 44) > __ xar(v6, __ T2D, v9, v28, (64 - 20)); // a6 = rol((a9^d4), 20) > __ xar(v... Although this piece of code is not new, and I don't really think that this level of commenting is necessary, especially in code that is very unlikely to change, I added the comments. - PR Review Comment: https://git.openjdk.org/jdk/pull/23300#discussion_r1965220606
Re: RFR: 8348561: Add aarch64 intrinsics for ML-DSA [v5]
On Wed, 19 Feb 2025 02:55:18 GMT, Hao Sun wrote: >> Ferenc Rakoczi has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Adding comments + some code reorganization > > Hi. Here is the test result of our CI. > > ### copyright year > > the following files should update the copyright year to 2025. > > > src/hotspot/cpu/aarch64/assembler_aarch64.hpp > src/hotspot/cpu/aarch64/stubRoutines_aarch64.hpp > src/hotspot/share/runtime/globals.hpp > src/java.base/share/classes/sun/security/provider/ML_DSA.java > src/java.base/share/classes/sun/security/provider/SHA3Parallel.java > test/micro/org/openjdk/bench/java/security/MLDSA.java > > > ### cross-build failure > > Cross build for riscv64/s390/ppc64 failed. > > Here shows the error msg for ppc64 > > > === Output from failing command(s) repeated here === > * For target support_interim-jmods_support__create_java.base.jmod_exec: > # > # A fatal error has been detected by the Java Runtime Environment: > # > # Internal Error (/tmp/jdk-src/src/hotspot/share/asm/codeBuffer.hpp:200), > pid=72752, tid=72769 > # assert(allocates2(pc)) failed: not in CodeBuffer memory: > 0xe85cb03dc620 <= 0xe85cb03e8ab4 <= 0xe85cb03e8ab0 > # > # JRE version: OpenJDK Runtime Environment (25.0) (fastdebug build > 25-internal-git-1e01c6deec3) > # Java VM: OpenJDK 64-Bit Server VM (fastdebug 25-internal-git-1e01c6deec3, > mixed mode, tiered, compressed oops, compressed class ptrs, g1 gc, > linux-aarch64) > # Problematic frame: > # V [libjvm.so+0x3b391c] Instruction_aarch64::~Instruction_aarch64()+0xbc > # > # Core dump will be written. Default location: Core dumps may be processed > with "/usr/share/apport/apport -p%p -s%s -c%c -d%d -P%P -u%u -g%g -- %E" (or > dumping to /tmp/ci-scripts/jdk-src/make/ > # > # An error report file with more information is saved as: > # /tmp/jdk-src/make/hs_err_pid72752.log >... (rest of output omitted) > > * All command lines available in > /sysroot/ppc64el/tmp/build-ppc64el/make-support/failure-logs. > === End of repeated output === > > > I suppose we should make the similar update at file > `src/hotspot/cpu/aarch64/stubDeclarations_aarch64.hpp` to other platforms @shqking, I changed the copyright years, but I don't really understand how the aarch64-specific code can overflow buffers on other architectures. As far as I understand, Instruction_aarch64 should not have been there in a ppc build. Was this a build attempted on an aarch64 for the other architectures? - PR Comment: https://git.openjdk.org/jdk/pull/23300#issuecomment-2674156680
Re: RFR: 8346129: Simplify EdDSA & XDH curve name usage
On Fri, 21 Feb 2025 22:05:03 GMT, Weijun Wang wrote:
>> This is complicated by `KeyPairGenerator.getInstance("EdDSA")` returning an
>> Ed25519 key
>>
>> If someone were to check permits() with "EdDSA" the above code recognizes
>> that "Ed25519" on the disabled algorithm list overlaps with "EdDSA". This
>> is the first test in the test coded included in the PR.
>
> Do we call `permits` before instantiating a `KeyPairGenerator`? What if
> people call `kpg.initialize(NPS.Ed448)` after the instantiation?
>
> In reality, I think it depends on how many `permits` calls there are. Modern
> algorithms have the key same algorithm name and signature algorithm name.
> When a signature operation is carried out, do we check on both the signature
> algorithm and the key? It seems only checking on the key is enough. It's
> actually more precise, since you can get the exact parameter set name there.
> This is why I asked if the method is "never called on a family algorithm
> name". When checking a key, if we always call `permits` on the parameter set
> name, we get the precise result.
`permits()` are used in situations for
jdk[tls|certpath|jar].disabledAlgorithms, and the SSLAlgorithmConstraints.
It's not called for APIs like KPG, Signature, etc.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23647#discussion_r1966279183
RFR: 8350476: Fix typo introduced in JDK-8350147
Typo: s/ficticious/fictitious/ No unit test. Check that javadoc still builds. - Commit messages: - 8350476: Fix typo introduced in JDK-8350147 Changes: https://git.openjdk.org/jdk/pull/23733/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=23733&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8350476 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/23733.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23733/head:pull/23733 PR: https://git.openjdk.org/jdk/pull/23733
Re: RFR: 8350476: Fix typo introduced in JDK-8350147
On Sat, 22 Feb 2025 02:25:42 GMT, Bradford Wetmore wrote: > Typo: s/ficticious/fictitious/ > > No unit test. Check that javadoc still builds. Marked as reviewed by jnimeh (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/23733#pullrequestreview-2634674252
Re: RFR: 8347606: Optimize Java implementation of ML-DSA
On Sun, 16 Feb 2025 15:41:52 GMT, Chen Liang wrote: >> src/java.base/share/classes/sun/security/provider/ML_DSA.java line 1237: >> >>> 1235: return res; >>> 1236: } >>> 1237: >> >> Centralizing the allocation into a helper on its own Looks unseful (for >> resource Management, debugging/profiling and to pick the optimal >> implementation). >> >> but it’s a shame that 2 dimensional allocations are sub-optimal, shouldnt >> that be addressed in the jvm (or c2?) > > Indeed, it's better if hotspot can recognize and optimize the bytecode > sequence generated by javac, or javac should generate bytecode like these > methods to speed up allocation in general. > > Is splitting the allocation into a dedicated method a factor? I know this may > affect JIT compilation heuristics. It is almost always a mistake to try to generate optimized code in javac behind the user's back. The VM does a better job when javac stays out of the optimization business and just provides a concise bytecode representation of the desired computation. The VM always has more information than javac, and so all optimization descisions are best deferred until VM runtime, when all classes are loaded. In addition, increasing classfile sizes to improve peak performance is likely to harm startup performance, and (of course) static application footprint. Also, even if javac gets an optimization perfectly correct for current platforms (and the footprint costs are negigible), in a few years platforms will change and the VM will want to optimize the bytecodes a different way. But the "optimization" in the classfile, will be obsolete, and the now-suboptimal code will stick around forever in release JARs; the VM's problem will then be to reverse-engineer the "optimized not-optimized" code and reorganize it, which is an unfair ask of the VM. In summary, there's nothing javac can do for optimization that is not better done by the VM, later on. Classfiles are semantic specifications, not performance engines. https://bugs.openjdk.org/browse/JDK-8308105#comment-14755577 In that line of thinking, I recommend replacing the multianewarray instruction, in javac's current translation strategy, with an invokedynamic, which can spin code that is as optimal as we need, now and in the foreseeable future. - PR Review Comment: https://git.openjdk.org/jdk/pull/23642#discussion_r1966296640
Re: RFR: 8346129: Simplify EdDSA & XDH curve name usage
On Fri, 21 Feb 2025 22:29:01 GMT, Anthony Scarpino wrote: >> Do we call `permits` before instantiating a `KeyPairGenerator`? What if >> people call `kpg.initialize(NPS.Ed448)` after the instantiation? >> >> In reality, I think it depends on how many `permits` calls there are. Modern >> algorithms have the key same algorithm name and signature algorithm name. >> When a signature operation is carried out, do we check on both the signature >> algorithm and the key? It seems only checking on the key is enough. It's >> actually more precise, since you can get the exact parameter set name there. >> This is why I asked if the method is "never called on a family algorithm >> name". When checking a key, if we always call `permits` on the parameter set >> name, we get the precise result. > > `permits()` are used in situations for > jdk[tls|certpath|jar].disabledAlgorithms, and the SSLAlgorithmConstraints. > It's not called for APIs like KPG, Signature, etc. That's what I meant. Suppose in TLS when you verify a signature and you call `permits` on both the signature algorithm name and the key used to init the signature, it's OK if only one fails. - PR Review Comment: https://git.openjdk.org/jdk/pull/23647#discussion_r1966282538
Re: RFR: 8346129: Simplify EdDSA & XDH curve name usage
On Fri, 21 Feb 2025 20:31:35 GMT, Anthony Scarpino
wrote:
>> Or what about this?
>>
>> if (key instanceof AsymmetricKey ak) {
>> if (ak.getParams() instanceof NamedParameterSpec nps) {
>> return nps.getName();
>> }
>> }
>> return key.getAlgorithm();
>>
>> `AsymmetricKey` was introduced to make our lives easier.
>
> I stayed away from that because this is likely being backported
Ok, well our backporters are usually good about extracting only what is
necessary, but if not, can you file another issue to add support for disabling
PQC algorithms?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/23647#discussion_r1966165821
Re: RFR: 8350476: Fix typo introduced in JDK-8350147
On Sat, 22 Feb 2025 02:25:42 GMT, Bradford Wetmore wrote: > Typo: s/ficticious/fictitious/ > > No unit test. Check that javadoc still builds. Marked as reviewed by jpai (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/23733#pullrequestreview-2634730559
