Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v2]

2022-04-29 Thread Bradford Wetmore
On Thu, 7 Apr 2022 20:17:28 GMT, Xue-Lei Andrew Fan  wrote:

>> Please review the update to remove finalizer method in the SunJSSE provider 
>> implementation.  It is one of the efforts to clean up the use of finalizer 
>> method in JDK.
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with two 
> additional commits since the last revision:
> 
>  - typo blank linke correction
>  - revise the update

Marked as reviewed by wetmore (Reviewer).

src/java.base/share/classes/sun/security/ssl/BaseSSLSocketImpl.java line 265:

> 263: }
> 264: 
> 265: /**

Can you please add a quick couple lines here with the technical rationale for 
removing it, so we don't forget down the road.

i.e.

There used to be a finalize() here, but decided that was not
needed.  If users don't properly close the socket...

The native resources are handled by the Socket Cleaner.

-

PR: https://git.openjdk.java.net/jdk/pull/8065


Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v2]

2022-04-29 Thread Bradford Wetmore
On Thu, 7 Apr 2022 20:17:28 GMT, Xue-Lei Andrew Fan  wrote:

>> Please review the update to remove finalizer method in the SunJSSE provider 
>> implementation.  It is one of the efforts to clean up the use of finalizer 
>> method in JDK.
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with two 
> additional commits since the last revision:
> 
>  - typo blank linke correction
>  - revise the update

Ok, after much back and forth, I'm convinced this is ok.  

@dfuch commented in another thread:

> An application should really close its sockets and not let them get garbage 
> collected without closing them: this is sloppy.
> So brutally closing the underlying TCP connection in that case should be an 
> acceptable behaviour, and that would be achieved by just removing the 
> finalize. 

Thanks for allowing me to look more deeply into this.

-

Marked as reviewed by wetmore (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8065


Re: RFR: 8285743: Ensure each IntegerPolynomial object is only created once [v2]

2022-04-29 Thread Weijun Wang
> All `IntegerPolynimial`s are singletons now. Also, hand-coded implementations 
> for Ed25519 and Ed448 are removed. They were not used since `FieldGen` starts 
> generating classes for them.
> 
> No new regression test. This is a clean-up.

Weijun Wang has updated the pull request incrementally with one additional 
commit since the last revision:

  move singleton back into impl and make constructor private

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8476/files
  - new: https://git.openjdk.java.net/jdk/pull/8476/files/b24ca783..331e9519

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=8476=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8476=00-01

  Stats: 54 lines in 8 files changed: 7 ins; 19 del; 28 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8476.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8476/head:pull/8476

PR: https://git.openjdk.java.net/jdk/pull/8476


RFR: 8285743: Ensure each IntegerPolynomial object is only created once

2022-04-29 Thread Weijun Wang
All `IntegerPolynimial`s are singletons now. Also, hand-coded implementations 
for Ed25519 and Ed448 are removed. They were not used since `FieldGen` starts 
generating classes for them.

No new regression test. This is a clean-up.

-

Commit messages:
 - the fix

Changes: https://git.openjdk.java.net/jdk/pull/8476/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8476=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8285743
  Stats: 530 lines in 10 files changed: 28 ins; 465 del; 37 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8476.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8476/head:pull/8476

PR: https://git.openjdk.java.net/jdk/pull/8476


Re: RFR: 8285827: Describe the keystore.pkcs12.legacy system property in the java.security file [v2]

2022-04-29 Thread Weijun Wang
On Fri, 29 Apr 2022 20:47:08 GMT, Sean Mullan  wrote:

>> The reason I added the last sentence is because this property has no value. 
>> Someone might think they can set it to false to disable it, but that is 
>> equivalent to set it to true.
>
> Ah I see. Maybe put in the previous sentence, ex: "When set, this system 
> property (which can only be enabled and has no value) is equivalent to:"
> 
> Just a suggestion.

Suggested accepted. I just pushed a new commit. I modified "PKCS #12" to 
"PKCS12" because that's the word we used throughout the file.

-

PR: https://git.openjdk.java.net/jdk/pull/8452


Re: RFR: 8285827: Describe the keystore.pkcs12.legacy system property in the java.security file [v2]

2022-04-29 Thread Weijun Wang
> 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`.

Weijun Wang has updated the pull request incrementally with one additional 
commit since the last revision:

  clearer text

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8452/files
  - new: https://git.openjdk.java.net/jdk/pull/8452/files/08700389..8a24c745

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=8452=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8452=00-01

  Stats: 18 lines in 1 file changed: 7 ins; 0 del; 11 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: 8285827: Describe the keystore.pkcs12.legacy system property in the java.security file

2022-04-29 Thread Sean Mullan
On Fri, 29 Apr 2022 20:40:46 GMT, Weijun Wang  wrote:

>> It's a little long, but I can see why it is useful, so I think it's good. I 
>> would avoid the word "new" as this won't be new in a few years time. Here is 
>> an edit where I removed words which I thought were not essential:
>> 
>>> Some PKCS #12 tools and libraries may not support algorithms based on PBES2 
>>> and AES. 
>>> To create a PKCS #12 keystore which they can load, set the system property
>>> "keystore.pkcs12.legacy" which overrides the values of the properties 
>>> defined below with
>>> legacy algorithms. Setting this system property is equivalent to
>>> 
>>>   
>>> 
>>> Also, you can downgrade an existing PKCS #12 keystore created with stronger 
>>> algorithms
>>> to legacy algorithms with
>>> 
>>>keytool -J-Dkeystore.pkcs12.legacy -importkeystore -srckeystore ks 
>>> -destkeystore ks
>>> 
>>> This system property should be used at your own risk. 
>> 
>> Don't think you really need the sentence below, as you have already given 
>> several examples:
>> 
>>> Please note there is
>>> no value defined for this system property, i.e. "-Dkeystore.pkcs12.legacy"
>>> has the same effect as "-Dkeystore.pkcs12.legacy=".
>
> The reason I added the last sentence is because this property has no value. 
> Someone might think they can set it to false to disable it, but that is 
> equivalent to set it to true.

Ah I see. Maybe put in the previous sentence, ex: "When set, this system 
property (which can only be enabled and has no value) is equivalent to:"

Just a suggestion.

-

PR: https://git.openjdk.java.net/jdk/pull/8452


Re: RFR: 8285827: Describe the keystore.pkcs12.legacy system property in the java.security file

2022-04-29 Thread Weijun Wang
On Fri, 29 Apr 2022 20:35:14 GMT, Sean Mullan  wrote:

>> Can we say both? All these properties are only used when creating the file 
>> (key-related ones when creating the key). If a compatibility issue already 
>> happens, users need to downgrade their keystore.
>> 
>> So, the full text will be something like
>> 
>> Some legacy PKCS #12 tools or libraries do not support the new algorithms 
>> based on
>> PBES2 and AES. In order to create a PKCS #12 keystore for them, the system 
>> property
>> "keystore.pkcs12.legacy" can be set which overrides the properties defined 
>> here with
>> legacy algorithm. Setting this system property is equivalent to
>> 
>>   
>> 
>> Also, you can downgrade an existing PKCS #12 keystore that already uses new 
>> algorithms
>> to use legacy algorithms with
>> 
>>keytool -J-Dkeystore.pkcs12.legacy -importkeystore -srckeystore ks 
>> -destkeystore ks
>> 
>> This system property should be used at your own risk. Please note there is
>> no value defined for this system property, i.e. "-Dkeystore.pkcs12.legacy"
>> has the same effect as "-Dkeystore.pkcs12.legacy=".
>> 
>> I'll double check if the command can indeed downgrade key algorithms as 
>> well. *Update*: it works. All 3 algorithms (key, cert, mac) downgraded to 
>> legacy ones.
>
> It's a little long, but I can see why it is useful, so I think it's good. I 
> would avoid the word "new" as this won't be new in a few years time. Here is 
> an edit where I removed words which I thought were not essential:
> 
>> Some PKCS #12 tools and libraries may not support algorithms based on PBES2 
>> and AES. 
>> To create a PKCS #12 keystore which they can load, set the system property
>> "keystore.pkcs12.legacy" which overrides the values of the properties 
>> defined below with
>> legacy algorithms. Setting this system property is equivalent to
>> 
>>   
>> 
>> Also, you can downgrade an existing PKCS #12 keystore created with stronger 
>> algorithms
>> to legacy algorithms with
>> 
>>keytool -J-Dkeystore.pkcs12.legacy -importkeystore -srckeystore ks 
>> -destkeystore ks
>> 
>> This system property should be used at your own risk. 
> 
> Don't think you really need the sentence below, as you have already given 
> several examples:
> 
>> Please note there is
>> no value defined for this system property, i.e. "-Dkeystore.pkcs12.legacy"
>> has the same effect as "-Dkeystore.pkcs12.legacy=".

The reason I added the last sentence is because this property has no value. 
Someone might think they can set it to false to disable it, but that is 
equivalent to set it to true.

-

PR: https://git.openjdk.java.net/jdk/pull/8452


Re: RFR: 8285827: Describe the keystore.pkcs12.legacy system property in the java.security file

2022-04-29 Thread Sean Mullan
On Fri, 29 Apr 2022 13:28:11 GMT, Weijun Wang  wrote:

>> I think the text above might still make some users concerned that they 
>> should always set this property.
>> Maybe we can be less specific, and just say: "If you encounter compatibility 
>> issues with software that doesn't support the stronger algorithms, the 
>> system property ..."
>
> Can we say both? All these properties are only used when creating the file 
> (key-related ones when creating the key). If a compatibility issue already 
> happens, users need to downgrade their keystore.
> 
> So, the full text will be something like
> 
> Some legacy PKCS #12 tools or libraries do not support the new algorithms 
> based on
> PBES2 and AES. In order to create a PKCS #12 keystore for them, the system 
> property
> "keystore.pkcs12.legacy" can be set which overrides the properties defined 
> here with
> legacy algorithm. Setting this system property is equivalent to
> 
>   
> 
> Also, you can downgrade an existing PKCS #12 keystore that already uses new 
> algorithms
> to use legacy algorithms with
> 
>keytool -J-Dkeystore.pkcs12.legacy -importkeystore -srckeystore ks 
> -destkeystore ks
> 
> This system property should be used at your own risk. Please note there is
> no value defined for this system property, i.e. "-Dkeystore.pkcs12.legacy"
> has the same effect as "-Dkeystore.pkcs12.legacy=".
> 
> I'll double check if the command can indeed downgrade key algorithms as well. 
> *Update*: it works. All 3 algorithms (key, cert, mac) downgraded to legacy 
> ones.

It's a little long, but I can see why it is useful, so I think it's good. I 
would avoid the word "new" as this won't be new in a few years time. Here is an 
edit where I removed words which I thought were not essential:

> Some PKCS #12 tools and libraries may not support algorithms based on PBES2 
> and AES. 
> To create a PKCS #12 keystore which they can load, set the system property
> "keystore.pkcs12.legacy" which overrides the values of the properties defined 
> below with
> legacy algorithms. Setting this system property is equivalent to
> 
>   
> 
> Also, you can downgrade an existing PKCS #12 keystore created with stronger 
> algorithms
> to legacy algorithms with
> 
>keytool -J-Dkeystore.pkcs12.legacy -importkeystore -srckeystore ks 
> -destkeystore ks
> 
> This system property should be used at your own risk. 

Don't think you really need the sentence below, as you have already given 
several examples:

> Please note there is
> no value defined for this system property, i.e. "-Dkeystore.pkcs12.legacy"
> has the same effect as "-Dkeystore.pkcs12.legacy=".

-

PR: https://git.openjdk.java.net/jdk/pull/8452


Re: RFR: 8255552: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms [v4]

2022-04-29 Thread Hai-May Chao
On Fri, 29 Apr 2022 19:18:06 GMT, Sean Mullan  wrote:

>> Hai-May Chao has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Removed RC2 changes
>
> src/java.base/share/conf/security/java.security line 644:
> 
>> 642: #
>> 643: # In some environments, a certain algorithm or key length may be 
>> undesirable
>> 644: # but is not yet disabled.
> 
> I would also remove the words "but is not yet disabled." An algorithm may be 
> disabled at different times for different components, such as TLS or 
> Kerberos, so it isn't always a yes or no answer. Also, if a disabled 
> algorithm is re-enabled (by modifying the security properties), we still want 
> `keytool` or `jarsigner` to show warnings.

Good point. Updated the java.security file and the CSR.

-

PR: https://git.openjdk.java.net/jdk/pull/8300


Re: RFR: 8255552: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms [v5]

2022-04-29 Thread Hai-May Chao
> 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:

  Updated spec in java.security

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8300/files
  - new: https://git.openjdk.java.net/jdk/pull/8300/files/e10ed759..ac92a5f7

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=8300=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8300=03-04

  Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8300.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8300/head:pull/8300

PR: https://git.openjdk.java.net/jdk/pull/8300


RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer

2022-04-29 Thread Anthony Scarpino
Hi,

I need a review of this fix to allow a read-only 'src' buffer to be used with 
SSLEngine.unwrap(). A temporary read-write buffer is created in the SSLCipher 
operation when a read-only buffer is passed. If the 'src' is read-write, there 
is no effect on the current operation

The PR also includes a CSR for an API implementation note to the 
SSLEngine.unwrap. The 'src' buffer may be modified during the decryption 
operation. 'unwrap()' has had this behavior forever, so there is no 
compatibility issue with this note. Using the 'src' buffer for in-place 
decryption was a performance decision.

Tony

-

Commit messages:
 - update some nits
 - PR ready
 - Initial

Changes: https://git.openjdk.java.net/jdk/pull/8462/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8462=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8283577
  Stats: 401 lines in 3 files changed: 301 ins; 20 del; 80 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8462.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8462/head:pull/8462

PR: https://git.openjdk.java.net/jdk/pull/8462


Re: RFR: 8255552: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms [v4]

2022-04-29 Thread Sean Mullan
On Fri, 29 Apr 2022 17:06:28 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:
> 
>   Removed RC2 changes

src/java.base/share/conf/security/java.security line 644:

> 642: #
> 643: # In some environments, a certain algorithm or key length may be 
> undesirable
> 644: # but is not yet disabled.

I would also remove the words "but is not yet disabled." An algorithm may be 
disabled at different times for different components, such as TLS or Kerberos, 
so it isn't always a yes or no answer. Also, if a disabled algorithm is 
re-enabled (by modifying the security properties), we still want `keytool` or 
`jarsigner` to show warnings.

-

PR: https://git.openjdk.java.net/jdk/pull/8300


Re: RFR: 8285890: Fix some @param tags

2022-04-29 Thread Bradford Wetmore
On Fri, 29 Apr 2022 09:03:58 GMT, Pavel Rappo  wrote:

> * Syntactically improves a few cases from 8285676 
> (https://github.com/openjdk/jdk/pull/8410)
> * Fixes a few misspelled `@param` tags for elements that, although are not 
> included in the API Documentation, are visible in and processed by IDEs

LGTM also.

-

Marked as reviewed by wetmore (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8465


javax.crypto.CryptoPolicyParser#isConsistent always returns 'true'

2022-04-29 Thread Andrey Turbanov
Hello.
I found a suspicious code in CryptoPolicyParser method calls.

Method 'isConsistent' is called only from a method
'parsePermissionEntry'. It accepts the 'processedPermissions'
parameter from 'parsePermissionEntry'.
Method 'parsePermissionEntry' is called only from a method
'parseGrantEntry'. It accepts the 'processedPermissions' parameter
from 'parseGrantEntry'.
Method 'parseGrantEntry' is called only from a method 'read' and
always with null value of parameter 'processedPermissions'.

So, it seems in method 'isConsistent' value of parameter
'processedPermissions' will always be 'null'. And the method will
always return true.
Is this the result of some refactoring? Or did I miss something?



Andrey Turbanov


Re: RFR: 8255552: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms [v4]

2022-04-29 Thread Hai-May Chao
> 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:

  Removed RC2 changes

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8300/files
  - new: https://git.openjdk.java.net/jdk/pull/8300/files/45066a72..e10ed759

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=8300=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8300=02-03

  Stats: 7 lines in 2 files changed: 0 ins; 7 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8300.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8300/head:pull/8300

PR: https://git.openjdk.java.net/jdk/pull/8300


Re: RFR: 8255552: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms [v3]

2022-04-29 Thread Hai-May Chao
> 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:

  Removed bugid and updated property spec

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8300/files
  - new: https://git.openjdk.java.net/jdk/pull/8300/files/2079c60b..45066a72

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=8300=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8300=01-02

  Stats: 3 lines in 3 files changed: 0 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8300.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8300/head:pull/8300

PR: https://git.openjdk.java.net/jdk/pull/8300


Re: RFR: 8285890: Fix some @param tags

2022-04-29 Thread Mandy Chung
On Fri, 29 Apr 2022 09:03:58 GMT, Pavel Rappo  wrote:

> * Syntactically improves a few cases from 8285676 
> (https://github.com/openjdk/jdk/pull/8410)
> * Fixes a few misspelled `@param` tags for elements that, although are not 
> included in the API Documentation, are visible in and processed by IDEs

Marked as reviewed by mchung (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/8465


Re: RFR: 8285890: Fix some @param tags

2022-04-29 Thread Joe Darcy
On Fri, 29 Apr 2022 09:03:58 GMT, Pavel Rappo  wrote:

> * Syntactically improves a few cases from 8285676 
> (https://github.com/openjdk/jdk/pull/8410)
> * Fixes a few misspelled `@param` tags for elements that, although are not 
> included in the API Documentation, are visible in and processed by IDEs

Thanks for fixing these Pavel.

-

Marked as reviewed by darcy (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8465


Re: RFR: 8209038: Clarify the javadoc of Cipher.getParameters() [v3]

2022-04-29 Thread Sean Mullan
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()

Changes requested by mullan (Reviewer).

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}_."

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".

-

PR: https://git.openjdk.java.net/jdk/pull/8117


Re: RFR: 8285827: Describe the keystore.pkcs12.legacy system property in the java.security file

2022-04-29 Thread Weijun Wang
On Fri, 29 Apr 2022 13:17:55 GMT, Sean Mullan  wrote:

>> 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
>
> I think the text above might still make some users concerned that they should 
> always set this property.
> Maybe we can be less specific, and just say: "If you encounter compatibility 
> issues with software that doesn't support the stronger algorithms, the system 
> property ..."

Can we say both? All these properties are only used when creating the file 
(key-related ones when creating the key). If a compatibility issue already 
happens, users need to downgrade their keystore.

So, the full text will be something like

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.
If you encounter compatibility issues with software that doesn't support the 
stronger algorithms,
you can downgrade the keystore with

   keytool -J-Dkeystore.pkcs12.legacy -importkeystore -keystore ks ...

I'll double check if the command can indeed downgrade key algorithms as well.

-

PR: https://git.openjdk.java.net/jdk/pull/8452


Re: RFR: 8285827: Describe the keystore.pkcs12.legacy system property in the java.security file

2022-04-29 Thread Sean Mullan
On Thu, 28 Apr 2022 23:20:18 GMT, Weijun Wang  wrote:

>> 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

I think the text above might still make some users concerned that they should 
always set this property.
Maybe we can be less specific, and just say: "If you encounter compatibility 
issues with software that doesn't support the stronger algorithms, the system 
property ..."

-

PR: https://git.openjdk.java.net/jdk/pull/8452


Re: RFR: 8285890: Fix some @param tags

2022-04-29 Thread Sean Mullan
On Fri, 29 Apr 2022 09:03:58 GMT, Pavel Rappo  wrote:

> * Syntactically improves a few cases from 8285676 
> (https://github.com/openjdk/jdk/pull/8410)
> * Fixes a few misspelled `@param` tags for elements that, although are not 
> included in the API Documentation, are visible in and processed by IDEs

GSSHeader looks fine.

-

Marked as reviewed by mullan (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8465


Integrated: 8225433: Clarify behavior of PKIXParameters.setRevocationEnabled when PKIXRevocationChecker is used

2022-04-29 Thread Sean Mullan
On Mon, 18 Apr 2022 13:35:25 GMT, Sean Mullan  wrote:

> This change improves the specification for the case when a 
> `PKIXRevocationChecker` is supplied as one of the `CertPathChecker` 
> parameters. Specifically, it makes it more clear that a 
> `PKIXRevocationChecker` overrides the default revocation checking mechanism 
> of a PKIX service provider, and will be used to check revocation irrespective 
> of the setting of the RevocationEnabled parameter.
> 
> Will also file a CSR.

This pull request has now been integrated.

Changeset: 694556e1
Author:Sean Mullan 
URL:   
https://git.openjdk.java.net/jdk/commit/694556e1374eb45776e6105cc4f7a3445a43c3cc
Stats: 17 lines in 2 files changed: 9 ins; 0 del; 8 mod

8225433: Clarify behavior of PKIXParameters.setRevocationEnabled when 
PKIXRevocationChecker is used

Reviewed-by: xuelei, hchao

-

PR: https://git.openjdk.java.net/jdk/pull/8287


Re: RFR: 8285890: Fix some @param tags

2022-04-29 Thread Daniel Fuchs
On Fri, 29 Apr 2022 09:03:58 GMT, Pavel Rappo  wrote:

> * Syntactically improves a few cases from 8285676 
> (https://github.com/openjdk/jdk/pull/8410)
> * Fixes a few misspelled `@param` tags for elements that, although are not 
> included in the API Documentation, are visible in and processed by IDEs

LGTM

-

Marked as reviewed by dfuchs (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8465


RFR: 8285890: Fix some @param tags

2022-04-29 Thread Pavel Rappo
* Syntactically improves a few cases from 8285676 
(https://github.com/openjdk/jdk/pull/8410)
* Fixes a few misspelled `@param` tags for elements that, although are not 
included in the API Documentation, are visible in and processed by IDEs

-

Commit messages:
 - Fix misspelled `@param`
 - Clarify with whitespace tags from 8285676

Changes: https://git.openjdk.java.net/jdk/pull/8465/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8465=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8285890
  Stats: 16 lines in 8 files changed: 0 ins; 0 del; 16 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8465.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8465/head:pull/8465

PR: https://git.openjdk.java.net/jdk/pull/8465


Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v5]

2022-04-29 Thread Pavel Rappo
On Fri, 29 Apr 2022 08:45:42 GMT, Pavel Rappo  wrote:

> Good catch. Sorry I missed it. This occurs in all `java/lang/ref` files.

I've created a PR; feel free to review it at your convenience.

-

PR: https://git.openjdk.java.net/jdk/pull/8410


Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v5]

2022-04-29 Thread Pavel Rappo
On Thu, 28 Apr 2022 19:06:04 GMT, Mandy Chung  wrote:

>> 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.

> Shouldn't there be a space after `@param` ?

> Good catch. Sorry I missed it. This occurs in all `java/lang/ref` files.

I built the API documentation after this PR has been integrated and the result 
was okay. I saw this output in every such case:

Type Parameters:
T - the type of the referent

javadoc is quite robust. However, for some IDEs such missing whitespace seems 
significant. Not only do they highlight the `@param` tag, but the type 
parameter information is missing from the rendered output.

Although it's not critical, we should fix it; I have filed JDK-8285890.

-

PR: https://git.openjdk.java.net/jdk/pull/8410