Re: RFR: 8367104: Check for RSASSA-PSS parameters when validating certificates against algorithm constraints [v8]
On Thu, 11 Sep 2025 19:57:48 GMT, Artur Barashev wrote: >> src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java >> line 312: >> >>> 310: checksDisabled = false; >>> 311: >>> 312: if (mode == null >> >> I can't find any code where `mode` can be `null`. > > There is no such code currently. But if somebody makes a call with `null` > mode in the future it will create `SupportedSignatureAlgorithmConstraints` > object that will always return `false` on permit calls because of the `if > (supportedAlgorithms == null || supportedAlgorithms.isEmpty())` check below. > So I think it makes sense to check for it here. Ok. - PR Review Comment: https://git.openjdk.org/jdk/pull/27146#discussion_r2344215826
Re: RFR: 8364588: Export the NPE backtracking functionality to general null-checking APIs [v4]
On Tue, 5 Aug 2025 16:04:08 GMT, Chen Liang wrote: >> Provide a general facility for our null check APIs like >> Objects::requireNonNull or future Checks::nullCheck (void), converting the >> existing infrastructure to start tracking from a given stack site (depth >> offset) and a given stack slot (offset value). >> >> This is a necessary prerequisite for >> https://bugs.openjdk.org/browse/JDK-8233268, which proposes enhanced null >> messages to `Objects::requireNonNull`. > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > Update NPE per roger review Hello Chen, I had a look at the changes, but I'm missing some broader context of this change. The JBS issue and the PR description states that this PR is in preparation (to upcoming additional work?) to support better `NullPointerException` exception messages when some code uses `Objects.requireNonNull()`. It also talks about a `java.lang.runtime.Checks::nullCheck` method, but there's no such `java.lang.runtime.Checks` class in mainline and it appears to be a proposed class in Valhalla project (https://bugs.openjdk.org/browse/JDK-8357936). But there too it's non-existent for now. So I'll leave out the `Checks` class from this discussion. If this change were to be integrated into mainline, would there be any change to NullPointerException exception messages in any code paths? If not, then would it better to do these changes along with the `Objects.requireNonNull()` change itself, as part of https://bugs.openjdk.org/browse/JDK-8233268? In JDK-8233268, there's also a comment from Goetz proposing how the new exception message should look like. If the current changes were to be done, would it allow for that proposal to be implemented? I did have a brief look at the new jtreg test cases in this PR, but it wasn't immediately clear to me which APIs would start seeing this new proposed exception messages. - PR Comment: https://git.openjdk.org/jdk/pull/26600#issuecomment-3284904586
Re: RFR: 8366833: Poly1305 does not always correctly update position for array-backed ByteBuffers after processMultipleBlocks [v5]
On Thu, 11 Sep 2025 00:54:04 GMT, Jamil Nimeh wrote:
>> This fix resolves an issue where the `Cipher.updateAAD(ByteBuffer)` method,
>> when used on a ChaCha20-Poly1305 Cipher, may throw an exception due to an
>> offset calculation error. This occurs when the ByteBuffer is array-backed,
>> and when the buffer passed into the method is a slice of another
>> array-backed buffer and that slice begins at a non-zero offset in the parent
>> ByteBuffer.
>>
>> Credit and thanks to @jaikiran for finding the issue and providing
>> reproducer code.
>
> Jamil Nimeh has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add tests for encryption correctness, also add tests for MemorySegment
> ByteBuffers
test/jdk/com/sun/crypto/provider/Cipher/ChaCha20/UpdateAADTest.java line 74:
> 72:
> 73: public interface TestAction {
> 74: void runTest(ByteBuffer buffer);
If you allow `runTest` to throw an exception, then there is no need to catch
and rethrow in the 2 instances.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/27081#discussion_r2344388738
Re: RFR: 8361711: Add library name configurability to PKCS11Test.java [v3]
On Thu, 11 Sep 2025 22:03:47 GMT, Valerie Peng wrote: > For consistency sake, could you please update the description of > https://bugs.openjdk.org/browse/JDK-8361711 to match the current PR? It no > longer adds the new CUSTOM_P11_CONFIG_VARIANT property. Done. - PR Comment: https://git.openjdk.org/jdk/pull/26325#issuecomment-3285532662
Re: RFR: 8361711: Add library name configurability to PKCS11Test.java [v3]
On Thu, 11 Sep 2025 22:00:27 GMT, Valerie Peng wrote: >> @valeriepeng I simplified the approach; let me know what you think. I >> confirmed I can still configure Kryoptic as needed, though I have to name >> the `Kryoptic` configuration files `nss/p11-nss.txt` and >> `nss/p11-nss-sensitive.txt`, which is a little strange. However this has >> the advantage of eliminating the need to change any test case arguments. > > Yes, I like this approach, it minimizes the changes at the cost of setting > up, i.e. rename Kryoptic files to match NSS file names. Thanks for this very > simple approach. OK, thank you for re-reviewing and approving. - PR Review Comment: https://git.openjdk.org/jdk/pull/26325#discussion_r237880
Re: RFR: 8366833: Poly1305 does not always correctly update position for array-backed ByteBuffers after processMultipleBlocks [v6]
> This fix resolves an issue where the `Cipher.updateAAD(ByteBuffer)` method, > when used on a ChaCha20-Poly1305 Cipher, may throw an exception due to an > offset calculation error. This occurs when the ByteBuffer is array-backed, > and when the buffer passed into the method is a slice of another array-backed > buffer and that slice begins at a non-zero offset in the parent ByteBuffer. > > Credit and thanks to @jaikiran for finding the issue and providing reproducer > code. Jamil Nimeh has updated the pull request incrementally with one additional commit since the last revision: Simplify exception behavior in test callbacks - Changes: - all: https://git.openjdk.org/jdk/pull/27081/files - new: https://git.openjdk.org/jdk/pull/27081/files/10e7e1f5..f9f4f212 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=27081&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=27081&range=04-05 Stats: 38 lines in 1 file changed: 0 ins; 9 del; 29 mod Patch: https://git.openjdk.org/jdk/pull/27081.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/27081/head:pull/27081 PR: https://git.openjdk.org/jdk/pull/27081
Re: RFR: 8367104: Check for RSASSA-PSS parameters when validating certificates against algorithm constraints [v9]
On Fri, 12 Sep 2025 13:18:25 GMT, Sean Mullan wrote:
>> Artur Barashev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Update comments. Remove unnecessary variable assignments.
>
> src/java.base/share/classes/sun/security/ssl/X509TrustManagerImpl.java line
> 475:
>
>> 473: // Omit checks if EE cert is also a trust anchor
>> 474: if (chain.length > 1) {
>> 475: AlgorithmChecker checker = new AlgorithmChecker(
>
> Another option would be to add this `AlgorithmChecker` as another checker in
> the `PKIXBuilderParameters` when instantiating a `PKIXValidator`, and then
> the `Validator` would just call this additional checker when validating the
> chain. But this is a bit more complicated because the caller can pass in
> their own `PKIXBuilderParameters`. But noting here for reference that it is
> another option.
I put together an alternative solution that avoids duplicate calls:
https://github.com/openjdk/jdk/pull/27262/files#diff-c691895596058f5eb4ec609c75ad83ef4a16da85ce6f3499ca89ef412eab15bf
-
PR Review Comment: https://git.openjdk.org/jdk/pull/27146#discussion_r2345080596
Re: RFR: 8044609: javax.net.debug options not working and documented as expected [v20]
On Thu, 14 Nov 2024 02:21:53 GMT, Bradford Wetmore wrote: >> good point. I think it might have been wiped away in earlier iterations. >> I've added it back and added test coverage for this option. The separator >> logic for such options isn't specified which makes coding more tricky. I've >> tried to keep the syntax parsing flexible. > > Maybe add a bit about what `expand` does? > >> expand expanded (less compact) output format Thanks. This can be marked as resolved. - PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2345123795
Re: RFR: 8044609: javax.net.debug options not working and documented as expected [v19]
On Tue, 2 Sep 2025 11:00:08 GMT, Sean Coffey wrote: >> src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 128: >> >>> 126: // javax.net.debug would be misconfigured property with >>> respect >>> 127: // to logging if value didn't contain "all" or "ssl" >>> 128: logging = Opt.ALL.on || Opt.SSL.on; >> >> I don't have a strong feeling about this either way, but a usage question >> here. >> >> If they have specified some args but not `ssl`. e.g. >> >> -Djavax.net.debug=trustmanager,keymanager >> >> Should we: >> >> 1. quietly ignore as this is are doing >> 2. output a single log message saying logging is disabled. >> 3. a fatal error > > I'm leaning towards option 1. no logging gets enabled in misconfigured > values. I've test code covering this scenario. > > option 2 might be useful and if there's a strong desire for that, perhaps we > can alter in follow on JDK issue. > option 3 sounds a bit heavy handed, Don't think we need to have a fatal > error. Also, since the load time for loading SSLLogger is variable, it could > be dangerous (e.g. app runs fine for 1st 10 mins and then trigger an error > etc) I am fine with #1. We'll see if anyone requests. Just wanted to bring it up as an option. - PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2342660580
Re: RFR: 8044609: javax.net.debug options not working and documented as expected [v20]
On Wed, 20 Nov 2024 11:38:41 GMT, Sean Coffey wrote: >> Can we get that added back, or maybe added to a follow-on bug? That seemed >> useful. > > found the change where this debug option got dropped circa 2009: JDK-4918870 > > yes - we can log a new issue to track this request Thanks for adding to the `enum` and help message. Just need to add it to the actual call sites, which can be done as part of JDK-8344158. - PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2345115169
Re: RFR: 8044609: javax.net.debug options not working and documented as expected [v20]
On Tue, 2 Sep 2025 14:27:20 GMT, Sean Coffey wrote:
>> The `javax.net.debug` TLS debug option is buggy since TLSv1.3 implementation
>> was introduced many years ago.
>>
>> Where "ssl" was previously a value to obtain all TLS debug traces (except
>> network type dumps, verbose data), it now prints only a few lines for a
>> standard client TLS connection.
>>
>> The property parsing was also lax and allowed users to declare verbose
>> logging options by themselves where the documentation stated that such
>> verbose options were only meant to be used in conjunction with other TLS
>> options :
>>
>>
>> System.err.println("help print the help messages");
>> System.err.println("expand expand debugging information");
>> System.err.println();
>> System.err.println("allturn on all debugging");
>> System.err.println("sslturn on ssl debugging");
>> System.err.println();
>> System.err.println("The following can be used with ssl:");
>> System.err.println("\trecord enable per-record tracing");
>> System.err.println("\thandshakeprint each handshake message");
>> System.err.println("\tkeygen print key generation data");
>> System.err.println("\tsession print session activity");
>> System.err.println("\tdefaultctx print default SSL
>> initialization");
>> System.err.println("\tsslctx print SSLContext tracing");
>> System.err.println("\tsessioncache print session cache tracing");
>> System.err.println("\tkeymanager print key manager tracing");
>> System.err.println("\ttrustmanager print trust manager tracing");
>> System.err.println("\tpluggability print pluggability tracing");
>> System.err.println();
>> System.err.println("\thandshake debugging can be widened with:");
>> System.err.println("\tdata hex dump of each handshake
>> message");
>> System.err.println("\tverbose verbose handshake message
>> printing");
>> System.err.println();
>> System.err.println("\trecord debugging can be widened with:");
>> System.err.println("\tplaintexthex dump of record plaintext");
>> System.err.println("\tpacket print raw SSL/TLS packets");
>>
>>
>> as part of this patch, I've also moved the log call to the more performant
>> friendly
>> `System.Logger#log(java.lang.System.Logger.Level,java.util.function.Supplier)`
>> method.
>>
>> the output has changed slightly with respect to that - less verbose
>>
>> e.g. old...
>
> Sean Coffey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Further review comments, copyright years also
A couple more minor comments.
All copyrights look good. (Thanks.)
src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 217:
> 215: "print each handshake message");
> 216: System.err.printf(" %-12s %s%n", "verbose",
> 217: "-verbose handshake message printing (widens
> handshake)");
Minor nit. Having "-" at the beginning of the textual description on these
"wideners" looks odd to my eye. "-" are usually for in front of the command
args. e.g.
The following filters can be used with ssl:
handshake print each handshake message
verbose-verbose handshake message printing (widens handshake)
record enable per-record tracing
packet -print raw SSL/TLS packets (widens record)
plaintext -hex dump of record plaintext (widens record)
src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 236:
> 234: System.err.printf("%-14s %s%n", "trustmanager",
> 235: "print trust manager tracing");
> 236: System.err.println();
Do we want to include something like this at the bottom?:
> Adding filters to "ssl" will filter log messages to include just those
> categories. If "ssl" is specified by itself, all non-widening filters are
> enabled.
-
PR Review: https://git.openjdk.org/jdk/pull/18764#pullrequestreview-3214120399
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2342697466
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2342710103
Re: RFR: 8244336: Restrict algorithms at JCE layer [v10]
On Fri, 12 Sep 2025 14:26:01 GMT, Sean Mullan wrote: > > > One more comment, otherwise looks good. > > > Also, add a release note. > > > > > > Done, please find it at https://bugs.openjdk.org/browse/JDK-8367477 > > Thanks, I made a few minor wording edits. Great, thanks for the review! Edits look good and I have marked it as Delivered. - PR Comment: https://git.openjdk.org/jdk/pull/26377#issuecomment-3286503429
Re: RFR: 8367104: Check for RSASSA-PSS parameters when validating certificates against algorithm constraints [v9]
On Thu, 11 Sep 2025 22:06:10 GMT, Artur Barashev wrote:
>> RSASSA-PSS is currently the only signature algorithm we support that comes
>> with algorithm parameters. We don't check for those parameters when
>> validating certificates against supported signature algorithm constraints.
>
> Artur Barashev has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update comments. Remove unnecessary variable assignments.
Looks good.
src/java.base/share/classes/sun/security/ssl/X509TrustManagerImpl.java line 475:
> 473: // Omit checks if EE cert is also a trust anchor
> 474: if (chain.length > 1) {
> 475: AlgorithmChecker checker = new AlgorithmChecker(
Another option would be to add this `AlgorithmChecker` as another checker in
the `PKIXBuilderParameters` when instantiating a `PKIXValidator`, and then the
`Validator` would just call this additional checker when validating the chain.
But this is a bit more complicated because the caller can pass in their own
`PKIXBuilderParameters`. But noting here for reference that it is another
option.
-
Marked as reviewed by mullan (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/27146#pullrequestreview-3216635377
PR Review Comment: https://git.openjdk.org/jdk/pull/27146#discussion_r2344242058
Re: RFR: 8361711: Add library name configurability to PKCS11Test.java [v3]
On Wed, 3 Sep 2025 00:22:16 GMT, Thomas Fitzsimmons wrote: >> This patch adds configurability to `PKCS11Test.java`. >> >> Specifically, it adds two new system properties: >> >> - `CUSTOM_P11_LIBRARY_NAME`: Allow overriding the value assigned to the >> `nss_library` field. Prior to this patch, `nss_library` was set to either >> `"softokn3"` or `"nss3"`. >> >> - `CUSTOM_P11_CONFIG_VARIANT`: Abstract the configuration file type to load. >> Prior to this patch, test cases that wanted to run `NSS` in sensitive mode >> would hard-code `p11-nss-sensitive.txt` on their command lines. >> >> The patch updates the three `p11-nss-sensitive.txt`-using test cases to use >> the new `CUSTOM_P11_CONFIG_VARIANT` property: >> >> >> test/jdk/java/security/KeyAgreement/Generic.java >> test/jdk/sun/security/pkcs11/Mac/TestLargeSecretKeys.java >> test/jdk/sun/security/pkcs11/rsa/TestP11KeyFactoryGetRSAKeySpec.java >> >> >> I have been using this change to run `PKCS11Test.java` against the >> [Kryoptic](https://github.com/latchset/kryoptic) PKCS11 soft token, using >> this invocation: >> >> >> make test \ >> >> JTREG="JAVA_OPTIONS=-DCUSTOM_P11_CONFIG=/tmp/kryoptic-configuration/p11-kryoptic.txt >> \ >> -DCUSTOM_P11_LIBRARY_NAME=kryoptic_pkcs11 \ >> >> -Djdk.test.lib.artifacts.nsslib-linux_x64=/tmp/kryoptic-configuration \ >> -DCUSTOM_DB_DIR=/tmp/kryoptic-configuration" >> >> >> `/tmp/kryoptic-configuration` contains (among other files): >> >> >> libkryoptic_pkcs11.so >> p11-kryoptic.txt >> p11-kryoptic-sensitive.txt >> >> >> With `CUSTOM_P11_LIBRARY_NAME` set, `PKCS11Test.java` can find >> `libkryoptic_pkcs11.so`. >> >> And setting `CUSTOM_P11_CONFIG` causes the sensitive tests to use >> `p11-kryoptic-sensitive.txt` via the new `CUSTOM_P11_CONFIG_VARIANT` >> property. >> >> On my `Fedora 42` `x86-64` machine, I tested for regressions with: >> >> $ time make test JOBS=4 >> JTREG="JAVA_OPTIONS=-Djdk.test.lib.artifacts.nsslib-linux_x64=/usr/lib64" >> TEST="test/jdk/sun/security/pkcs11" >> >> and: >> >> $ time make test JOBS=4 TEST="test/jdk/sun/security/pkcs11" > > Thomas Fitzsimmons has updated the pull request incrementally with one > additional commit since the last revision: > > Remove CUSTOM_P11_CONFIG_VARIANT, add CUSTOM_P11_BASE_DIR @fitzsim Your change (at version 423962fe813c5c3ab656771aebcb54f493da1ff9) is now ready to be sponsored by a Committer. - PR Comment: https://git.openjdk.org/jdk/pull/26325#issuecomment-3285557586
Re: RFR: 8349910: Implement JEP 517: HTTP/3 for the HTTP Client API [v17]
On Wed, 10 Sep 2025 18:08:20 GMT, Daniel Fuchs wrote: >> Hi, >> >> Please find here a PR for the implementation of [JEP 517: HTTP/3 for the >> HTTP Client API](https://openjdk.org/jeps/517). >> >> The CSR can be viewed at [JDK-8350588: Implement JEP 517: HTTP/3 for the >> HTTP Client API](https://bugs.openjdk.org/browse/JDK-8350588) >> >> This JEP proposes to enhance the HttpClient implementation to support HTTP/3. >> It adds a non-exposed / non-exported internal implementation of the QUIC >> protocol based on DatagramChannel and the SunJSSE SSLContext provider. > > Daniel Fuchs has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 656 commits: > > - merge latest changes from master branch > - modify http3 test timeouts > - qpack - take knownReceivedCount into account during DT entries eviction > - merge latest changes from master branch > - http3: fix at since > - merge latest http3 changes > - Remove unused variables > - Simplify control flow > - Remove unnecessary instanceof checks > - Use IntFunction for header generator > - ... and 646 more: https://git.openjdk.org/jdk/compare/5cd7721a...9acc3590 Marked as reviewed by aefimov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/24751#pullrequestreview-3216731618
Re: RFR: 8360564: Implement JEP 524: PEM Encodings of Cryptographic Objects (Second Preview)
On Fri, 12 Sep 2025 18:29:55 GMT, Weijun Wang wrote:
>> Hi
>>
>> Please review the [Second Preview](https://openjdk.org/jeps/8360563) for the
>> PEM API. The most significant changes from [JEP
>> 470](https://openjdk.org/jeps/470) are:
>>
>> - Renamed the name of `PEMRecord` class to `PEM`.
>> - Revised the new `encryptKey` methods of the `EncryptedPrivateKeyInfo`
>> class to accept `DEREncodable` objects rather than just `PrivateKey` objects
>> so that cryptographic objects with public keys, i.e., `KeyPair` and
>> `PKCS8EncodedKeySpec`, can also be encrypted.
>> - Enhanced the `PEMEncoder` and `PEMDecoder` classes to support the
>> encryption and decryption of `KeyPair` and `PKCS8EncodedKeySpec` objects.
>>
>> thanks
>>
>> Tony
>
> src/java.base/share/classes/sun/security/ec/ECPrivateKeyImpl.java line 255:
>
>> 253: if (seq.data.available() != 0) {
>> 254: DerValue derValue = seq.data.getDerValue();
>> 255: if (derValue.isContextSpecific((byte) 1)) {
>
> If any of these `if`s is false `null` is returned. Would you rather throw an
> IAE?
I see there could be a
parameters [0] ECDomainParameters {{ SECGCurveNames }} OPTIONAL,
Shall we skip it?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2345120268
Integrated: 8244336: Restrict algorithms at JCE layer
On Fri, 18 Jul 2025 01:44:33 GMT, Valerie Peng wrote: > This enhancement introduces a new security property > "jdk.crypto.disabledAlgorithms" which can be leveraged to disable algorithms > for JCE/JCA crypto services. For now, only Cipher, KeyStore, MessageDigest, > and Signature services support this new security property. The support can be > expanded later to cover more services if needed. Note that this security > property is meant to disable algorithms irrespective of providers. If the > algorithm is found to be disabled, it will be rejected before reaching out to > provider(s) for the corresponding implementation(s). > > A few implementation notes: > 1) The specified security property value is lazily loaded and all changes > after it's been loaded are ignored. Invalid entries, e.g. wrong syntax, are > ignored and removed. The algorithm name check is case-insensitive. If a > disabled algorithm is known to has an object identifier (oid) by JDK, this > oid and its aliases is also added to the disabled services. > 2) The algorithm name checking impl is based on the > sun.security.util.AlgorithmConstraints class, but without the decomposing and > different constraints. > 3) The hardwiring of NONEwithRSA signature to RSA/ECB/PKCS1Padding cipher in > java.security.Signature class is removed. Instead, this is moved to the > provider level, i.e. SunJCE and SunPKCS11 provider are changed to claim the > NONEwithRSA signature support. Disabling one will not affect the other. > > CSR will be filed once the review is wrapping up. > > Thanks~ > Valerie This pull request has now been integrated. Changeset: 35dabb1a Author:Valerie Peng URL: https://git.openjdk.org/jdk/commit/35dabb1a5f31d985f00de21badeeedb026a63b94 Stats: 1566 lines in 19 files changed: 1364 ins; 163 del; 39 mod 8244336: Restrict algorithms at JCE layer Reviewed-by: mullan, ascarpino, abarashev - PR: https://git.openjdk.org/jdk/pull/26377
Re: RFR: 8360564: Implement JEP 524: PEM Encodings of Cryptographic Objects (Second Preview)
On Mon, 8 Sep 2025 15:42:13 GMT, Anthony Scarpino wrote:
> Hi
>
> Please review the [Second Preview](https://openjdk.org/jeps/8360563) for the
> PEM API. The most significant changes from [JEP
> 470](https://openjdk.org/jeps/470) are:
>
> - Renamed the name of `PEMRecord` class to `PEM`.
> - Revised the new `encryptKey` methods of the `EncryptedPrivateKeyInfo` class
> to accept `DEREncodable` objects rather than just `PrivateKey` objects so
> that cryptographic objects with public keys, i.e., `KeyPair` and
> `PKCS8EncodedKeySpec`, can also be encrypted.
> - Enhanced the `PEMEncoder` and `PEMDecoder` classes to support the
> encryption and decryption of `KeyPair` and `PKCS8EncodedKeySpec` objects.
>
> thanks
>
> Tony
Some comments.
src/java.base/share/classes/sun/security/ec/ECPrivateKeyImpl.java line 255:
> 253: if (seq.data.available() != 0) {
> 254: DerValue derValue = seq.data.getDerValue();
> 255: if (derValue.isContextSpecific((byte) 1)) {
If any of these `if`s is false `null` is returned. Would you rather throw an
IAE?
src/java.base/share/classes/sun/security/pkcs/PKCS8Key.java line 159:
> 157: privKeyMaterial = val.data.getOctetString();
> 158:
> 159: // Special check and parsing for ECDSA's SEC1v2 format
So there are 2 ways to extract public key from PKCS8. Are you going to check
whether they match?
Also, can we add a test case for this format?
src/java.base/share/classes/sun/security/util/Pem.java line 355:
> 353: return pemEncoded(pem.type(), p);
> 354: }
> 355:
Is it correct to put the 2 new methods here? Seems not closely related to PEM.
src/java.base/share/classes/sun/security/util/Pem.java line 387:
> 385: * return a PrivateKey
> 386: * @param provider KeyFactory provider
> 387: */
This method is very similar to `PKCS8Key::parseKey`. Maybe we should combine
them.
test/jdk/javax/crypto/EncryptedPrivateKeyInfo/GetKeyPair.java line 87:
> 85: kp.getPublic().getEncoded())) {
> 86: throw new AssertionError("PublicKey didn't match the
> original.");
> 87: }
Maybe put these checks into a method and call it three times? Also, you can use
`Asserts.assertEqualsByteArray`.
-
PR Review: https://git.openjdk.org/jdk/pull/27147#pullrequestreview-3218055847
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2345111099
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2345047110
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2345040355
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2345041626
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2345020741
Integrated: 8366833: Poly1305 does not always correctly update position for array-backed ByteBuffers after processMultipleBlocks
On Thu, 4 Sep 2025 02:23:50 GMT, Jamil Nimeh wrote: > This fix resolves an issue where the `Cipher.updateAAD(ByteBuffer)` method, > when used on a ChaCha20-Poly1305 Cipher, may throw an exception due to an > offset calculation error. This occurs when the ByteBuffer is array-backed, > and when the buffer passed into the method is a slice of another array-backed > buffer and that slice begins at a non-zero offset in the parent ByteBuffer. > > Credit and thanks to @jaikiran for finding the issue and providing reproducer > code. This pull request has now been integrated. Changeset: 3eb3e0dc Author:Jamil Nimeh URL: https://git.openjdk.org/jdk/commit/3eb3e0dcb0aa06fe427adeeaa40b9568c7f07ee8 Stats: 191 lines in 2 files changed: 189 ins; 0 del; 2 mod 8366833: Poly1305 does not always correctly update position for array-backed ByteBuffers after processMultipleBlocks Co-authored-by: Jaikiran Pai Reviewed-by: weijun, jpai - PR: https://git.openjdk.org/jdk/pull/27081
Re: RFR: 8325448: Hybrid Public Key Encryption [v43]
On Wed, 10 Sep 2025 23:03:49 GMT, Weijun Wang wrote:
>> Implement HPKE as defined in https://datatracker.ietf.org/doc/rfc9180/.
>> > src="https://github.com/user-attachments/assets/366a9f3e-375a-48fb-8ee6-95cabcfeccc8";
>> />
>
> Weijun Wang has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 58 commits:
>
> - Merge branch 'master' into 8325448
> - about transformation
> - cannot reset with withMethods
> - algorithm identifier
> - withMethods
> - duplicated "value" words
> - receiver to recipient; different to specified
> - use different exception type
> - more spec change
> - address Sean's comments
> - ... and 48 more: https://git.openjdk.org/jdk/compare/7fcce270...1ec31cf5
src/java.base/share/classes/javax/crypto/spec/HPKEParameterSpec.java line 154:
> 152: /**
> 153: * KEM algorithm identifier for DHKEM(P-256, HKDF-SHA256) as defined
> in
> 154: * href="https://www.rfc-editor.org/rfc/rfc9180.html#name-key-encapsulation-mechanism";>Section
> 7.1 of RFC 9180.
Suggestion: It has been my impression that `@spec` was for things like this.
Might it be cleaner to remove the "as defined in " and just list the RFC
in an `@spec`? It also seems overkill to refer to the RFC sections for each
entry as you mention that the id's are in in Section 7 in the class javadoc.
src/java.base/share/classes/javax/crypto/spec/HPKEParameterSpec.java line 286:
> 284: * @throws IllegalArgumentException if {@code info} is empty.
> 285: */
> 286: public HPKEParameterSpec withInfo(byte[] info) {
After reading your example at the class javadoc level, it left me with the
impression that `withInfo()` would be used with `String`. Does it make sense
to have a `withInfo(String)` method? Or maybe the example less String-specific?
src/java.base/share/classes/javax/crypto/spec/HPKEParameterSpec.java line 317:
> 315: throw new IllegalArgumentException("psk_id is empty");
> 316: }
> 317: if ("RAW".equalsIgnoreCase(psk.getFormat())) {
What happens if the format is not RAW? Is that allowed or should it be an IAE?
If `psk` is an 16 byte AES Secret key is that checked somewhere or at all
relevant?
src/java.base/share/classes/javax/crypto/spec/HPKEParameterSpec.java line 352:
> 350: * authentication key value.
> 351: *
> 352: * Note: this method does not check whether the KEM supports
"the KEM supports" sounds awkward to me. Do you mean non-DHKEM or the KEM
provider implementation?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18411#discussion_r2345198777
PR Review Comment: https://git.openjdk.org/jdk/pull/18411#discussion_r2345243742
PR Review Comment: https://git.openjdk.org/jdk/pull/18411#discussion_r2345408266
PR Review Comment: https://git.openjdk.org/jdk/pull/18411#discussion_r2345423554
Re: RFR: 8325448: Hybrid Public Key Encryption [v43]
On Fri, 12 Sep 2025 19:30:21 GMT, Anthony Scarpino
wrote:
>> Weijun Wang has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 58 commits:
>>
>> - Merge branch 'master' into 8325448
>> - about transformation
>> - cannot reset with withMethods
>> - algorithm identifier
>> - withMethods
>> - duplicated "value" words
>> - receiver to recipient; different to specified
>> - use different exception type
>> - more spec change
>> - address Sean's comments
>> - ... and 48 more: https://git.openjdk.org/jdk/compare/7fcce270...1ec31cf5
>
> src/java.base/share/classes/javax/crypto/spec/HPKEParameterSpec.java line 286:
>
>> 284: * @throws IllegalArgumentException if {@code info} is empty.
>> 285: */
>> 286: public HPKEParameterSpec withInfo(byte[] info) {
>
> After reading your example at the class javadoc level, it left me with the
> impression that `withInfo()` would be used with `String`. Does it make sense
> to have a `withInfo(String)` method? Or maybe the example less
> String-specific?
Oh, I just want to be self-contained in the example and `new byte[10]` seems
too trivial. Actually, it's very likely not a string. For example, in TLS
Encrypted Client Hello, it's `"tls ech" || 0x00 || ECHConfig`. I don't think
it's worth adding an overloaded `withInfo(String)`. It's similar to the `info`
in HKDF-Expand.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18411#discussion_r2345601851
Re: RFR: 8325448: Hybrid Public Key Encryption [v43]
On Fri, 12 Sep 2025 20:50:57 GMT, Anthony Scarpino
wrote:
>> Weijun Wang has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 58 commits:
>>
>> - Merge branch 'master' into 8325448
>> - about transformation
>> - cannot reset with withMethods
>> - algorithm identifier
>> - withMethods
>> - duplicated "value" words
>> - receiver to recipient; different to specified
>> - use different exception type
>> - more spec change
>> - address Sean's comments
>> - ... and 48 more: https://git.openjdk.org/jdk/compare/7fcce270...1ec31cf5
>
> src/java.base/share/classes/javax/crypto/spec/HPKEParameterSpec.java line 317:
>
>> 315: throw new IllegalArgumentException("psk_id is empty");
>> 316: }
>> 317: if ("RAW".equalsIgnoreCase(psk.getFormat())) {
>
> What happens if the format is not RAW? Is that allowed or should it be an
> IAE?
> If `psk` is an 16 byte AES Secret key is that checked somewhere or at all
> relevant?
I just meant if it's not "RAW" (maybe `null`?) then I have no way to check its
length. A 16 byte AES will be rejected if it has an encoding which is almost
always of "RAW" format.
Or, did you confuse `getAlgorithm` and `getFormat`?
> src/java.base/share/classes/javax/crypto/spec/HPKEParameterSpec.java line 352:
>
>> 350: * authentication key value.
>> 351: *
>> 352: * Note: this method does not check whether the KEM supports
>
> "the KEM supports" sounds awkward to me. Do you mean non-DHKEM or the KEM
> provider implementation?
Section 5 of RFC 9180:
> Note that some KEMs may not support AuthEncap() or AuthDecap(). For such
> KEMs, only mode_base or mode_psk are supported. Future specifications which
> define new KEMs MUST indicate whether these modes are supported. See [Section
> 7.1.5](https://www.rfc-editor.org/rfc/rfc9180.html#future-kems) for more
> details.
I can change to "the KEM algorithm supports".
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18411#discussion_r2345617709
PR Review Comment: https://git.openjdk.org/jdk/pull/18411#discussion_r2345620663
Re: RFR: 8244336: Restrict algorithms at JCE layer [v10]
On Fri, 12 Sep 2025 19:01:24 GMT, Valerie Peng wrote: >>> > One more comment, otherwise looks good. >>> > Also, add a release note. >>> >>> Done, please find it at https://bugs.openjdk.org/browse/JDK-8367477 >> >> Thanks, I made a few minor wording edits. > >> > > One more comment, otherwise looks good. >> > > Also, add a release note. >> > >> > >> > Done, please find it at https://bugs.openjdk.org/browse/JDK-8367477 >> >> Thanks, I made a few minor wording edits. > > Great, thanks for the review! Edits look good and I have marked it as > Delivered. @valeriepeng fyi, the PR has failing tests in the CI: sun/security/util/AlgorithmConstraints/InvalidCryptoDisabledAlgos.java - PR Comment: https://git.openjdk.org/jdk/pull/26377#issuecomment-3287004858
Re: RFR: 8244336: Restrict algorithms at JCE layer [v10]
On Fri, 12 Sep 2025 19:01:24 GMT, Valerie Peng wrote: >>> > One more comment, otherwise looks good. >>> > Also, add a release note. >>> >>> Done, please find it at https://bugs.openjdk.org/browse/JDK-8367477 >> >> Thanks, I made a few minor wording edits. > >> > > One more comment, otherwise looks good. >> > > Also, add a release note. >> > >> > >> > Done, please find it at https://bugs.openjdk.org/browse/JDK-8367477 >> >> Thanks, I made a few minor wording edits. > > Great, thanks for the review! Edits look good and I have marked it as > Delivered. > @valeriepeng fyi, the PR has failing tests in the CI: > sun/security/util/AlgorithmConstraints/InvalidCryptoDisabledAlgos.java Strange, I did run a mach5 job and didn't see this failure... Wonder what happened... - PR Comment: https://git.openjdk.org/jdk/pull/26377#issuecomment-3287027680
