RFR: 8285785: CheckCleanerBound test fails with PasswordCallback object is not released
Hi, May I have this test update reviewed? The javax/security/auth/callback/PasswordCallback/CheckCleanerBound.java test case failed on one of the test setups. The test runs gc in a loop and expects the GC to have garbage collected contents of a WeakHashMap. The loop runs for 10 iterations. Some delay needs to be added between each iteration to increase the chances of GC garbage collecting the instances. Thanks, Xuelei - Commit messages: - 8285785: CheckCleanerBound test fails with PasswordCallback object is not released Changes: https://git.openjdk.java.net/jdk/pull/8443/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8443&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285785 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8443.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8443/head:pull/8443 PR: https://git.openjdk.java.net/jdk/pull/8443
Re: RFR: 8255552: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms [v2]
On Wed, 27 Apr 2022 21:04:59 GMT, Weijun Wang wrote: >> Changes requested by mullan (Reviewer). > > @seanjmullan Since we use symmetric keys to encrypt entries and add integrity > check, should this enhancement cover them as well? For example, if a PKCS12 > keystore is created with `-J-Dkeystore.pkcs12.legacy=true`, should the > algorithms used be warned? BTW, in legacy mode, we use PBEWithSHA1AndRC2_40 > when encrypting keys. Should the security property include "RC2" as well? > > Not sure if it's doable, because those are PKCS12-specific codes. `keytool` > is not able to see them. @wangweij This is an interesting question that you raised. From keytool perspective, this security property `keystore.pkcs12.legacy` is implemented in underlying `PKCS12 KeyStore` as you pointed out. It’s not clear to me the need to add RC2 to the security property. Regarding PBEWithSHA1AndRC2_40 algorithm, the algorithm constraint checking will always flag “SHA1” as a weak algorithm prior to RC2 after decomposing this algorithm. And RC2 is not supported by the PKCS12 KeyStore already. - PR: https://git.openjdk.java.net/jdk/pull/8300
Re: RFR: 8255552: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms [v2]
On Wed, 27 Apr 2022 19:35:04 GMT, Sean Mullan wrote: >> Hai-May Chao has updated the pull request incrementally with one additional >> commit since the last revision: >> >> SecretKeyConstraintsParameters subclass created and property description >> updated > > Changes requested by mullan (Reviewer). @seanjmullan Also, fixed the problem that you brought up for `-genseckey -keyalg RC2` in a PKCS12 keystore. Fix is to throw NoSuchAlgorithmException when a key size is not specified. - PR: https://git.openjdk.java.net/jdk/pull/8300
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v3]
On Thu, 28 Apr 2022 01:34:19 GMT, Joe Darcy wrote: >> To enable more complete doclint checking (courtesy @jonathan-gibbons), >> please review this PR to add type-level @param tags where they are missing. >> >> To the maintainers of java.util.concurrent, those changes could be separated >> out in another bug if that would ease maintenance of that code. >> >> Making these library fixes is a blocker for correcting and expanding the >> doclint checks (JDK-8285496). >> >> I'll update copyright years before pushing. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to more review feedback. Thanks for the updates Joe. Your new wording looks good to me. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8410
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v3]
On Thu, 28 Apr 2022 01:34:19 GMT, Joe Darcy wrote: >> To enable more complete doclint checking (courtesy @jonathan-gibbons), >> please review this PR to add type-level @param tags where they are missing. >> >> To the maintainers of java.util.concurrent, those changes could be separated >> out in another bug if that would ease maintenance of that code. >> >> Making these library fixes is a blocker for correcting and expanding the >> doclint checks (JDK-8285496). >> >> I'll update copyright years before pushing. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to more review feedback. src/java.base/share/classes/java/nio/file/SecureDirectoryStream.java line 55: > 53: * against the original path of the directory (irrespective of if > the > 54: * directory is moved since it was opened). > 55: * @param the type of path It may not be a path. The type parameter is specified in the super interfaces, can you copy that down instead? src/java.base/share/classes/java/nio/file/WatchEvent.java line 51: > 49: /** > 50: * An event kind, for the purposes of identification. > 51: * @param the type of the context value This is okay but the it differs slightly to the type parameters specified further up. I think the issue here is that it was just wasn't copied down to WatchEvent.Kind. - PR: https://git.openjdk.java.net/jdk/pull/8410
Re: RFR: 8285785: CheckCleanerBound test fails with PasswordCallback object is not released
On Thu, 28 Apr 2022 07:01:25 GMT, Xue-Lei Andrew Fan wrote: > Hi, > > May I have this test update reviewed? > > The javax/security/auth/callback/PasswordCallback/CheckCleanerBound.java test > case failed on one of the test setups. The test runs gc in a loop and > expects the GC to have garbage collected contents of a WeakHashMap. The loop > runs for 10 iterations. Some delay needs to be added between each iteration > to increase the chances of GC garbage collecting the instances. > > Thanks, > Xuelei LGTM. It's a bit strange to use a weak hash map for that though. It could be simpler to use `WeakReference` and `ReferenceQueue`. Marked as reviewed by dfuchs (Reviewer). - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8443
Re: RFR: 8285785: CheckCleanerBound test fails with PasswordCallback object is not released
On Thu, 28 Apr 2022 07:01:25 GMT, Xue-Lei Andrew Fan wrote:
> Hi,
>
> May I have this test update reviewed?
>
> The javax/security/auth/callback/PasswordCallback/CheckCleanerBound.java test
> case failed on one of the test setups. The test runs gc in a loop and
> expects the GC to have garbage collected contents of a WeakHashMap. The loop
> runs for 10 iterations. Some delay needs to be added between each iteration
> to increase the chances of GC garbage collecting the instances.
>
> Thanks,
> Xuelei
Marked as reviewed by mullan (Reviewer).
test/jdk/javax/security/auth/callback/PasswordCallback/CheckCleanerBound.java
line 50:
> 48: for (int i = 0; i < 10 && weakHashMap.size() != 0; i++) {
> 49: System.gc();
> 50: Thread.sleep(100);
Looks ok to me although @RogerRiggs commented in the previous PR that 10ms
should be sufficient.
-
PR: https://git.openjdk.java.net/jdk/pull/8443
Re: RFR: 8285785: CheckCleanerBound test fails with PasswordCallback object is not released
On Thu, 28 Apr 2022 07:01:25 GMT, Xue-Lei Andrew Fan wrote: > Hi, > > May I have this test update reviewed? > > The javax/security/auth/callback/PasswordCallback/CheckCleanerBound.java test > case failed on one of the test setups. The test runs gc in a loop and > expects the GC to have garbage collected contents of a WeakHashMap. The loop > runs for 10 iterations. Some delay needs to be added between each iteration > to increase the chances of GC garbage collecting the instances. > > Thanks, > Xuelei Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8443
Re: RFR: 8285785: CheckCleanerBound test fails with PasswordCallback object is not released
On Thu, 28 Apr 2022 12:19:35 GMT, Sean Mullan wrote: >> Hi, >> >> May I have this test update reviewed? >> >> The javax/security/auth/callback/PasswordCallback/CheckCleanerBound.java >> test case failed on one of the test setups. The test runs gc in a loop and >> expects the GC to have garbage collected contents of a WeakHashMap. The loop >> runs for 10 iterations. Some delay needs to be added between each iteration >> to increase the chances of GC garbage collecting the instances. >> >> Thanks, >> Xuelei > > test/jdk/javax/security/auth/callback/PasswordCallback/CheckCleanerBound.java > line 50: > >> (failed to retrieve contents of file, check the PR for context) > Looks ok to me although @RogerRiggs commented in the previous PR that 10ms > should be sufficient. Alternatively, the loop count could be raised by 10x. That would keep the typical running time low and still allow for a worst case. Your choice. - PR: https://git.openjdk.java.net/jdk/pull/8443
Re: RFR: 8255552: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms [v2]
On Thu, 28 Apr 2022 06:46:35 GMT, Hai-May Chao wrote:
>> Please review these changes to add DES/3DES/MD5 to
>> `jdk.security.legacyAlgorithms` security property, and to add the legacy
>> algorithm constraint checking to `keytool` commands that are associated with
>> secret key entries stored in the keystore. These `keytool` commands are
>> -genseckey, -importpass, -list, and -importkeystore. As a result, `keytool`
>> will be able to generate warnings when it detects that the secret key based
>> algorithms and PBE based Mac and cipher algorithms are weak. Also removes
>> the "This algorithm will be disabled in a future update.” from the existing
>> warnings for the asymmetric keys/certificates.
>> Will also file a CSR.
>
> Hai-May Chao has updated the pull request incrementally with one additional
> commit since the last revision:
>
> SecretKeyConstraintsParameters subclass created and property description
> updated
A few more comments.
src/java.base/share/classes/sun/security/tools/keytool/Main.java line 1862:
> 1860: keysize = 168;
> 1861: } else if ("RC2".equalsIgnoreCase(keyAlgName)) {
> 1862: keysize = 128;
There are other algorithms that are also weak such as RC4, and maybe more in
the future. Rather than special-casing each of them, I think instead you should
call `keygen.init()` for DES and DESede only where you know that the keysize is
always fixed.
src/java.base/share/conf/security/java.security line 641:
> 639:
> 640: #
> 641: # Legacy cryptographic algorithms and key lengths
Nit: add period at end of sentence.
test/jdk/sun/security/tools/keytool/ReadJar.java line 26:
> 24: /**
> 25: * @test
> 26: * @bug 6890872 8168882 8257722 822
Here we are just updating the warning message, so I don't think the bugid needs
to be added.
test/jdk/sun/security/tools/keytool/ReadJar.java line 162:
> 160: .shouldContain("Certificate #2:")
> 161: .shouldContain("Signer #2:")
> 162: .shouldNotMatch("The certificate #.* of signer #.*" +
> "uses the SHA1withRSA.*will be disabled")
You probably don't need to check for a non-occurrence here since the message
has been changed and can no longer occur. I also think it doesn't need to list
the bugid because it is not testing the main fix which is warnings on symmetric
key algs.
-
Changes requested by mullan (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8300
Re: RFR: 8255552: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms [v2]
On Wed, 27 Apr 2022 19:35:04 GMT, Sean Mullan wrote: >> Hai-May Chao has updated the pull request incrementally with one additional >> commit since the last revision: >> >> SecretKeyConstraintsParameters subclass created and property description >> updated > > Changes requested by mullan (Reviewer). > @seanjmullan Since we use symmetric keys to encrypt entries and add integrity > check, should this enhancement cover them as well? For example, if a PKCS12 > keystore is created with `-J-Dkeystore.pkcs12.legacy=true`, should the > algorithms used be warned? BTW, in legacy mode, we use PBEWithSHA1AndRC2_40 > when encrypting keys. Should the security property include "RC2" as well? > > Not sure if it's doable, because those are PKCS12-specific codes. `keytool` > is not able to see them. Right, I think this would require knowledge of what keystore type is being used and parsing the PKCS12 encoded bytes which seems beyond the scope of this RFE. Also, those algorithms are disabled by default, so in some sense the user is making a decision to use them by enabling the system property and therefore are taking the risk themselves. - PR: https://git.openjdk.java.net/jdk/pull/8300
RFR: 8285827: Describe the keystore.pkcs12.legacy system property in the java.security file
We added a new system property back in https://bugs.openjdk.java.net/browse/JDK-8153005 but it's better to describe it in the `java.security` file as well. Please review the text. I especially added the last sentence so that people won't set `-Dkeystore.pkcs12.legacy=false`. - Commit messages: - add description Changes: https://git.openjdk.java.net/jdk/pull/8452/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8452&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285827 Stats: 13 lines in 1 file changed: 13 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8452.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8452/head:pull/8452 PR: https://git.openjdk.java.net/jdk/pull/8452
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]
On Wed, 27 Apr 2022 20:22:42 GMT, Mark Powers wrote: >> JDK-6725221 is about obtaining boolean properties, so not an exact match. >> The suggested change is so easy, I'm going to do it. > > sun.security.action.GetPropertyAction::privilegedGetProperty doesn't trim the > return value. Could this be a problem? Answer is no. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 02:33:49 GMT, Mark Powers wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8285504
>>
>> JDK-8273046 is the umbrella bug for this bug. The changes were too large for
>> a single code review, so it was decided to split into smaller chunks. This
>> is one such chunk:
>>
>> open/src/java.base/share/classes/java/net
>
> Mark Powers 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 eight additional
> commits since the last revision:
>
> - Merge
> - Max and Brad comments
> - jaikiran comments
> - Alan Bateman comments
> - second iteration
> - Merge
> - Merge
> - first iteration
I made some wrong suggestions earlier.
src/java.base/share/classes/javax/net/ssl/KeyManagerFactory.java line 70:
> 68: String type;
> 69: type = GetPropertyAction.privilegedGetProperty(
> 70: "ssl.KeyManagerFactory.algorithm");
So sorry I got it wrong here, this is a security property.
`GetPropertyAction.privilegedGetProperty` is for system properties.
src/java.base/share/classes/javax/net/ssl/SSLSocketFactory.java line 92:
> 90: static String getSecurityProperty(final String name) {
> 91: return AccessController.doPrivileged((PrivilegedAction)
> () -> {
> 92: return Security.getProperty(name);
I assume we still need to do the if-empty-then-null conversion?
src/java.base/share/classes/javax/net/ssl/TrustManagerFactory.java line 82:
> 80: String type;
> 81: type = GetPropertyAction.privilegedGetProperty(
> 82: "ssl.TrustManagerFactory.algorithm");
Sorry I got it wrong here, this is a security property.
-
PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: 8285785: CheckCleanerBound test fails with PasswordCallback object is not released
On Thu, 28 Apr 2022 13:34:04 GMT, Roger Riggs wrote: >> Hi, >> >> May I have this test update reviewed? >> >> The javax/security/auth/callback/PasswordCallback/CheckCleanerBound.java >> test case failed on one of the test setups. The test runs gc in a loop and >> expects the GC to have garbage collected contents of a WeakHashMap. The loop >> runs for 10 iterations. Some delay needs to be added between each iteration >> to increase the chances of GC garbage collecting the instances. >> >> Thanks, >> Xuelei > > Marked as reviewed by rriggs (Reviewer). I will check the proposal suggested by @RogerRiggs and @dfuch. As the test failure could be annoying, I would like to integrate this update first. - PR: https://git.openjdk.java.net/jdk/pull/8443
Integrated: 8285785: CheckCleanerBound test fails with PasswordCallback object is not released
On Thu, 28 Apr 2022 07:01:25 GMT, Xue-Lei Andrew Fan wrote: > Hi, > > May I have this test update reviewed? > > The javax/security/auth/callback/PasswordCallback/CheckCleanerBound.java test > case failed on one of the test setups. The test runs gc in a loop and > expects the GC to have garbage collected contents of a WeakHashMap. The loop > runs for 10 iterations. Some delay needs to be added between each iteration > to increase the chances of GC garbage collecting the instances. > > Thanks, > Xuelei This pull request has now been integrated. Changeset: b9d1e851 Author:Xue-Lei Andrew Fan URL: https://git.openjdk.java.net/jdk/commit/b9d1e85151d9d4016639e6298c90737db10f6072 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod 8285785: CheckCleanerBound test fails with PasswordCallback object is not released Reviewed-by: dfuchs, mullan, rriggs - PR: https://git.openjdk.java.net/jdk/pull/8443
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 15:47:44 GMT, Weijun Wang wrote: >> Mark Powers 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 eight additional >> commits since the last revision: >> >> - Merge >> - Max and Brad comments >> - jaikiran comments >> - Alan Bateman comments >> - second iteration >> - Merge >> - Merge >> - first iteration > > src/java.base/share/classes/javax/net/ssl/KeyManagerFactory.java line 70: > >> 68: String type; >> 69: type = GetPropertyAction.privilegedGetProperty( >> 70: "ssl.KeyManagerFactory.algorithm"); > > So sorry I got it wrong here, this is a security property. > `GetPropertyAction.privilegedGetProperty` is for system properties. I just noticed the same. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 02:33:49 GMT, Mark Powers wrote: >> https://bugs.openjdk.java.net/browse/JDK-8285504 >> >> JDK-8273046 is the umbrella bug for this bug. The changes were too large for >> a single code review, so it was decided to split into smaller chunks. This >> is one such chunk: >> >> open/src/java.base/share/classes/java/net > > Mark Powers 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 eight additional > commits since the last revision: > > - Merge > - Max and Brad comments > - jaikiran comments > - Alan Bateman comments > - second iteration > - Merge > - Merge > - first iteration These need to be addressed before integration. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 16:14:01 GMT, Bradford Wetmore wrote: >> src/java.base/share/classes/javax/net/ssl/KeyManagerFactory.java line 70: >> >>> 68: String type; >>> 69: type = GetPropertyAction.privilegedGetProperty( >>> 70: "ssl.KeyManagerFactory.algorithm"); >> >> So sorry I got it wrong here, this is a security property. >> `GetPropertyAction.privilegedGetProperty` is for system properties. > > I just noticed the same. Interesting that mach5 tier1 and tier2 tests passed. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 15:45:58 GMT, Weijun Wang wrote:
>> Mark Powers 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 eight additional
>> commits since the last revision:
>>
>> - Merge
>> - Max and Brad comments
>> - jaikiran comments
>> - Alan Bateman comments
>> - second iteration
>> - Merge
>> - Merge
>> - first iteration
>
> src/java.base/share/classes/javax/net/ssl/SSLSocketFactory.java line 92:
>
>> 90: static String getSecurityProperty(final String name) {
>> 91: return AccessController.doPrivileged((PrivilegedAction)
>> () -> {
>> 92: return Security.getProperty(name);
>
> I assume we still need to do the if-empty-then-null conversion?
Just found the same. This needs to be reverted. You can set a Security
Property to an "empty" string which won't work here. Suggest you revert to
previous code, possibly using a lambda if that was the original intent.
> src/java.base/share/classes/javax/net/ssl/TrustManagerFactory.java line 82:
>
>> 80: String type;
>> 81: type = GetPropertyAction.privilegedGetProperty(
>> 82: "ssl.TrustManagerFactory.algorithm");
>
> Sorry I got it wrong here, this is a security property.
Similar comment.
-
PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 16:22:43 GMT, Bradford Wetmore wrote:
>> src/java.base/share/classes/javax/net/ssl/SSLSocketFactory.java line 92:
>>
>>> 90: static String getSecurityProperty(final String name) {
>>> 91: return AccessController.doPrivileged((PrivilegedAction)
>>> () -> {
>>> 92: return Security.getProperty(name);
>>
>> I assume we still need to do the if-empty-then-null conversion?
>
> Just found the same. This needs to be reverted. You can set a Security
> Property to an "empty" string which won't work here. Suggest you revert to
> previous code, possibly using a lambda if that was the original intent.
`Security.getProperty()` does not specify the value will be `trim()`.
-
PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 16:27:08 GMT, Bradford Wetmore wrote: >> Just found the same. This needs to be reverted. You can set a Security >> Property to an "empty" string which won't work here. Suggest you revert to >> previous code, possibly using a lambda if that was the original intent. > > `Security.getProperty()` does not specify the value will be `trim()`. My mistake. It's only the trim that you wanted removed, line 94. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v3]
On Thu, 28 Apr 2022 08:08:37 GMT, Alan Bateman wrote: >> Joe Darcy has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Respond to more review feedback. > > src/java.base/share/classes/java/nio/file/SecureDirectoryStream.java line 55: > >> 53: * against the original path of the directory (irrespective of if >> the >> 54: * directory is moved since it was opened). >> 55: * @param the type of path > > It may not be a path. The type parameter is specified in the super > interfaces, can you copy that down instead? Will change in the next push. - PR: https://git.openjdk.java.net/jdk/pull/8410
Re: RFR: 8255552: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms [v2]
On Thu, 28 Apr 2022 06:46:35 GMT, Hai-May Chao wrote: >> Please review these changes to add DES/3DES/MD5 to >> `jdk.security.legacyAlgorithms` security property, and to add the legacy >> algorithm constraint checking to `keytool` commands that are associated with >> secret key entries stored in the keystore. These `keytool` commands are >> -genseckey, -importpass, -list, and -importkeystore. As a result, `keytool` >> will be able to generate warnings when it detects that the secret key based >> algorithms and PBE based Mac and cipher algorithms are weak. Also removes >> the "This algorithm will be disabled in a future update.” from the existing >> warnings for the asymmetric keys/certificates. >> Will also file a CSR. > > Hai-May Chao has updated the pull request incrementally with one additional > commit since the last revision: > > SecretKeyConstraintsParameters subclass created and property description > updated There is no way to `-genseckey` an RC2 key in a PKCS12 keystore but the only reason is that we don't have a known RC2 OID registered. (In fact, I was preparing to add one in the attempted code change to add OIDs into the standard names doc). You can add an RC4 key to PKCS12. Also, you can add both RC2 and RC4 to a JCEKS keystore. - PR: https://git.openjdk.java.net/jdk/pull/8300
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v4]
> To enable more complete doclint checking (courtesy @jonathan-gibbons), please > review this PR to add type-level @param tags where they are missing. > > To the maintainers of java.util.concurrent, those changes could be separated > out in another bug if that would ease maintenance of that code. > > Making these library fixes is a blocker for correcting and expanding the > doclint checks (JDK-8285496). > > I'll update copyright years before pushing. Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Respond to more review feedback. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8410/files - new: https://git.openjdk.java.net/jdk/pull/8410/files/db4919a9..aaa8f828 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8410&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8410&range=02-03 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8410.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8410/head:pull/8410 PR: https://git.openjdk.java.net/jdk/pull/8410
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v3]
On Thu, 28 Apr 2022 08:10:38 GMT, Alan Bateman wrote: >> Joe Darcy has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Respond to more review feedback. > > src/java.base/share/classes/java/nio/file/WatchEvent.java line 51: > >> 49: /** >> 50: * An event kind, for the purposes of identification. >> 51: * @param the type of the context value > > This is okay but the it differs slightly to the type parameters specified > further up. I think the issue here is that it was just wasn't copied down to > WatchEvent.Kind. Okay -- I'll for the T type parameter of the Kind interface I'll reuse the wording of the T type parameter of the enclosing WatchEvent interface. (The type variable on Kind could be renamed to show that the two type parameters are distinct.) - PR: https://git.openjdk.java.net/jdk/pull/8410
Re: RFR: 8209038: Clarify the javadoc of Cipher.getParameters() [v2]
On Wed, 27 Apr 2022 20:01:26 GMT, Sean Mullan wrote: >> 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.) > > Sorry, I should have been more specific. JDK-8209038 references JDK-8206171 > which I think was filed by the TCK team. In that bug description, it says: > >> This bug is filed for clarification of specification (see comment) >> Please clarify the specification to include a possible exception being >> thrown (ProviderException for RSASSA-PSS) or other possible exceptions for >> future Signature algorithms that require mandatory parameters by the user >> before any operations could be performed, and user did not set any >> parameters before using the Signature operations (sign, update, verify). >> Or >> null could be returned (as per specification) > > I assumed the `ProviderException` case could potentially apply to a `Cipher` > algorithm as well. You can ignore my last comment. I had not realized that the fix for JDK-8209038 was to make `engineGetParameters` return `null` instead of throwing `ProviderException` when RSASSA-PSS params are not specified. - PR: https://git.openjdk.java.net/jdk/pull/8117
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v4]
On Thu, 28 Apr 2022 16:58:40 GMT, Joe Darcy wrote: >> To enable more complete doclint checking (courtesy @jonathan-gibbons), >> please review this PR to add type-level @param tags where they are missing. >> >> To the maintainers of java.util.concurrent, those changes could be separated >> out in another bug if that would ease maintenance of that code. >> >> Making these library fixes is a blocker for correcting and expanding the >> doclint checks (JDK-8285496). >> >> I'll update copyright years before pushing. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to more review feedback. Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8410
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 16:37:35 GMT, Mark Powers wrote: >> `Security.getProperty()` does not specify the value will be `trim()`. > > My mistake. It's only the trim that you wanted removed, line 94. No, the API for Security.getProperty doesn't specify trimming, so suggest leaving the trim() part also. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v4]
On Thu, 28 Apr 2022 16:58:40 GMT, Joe Darcy wrote: >> To enable more complete doclint checking (courtesy @jonathan-gibbons), >> please review this PR to add type-level @param tags where they are missing. >> >> To the maintainers of java.util.concurrent, those changes could be separated >> out in another bug if that would ease maintenance of that code. >> >> Making these library fixes is a blocker for correcting and expanding the >> doclint checks (JDK-8285496). >> >> I'll update copyright years before pushing. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to more review feedback. Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8410
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v12]
On Thu, 28 Apr 2022 04:34:36 GMT, Xue-Lei Andrew Fan wrote: >> Please review the update to remove finalizer method in the >> java.security.jgss module. It is one of the efforts to clean up the use of >> finalizer method in JDK. > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > change to use othervm I see you removed the `Thread.sleep(100)` calls. Given the failure of another similar test, maybe it's safer to add them back? - PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 17:29:53 GMT, Bradford Wetmore wrote: >> My mistake. It's only the trim that you wanted removed, line 94. > > No, the API for Security.getProperty doesn't specify trimming, so suggest > leaving the trim() part also. Okay. Line 94 is back. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v5]
> To enable more complete doclint checking (courtesy @jonathan-gibbons), please > review this PR to add type-level @param tags where they are missing. > > To the maintainers of java.util.concurrent, those changes could be separated > out in another bug if that would ease maintenance of that code. > > Making these library fixes is a blocker for correcting and expanding the > doclint checks (JDK-8285496). > > I'll update copyright years before pushing. Joe Darcy 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 seven additional commits since the last revision: - Update copyright years. - Merge branch 'master' into JDK-8285676 - Respond to more review feedback. - Respond to more review feedback. - Respond to review feedback. - Merge branch 'master' into JDK-8285676 - JDK-8285676: Add missing @param tags for type parameters on classes and interfaces - Changes: - all: https://git.openjdk.java.net/jdk/pull/8410/files - new: https://git.openjdk.java.net/jdk/pull/8410/files/aaa8f828..cb1fe1c2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8410&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8410&range=03-04 Stats: 687 lines in 59 files changed: 610 ins; 8 del; 69 mod Patch: https://git.openjdk.java.net/jdk/pull/8410.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8410/head:pull/8410 PR: https://git.openjdk.java.net/jdk/pull/8410
Integrated: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces
On Tue, 26 Apr 2022 22:24:26 GMT, Joe Darcy wrote: > To enable more complete doclint checking (courtesy @jonathan-gibbons), please > review this PR to add type-level @param tags where they are missing. > > To the maintainers of java.util.concurrent, those changes could be separated > out in another bug if that would ease maintenance of that code. > > Making these library fixes is a blocker for correcting and expanding the > doclint checks (JDK-8285496). > > I'll update copyright years before pushing. This pull request has now been integrated. Changeset: bba456a8 Author:Joe Darcy URL: https://git.openjdk.java.net/jdk/commit/bba456a8dbf9027e4b015567c17a79fc7441aa08 Stats: 102 lines in 40 files changed: 76 ins; 0 del; 26 mod 8285676: Add missing @param tags for type parameters on classes and interfaces Reviewed-by: wetmore, smarks, dfuchs, prr, alanb, mchung - PR: https://git.openjdk.java.net/jdk/pull/8410
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v12]
On Thu, 28 Apr 2022 17:48:20 GMT, Weijun Wang wrote: > I see you removed the `Thread.sleep(100)` calls. Given the failure of another > similar test, maybe it's safer to add them back? Yes. I'm evaluating if other proposal works or not. Otherwise, I will add the sleep back. - PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8285785: CheckCleanerBound test fails with PasswordCallback object is not released
On Thu, 28 Apr 2022 07:01:25 GMT, Xue-Lei Andrew Fan wrote: > Hi, > > May I have this test update reviewed? > > The javax/security/auth/callback/PasswordCallback/CheckCleanerBound.java test > case failed on one of the test setups. The test runs gc in a loop and > expects the GC to have garbage collected contents of a WeakHashMap. The loop > runs for 10 iterations. Some delay needs to be added between each iteration > to increase the chances of GC garbage collecting the instances. > > Thanks, > Xuelei > Alternatively, the loop count could be raised by 10x. That would keep the > typical running time low and still allow for a worst case. Update: I tried to run the test 1000 times. I still can see failure if using 10x loop count, without sleep. - PR: https://git.openjdk.java.net/jdk/pull/8443
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v12]
On Thu, 28 Apr 2022 04:34:36 GMT, Xue-Lei Andrew Fan wrote: >> Please review the update to remove finalizer method in the >> java.security.jgss module. It is one of the efforts to clean up the use of >> finalizer method in JDK. > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > change to use othervm I'm sure you have well established testing environment as well, but if you need any help to test out your "other proposals", I'm glad to help. - PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v5]
On Thu, 28 Apr 2022 18:05:39 GMT, Joe Darcy wrote:
>> To enable more complete doclint checking (courtesy @jonathan-gibbons),
>> please review this PR to add type-level @param tags where they are missing.
>>
>> To the maintainers of java.util.concurrent, those changes could be separated
>> out in another bug if that would ease maintenance of that code.
>>
>> Making these library fixes is a blocker for correcting and expanding the
>> doclint checks (JDK-8285496).
>>
>> I'll update copyright years before pushing.
>
> Joe Darcy 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 seven additional commits since
> the last revision:
>
> - Update copyright years.
> - Merge branch 'master' into JDK-8285676
> - Respond to more review feedback.
> - Respond to more review feedback.
> - Respond to review feedback.
> - Merge branch 'master' into JDK-8285676
> - JDK-8285676: Add missing @param tags for type parameters on classes and
> interfaces
src/java.base/share/classes/java/lang/ref/PhantomReference.java line 48:
> 46: * The {@link #refersTo(Object) refersTo} method can be used to test
> 47: * whether some object is the referent of a phantom reference.
> 48: * @param the type of the referent
Shouldn't there be a space after `@param` ?
-
PR: https://git.openjdk.java.net/jdk/pull/8410
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v5]
> https://bugs.openjdk.java.net/browse/JDK-8285504 > > JDK-8273046 is the umbrella bug for this bug. The changes were too large for > a single code review, so it was decided to split into smaller chunks. This is > one such chunk: > > open/src/java.base/share/classes/java/net Mark Powers has updated the pull request incrementally with one additional commit since the last revision: Max and Brad comments round two - Changes: - all: https://git.openjdk.java.net/jdk/pull/8384/files - new: https://git.openjdk.java.net/jdk/pull/8384/files/6997837f..15a9f3b8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8384&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8384&range=03-04 Stats: 14 lines in 3 files changed: 7 ins; 2 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/8384.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8384/head:pull/8384 PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v13]
> Please review the update to remove finalizer method in the java.security.jgss > module. It is one of the efforts to clean up the use of finalizer method in > JDK. Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: add sleep back - Changes: - all: https://git.openjdk.java.net/jdk/pull/8136/files - new: https://git.openjdk.java.net/jdk/pull/8136/files/c0890841..d554c724 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8136&range=12 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8136&range=11-12 Stats: 2 lines in 2 files changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8136.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8136/head:pull/8136 PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v12]
On Thu, 28 Apr 2022 04:34:36 GMT, Xue-Lei Andrew Fan wrote: >> Please review the update to remove finalizer method in the >> java.security.jgss module. It is one of the efforts to clean up the use of >> finalizer method in JDK. > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > change to use othervm The Thread.sleep() was added back. Without the sleep, the test may fail intermittent. - PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v12]
On Thu, 28 Apr 2022 04:34:36 GMT, Xue-Lei Andrew Fan wrote: >> Please review the update to remove finalizer method in the >> java.security.jgss module. It is one of the efforts to clean up the use of >> finalizer method in JDK. > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > change to use othervm My understanding is such kind of tests that rely on GC is fundementally unreliable. I would preemptively add an intermittent keyword to it. :-) - PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v5]
On Thu, 28 Apr 2022 18:29:35 GMT, Mark Powers wrote: >> https://bugs.openjdk.java.net/browse/JDK-8285504 >> >> JDK-8273046 is the umbrella bug for this bug. The changes were too large for >> a single code review, so it was decided to split into smaller chunks. This >> is one such chunk: >> >> open/src/java.base/share/classes/java/net > > Mark Powers has updated the pull request incrementally with one additional > commit since the last revision: > > Max and Brad comments round two Final changes LGTM. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v5]
On Thu, 28 Apr 2022 18:24:33 GMT, Andrey Turbanov wrote:
>> Joe Darcy 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 seven additional commits
>> since the last revision:
>>
>> - Update copyright years.
>> - Merge branch 'master' into JDK-8285676
>> - Respond to more review feedback.
>> - Respond to more review feedback.
>> - Respond to review feedback.
>> - Merge branch 'master' into JDK-8285676
>> - JDK-8285676: Add missing @param tags for type parameters on classes and
>> interfaces
>
> src/java.base/share/classes/java/lang/ref/PhantomReference.java line 48:
>
>> 46: * The {@link #refersTo(Object) refersTo} method can be used to test
>> 47: * whether some object is the referent of a phantom reference.
>> 48: * @param the type of the referent
>
> Shouldn't there be a space after `@param` ?
Good catch. Sorry I missed it. This occurs in all `java/lang/ref` files.
-
PR: https://git.openjdk.java.net/jdk/pull/8410
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&pr=8117&range=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8117&range=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: 8209038: Clarify the javadoc of Cipher.getParameters() [v3]
On Thu, 28 Apr 2022 19:11:23 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 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"?
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.
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.
-
PR: https://git.openjdk.java.net/jdk/pull/8117
Re: RFR: 8285827: Describe the keystore.pkcs12.legacy system property in the java.security file
On Thu, 28 Apr 2022 14:35:54 GMT, Weijun Wang wrote: > We added a new system property back in > https://bugs.openjdk.java.net/browse/JDK-8153005 but it's better to describe > it in the `java.security` file as well. > > Please review the text. I especially added the last sentence so that people > won't set `-Dkeystore.pkcs12.legacy=false`. src/java.base/share/conf/security/java.security line 1174: > 1172: # If the property is not set or empty, a default value will be used. > 1173: # > 1174: # For compatibility, the system property "keystore.pkcs12.legacy" can > be set Was wondering if we should add why you might want to set this property, ex: "For compatibility with JDK or PKCS12 implementations that do not support the stronger algorithms ..." Compatibility with prior JDK versions should be less of an issue over time as these stronger settings and algs have been backported to prior JDKs. - PR: https://git.openjdk.java.net/jdk/pull/8452
Re: RFR: 8285827: Describe the keystore.pkcs12.legacy system property in the java.security file
On Thu, 28 Apr 2022 19:48:38 GMT, Sean Mullan wrote: >> We added a new system property back in >> https://bugs.openjdk.java.net/browse/JDK-8153005 but it's better to describe >> it in the `java.security` file as well. >> >> Please review the text. I especially added the last sentence so that people >> won't set `-Dkeystore.pkcs12.legacy=false`. > > src/java.base/share/conf/security/java.security line 1174: > >> 1172: # If the property is not set or empty, a default value will be used. >> 1173: # >> 1174: # For compatibility, the system property "keystore.pkcs12.legacy" can >> be set > > Was wondering if we should add why you might want to set this property, ex: > "For compatibility with JDK or PKCS12 implementations that do not support the > stronger algorithms ..." > > Compatibility with prior JDK versions should be less of an issue over time as > these stronger settings and algs have been backported to prior JDKs. OpenSSL's help page shows -legacy Use legacy encryption: 3DES_CBC for keys, RC2_CBC for certs Can we also say "To work with legacy PKCS #12 files"? - PR: https://git.openjdk.java.net/jdk/pull/8452
Re: RFR: 8285827: Describe the keystore.pkcs12.legacy system property in the java.security file
On Thu, 28 Apr 2022 19:54:36 GMT, Weijun Wang wrote: >> src/java.base/share/conf/security/java.security line 1174: >> >>> 1172: # If the property is not set or empty, a default value will be used. >>> 1173: # >>> 1174: # For compatibility, the system property "keystore.pkcs12.legacy" can >>> be set >> >> Was wondering if we should add why you might want to set this property, ex: >> "For compatibility with JDK or PKCS12 implementations that do not support >> the stronger algorithms ..." >> >> Compatibility with prior JDK versions should be less of an issue over time >> as these stronger settings and algs have been backported to prior JDKs. > > OpenSSL's help page shows > > -legacy Use legacy encryption: 3DES_CBC for keys, RC2_CBC for > certs > > Can we also say "To work with legacy PKCS #12 files"? But isn't it mostly an issue when creating new keystores and not reading existing ones? I would want to avoid users thinking that they had to set this in more cases than needed. - PR: https://git.openjdk.java.net/jdk/pull/8452
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 16:23:25 GMT, Bradford Wetmore wrote: >> src/java.base/share/classes/javax/net/ssl/TrustManagerFactory.java line 82: >> >>> 80: String type; >>> 81: type = GetPropertyAction.privilegedGetProperty( >>> 82: "ssl.TrustManagerFactory.algorithm"); >> >> Sorry I got it wrong here, this is a security property. > > Similar comment. Back to the original lambda expression. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v5]
On Thu, 28 Apr 2022 18:29:35 GMT, Mark Powers wrote: >> https://bugs.openjdk.java.net/browse/JDK-8285504 >> >> JDK-8273046 is the umbrella bug for this bug. The changes were too large for >> a single code review, so it was decided to split into smaller chunks. This >> is one such chunk: >> >> open/src/java.base/share/classes/java/net > > Mark Powers has updated the pull request incrementally with one additional > commit since the last revision: > > Max and Brad comments round two LGTM, Pt2... - Marked as reviewed by wetmore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8384
Integrated: JDK-8285504 Minor cleanup could be done in javax.net
On Mon, 25 Apr 2022 17:40:13 GMT, Mark Powers wrote: > https://bugs.openjdk.java.net/browse/JDK-8285504 > > JDK-8273046 is the umbrella bug for this bug. The changes were too large for > a single code review, so it was decided to split into smaller chunks. This is > one such chunk: > > open/src/java.base/share/classes/java/net This pull request has now been integrated. Changeset: 573eacec Author:Mark Powers Committer: Bradford Wetmore URL: https://git.openjdk.java.net/jdk/commit/573eaceca559a8a0832b1e1a7181b2f21d3978c7 Stats: 129 lines in 20 files changed: 1 ins; 22 del; 106 mod 8285504: Minor cleanup could be done in javax.net Reviewed-by: wetmore - PR: https://git.openjdk.java.net/jdk/pull/8384
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: 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 23:08:17 GMT, Valerie Peng wrote:
>> 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?
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.
-
PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8285827: Describe the keystore.pkcs12.legacy system property in the java.security file
On Thu, 28 Apr 2022 19:59:07 GMT, Sean Mullan wrote: >> OpenSSL's help page shows >> >> -legacy Use legacy encryption: 3DES_CBC for keys, RC2_CBC for >> certs >> >> Can we also say "To work with legacy PKCS #12 files"? > > But isn't it mostly an issue when creating new keystores and not reading > existing ones? I would want to avoid users thinking that they had to set this > in more cases than needed. How about this? To work with legacy PKCS #12 tools that does not support the new algorithms, the system property "keystore.pkcs12.legacy" can be set which will override the properties defined here with old settings. This system property is equivalent to - PR: https://git.openjdk.java.net/jdk/pull/8452
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 Thu, 28 Apr 2022 23:22:30 GMT, Valerie Peng wrote: >> 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. Then what does "cannot generate parameter values" mean? Any example? - PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v2]
On Thu, 14 Apr 2022 15:37:05 GMT, Daniel Jeliński wrote: > IMO we should not send close_notify in the finalizer. It's the application's > responsibility to send close_notify when it's done with the socket; we should > not pretend that it was closed normally when it was not. @djelinski makes an excellent point which I hadn't really considered. This is an error situation. As the native Socket resources are cleaned/released as needed, a simple removal might actually be appropriate. @XueleiFan I'm going to send you some internal discussion we've had in a minute. Let's both parse it and see if there is anything further we should consider, and circle back tomorrow and finalize the plan & push. - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8255552: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms [v2]
On Thu, 28 Apr 2022 13:25:13 GMT, Sean Mullan wrote:
>> Hai-May Chao has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> SecretKeyConstraintsParameters subclass created and property description
>> updated
>
> src/java.base/share/conf/security/java.security line 641:
>
>> 639:
>> 640: #
>> 641: # Legacy cryptographic algorithms and key lengths
>
> Nit: add period at end of sentence.
Ok.
> test/jdk/sun/security/tools/keytool/ReadJar.java line 26:
>
>> 24: /**
>> 25: * @test
>> 26: * @bug 6890872 8168882 8257722 822
>
> Here we are just updating the warning message, so I don't think the bugid
> needs to be added.
Ok.
> test/jdk/sun/security/tools/keytool/ReadJar.java line 162:
>
>> 160: .shouldContain("Certificate #2:")
>> 161: .shouldContain("Signer #2:")
>> 162: .shouldNotMatch("The certificate #.* of signer #.*" +
>> "uses the SHA1withRSA.*will be disabled")
>
> You probably don't need to check for a non-occurrence here since the message
> has been changed and can no longer occur. I also think it doesn't need to
> list the bugid because it is not testing the main fix which is warnings on
> symmetric key algs.
In webrev 01, I’ve made the change to remove the checking for a non-occurrence.
I added the bugid as the test verifies the warning message for asymmetric keys
and certificates, which is changed In this PR. I'll remove the bugid as it is
not testing the main fix.
-
PR: https://git.openjdk.java.net/jdk/pull/8300
Re: RFR: 8255552: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms [v2]
On Thu, 28 Apr 2022 06:46:35 GMT, Hai-May Chao wrote: >> Please review these changes to add DES/3DES/MD5 to >> `jdk.security.legacyAlgorithms` security property, and to add the legacy >> algorithm constraint checking to `keytool` commands that are associated with >> secret key entries stored in the keystore. These `keytool` commands are >> -genseckey, -importpass, -list, and -importkeystore. As a result, `keytool` >> will be able to generate warnings when it detects that the secret key based >> algorithms and PBE based Mac and cipher algorithms are weak. Also removes >> the "This algorithm will be disabled in a future update.” from the existing >> warnings for the asymmetric keys/certificates. >> Will also file a CSR. > > Hai-May Chao has updated the pull request incrementally with one additional > commit since the last revision: > > SecretKeyConstraintsParameters subclass created and property description > updated Please review CSR at https://bugs.openjdk.java.net/browse/JDK-8285873 Thanks! - PR: https://git.openjdk.java.net/jdk/pull/8300
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v2]
On Thu, 28 Apr 2022 23:09:00 GMT, Valerie Peng wrote:
> 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}.**
-
PR: https://git.openjdk.java.net/jdk/pull/8396
