Re: RFR: 8322767: TLS full handshake is slow with PKCS12KeyStore and X509KeyManagerImpl [v3]

[Xue-Lei Andrew Fan]

On Wed, 27 Mar 2024 08:15:08 GMT, Hai-May Chao  wrote:

> I ran the benchmark to measure the time needed to build a TLS context using 
> PKIX KeyManager (with protocols "TLSv1.2" and "TLS”) before and after the 
> changes to X509KeyManagerImpl.java. Here are the results:
> 
> Without changes: Benchmark (tlsVersion) Mode Cnt Score Error Units 
> SSLHandshake.doHandshake TLSv1.2 thrpt 15 1073890.853 ? 18249.629 ops/s 
> SSLHandshake.doHandshake TLS thrpt 15 1139049.624 ? 5507.867 ops/s
> 
> With changes: Benchmark (tlsVersion) Mode Cnt Score Error Units 
> SSLHandshake.doHandshake TLSv1.2 thrpt 15 130.039 ? 4.372 ops/s 
> SSLHandshake.doHandshake TLS thrpt 15 134.157 ? 1.903 ops/s
> 
> For the case without changes, it has higher Error values and looks like there 
> is more variability in the measurements. I ran it again, and got similar 
> results with no background processes running. I can’t explain if there may be 
> environmental variables affecting the Error values. The changes that arise 
> time to set up cached map at initialization time will be paid off for 
> multiple handshaking.

The Error values looks fine to me.  The differences between them are caused by 
the differences between op/s numbers.

If I read the numbers correctly, the TLS context buildup performance impact is 
about 100 times.  The performance downgrading is not small.  If I remembered 
correctly, a few years ago we tried hard to speed up the TLS booting up as it 
is very slow to initialize a TLS context and customers run into problems in 
their environment.  Thinking about the industry effort to deploy the service in 
cloud environment, where need to speed up the booting time as soon as possible. 
 In cloud or micro-service loading environment, it is more important to boot-up 
the context as soon as possible, comparing to speed up each connections.

I don't think we have to complete the cache during the context building period. 
 It is also possible to build the cache when the entry is required.

-

PR Comment: https://git.openjdk.org/jdk/pull/17956#issuecomment-2023101804

Re: RFR: 8322767: TLS full handshake is slow with PKCS12KeyStore and X509KeyManagerImpl [v3]

[Xue-Lei Andrew Fan]

On Tue, 26 Mar 2024 06:00:33 GMT, Hai-May Chao  wrote:

>> For the PKIX KeyManager and PKCS12 Keystore, when the TLS server sends the 
>> ServerHello message and ultimately calls the 
>> X509KeyManagerImpl.chooseEngineServerAlias() method, it retrieves the 
>> private key from the keystore, decrypts it, and caches both the key and its 
>> certificate. This caching currently occurs only during a single handshake. 
>> Since decryption can be time-consuming, a modification has been implemented 
>> to cache the keystore entries at initialization time. This way, it won't be 
>> necessary to retrieve and decrypt the keys for multiple handshakes, which 
>> could lead to performance drawbacks.
>> 
>> A change was made to also update/refresh the cached entry as the 
>> certificates in the PKCS12 keystore may change, for scenarios like when the 
>> certificate expires and a new one is added under a different alias, and the 
>> certificate chain returned by the PKCS12 keystore is not the same as the one 
>> in the cache. While attempting to handle when to refresh a cached entry to 
>> accommodate keystore changes, we would like to know if you agree that this 
>> improvement is worth the risk. We would also like to know if you have a 
>> preference for other options:
>> 
>> 1. Accept that PKIX+PKCS12 is slow.
>> 2. Add a configuration option (system property, maybe) to decide the level 
>> of caching (1 - same as the existing one, 2 - same caching as in 
>> SunX509KeyManagerImpl, 3 - the new caching introduced in this change).
>> 
>> Additionally, the benchmark test SSLHandshake.java is modified to include a 
>> @Param annotation, allowing it to pass different KeyManagerFactory values 
>> (SunX509 and PKIX) to the benchmark method.
>> 
>> Running modified SSLHandshake.java test prior to the change that caches the 
>> PKCS12 keystore entries for PKIX:
>> Benchmark (keyMgr)  (resume)  (tlsVersion)   Mode  Cnt 
>> Score Error  Units
>> SSLHandshake.doHandshake   SunX509  true   TLSv1.2  thrpt   15  
>> 9346.292 ? 379.023  ops/s
>> SSLHandshake.doHandshake   SunX509  true   TLS  thrpt   15   
>> 940.175 ?  21.215  ops/s
>> SSLHandshake.doHandshake   SunX509 false   TLSv1.2  thrpt   15   
>> 594.418 ?  23.374  ops/s
>> SSLHandshake.doHandshake   SunX509 false   TLS  thrpt   15   
>> 534.030 ?  16.709  ops/s
>> SSLHandshake.doHandshake  PKIX  true   TLSv1.2  thrpt   15  
>> 9359.086 ? 246.257  ops/s
>> SSLHandshake.doHandshake  PKIX  true   TLS  thrpt   15   
>> 933.835 ?  81.388  ops/s
>> SSLHandshake.doHandshake  PKIX false   TLSv1.2  thrpt   15   ...
>
> Hai-May Chao has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Updated with John's comments

>From the description, it looks like you are concerning about that new bugs 
>could be introduced and if the performance improvement worth the risk.  Before 
>looking into the performance numbers, I think it would be nice to make sure no 
>bugs was introduced. Otherwise, the performance improvement makes no sense.

> Without changes:
> SSLHandshake.doHandshake PKIX true TLS thrpt 15 937.166 ? 23.937 ops/s
>
> With changes:
> SSLHandshake.doHandshake PKIX true TLS thrpt 15 910.733 ? 87.172 ops/s

Here is a performance regression, from 937 to 910 ops/s.

> I created a JMH benchmark to measure the time needed to build a TLS context 
> using different SSL protocols ("TLSv1.2" and "TLS") and different KeyManager 
> types ("SunX509" and "PKIX”). Here is the benchmark result:
> 
> Benchmark (keyMgr) (tlsVersion) Mode Cnt Score Error Units 
> SSLHandshake.doHandshake SunX509 TLSv1.2 thrpt 15 112.920 ? 10.518 ops/s 
> SSLHandshake.doHandshake SunX509 TLS thrpt 15 122.255 ? 3.128 ops/s 
> SSLHandshake.doHandshake PKIX TLSv1.2 thrpt 15 115.956 ? 5.490 ops/s 
> SSLHandshake.doHandshake PKIX TLS thrpt 15 118.512 ? 3.574 ops/s
> 
> The performance between the SunX509 and PKIX KeyManagers for building SSL 
> contexts seems fairly comparable. The changes made to X509KeyManagerImpl.java 
> to cache keystore entries in the credentialsMap at initialization time for 
> the PKIX KeyManager do not impact performance compared to SunX509.
> 

I think the comparing could between apple to apple.  PKIX and SunX509 are 
different key managers and could have different performance numbers.  Did you 
have the number for PKIX before and after the update for TLS boot-up?

-

PR Comment: https://git.openjdk.org/jdk/pull/17956#issuecomment-2021896501

Re: RFR: 8322767: TLS full handshake is slow with PKCS12KeyStore and X509KeyManagerImpl [v2]

[Xue-Lei Andrew Fan]

On Fri, 22 Mar 2024 06:56:33 GMT, Hai-May Chao  wrote:

>> For the PKIX KeyManager and PKCS12 Keystore, when the TLS server sends the 
>> ServerHello message and ultimately calls the 
>> X509KeyManagerImpl.chooseEngineServerAlias() method, it retrieves the 
>> private key from the keystore, decrypts it, and caches both the key and its 
>> certificate. This caching currently occurs only during a single handshake. 
>> Since decryption can be time-consuming, a modification has been implemented 
>> to cache the keystore entries at initialization time. This way, it won't be 
>> necessary to retrieve and decrypt the keys for multiple handshakes, which 
>> could lead to performance drawbacks.
>> 
>> A change was made to also update/refresh the cached entry as the 
>> certificates in the PKCS12 keystore may change, for scenarios like when the 
>> certificate expires and a new one is added under a different alias, and the 
>> certificate chain returned by the PKCS12 keystore is not the same as the one 
>> in the cache. While attempting to handle when to refresh a cached entry to 
>> accommodate keystore changes, we would like to know if you agree that this 
>> improvement is worth the risk. We would also like to know if you have a 
>> preference for other options:
>> 
>> 1. Accept that PKIX+PKCS12 is slow.
>> 2. Add a configuration option (system property, maybe) to decide the level 
>> of caching (1 - same as the existing one, 2 - same caching as in 
>> SunX509KeyManagerImpl, 3 - the new caching introduced in this change).
>> 
>> Additionally, the benchmark test SSLHandshake.java is modified to include a 
>> @Param annotation, allowing it to pass different KeyManagerFactory values 
>> (SunX509 and PKIX) to the benchmark method.
>> 
>> Running modified SSLHandshake.java test prior to the change that caches the 
>> PKCS12 keystore entries for PKIX:
>> Benchmark (keyMgr)  (resume)  (tlsVersion)   Mode  Cnt 
>> Score Error  Units
>> SSLHandshake.doHandshake   SunX509  true   TLSv1.2  thrpt   15  
>> 9346.292 ? 379.023  ops/s
>> SSLHandshake.doHandshake   SunX509  true   TLS  thrpt   15   
>> 940.175 ?  21.215  ops/s
>> SSLHandshake.doHandshake   SunX509 false   TLSv1.2  thrpt   15   
>> 594.418 ?  23.374  ops/s
>> SSLHandshake.doHandshake   SunX509 false   TLS  thrpt   15   
>> 534.030 ?  16.709  ops/s
>> SSLHandshake.doHandshake  PKIX  true   TLSv1.2  thrpt   15  
>> 9359.086 ? 246.257  ops/s
>> SSLHandshake.doHandshake  PKIX  true   TLS  thrpt   15   
>> 933.835 ?  81.388  ops/s
>> SSLHandshake.doHandshake  PKIX false   TLSv1.2  thrpt   15   ...
>
> Hai-May Chao has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Updated with John's comments

> While attempting to handle when to refresh a cached entry to accommodate 
> keystore changes, we would like to know if you agree that this improvement is 
> worth the risk. We would also like to know if you have a preference for other 
> options:

I'm not sure if it is the direction to improve performance by introducing new 
bugs.

If I read the benchmark result correction, for some cases, the performance was 
hurt by the update, some others was improved.  For the performance improved 
cases, it may be just in the cost of the key manager initialization 
performance.  The cost may be not desired as it hurts the boot-up performance 
to build a TLS context.

Alternatively, there might/could be an event get triggered when the key store 
get updated. It may worth to explore how to trigger the event, listen to the 
even and handle the event accordingly in the key manager and key store.

-

PR Comment: https://git.openjdk.org/jdk/pull/17956#issuecomment-2019427132

Re: RFR: 8311596: Add separate system properties for TLS server and client for maximum chain length [v8]

[Xue-Lei Andrew Fan]

On Thu, 9 Nov 2023 19:03:27 GMT, Sean Mullan  wrote:

>> I'm not sure if the number 8 or 10 really make a good difference in 
>> practice.  No matter 8 or 10,  if customers need lower value, they can 
>> always consider adjusting it.  My concern is mainly about compatibility 
>> issues.  If you want to keep the behavior changes, as there is potential 
>> compatibility issue, please feel free to describe the behavior change in 
>> release note and CSR if you would like.
>
> Good point - the CSR and RN could have been a bit more specific about the 
> compatibility effect of changing the default from 10 to 8, so we will update 
> that. Note that the CertPathBuilder default max path length [is 5 
> non-self-issued intermediate CA 
> certificates](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/security/cert/PKIXBuilderParameters.html#setMaxPathLength(int)),
>  so even a lower value of 8 should have low risk as rigid TLS cert chains 
> greater than 6 certs (where the 6th cert is the end entity cert which is not 
> affected by the CertPathBuilder limit) on the wire will already be rejected. 
> The additional certs permitted in the wire format is more for including a few 
> more additional certs that might help build a valid path when there is more 
> than one possible chain that a server or client might accept, which sometimes 
> happens.

Good point about the setMaxPathLength() limitation.  It looks like there might 
be an issue when the interactivities of setMaxPathLength(and its default value) 
and the properties defined here are not considered.  For example, what if the 
property is set to 8, while the setMaxPathLength is of value 5?  For the "PKIX" 
trust manager, per your description, if the property is set to 8, but 5 is the 
limit actually.  It looks like a weird behavior to me.  If I remember 
correctly, the "SunX509" trust manager does not use PKIXBuilderParameters, 
while the "PKIX" trust manager does.  It might be not the behavior we'd like to 
have that property 8 work for "SunX509" but not for "PKIX" trust manager.

Anyway, it might be better to look into the interactive behaviors among the 
properties and setMaxPathLength/default value.

> Good point - the CSR and RN could have been a bit more specific about the 
> compatibility effect of changing the default from 10 to 8, so we will update 
> that. Note that the CertPathBuilder default max path length [is 5 
> non-self-issued intermediate CA 
> certificates](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/security/cert/PKIXBuilderParameters.html#setMaxPathLength(int)),
>  so even a lower value of 8 should have low risk as rigid TLS cert chains 
> greater than 6 certs (where the 6th cert is the end entity cert which is not 
> affected by the CertPathBuilder limit) on the wire will already be rejected. 
> The additional certs permitted in the wire format is more for including a few 
> more additional certs that might help build a valid path when there is more 
> than one possible chain that a server or client might accept, which sometimes 
> happens.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/15163#discussion_r1388988092

Re: RFR: 8311596: Add separate system properties for TLS server and client for maximum chain length [v8]

[Xue-Lei Andrew Fan]

On Tue, 7 Nov 2023 20:27:06 GMT, Sean Mullan  wrote:

>> I'm not sure if there is a clear reason to change the default value from 10 
>> to 8.  I'm fine if you want to keep to use value 10 for less compatibility 
>> issues. Otherwise, I have no more comment.  Thanks!
>> 
>>> Yes, I can place the comments in the code blocks for the server-side 
>>> setting and client-side setting, respectively. @XueleiFan Any feedback 
>>> before I'm making this comment change? I will also update the release note 
>>> accordingly. Thanks!
>
> The choice of 8 for the client is mostly based on different processing 
> requirements and use cases for TLS client vs server certificate chains. If we 
> see evidence that 8 is too low, we can always consider adjusting it.

I'm not sure if the number 8 or 10 really make a good difference in practice.  
No matter 8 or 10,  if customers need lower value, they can always consider 
adjusting it.  My concern is mainly about compatibility issues.  If you want to 
keep the behavior changes, as there is potential compatibility issue, please 
feel free to describe the behavior change in release note and CSR if you would 
like.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/15163#discussion_r1385958137

Re: RFR: 8311596: Add separate system properties for TLS server and client for maximum chain length [v8]

[Xue-Lei Andrew Fan]

On Mon, 6 Nov 2023 20:48:59 GMT, Hai-May Chao  wrote:

>> I think the wording of the comment is somewhat confusing because it is 
>> trying to explain the behavior of both properties together and the words 
>> "either" and "neither" may be hard to parse. I recommend separate comment 
>> blocks for each property. Here is a suggestion for the server side setting:
>> 
>> 
>> /* 
>>  * maxInboundClientCertChainLen is the maximum length of a client certificate
>>  * chain accepted by a server. It is determined as follows:
>>  *  - If the jdk.tls.server.maxInboundCertificateChainLength system property
>>  *is set and its value >= 0, it uses that value.
>>  *  - Otherwise, if the jdk.tls.maxCertificateChainLength system property is
>>  *set and its value >= 0, it uses that value.
>>  *  - Otherwise it is set to a default value of 8.
>>  */
>> 
>> 
>> The client side setting would be similar.
>
> Yes, I can place the comments in the code blocks for the server-side setting 
> and client-side setting, respectively.
> @XueleiFan Any feedback before I'm making this comment change?
> I will also update the release note accordingly. Thanks!

I'm not sure if there is a clear reason to change the default value from 10 to 
8.  I'm fine if you want to keep to use value 10 for less compatibility issues. 
Otherwise, I have no more comment.  Thanks!

> Yes, I can place the comments in the code blocks for the server-side setting 
> and client-side setting, respectively. @XueleiFan Any feedback before I'm 
> making this comment change? I will also update the release note accordingly. 
> Thanks!

-

PR Review Comment: https://git.openjdk.org/jdk/pull/15163#discussion_r1384494328

Re: RFR: 8311596: Add separate system properties for TLS server and client for maximum chain length [v8]

[Xue-Lei Andrew Fan]

On Mon, 30 Oct 2023 21:53:44 GMT, Hai-May Chao  wrote:

>> I agree that wording is more clear. We should also update the RN with that 
>> wording.
>
> This section of comments was taken from the CSR. I updated the comments as 
> follows. If it looks fine, I will update the related doc. Thanks!
> 
> /*
>  * If either jdk.tls.server.maxInboundCertificateChainLength or
>  * jdk.tls.client.maxInboundCertificateChainLength is set, it will
>  * override jdk.tls.maxCertificateChainLength, regardless of whether
>  * jdk.tls.maxCertificateChainLength is set or not.
>  * If neither jdk.tls.server.maxInboundCertificateChainLength nor
>  * jdk.tls.client.maxInboundCertificateChainLength is set, the 
> behavior
>  * depends on the setting of jdk.tls.maxCertificateChainLength. If
>  * jdk.tls.maxCertificateChainLength is set, it falls back to that
>  * value; otherwise, it defaults to 8 for
>  * jdk.tls.server.maxInboundCertificateChainLength
>  * and 10 for jdk.tls.client.maxInboundCertificateChainLength.
>  * Usesrs can independently set either
>  * jdk.tls.server.maxInboundCertificateChainLength or
>  * jdk.tls.client.maxInboundCertificateChainLength.
>  */

Sorry, I did not get time to review this behavior update.

> This section of comments was taken from the CSR. I updated the comments as 
> follows. If it looks fine, I will update the related doc. Thanks!
> 
> ```
> /*
>  * If either jdk.tls.server.maxInboundCertificateChainLength or
>  * jdk.tls.client.maxInboundCertificateChainLength is set, it will
>  * override jdk.tls.maxCertificateChainLength, regardless of whether
>  * jdk.tls.maxCertificateChainLength is set or not.
I'm not sure the statement is clear enough.  I think there are two points that 
need to clarify.  The 1st one is that there is a default value of  
jdk.tls.maxCertificateChainLength, which is 10.  The 2nd one is that  
jdk.tls.maxCertificateChainLength works for both client and server mode.  
jdk.tls.server.maxInboundCertificateChainLength works on server mode, and it 
does not work for client mode, and therefore it cannot override client mode 
behavior for jdk.tls.maxCertificateChainLength.  The same for 
jdk.tls.client.maxInboundCertificateChainLength.

To be clear, the release note or the comment might be placed in different code 
block like:


/* If jdk.tls.server.maxInboundCertificateChainLength is set, it will override 
jdk.tls.maxCertificateChainLength behavior for server side.
 */

Integer inboundClientLen = ...

/* If jdk.tls.client.maxInboundCertificateChainLength is set, it will override 
jdk.tls.maxCertificateChainLength behavior for client side.
 */
 Integer inboundServerLen = GetIntegerAction.privilegedGetProperty(



>  * If neither jdk.tls.server.maxInboundCertificateChainLength nor
>  * jdk.tls.client.maxInboundCertificateChainLength is set, the 
> behavior
>  * depends on the setting of jdk.tls.maxCertificateChainLength. If
>  * jdk.tls.maxCertificateChainLength is set, it falls back to that
>  * value; otherwise, it defaults to 8 for
>  * jdk.tls.server.maxInboundCertificateChainLength

Previously, the jdk.tls.maxCertificateChainLength default value is 10.  Now it 
is changed to 8.  This is a behavior change, please document it in release 
note, or just use 10 in the implementation.


>  * and 10 for jdk.tls.client.maxInboundCertificateChainLength.
>  * Usesrs can independently set either
>  * jdk.tls.server.maxInboundCertificateChainLength or
>  * jdk.tls.client.maxInboundCertificateChainLength.
>  */ 
> ```

-

PR Review Comment: https://git.openjdk.org/jdk/pull/15163#discussion_r1378477689

Re: RFR: 8311596: Add separate system properties for TLS server and client for maximum chain length [v6]

[Xue-Lei Andrew Fan]

On Fri, 20 Oct 2023 17:19:52 GMT, Xue-Lei Andrew Fan  wrote:

>> Hai-May Chao 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 six additional 
>> commits since the last revision:
>> 
>>  - Merge
>>  - Override the client/server defaults
>>  - Change made to configure max allowed cert chain lengths based on updated 
>> CSR
>>  - Merge
>>  - Set to default if a negative value is set
>>  - 8311596: Add separate system properties for TLS server and client for 
>> maximum chain length
>
> I was wondering, if it is easier to learn and remember/search by following 
> the naming style "jdk.tls.client.XXX" or "jdk.tls.server.XXX" in SunJSSE 
> provider?

> @XueleiFan The current properties named` jdk.tls.client.*` and 
> `jdk.tls.server.*` apply to settings either on the client or the server, so 
> we'd have to rename the properties here. My suggestion is to:
> 
> * Change `jdk.tls.maxServerCertificateChainLength` to 
> `jdk.tls.client.maxAcceptedCertificateChainLength`
> * Change `jdk.tls.maxClientCertificateChainLength` to 
> `jdk.tls.server.maxAcceptedCertificateChainLength`
> 
> Thanks!

For the name "jdk.tls.maxServerCertificateChainLength", it is not clear to me 
which side, client or server, the property should be applied to.   It could 
also mean that server can only send out certification with this limitation.

For the name `jdk.tls.client.maxAcceptedCertificateChainLength`, it could be 
confused to parse the word "accepted".  It could mean that the accepted 
cert-chain length for sending out certificates.

Maybe, you can have a try with 
"jdk.tls.client.maxServerCertificateChainLength", which means for client side, 
the server certificate chain length (inbound) is limited.  Or if you want to 
simplify the property name, you can have a try for 
""jdk.tls.client.maxInboundCertificateChainLength"".

-

PR Comment: https://git.openjdk.org/jdk/pull/15163#issuecomment-1779753163

Re: RFR: 8311596: Add separate system properties for TLS server and client for maximum chain length [v6]

[Xue-Lei Andrew Fan]

On Wed, 18 Oct 2023 00:25:02 GMT, Hai-May Chao  wrote:

>> Please review the enhancement for JDK-8311596 and its CSR JDK-8313236. Thank 
>> you.
>
> Hai-May Chao 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 six additional 
> commits since the last revision:
> 
>  - Merge
>  - Override the client/server defaults
>  - Change made to configure max allowed cert chain lengths based on updated 
> CSR
>  - Merge
>  - Set to default if a negative value is set
>  - 8311596: Add separate system properties for TLS server and client for 
> maximum chain length

I was wondering, if it is easier to learn and remember/search by following the 
naming style "jdk.tls.client.XXX" or "jdk.tls.server.XXX" in SunJSSE provider?

-

PR Comment: https://git.openjdk.org/jdk/pull/15163#issuecomment-1773113371

Integrated: 8312259: StatusResponseManager unused code clean up

[Xue-Lei Andrew Fan]

On Tue, 18 Jul 2023 17:43:42 GMT, Xue-Lei Andrew Fan  wrote:

> Hi,
> 
> May I have the code cleanup update reviewed?  With this update, the unused 
> code in StatusResponseManager.java will be removed.
> 
> Thanks,
> Xuelei

This pull request has now been integrated.

Changeset: 79be8d93
Author:    Xue-Lei Andrew Fan 
URL:   
https://git.openjdk.org/jdk/commit/79be8d9383c31be64e57ce1825a79dbbc2aefdd8
Stats: 173 lines in 2 files changed: 17 ins; 142 del; 14 mod

8312259: StatusResponseManager unused code clean up

Reviewed-by: mpowers, jnimeh

-

PR: https://git.openjdk.org/jdk/pull/14924

Re: RFR: 8312259: StatusResponseManager unused code clean up [v3]

[Xue-Lei Andrew Fan]

On Mon, 31 Jul 2023 18:03:22 GMT, Sean Mullan  wrote:

> I think @jnimeh should review this, as I think these methods were added when 
> implementing OCSP Stapling, and it would be good for him to make sure they 
> are no longer needed..

@jnimeh Did you have a chance to have a look at this update?

-

PR Comment: https://git.openjdk.org/jdk/pull/14924#issuecomment-1661296222

Re: RFR: 8312259: StatusResponseManager unused code clean up [v4]

[Xue-Lei Andrew Fan]

> Hi,
> 
> May I have the code cleanup update reviewed?  With this update, the unused 
> code in StatusResponseManager.java will be removed.
> 
> Thanks,
> Xuelei

Xue-Lei Andrew Fan has updated the pull request incrementally with one 
additional commit since the last revision:

  remove unintended blank line

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/14924/files
  - new: https://git.openjdk.org/jdk/pull/14924/files/cf894899..8a885e0b

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=14924=03
 - incr: https://webrevs.openjdk.org/?repo=jdk=14924=02-03

  Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod
  Patch: https://git.openjdk.org/jdk/pull/14924.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/14924/head:pull/14924

PR: https://git.openjdk.org/jdk/pull/14924

Re: RFR: 8312259: StatusResponseManager unused code clean up [v3]

[Xue-Lei Andrew Fan]

On Mon, 31 Jul 2023 17:46:22 GMT, Mark Powers  wrote:

>> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   revise test case
>
> test/jdk/sun/security/ssl/Stapling/java.base/sun/security/ssl/StatusResponseManagerTests.java
>  line 105:
> 
>> 103: }
>> 104: 
>> 105: 
> 
> Nit: added space

Thanks for the review.  I just removed the space.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14924#discussion_r1279753160

Re: RFR: 8312259: StatusResponseManager unused code clean up [v3]

[Xue-Lei Andrew Fan]

On Wed, 19 Jul 2023 04:35:56 GMT, Xue-Lei Andrew Fan  wrote:

>> Hi,
>> 
>> May I have the code cleanup update reviewed?  With this update, the unused 
>> code in StatusResponseManager.java will be removed.
>> 
>> Thanks,
>> Xuelei
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   revise test case

ping ...

-

PR Comment: https://git.openjdk.org/jdk/pull/14924#issuecomment-1658662392

Re: RFR: 8302017: Allocate BadPaddingException only if it will be thrown [v3]

[Xue-Lei Andrew Fan]

On Thu, 27 Jul 2023 20:35:42 GMT, Valerie Peng  wrote:

>> I checked back the specification back to RFC 2437, released on October 1998, 
>> which requires to encode NULL parameters as well.  As the update to keep the 
>> consistency is not trivial, I may just remove it and see if it could be a 
>> real problem in practice.
>
> Yes, let's try it and see.

Thank you.  I have no more comments.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14839#discussion_r1276811563

Re: RFR: 8302017: Allocate BadPaddingException only if it will be thrown [v4]

[Xue-Lei Andrew Fan]

On Thu, 27 Jul 2023 20:42:37 GMT, Valerie Peng  wrote:

>> This change refactors the RSAPadding class to return an output record 
>> containing the status instead of relying on exception object to indicate a 
>> failure.
>> 
>> Thanks in advance for review~
>> Valerie
>
> Valerie Peng has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Remove the changes for the null-omission fallback.

Marked as reviewed by xuelei (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/14839#pullrequestreview-1550760599

Re: RFR: 8312630: java/security should not create unmodifiable collections with redundant wrapping

[Xue-Lei Andrew Fan]

On Wed, 26 Jul 2023 06:11:56 GMT, Xue-Lei Andrew Fan  wrote:

>> Some java/security classes apply the below coding style,
>> 
>> Set set = ...;
>> Set unmodifiableSet = Collections.unmodifiableSet(new HashSet<>(set));
>> 
>> It may be unnecessary to wrap that `set` with HashSet before creating 
>> `unmodifiableSet`.
>> Some usages on `Collections.unmodifiableList` and 
>> `Collections.unmodifiableMap` have the same issue.
>
> Note: Please don't backport this update unless 
> [JDK-6323374](https://bugs.openjdk.org/browse/JDK-6323374) is backport as 
> well.

> @XueleiFan Thanks for your review and more infos!
> 
> I just dug a bit history and found 
> [JDK-8258514](https://bugs.openjdk.org/browse/JDK-8258514) applied 
> `List::copyOf` instead of `Collections::unmodifiableList`. Now, I also follow 
> this way and use `List/Set/Map::copyOf`.

The use of copyOf for the update is mainly because the 
Collections::unmodifiableList was not updated yet at that time.  It could be 
simpler to keep the behavior consistent by using Collections::unmodifiableList 
in this update.  Otherwise, more effect may be required to check if the use of 
copyOf could change the behaviors.

-

PR Comment: https://git.openjdk.org/jdk/pull/15008#issuecomment-1652370187

Re: RFR: 8312630: java/security should not create unmodifiable collections with redundant wrapping

[Xue-Lei Andrew Fan]

On Tue, 25 Jul 2023 06:27:25 GMT, John Jiang  wrote:

> Some java/security classes apply the below coding style,
> 
> Set set = ...;
> Set unmodifiableSet = Collections.unmodifiableSet(new HashSet<>(set));
> 
> It may be unnecessary to wrap that `set` with HashSet before creating 
> `unmodifiableSet`.
> Some usages on `Collections.unmodifiableList` and 
> `Collections.unmodifiableMap` have the same issue.

Marked as reviewed by xuelei (Reviewer).

Note: Please don't backport this update unless 
[JDK-6323374](https://bugs.openjdk.org/browse/JDK-6323374) is backport as well.

-

PR Review: https://git.openjdk.org/jdk/pull/15008#pullrequestreview-1546896379
PR Comment: https://git.openjdk.org/jdk/pull/15008#issuecomment-1651039887

Re: RFR: 8302017: Allocate BadPaddingException only if it will be thrown [v3]

[Xue-Lei Andrew Fan]

On Mon, 24 Jul 2023 19:58:34 GMT, Valerie Peng  wrote:

>> src/java.base/share/classes/sun/security/rsa/RSASignature.java line 227:
>> 
>>> 225: byte[] padded2 = padding.pad(encoded2);
>>> 226: return MessageDigest.isEqual(padded2, decrypted);
>>> 227: }
>> 
>> I had a check of the specification (Section A.2.4 of RFC 8017), and the 
>> [update](https://github.com/openjdk/jdk/pull/8365) and the [JBS 
>> entry](https://bugs.openjdk.org/browse/JDK-8285404) that added the comment 
>> "some vendors might omit the NULL params".
>> 
>> Per section A.2.4 of RFC 8017, it is said "For each OID, the parameters 
>> field associated with this OID in a value of type  AlgorithmIdentifier SHALL 
>> have a value of type NULL."
>> 
>> Per the key words specification, RFC 2119, "SHALL" is the same as MUST which 
>>  "mean that the definition is an absolute requirement of the specification."
>> 
>> In the bug description of bug JDK-8285404, there is a section "*Update*: We 
>> think it's possible that there might be signers omitting the NULL params in 
>> the digest algorithm identifier. "
>> 
>> For this case, if the signers omitting the NULL params, does it means the 
>> signer does not follow the specification and should be rejected?  @wangweij 
>> could you recall if there is a real case that omits the NULL params in 
>> practice?
>
> Max is on vacation and may not see your question for a while...
> IIRC, the inconsistency (NULL vs omission) goes way back. As time goes on, 
> this may no longer be an issue as spec is clarified and vendors update their 
> implementation.

I checked back the specification back to RFC 2437, released on October 1998, 
which requires to encode NULL parameters as well.  As the update to keep the 
consistency is not trivial, I may just remove it and see if it could be a real 
problem in practice.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14839#discussion_r1273015418

Re: RFR: 8312578: Redundant javadoc in X400Address

[Xue-Lei Andrew Fan]

On Mon, 24 Jul 2023 08:04:53 GMT, John Jiang  wrote:

> [JDK-8296741] removed the constructor `X400Address(byte[] value)`, but it 
> didn't remove the javadoc for this constructor.
> This simple patch just removes this javadoc.
> 
> [JDK-8296741]:
> 

Marked as reviewed by xuelei (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/14990#pullrequestreview-1543770745

Re: RFR: 8312443: sun.security should use toLowerCase(Locale.ROOT)

[Xue-Lei Andrew Fan]

On Thu, 20 Jul 2023 09:29:50 GMT, John Jiang  wrote:

> sun.security codes should use `toLowerCase(Locale.ROOT)` instead of 
> `toLowerCase()`.
> In addition, no `toUpperCase()` was found in this code scope.

Looks good to me.

-

Marked as reviewed by xuelei (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14948#pullrequestreview-1539584307

Re: RFR: 8302017: Allocate BadPaddingException only if it will be thrown [v3]

[Xue-Lei Andrew Fan]

On Thu, 20 Jul 2023 00:39:08 GMT, Valerie Peng  wrote:

>> This change refactors the RSAPadding class to return an output record 
>> containing the status instead of relying on exception object to indicate a 
>> failure.
>> 
>> Thanks in advance for review~
>> Valerie
>
> Valerie Peng has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Update to address review feedbacks

Let make sure the fall-back is really necessary before integration.  Thanks!

src/java.base/share/classes/sun/security/rsa/RSASignature.java line 227:

> 225: byte[] padded2 = padding.pad(encoded2);
> 226: return MessageDigest.isEqual(padded2, decrypted);
> 227: }

I had a check of the specification (Section A.2.4 of RFC 8017), and the 
[update](https://github.com/openjdk/jdk/pull/8365) and the [JBS 
entry](https://bugs.openjdk.org/browse/JDK-8285404) that added the comment 
"some vendors might omit the NULL params".

Per section A.2.4 of RFC 8017, it is said "For each OID, the parameters field 
associated with this OID in a value of type  AlgorithmIdentifier SHALL have a 
value of type NULL."

Per the key words specification, RFC 2119, "SHALL" is the same as MUST which  
"mean that the definition is an absolute requirement of the specification."

In the bug description of bug JDK-8285404, there is a section "*Update*: We 
think it's possible that there might be signers omitting the NULL params in the 
digest algorithm identifier. "

For this case, if the signers omitting the NULL params, does it means the 
signer does not follow the specification and should be rejected?  @wangweij 
could you recall if there is a real case that omits the NULL params in practice?

-

Changes requested by xuelei (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14839#pullrequestreview-1538434261
PR Review Comment: https://git.openjdk.org/jdk/pull/14839#discussion_r1268937806

Re: [jdk21] RFR: 8311902: Concurrency regression in the PBKDF2 key impl of SunJCE provider

[Xue-Lei Andrew Fan]

On Wed, 19 Jul 2023 17:51:20 GMT, Valerie Peng  wrote:

> 8311902: Concurrency regression in the PBKDF2 key impl of SunJCE provider

Marked as reviewed by xuelei (Reviewer).

src/java.base/share/classes/com/sun/crypto/provider/PBKDF2KeyImpl.java line 274:

> 272: }
> 273: 
> 274: SecretKey that = (SecretKey) obj;

is there a tab space?

-

PR Review: https://git.openjdk.org/jdk21/pull/139#pullrequestreview-1538015946
PR Review Comment: https://git.openjdk.org/jdk21/pull/139#discussion_r1268662947

Re: RFR: 8312259: StatusResponseManager unused code clean up [v3]

[Xue-Lei Andrew Fan]

> Hi,
> 
> May I have the code cleanup update reviewed?  With this update, the unused 
> code in StatusResponseManager.java will be removed.
> 
> Thanks,
> Xuelei

Xue-Lei Andrew Fan has updated the pull request incrementally with one 
additional commit since the last revision:

  revise test case

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/14924/files
  - new: https://git.openjdk.org/jdk/pull/14924/files/72879cff..cf894899

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=14924=02
 - incr: https://webrevs.openjdk.org/?repo=jdk=14924=01-02

  Stats: 31 lines in 1 file changed: 18 ins; 0 del; 13 mod
  Patch: https://git.openjdk.org/jdk/pull/14924.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/14924/head:pull/14924

PR: https://git.openjdk.org/jdk/pull/14924

Re: RFR: 8312259: StatusResponseManager unused code clean up [v2]

[Xue-Lei Andrew Fan]

On Tue, 18 Jul 2023 21:54:07 GMT, Mark Powers  wrote:

>> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   add final keyword back
>
> src/java.base/share/classes/sun/security/ssl/StatusResponseManager.java line 
> 584:
> 
>> 582: }
>> 583: 
>> 584: static StaplingParameters processStapling(
> 
> Even though a static method is implicitly final, the final keyword does 
> prevent subclasses from inadvertently using the same method signature.

Good point.  I got what you meant and will add the final keyword back.  Thanks!

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14924#discussion_r1267511186

Re: RFR: 8312259: StatusResponseManager unused code clean up [v2]

[Xue-Lei Andrew Fan]

> Hi,
> 
> May I have the code cleanup update reviewed?  With this update, the unused 
> code in StatusResponseManager.java will be removed.
> 
> Thanks,
> Xuelei

Xue-Lei Andrew Fan has updated the pull request incrementally with one 
additional commit since the last revision:

  add final keyword back

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/14924/files
  - new: https://git.openjdk.org/jdk/pull/14924/files/a122e594..72879cff

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=14924=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=14924=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/14924.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/14924/head:pull/14924

PR: https://git.openjdk.org/jdk/pull/14924

Re: RFR: 8312259: StatusResponseManager unused code clean up

[Xue-Lei Andrew Fan]

On Tue, 18 Jul 2023 21:54:07 GMT, Mark Powers  wrote:

>> Hi,
>> 
>> May I have the code cleanup update reviewed?  With this update, the unused 
>> code in StatusResponseManager.java will be removed.
>> 
>> Thanks,
>> Xuelei
>
> src/java.base/share/classes/sun/security/ssl/StatusResponseManager.java line 
> 584:
> 
>> 582: }
>> 583: 
>> 584: static StaplingParameters processStapling(
> 
> Even though a static method is implicitly final, the final keyword does 
> prevent subclasses from inadvertently using the same method signature.

Did you mean a subclass can declare a static method with the same signature?  I 
tried an example and the compiling failed.  Did I miss something?


class Base {
public static void display() {
System.out.println();
}

public void print() {
System.out.println();
}
}

class Derived extends Base {
//  public void display() {
 public static void display() {
System.out.println();
}

public void print() {
System.out.println();
}
}




XXX: error: display() in Derived cannot override display() in Base
public void display() {
^
  overridden method is static

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14924#discussion_r1267510020

RFR: 8312259: StatusResponseManager unused code clean up

[Xue-Lei Andrew Fan]

Hi,

May I have the code cleanup update reviewed?  With this update, the unused code 
in StatusResponseManager.java will be removed.

Thanks,
Xuelei

-

Commit messages:
 - 8312259: StatusResponseManager unused code clean up

Changes: https://git.openjdk.org/jdk/pull/14924/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk=14924=00
  Issue: https://bugs.openjdk.org/browse/JDK-8312259
  Stats: 144 lines in 1 file changed: 0 ins; 142 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/14924.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/14924/head:pull/14924

PR: https://git.openjdk.org/jdk/pull/14924

Re: RFR: 8302017: Allocate BadPaddingException only if it will be thrown [v2]

[Xue-Lei Andrew Fan]

On Thu, 13 Jul 2023 09:58:34 GMT, Ferenc Rakoczi  wrote:

>> src/java.base/share/classes/sun/security/rsa/RSASignature.java line 231:
>> 
>>> 229: RSAUtil.decodeSignature(digestOID, 
>>> unpadded));
>>> 230: }
>>> 231: }
>> 
>> I understand where the fallback code came from.  As the padding code is 
>> exactly the same as engineSign(), the risk may be minimal.  With the 
>> fallback code, the security concern (time-constant) we cared about will come 
>> back.  Did you run into testing failure without the fallback doe?
>
> Instead of falling back to unpad()/decodeSignature() I suggest to try a new 
> version of encodeSignature() (in addition to trying the current version of 
> it) in which you omit putting the null for params into the DER encoding and 
> compare the decrypted message with that, too. Accept if any of the two 
> encodings matches the decrypted one, reject otherwise. This can be done in 
> constant time, although it is not necessary to be constant time as the time 
> of doing it does not depend on any secret.

I'm OK with @ferakocz's suggestion, by avoiding the use of null params.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14839#discussion_r1267141110

Re: RFR: 8302017: Allocate BadPaddingException only if it will be thrown [v2]

[Xue-Lei Andrew Fan]

On Tue, 18 Jul 2023 10:46:52 GMT, Ferenc Rakoczi  wrote:

>> Yes, BadPaddingException is still thrown by RSACore class.
>
> That exception is thrown by RSACore.parseMsg() if the message is larger than 
> the modulus. I think it is at least debatable whether it should be a 
> BadPaddingException. I would rather use an IllegalArgumentException there.

I don't have a preference to change the BadPaddingException  to something else. 
 But if you want it, I may prefer to use checked exception, for example 
GeneralSecurityException,  so that the compiler could check it.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14839#discussion_r1267025300

Re: RFR: 8311902: Concurrency regression in the PBKDF2 key impl of SunJCE provider [v2]

[Xue-Lei Andrew Fan]

On Fri, 14 Jul 2023 21:57:32 GMT, Valerie Peng  wrote:

>> This change adds back the Reference.ReachabilityFence(Object) call removed 
>> by [JDK-8301553](https://bugs.openjdk.org/browse/JDK-8301553).
>> 
>> Please help review.
>> Thanks!
>> Valerie
>
> Valerie Peng has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Include more methods into the scope for consistency/correctness sake.

I have no more comments.  Thanks!

-

Marked as reviewed by xuelei (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14859#pullrequestreview-1531239782

Re: RFR: 8290005: com/sun/jndi/ldap/LdapCBPropertiesTest.java failling with NullPointerException [v6]

[Xue-Lei Andrew Fan]

On Fri, 14 Jul 2023 20:00:55 GMT, Matthew Donovan  wrote:

>> In this PR, I added methods to the TransportContext class to synchronize 
>> access to the handshakeContext field. I also updated locations in the code 
>> that rely on the handshakeContext field to not be null to use the 
>> synchronized methods.
>> 
>> Thanks
>
> Matthew Donovan 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 10 additional 
> commits since the last revision:
> 
>  - Merge branch 'master' into socket-close-null
>  - Removed changes in TransportContext; changed SSLSocketImpl to use 
> conContext.protocolVersion during close operation
>  - Merge branch 'master' into socket-close-null
>  - Merge branch 'master' into socket-close-null
>  - reverted changes in SSLEngineImpl
>  - I missed a method that needed updating
>  - Merge branch 'master' into socket-close-null
>  - made handshake context lock final
>  - using try/finally in terminateHandshakeContext and using local context 
> variable in all places it should be
>  - 8290005: com/sun/jndi/ldap/LdapCBPropertiesTest.java failling with 
> NullPointerException

Looks good to me.  Thank you!

-

Marked as reviewed by xuelei (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13742#pullrequestreview-1531045819

Re: RFR: 8290005: com/sun/jndi/ldap/LdapCBPropertiesTest.java failling with NullPointerException [v2]

[Xue-Lei Andrew Fan]

On Fri, 14 Jul 2023 14:16:00 GMT, Matthew Donovan  wrote:

> TransportContext also has a `protocolVersion` field. Is it possible to just 
> use that instead?

Did you mean a change in duplexCloseOutput() like the following?


-   // The protocol version may have been negotiated.
-   ProtocolVersion pv = conContext.handshakeContext.negotiatedProtocol;
+   // The protocol version may have been negotiated.  The 
+   // conContext.handshakeContext.negotiatedProtocol is not used as 
there
+   // may be a race to set it to null.
+  ProtocolVersion pv = conContext.protocolVersion;
if (pv == null || (!pv.useTLS13PlusSpec())) {
hasCloseReceipt = true;
}   


I like the idea as it looks like a safe update in the current implementation.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13742#discussion_r1263934419

Re: RFR: 8311902: Concurrency regression in the PBKDF2 key impl of SunJCE provider

[Xue-Lei Andrew Fan]

On Fri, 14 Jul 2023 02:37:09 GMT, Xue-Lei Andrew Fan  wrote:

> > > It looks good to me to rollback to previous behaviors. I was just 
> > > wondering, if the use of key in other methods, like hashCode()/equals(), 
> > > has the similar issue? Thanks!
> > 
> > 
> > For the usage of hashCode()/equals(), there should be strong reference to 
> > the key object for the usage scenarios I'd think. Thus, probably not an 
> > issue?
> 
> Yes, it makes sense to me.

I may reply too quickly to get the idea.  Why you think there will be strong 
reference to the key object for hashCode()/equals(), but not for getEncoded()?  
I did not get the idea.

-

PR Comment: https://git.openjdk.org/jdk/pull/14859#issuecomment-1635198764

Re: RFR: 8311902: Concurrency regression in the PBKDF2 key impl of SunJCE provider

[Xue-Lei Andrew Fan]

On Thu, 13 Jul 2023 21:01:06 GMT, Valerie Peng  wrote:

> > It looks good to me to rollback to previous behaviors. I was just 
> > wondering, if the use of key in other methods, like hashCode()/equals(), 
> > has the similar issue? Thanks!
> 
> For the usage of hashCode()/equals(), there should be strong reference to the 
> key object for the usage scenarios I'd think. Thus, probably not an issue?

Yes, it makes sense to me.

-

PR Comment: https://git.openjdk.org/jdk/pull/14859#issuecomment-1635180918

Re: RFR: 8290005: com/sun/jndi/ldap/LdapCBPropertiesTest.java failling with NullPointerException [v2]

[Xue-Lei Andrew Fan]

On Thu, 13 Jul 2023 12:11:09 GMT, Matthew Donovan  wrote:

>> I reverted the changes to SSLEngineImpl and looked at the history of 
>> SSLSocketImpl and TransportContext but didn't see anything that I would be 
>> undoing with this change.
>
> Sorry for the delay on any updates here. 
> 
> I updated this branch and verified the tests still pass. I ran jdk_security3 
> tests from test/jdk/TEST.groups. Is there anything else I should do to test 
> this change?

SSLSocketImpl uses the locks similar to SSLEngineImpl.  It may get it 
complicated and hard to maintain if using different locks in SSLSocketImpl and 
SSLEngineImpl.  You may be able to find similar locking scenarios in SSLSocket 
implementation like: 
SSLEngineImpl.DelegatedTask.run()->TransportContext.dispatch()->SSLHandshake().consume()->HandshakeConsumer.consume()->SSLExtensions.product()->ServerHello.produce()->shc.handshakeSession
 = session.

I understand where you came from for the NullPointerException in the bug.  But 
it is hardly the direction to change the overall locking scenarios.  Maybe, the 
close() function implementation could be focused on instead.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13742#discussion_r1262802858

Re: RFR: 8302017: Allocate BadPaddingException only if it will be thrown [v2]

[Xue-Lei Andrew Fan]

On Wed, 12 Jul 2023 23:12:18 GMT, Valerie Peng  wrote:

>> This change refactors the RSAPadding class to return an output record 
>> containing the status instead of relying on exception object to indicate a 
>> failure.
>> 
>> Thanks in advance for review~
>> Valerie
>
> Valerie Peng has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Address review feedbacks, e.g. Removed RSAPadding.Output and use byte[] as 
> before.

Thank you for the update.

src/java.base/share/classes/sun/security/rsa/RSASignature.java line 196:

> 194: return RSACore.rsa(padded, privateKey, true);
> 195: }
> 196: throw new SignatureException("Could not sign data");

It may be clearer if the throw line is moved to the end of the method.  
Otherwise, I have to check if SignatureException is a sub-class of 
GeneralSecurityException.

src/java.base/share/classes/sun/security/rsa/RSASignature.java line 231:

> 229: RSAUtil.decodeSignature(digestOID, 
> unpadded));
> 230: }
> 231: }

I understand where the fallback code came from.  As the padding code is exactly 
the same as engineSign(), the risk may be minimal.  With the fallback code, the 
security concern (time-constant) we cared about will come back.  Did you run 
into testing failure without the fallback doe?

-

PR Review: https://git.openjdk.org/jdk/pull/14839#pullrequestreview-1527547950
PR Review Comment: https://git.openjdk.org/jdk/pull/14839#discussion_r1261964861
PR Review Comment: https://git.openjdk.org/jdk/pull/14839#discussion_r1261970478

Re: RFR: 8311902: Concurrency regression in the PBKDF2 key impl of SunJCE provider

[Xue-Lei Andrew Fan]

On Thu, 13 Jul 2023 02:46:10 GMT, Valerie Peng  wrote:

> This change adds back the Reference.ReachabilityFence(Object) call removed by 
> [JDK-8301553](https://bugs.openjdk.org/browse/JDK-8301553).
> 
> Please help review.
> Thanks!
> Valerie

It looks good to me to rollback to previous behaviors.  I was just wondering, 
if the use of key in other methods, like hashCode()/equals(), has the similar 
issue?  Thanks!

-

Marked as reviewed by xuelei (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14859#pullrequestreview-1527542265

Re: RFR: 8302017: Allocate BadPaddingException only if it will be thrown

[Xue-Lei Andrew Fan]

On Tue, 11 Jul 2023 23:19:40 GMT, Valerie Peng  wrote:

> This change refactors the RSAPadding class to return an output record 
> containing the status instead of relying on exception object to indicate a 
> failure.
> 
> Thanks in advance for review~
> Valerie

Changes requested by xuelei (Reviewer).

src/java.base/share/classes/sun/security/rsa/RSAPadding.java line 372:

> 370: return Output.FAIL;
> 371: } else {
> 372: return Output.pass(data);

The Output.pass(byte[]) will create a new instance and thus make the behavior 
detectable.  Maybe, the Output class is not necessary, 'null' value return 
could be used instead.


- if (bp) {
- return Output.FAIL;
- } else {
- return Output.pass(data);
+ return bp ? null : data;

src/java.base/share/classes/sun/security/rsa/RSASignature.java line 217:

> 215: byte[] digest = getDigestValue();
> 216: byte[] decrypted = RSACore.rsa(sigBytes, publicKey);
> 217: RSAPadding.Output po = padding.unpad(decrypted);

In case you are already here, what if comparing the padded/encoded result, 
without use unpad() any longer? I meant to follow the spec as described in 
RFC8017#section-8.2.2: encode the `decryped` bytes and then compare the result 
with the `digest` bytes.

-

PR Review: https://git.openjdk.org/jdk/pull/14839#pullrequestreview-1526794400
PR Review Comment: https://git.openjdk.org/jdk/pull/14839#discussion_r1261451456
PR Review Comment: https://git.openjdk.org/jdk/pull/14839#discussion_r1261463911

Re: RFR: 8295894: Remove SECOM certificate that is expiring in September 2023

[Xue-Lei Andrew Fan]

On Tue, 11 Jul 2023 20:42:14 GMT, Rajan Halade  wrote:

> The fix is to remove the expiring SECOM root certificate after approval from 
> root CA to remove it.
> 
> Release note is at - https://bugs.openjdk.org/browse/JDK-8311884

Marked as reviewed by xuelei (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/14838#pullrequestreview-1525157343

Re: RFR: 8295068: SSLEngine throws NPE parsing CertificateRequests [v2]

[Xue-Lei Andrew Fan]

On Thu, 6 Jul 2023 15:52:00 GMT, Kevin Driver  wrote:

>> JDK-8295068: an NPE is thrown when an invalid `id` is found to match up a 
>> `ClientCertificateType`; rather than throwing the `NPE`, we now ignore an 
>> `id` which does match a `ClientCertificateType`.
>
> Kevin Driver has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - remove extra line break
>  - fail to look up the id silently to not throw an unexpected error

It looks good to me.  Thank you for the update.

-

Marked as reviewed by xuelei (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14778#pullrequestreview-1516909264

Re: RFR: 8295068: SSLEngine throws NPE parsing CertificateRequests

[Xue-Lei Andrew Fan]

On Wed, 5 Jul 2023 20:25:26 GMT, Kevin Driver  wrote:

> JDK-8295068: an NPE is thrown when an invalid `id` is found to match up a 
> `ClientCertificateType`; rather than throwing the `NPE`, we now throw an 
> `IllegalArgumentException`. This does not seem to be a scenario where 
> recovery is possible or desired, so the `IAE` should be the proper behavior.

Changes requested by xuelei (Reviewer).

src/java.base/share/classes/sun/security/ssl/CertificateRequest.java line 134:

> 132: throw new IllegalArgumentException(id + " was " +
> 133: "not a valid ClientCertificateType id");
> 134: }

It may not comply to TLS specification if throwing exception here.  Unknown 
types should be ignored for compatibility.

- if (cct.isAvailable) {
+ if (cct != null && cct.isAvailable) {

-

PR Review: https://git.openjdk.org/jdk/pull/14778#pullrequestreview-1515892219
PR Review Comment: https://git.openjdk.org/jdk/pull/14778#discussion_r1254023615

Re: RFR: 8308540: On Kerberos TGT referral, if krb5.conf is missing realm, bad exception message

[Xue-Lei Andrew Fan]

On Mon, 22 May 2023 15:03:15 GMT, Weijun Wang  wrote:

> Add realm name to the exception message, and make it the primary exception 
> (retry exception added suppressed).

Marked as reviewed by xuelei (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/14086#pullrequestreview-1498866747

Re: [jdk21] RFR: 8309740: Expand timeout windows for tests in JDK-8179502

[Xue-Lei Andrew Fan]

On Fri, 23 Jun 2023 14:55:45 GMT, Jamil Nimeh  wrote:

> This is a backport of the test fixes comprising JDK-8309740.

Looks good to me.  I was just wondering, if the /backport tag could work from 
the original PR for backporting.

-

Marked as reviewed by xuelei (Reviewer).

PR Review: https://git.openjdk.org/jdk21/pull/58#pullrequestreview-1495698832

Re: RFR: 8309740: Expand timeout windows for tests in JDK-8179502

[Xue-Lei Andrew Fan]

On Fri, 16 Jun 2023 18:19:45 GMT, Jamil Nimeh  wrote:

> This PR is for tests that were modified/added in JDK-8179502.  The timeout 
> windows for those tests were a little too short on some test systems, 
> especially when the system is under heavy load.  After a few iterations 
> trying out various longer time windows I have a set that should not run into 
> issues.

Marked as reviewed by xuelei (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/14526#pullrequestreview-1484122264

Re: RFR: 8290005: com/sun/jndi/ldap/LdapCBPropertiesTest.java failling with NullPointerException [v4]

[Xue-Lei Andrew Fan]

On Fri, 16 Jun 2023 14:45:52 GMT, Matthew Donovan  wrote:

>> I may update both SSLSocketImpl and SSLEngineImpl, as SSLSocketImpl uses the 
>> locks similar to SSLEngineImpl.
>
> There is a difference, though. The close() method in SSLSocketImpl is not 
> synchronized and there is the chance of a NullPointerException in 
> duplexCloseOutput() because `conContext.handshakeContext` is being set to 
> null by the TransportContext class in a different thread. I think that can 
> affect any method in SSLSocketImpl that accesses that field.

Yes, socket close is a headache problem for me.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13742#discussion_r1232359603

Re: RFR: 8290005: com/sun/jndi/ldap/LdapCBPropertiesTest.java failling with NullPointerException [v4]

[Xue-Lei Andrew Fan]

On Fri, 16 Jun 2023 14:24:17 GMT, Matthew Donovan  wrote:

>> I had a search of the value assignment of handshakeSession.  Here is one 
>> example of the calling stack:
>> SSLEngineImpl.DelegatedTask.run()->TransportContext.dispatch()->SSLHandshake().consume()->HandshakeConsumer.consume()->SSLExtensions.product()->ServerHello.produce()->shc.handshakeSession
>>  = session
>> 
>> The engineLock was placed on SSLEngineImpl.DelegatedTask.run() and released 
>> after complete the job, which means the value assignment of handshakeSession 
>> is synchronized.
>> 
>> The locks in SSLEngineImpl and SSLSocketImpl are used for operations 
>> synchronization so that other classes may not need additional locks in most 
>> cases.
>
> I see what you're saying with respect to SSLEngineImpl. It looks like all of 
> the public methods are synchronized with `engineLock` so I shouldn't have 
> made any changes there. I will revert them.
> 
> Are you ok with the changes in SSLSocketImpl?

I may update both SSLSocketImpl and SSLEngineImpl, as SSLSocketImpl uses the 
locks similar to SSLEngineImpl.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13742#discussion_r1232331470

Re: RFR: 8290005: com/sun/jndi/ldap/LdapCBPropertiesTest.java failling with NullPointerException [v4]

[Xue-Lei Andrew Fan]

On Fri, 16 Jun 2023 11:59:01 GMT, Matthew Donovan  wrote:

>> src/java.base/share/classes/sun/security/ssl/SSLEngineImpl.java line 914:
>> 
>>> 912: @Override
>>> 913: public SSLSession getHandshakeSession() {
>>> 914: engineLock.lock();
>> 
>> I'm not very sure of this update. By removing the enginLock on socket/engine 
>> level here,  is it possible there are two threads that one read the 
>> handshake session and another on write?
>
> `engineLock` was used to here to make sure that `conContext.handshakeContext` 
> is not null before accessing it. The problem is that the `handshakeContext` 
> is modified (set to null) by the `TransportContext` class. So using a lock in 
> SSLEngineImpl or SSLSocketImpl doesn't protect `handshakeContext` from 
> becoming null between the check and use.
> 
> The actual SSLSession object was never protected from concurrent modification 
> (unless it has its own lock.)
> 
> When reviewing the code to answer this, I noticed that I needed to update 
> `SSLEngineImpl.getHandshakeApplicationProtocol()` so there is a new commit.

I had a search of the value assignment of handshakeSession.  Here is one 
example of the calling stack:
SSLEngineImpl.DelegatedTask.run()->TransportContext.dispatch()->SSLHandshake().consume()->HandshakeConsumer.consume()->SSLExtensions.product()->ServerHello.produce()->shc.handshakeSession
 = session

The engineLock was placed on SSLEngineImpl.DelegatedTask.run() and released 
after complete the job, which means the value assignment of handshakeSession is 
synchronized.

The locks in SSLEngineImpl and SSLSocketImpl are used for operations 
synchronization so that other classes may not need additional locks in most 
cases.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13742#discussion_r1232286405

Re: RFR: 8290005: com/sun/jndi/ldap/LdapCBPropertiesTest.java failling with NullPointerException [v3]

[Xue-Lei Andrew Fan]

On Wed, 3 May 2023 11:26:32 GMT, Matthew Donovan  wrote:

>> In this PR, I added methods to the TransportContext class to synchronize 
>> access to the handshakeContext field. I also updated locations in the code 
>> that rely on the handshakeContext field to not be null to use the 
>> synchronized methods.
>> 
>> Thanks
>
> Matthew Donovan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   made handshake context lock final

src/java.base/share/classes/sun/security/ssl/SSLEngineImpl.java line 914:

> 912: @Override
> 913: public SSLSession getHandshakeSession() {
> 914: engineLock.lock();

I'm not very sure of this update. By removing the enginLock on socket/engine 
level here,  is it possible there are two threads that one read the handshake 
session and another on write?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13742#discussion_r1231827430

Re: RFR: 8310106: sun.security.ssl.SSLHandshake.getHandshakeProducer() incorrectly checks handshakeConsumers

[Xue-Lei Andrew Fan]

On Thu, 15 Jun 2023 06:02:13 GMT, Jaikiran Pai  wrote:

> Can I please get a review of this change which fixes the issue noted in 
> https://bugs.openjdk.org/browse/JDK-8310106?
> 
> The commit here fixes the typo in 
> `sun.security.ssl.SSLHandshake.getHandshakeProducer()` method to correctly 
> use `handshakeProducers`.
> 
> I checked if a new jtreg test could be added for this, but given where this 
> change resides and the nature of this change, I don't see a way to add one. 
> Existing tests in `tier1`, `tier2` and `tier3` continue to pass with this 
> change.

Nice catch.  Thanks!

-

Marked as reviewed by xuelei (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14483#pullrequestreview-1482644422

Re: RFR: 8309667: TLS handshake fails because of ConcurrentModificationException in PKCS12KeyStore.engineGetEntry

[Xue-Lei Andrew Fan]

On Fri, 16 Jun 2023 01:21:57 GMT, Weijun Wang  wrote:

> The `attributes` field inside the `PKCS12KeyStore.Entry` class might be 
> modified and retrieved at the same time. Make it concurrent.
> 
> The test uses some reflection to get this field and try updating it in 
> multiple threads.

Nice catch!

-

Marked as reviewed by xuelei (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14506#pullrequestreview-1482638205

Integrated: 8309867: redundant class field RSAPadding.md

[Xue-Lei Andrew Fan]

On Mon, 12 Jun 2023 16:39:33 GMT, Xue-Lei Andrew Fan  wrote:

> Hi,
> 
> May I get this simple update reviewed?
> 
> The class field RSAPadding.md can be converted to a local variable of the 
> constructor, and save the class footprint.
> 
> Thanks,
> Xuelei

This pull request has now been integrated.

Changeset: 80a8144a
Author:Xue-Lei Andrew Fan 
URL:   
https://git.openjdk.org/jdk/commit/80a8144af5aae104188de9cc182e6d59c1466732
Stats: 6 lines in 1 file changed: 2 ins; 3 del; 1 mod

8309867: redundant class field RSAPadding.md

Reviewed-by: hchao, weijun, valeriep

-

PR: https://git.openjdk.org/jdk/pull/14422

Re: RFR: 8309867: redundant class field RSAPadding.md

[Xue-Lei Andrew Fan]

On Mon, 12 Jun 2023 17:53:42 GMT, Hai-May Chao  wrote:

>> Hi,
>> 
>> May I get this simple update reviewed?
>> 
>> The class field RSAPadding.md can be converted to a local variable of the 
>> constructor, and save the class footprint.
>> 
>> Thanks,
>> Xuelei
>
> LGTM

Thank you @haimaychao and @wangweij!

-

PR Comment: https://git.openjdk.org/jdk/pull/14422#issuecomment-1587844728

RFR: 8309867: redundant class field RSAPadding.md

[Xue-Lei Andrew Fan]

Hi,

May I get this simple update reviewed?

The class field RSAPadding.md can be converted to a local variable of the 
constructor, and save the class footprint.

Thanks,
Xuelei

-

Commit messages:
 - 8309867: redundant class field RSAPadding.md

Changes: https://git.openjdk.org/jdk/pull/14422/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk=14422=00
  Issue: https://bugs.openjdk.org/browse/JDK-8309867
  Stats: 6 lines in 1 file changed: 2 ins; 3 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/14422.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/14422/head:pull/14422

PR: https://git.openjdk.org/jdk/pull/14422

Re: RFR: 8307144: namedParams in XECParameters and EdDSAParameters can be private final

[Xue-Lei Andrew Fan]

On Thu, 25 May 2023 21:17:40 GMT, Ben Perez  wrote:

> Changed `namedParams` in XECParameters and EdDSAParameters to be `private 
> final`

Marked as reviewed by xuelei (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/14162#pullrequestreview-1466696530

Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v25]

[Xue-Lei Andrew Fan]

On Tue, 30 May 2023 19:24:09 GMT, Kevin Driver  wrote:

>> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985)
>
> Kevin Driver has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - undo import changes
>  - undo import changes

My concerns for tests were addressed.  Thanks!

-

Marked as reviewed by xuelei (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13466#pullrequestreview-1451732655

Re: RFR: 8297885: misc sun/security/pkcs11 tests timed out

[Xue-Lei Andrew Fan]

On Sat, 27 May 2023 01:49:22 GMT, Valerie Peng  wrote:

> Increase the timeout value of the "LargeDSAKey,java" test to address the 
> intermittent test failure(s). As for the other mentioned test failures, the 
> execution time fluctuates even less, so leaving them alone for now.
> 
> Thanks in advance for the review~

Marked as reviewed by xuelei (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/14190#pullrequestreview-1447129035

Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v19]

[Xue-Lei Andrew Fan]

On Mon, 22 May 2023 19:41:25 GMT, Kevin Driver  wrote:

>> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985)
>
> Kevin Driver 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 17 additional 
> commits since the last revision:
> 
>  - Merge remote-tracking branch 'upstream/master' into JDK-8294985
>  - additional code review comments
>  - rename class and remove bug id from test header
>  - removing block that isn't reached
>  - fix bug id in test header
>  - reworked example into a jtreg test
>  - whitespace adjustments
>  - all review comments applied
>  - optimize imports and change toString
>  - review comments addressed
>  - ... and 7 more: https://git.openjdk.org/jdk/compare/1950bd92...d9f0c667

Changes requested by xuelei (Reviewer).

test/jdk/sun/security/ssl/SSLEngineImpl/TestBadDNForPeerCA.java line 29:

> 27:  * @summary verify correct exception handling in the event of an 
> unparseable
> 28:  *  DN in the peer CA
> 29:  */

Please run tests for JSSE/TLS in othervm mode.  Otherwise, the behavior may be 
impacted with each other, and the failure debugging is pretty hard.

-

PR Review: https://git.openjdk.org/jdk/pull/13466#pullrequestreview-1440319320
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1202812974

Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v19]

[Xue-Lei Andrew Fan]

On Tue, 23 May 2023 16:48:52 GMT, Kevin Driver  wrote:

>> test/jdk/sun/security/ssl/SSLEngineImpl/TestBadDNForPeerCA.java line 27:
>> 
>>> 25:  * @test
>>> 26:  * @library /test/lib
>>> 27:  * @summary verify correct exception handling in the event of an 
>>> unparseable
>> 
>> Missing @bug field.
>
> I was asked to remove it previously. I can add it back. Just looking for a 
> consistent POV here.

Generally, the @bug tag should be there to easy maintenance.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1202809318

Re: RFR: 8301381: Verify DTLS 1.0 cannot be negotiated

[Xue-Lei Andrew Fan]

On Fri, 19 May 2023 12:03:54 GMT, Matthew Donovan  wrote:

> This PR implements a test to verify that a DTLS server running "out of the 
> box" (i.e., DTLSv1.0 disabled in java.security) will not handshake with a 
> client requesting DTLSv1.0. The test also implements the opposite: a client 
> won't handshake with a server that uses DTLSv1.0.

Marked as reviewed by xuelei (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/14059#pullrequestreview-1434532784

Re: RFR: 8305091: Change ChaCha20 cipher init behavior to match AES-GCM

[Xue-Lei Andrew Fan]

On Thu, 18 May 2023 18:16:49 GMT, Daniel Jeliński  wrote:

> Here you go:

@djelinski Thank you!

-

PR Comment: https://git.openjdk.org/jdk/pull/13428#issuecomment-1553459091

Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v13]

[Xue-Lei Andrew Fan]

On Thu, 18 May 2023 17:49:07 GMT, Kevin Driver  wrote:

>> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985)
>
> Kevin Driver has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   all review comments applied

Marked as reviewed by xuelei (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/13466#pullrequestreview-1433195956

Re: RFR: 8305091: Change ChaCha20 cipher init behavior to match AES-GCM

[Xue-Lei Andrew Fan]

On Thu, 18 May 2023 16:55:05 GMT, Daniel Jeliński  wrote:

> the QUIC specification permits dropping duplicate packets only after fully 
> decrypting them.

May I have a reference, for example the section number, of the specification?

-

PR Comment: https://git.openjdk.org/jdk/pull/13428#issuecomment-1553425386

Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v11]

[Xue-Lei Andrew Fan]

On Thu, 18 May 2023 16:15:39 GMT, Kevin Driver  wrote:

>> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985)
>
> Kevin Driver has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   review comments addressed

src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java
 line 290:

> 288: shc.peerSupportedAuthorities = spec.getAuthorities();
> 289: } catch (IllegalArgumentException iae) {
> 290: shc.conContext.fatal(Alert.DECODE_ERROR, "X500Principal 
> could not be parsed", iae);

In the context, it may be easier to catch the idea if the message is about the 
authorities, and easier to update getAuthorities() implementation, for example 
X500Principal is not used any longer, if needed in the future.

- "X500Principal could not be parsed"
+ "Peer authorities could not be parsed"

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1198051462

Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v11]

[Xue-Lei Andrew Fan]

On Thu, 18 May 2023 16:15:39 GMT, Kevin Driver  wrote:

>> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985)
>
> Kevin Driver has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   review comments addressed

src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java
 line 148:

> 146: builder.append(principal.toString());
> 147: } catch (IllegalArgumentException iae) {
> 148: builder.append("unparseable X500Principal: " + 
> iae.getMessage());

Throwable.getMessage() may return `null`.  I may use iae.toString() instead.

`builder.append("unparseable X500Principal: " + iae);`

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1198044980

Re: RFR: 8308118: Avoid multiarray allocations in AESCrypt.makeSessionKey [v2]

[Xue-Lei Andrew Fan]

On Thu, 18 May 2023 07:18:04 GMT, Aleksey Shipilev  wrote:

> > jdk_security or tier2.
> 
> Gotcha, I already tested both, see "Additional Testing" section in PR.

Thanks!

-

PR Comment: https://git.openjdk.org/jdk/pull/13996#issuecomment-1553126729

Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v10]

[Xue-Lei Andrew Fan]

On Thu, 18 May 2023 04:04:17 GMT, Xue-Lei Andrew Fan  wrote:

>> Kevin Driver has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   rework based upon code review comments
>
> src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java
>  line 289:
> 
>> 287: shc.peerSupportedAuthorities = spec.getAuthorities();
>> 288: } catch (IllegalArgumentException iae) {
>> 289: shc.conContext.fatal(Alert.DECODE_ERROR, "X500Principal 
>> could not be parsed");
> 
> To easy debugging, I may use the exception with fatal(Alert alert, String 
> diagnostic, Throwable cause).  Considering this point, I may prefer to throw 
> SSLException in getAuthorities() method.
> 
> 
> X500Principal[] getAuthorities() throws SSLException {
> ...
> try {
> shc.peerSupportedAuthorities = spec.getAuthorities();
> } catch (SSLException ssle) {
> shc.conContext.fatal(Alert.DECODE_ERROR, "Cannot parse peer supported 
> authorities", ssle);
> }

> @XueleiFan Seems like an unnecessary wrapping of IAE when it is going to be 
> rethrown anyway.

I'm fine if IAE get checked for every call to getAuthorities().

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1197881532

Re: RFR: 8308118: Avoid multiarray allocations in AESCrypt.makeSessionKey [v2]

[Xue-Lei Andrew Fan]

On Thu, 18 May 2023 06:44:10 GMT, Aleksey Shipilev  wrote:

> > Looks good to me. Please make sure the security regression testing passed.
> 
> Thanks! By "security regression testing" that you mean `jdk_security`, or 
> something else?

jdk_security or tier2.

-

PR Comment: https://git.openjdk.org/jdk/pull/13996#issuecomment-1552616281

Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v10]

[Xue-Lei Andrew Fan]

On Wed, 17 May 2023 21:54:20 GMT, Kevin Driver  wrote:

>> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985)
>
> Kevin Driver has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   rework based upon code review comments

Similar comments for update in CertificateRequest.java

Similar comments for update in CertificateRequest.java

src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java
 line 126:

> 124: }
> 125: 
> 126: X500Principal[] getAuthorities() throws IllegalArgumentException 
> {

IAE is unchecked exception, and should not be throwing explicitly in method 
signature/statement.  I'm not sure if this throwing is really helpful for 
caller to check the exception.

src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java
 line 147:

> 145: builder.append(principal.toString());
> 146: } catch (IllegalArgumentException iae) {
> 147: builder.append("unparseable X500Principal");

I may use the iae message as well for better debugging.

src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java
 line 289:

> 287: shc.peerSupportedAuthorities = spec.getAuthorities();
> 288: } catch (IllegalArgumentException iae) {
> 289: shc.conContext.fatal(Alert.DECODE_ERROR, "X500Principal 
> could not be parsed");

To easy debugging, I may use the exception with fatal(Alert alert, String 
diagnostic, Throwable cause).  Considering this point, I may prefer to throw 
SSLException in getAuthorities() method.


X500Principal[] getAuthorities() throws SSLException {
...
try {
shc.peerSupportedAuthorities = spec.getAuthorities();
} catch (SSLException ssle) {
shc.conContext.fatal(Alert.DECODE_ERROR, "Cannot parse peer supported 
authorities", ssle);
}

-

Changes requested by xuelei (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13466#pullrequestreview-1431961001
PR Review: https://git.openjdk.org/jdk/pull/13466#pullrequestreview-1431970382
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1197326576
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1197327083
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1197334523

Re: RFR: 8308118: Avoid multiarray allocations in AESCrypt.makeSessionKey [v2]

[Xue-Lei Andrew Fan]

On Tue, 16 May 2023 09:18:57 GMT, Aleksey Shipilev  wrote:

>> One of our services has a hot path with AES/GCM cipher reuse. The JDK code 
>> reinitializes the session key on that path, and 
>> [JDK-8308105](https://bugs.openjdk.org/browse/JDK-8308105) shows up 
>> prominently there.
>> 
>> Fixing [JDK-8308105](https://bugs.openjdk.org/browse/JDK-8308105) would take 
>> a while, as would likely require multiple patches in VM internals. 
>> Meanwhile, we can avoid the multiarray allocations in 
>> AESCrypt.makeSessionKey completely, reaping performance benefits. We can go 
>> even deeper: replace the multi-array with the flat array and drop 
>> `expandToSubKey` completely.
>> 
>> Example original profile is in the bug.
>> 
>> There are other things we can polish in that code, but experiments show 
>> those polishings have rather diminshed returns.
>> 
>> On new benchmark:
>> 
>> 
>> Benchmark   Mode  CntScore   Error  Units
>> 
>> ## Mac M1
>> 
>> # Before
>> AESReinit.test  avgt   15   873,842 ± 6,911  ns/op
>> 
>> # After
>> AESReinit.test  avgt   15   347,632 ± 8,764  ns/op  ; <--- 2.5x faster
>> 
>> ## Xeon, c6.8xlarge
>> 
>> # Before
>> AESReinit.test  avgt   15  1524.307 ± 24.231 ns/op
>> 
>> # After
>> AESReinit.test  avgt   15   554.727 ± 12.876 ns/op  ; <--- 2.75x faster
>> 
>> ## Graviton, m6g.4xlarge
>> 
>> # Before
>> AESReinit.test  avgt   15  1913.492 ± 23.489 ns/op
>> 
>> # After
>> AESReinit.test  avgt   15   639.701 ± 5.033  ns/op  ; <--- 2.99x faster
>> 
>> 
>> Additional testing:
>>  - [x] Benchmarks
>>  - [x] macos-aarch64-server-release, `jdk_security`
>>  - [x] linux-x86_64-server-fastdebug, `tier1 tier2 tier3`
>
> Aleksey Shipilev has updated the pull request incrementally with three 
> additional commits since the last revision:
> 
>  - Micro-optimizations
>  - Handle Kd
>  - Handle Ke

Looks good to me.  Please make sure the security regression testing passed.

-

Marked as reviewed by xuelei (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13996#pullrequestreview-1431510226

Re: RFR: 8308118: Avoid multiarray allocations in AESCrypt.makeSessionKey [v2]

[Xue-Lei Andrew Fan]

On Wed, 17 May 2023 12:57:15 GMT, Aleksey Shipilev  wrote:

> @XueleiFan, or anyone else, please take a look?

I will have a look, but I may need more time.

-

PR Comment: https://git.openjdk.org/jdk/pull/13996#issuecomment-1551895053

Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v6]

[Xue-Lei Andrew Fan]

On Fri, 12 May 2023 20:29:47 GMT, Kevin Driver  wrote:

>> Do you have any plans to write a test? If not, the bug needs a `noreg` label.
>
>> Do you have any plans to write a test? If not, the bug needs a `noreg` label.
> 
> As discussed internally, the test that surfaced this issue will be 
> incorporated into regular testing. I have added `noreg-other` since none of 
> the other labels seemed quite appropriate.

> @driverkt Please do not rebase or force-push to an active PR as it 
> invalidates existing review comments. Note for future reference, the bots 
> always squash all changes into a single commit automatically as part of the 
> integration. See [OpenJDK Developers’ 
> Guide](https://openjdk.org/guide/#working-with-pull-requests) for more 
> information.

@driverkt I'm not very sure.  But per this message and the webrevs, it looks 
like the git use for the PR request might be able to improved by working on git 
branches, so that you can push the commit for every little change, and avoid 
force-push.

-

PR Comment: https://git.openjdk.org/jdk/pull/13466#issuecomment-1551644675

Re: RFR: 8308118: Avoid multiarray allocations in AESCrypt.makeSessionKey

[Xue-Lei Andrew Fan]

On Mon, 15 May 2023 19:59:13 GMT, Aleksey Shipilev  wrote:

> One of our services has a hot path with AES/GCM cipher reuse. The JDK code 
> reinitializes the session key on that path, and 
> [JDK-8308105](https://bugs.openjdk.org/browse/JDK-8308105) shows up 
> prominently there. While 
> [JDK-8308105](https://bugs.openjdk.org/browse/JDK-8308105) is being fixed, 
> which would take a while, and would likely require multiple patches, we can 
> work this issue around by avoiding the multiarray allocations in 
> AESCrypt.makeSessionKey.
> 
> Example profile is in the bug.
> 
> On new benchmark and M1 Mac:
> 
> 
> Benchmark   Mode  CntScore   Error  Units
> 
> # Before
> AESReinit.test  avgt   15  873,842 ± 6,911  ns/op
> 
> # After
> AESReinit.test  avgt   15  533,018 ± 4,048  ns/op
> 
> 
> Additional testing:
>  - [x] Benchmarks
>  - [x] macos-aarch64-server-release, `jdk_security`
>  - [ ] linux-x86_64-server-fastdebug, `tier1 tier2`
>  - [ ] linux-aarch64-server-fastdebug, `tier1 tier2`

src/java.base/share/classes/com/sun/crypto/provider/AESCrypt.java line 1372:

> 1370: 
> 1371: // It is significantly faster to allocate individual arrays,
> 1372: // instead of doing the multi-array allocation. See JDK-8308105.

Alternatively, could the multi-array allocation get improved?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13996#discussion_r1194351546

Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v6]

[Xue-Lei Andrew Fan]

On Fri, 12 May 2023 20:30:04 GMT, Kevin Driver  wrote:

>> src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java
>>  line 136:
>> 
>>> 134: } catch (IllegalArgumentException iae) {
>>> 135: throw new SSLException("X500Principal could not be 
>>> parsed " +
>>> 136: "successfully", iae);
>> 
>> Is it ok to throw a general `SSLException` here? Or do you need to call 
>> `TransportContext.fatal()` so that additional cleanup happens? Perhaps 
>> @XueleiFan would know.
>
> Yes, let's wait for @XueleiFan

It is not easy to understand the final behavior if throwing SSLException here.  
I would like to call `TransportContext.fatal()` directly to make the behavior 
more accuracy, by using Alert.DECODE_ERROR.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1194266764

Re: RFR: 8298127: HSS/LMS Signature Verification [v9]

[Xue-Lei Andrew Fan]

On Thu, 11 May 2023 06:27:39 GMT, Ferenc Rakoczi  wrote:

> I had considered that and decided not to use it. In my opinion, Java Enum is 
> much more complicated than it should be for this case. 

OK.

> Efficiency is not a concern here

OK.

> but I also don't see how enum could be more efficient.

enum switch is O(1), while if-else not.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1190697557

Re: RFR: 8298127: HSS/LMS Signature Verification [v9]

[Xue-Lei Andrew Fan]

On Wed, 10 May 2023 15:20:50 GMT, Ferenc Rakoczi  wrote:

>> Implement support for Leighton-Micali Signatures (LMS) as described in RFC 
>> 8554. LMS is an approved software signing algorithm for CNSA 2.0, with 
>> SHA-256/192 parameters recommended.
>
> Ferenc Rakoczi has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   serialization fixes, more code shaping

src/java.base/share/classes/sun/security/provider/HSS.java line 165:

> 163: }
> 164: 
> 165: static class LMSUtils {

It might be simpler and more efficient to use enum for lms and lmots types.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1190680173

Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v3]

[Xue-Lei Andrew Fan]

On Wed, 3 May 2023 22:11:20 GMT, Kevin Driver  wrote:

> I will look at making related changes in these spots as well.
> 
OK.

> @XueleiFan wrt your other question about updating the `getAuthorities` 
> method, I considered this, but it looks like it would involve changing a 
> method signature for that method.

Changing the signature should be fine as it is a internal method.  But I'm fine 
if the calls to getAuthorities() have considered the impact of illegal values 
of X500Principal.

Anyway, this is a typical example to show how hard to use runtime exception.  
From the viewpoint of X500Principal, and unchecked exception should be thrown 
for invalid input values.  But for the caller, it may need to check the input 
values for sure everything is good.  However, an unchecked exception cannot be 
detected by Java compiler and thus the checking of unchecked exception could be 
missed.

-

PR Comment: https://git.openjdk.org/jdk/pull/13466#issuecomment-1540501498

Re: RFR: 8306015: Update sun.security.ssl TLS tests to use SSLContextTemplate or SSLEngineTemplate [v2]

[Xue-Lei Andrew Fan]

On Mon, 8 May 2023 17:56:41 GMT, Matthew Donovan  wrote:

>> I refactored tests in the test/jdk/sun/security/ssl directories to use the 
>> test template classes.
>
> Matthew Donovan has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains two commits:
> 
>  - Merge branch 'master' into sun.security.ssl
>  - 8306015: Update sun.security.ssl TLS tests to use SSLContextTemplate or 
> SSLEngineTemplate

Marked as reviewed by xuelei (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/13495#pullrequestreview-1418772038

Re: RFR: 8294983: SSLEngine throws ClassCastException during handshake [v3]

[Xue-Lei Andrew Fan]

On Wed, 3 May 2023 20:51:30 GMT, Kevin Driver  wrote:

>> Fixes a scenario where a `ServerHandshakeContext` might be cast as a 
>> `ClientHandshakeContext`.
>
> Kevin Driver has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains three commits:
> 
>  - address code review comment
>  - updated copyright
>  - set consumer to null if we're not in client mode

Marked as reviewed by xuelei (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/13727#pullrequestreview-1412177409

Re: RFR: 8306014: Update javax.net.ssl TLS tests to use SSLContextTemplate or SSLEngineTemplate

[Xue-Lei Andrew Fan]

On Mon, 17 Apr 2023 13:25:53 GMT, Matthew Donovan  wrote:

> I refactored tests in the test/jdk/javax/net/ssl directories to use the test 
> template classes.

Looks good to me.

The checks are not fully passed.  Please double check if the failures are 
related to this update.

-

Marked as reviewed by xuelei (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13494#pullrequestreview-1409133989
PR Comment: https://git.openjdk.org/jdk/pull/13494#issuecomment-1531504915

Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v3]

[Xue-Lei Andrew Fan]

On Fri, 28 Apr 2023 19:15:59 GMT, Kevin Driver  wrote:

>> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985)
>
> Kevin Driver has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Update 
> src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java
>   
>   Co-authored-by: Daniel Jelinski 

There are two blocks called CertificateAuthoritiesSpec.getAuthorities(). 
Another call is in the CRCertificateAuthoritiesConsumer inner class. Did you 
check if both should be updated?  Or Is  it possible to update the 
getAuthorities() implementation directly?

There are similar code in CertificateRequest.  Did you have a chance to look at 
if it is impacted as well?

Basically, the issue is caused by the X500Principal constructor with DER bytes. 
 It may be good to check the call to the constructor and handle the IAE 
accordingly as well, for example in the CertificateAuthoritiesSpec.toString() 
method.

-

Changes requested by xuelei (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13466#pullrequestreview-1407415460

Re: RFR: 8297878: KEM: Implementation [v2]

[Xue-Lei Andrew Fan]

On Fri, 14 Apr 2023 16:23:03 GMT, Weijun Wang  wrote:

>>> 2. Do validate parameters on your own (because no one else does)
>>> 
>> I would say: validate the parameters on your own, because not everyone else 
>> does.
>
> I added another proposal in you PR at 
> https://github.com/openjdk/jdk/pull/13470#issuecomment-1508915798.

> I added another proposal in you PR at [#13470 
> (comment)](https://github.com/openjdk/jdk/pull/13470#issuecomment-1508915798).

I like the idea to use abstract class.  The concerns about provider() method 
get addressed.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1167054730

Re: RFR: 8297878: KEM: Implementation [v2]

[Xue-Lei Andrew Fan]

On Fri, 14 Apr 2023 15:00:51 GMT, Weijun Wang  wrote:

> 2. Do validate parameters on your own (because no one else does)
> 
I would say: validate the parameters on your own, because not everyone else 
does.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166995710

Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal

[Xue-Lei Andrew Fan]

On Thu, 13 Apr 2023 18:49:48 GMT, Kevin Driver  wrote:

> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985)

Is [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) in JBS a closed 
bug?

-

PR Comment: https://git.openjdk.org/jdk/pull/13466#issuecomment-1507925845

Re: RFR: 8297878: KEM: Implementation [v2]

[Xue-Lei Andrew Fan]

On Thu, 13 Apr 2023 22:36:04 GMT, Weijun Wang  wrote:

>> src/java.base/share/classes/javax/crypto/KEMSpi.java line 119:
>> 
>>> 117:  * of {@code from} and {@code to} are within the correct range.
>>> 118:  * Therefore an implementation of this method does not need to
>>> 119:  * validate them.
>> 
>> The KEM caller does validate the parameters, but the caller may be more 
>> widely other than the KEM.   Then, the statement here could be wrong at that 
>> time.
>
> I can rewrite this into something like "The caller of this method must 
> validate..." so it becomes a requirement. We'll make sure the `KEM` class 
> follows it. Any other class that wishes to call it directly must do it as 
> well.

You can make it a required part of the specification.  But it is a error-prone 
design.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166196969

Re: RFR: 8297878: KEM: Implementation

[Xue-Lei Andrew Fan]

On Fri, 31 Mar 2023 02:25:04 GMT, Weijun Wang  wrote:

> The KEM API and DHKEM impl. Note that this PR uses new methods in 
> https://github.com/openjdk/jdk/pull/13250.

Changes requested by xuelei (Reviewer).

src/java.base/share/classes/javax/crypto/KEMSpi.java line 119:

> 117:  * of {@code from} and {@code to} are within the correct range.
> 118:  * Therefore an implementation of this method does not need to
> 119:  * validate them.

The KEM caller does validate the parameters, but the caller may be more widely 
other than the KEM.   Then, the statement here could be wrong at that time.

src/java.base/share/classes/javax/crypto/KEMSpi.java line 172:

> 170:  * within the correct range. Therefore an implementation of this 
> method
> 171:  * does not to validate them.
> 172:  *

Same comment as above.

src/java.base/share/classes/javax/crypto/KEMSpi.java line 211:

> 209:  * The caller of this method has already validated the parameters to
> 210:  * ensure that {@code pk} is not {@code null}. Therefore an 
> implementation
> 211:  * of this method does not to validate it.

Same as above, the caller may not validate the parameters yet.  For example, 
the instance could be accessed just like what you do in the KEM implementation 
manner (use Provider method and Service look-up APIs), but without validating 
the parameters as you did.

-

PR Review: https://git.openjdk.org/jdk/pull/13256#pullrequestreview-1384268215
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166059684
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166059934
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166061964

Re: RFR: 8297878: KEM: Implementation

[Xue-Lei Andrew Fan]

On Thu, 13 Apr 2023 19:01:24 GMT, Xue-Lei Andrew Fan  wrote:

>> Currently, `provider()` is a method of `KEM.Encapsulator`. If `KEMSpi. 
>> newEncapsulator` also returns this interface, then what value should its 
>> `provider()` method return? This is what I meant registering itself to a 
>> provider.
>> 
>> When I said different instances, I was asking
>> 
>> var k = KEM.getInstance("DHKEM", p);
>> var e = k.newEncapsulator(pk);
>> // now, is p == e.provider()?
>> 
>> 
>> Or, are you suggesting we should define `provider()` somewhere else? It's 
>> possible, but I have difficulty making every class immutable.
>
>> Currently, `provider()` is a method of `KEM.Encapsulator`. If `KEMSpi. 
>> newEncapsulator` also returns this interface, then what value should its 
>> `provider()` method return? This is what I meant registering itself to a 
>> provider.
>> 
>> When I said different instances, I was asking
>> 
>> ```
>> var k = KEM.getInstance("DHKEM", p);
>> var e = k.newEncapsulator(pk);
>> // now, is p == e.provider()?
>> ```
>> 
>> Or, are you suggesting we should define `provider()` somewhere else? It's 
>> possible, but I have difficulty making every class immutable.
> 
> If the provider() method in KEM.Encapsulator is the only reason, the cost to 
> support it may be too high with so many duplicated/similar 
> specifications/names and code.
> 
> Option 1: Remove the KEM.Encapsulator.provider() method, and provide no 
> access to the underlying provider object.
> 
>>  do you expect it to return new SunJCE()? This means the p in 
>> getInstance("DHKEM", p) will be a different instance from the value returned 
>> by getProvider(). 
> 
> The Provider class is mutable, we may not want to change the provider object 
> asked for "DHKEM".  I think you have used a solution to pass the provider 
> object in the KEM.java implementation currently.  Maybe, it could be twitted 
> a little bit so that the provider can be passed to a delegated 
> KM.Encapsulator interface implementation.
> 
> Option 2:
> 
> public final class KEM {
> interface Encapsulator {
> ...
> KEM.Encapsulated encapsulate(...);
> ...
> 
> default Provider provider() {
> return null;
> }
> }
> 
> private static class DelegatedEncapsulator implements Encapsulator {
> private final Provider p;
> private DelegatedEncapsulator(Encapsulator e, Provider p) {
> this.p = p;
> ...
> } 
> public Provider provider() {
> return this.p;
> }
> }
> 
> ...
>   KEMSpi spi = (KEMSpi) service.newInstance(null);
>   return new DelegatedEncapsulator(
>spi.engineNewEncapsulator(pk, spec, secureRandom),  // 
> This is the interface implementation, use the same provider as KEM.
> service.getProvider());// This is the provider passed to 
> the delegated KEM.Encapsulator object.
> ...
> }

For more details about option 2, please refer to 
https://github.com/openjdk/jdk/pull/13470/files.  The KEM.java and KEMSpi.java 
is pretty much the same except the clean up of En/Decapsulator(s) in this PR.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1166057562

Re: RFR: 8297878: KEM: Implementation

[Xue-Lei Andrew Fan]

On Thu, 13 Apr 2023 17:54:22 GMT, Weijun Wang  wrote:

> Currently, `provider()` is a method of `KEM.Encapsulator`. If `KEMSpi. 
> newEncapsulator` also returns this interface, then what value should its 
> `provider()` method return? This is what I meant registering itself to a 
> provider.
> 
> When I said different instances, I was asking
> 
> ```
> var k = KEM.getInstance("DHKEM", p);
> var e = k.newEncapsulator(pk);
> // now, is p == e.provider()?
> ```
> 
> Or, are you suggesting we should define `provider()` somewhere else? It's 
> possible, but I have difficulty making every class immutable.

If the provider() method in KEM.Encapsulator is the only reason, the cost to 
support it may be too high with so many duplicated/similar specifications/names 
and code.

Option 1: Remove the KEM.Encapsulator.provider() method, and provide no access 
to the underlying provider object.

>  do you expect it to return new SunJCE()? This means the p in 
> getInstance("DHKEM", p) will be a different instance from the value returned 
> by getProvider(). 

The Provider class is mutable, we may not want to change the provider object 
asked for "DHKEM".  I think you have used a solution to pass the provider 
object in the KEM.java implementation currently.  Maybe, it could be twitted a 
little bit so that the provider can be passed to a delegated KM.Encapsulator 
interface implementation.

Option 2:

public final class KEM {
interface Encapsulator {
...
KEM.Encapsulated encapsulate(...);
...

default Provider provider() {
return null;
}
}

private static class DelegatedEncapsulator implements Encapsulator {
private final Provider p;
private DelegatedEncapsulator(Encapsulator e, Provider p) {
this.p = p;
...
} 
public Provider provider() {
return this.p;
}
}

...
  KEMSpi spi = (KEMSpi) service.newInstance(null);
  return new DelegatedEncapsulator(
   spi.engineNewEncapsulator(pk, spec, secureRandom),  // 
This is the interface implementation, use the same provider as KEM.
service.getProvider());// This is the provider passed to 
the delegated KEM.Encapsulator object.
...
}

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1165920458

Re: RFR: 8297878: KEM: Implementation

[Xue-Lei Andrew Fan]

On Thu, 13 Apr 2023 17:10:01 GMT, Weijun Wang  wrote:

> `DHKEM.java` is the implementation, and it does not know which provider it 
> will be put into. It's inside the provider that calls `putService` or `put` 
> to add an implementation there, not that the implementation registered itself 
> in a provider.
> 
I did not get the idea.  Why DHKEM.java need register itself in a provider?  A 
DHKEM.java is a part of a provider, and the Provider implementation in the 
provider knows how to register DHKEM.

> If `getProvider()` is implemented inside the implementation, then it can only 
> be attached to one provider. Also, do you expect it to return `new SunJCE()`? 
> This means the `p` in `getInstance("DHKEM", p)` will be a different instance 
> from the value returned by `getProvider()`. 

I did not get the idea.  How could it be possible to return different 
instances. `getInstance("DHKEM", p)` returns the DHKEM implementation in 
provider p.  The "DHKEM" string name here is not the DHKEM.java class in SunJCE 
provider.

Back to the question you have previously: 
> If the interface is only in KEM, then it needs a provider() method, but an 
> implementation actually does not know what the provider is.

Why it is needed to know the provider of the interface?  Do you mean the 
Encapsulator provider could be different from the KEM provider?  That's, KEM 
provider is ProviderK, and the Encapsulator provider is ProviderE, and you want 
them work together?   Does it make sense that it is required that Encapsulator 
is an internal implementation of the KEM provider (i.e., both from the same 
provider)?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1165838768

Re: RFR: 8297878: KEM: Implementation

[Xue-Lei Andrew Fan]

On Wed, 12 Apr 2023 23:23:02 GMT, Weijun Wang  wrote:

> If the interface is only in `KEM`, then it needs a `provider()` method, but 
> an implementation actually does not know what the provider is.

With "implementation", do you mean the javax/crypto/KEPSpi.java or 
src/java.base/share/classes/com/sun/crypto/provider/DHKEM.java?

If it is refer to KEPSpi.java, why KEPSpi.java need to know what the provider 
is?  Is it sufficient to use engineNewEncapsulator() to get the provider 
implementation?

If it is refer to DHKEM.java, I did not get the idea why the  provider is 
unknown.

> An implementation can be registered in any (or even multiple) providers.

I did not get the idea.  Why it is not registered in SunJCE?

I think you may have evaluated the following idea, but I'm not why it is not 
work.  I may missed something.  Would you mind explain in more details?


public final class KEM {
interface Encapsulator {
...
KEM.Encapsulated encapsulate(...);
...
}

public static KEM getInstance(String algorithm) {
...
}

// Search for the registered providers, return the 1st non-null 
provider.newEncapsulator() or throw exception.
public Encapsulator newEncapsulator(PublicKey pk,
AlgorithmParameterSpec spec, SecureRandom secureRandom)
...
}
}

public interface KEMSpi {
// A provider implementation will implement the KEM.Encapsulator 
// interface internally.  If a provider does not support the parameters,
// null or nil object will be returned.
public KEM.Encapsulator newEncapsulator(PublicKey pk,
AlgorithmParameterSpec spec, SecureRandom secureRandom);
}   

Use case:
KEM.getInstance(DHKEM).newEncapsulator(...);

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1164923024

Re: RFR: 8297878: KEM: Implementation

[Xue-Lei Andrew Fan]

On Wed, 12 Apr 2023 22:17:00 GMT, Weijun Wang  wrote:

> So you think the name is misleading?

Yes.

> But then if you want to talk about one you have to say which class it is in, 
> and this could be confusing and sometimes wrong. 

I did not get the idea yet to define it in KEPSpi.  It looks like that only one 
interface in the KEM should be sufficient.  The interface implementation 
details could be provider based.  It is OK to have an provider implementation 
implements an interface internally.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1164743148

Re: RFR: 8297878: KEM: Implementation

[Xue-Lei Andrew Fan]

On Wed, 12 Apr 2023 18:32:30 GMT, Weijun Wang  wrote:

>> src/java.base/share/classes/javax/crypto/KEMSpi.java line 107:
>> 
>>> 105:  * @see KEM.Encapsulator
>>> 106:  */
>>> 107: interface EncapsulatorSpi {
>> 
>> Is it really necessary to define a EncapsulatorSpi?  It looks like an 
>> internal implementation for KEMSpi service, rather than a public individual 
>> interface that EncapsulatorSpi.getInstance() could be used.
>
> Not exactly sure what you want. Security providers need to implement it and 
> the `KEM` class uses it. Unlike other `AbcSpi` classes, it will not be 
> created with a `getInstance` method.

If I get it right, SPI means "service provider interface", which is normally 
for public service access by searching the registers.  In the provider 
implementation, a SPI implementation is normally registered so that it can be 
accessed.  I did not find the registration in the provider implementation.  I 
know it is right because it is not expected to be registered as it is not 
expected to be accessed directly.

> Security providers need to implement it and the KEM class uses it.
I know, but it is not necessarily designed as an SPI (it could be a normal 
interface that a provider need to implement and KEM class uses).  You can code 
like this, but I'm not sure if it is the common way.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1164684415

Re: RFR: 8297878: KEM: Implementation

[Xue-Lei Andrew Fan]

On Fri, 31 Mar 2023 02:25:04 GMT, Weijun Wang  wrote:

> The KEM API and DHKEM impl. Note that this PR uses new methods in 
> https://github.com/openjdk/jdk/pull/13250.

src/java.base/share/classes/javax/crypto/KEMSpi.java line 107:

> 105:  * @see KEM.Encapsulator
> 106:  */
> 107: interface EncapsulatorSpi {

Is it really necessary to define a EncapsulatorSpi?  It looks like an internal 
implementation for KEMSpi service, rather than a public individual interface 
that EncapsulatorSpi.getInstance() could be used.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1164195329

Re: RFR: 8305091: Change ChaCha20 cipher init behavior to match AES-GCM

[Xue-Lei Andrew Fan]

On Tue, 11 Apr 2023 17:26:25 GMT, Jamil Nimeh  wrote:

> This fixes an issue where the key/nonce reuse policy for SunJCE ChaCha20 and 
> ChaCha20-Poly1305 was overly strict in enforcing no-reuse when the Cipher was 
> in DECRYPT_MODE.  For decryption, this should be allowed and be consistent 
> with the AES-GCM decryption initialization behavior.
> 
> - Issue: https://bugs.openjdk.org/browse/JDK-8305091
> - CSR: https://bugs.openjdk.org/browse/JDK-8305822

In the decryption side, does it sound like good to detect and reject key/nonce 
reuse for security reason(i.e., if key/nonce is reused, the decryption side 
will not accept the encryption)?  Did you known real problems in practice for 
the key/nonce reuse for decryption?

-

PR Comment: https://git.openjdk.org/jdk/pull/13428#issuecomment-1503872733

Re: RFR: 8182621: JSSE should reject empty TLS plaintexts [v2]

[Xue-Lei Andrew Fan]

On Fri, 7 Apr 2023 13:56:39 GMT, Matthew Donovan  wrote:

>> Added code similar to the suggested patches for empty Handshake messages. I 
>> also implemented tests to verify empty Handshake, Alert, and 
>> ChangeCipherSpec messages result in expected behavior: for SSLEngineImpl, 
>> exceptions are thrown, for SSLSockets the connection is closed.
>
> Matthew Donovan has updated the pull request incrementally with two 
> additional commits since the last revision:
> 
>  - added comment referring to relevant RFC
>  - clarified if-statements; fixed exception message wording

@mpdonova Did you have a chance to pass Mach5 testing?  If the testing is good, 
I would like to sponsor the committing.

-

PR Comment: https://git.openjdk.org/jdk/pull/13306#issuecomment-1502332785

Re: RFR: 8182621: JSSE should reject empty TLS plaintexts [v2]

[Xue-Lei Andrew Fan]

On Fri, 7 Apr 2023 13:56:39 GMT, Matthew Donovan  wrote:

>> Added code similar to the suggested patches for empty Handshake messages. I 
>> also implemented tests to verify empty Handshake, Alert, and 
>> ChangeCipherSpec messages result in expected behavior: for SSLEngineImpl, 
>> exceptions are thrown, for SSLSockets the connection is closed.
>
> Matthew Donovan has updated the pull request incrementally with two 
> additional commits since the last revision:
> 
>  - added comment referring to relevant RFC
>  - clarified if-statements; fixed exception message wording

Looks good to me.  Thank you!

-

Marked as reviewed by xuelei (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13306#pullrequestreview-1376985093

Re: RFR: 8182621: JSSE should reject empty TLS plaintexts

[Xue-Lei Andrew Fan]

On Mon, 3 Apr 2023 18:13:19 GMT, Matthew Donovan  wrote:

> Added code similar to the suggested patches for empty Handshake messages. I 
> also implemented tests to verify empty Handshake, Alert, and ChangeCipherSpec 
> messages result in expected behavior: for SSLEngineImpl, exceptions are 
> thrown, for SSLSockets the connection is closed.

src/java.base/share/classes/sun/security/ssl/SSLEngineInputRecord.java line 266:

> 264: //
> 265: if (contentType == ContentType.HANDSHAKE.id) {
> 266: if (!fragment.hasRemaining()) {

It may be easier to read if using "contentLen != 0", instead of 
"fragment.hasRemaining()".  I may add a comment like what is says in RFC 8446, 
"Implementations MUST NOT send zero-length fragments of Handshake types, even 
if those fragments contain padding."

src/java.base/share/classes/sun/security/ssl/SSLEngineInputRecord.java line 267:

> 265: if (contentType == ContentType.HANDSHAKE.id) {
> 266: if (!fragment.hasRemaining()) {
> 267: throw new SSLProtocolException("Handshake packets may 
> not be zero-length");

"may not" -> "must not"

src/java.base/share/classes/sun/security/ssl/SSLSocketInputRecord.java line 287:

> 285: if (contentType == ContentType.HANDSHAKE.id) {
> 286: ByteBuffer handshakeFrag = fragment;
> 287: if (!handshakeFrag.hasRemaining()) {

Same comment as 
src/java.base/share/classes/sun/security/ssl/SSLEngineInputRecord.java

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13306#discussion_r1160385752
PR Review Comment: https://git.openjdk.org/jdk/pull/13306#discussion_r1160385932
PR Review Comment: https://git.openjdk.org/jdk/pull/13306#discussion_r1160386380

Re: RFR: 8302017: Allocate BadPaddingException only if it will be thrown

[Xue-Lei Andrew Fan]

On Mon, 27 Mar 2023 08:44:31 GMT, Aleksey Shipilev  wrote:

> If the exception creation is the problem, maybe we should turn it stackless

This is one direction of the update, I think.  Alternatively, it may be doable 
by not using exception in the unpad() implementation any longer.

-

PR Comment: https://git.openjdk.org/jdk/pull/12732#issuecomment-1485407309

Re: RFR: 8302017: Allocate BadPaddingException only if it will be thrown

[Xue-Lei Andrew Fan]

On Tue, 14 Mar 2023 21:58:46 GMT, Xue-Lei Andrew Fan  wrote:

>> May I get a chance to review it before the integration?  I may need more 
>> time to dig into time-constant issue.
>
>> May I get a chance to review it before the integration? I may need more time 
>> to dig into time-constant issue.
> 
> If I read the Bleichenbacher's 
> Attack[[1]](https://archiv.infsec.ethz.ch/education/fs08/secsem/bleichenbacher98.pdf)[[2]](https://medium.com/@c0D3M/bleichenbacher-attack-explained-bc630f88ff25)[[3]](https://asecuritysite.com/encryption/c_c3)
>  right, the attack works if it can tell the difference between good 
> conditions and error conditions.  RFC 8017 says "distinguish the different 
> error conditions", but it may be parsed differently for various context.  
> Please be careful about this update.
> 
> Thank you for giving me more time to look into the details.

> @XueleiFan are you still looking into the details of this change?

I'm not sure this update is safe.  It would be good (it is possible) to have an 
improvement that  there is no timing differences between success and failure by 
not using exception in unpad() implementation any longer.

-

PR Comment: https://git.openjdk.org/jdk/pull/12732#issuecomment-1483174451

Re: RFR: 8302017: Allocate BadPaddingException only if it will be thrown

[Xue-Lei Andrew Fan]

On Tue, 14 Mar 2023 21:23:02 GMT, Xue-Lei Andrew Fan  wrote:

> May I get a chance to review it before the integration? I may need more time 
> to dig into time-constant issue.

If I read the Bleichenbacher's Attack[1][2] right, the attack works if it can 
tell the difference between good conditions and error conditions.  RFC 8017 
says "distinguish the different error conditions", but it may be parsed 
differently for various context.  Please be careful about this update.

Thank you for giving me more time to look into the details.

[1]: https://archiv.infsec.ethz.ch/education/fs08/secsem/bleichenbacher98.pdf
[2]: https://medium.com/@c0D3M/bleichenbacher-attack-explained-bc630f88ff25
[3]: https://asecuritysite.com/encryption/c_c3

-

PR: https://git.openjdk.org/jdk/pull/12732