RFR: 8286423: Destroy password protection in the example code in KeyStore
Hi, May I have this simple example update in the KeyStore specification? Password protection should be destroyed in the example code in KeyStore specification. Otherwise, applications may just copy and past the code, and forget to clean up password protection. It's a trivial update, and may not worthy of a CSR. But please let me know if you would like to have a CSR filed. Thanks, Xuelei - Commit messages: - 8286423: Destroy password protection in the example code in KeyStore Changes: https://git.openjdk.java.net/jdk/pull/8623/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8623=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286423 Stats: 18 lines in 1 file changed: 3 ins; 0 del; 15 mod Patch: https://git.openjdk.java.net/jdk/pull/8623.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8623/head:pull/8623 PR: https://git.openjdk.java.net/jdk/pull/8623
Re: RFR: 8002277: Refactor two PBE classes to simplify maintenance [v2]
On Tue, 10 May 2022 01:22:21 GMT, Valerie Peng wrote:
>> src/java.base/share/classes/com/sun/crypto/provider/PKCS12PBECipherCore.java
>> line 314:
>>
>>> 312: } else if (cipher instanceof DESedeCipher
>>> tripleDes) {
>>> 313: tripleDes.engineInit(opmode, cipherKey,
>>> ivSpec, random);
>>> 314: } else {
>>
>> Can we combine the 2 if above? What else type can it be?
>
> Currently, the specified CipherSpi object is one of RC4, RC2, DESede. The
> "else" part is for catching new PKCS12 PBE algorithms support which uses
> other cipher algorithms.
> CipherSpi.engineInit(...) is protected, so that's why we use the instanceof
> to cast the CipherSpi object into the actual impl class in order to call the
> engineInit(...) since the actual impl class is in the same package of this
> class.
The `core.init(..., cipher)` is actually
`cipher.init(core.translateKeyAndParams())`. Is it possible we write it this
way?
-
PR: https://git.openjdk.java.net/jdk/pull/8521
Re: RFR: 8002277: Refactor two PBE classes to simplify maintenance [v2]
On Mon, 9 May 2022 14:09:28 GMT, Weijun Wang wrote: > It's a pity you have to implement all those `engineXyz` methods in all three > `CipherSpi` implementations here. Is there something simpler? I debated shifting all these "engineXyz" methods into the PKCS12PBECipherCore class and store the CipherSpi object inside PKCS12PBECipher class, but then whenever we need to call "Cipher.engineXyz" methods, we'd need to cast the CipherSpi object to the actual impl class. There are also additional logic for handling mode, padding in PKCS12PBECipherCore class if we go this route. Comparing to the current approach, there are different pros and cons. I prefer less casting. - PR: https://git.openjdk.java.net/jdk/pull/8521
Re: RFR: 8002277: Refactor two PBE classes to simplify maintenance [v2]
On Fri, 6 May 2022 22:24:57 GMT, Weijun Wang wrote:
>> Valerie Peng has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> update copyright year for PBES2Core.java
>
> src/java.base/share/classes/com/sun/crypto/provider/PKCS12PBECipherCore.java
> line 314:
>
>> 312: } else if (cipher instanceof DESedeCipher
>> tripleDes) {
>> 313: tripleDes.engineInit(opmode, cipherKey, ivSpec,
>> random);
>> 314: } else {
>
> Can we combine the 2 if above? What else type can it be?
Currently, the specified CipherSpi object is one of RC4, RC2, DESede. The
"else" part is for catching new PKCS12 PBE algorithms support which uses other
cipher algorithms.
CipherSpi.engineInit(...) is protected, so that's why we use the instanceof to
cast the CipherSpi object into the actual impl class in order to call the
engineInit(...) since the actual impl class is in the same package of this
class.
-
PR: https://git.openjdk.java.net/jdk/pull/8521
Re: RFR: 8209038: Clarify the javadoc of Cipher.getParameters() [v5]
On Mon, 9 May 2022 18:28:04 GMT, Valerie Peng wrote:
>> Anyone can help review this javadoc update? The main change is the wording
>> for the method javadoc of
>> Cipher.getParameters()/CipherSpi.engineGetParameters(). The original wording
>> is somewhat restrictive and request is to broaden this to accommodate more
>> scenarios such as when null can be returned.
>> The rest are minor things like add {@code } to class name and null, and
>> remove redundant ".".
>>
>> Will file CSR after the review is close to being wrapped up.
>> Thanks~
>
> Valerie Peng 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:
>
> - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8209038
> - update to address review feedback and minor javadoc cleanup
> - Update for getParameters()
> - Update w/ review feedbacks
> - 8209038: Clarify the javadoc of Cipher.getParameters()
Marked as reviewed by weijun (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/8117
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v4]
On Mon, 9 May 2022 18:45:05 GMT, Valerie Peng wrote: >> This is to update the method javadoc of >> java.security.Signature.getParameters() with the missing `@throws >> UnsupportedOperationException`. In addition, the wording on the returned >> parameters are updated to match those in Cipher and CipherSpi classes. >> >> CSR will be filed later. >> >> Thanks, >> Valerie > > Valerie Peng 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 four additional > commits since the last revision: > > - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8253176 > - Sync'ed w/ the wording in the other Cipher.getParameters() PR. > - Undo un-intentional changes. > - 8253176: Signature.getParameters should specify that it can throw > UnsupportedOperationException Marked as reviewed by weijun (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8002277: Refactor two PBE classes to simplify maintenance [v2]
On Mon, 9 May 2022 23:23:05 GMT, Valerie Peng wrote:
>> src/java.base/share/classes/com/sun/crypto/provider/PBES2Core.java line 229:
>>
>>> 227: if (key instanceof javax.crypto.interfaces.PBEKey
>>> pbeKey) {
>>> 228: salt = check(pbeKey.getSalt()); // may be null
>>> 229: iCount = check(pbeKey.getIterationCount()); // may
>>> be 0
>>
>> It seems the return value is never 0.
>
> Oh, the comment about "may be 0" is meant toward the
> pbeKey.getInterationCount() call... Hmm, I will make it clearer.
I see. Another question, shall we reset `salt` and `iCount` at the beginning?
If `params` is null and `key` is not `PBEKey` and there is an existing positive
`iCount`, it will not be set to `DEFAULT_COUNT`.
-
PR: https://git.openjdk.java.net/jdk/pull/8521
Re: RFR: 8002277: Refactor two PBE classes to simplify maintenance [v2]
On Fri, 6 May 2022 22:22:47 GMT, Weijun Wang wrote:
>> Valerie Peng has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> update copyright year for PBES2Core.java
>
> src/java.base/share/classes/com/sun/crypto/provider/PKCS12PBECipherCore.java
> line 215:
>
>> 213: void implInit(int opmode, Key key, AlgorithmParameterSpec params,
>> 214: SecureRandom random, CipherSpi cipher)
>> 215: throws InvalidKeyException, InvalidAlgorithmParameterException {
>
> Indent the line above.
Ok, I will change the indentation of other throws to be consistent.
-
PR: https://git.openjdk.java.net/jdk/pull/8521
Re: RFR: 8002277: Refactor two PBE classes to simplify maintenance [v2]
On Fri, 6 May 2022 22:26:31 GMT, Weijun Wang wrote: >> Valerie Peng has updated the pull request incrementally with one additional >> commit since the last revision: >> >> update copyright year for PBES2Core.java > > src/java.base/share/classes/com/sun/crypto/provider/PKCS12PBECipherCore.java > line 214: > >> 212: >> 213: void implInit(int opmode, Key key, AlgorithmParameterSpec params, >> 214: SecureRandom random, CipherSpi cipher) > > Why rename `cipherImpl` to `cipher`? I think `cipher` is usually a `Cipher` > object and `cipherImpl` is a good name for a `CipherSpi` object. cipher vs cipherImpl looks same to me. I can revert it back since you have a preference. - PR: https://git.openjdk.java.net/jdk/pull/8521
Re: RFR: 8002277: Refactor two PBE classes to simplify maintenance [v2]
On Fri, 6 May 2022 22:20:32 GMT, Weijun Wang wrote: >> Valerie Peng has updated the pull request incrementally with one additional >> commit since the last revision: >> >> update copyright year for PBES2Core.java > > src/java.base/share/classes/com/sun/crypto/provider/PKCS12PBECipherCore.java > line 183: > >> 181:"for PBEWithSHA1And" + algo); >> 182: } >> 183: } > > How about using switch/case or at least put the `if` in same indentation > level? Yes, will use switch. - PR: https://git.openjdk.java.net/jdk/pull/8521
Re: RFR: 8002277: Refactor two PBE classes to simplify maintenance [v3]
> This change refactors the PBES2Core and PKCS12PBECipherCore classes in SunJCE > provider as requested in the bug record. Functionality should remain the same > with a clearer and simplified code/control flow with less lines of code. > This should improve readability and maintenance. I enhanced one existing > regression test to test more scenarios. This test would pass before the > proposed change and continues to pass with the proposed changes. Valerie Peng 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 four additional commits since the last revision: - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8002277 - update copyright year for PBES2Core.java - Enhanced test with more decryption w/o parameters scenario. - 8002277: Refactor two PBE classes to simplify maintenance - Changes: - all: https://git.openjdk.java.net/jdk/pull/8521/files - new: https://git.openjdk.java.net/jdk/pull/8521/files/1a2b3f90..307d2765 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8521=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8521=01-02 Stats: 126825 lines in 1830 files changed: 108724 ins; 8599 del; 9502 mod Patch: https://git.openjdk.java.net/jdk/pull/8521.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8521/head:pull/8521 PR: https://git.openjdk.java.net/jdk/pull/8521
Re: RFR: 8002277: Refactor two PBE classes to simplify maintenance [v2]
On Mon, 9 May 2022 14:00:58 GMT, Weijun Wang wrote:
>> Valerie Peng has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> update copyright year for PBES2Core.java
>
> src/java.base/share/classes/com/sun/crypto/provider/PBES2Core.java line 229:
>
>> 227: if (key instanceof javax.crypto.interfaces.PBEKey
>> pbeKey) {
>> 228: salt = check(pbeKey.getSalt()); // may be null
>> 229: iCount = check(pbeKey.getIterationCount()); // may
>> be 0
>
> It seems the return value is never 0.
Oh, the comment about "may be 0" is meant toward the
pbeKey.getInterationCount() call... Hmm, I will make it clearer.
-
PR: https://git.openjdk.java.net/jdk/pull/8521
RFR: 8286428: AlgorithmId should understand PBES2
`AlgorithmId.getName` is updated for PBES2 algorithm identifiers so it directly returns the standard algorithm defined by Java (Ex: `PBEWithHmacSHA256AndAES_256`), instead of a simple "PBES2". Please note I specifically update the javadoc for this method to clarify that this name is meant to be a name that's recognized by various `getInstance()` methods. This is how we are actually using this method. After this change, the `javax.crypto.EncryptedPrivateKeyInfo` API automatically works with PBES2 encrypted data. As the spec of its `getAlgName()` methods says, "Standard name is returned". This is shown by the newly include regression test. Existing security-related tests run fine. - Commit messages: - typo - the fix Changes: https://git.openjdk.java.net/jdk/pull/8615/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8615=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286428 Stats: 149 lines in 3 files changed: 118 ins; 16 del; 15 mod Patch: https://git.openjdk.java.net/jdk/pull/8615.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8615/head:pull/8615 PR: https://git.openjdk.java.net/jdk/pull/8615
Integrated: JDK-8286348: incorrect use of `@serial`
On Sat, 7 May 2022 01:04:03 GMT, Jonathan Gibbons wrote: > Please review a fix to remove incorrect use of the `@serial` tag from the doc > comments for methods such as `readObject` and `readResolve`. The tag has no > effect in this position other than to trigger warnings from the standard > doclet when running javadoc. > > There is no change to the generated documentation as a result off this > change. In particular, there is no change to the API docs for any of the > modified files, or to the overall top-level serialized-form.html file. > > Although most of the affected files are in the `java.desktop` module, there > is one outlier, in `java.security.Provider`. This pull request has now been integrated. Changeset: 54e33082 Author:Jonathan Gibbons URL: https://git.openjdk.java.net/jdk/commit/54e33082105dcbcfc795839c954f6e63402edff1 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod 8286348: incorrect use of `@serial` Reviewed-by: iris, prr - PR: https://git.openjdk.java.net/jdk/pull/8586
Re: RFR: JDK-8286348: incorrect use of `@serial` [v3]
On Mon, 9 May 2022 20:19:45 GMT, Jonathan Gibbons wrote: >> Please review a fix to remove incorrect use of the `@serial` tag from the >> doc comments for methods such as `readObject` and `readResolve`. The tag has >> no effect in this position other than to trigger warnings from the standard >> doclet when running javadoc. >> >> There is no change to the generated documentation as a result off this >> change. In particular, there is no change to the API docs for any of the >> modified files, or to the overall top-level serialized-form.html file. >> >> Although most of the affected files are in the `java.desktop` module, there >> is one outlier, in `java.security.Provider`. > > Jonathan Gibbons has updated the pull request incrementally with one > additional commit since the last revision: > > Fix whitespace (blank lines) after merge Marked as reviewed by prr (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8586
Re: RFR: 8286433: Cache certificates decoded from TLS session tickets
On Mon, 9 May 2022 19:38:36 GMT, Daniel Jeliński wrote: > When a TLS server resumes a session from a stateless session ticket, it > populates the `SSLSessionImpl`'s `localCerts` and `peerCerts` fields with > certificates deserialized from the session ticket. These certificates are > often the same across a large number of tickets. > > This patch implements a certificate cache lookup for these certificates. This > enables us to avoid deserializing the same certificates repeatedly, and saves > memory by reusing the same certificate objects. Performance results: Before: Benchmark (resume) (tlsVersion) Mode Cnt Score Error Units SSLHandshake.doHandshake true TLSv1.2 thrpt5 10425.534 ± 785.613 ops/s SSLHandshake.doHandshake true TLS thrpt5673.131 ± 24.857 ops/s after: Benchmark (resume) (tlsVersion) Mode Cnt Score Error Units SSLHandshake.doHandshake true TLSv1.2 thrpt5 11882.724 ± 106.444 ops/s SSLHandshake.doHandshake true TLS thrpt5717.195 ± 210.658 ops/s The benchmark shows a nice improvement in throughput on session resumption; it uses the same `localCerts` on all sessions, and `peerCerts` are empty. The performance of full handshakes (not shown) didn't change, which is expected because full handshakes do not use the changed code. GC profiling results: Before: Benchmark (resume) (tlsVersion) Mode Cnt Score Error Units SSLHandshake.doHandshake:·gc.alloc.rate.norm true TLSv1.2 thrpt 15 173868.322 ± 1554.251B/op SSLHandshake.doHandshake:·gc.alloc.rate.norm true TLS thrpt 15 404166.493 ± 1640.523B/op After: Benchmark (resume) (tlsVersion) Mode Cnt Score Error Units SSLHandshake.doHandshake:·gc.alloc.rate.norm true TLSv1.2 thrpt 15 140972.286 ± 1782.103B/op SSLHandshake.doHandshake:·gc.alloc.rate.norm true TLS thrpt 15 370317.660 ± 1846.107B/op Memory allocation is reduced by ~30kB per handshake on session resumption. The allocation profile of full handshakes (not shown) didn't change. - PR: https://git.openjdk.java.net/jdk/pull/8608
RFR: 8286433: Cache certificates decoded from TLS session tickets
When a TLS server resumes a session from a stateless session ticket, it populates the `SSLSessionImpl`'s `localCerts` and `peerCerts` fields with certificates deserialized from the session ticket. These certificates are often the same across a large number of tickets. This patch implements a certificate cache lookup for these certificates. This enables us to avoid deserializing the same certificates repeatedly, and saves memory by reusing the same certificate objects. - Commit messages: - Cache received certificates Changes: https://git.openjdk.java.net/jdk/pull/8608/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8608=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286433 Stats: 26 lines in 2 files changed: 13 ins; 8 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/8608.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8608/head:pull/8608 PR: https://git.openjdk.java.net/jdk/pull/8608
Re: RFR: JDK-8286348: incorrect use of `@serial` [v3]
> Please review a fix to remove incorrect use of the `@serial` tag from the doc > comments for methods such as `readObject` and `readResolve`. The tag has no > effect in this position other than to trigger warnings from the standard > doclet when running javadoc. > > There is no change to the generated documentation as a result off this > change. In particular, there is no change to the API docs for any of the > modified files, or to the overall top-level serialized-form.html file. > > Although most of the affected files are in the `java.desktop` module, there > is one outlier, in `java.security.Provider`. Jonathan Gibbons has updated the pull request incrementally with one additional commit since the last revision: Fix whitespace (blank lines) after merge - Changes: - all: https://git.openjdk.java.net/jdk/pull/8586/files - new: https://git.openjdk.java.net/jdk/pull/8586/files/d1920183..8684b44b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8586=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8586=01-02 Stats: 10 lines in 10 files changed: 10 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8586.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8586/head:pull/8586 PR: https://git.openjdk.java.net/jdk/pull/8586
Re: RFR: JDK-8286348: incorrect use of `@serial` [v2]
On Mon, 9 May 2022 18:14:35 GMT, Jonathan Gibbons wrote: >> Please review a fix to remove incorrect use of the `@serial` tag from the >> doc comments for methods such as `readObject` and `readResolve`. The tag has >> no effect in this position other than to trigger warnings from the standard >> doclet when running javadoc. >> >> There is no change to the generated documentation as a result off this >> change. In particular, there is no change to the API docs for any of the >> modified files, or to the overall top-level serialized-form.html file. >> >> Although most of the affected files are in the `java.desktop` module, there >> is one outlier, in `java.security.Provider`. > > Jonathan Gibbons has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains two commits: > > - Merge with upstream/master > - JDK-8286348: incorrect use of `@serial` Marked as reviewed by prr (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8586
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v4]
> This is to update the method javadoc of > java.security.Signature.getParameters() with the missing `@throws > UnsupportedOperationException`. In addition, the wording on the returned > parameters are updated to match those in Cipher and CipherSpi classes. > > CSR will be filed later. > > Thanks, > Valerie Valerie Peng 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 four additional commits since the last revision: - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8253176 - Sync'ed w/ the wording in the other Cipher.getParameters() PR. - Undo un-intentional changes. - 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException - Changes: - all: https://git.openjdk.java.net/jdk/pull/8396/files - new: https://git.openjdk.java.net/jdk/pull/8396/files/41e76e6e..660adeeb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8396=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8396=02-03 Stats: 135411 lines in 2025 files changed: 115484 ins; 9117 del; 10810 mod Patch: https://git.openjdk.java.net/jdk/pull/8396.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8396/head:pull/8396 PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8209038: Clarify the javadoc of Cipher.getParameters() [v5]
> Anyone can help review this javadoc update? The main change is the wording
> for the method javadoc of
> Cipher.getParameters()/CipherSpi.engineGetParameters(). The original wording
> is somewhat restrictive and request is to broaden this to accommodate more
> scenarios such as when null can be returned.
> The rest are minor things like add {@code } to class name and null, and
> remove redundant ".".
>
> Will file CSR after the review is close to being wrapped up.
> Thanks~
Valerie Peng 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:
- Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8209038
- update to address review feedback and minor javadoc cleanup
- Update for getParameters()
- Update w/ review feedbacks
- 8209038: Clarify the javadoc of Cipher.getParameters()
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/8117/files
- new: https://git.openjdk.java.net/jdk/pull/8117/files/77e8fa0d..f9727edf
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=8117=04
- incr: https://webrevs.openjdk.java.net/?repo=jdk=8117=03-04
Stats: 313143 lines in 4579 files changed: 242497 ins; 21053 del; 49593 mod
Patch: https://git.openjdk.java.net/jdk/pull/8117.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/8117/head:pull/8117
PR: https://git.openjdk.java.net/jdk/pull/8117
Re: RFR: JDK-8286348: incorrect use of `@serial` [v2]
> Please review a fix to remove incorrect use of the `@serial` tag from the doc > comments for methods such as `readObject` and `readResolve`. The tag has no > effect in this position other than to trigger warnings from the standard > doclet when running javadoc. > > There is no change to the generated documentation as a result off this > change. In particular, there is no change to the API docs for any of the > modified files, or to the overall top-level serialized-form.html file. > > Although most of the affected files are in the `java.desktop` module, there > is one outlier, in `java.security.Provider`. Jonathan Gibbons has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits: - Merge with upstream/master - JDK-8286348: incorrect use of `@serial` - Changes: https://git.openjdk.java.net/jdk/pull/8586/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8586=01 Stats: 11 lines in 11 files changed: 0 ins; 11 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8586.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8586/head:pull/8586 PR: https://git.openjdk.java.net/jdk/pull/8586
Integrated: 8285743: Ensure each IntegerPolynomial object is only created once
On Fri, 29 Apr 2022 22:30:04 GMT, Weijun Wang wrote: > All `IntegerPolynimial`s are singletons now. Also, hand-coded implementations > for Ed25519 and Ed448 are removed. They were not used since `FieldGen` starts > generating classes for them. > > No new regression test. This is a clean-up. This pull request has now been integrated. Changeset: 397d095f Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/397d095f661e9d9c98b8254fb7867dc87047b0b8 Stats: 507 lines in 10 files changed: 16 ins; 465 del; 26 mod 8285743: Ensure each IntegerPolynomial object is only created once Reviewed-by: xuelei, ascarpino - PR: https://git.openjdk.java.net/jdk/pull/8476
Re: RFR: 8285743: Ensure each IntegerPolynomial object is only created once [v2]
On Fri, 29 Apr 2022 22:57:20 GMT, Weijun Wang wrote: >> All `IntegerPolynimial`s are singletons now. Also, hand-coded >> implementations for Ed25519 and Ed448 are removed. They were not used since >> `FieldGen` starts generating classes for them. >> >> No new regression test. This is a clean-up. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > move singleton back into impl and make constructor private I'm good with this change - Marked as reviewed by ascarpino (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8476
Re: RFR: 8285743: Ensure each IntegerPolynomial object is only created once [v2]
On Fri, 29 Apr 2022 22:57:20 GMT, Weijun Wang wrote: >> All `IntegerPolynimial`s are singletons now. Also, hand-coded >> implementations for Ed25519 and Ed448 are removed. They were not used since >> `FieldGen` starts generating classes for them. >> >> No new regression test. This is a clean-up. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > move singleton back into impl and make constructor private It looks good and safe cleanup to me. - Marked as reviewed by xuelei (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8476
Integrated: 8285516: clearPassword should be called in a finally try block
On Sun, 24 Apr 2022 05:13:36 GMT, Xue-Lei Andrew Fan wrote: > Hi, > > Could I have the simple update reviewed? > > In the PKCS12 key store implementation, the PBEKeySpec.clearPassword() should > be called in a finally try block. Otherwise, the password cleanup could be > interrupted by exceptions. > > Thanks, > Xuelei This pull request has now been integrated. Changeset: 36e4df9d Author:Xue-Lei Andrew Fan URL: https://git.openjdk.java.net/jdk/commit/36e4df9d66134ef160bbba0e59d0e3dbb183ba4b Stats: 6 lines in 1 file changed: 2 ins; 2 del; 2 mod 8285516: clearPassword should be called in a finally try block Reviewed-by: mullan, hchao - PR: https://git.openjdk.java.net/jdk/pull/8377
Integrated: 8212136: Remove finalizer implementation in SSLSocketImpl
On Thu, 31 Mar 2022 20:15:35 GMT, Xue-Lei Andrew Fan wrote: > Please review the update to remove finalizer method in the SunJSSE provider > implementation. It is one of the efforts to clean up the use of finalizer > method in JDK. This pull request has now been integrated. Changeset: 034f20fe Author:Xue-Lei Andrew Fan URL: https://git.openjdk.java.net/jdk/commit/034f20fe86babb63bf178251a732ac004297cc2d Stats: 39 lines in 1 file changed: 7 ins; 31 del; 1 mod 8212136: Remove finalizer implementation in SSLSocketImpl Reviewed-by: wetmore - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8002277: Refactor two PBE classes to simplify maintenance [v2]
On Thu, 5 May 2022 19:38:06 GMT, Valerie Peng wrote:
>> This change refactors the PBES2Core and PKCS12PBECipherCore classes in
>> SunJCE provider as requested in the bug record. Functionality should remain
>> the same with a clearer and simplified code/control flow with less lines of
>> code. This should improve readability and maintenance. I enhanced one
>> existing regression test to test more scenarios. This test would pass before
>> the proposed change and continues to pass with the proposed changes.
>
> Valerie Peng has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update copyright year for PBES2Core.java
Is it possible to rewrite `PKCS12PBECipherCore.java` so that the implementation
inside is `CipherCore` instead of `CipherSpi`? I'd rather create a `CipherSpi`
child inside this package as the base for all implementations that does nothing
more but simply is able to expose all those `engineXYZ` methods to other
classes in the same package. Sigh.
src/java.base/share/classes/com/sun/crypto/provider/PBES2Core.java line 229:
> 227: if (key instanceof javax.crypto.interfaces.PBEKey
> pbeKey) {
> 228: salt = check(pbeKey.getSalt()); // may be null
> 229: iCount = check(pbeKey.getIterationCount()); // may
> be 0
It seems the return value is never 0.
src/java.base/share/classes/com/sun/crypto/provider/PKCS12PBECipherCore.java
line 183:
> 181:"for PBEWithSHA1And" + algo);
> 182: }
> 183: }
How about using switch/case or at least put the `if` in same indentation level?
src/java.base/share/classes/com/sun/crypto/provider/PKCS12PBECipherCore.java
line 214:
> 212:
> 213: void implInit(int opmode, Key key, AlgorithmParameterSpec params,
> 214: SecureRandom random, CipherSpi cipher)
Why rename `cipherImpl` to `cipher`? I think `cipher` is usually a `Cipher`
object and `cipherImpl` is a good name for a `CipherSpi` object.
src/java.base/share/classes/com/sun/crypto/provider/PKCS12PBECipherCore.java
line 215:
> 213: void implInit(int opmode, Key key, AlgorithmParameterSpec params,
> 214: SecureRandom random, CipherSpi cipher)
> 215: throws InvalidKeyException, InvalidAlgorithmParameterException {
Indent the line above.
src/java.base/share/classes/com/sun/crypto/provider/PKCS12PBECipherCore.java
line 314:
> 312: } else if (cipher instanceof DESedeCipher tripleDes)
> {
> 313: tripleDes.engineInit(opmode, cipherKey, ivSpec,
> random);
> 314: } else {
Can we combine the 2 if above? What else type can it be?
-
PR: https://git.openjdk.java.net/jdk/pull/8521
Re: RFR: 8285743: Ensure each IntegerPolynomial object is only created once [v2]
On Fri, 29 Apr 2022 22:57:20 GMT, Weijun Wang wrote: >> All `IntegerPolynimial`s are singletons now. Also, hand-coded >> implementations for Ed25519 and Ed448 are removed. They were not used since >> `FieldGen` starts generating classes for them. >> >> No new regression test. This is a clean-up. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > move singleton back into impl and make constructor private make/jdk/src/classes/build/tools/intpoly/FieldGen.java line 837: > 835: // c[i + j] += 2 * a[i] * a[j] > 836: // } > 837: // } The comment was in the hand-coded curve25519 code. Moved here. - PR: https://git.openjdk.java.net/jdk/pull/8476
Re: RFR: JDK-8286348: incorrect use of `@serial`
On Sat, 7 May 2022 01:04:03 GMT, Jonathan Gibbons wrote: > Please review a fix to remove incorrect use of the `@serial` tag from the doc > comments for methods such as `readObject` and `readResolve`. The tag has no > effect in this position other than to trigger warnings from the standard > doclet when running javadoc. > > There is no change to the generated documentation as a result off this > change. In particular, there is no change to the API docs for any of the > modified files, or to the overall top-level serialized-form.html file. > > Although most of the affected files are in the `java.desktop` module, there > is one outlier, in `java.security.Provider`. Marked as reviewed by limck...@github.com (no known OpenJDK username). - PR: https://git.openjdk.java.net/jdk/pull/8586
