Integrated: 8286211: Update PCSC-Lite for Suse Linux to 1.9.5
On Mon, 23 May 2022 19:31:02 GMT, Valerie Peng wrote: > Need to update the 3 header files due to expiring business approval for 3rd > party. > > The header files contain tabs which jcheck disallows, so I have to replace > them with spaces. > > Thanks, > Valerie This pull request has now been integrated. Changeset: d792cbcc Author:Valerie Peng URL: https://git.openjdk.java.net/jdk/commit/d792cbcc063c426fbf8add8697cbafc9b0e3fc3b Stats: 197 lines in 3 files changed: 2 ins; 15 del; 180 mod 8286211: Update PCSC-Lite for Suse Linux to 1.9.5 Reviewed-by: weijun - PR: https://git.openjdk.java.net/jdk/pull/8853
Re: RFR: 8286211: Update PCSC-Lite for Suse Linux to 1.9.5 [v3]
> Need to update the 3 header files due to expiring business approval for 3rd > party. > > The header files contain tabs which jcheck disallows, so I have to replace > them with spaces. > > Thanks, > Valerie Valerie Peng has updated the pull request incrementally with one additional commit since the last revision: Added a link for COPYING as on the pcsc-lite website - Changes: - all: https://git.openjdk.java.net/jdk/pull/8853/files - new: https://git.openjdk.java.net/jdk/pull/8853/files/42c9df2a..32b75b6a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8853=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8853=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8853.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8853/head:pull/8853 PR: https://git.openjdk.java.net/jdk/pull/8853
Re: RFR: 8286211: Update PCSC-Lite for Suse Linux to 1.9.5 [v2]
> Need to update the 3 header files due to expiring business approval for 3rd > party. > > The header files contain tabs which jcheck disallows, so I have to replace > them with spaces. > > Thanks, > Valerie Valerie Peng has updated the pull request incrementally with one additional commit since the last revision: Fix indentation - Changes: - all: https://git.openjdk.java.net/jdk/pull/8853/files - new: https://git.openjdk.java.net/jdk/pull/8853/files/8344ddb5..42c9df2a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8853=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8853=00-01 Stats: 192 lines in 3 files changed: 0 ins; 1 del; 191 mod Patch: https://git.openjdk.java.net/jdk/pull/8853.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8853/head:pull/8853 PR: https://git.openjdk.java.net/jdk/pull/8853
RFR: 8286211: Update PCSC-Lite for Suse Linux to 1.9.5
Need to update the 3 header files due to expiring business approval for 3rd party. The header files contain tabs which jcheck disallows, so I have to replace them with spaces. Thanks, Valerie - Commit messages: - Replaced tab with spaces in order to pass jcheck. - 8286211: Update PCSC-Lite for Suse Linux to 1.9.5 Changes: https://git.openjdk.java.net/jdk/pull/8853/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8853=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286211 Stats: 221 lines in 4 files changed: 2 ins; 14 del; 205 mod Patch: https://git.openjdk.java.net/jdk/pull/8853.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8853/head:pull/8853 PR: https://git.openjdk.java.net/jdk/pull/8853
Integrated: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException
On Tue, 26 Apr 2022 02:59:48 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 This pull request has now been integrated. Changeset: e60d8b51 Author:Valerie Peng URL: https://git.openjdk.java.net/jdk/commit/e60d8b516e97a2c1f662e1f31f1cfde2c9fd163e Stats: 59 lines in 2 files changed: 3 ins; 1 del; 55 mod 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException Reviewed-by: weijun - PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v10]
> 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 incrementally with one additional commit since the last revision: rewording the UOE for Signature.getParameters() and remove redundant sentences from SignatureSpi class. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8396/files - new: https://git.openjdk.java.net/jdk/pull/8396/files/681a5bc4..d5d75d74 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8396=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8396=08-09 Stats: 8 lines in 2 files changed: 0 ins; 6 del; 2 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: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v9]
On Tue, 17 May 2022 23:11:06 GMT, Weijun Wang wrote: >> Valerie Peng has updated the pull request incrementally with one additional >> commit since the last revision: >> >> more minor cleanups for consistencies. > > src/java.base/share/classes/java/security/SignatureSpi.java line 377: > >> 375: * >> 376: * This method is overridden by providers to initialize >> 377: * this signature object with the specified parameter set. > > 1st paragraph: values, 2nd: set. Make them the same. Will also remove the second sentence here. Thanks. > src/java.base/share/classes/java/security/SignatureSpi.java line 397: > >> 395: * >> 396: * This method is overridden by providers to return the >> parameters >> 397: * used with this signature object. > > The 2 paragraphs above look the same. Of course, if you believe the 1st > paragraph should always be a simple description, that's OK. Yes, I will remove the second sentence to match the javadoc of other methods. - PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8286908: ECDSA signature should not return parameters [v2]
On Wed, 18 May 2022 22:27:18 GMT, Weijun Wang wrote: >> Let ECDSA's `engineGetParameters()` always return null. At the same time, >> remove the remembered `sigParams` field. One behavior change is that after >> calling `setParameter()`, one can call `init()` again with a key using >> different parameters. I think this should be allowed since we are reusing >> the signature object with a brand new key. >> >> `setParameter` is kept unchanged to be able to deal with certificates still >> having parameters after the signature algorithm object identifier. See >> https://bugs.openjdk.java.net/browse/JDK-8225745. >> >> Also added SHA1withECDSA to the no-NULL list in `KnownOIDs`. >> >> All security-related tests passed. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > will not read params for ECDSA Looks fine. - Marked as reviewed by valeriep (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8758
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v9]
> 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 incrementally with one additional commit since the last revision: more minor cleanups for consistencies. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8396/files - new: https://git.openjdk.java.net/jdk/pull/8396/files/94a584bc..681a5bc4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8396=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8396=07-08 Stats: 43 lines in 2 files changed: 4 ins; 0 del; 39 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
Integrated: 8002277: Refactor two PBE classes to simplify maintenance
On Tue, 3 May 2022 19:30:40 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. This pull request has now been integrated. Changeset: 61ddbef3 Author:Valerie Peng URL: https://git.openjdk.java.net/jdk/commit/61ddbef3681770b7a1f56456f686fcb176063329 Stats: 733 lines in 6 files changed: 185 ins; 402 del; 146 mod 8002277: Refactor two PBE classes to simplify maintenance Reviewed-by: weijun - PR: https://git.openjdk.java.net/jdk/pull/8521
Integrated: 8209038: Clarify the javadoc of Cipher.getParameters()
On Wed, 6 Apr 2022 00:14: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~
This pull request has now been integrated.
Changeset: 0c5ab6da
Author:Valerie Peng
URL:
https://git.openjdk.java.net/jdk/commit/0c5ab6daa93cd063d8fa54880f7b1aa981c27c5f
Stats: 417 lines in 2 files changed: 3 ins; 5 del; 409 mod
8209038: Clarify the javadoc of Cipher.getParameters()
Reviewed-by: xuelei, mullan, weijun
-
PR: https://git.openjdk.java.net/jdk/pull/8117
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v7]
On Mon, 16 May 2022 13:28:13 GMT, Sean Mullan wrote:
>> With this modification of 2nd sentence. The whole paragraph becomes:
>>
>> * The returned parameters may be the same that were used to
>> initialize
>> * this signature, or may contain additional default or random parameter
>> * values used by the underlying signature scheme. If the required
>> * parameters were not supplied and can be generated by the signature,
>> * the generated parameters are returned; otherwise {@code null} is
>> * returned. However, if the signature scheme does not support returning
>> * the parameters as {@code AlgorithmParameters}, {@code null} is always
>> * returned.
>>
>> For the last sentence, would it be better to use "also" instead of "always"?
>
> I think "also" would not be quite right, because I would read that as there
> is something else that is also returned with `null`. I think you can remove
> the word "always" -- it isn't really necessary and that might address your
> concern.
Max suggested moving the last sentence to a new paragraph, so I did that and
leave the "always" in.
-
PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v8]
> 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 incrementally with one additional commit since the last revision: review feedback - Changes: - all: https://git.openjdk.java.net/jdk/pull/8396/files - new: https://git.openjdk.java.net/jdk/pull/8396/files/68b36fc1..94a584bc Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8396=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8396=06-07 Stats: 12 lines in 2 files changed: 6 ins; 0 del; 6 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: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v7]
On Fri, 13 May 2022 20:29:11 GMT, Sean Mullan wrote:
>> Valerie Peng has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fix newline.
>
> src/java.base/share/classes/java/security/Signature.java line 1012:
>
>> 1010: * values used by the underlying signature scheme. If the required
>> 1011: * parameters were not supplied and can be generated by the
>> signature,
>> 1012: * the generated parameters are returned. However, if the
>> signature scheme
>
> I think one small addition to the 2nd sentence would help to cover the case
> where null is returned if params were not set and the impl does not generate
> params (regardless as to whether it supports returning them as
> `AlgorithmParameters`): "If the required parameters were not supplied and can
> be generated by the signature, the generated parameters are returned;
> otherwise `null` is returned."
With this modification of 2nd sentence. The whole paragraph becomes:
* The returned parameters may be the same that were used to initialize
* this signature, or may contain additional default or random parameter
* values used by the underlying signature scheme. If the required
* parameters were not supplied and can be generated by the signature,
* the generated parameters are returned; otherwise {@code null} is
* returned. However, if the signature scheme does not support returning
* the parameters as {@code AlgorithmParameters}, {@code null} is always
* returned.
For the last sentence, would it be better to use "also" instead of "always"?
-
PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v7]
> 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 incrementally with one additional commit since the last revision: fix newline. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8396/files - new: https://git.openjdk.java.net/jdk/pull/8396/files/7026bb65..68b36fc1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8396=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8396=05-06 Stats: 4 lines in 2 files changed: 2 ins; 0 del; 2 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: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v6]
> 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 incrementally with one additional commit since the last revision: change the last sentence for the "cannot return AlgorithmParameters" case. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8396/files - new: https://git.openjdk.java.net/jdk/pull/8396/files/1f02b558..7026bb65 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8396=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8396=04-05 Stats: 4 lines in 2 files changed: 0 ins; 0 del; 4 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: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v4]
On Fri, 13 May 2022 14:35:56 GMT, Sean Mullan wrote:
>> Hmm, would it fall under the "Otherwise, null is returned"? If not, perhaps
>> we can add back the part about returning AlgorithmParameters as below:
>>
>> **If the underlying signature implementation supports returning the
>> parameters as {@code AlgorithmParameters},** the returned parameters may be
>> the same that were used to initialize
>> this signature, or may contain additional default or random parameter
>> values used by the underlying signature scheme. If the required parameters
>> were not supplied and can be generated by the signature, the generated
>> parameters are returned. Otherwise, {@code null} is returned.
>
> I think it is a special case that should be specified. Theoretically, it
> could also occur if params were not supplied by the caller, but the
> implementation generated parameters and then could not return them as
> AlgorithmParameters. I think the underlying issue is that not all Signature
> algorithms should be required to support an encoded form of the parameters.
>
> It is hard to fit all of these cases in a couple of sentences. I would be
> more inclined to add something like the following after the two sentences you
> currently have: "However, if the underlying implementation does not support
> returning the parameters as `AlgorithmParameters`, `null` is always returned."
Yes, I agree that it's a special case as the convention is to define the
parameter spec class based on the ASN.1 structure. I like your suggestion
better also. Will update using it.
-
PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8002277: Refactor two PBE classes to simplify maintenance [v7]
On Thu, 12 May 2022 15:03:40 GMT, Weijun Wang wrote: >> Valerie Peng has updated the pull request incrementally with one additional >> commit since the last revision: >> >> trivial syntax fix. > > src/java.base/share/classes/com/sun/crypto/provider/PKCS12PBECipherCore.java > line 363: > >> 361: return core.implGetParameters(); >> 362: } >> 363: @Override > > These 3 methods are the same in 3 classes. I wish we had a way to simplify > this but I don't know myself. Me neither. However, the refactoring is already reduced quite some code (~300 lines less for the two PBE classes), just can't be 100% lean. - PR: https://git.openjdk.java.net/jdk/pull/8521
Re: RFR: 8002277: Refactor two PBE classes to simplify maintenance [v8]
> 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: reset ivSpec and minor nit fix. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8521/files - new: https://git.openjdk.java.net/jdk/pull/8521/files/c1893f8b..95dfa948 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8521=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8521=06-07 Stats: 8 lines in 1 file changed: 1 ins; 2 del; 5 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 [v7]
On Thu, 12 May 2022 20:53:00 GMT, Weijun Wang wrote:
>> Valerie Peng has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> trivial syntax fix.
>
> src/java.base/share/classes/com/sun/crypto/provider/PBES2Core.java line 244:
>
>> 242: iCount = DEFAULT_COUNT;
>> 243: }
>> 244: //if (ivSpec == null) { // old behavior always generate
>
> How could `ivSpec` be non-null here? IIUC the only answer is from a previous
> `engineInit`, and it should not be retained. I suggest removing this check
> plus adding an explicit `ivSpec = null` at the beginning of this method along
> with `iCount` and `salt`. Those are the only 3 non final instance fields.
Yeah, it's also possible that ivSpec is non-null if getParameters() is called
before init(). Now that salt and iCount is reset in the beginning of init(),
ivSpec should be reset too.
-
PR: https://git.openjdk.java.net/jdk/pull/8521
Re: RFR: 8002277: Refactor two PBE classes to simplify maintenance [v7]
On Thu, 12 May 2022 19:27:20 GMT, Sean Mullan wrote: >> Valerie Peng has updated the pull request incrementally with one additional >> commit since the last revision: >> >> trivial syntax fix. > > src/java.base/share/classes/com/sun/crypto/provider/PBES2Core.java line 184: > >> 182: "Iteration count must be a positive number"); >> 183: } >> 184: return iCount == 0? DEFAULT_COUNT : iCount; > > Nit: add space before '?'. Sure - PR: https://git.openjdk.java.net/jdk/pull/8521
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v4]
On Thu, 12 May 2022 18:23:18 GMT, Sean Mullan wrote:
>> Fixed the nit. Thanks~
>> As for the part about returning the parameters as `{@code
>> AlgorithmParameters}`, it should be covered by current sentence, i.e. `and
>> can be generated by the signature`. Perhaps we don't have to spell out all
>> possible causes where the parameter can't be generated?
>
> That sentence is specifically if the caller did not specify parameters
> though. The previous wording is if the caller did specify parameters (as say
> an `AlgorithmParameterSpec`) and the implementation cannot return them as
> `AlgorithmParameters`. So I think it needs to be mentioned as it is kind of
> surprising if you specify parameters to `init` but `getParameters` returns
> null.
Hmm, would it fall under the "Otherwise, null is returned"? If not, perhaps we
can add back the part about returning AlgorithmParameters as below:
**If the underlying signature implementation supports returning the parameters
as {@code AlgorithmParameters},** the returned parameters may be the same that
were used to initialize
this signature, or may contain additional default or random parameter values
used by the underlying signature scheme. If the required parameters were not
supplied and can be generated by the signature, the generated parameters are
returned. Otherwise, {@code null} is returned.
-
PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8002277: Refactor two PBE classes to simplify maintenance [v7]
> 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: trivial syntax fix. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8521/files - new: https://git.openjdk.java.net/jdk/pull/8521/files/d253f8d6..c1893f8b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8521=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8521=05-06 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 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 [v5]
On Wed, 11 May 2022 23:45:00 GMT, Weijun Wang wrote:
>> Valerie Peng has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Changed to extend various CipherSpi implementations.
>
> src/java.base/share/classes/com/sun/crypto/provider/PKCS12PBECipherCore.java
> line 171:
>
>> 169:
>> 170: // holder for the derived key and iv
>> 171: private static class KeyAndIv implements Closeable {
>
> You can make this a `record`.
Sure, good suggestion~
> src/java.base/share/classes/com/sun/crypto/provider/PKCS12PBECipherCore.java
> line 317:
>
>> 315: Arrays.fill(derivedKey, (byte)0);
>> 316:
>> 317: KeyAndIv result;
>
> I think the fancy 2022 way is to `return switch (...) { case "RC4" -> new
> KeyAndIv(...) ... }`.
Ok~
-
PR: https://git.openjdk.java.net/jdk/pull/8521
Re: RFR: 8002277: Refactor two PBE classes to simplify maintenance [v6]
> 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: Updated to use record and switch expression. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8521/files - new: https://git.openjdk.java.net/jdk/pull/8521/files/39f626e1..d253f8d6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8521=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8521=04-05 Stats: 25 lines in 1 file changed: 0 ins; 13 del; 12 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: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v5]
On Thu, 12 May 2022 00:21:34 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 incrementally with one additional > commit since the last revision: > > Minor update. CSR filed at https://bugs.openjdk.java.net/browse/JDK-8286616 Thanks! - PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v5]
> 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 incrementally with one additional commit since the last revision: Minor update. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8396/files - new: https://git.openjdk.java.net/jdk/pull/8396/files/660adeeb..1f02b558 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8396=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8396=03-04 Stats: 4 lines in 2 files changed: 0 ins; 0 del; 4 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]
On Tue, 10 May 2022 20:38:31 GMT, Sean Mullan wrote:
>> 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()
>
> src/java.base/share/classes/javax/crypto/CipherSpi.java line 294:
>
>> 292: * values used by the underlying cipher implementation. If the
>> required
>> 293: * parameters were not supplied and can be generated by the cipher,
>> the
>> 294: * generated parameters will be returned. Otherwise, {@code null} is
>
> Minor wording nit - change "will be" to "are" to be consistent with other
> wording which uses the present tense. Same comment applies to
> `Cipher.getParameters`.
Yes, will fix the nit.
-
PR: https://git.openjdk.java.net/jdk/pull/8117
Re: RFR: 8209038: Clarify the javadoc of Cipher.getParameters() [v6]
> 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 incrementally with one additional
commit since the last revision:
Minor wording changes.
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/8117/files
- new: https://git.openjdk.java.net/jdk/pull/8117/files/f9727edf..10228192
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=8117=05
- incr: https://webrevs.openjdk.java.net/?repo=jdk=8117=04-05
Stats: 4 lines in 2 files changed: 0 ins; 2 del; 2 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: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v4]
On Tue, 10 May 2022 20:42:55 GMT, Sean Mullan wrote:
>> 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
>
> src/java.base/share/classes/java/security/SignatureSpi.java line 399:
>
>> 397: * values used by the underlying signature scheme. If the required
>> 398: * parameters were not supplied and can be generated by the
>> signature,
>> 399: * the generated parameters will be returned. Otherwise, {@code
>> null}
>
> Minor wording nit - change "will be" to "are" to be consistent with other
> wording which uses the present tense. Same comment applies to
> `Signature.getParameters`.
>
> Also, do we no longer need to mention the part "and the underlying signature
> implementation supports returning the parameters as {@code
> AlgorithmParameters}"?
Fixed the nit. Thanks~
As for the part about returning the parameters as `{@code
AlgorithmParameters}`, it should be covered by current sentence, i.e. `and can
be generated by the signature`. Perhaps we don't have to spell out all possible
causes where the parameter can't be generated?
-
PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8002277: Refactor two PBE classes to simplify maintenance [v5]
> 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: Changed to extend various CipherSpi implementations. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8521/files - new: https://git.openjdk.java.net/jdk/pull/8521/files/0907114b..39f626e1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8521=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8521=03-04 Stats: 255 lines in 4 files changed: 63 ins; 145 del; 47 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 Wed, 11 May 2022 04:05:27 GMT, Weijun Wang wrote: >> It's possible, more refactoring would be needed and not necessarily less >> lines of code. With your suggested change, the caller has to explicitly >> destroy the derived key after the cipher.engineInit() call. This would be >> repeated in all PKCS12 PBE cipher impl classes, but then there'd be no >> casting of the actual classes. I assume this is what you are referring to? >> Can code it out and see how it looks. > > If the returned key-and-iv class implements Closeable, then you can do a > try-with-resources to destroy the key, which saves you more lines. Yes, good point, will do so. - PR: https://git.openjdk.java.net/jdk/pull/8521
Re: RFR: 8002277: Refactor two PBE classes to simplify maintenance [v2]
On Tue, 10 May 2022 02:27:13 GMT, Weijun Wang wrote: >> 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? It's possible, more refactoring would be needed and not necessarily less lines of code. With your suggested change, the caller has to explicitly destroy the derived key after the cipher.engineInit() call. This would be repeated in all PKCS12 PBE cipher impl classes, but then there'd be no casting of the actual classes. I assume this is what you are referring to? Can code it out and see how it looks. - PR: https://git.openjdk.java.net/jdk/pull/8521
Re: RFR: 8286428: AlgorithmId should understand PBES2
On Mon, 9 May 2022 21:44:10 GMT, Weijun Wang wrote: > `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. Marked as reviewed by valeriep (Reviewer). Changes looks fine. - PR: https://git.openjdk.java.net/jdk/pull/8615
Re: RFR: 8002277: Refactor two PBE classes to simplify maintenance [v4]
> 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: Address review feedbacks - Changes: - all: https://git.openjdk.java.net/jdk/pull/8521/files - new: https://git.openjdk.java.net/jdk/pull/8521/files/307d2765..0907114b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8521=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8521=02-03 Stats: 63 lines in 2 files changed: 5 ins; 5 del; 53 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 Tue, 10 May 2022 00:09:16 GMT, Weijun Wang wrote: >> 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`. Yeah, I see PKCS12PBECipherCore does that. Will update PBES2Core with it too. - 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: 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
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: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v3]
> 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 incrementally with one additional commit since the last revision: Sync'ed w/ the wording in the other Cipher.getParameters() PR. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8396/files - new: https://git.openjdk.java.net/jdk/pull/8396/files/9be3644d..41e76e6e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8396=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8396=01-02 Stats: 11 lines in 2 files changed: 0 ins; 3 del; 8 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: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v2]
On Wed, 4 May 2022 04:16:42 GMT, Xue-Lei Andrew Fan wrote: >> How about the case when no parameters are given? Say A is the user-supplied >> values, B is the provider specific default or random values, your suggestion >> has A, A+B, and null. Isn't the sentence about B needed (no A and provider >> can generate the parameters)? > > I think this sentence covers case B, "... or may contain additional default > or random parameter > values used by the underlying signature implementation." Sean's comment on the other PR regarding Cipher.getParameters implies otherwise, i.e. `A few comments: I don't think this new text covers the case where the cipher is not initialized with any parameters, but the provider returns parameters. I tried to think of how to say that all in one sentence, but I think it is best to have two sentences covering each case, ex: "If this cipher was not initialized with parameters, the returned parameters may be null or may be ..." "If this cipher was initialized with parameters, the returned parameters may be the same or may be ..."` - PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v2]
On Thu, 5 May 2022 22:03:16 GMT, Valerie Peng wrote: >> I think this sentence covers case B, "... or may contain additional default >> or random parameter >> values used by the underlying signature implementation." > > Sean's comment on the other PR regarding Cipher.getParameters implies > otherwise, i.e. `A few comments: > > I don't think this new text covers the case where the cipher is not > initialized with any parameters, but the provider returns parameters. I tried > to think of how to say that all in one sentence, but I think it is best to > have two sentences covering each case, ex: "If this cipher was not > initialized with parameters, the returned parameters may be null or may be > ..." "If this cipher was initialized with parameters, the returned parameters > may be the same or may be ..."` He later agrees that two sentences are probably too lengthy, so we ended up exploring other wording choices. - PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8002277: Refactor two PBE classes to simplify maintenance [v2]
> 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/8521/files - new: https://git.openjdk.java.net/jdk/pull/8521/files/3cc07c98..1a2b3f90 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8521=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8521=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 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: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v2]
On Tue, 3 May 2022 00:17:11 GMT, Weijun Wang wrote: >> An example is RSASSA-PSS, i.e. it requires the caller to explicitly state >> which message digest to use, etc. > > You listed 2 cases when null is returned: 1) not supplied. 2) cannot > generate. My understanding is that the RSASSA-PSS case is 1), and the EdDSA > case is 2). This is why I suggested an "or" relation between them. For EdDSA, it's really the case of the signature impl cannot convert the supplied parameter values to "AlgorithmParameters" object. - PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8286069: keytool prints out wrong key algorithm for -importpass command
On Tue, 3 May 2022 17:51:43 GMT, Weijun Wang wrote:
> Since `keytool -importpass` always uses `KeyFactory.getInstance("PBE")` to
> generate the secret key, and "PBE" is an alias of "PBEwithMD5andDES" inside
> the SunJCE security provider, its `getAlgorithm` is always `PBEwithMD5andDES`.
>
> This code change modifies it to "PBE".
>
> Note that I haven't chosen the `-keyalg` option value here because it is
> actually the algorithm used to protect the PBE secret key entry. It's a
> cipher algorithm instead of a key algorithm.
Marked as reviewed by valeriep (Reviewer).
test/jdk/sun/security/pkcs12/ImportPassKeyAlg.java line 75:
> 73: .shouldContain("Generated PBE secret key");
> 74:
> 75: // The aid of a protected entry (at 110c010c01010c0 inside p12)
> is:
nit: use "algorithm id" instead.
-
PR: https://git.openjdk.java.net/jdk/pull/8520
RFR: 8002277: Refactor two PBE classes to simplify maintenance
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. - Commit messages: - Enhanced test with more decryption w/o parameters scenario. - 8002277: Refactor two PBE classes to simplify maintenance Changes: https://git.openjdk.java.net/jdk/pull/8521/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8521=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8002277 Stats: 542 lines in 3 files changed: 168 ins; 289 del; 85 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: 8209038: Clarify the javadoc of Cipher.getParameters() [v4]
> 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 incrementally with one additional
commit since the last revision:
update to address review feedback and minor javadoc cleanup
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/8117/files
- new: https://git.openjdk.java.net/jdk/pull/8117/files/baae26fe..77e8fa0d
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=8117=03
- incr: https://webrevs.openjdk.java.net/?repo=jdk=8117=02-03
Stats: 171 lines in 2 files changed: 1 ins; 0 del; 170 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: 8286024: PKCS12 keystore shows "DES/CBC" as the algorithm of a DES SecretKeyEntry
On Mon, 2 May 2022 17:41:52 GMT, Weijun Wang wrote: > PKCS12 stores the object identifier of a SecretKey along with it, and when > retrieved, translate the object identifier to an algorithm name. > Unfortunately, inside `KnownOIDs.java`, "DES" is [only registered > as](https://github.com/wangweij/jdk/blob/7a6cbef157b67bb4fb877617f2a23228aade9a5d/src/java.base/share/classes/sun/security/util/KnownOIDs.java#L368-L368) > an alias of another name "DES/CBC". We should modify it to "DES" before > returning the secret key. Looks fine. Thanks. - Marked as reviewed by valeriep (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8505
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v2]
On Fri, 29 Apr 2022 04:27:36 GMT, Xue-Lei Andrew Fan wrote:
>> What kind of additional sentence do you have in mind?
>
>> What kind of additional sentence do you have in mind?
>
> It may be fine to put it into the state for 'null" returned value. For
> example:
>
>
> The returned parameters may be the same that were used to initialize
> this signature, or may contain additional default or random parameter
> values used by the underlying signature implementation, or null if the
> underlying signature implementation does not support returning the
> parameters as {@code AlgorithmParameters}.
>
>
>
> The null return conditional in the following sentence may be able to combine
> together.
>
>
> The returned parameters may be the same that were used to initialize
> this signature, or may contain additional default or random parameter
> values used by the underlying signature implementation. {@code null}
> may be returned if the underlying signature implementation does not
> support returning the parameters as {@code AlgorithmParameters}, or conditions>
How about the case when no parameters are given? Say A is the user-supplied
values, B is the provider specific default or random values, your suggestion
has A, A+B, and null. Isn't the sentence about B needed (no A and provider can
generate the parameters)?
-
PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v2]
On Thu, 28 Apr 2022 23:28:39 GMT, Weijun Wang wrote: >> The impl does not need to generate parameter values, but rather cannot >> convert the supplied parameter values into AlgorithmParameter objects. By >> parameter values, I mean the components of the parameters. > > Then what does "cannot generate parameter values" mean? Any example? An example is RSASSA-PSS, i.e. it requires the caller to explicitly state which message digest to use, etc. - PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8209038: Clarify the javadoc of Cipher.getParameters() [v3]
On Fri, 29 Apr 2022 15:23:47 GMT, Sean Mullan wrote:
>> Valerie Peng has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Update for getParameters()
>
> src/java.base/share/classes/javax/crypto/Cipher.java line 1053:
>
>> 1051: * The returned parameters may be the same that were used to
>> initialize
>> 1052: * this cipher, or may contain additional default or random
>> parameter
>> 1053: * values used by the underlying cipher implementation. If the
>> required
>
> The PR for https://github.com/openjdk/jdk/pull/8396/ has one difference in
> this sentence in that it says "... values used by the underlying signature
> implementation _if the underlying signature implementation supports returning
> the parameters as {@code AlgorithmParameters}_." Should this also be
> consistent?
>
> Also, I don't think you need to repeat "underlying signature " again above,
> you could just say: ... values used by the underlying signature
> implementation _if the implementation supports returning the parameters as
> {@code AlgorithmParameters}_."
Signature class has that sentence is due to the peculiar case of EdDSA
signature impl where its parameter spec does not correlate to an ASN.1
structure. Since there is no such case for current cipher impls (as far as I
know), that's why I have not added it to Cipher class.
Ok, how about `If the required parameters were not supplied and can be
generated by the cipher, the generated parameters will be returned.`?
-
PR: https://git.openjdk.java.net/jdk/pull/8117
Re: RFR: 8209038: Clarify the javadoc of Cipher.getParameters() [v3]
On Fri, 29 Apr 2022 15:18:34 GMT, Sean Mullan wrote: >> Valerie Peng has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Update for getParameters() > > src/java.base/share/classes/javax/crypto/Cipher.java line 1055: > >> 1053: * values used by the underlying cipher implementation. If the >> required >> 1054: * parameters were not supplied and the underlying cipher >> implementation >> 1055: * can generate the parameter values, it will be returned. >> Otherwise, > > "it will be returned" sounds a bit awkward here. I would change this to "the > generated parameter values will be returned". Ok. - PR: https://git.openjdk.java.net/jdk/pull/8117
Re: RFR: 8209038: Clarify the javadoc of Cipher.getParameters() [v3]
On Thu, 28 Apr 2022 19:23:18 GMT, Weijun Wang wrote:
>> Valerie Peng has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Update for getParameters()
>
> src/java.base/share/classes/javax/crypto/CipherSpi.java line 449:
>
>> 447: *
>> 448: * Note that when a Cipher object is initialized, it loses all
>> 449: * previously-acquired state. In other words, initializing a Cipher
>> is
>
> Two `{@code Cipher}` above.
Yes, I will look through the whole class (and CipherSpi.java too) regarding the
Cipher vs cipher thing.
-
PR: https://git.openjdk.java.net/jdk/pull/8117
Re: RFR: 8209038: Clarify the javadoc of Cipher.getParameters() [v3]
On Thu, 28 Apr 2022 19:17:08 GMT, Weijun Wang wrote:
>> Valerie Peng has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Update for getParameters()
>
> src/java.base/share/classes/javax/crypto/Cipher.java line 1056:
>
>> 1054: * parameters were not supplied and the underlying cipher
>> implementation
>> 1055: * can generate the parameter values, it will be returned.
>> Otherwise,
>> 1056: * {@code null} returned.
>
> Should this be "null is returned"?
Yes, will fix.
> src/java.base/share/classes/javax/crypto/Cipher.java line 1787:
>
>> 1785: * Ensures that Cipher is in a valid state for update() and
>> doFinal()
>> 1786: * calls - should be initialized and in ENCRYPT_MODE or
>> DECRYPT_MODE.
>> 1787: * @throws IllegalStateException if Cipher object is not in valid
>> state
>
> "Cipher" in `{@code}`? Or make it lowercase.
Ok, I will use lowercase.
-
PR: https://git.openjdk.java.net/jdk/pull/8117
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v2]
On Thu, 28 Apr 2022 23:14:56 GMT, Weijun Wang wrote:
>> I assume you were suggesting this? `"The returned parameters may be the same
>> that were used to initialize this signature, or may contain additional
>> default or random parameter values used by the underlying signature
>> implementation. {https://github.com/code null} is returned if the required
>> parameters were not supplied and the underlying signature implementation
>> cannot generate the parameter values."`
>> But the "the underlying signature implementation supports returning the
>> parameters as {https://github.com/code AlgorithmParameters}" is necessary.
>> Strictly speaking, this is somewhat different than the "cannot generate
>> parameter values" though. Perhaps we should go a bit broader for the last
>> sentence regarding null return value?
>
> I suggest the last sentence to be "null is returned if the required
> parameters were not supplied **or** the underlying signature implementation
> cannot generate the parameter values." I used "or" because for EdDSA
> parameters are supplied but the impl cannot generate parameter values.
The impl does not need to generate parameter values, but rather cannot convert
the supplied parameter values into AlgorithmParameter objects. By parameter
values, I mean the components of the parameters.
-
PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v2]
On Wed, 27 Apr 2022 23:02:28 GMT, Weijun Wang wrote:
>> Right, the user-supplied values takes precedence and provider-specific
>> default/random values should just be supplemental.
>>
>> As for EdDSA, looks like the prehash and context are only in RFC 8032 and
>> NOT RFC 8410. caller has to pass these settings/values to the other entity
>> through some other means since the getParameters() return null as there is
>> no ASN.1 definition for the parameters. Looks like these values are packaged
>> into a parameter spec and passed into the underlying EdDSA signature
>> implementation in order to fit into existing API without adding new
>> algorithm specific APIs.
>
> So, "the underlying signature implementation supports returning the
> parameters as {@code AlgorithmParameters}" is quite necessary. Xuelei's
> suggestion is quite good, just change the last "and" to "or".
I assume you were suggesting this? `"The returned parameters may be the same
that were used to initialize this signature, or may contain additional default
or random parameter values used by the underlying signature implementation.
{https://github.com/code null} is returned if the required parameters were not
supplied and the underlying signature implementation cannot generate the
parameter values."`
But the "the underlying signature implementation supports returning the
parameters as {https://github.com/code AlgorithmParameters}" is necessary.
Strictly speaking, this is somewhat different than the "cannot generate
parameter values" though. Perhaps we should go a bit broader for the last
sentence regarding null return value?
-
PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v2]
On Thu, 28 Apr 2022 04:56:47 GMT, Xue-Lei Andrew Fan wrote:
>>> Can you clarify what is the A and B that you are referring to?
>>
>> The sentence is, “If the required parameters were not supplied and the
>> underlying signature implementation can generate the parameter values, it
>> will be returned. Otherwise, {https://github.com/code null} is returned."
>>
>> I read "the required parameters were not supplied" as condition A; and "the
>> underlying signature implementation can generate the parameter values" as
>> condition B.
>
>> With Signature class, there is a caveat for EdDSA, the supplied parameters
>> are set but null is being returned when getParameters() is called. This is
>> currently covered by the condition `if the underlying signature
>> implementation supports returning the parameters as {@code
>> AlgorithmParameters}` as the underlying signature does not support
>> AlgorithmParameters for the supplied EdDSAParameterSpec object due to lack
>> of ASN.1 definition.
>
> I see now. My 1st read of the condition, I did not get the point and thought
> it is not necessary as the method is trying to return "AlgorithmParameters".
> Could it be more clear if describe this case in an additional sentence?
What kind of additional sentence do you have in mind?
-
PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8209038: Clarify the javadoc of Cipher.getParameters() [v3]
> 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 incrementally with one additional
commit since the last revision:
Update for getParameters()
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/8117/files
- new: https://git.openjdk.java.net/jdk/pull/8117/files/9b8b71a6..baae26fe
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=8117=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk=8117=01-02
Stats: 8 lines in 2 files changed: 2 ins; 0 del; 6 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: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v2]
On Wed, 27 Apr 2022 23:30:22 GMT, Valerie Peng wrote:
>> Can you clarify what is the A and B that you are referring to? The way I
>> read it, it has more than 2 conditions... So, best to clarify the conditions
>> first.
>> I see your point with the wording suggestion at the end. Was a bit lost when
>> trying to go through the various if-else logic and figure out what you
>> mean...
>
> With Signature class, there is a caveat for EdDSA, the supplied parameters
> are set but null is being returned when getParameters() is called. This is
> currently covered by the condition `if the underlying signature
> implementation supports returning the parameters as {@code
> AlgorithmParameters}` as the underlying signature does not support
> AlgorithmParameters for the supplied EdDSAParameterSpec object due to lack of
> ASN.1 definition.
Besides this Signature-specific condition, there is the common condition where
provider cannot (or do not) generate default parameter values. {@code null} is
used as the catch-all result, but as you said, describe various conditions
tersely and correctly is key.
-
PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v2]
On Wed, 27 Apr 2022 23:19:56 GMT, Valerie Peng wrote:
>>> What does it refer to with 'it'? Is 'it' refer to the implementation
>>> generated parameter values?
>>
>> 'It' refers to the parameters containing all of the parameter values
>> including the supplied ones and provider-generated ones if any.
>
> Can you clarify what is the A and B that you are referring to? The way I read
> it, it has more than 2 conditions... So, best to clarify the conditions
> first.
> I see your point with the wording suggestion at the end. Was a bit lost when
> trying to go through the various if-else logic and figure out what you mean...
With Signature class, there is a caveat for EdDSA, the supplied parameters are
set but null is being returned when getParameters() is called. This is
currently covered by the condition `if the underlying signature implementation
supports returning the parameters as {@code AlgorithmParameters}` as the
underlying signature does not support AlgorithmParameters for the supplied
EdDSAParameterSpec object due to lack of ASN.1 definition.
-
PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v2]
On Wed, 27 Apr 2022 23:15:41 GMT, Valerie Peng wrote:
>> src/java.base/share/classes/java/security/Signature.java line 1014:
>>
>>> 1012: * {@code AlgorithmParameters}. If the required
>>> 1013: * parameters were not supplied and the underlying signature
>>> implementation
>>> 1014: * can generate the parameter values, it will be returned.
>>> Otherwise,
>>
>>> If the required parameters were not supplied and the underlying signature
>>> implementation can generate the parameter values, it will be returned.
>>
>> What does it refer to with 'it'? Is 'it' refer to the implementation
>> generated parameter values?
>>
>>> If the required parameters were not supplied and the underlying signature
>>> implementation can generate the parameter values, it will be returned.
>>> Otherwise, {@code null} is returned.
>>
>> The logic looks like
>>
>> if (A & B) {
>> // it will be returned
>> } else {
>> // {@code null} is returned.
>> }
>>
>> If I read it correctly, the behavior may look like:
>> 1. If the required parameters were supplied, {@code null} is returned; (if
>> !A)
>> 2. if the underlying signature implementation cannot generate the parameter
>> values, {@code null} is returned; (if !B)
>> 3. if not 1 and 2, ... (if A & B)
>>
>> It does not look like right. The expected behavior may be:
>>
>> if (A) {
>> if (B) {
>> // it will be returned
>> } else {
>> // {@code null} is returned.
>> }
>> }
>>
>>
>> Maybe, the confusion could be mitigated by re-org the logic:
>>
>> if (A & !B) {
>> // {@code null} is returned.
>> } // Otherwise, refer to 1st sentence.
>>
>>
>> "The returned parameters may be the same that were used to initialize this
>> signature, or may contain additional default or random parameter values used
>> by the underlying signature implementation. {@code null} is returned if
>> the required parameters were not supplied and the underlying signature
>> implementation cannot generate the parameter values."
>>
>> Similar comment to [PR 8117](https://github.com/openjdk/jdk/pull/8117), if
>> you want to use similar description there as well.
>
>> What does it refer to with 'it'? Is 'it' refer to the implementation
>> generated parameter values?
>
> 'It' refers to the parameters containing all of the parameter values
> including the supplied ones and provider-generated ones if any.
Can you clarify what is the A and B that you are referring to? The way I read
it, it has more than 2 conditions... So, best to clarify the conditions first.
-
PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v2]
On Wed, 27 Apr 2022 05:25:42 GMT, Xue-Lei Andrew Fan wrote:
>> Valerie Peng has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Undo un-intentional changes.
>
> src/java.base/share/classes/java/security/Signature.java line 1012:
>
>> 1010: * values used by the underlying signature implementation if the
>> underlying
>> 1011: * signature implementation supports returning the parameters as
>> 1012: * {@code AlgorithmParameters}. If the required
>
> "... if the underlying signature implementation supports returning the
> parameters a {@code AlgorithmParameters}", it looks this part is duplicated
> and may be removed.
I am not sure which part duplicates the text that you quoted? The original text
only mentions the supplied values. The proposed changes add that there may be
additional parameters. Are you saying that these additional parameters are
"implied" by the part that you quoted?
> What does it refer to with 'it'? Is 'it' refer to the implementation
> generated parameter values?
'It' refers to the parameters containing all of the parameter values including
the supplied ones and provider-generated ones if any.
-
PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v2]
On Wed, 27 Apr 2022 15:10:42 GMT, Weijun Wang wrote: >> I searched about "and/or" and it is said that "or" covers "and". So, >> "and/or" should just be "or". >> >> I am on the fence for requiring provider to generate default parameters >> (using provider-specific or random values). Could there be scenarios where >> users are required to supply parameters? >> That aside, this javadoc is lastly updated by JDK-8243424. It seems that >> JDK's EdDSA signature impl has some interesting usage of >> AlgorithmParameterSpec which this javadoc has to cover/allow. > > RSASSA-PSS always requires a user-provided params. > > I think one thing we can guarantee is that the default/random values > generated by the impl will never overwrite the user-provided ones, they will > only be supplemented. Also, I think the real usage of this method is that the > result -- after converting to a spec -- can be set on the verify side and the > verification should succeed. > > I don't quite understand EdDSA. Looks like the params is not stored along > with the signature in the algorithm identifier. I also haven't found > different OIDs defined when prehash or context is used. How does the verifier > know this kind of flavor settings? Right, the user-supplied values takes precedence and provider-specific default/random values should just be supplemental. As for EdDSA, looks like the prehash and context are only in RFC 8032 and NOT RFC 8410. caller has to pass these settings/values to the other entity through some other means since the getParameters() return null as there is no ASN.1 definition for the parameters. Looks like these values are packaged into a parameter spec and passed into the underlying EdDSA signature implementation in order to fit into existing API without adding new algorithm specific APIs. - PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8209038: Clarify the javadoc of Cipher.getParameters() [v2]
On Tue, 26 Apr 2022 19:26:41 GMT, Sean Mullan wrote: >> I have filed the PR for the Signature at: >> https://github.com/openjdk/jdk/pull/8396 >> Best to get it done along with this one. > >> As for the 2nd sentence, it boils down whether we requires provider to >> generate default parameters and return it when parameter is required. >> Existing javadoc states that null is returned when parameter is not >> required. Given Cipher covers many algorithms, if a provider does not want >> to generate a default parameter when parameter is required, it can't return >> null when getParameters() is called. > > Ok, maybe we need to reset the context. What was the main issue with the > current specification that you were trying to fix? I read the associated bug > report. It sounds like there were two issues: > > - ProviderException can be thrown, but the method does not declare it in the > throws clause. That doesn't seem to be addressed here - do you think it > should be? > - cases when null may be returned as opposed to throwing a ProviderException > > It could be that what is really just needed is an @throws ProviderException, > ex: > > "If this cipher implementation requires algorithm parameters to be passed in > at initialization but was not initialized with any". > > and a slight tweak to the description (changes in italics): > > "... if this cipher _can generate required_ algorithm parameters but was not > initialized with any." I don't see the ProviderException being mentioned? Per the description under JDK-8209038, the requests are: 1) describe the returned parameters following what's in Signature class, i.e. if this object has been initialized with parameters then ..., if this object has not been initialized with parameters, then . (<= Xuelei raises compatibility concern and trying to describe all this would make it very lengthy, so the proposed changes reverted back to the original syntax, e.g. describing the returned parameters but not including the scenarios) 2) allow null to be returned if providers cannot generate default parameters. (<= this is addressed in the proposed changes) 3) accommodate algorithm-specific/provider-specific implementation on how parameters is handled. (<= this is addressed in the proposed changes as well. However, this part in Signature class needs update since it states the the SAME parameters are returned but AlgorithmParameterSpec may not require all parameter values to be specified.) - PR: https://git.openjdk.java.net/jdk/pull/8117
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v2]
On Tue, 26 Apr 2022 14:59:35 GMT, Weijun Wang wrote:
>> Valerie Peng has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Undo un-intentional changes.
>
> src/java.base/share/classes/java/security/Signature.java line 1015:
>
>> 1013: * parameters were not supplied and the underlying signature
>> implementation
>> 1014: * can generate the parameter values, it will be returned.
>> Otherwise,
>> 1015: * {@code null} is returned.
>
> If we cannot guarantee anything but just want to clarify. how about just say
> "The returned parameters is a combination of user-provided values,
> implementation default values, and/or random values used by the underlying
> signature implementation. It can also be {@code null} if there's none."
>
> I assume that if a value is necessary but user has not provided one, then
> there will be a default or a random one.
I searched about "and/or" and it is said that "or" covers "and". So, "and/or"
should just be "or".
I am on the fence for requiring provider to generate default parameters (using
provider-specific or random values). Could there be scenarios where users are
required to supply parameters?
That aside, this javadoc is lastly updated by JDK-8243424. It seems that JDK's
EdDSA signature impl has some interesting usage of AlgorithmParameterSpec which
this javadoc has to cover/allow.
-
PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8285683: Missing @ since 11 in java.security.spec.MGF1ParameterSpec fields
On Tue, 26 Apr 2022 22:55:29 GMT, Bradford Wetmore wrote: > Two new constant fields `MGF1ParameterSpec.SHA512_224` and > `MGF1ParameterSpec.SHA512_256` didn't have `@since 11` tag added as part of > [JDK-8146293](https://bugs.openjdk.java.net/browse/JDK-8146293). > > This bug addresses this issue. Is a CSR needed? - PR: https://git.openjdk.java.net/jdk/pull/8411
Re: RFR: 8285683: Missing @ since 11 in java.security.spec.MGF1ParameterSpec fields
On Tue, 26 Apr 2022 22:55:29 GMT, Bradford Wetmore wrote: > Two new constant fields `MGF1ParameterSpec.SHA512_224` and > `MGF1ParameterSpec.SHA512_256` didn't have `@since 11` tag added as part of > [JDK-8146293](https://bugs.openjdk.java.net/browse/JDK-8146293). > > This bug addresses this issue. Looks good. Thanks~ - Marked as reviewed by valeriep (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8411
Re: RFR: 8209038: Clarify the javadoc of Cipher.getParameters() [v2]
On Mon, 25 Apr 2022 18:14:07 GMT, Sean Mullan wrote: >> As for the 2nd sentence, it boils down whether we requires provider to >> generate default parameters and return it when parameter is required. >> Existing javadoc states that null is returned when parameter is not >> required. Given Cipher covers many algorithms, if a provider does not want >> to generate a default parameter when parameter is required, it can't return >> null when getParameters() is called. > >> I can do the Signature update through >> https://bugs.openjdk.java.net/browse/JDK-8253176 which I have targeted to >> fix in 19 also. > > Ok. I have filed the PR for the Signature at: https://github.com/openjdk/jdk/pull/8396 Best to get it done along with this one. - PR: https://git.openjdk.java.net/jdk/pull/8117
Re: RFR: 8285404: RSA signature verification should reject non-DER OCTET STRING [v2]
On Tue, 26 Apr 2022 16:02:41 GMT, Weijun Wang wrote: >> Compare encoded instead of decoded digest in RSA signature verification. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > only check digest value Marked as reviewed by valeriep (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8365
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v2]
> 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 incrementally with one additional commit since the last revision: Undo un-intentional changes. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8396/files - new: https://git.openjdk.java.net/jdk/pull/8396/files/033d4d85..9be3644d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8396=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8396=00-01 Stats: 3 lines in 1 file changed: 0 ins; 2 del; 1 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: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException
On Tue, 26 Apr 2022 14:51:31 GMT, Weijun Wang 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
>
> src/java.base/share/classes/java/security/Signature.java line 1510:
>
>> 1508: try {
>> 1509: return cipher.doFinal();
>> 1510: } catch (IllegalBlockSizeException | BadPaddingException
>> e) {
>
> Is this a revert of a recent fix from someone else?
Must be. Thanks
-
PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8285404: RSA signature verification should reject non-DER OCTET STRING
On Sun, 24 Apr 2022 14:34:46 GMT, Weijun Wang wrote: > Regardless whether we ended up with decode/encode, we should make sure > RSASSA-PSS signature impl is also covered and consistent. Never mind, PSS has its own way of verification and its impl is based on RFC 8017. - PR: https://git.openjdk.java.net/jdk/pull/8365
RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException
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 - Commit messages: - 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException Changes: https://git.openjdk.java.net/jdk/pull/8396/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8396=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8253176 Stats: 28 lines in 2 files changed: 4 ins; 3 del; 21 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: 8285404: RSA signature verification should follow RFC 8017 8.2.2 Step 4
On Fri, 22 Apr 2022 17:10:58 GMT, Weijun Wang wrote: > Compare encoded instead of decoded digest in RSA signature verification. Regardless whether we ended up with decode/encode, we should make sure RSASSA-PSS signature impl is also covered and consistent. - PR: https://git.openjdk.java.net/jdk/pull/8365
Re: RFR: 8285431: Assertion in NativeGSSContext constructor
On Fri, 22 Apr 2022 06:26:01 GMT, Xue-Lei Andrew Fan wrote:
> Hi,
>
> May I have the simple update reviewed.
>
> In the NativeGSSContext constructor for imported context, the assert is use
> on the object field, instead of the input parameters. As in a constructor,
> `'this'` object does not exist yet, this looks like an obvious issue. The
> fix is straightforward as well.
>
>
> NativeGSSContext(long pCtxt, GSSLibStub stub) throws GSSException {
> - assert(pContext != 0);
> + assert(pCtxt != 0);
> pContext = pCtxt;
> ...
Marked as reviewed by valeriep (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/8355
Re: RFR: 8209038: Clarify the javadoc of Cipher.getParameters() [v2]
On Thu, 21 Apr 2022 23:15:10 GMT, Valerie Peng wrote:
>>> For (1), how about something like below:
>>>
>>> > ```
>>> > * The returned parameters may be the same that were used to initialize
>>> > * this cipher, or may contain additional default or random parameter
>>> > * values used by the underlying cipher implementation.
>>
>> I like this first sentence.
>>
>>> > If no parameters
>>> > * is supplied and this cipher successfully generated the required
>>> > * parameter values, it will be returned.
>>
>> What does "successfully" mean? If it wasn't successful, what happens? Maybe
>> we should avoid that word. How about:
>>
>> "If parameters were not supplied and this cipher requires parameters, the
>> returned parameters will contain the parameter values generated by the
>> underlying cipher implementation."
>>
>>> > Otherwise, {@code null} is
>>> > * returned.
>>> > ```
>
>> > Hmm, I tried the suggested approach in (1), the result looks very lengthy.
>> > Actually, the Cipher.init(..) methods already has a few paragraphs
>> > describing the behavior for parameter generation, perhaps we should not
>> > repeat stating the "If this cipher was initialized" vs "was not
>> > initialized" since lengthy description may confuse users and have higher
>> > risks of inconsistency. What do you think?
>>
>> That's a good point, the `init` methods do go into a lot of detail about
>> that.
>>
>> > As for (2), currently only RSASSA-PSS signature impl uses parameters. When
>> > specified using PSSParameterSpec, it's possible that some of the
>> > parameters are not set, so it's possible for providers to use
>> > provider-specific default values for PSS case. So, yes, Signature class
>> > may have to updated in the same way to cover this particular scenario.
>>
>> Ok, I think we should try to make the spec consistent across `Cipher` and
>> `Signature` once we settle on the right wording. I think it would be better
>> to do it as part of this issue, but I would be ok doing it later as long as
>> it is also fixed in 19.
>
> I can do the Signature update through
> https://bugs.openjdk.java.net/browse/JDK-8253176 which I have targeted to fix
> in 19 also.
As for the 2nd sentence, it boils down whether we requires provider to generate
default parameters and return it when parameter is required. Existing javadoc
states that null is returned when parameter is not required. Given Cipher
covers many algorithms, if a provider does not want to generate a default
parameter when parameter is required, it can't return null when getParameters()
is called.
-
PR: https://git.openjdk.java.net/jdk/pull/8117
Re: RFR: 8209038: Clarify the javadoc of Cipher.getParameters() [v2]
On Wed, 20 Apr 2022 16:40:34 GMT, Sean Mullan wrote:
>>> Hmm, I tried the suggested approach in (1), the result looks very lengthy.
>>> Actually, the Cipher.init(..) methods already has a few paragraphs
>>> describing the behavior for parameter generation, perhaps we should not
>>> repeat stating the "If this cipher was initialized" vs "was not
>>> initialized" since lengthy description may confuse users and have higher
>>> risks of inconsistency. What do you think?
>>
>> That's a good point, the `init` methods do go into a lot of detail about
>> that.
>>
>>> As for (2), currently only RSASSA-PSS signature impl uses parameters. When
>>> specified using PSSParameterSpec, it's possible that some of the parameters
>>> are not set, so it's possible for providers to use provider-specific
>>> default values for PSS case. So, yes, Signature class may have to updated
>>> in the same way to cover this particular scenario.
>>
>> Ok, I think we should try to make the spec consistent across `Cipher` and
>> `Signature` once we settle on the right wording. I think it would be better
>> to do it as part of this issue, but I would be ok doing it later as long as
>> it is also fixed in 19.
>
>> For (1), how about something like below:
>>
>> > ```
>> > * The returned parameters may be the same that were used to initialize
>> > * this cipher, or may contain additional default or random parameter
>> > * values used by the underlying cipher implementation.
>
> I like this first sentence.
>
>> > If no parameters
>> > * is supplied and this cipher successfully generated the required
>> > * parameter values, it will be returned.
>
> What does "successfully" mean? If it wasn't successful, what happens? Maybe
> we should avoid that word. How about:
>
> "If parameters were not supplied and this cipher requires parameters, the
> returned parameters will contain the parameter values generated by the
> underlying cipher implementation."
>
>> > Otherwise, {@code null} is
>> > * returned.
>> > ```
> > Hmm, I tried the suggested approach in (1), the result looks very lengthy.
> > Actually, the Cipher.init(..) methods already has a few paragraphs
> > describing the behavior for parameter generation, perhaps we should not
> > repeat stating the "If this cipher was initialized" vs "was not
> > initialized" since lengthy description may confuse users and have higher
> > risks of inconsistency. What do you think?
>
> That's a good point, the `init` methods do go into a lot of detail about that.
>
> > As for (2), currently only RSASSA-PSS signature impl uses parameters. When
> > specified using PSSParameterSpec, it's possible that some of the parameters
> > are not set, so it's possible for providers to use provider-specific
> > default values for PSS case. So, yes, Signature class may have to updated
> > in the same way to cover this particular scenario.
>
> Ok, I think we should try to make the spec consistent across `Cipher` and
> `Signature` once we settle on the right wording. I think it would be better
> to do it as part of this issue, but I would be ok doing it later as long as
> it is also fixed in 19.
I can do the Signature update through
https://bugs.openjdk.java.net/browse/JDK-8253176 which I have targeted to fix
in 19 also.
-
PR: https://git.openjdk.java.net/jdk/pull/8117
Integrated: 8284553: Deprecate the DEFAULT static field of OAEPParameterSpec
On Tue, 12 Apr 2022 01:27:35 GMT, Valerie Peng wrote: > This trivial change is to deprecate the DEFAULT static field of > OAEPParameterSpec class. Wordings are mostly the same as the previous > PSSParameterSpec deprecation change. Rest are just minor code re-factoring. > > The CSR will be filed once review is somewhat finished. > > Thanks, > Valerie This pull request has now been integrated. Changeset: 15ce8c61 Author:Valerie Peng URL: https://git.openjdk.java.net/jdk/commit/15ce8c61956ec433bcb713c694e6cef7a61e3837 Stats: 53 lines in 2 files changed: 15 ins; 18 del; 20 mod 8284553: Deprecate the DEFAULT static field of OAEPParameterSpec Reviewed-by: mullan - PR: https://git.openjdk.java.net/jdk/pull/8191
Re: RFR: 8284933: Improve debug in jdk.crypto.cryptoki [v2]
On Tue, 19 Apr 2022 13:48:26 GMT, Xue-Lei Andrew Fan wrote: >> Hi, >> >> May I have the simple update reviewed? >> >> In the jdk.crypto.cryptoki module implementation, some of the debug >> information could be calculated even if the debug is not enabled, which is >> not resource friendly. >> >> Thanks, >> Xuelei > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > Remove debug() method Looks good to me. Thanks~ - Marked as reviewed by valeriep (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8277
Re: RFR: 8284553: Deprecate the DEFAULT static field of OAEPParameterSpec [v4]
> This trivial change is to deprecate the DEFAULT static field of > OAEPParameterSpec class. Wordings are mostly the same as the previous > PSSParameterSpec deprecation change. Rest are just minor code re-factoring. > > The CSR will be filed once review is somewhat finished. > > Thanks, > Valerie Valerie Peng has updated the pull request incrementally with one additional commit since the last revision: Remove "the" and change to "@throws". - Changes: - all: https://git.openjdk.java.net/jdk/pull/8191/files - new: https://git.openjdk.java.net/jdk/pull/8191/files/a760fc51..cbae8613 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8191=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8191=02-03 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8191.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8191/head:pull/8191 PR: https://git.openjdk.java.net/jdk/pull/8191
Re: RFR: 8284553: Deprecate the DEFAULT static field of OAEPParameterSpec [v3]
On Tue, 19 Apr 2022 19:54:24 GMT, Sean Mullan wrote:
>> Valerie Peng has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Removed the private no-arg default constructor and minor javadoc update.
>
> src/java.base/share/classes/javax/crypto/spec/OAEPParameterSpec.java line 126:
>
>> 124: * {@link #getMGFParameters()}
>> 125: * @param pSrc the source of the encoding input P
>> 126: * @exception NullPointerException if {@code mdName},
>
> Change `@exception` to the more accepted `@throws` tag.
Yes, will change.
-
PR: https://git.openjdk.java.net/jdk/pull/8191
Re: RFR: 8284553: Deprecate the DEFAULT static field of OAEPParameterSpec [v3]
On Tue, 19 Apr 2022 19:52:34 GMT, Sean Mullan wrote: >> Valerie Peng has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Removed the private no-arg default constructor and minor javadoc update. > > src/java.base/share/classes/javax/crypto/spec/OAEPParameterSpec.java line 99: > >> 97: * @deprecated This field uses the default values defined in the >> PKCS #1 >> 98: * standard. Some of these defaults are no longer >> recommended due >> 99: * to advances in cryptanalysis -- see the > > Remove "the". Ok~ - PR: https://git.openjdk.java.net/jdk/pull/8191
Re: RFR: 8284855: Update needed to Cleaners added to jdk.crypto.cryptoki [v7]
On Tue, 19 Apr 2022 14:00:06 GMT, Xue-Lei Andrew Fan wrote:
>> This is an effort to fix a problem introduced in the fix for
>> [JDK-8284368](https://bugs.openjdk.java.net/browse/JDK-8284368), which
>> replaced the finalizers in jdk.crypto.cryptoki with Cleaners. However,
>> there is a problem with the code changes. The Runnables registered with
>> Cleaner refer to the object being registered ('this'). Meaning, the Cleaner
>> mechanism will keep the objects reachable, preventing them from being
>> cleaned and collected.
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> remove duplicated checking
Marked as reviewed by valeriep (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/8248
Re: RFR: 8209038: Clarify the javadoc of Cipher.getParameters() [v2]
On Tue, 19 Apr 2022 02:20:10 GMT, Valerie Peng wrote:
>> src/java.base/share/classes/javax/crypto/Cipher.java line 1054:
>>
>>> 1052: * this cipher, or may contain additional default or random
>>> parameter
>>> 1053: * values used by the underlying cipher implementation if this
>>> cipher can
>>> 1054: * successfully generate the required parameter values when not
>>> supplied.
>>
>> A few comments:
>>
>> 1. I don't think this new text covers the case where the cipher is not
>> initialized with any parameters, but the provider returns parameters. I
>> tried to think of how to say that all in one sentence, but I think it is
>> best to have two sentences covering each case, ex:
>>
>> "If this cipher was not initialized with parameters, the returned parameters
>> may be null or may be ..."
>> "If this cipher was initialized with parameters, the returned parameters may
>> be the same or may be ..."
>>
>> 2. Should `Signature.getParameters` be updated to be consistent? Are there
>> any cases, or could there be, where a signature provider may return
>> additional parameters other than what the user specified?
>
> Hmm, I tried the suggested approach in (1), the result looks very lengthy.
> Actually, the Cipher.init(..) methods already has a few paragraphs describing
> the behavior for parameter generation, perhaps we should not repeat stating
> the "If this cipher was initialized" vs "was not initialized" since lengthy
> description may confuse users and have higher risks of inconsistency. What do
> you think?
> As for (2), currently only RSASSA-PSS signature impl uses parameters. When
> specified using PSSParameterSpec, it's possible that some of the parameters
> are not set, so it's possible for providers to use provider-specific default
> values for PSS case. So, yes, Signature class may have to updated in the same
> way to cover this particular scenario.
For (1), how about something like below:
> * The returned parameters may be the same that were used to initialize
> * this cipher, or may contain additional default or random parameter
> * values used by the underlying cipher implementation. If no parameters
> * is supplied and this cipher successfully generated the required
> * parameter values, it will be returned. Otherwise, {@code null} is
> * returned.
-
PR: https://git.openjdk.java.net/jdk/pull/8117
Re: RFR: 8209038: Clarify the javadoc of Cipher.getParameters() [v2]
On Fri, 15 Apr 2022 17:30:38 GMT, Sean Mullan wrote: >> Valerie Peng has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Update w/ review feedbacks > > src/java.base/share/classes/javax/crypto/Cipher.java line 1054: > >> 1052: * this cipher, or may contain additional default or random >> parameter >> 1053: * values used by the underlying cipher implementation if this >> cipher can >> 1054: * successfully generate the required parameter values when not >> supplied. > > A few comments: > > 1. I don't think this new text covers the case where the cipher is not > initialized with any parameters, but the provider returns parameters. I tried > to think of how to say that all in one sentence, but I think it is best to > have two sentences covering each case, ex: > > "If this cipher was not initialized with parameters, the returned parameters > may be null or may be ..." > "If this cipher was initialized with parameters, the returned parameters may > be the same or may be ..." > > 2. Should `Signature.getParameters` be updated to be consistent? Are there > any cases, or could there be, where a signature provider may return > additional parameters other than what the user specified? Hmm, I tried the suggested approach in (1), the result looks very lengthy. Actually, the Cipher.init(..) methods already has a few paragraphs describing the behavior for parameter generation, perhaps we should not repeat stating the "If this cipher was initialized" vs "was not initialized" since lengthy description may confuse users and have higher risks of inconsistency. What do you think? As for (2), currently only RSASSA-PSS signature impl uses parameters. When specified using PSSParameterSpec, it's possible that some of the parameters are not set, so it's possible for providers to use provider-specific default values for PSS case. So, yes, Signature class may have to updated in the same way to cover this particular scenario. - PR: https://git.openjdk.java.net/jdk/pull/8117
Re: RFR: 8284855: Update needed to Cleaners added to jdk.crypto.cryptoki [v6]
On Sat, 16 Apr 2022 05:35:20 GMT, Xue-Lei Andrew Fan wrote:
>> This is an effort to fix a problem introduced in the fix for
>> [JDK-8284368](https://bugs.openjdk.java.net/browse/JDK-8284368), which
>> replaced the finalizers in jdk.crypto.cryptoki with Cleaners. However,
>> there is a problem with the code changes. The Runnables registered with
>> Cleaner refer to the object being registered ('this'). Meaning, the Cleaner
>> mechanism will keep the objects reachable, preventing them from being
>> cleaned and collected.
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Update signatures for native code
src/jdk.crypto.cryptoki/unix/native/libj2pkcs11/p11_md.c line 274:
> 272: ModuleData *moduleData = jlong_to_ptr(ckpNativeData);
> 273:
> 274: if (moduleData != NULL && moduleData->hModule != NULL) {
The moduleData != NULL check seems to duplicate with the line 271? Otherwise
looks fine.
-
PR: https://git.openjdk.java.net/jdk/pull/8248
Re: RFR: 8284553: Deprecate the DEFAULT static field of OAEPParameterSpec [v3]
> This trivial change is to deprecate the DEFAULT static field of > OAEPParameterSpec class. Wordings are mostly the same as the previous > PSSParameterSpec deprecation change. Rest are just minor code re-factoring. > > The CSR will be filed once review is somewhat finished. > > Thanks, > Valerie Valerie Peng has updated the pull request incrementally with one additional commit since the last revision: Removed the private no-arg default constructor and minor javadoc update. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8191/files - new: https://git.openjdk.java.net/jdk/pull/8191/files/ba572eed..a760fc51 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8191=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8191=01-02 Stats: 12 lines in 2 files changed: 0 ins; 11 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8191.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8191/head:pull/8191 PR: https://git.openjdk.java.net/jdk/pull/8191
Re: RFR: 8284933: Improve debug in jdk.crypto.cryptoki
On Sun, 17 Apr 2022 14:45:49 GMT, Xue-Lei Andrew Fan wrote: > Hi, > > May I have the simple update reviewed? > > In the jdk.crypto.cryptoki module implementation, some of the debug > information could be calculated even if the debug is not enabled, which is > not resource friendly. > > Thanks, > Xuelei With this change, I see no reason to have debug(...) method. May as well just remove it and simply do a "System.out.println(...);"? - PR: https://git.openjdk.java.net/jdk/pull/8277
Re: RFR: 8284553: Deprecate the DEFAULT static field of OAEPParameterSpec [v2]
On Wed, 13 Apr 2022 19:27:00 GMT, Valerie Peng wrote: >> This trivial change is to deprecate the DEFAULT static field of >> OAEPParameterSpec class. Wordings are mostly the same as the previous >> PSSParameterSpec deprecation change. Rest are just minor code re-factoring. >> >> The CSR will be filed once review is somewhat finished. >> >> Thanks, >> Valerie > > Valerie Peng has updated the pull request incrementally with one additional > commit since the last revision: > > Update w/ review feedback. CSR filed at: https://bugs.openjdk.java.net/browse/JDK-8284919 - PR: https://git.openjdk.java.net/jdk/pull/8191
Re: RFR: 8284855: Update needed to Cleaners added to jdk.crypto.cryptoki [v5]
On Fri, 15 Apr 2022 15:50:19 GMT, Xue-Lei Andrew Fan wrote:
>> This is an effort to fix a problem introduced in the fix for
>> [JDK-8284368](https://bugs.openjdk.java.net/browse/JDK-8284368), which
>> replaced the finalizers in jdk.crypto.cryptoki with Cleaners. However,
>> there is a problem with the code changes. The Runnables registered with
>> Cleaner refer to the object being registered ('this'). Meaning, the Cleaner
>> mechanism will keep the objects reachable, preventing them from being
>> cleaned and collected.
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> add a reference to the clean up method
src/jdk.crypto.cryptoki/unix/native/libj2pkcs11/p11_md.c line 274:
> 272: ModuleData *moduleData = jlong_to_ptr(ckpNativeData);
> 273:
> 274: if (moduleData != NULL) {
The check should be (moduleData->hModule != NULL)?
-
PR: https://git.openjdk.java.net/jdk/pull/8248
Re: RFR: 8284855: Update needed to Cleaners added to jdk.crypto.cryptoki [v5]
On Fri, 15 Apr 2022 15:50:19 GMT, Xue-Lei Andrew Fan wrote:
>> This is an effort to fix a problem introduced in the fix for
>> [JDK-8284368](https://bugs.openjdk.java.net/browse/JDK-8284368), which
>> replaced the finalizers in jdk.crypto.cryptoki with Cleaners. However,
>> there is a problem with the code changes. The Runnables registered with
>> Cleaner refer to the object being registered ('this'). Meaning, the Cleaner
>> mechanism will keep the objects reachable, preventing them from being
>> cleaned and collected.
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> add a reference to the clean up method
src/jdk.crypto.cryptoki/unix/native/libj2pkcs11/p11_md.c line 265:
> 263: * Class: sun_security_pkcs11_wrapper_PKCS11
> 264: * Method:disconnect
> 265: * Signature: ()V
nit: update the JNI method signature? Same for all other p11_md.c for different
OS...
-
PR: https://git.openjdk.java.net/jdk/pull/8248
Re: RFR: 8209038: Clarify the javadoc of Cipher.getParameters() [v2]
On Wed, 13 Apr 2022 22:04:03 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 incrementally with one additional
> commit since the last revision:
>
> Update w/ review feedbacks
CSR is filed at: https://bugs.openjdk.java.net/browse/JDK-8284897
Please review, thanks!
-
PR: https://git.openjdk.java.net/jdk/pull/8117
Re: RFR: 8284855: Update needed to Cleaners added to jdk.crypto.cryptoki
On Thu, 14 Apr 2022 18:06:10 GMT, Xue-Lei Andrew Fan wrote:
> This is an effort to fix a problem introduced in the fix for
> [JDK-8284368](https://bugs.openjdk.java.net/browse/JDK-8284368), which
> replaced the finalizers in jdk.crypto.cryptoki with Cleaners. However, there
> is a problem with the code changes. The Runnables registered with Cleaner
> refer to the object being registered ('this'). Meaning, the Cleaner mechanism
> will keep the objects reachable, preventing them from being cleaned and
> collected.
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/.PKCS11.java.swp
file should not be there?
-
PR: https://git.openjdk.java.net/jdk/pull/8248
Re: RFR: 8209038: Clarify the javadoc of Cipher.getParameters() [v2]
> 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 incrementally with one additional
commit since the last revision:
Update w/ review feedbacks
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/8117/files
- new: https://git.openjdk.java.net/jdk/pull/8117/files/b0463fba..9b8b71a6
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=8117=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk=8117=00-01
Stats: 19 lines in 2 files changed: 0 ins; 0 del; 19 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
