Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v19]

[Anthony Scarpino]

On Wed, 8 Jun 2022 17:49:38 GMT, XenoAmess  wrote:

>> as title.
>
> XenoAmess has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   clean up Calendar

I gave a quick look at the security files touched and seems straightforward. I 
didn't see any problems

-

Marked as reviewed by ascarpino (Reviewer).

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

Re: RFR: 8277307: Pre shared key sent under both session_ticket and pre_shared_key extensions [v2]

[Anthony Scarpino]

On Thu, 2 Jun 2022 21:02:16 GMT, Daniel Jeliński  wrote:

>> Session ticket extension should only contain pre-TLS1.3 stateless session 
>> tickets; it should not be used for sending TLS1.3 pre-shared keys.
>
> Daniel Jeliński has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   different check for TLS13

Marked as reviewed by ascarpino (Reviewer).

please make sure all jdk_security tests and tier1 tests pass before integrating

-

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

Re: RFR: 8277307: Pre shared key sent under both session_ticket and pre_shared_key extensions [v2]

[Anthony Scarpino]

On Thu, 2 Jun 2022 21:02:16 GMT, Daniel Jeliński  wrote:

>> Session ticket extension should only contain pre-TLS1.3 stateless session 
>> tickets; it should not be used for sending TLS1.3 pre-shared keys.
>
> Daniel Jeliński has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   different check for TLS13

The bug and the PR could have used a lot more description that the issue here 
is that 1.2 and 1.3 are enabled at the same time. such as via 
`setEnabledProtocols()`.  At first I thought this bug was incorrect because 1.3 
does not display a session_ticket extension as it is only supported in the code 
by 1.0-1.2.  But with both enabled, it causes all the extensions to be enabled.

After thinking about it, this maybe the better way to fix this as the it a 
heterogeneous server environment, only sending 1.3 extension from the resumed 
TLS protocol may cause errors when talking to 1.2 server.  So both extensions 
need to be enabled globally, but since we are resuming 1.3 state, the same 
state does not to be passed in a 1.2 connection.  It should do a full handshake.

One could ask the reverse, if the resumption is from 1.2 should we be sending a 
1.3 pre_shared_key extension.. But that can be for another bug I suppose.

-

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

Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer [v2]

[Anthony Scarpino]

On Sat, 14 May 2022 03:29:14 GMT, Anthony Scarpino  
wrote:

>> Hi,
>> 
>> I need a review of this fix to allow a read-only 'src' buffer to be used 
>> with SSLEngine.unwrap(). A temporary read-write buffer is created in the 
>> SSLCipher operation when a read-only buffer is passed. If the 'src' is 
>> read-write, there is no effect on the current operation
>> 
>> The PR also includes a CSR for an API implementation note to the 
>> SSLEngine.unwrap. The 'src' buffer may be modified during the decryption 
>> operation. 'unwrap()' has had this behavior forever, so there is no 
>> compatibility issue with this note. Using the 'src' buffer for in-place 
>> decryption was a performance decision.
>> 
>> Tony
>
> Anthony Scarpino has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains four additional 
> commits since the last revision:
> 
>  - review update
>  - update some nits
>  - PR ready
>  - Initial

I think read-only has been allowed by the spec from the start, we just never 
allowed it to work properly.  I don't think this opens up possible 
complications in the code.

-

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

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

[Anthony Scarpino]

On Fri, 29 Apr 2022 03:58:57 GMT, Anthony Scarpino  
wrote:

> Hi,
> 
> I need a review of this fix to allow a read-only 'src' buffer to be used with 
> SSLEngine.unwrap(). A temporary read-write buffer is created in the SSLCipher 
> operation when a read-only buffer is passed. If the 'src' is read-write, 
> there is no effect on the current operation
> 
> The PR also includes a CSR for an API implementation note to the 
> SSLEngine.unwrap. The 'src' buffer may be modified during the decryption 
> operation. 'unwrap()' has had this behavior forever, so there is no 
> compatibility issue with this note. Using the 'src' buffer for in-place 
> decryption was a performance decision.
> 
> Tony

This pull request has now been integrated.

Changeset: f17c68ce
Author:Anthony Scarpino 
URL:   
https://git.openjdk.java.net/jdk/commit/f17c68ce4a0b4f5c3131f4e4626a5a55b7f2f61f
Stats: 393 lines in 3 files changed: 291 ins; 20 del; 82 mod

8283577: SSLEngine.unwrap on read-only input ByteBuffer

Reviewed-by: wetmore

-

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

Re: RFR: 8286908: ECDSA signature should not return parameters

[Anthony Scarpino]

On Tue, 17 May 2022 19:56:22 GMT, Weijun Wang  wrote:

> Let ECDSA's `engineGetParameters()` always return null. At the same time, 
> remove the remembered `sigParams` field. One behavior change is that after 
> calling `setParameter()`, one can call `init()` again with a key using 
> different parameters. I think this should be allowed since we are reusing the 
> signature object with a brand new key.
> 
> `setParameter` is kept unchanged to be able to deal with certificates still 
> having parameters after the signature algorithm object identifier. See 
> https://bugs.openjdk.java.net/browse/JDK-8225745.
> 
> Also added SHA1withECDSA to the no-NULL list in `KnownOIDs`.
> 
> All security-related tests passed.

Marked as reviewed by ascarpino (Reviewer).

Looks good to me.
The changes seem within the spec to me.

-

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

Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer [v2]

[Anthony Scarpino]

On Sat, 14 May 2022 03:29:14 GMT, Anthony Scarpino  
wrote:

>> Hi,
>> 
>> I need a review of this fix to allow a read-only 'src' buffer to be used 
>> with SSLEngine.unwrap(). A temporary read-write buffer is created in the 
>> SSLCipher operation when a read-only buffer is passed. If the 'src' is 
>> read-write, there is no effect on the current operation
>> 
>> The PR also includes a CSR for an API implementation note to the 
>> SSLEngine.unwrap. The 'src' buffer may be modified during the decryption 
>> operation. 'unwrap()' has had this behavior forever, so there is no 
>> compatibility issue with this note. Using the 'src' buffer for in-place 
>> decryption was a performance decision.
>> 
>> Tony
>
> Anthony Scarpino has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains four commits:
> 
>  - review update
>  - update some nits
>  - PR ready
>  - Initial

There is too much grey area.  It says the src buffer maybe modified, which one 
could interpret it cannot be a read-only, but that would still need 
clarification to explicitly say "no read only buffers".  And other than these 
internal 'in-place' crypto reason, there is no API reason to not allow 
read-only buffers as input.
I did think about closing the CSR as the text was already there about the src 
buffer, even thought it was using a different term.  But I felt strongly enough 
that I wanted to prevent that confusion in the future.

-

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

Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer [v2]

[Anthony Scarpino]

> Hi,
> 
> I need a review of this fix to allow a read-only 'src' buffer to be used with 
> SSLEngine.unwrap(). A temporary read-write buffer is created in the SSLCipher 
> operation when a read-only buffer is passed. If the 'src' is read-write, 
> there is no effect on the current operation
> 
> The PR also includes a CSR for an API implementation note to the 
> SSLEngine.unwrap. The 'src' buffer may be modified during the decryption 
> operation. 'unwrap()' has had this behavior forever, so there is no 
> compatibility issue with this note. Using the 'src' buffer for in-place 
> decryption was a performance decision.
> 
> Tony

Anthony Scarpino has updated the pull request with a new target base due to a 
merge or a rebase. The pull request now contains four commits:

 - review update
 - update some nits
 - PR ready
 - Initial

-

Changes: https://git.openjdk.java.net/jdk/pull/8462/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8462=01
  Stats: 393 lines in 3 files changed: 291 ins; 20 del; 82 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8462.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8462/head:pull/8462

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

Re: RFR: 8286422: Add OIDs for RC2 and Blowfish

[Anthony Scarpino]

On Wed, 11 May 2022 22:35:32 GMT, Weijun Wang  wrote:

> Add missing OIDs for 2 secret key algorithms. These will be used when storing 
> secret keys in a PKCS12 keystore. Like DES and DESede, the OIDs were 
> originally defined for CBC mode cipher algorithms, they are reused here for 
> key algorithms.
> 
> OpenSSL uses the same OIDs for cipher algorithms.
> 
> 1 3 6 1 4 1 3029 1 2  : BF-CBC: bf-cbc
> rsadsi 3 2: RC2-CBC   : rc2-cbc
> 
> 
> Similar to DES, RC2 is only added as an alias so it needs to be specially 
> treated when getting a secret key entry from a PKCS12 keystore.
> 
> No attempt is made to define these OIDs as aliases to existing cipher 
> algorithms.

Marked as reviewed by ascarpino (Reviewer).

-

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

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

[Anthony Scarpino]

On Wed, 11 May 2022 00:31:23 GMT, Bradford Wetmore  wrote:

>> Hi,
>> 
>> I need a review of this fix to allow a read-only 'src' buffer to be used 
>> with SSLEngine.unwrap(). A temporary read-write buffer is created in the 
>> SSLCipher operation when a read-only buffer is passed. If the 'src' is 
>> read-write, there is no effect on the current operation
>> 
>> The PR also includes a CSR for an API implementation note to the 
>> SSLEngine.unwrap. The 'src' buffer may be modified during the decryption 
>> operation. 'unwrap()' has had this behavior forever, so there is no 
>> compatibility issue with this note. Using the 'src' buffer for in-place 
>> decryption was a performance decision.
>> 
>> Tony
>
> test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 172:
> 
>> 170: out.clear();
>> 171: String testString = "ASDF";
>> 172: in.put(testString.getBytes()).flip();
> 
> If you're going to convert back from UTF_8 later, you should probably convert 
> using getBytes(UTF_8) here.

setting the input to UTF8 isn't a concern.  The latter line to decode it 
changes it from using the ByteBuffer.toString() to the contents of the 
ByteBuffer in a String.

> test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 188:
> 
>> 186: in.clear();
>> 187: System.out.println("2: Server send: " + testString);
>> 188: in.put(testString.getBytes()).flip();
> 
> Same

Same :)

> test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 189:
> 
>> 187: System.out.println("2: Server send: " + testString);
>> 188: in.put(testString.getBytes()).flip();
>> 189: send(server, in, out);
> 
> Did you want to try asReadOnlyBuffer() here also?

Yeah, they should have been.  I must have undid it to test something

> test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 191:
> 
>> 189: send(server, in, out);
>> 190: in.clear();
>> 191: receive(client, out, in);
> 
> And here?

Same

-

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

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

[Anthony Scarpino]

On Wed, 11 May 2022 05:52:38 GMT, Xue-Lei Andrew Fan  wrote:

>> Hi,
>> 
>> I need a review of this fix to allow a read-only 'src' buffer to be used 
>> with SSLEngine.unwrap(). A temporary read-write buffer is created in the 
>> SSLCipher operation when a read-only buffer is passed. If the 'src' is 
>> read-write, there is no effect on the current operation
>> 
>> The PR also includes a CSR for an API implementation note to the 
>> SSLEngine.unwrap. The 'src' buffer may be modified during the decryption 
>> operation. 'unwrap()' has had this behavior forever, so there is no 
>> compatibility issue with this note. Using the 'src' buffer for in-place 
>> decryption was a performance decision.
>> 
>> Tony
>
> src/java.base/share/classes/javax/net/ssl/SSLEngine.java line 677:
> 
>> 675:  * @see #unwrap(ByteBuffer, ByteBuffer[], int, int)
>> 676:  *
>> 677:  * @implNote The data in {@code src} may be modified during the 
>> decryption
> 
> It looks like a note for the API users to me.  Is apiNote tag more 
> appropriate here?

The CSR and this code is changing, the note I was adding was already 
documented, but its existing wording should be more clear.

-

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

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

[Anthony Scarpino]

On Mon, 9 May 2022 23:48:24 GMT, Bradford Wetmore  wrote:

>> Hi,
>> 
>> I need a review of this fix to allow a read-only 'src' buffer to be used 
>> with SSLEngine.unwrap(). A temporary read-write buffer is created in the 
>> SSLCipher operation when a read-only buffer is passed. If the 'src' is 
>> read-write, there is no effect on the current operation
>> 
>> The PR also includes a CSR for an API implementation note to the 
>> SSLEngine.unwrap. The 'src' buffer may be modified during the decryption 
>> operation. 'unwrap()' has had this behavior forever, so there is no 
>> compatibility issue with this note. Using the 'src' buffer for in-place 
>> decryption was a performance decision.
>> 
>> Tony
>
> test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 157:
> 
>> 155: // Do TLS handshake
>> 156: do {
>> 157: statusClient = doHandshake(client, out, in);
> 
> It's potentially a little inefficient returning after each wrap/unwrap() 
> instead of doing the task right away, but it works.  No need to change.
> 
> Is this style something you copied from another test?  The  SSLEngineTemplate 
> in the templates directory is what I often use.

I got it from somewhere that I don't remember off hand.  I'm just trying to get 
through the handshake.

> test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 162:
> 
>> 160: statusServer != HandshakeStatus.NOT_HANDSHAKING);
>> 161: 
>> 162: // Read NST
> 
> What is NST?

New Session Ticket

-

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

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

[Anthony Scarpino]

On Mon, 9 May 2022 23:15:40 GMT, Bradford Wetmore  wrote:

>> Hi,
>> 
>> I need a review of this fix to allow a read-only 'src' buffer to be used 
>> with SSLEngine.unwrap(). A temporary read-write buffer is created in the 
>> SSLCipher operation when a read-only buffer is passed. If the 'src' is 
>> read-write, there is no effect on the current operation
>> 
>> The PR also includes a CSR for an API implementation note to the 
>> SSLEngine.unwrap. The 'src' buffer may be modified during the decryption 
>> operation. 'unwrap()' has had this behavior forever, so there is no 
>> compatibility issue with this note. Using the 'src' buffer for in-place 
>> decryption was a performance decision.
>> 
>> Tony
>
> test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 1:
> 
>> 1: /*
> 
> Wondering why this is in javax/net/ssl/SSLSession instead of 
> sun/security/ssl/SSLCipher.

I can move it.. I created it from another test which happen to be in that 
directory

-

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

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

[Anthony Scarpino]

On Fri, 29 Apr 2022 22:57:20 GMT, Weijun Wang  wrote:

>> All `IntegerPolynimial`s are singletons now. Also, hand-coded 
>> implementations for Ed25519 and Ed448 are removed. They were not used since 
>> `FieldGen` starts generating classes for them.
>> 
>> No new regression test. This is a clean-up.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   move singleton back into impl and make constructor private

I'm good with this change

-

Marked as reviewed by ascarpino (Reviewer).

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

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

[Anthony Scarpino]

Hi,

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

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

Tony

-

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

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

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

Re: RFR: 8285493: ECC calculation error

[Anthony Scarpino]

On Tue, 26 Apr 2022 21:02:49 GMT, Weijun Wang  wrote:

> Only numbers from the same modular fields can be involved in arithmetic 
> calculations. Add `assert` to guarantee this.
> 
> Also, found one broken case and rewrote it.

Changes look good

-

Marked as reviewed by ascarpino (Reviewer).

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

Re: RFR: 8285398: Cache the results of constraint checks

[Anthony Scarpino]

On Thu, 21 Apr 2022 19:58:39 GMT, Daniel Jeliński  wrote:

> Profiling the TLS handshakes using SSLHandshake benchmark shows that a large 
> portion of time is spent in HandshakeContext initialization, specifically in 
> DisabledAlgorithmConstraints class.
> 
> There are only a few instances of that class, and they are immutable. Caching 
> the results should be a low-risk operation.
> 
> The cache is implemented as a softly reachable ConcurrentHashMap; this way it 
> can be removed from memory after a period of inactivity. Under normal 
> circumstances the cache holds no more than 100 algorithms.

I'm good with this idea.  It's not caching the whole list like I struggled 
with, but it's a good solution for the algorithm name based constraints.  It 
also shows that more caching probably would help further.
I'll let Xuelei finish his review comments

-

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

Integrated: 8285389: EdDSA trimming zeros

[Anthony Scarpino]

On Fri, 22 Apr 2022 21:04:58 GMT, Anthony Scarpino  
wrote:

> Hi,
> 
> I'd like a code review of this change to EdDSA. ed25519 and ed448 internally 
> was trimming extra zeros off the end of the signature before processing. This 
> can result in some verify testing failures which are strict about the 
> signature length passed into the operation. 
> 
> thanks
> 
> Tony

This pull request has now been integrated.

Changeset: 414918d9
Author:Anthony Scarpino 
URL:   
https://git.openjdk.java.net/jdk/commit/414918d9113b447c9ae774cdfd087f1636b8e5a0
Stats: 6 lines in 1 file changed: 5 ins; 0 del; 1 mod

8285389: EdDSA trimming zeros

Reviewed-by: xuelei

-

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

Re: Java Cryptographic Extension Survey

[Anthony Scarpino]

Reminder this survey ends Friday.  Please fill it out if you have not.

Thanks

On 4/12/22 8:10 AM, Anthony Scarpino wrote:

Hello,

Java Cryptographic Extension (JCE) has been in Java SE for a long time 
and has made incremental changes over the years.  Looking forward, we 
would like to know more about how projects are using JCE and what 
changes, features, and API enhancements would be helpful for your projects.


Below is a survey that we hope you can spend 5 minutes to fill out.  If 
you have written or maintain code that uses the JCE API, then we would 
appreciate if you would complete this survey:


https://www.questionpro.com/t/AUzP7ZrFWv

The survey will be open through April 29.

Thank you,
Anthony Scarpino
OpenJDK Security Group
https://openjdk.java.net/groups/security/

Re: RFR: 8285389: EdDSA trimming zeros

[Anthony Scarpino]

On Fri, 22 Apr 2022 21:04:58 GMT, Anthony Scarpino  
wrote:

> Hi,
> 
> I'd like a code review of this change to EdDSA. ed25519 and ed448 internally 
> was trimming extra zeros off the end of the signature before processing. This 
> can result in some verify testing failures which are strict about the 
> signature length passed into the operation. 
> 
> thanks
> 
> Tony

That issue has nothing to do with this. Please file a webbug or start another 
conversation on the mailing list regarding that problem

-

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

Re: RFR: 8285389: EdDSA trimming zeros

[Anthony Scarpino]

On Sat, 23 Apr 2022 14:39:50 GMT, Xue-Lei Andrew Fan  wrote:

> Did you want to correct the verify testing so that it could
> accept trimmed signature?  Or do not trimming the extra zeros
> of the signature any longer?  I did not get the point from the 
> patch.

The test is correct.
Perhaps a clear explanation is if the signature length is greater or less than 
expected an exception should be thrown for the length being wrong instead of 
trying to verify the signature.  In the particular test, zeros at the end of a 
too long signature, can get trimmed by BigInteger and the signature checked 
which should have been rejected before processing.

-

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

RFR: 8285389: EdDSA trimming zeros

[Anthony Scarpino]

Hi,

I'd like a code review of this change to EdDSA. ed25519 and ed448 internally 
was trimming extra zeros off the end of the signature before processing. This 
can result in some verify testing failures which are strict about the signature 
length passed into the operation. 

thanks

Tony

-

Commit messages:
 - verify length

Changes: https://git.openjdk.java.net/jdk/pull/8372/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8372=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8285389
  Stats: 6 lines in 1 file changed: 5 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8372.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8372/head:pull/8372

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

Re: RFR: 8283022: com/sun/crypto/provider/Cipher/AEAD/GCMBufferTest.java failing with -Xcomp after 8273297 [v2]

[Anthony Scarpino]

On Mon, 18 Apr 2022 20:43:06 GMT, Smita Kamath  wrote:

>> When input length provided to the intrinsic is 8192, only 7680 bytes are 
>> processed as the intrinsic operates on multiples of 768 bytes.
>> In implGCMCrypt(ByteBuffer src, ByteBuffer dst) method, 
>> dst.put(bout, 0, PARALLEL_LEN) statement caused the ciphertext mismatch as 
>> PARALLEL_LEN was set to 8192. 
>> Since the intrinsic only processed 7680 bytes, the rest output was incorrect.
>
> Smita Kamath has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Updated copyright year

I'm not sure if the last commit removed the approval or not.. so i'll approve 
again

-

Marked as reviewed by ascarpino (Reviewer).

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

Re: RFR: 8284291: sun/security/krb5/auto/Renew.java fails intermittently on Windows 11

[Anthony Scarpino]

On Mon, 4 Apr 2022 21:27:51 GMT, Weijun Wang  wrote:

> `Thread.sleep()` seems not very precise on some systems. Update this test to 
> check the current time continously.

Marked as reviewed by ascarpino (Reviewer).

-

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

Re: RFR: 8283022: com/sun/crypto/provider/Cipher/AEAD/GCMBufferTest.java failing with -Xcomp after 8273297 [v2]

[Anthony Scarpino]

On Mon, 18 Apr 2022 20:43:06 GMT, Smita Kamath  wrote:

>> When input length provided to the intrinsic is 8192, only 7680 bytes are 
>> processed as the intrinsic operates on multiples of 768 bytes.
>> In implGCMCrypt(ByteBuffer src, ByteBuffer dst) method, 
>> dst.put(bout, 0, PARALLEL_LEN) statement caused the ciphertext mismatch as 
>> PARALLEL_LEN was set to 8192. 
>> Since the intrinsic only processed 7680 bytes, the rest output was incorrect.
>
> Smita Kamath has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Updated copyright year

I reran the test on aarch64 and x64 using -Xcomp without failure, also 
repeating the failing test. Given the bug says it was intermittent I can never 
been 100% certain it is perfect, but looking at the aarch64, it is just looping 
on that data it is presented, not hardcoded for a particular amount of data 
like the x64 code.  That looping should work fine with the code change.
I think it's ready to go into the repo.

-

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

Re: RFR: 8283022: com/sun/crypto/provider/Cipher/AEAD/GCMBufferTest.java failing with -Xcomp after 8273297

[Anthony Scarpino]

On Mon, 18 Apr 2022 05:06:26 GMT, Smita Kamath  wrote:

> When input length provided to the intrinsic is 8192, only 7680 bytes are 
> processed as the intrinsic operates on multiples of 768 bytes.
> In implGCMCrypt(ByteBuffer src, ByteBuffer dst) method, 
> dst.put(bout, 0, PARALLEL_LEN) statement caused the ciphertext mismatch as 
> PARALLEL_LEN was set to 8192. 
> Since the intrinsic only processed 7680 bytes, the rest output was incorrect.

Please update the copyright year

-

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

Re: RFR: 8283022: com/sun/crypto/provider/Cipher/AEAD/GCMBufferTest.java failing with -Xcomp after 8273297

[Anthony Scarpino]

On Mon, 18 Apr 2022 05:06:26 GMT, Smita Kamath  wrote:

> When input length provided to the intrinsic is 8192, only 7680 bytes are 
> processed as the intrinsic operates on multiples of 768 bytes.
> In implGCMCrypt(ByteBuffer src, ByteBuffer dst) method, 
> dst.put(bout, 0, PARALLEL_LEN) statement caused the ciphertext mismatch as 
> PARALLEL_LEN was set to 8192. 
> Since the intrinsic only processed 7680 bytes, the rest output was incorrect.

Oh wow.. you're right. I never saw that come through.. thanks for seeing that.  
I'll run some tests that it's ok.

-

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

Re: RFR: 8283022: com/sun/crypto/provider/Cipher/AEAD/GCMBufferTest.java failing with -Xcomp after 8273297

[Anthony Scarpino]

On Mon, 18 Apr 2022 05:06:26 GMT, Smita Kamath  wrote:

> When input length provided to the intrinsic is 8192, only 7680 bytes are 
> processed as the intrinsic operates on multiples of 768 bytes.
> In implGCMCrypt(ByteBuffer src, ByteBuffer dst) method, 
> dst.put(bout, 0, PARALLEL_LEN) statement caused the ciphertext mismatch as 
> PARALLEL_LEN was set to 8192. 
> Since the intrinsic only processed 7680 bytes, the rest output was incorrect.

Marked as reviewed by ascarpino (Reviewer).

x86-64 only uses this intrinsic, aarch64 does not support this intrinsic and 
uses the java code.

-

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

AlgorithmConstraints caching [ was Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice]

[Anthony Scarpino]

Hi Sean,

Caching is an interesting idea.  I've wondered for a while off and on 
about how to speed it up, but hadn't come up with a solution I liked. 
The complication with caching is while something like an algorithm name 
only could be easy in a hashmap, it gets more complicated when you get 
into key sizes. Such as, how to represent RSA 1k being disallowed and 
but 2k allowed.. or certificate usage..


Tony

On 4/13/22 2:03 AM, Sean Coffey wrote:

On Tue, 12 Apr 2022 11:28:12 GMT, Daniel Jeliński  wrote:


During TLS handshake, hundreds of constraints are evaluated to determine which 
cipher suites are usable. Most of the evaluations are performed using 
`HandshakeContext#algorithmConstraints` object. By default that object contains 
a `SSLAlgorithmConstraints` instance wrapping another `SSLAlgorithmConstraints` 
instance. As a result the constraints defined in `SSLAlgorithmConstraints` are 
evaluated twice.

This PR improves the default case; if the user-specified constraints are left 
at defaults, we use a single `SSLAlgorithmConstraints` instance, and avoid 
duplicate checks.


Nice work. I've been looking at this area myself in recent weeks also while 
debugging some JDK 11u performance issues. JFR shows this area as costly. Some 
other JDK fixes in this area have already improved performance. This will 
certainly help. Pasting a stacktrace[1] from an old Oracle JDK 11.0.12 report 
for history purposes.

I think there are other improvements that can be made in the TLS constraints 
code. Caching the last known values from a constraints permits test is 
something I've begun looking into.

[1]
Stack Trace Count   Percentage
boolean java.lang.StringLatin1.regionMatchesCI(byte[], int, byte[], int, int)   
1637100 %
boolean java.lang.String.regionMatches(boolean, int, String, int, int)  1637
100 %
boolean java.lang.String.equalsIgnoreCase(String)   1637100 %
boolean sun.security.util.AbstractAlgorithmConstraints.checkAlgorithm(List, 
String, AlgorithmDecomposer)163199.6 %
boolean sun.security.util.DisabledAlgorithmConstraints.permits(Set, String, 
AlgorithmParameters)163199.6 %
boolean sun.security.ssl.SSLAlgorithmConstraints.permits(Set, String, 
AlgorithmParameters)  163199.6 %
boolean sun.security.ssl.SSLAlgorithmConstraints.permits(Set, String, 
AlgorithmParameters)  836 51.1 %
boolean sun.security.ssl.HandshakeContext.isActivatable(CipherSuite, 
AlgorithmConstraints, Map) 428 26.1 %
List sun.security.ssl.HandshakeContext.getActiveCipherSuites(List, List, 
AlgorithmConstraints)  418 25.5 %
void sun.security.ssl.HandshakeContext.(SSLContextImpl, TransportContext) 
  418 25.5 %
void sun.security.ssl.ClientHandshakeContext.(SSLContextImpl, 
TransportContext) 418 25.5 %
void sun.security.ssl.TransportContext.kickstart()  418 25.5 %
void sun.security.ssl.SSLSocketImpl.startHandshake(boolean) 418 25.5 %
void sun.security.ssl.SSLSocketImpl.startHandshake()418 25.5 %

-

Marked as reviewed by coffeys (Reviewer).

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

Java Cryptographic Extension Survey

[Anthony Scarpino]

Hello,

Java Cryptographic Extension (JCE) has been in Java SE for a long time 
and has made incremental changes over the years.  Looking forward, we 
would like to know more about how projects are using JCE and what 
changes, features, and API enhancements would be helpful for your projects.


Below is a survey that we hope you can spend 5 minutes to fill out.  If 
you have written or maintain code that uses the JCE API, then we would 
appreciate if you would complete this survey:


https://www.questionpro.com/t/AUzP7ZrFWv

The survey will be open through April 29.

Thank you,
Anthony Scarpino
OpenJDK Security Group
https://openjdk.java.net/groups/security/

Re: [Internet]Re: "Pluggable" key serialization in JCE/JCA

[Anthony Scarpino]

Thanks for all the info. We don’t have experience with JOSE or COSE, I think we 
need to digest some of this data before making a future step 

Not knowing this technology until you brought it up a few days ago, a few 
questions i have are how is this used and how common?  Would I see this used 
for more secure sites like banks or shopping through the browser or it more 
common sites. Is it only browser-based or are their other used cases?  Bernd 
mentioned QR codes, is COSE used inside the QR code or the authentication for 
the user to get to their QR code?

Thanks

Tony

> On Mar 26, 2022, at 11:48 PM, Anders Rundgren  
> wrote:
> 
> On 2022-03-26 23:14, Bernd Eckenfels wrote:
>> Just for completeness, the standard for key transport in JOSE is JWK 
>> (RFC7517).
>> In COSE it is a COSE_Key(Set) as defined in RFC8152 sect13.
>> BTW the most widely used CBOR/COSE application are probably the QR codes 
>> around Covid and Vaccination certificates of the EU.
> 
> Thanx Bernd and Michael for the additional clarifications!
> 
> Now to the thing that spurred this discussion: 
> https://datatracker.ietf.org/doc/html/rfc8037
> 
>   This document defines how to use the Diffie-Hellman algorithms
>   "X25519" and "X448" as well as the signature algorithms "Ed25519" and
>   "Ed448" from the IRTF CFRG elliptic curves work in JSON Object
>   Signing and Encryption (JOSE).
> 
> When RFC 8037 was created the assumption was that the "OKP" key container 
> {sh|c}ould be used for other crypto systems having the same parameter set.  
> This is now an active proposal:
> https://www.ietf.org/archive/id/draft-looker-cose-bls-key-representations-00.html
> 
> Obviously everything works just fine if you look at the container in 
> isolation. However, it means that "OKP" encoder/decoder code must be updated 
> for every new reuse ("overloading").  To be meaningful these new algorithms 
> would also have to coerced into the existing XEC or EdDSA interfaces.
> 
> IMO, this would be VERY UNFORTUNATE since it is incompatible with the 
> provider concept as well as with object oriented crypto APIs.  I'm therefore 
> suggesting that key containers for specific crypto systems should have unique 
> names.  In this particular case "BLS" seems logical.  In Java it could 
> eventually be mapped to BLSPublicKey and BLSPrivateKey.
> 
> WDYT?
> 
> There is no need for a JEP at this stage, only some kind of indication of 
> what the JDK crypto team see as the best way forward from your horizon.  The 
> same discussion has emerged for Post Quantum Crypto algorithms.
> 
> Thanx,
> Anders

Re: "Pluggable" key serialization in JCE/JCA

[Anthony Scarpino]

When you say "construct and EC key", do you mean creating an EC key from 
an existing set of values via PKCS8 or X509 encoding?  Or are you 
talking about EC key generation?


Tony

On 3/25/22 1:03 AM, Anders Rundgren wrote:

Hi Mike & the JDK crypto team,

What I'm saying is that key serialization in Java haven't gotten enough 
attention.  Examples:


AFAIK, there is still no support for using named curves to construct an 
EC key.  Names curves are MANDATORY in JOSE/CODE.


The gap between EdDSA keys as expressed in JOSE/COSE and the Java API is 
simply put GIGANTIC:
https://mail.openjdk.java.net/pipermail/security-dev/2020-August/022379.html 


The only obvious solution is rolling your own:
https://github.com/cyberphone/openkeystore/blob/master/library/src/org/webpki/crypto/OkpSupport.java 


This code [probably] stinks but it is hardly my fault...

The JDK crypto team is the only party who can clean up this mess.

Since importing X.509 certificates through PKCS12/KeyStore works out of 
the box without any a prior knowledge of key algorithms, there is 
already a precedent.  It is almost just copy and paste :)


The new crypto systems that are on the way in (BLS, Post Quantum), 
accentuates this issue.


Regarding HSMs, I gave it one more thought and I didn't come up with any 
connections except maybe in Keytool but HSMs appear to be X.509-oriented 
which not the case with COSE/JOSE.


Regards,
Anders

On 2022-03-25 3:12, Michael StJohns wrote:

On 3/24/2022 12:28 PM, Anders Rundgren wrote:

On 2022-03-24 16:59, Michael StJohns wrote:

On 3/24/2022 2:46 AM, Anders Rundgren wrote:

Hi List,

I find it a bit strange that every user of crypto either have to write
or install specific software for converting JOSE/COSE/PEM keys
back-and-forth to Java's internal representation. This reduces the
value of the abstract types as well.

Now there is whole bunch of new algorithms coming to the Java platform
making this task even less attractive.

So my question is simply: How about including this part in an enhanced
JCE/JCA?  That is, making the encoder/decoder "pluggable" and hiding
this complexity from users and library developers?


Hi Anders -


Hi Mike,



Isn't that the exact purpose for KeyFactory and its plugins?


They presume that you know not only the key algorithm but the
associated parameters as well.  Why should users or library developers
have to deal with that?


Um - no.  They presume you know the key algorithm, but (using EC Public
keys for example), you can use either of the X509PublicKeySpec (to
decode an encoded key), or the ECPublicKeySpec (to deal with building a
key from scratch using the parameters.  You can notionally (haven't
tried it, but should work) use KeyFactory.getKeySpec() to convert
between the two formats or use Key.getEncoded() to get the default
encoding for the key.

The equivalent on the ECPrivateKey side are the ECPrivateKeySpec and
PKCS8EncodedKeySpec clases.

PEM is actually not an encoding, but a wrapping of an encoding. The only
part of Java that deals with it natively (I believe) is the
CertificateFactory for X.509 certificates.  You may have an extra step
to add or remove a PEM style envelope (or the equivalent BASE64 bare
string), but that's reasonable.



The requirement to know key algorithm in advance forces you into ASN.1
decoding for dealing with PEMs.  Been there done that.


Ah - not sure why you wouldn't know the key algorithm, BUT:  It should
theoretically be possible to write a KeyFactory provider that does that
decoding for you from the X509PublicKeySpec (or PKCS8PrivateKeySpec) and
returns an appropriate PublicKey. Check the actual return type to figure
out what type of key.  E.g.:

KeyFactory kf = KeyFactory.getInstance("GenericDecoder");

As I said below, you'll need to define the equivalent opaque format
KeySpec classes to handle COSE and JOSE.  I think that's a class per
type of encoding.





You might need to add KeySpec types for Jose and Cose
(JOSEEncodedKeySpec and COSEEncodedKeySpec) assuming both of those 
cover

all of the secret, public and private key specifications.

Or hmm... GenericEncodedKeySpec (String alg, byte[] encoded key) or
GenericEncodedKeySpec (String alg, String encodedKey).




Eventually you could end up with something like:

PrivateKey privateKey = new KeyConverter().readPrivateKey(byte[] or
stream);

You would not even have to know in which format the key is supplied in
since this could be accomplished by simple "sniffing".


Nope.  This isn't safe as someone might up with yet another
representation that looks like one of the "sniffable" ones.  You could
build a private implementation that takes its best shot, but 


Finding out if the container is COSE, JOSE, or PEM is (AFAICT)
trivial.  If the guess is incorrect a properly designed decoder should
simply fail.


It's trivial NOW because its a closed set of possibilities.  And even
then, you're assuming you don't have to detect ASN1 vs raw key encodings
vs string 

Re: RFR: 8283665: Two Jarsigner tests needs to be updated with JDK-8267319

[Anthony Scarpino]

On Fri, 25 Mar 2022 05:11:18 GMT, Valerie Peng  wrote:

> Max, can you please help review this fix? It updates the two jarsigner tests 
> which are added to the main trunk during the code review of JDK-8267319.
> 
> Mach5 run succeeds.
> Thanks,
> Valerie

Marked as reviewed by ascarpino (Reviewer).

-

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

Re: RFR: 8267319: Use larger default key sizes and algorithms based on CNSA

[Anthony Scarpino]

On Wed, 2 Mar 2022 00:13:41 GMT, Valerie Peng  wrote:

> It's been several years since we increased the default key sizes. Before 
> shifting to PQC, NSA replaced its Suite B cryptography recommendations with 
> the Commercial National Security Algorithm Suite which suggests:
> 
> - SHA-384 for secure hashing
> - AES-256 for symmetric encryption
> - RSA with 3072 bit keys for digital signatures and for key exchange
> - Diffie Hellman (DH) with 3072 bit keys for key exchange
> - Elliptic curve [P-384] for key exchange (ECDH) and for digital signatures 
> (ECDSA)
> 
> So, this proposed changes made the suggested key size and algorithm changes. 
> The changes are mostly in keytool, jarsigner and their regression tests, so 
> @wangweij Could you please take a look?
> 
> Thanks!

Fair point that the keysize would determine the algorithm used and the default 
key sizes for keygen.

-

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

Re: RFR: 8267319: Use larger default key sizes and algorithms based on CNSA

[Anthony Scarpino]

On Wed, 2 Mar 2022 00:13:41 GMT, Valerie Peng  wrote:

> It's been several years since we increased the default key sizes. Before 
> shifting to PQC, NSA replaced its Suite B cryptography recommendations with 
> the Commercial National Security Algorithm Suite which suggests:
> 
> - SHA-384 for secure hashing
> - AES-256 for symmetric encryption
> - RSA with 3072 bit keys for digital signatures and for key exchange
> - Diffie Hellman (DH) with 3072 bit keys for key exchange
> - Elliptic curve [P-384] for key exchange (ECDH) and for digital signatures 
> (ECDSA)
> 
> So, this proposed changes made the suggested key size and algorithm changes. 
> The changes are mostly in keytool, jarsigner and their regression tests, so 
> @wangweij Could you please take a look?
> 
> Thanks!

I have some compatibility concerns about the AES change breaking code that 
expects a SecretKeySpec of 16 bytes. I can see situations where '.getEncoded()' 
returns a byte[32] when user code expects a byte[16]. Also, I'm pretty sure 
passing a 32 byte SecretKeySpec into an AES_128_GCM op will throw an exception. 
 I haven't looked at other modes.

-

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

Re: RFR: 8275914: SHA3: changing java implementation to help C2 create high-performance code [v3]

[Anthony Scarpino]

On Tue, 1 Feb 2022 09:52:46 GMT, Boris Ulasevich  wrote:

>> Background
>> 
>> The goal is to improve SHA3 implementation performance as it runs up to two 
>> times slower than native (OpenSSL, measured on AMD64 and AArch6464) 
>> implementation. Some hardware provides SHA3 accelerators, but most (AMD64 
>> and most AArch64) do not.
>> 
>> For AArch64 hardware that does support SHA3 accelerators, an intrinsic was 
>> implemented for ARMv8.2 with SHA3 instructions support:
>> - [JDK-8252204](https://bugs.openjdk.java.net/browse/JDK-8252204): AArch64: 
>> Implement SHA3 accelerator/intrinsic
>> 
>> SHA3 java implementation have already been reworked to eliminate memory 
>> consumption and make it work faster:
>> - [JDK-8157495](https://bugs.openjdk.java.net/browse/JDK-8157495): SHA-3 
>> Hash algorithm performance improvements (~12x speedup)
>> The latter issue addressed this previously, but there is still some room to 
>> improve SHA3 performance with current C2 implementation, which is proposed 
>> in this PR.
>> 
>> Option 1 (this PR)
>> 
>> With this change I unroll the internal cycles manually, inline it manually, 
>> and use local variables (not array) for data processing. Such an approach 
>> gives the best performance (see benchmark results). Without this change 
>> (with current array data processing) we observe a lot of load/store 
>> operations in comparison to processing in local variables, both on AMD64 and 
>> on AArch64.  
>> Native implementations shows that on AArch64 (32 GPU registers) SHA-3 
>> algorithm can hold all 25 data and all temporary variables in registers. C2 
>> can't optimize it as well because many regisers are allocated for internal 
>> usage: rscratch1, rscratch2, rmethod, rthread, etc.  With data in local 
>> variables the number of excessive load/stores is much smaller and 
>> performance result is much better.
>> 
>> Option 2 (alternative)
>> 
>> LINK: [the change](https://github.com/bulasevich/jdk/commit/095fc9db)
>> 
>> This is a more conservative change which minimizes code changes, but it has 
>> lower performance improvement. Please let me know if you think this change 
>> is better then main one: in this case I will replace it within this Pull 
>> Request.
>> 
>> With this change I unroll the internal cycles manually and use @Forceinline 
>> annotation. Manual unrolling is necessary because C2 does not recognize 
>> there is a counted cycle that can be completely unrolled. Instead of 
>> replacing the loop with five loop bodies C2 splits the loop to pre- main- 
>> and post- loop which is not good for this case. C2 works better when the 
>> array is created locally, but in our case the array is created on object 
>> instantiation, so C2 can't prove the array length is constant. The second 
>> issue affecting performance is that methods with unrolled loops get larger 
>> and C2 does not inline them. It's addressed here by using @Forceinline 
>> annotation.
>> 
>> Benchmark results
>> 
>> We measured the change on four platforms: ARM32, PPC, AArch64 (with no 
>> advanced SHA-3 instructions) and AMD on
>>   
>> [MessageDigests](https://github.com/openjdk/jdk/blob/master/test/micro/org/openjdk/bench/java/security/MessageDigests.java)
>>  benchmark. Here is the result: 
>> http://cr.openjdk.java.net/~bulasevich/8275914/sha3_bench.html
>> - fix1 (red) is the Option 1 (this PR) change: gains 50/38/83/38% on 
>> ARM32/PPC/AArch64/AMD64
>> - fix2 (green) is the Option 2 (alternative) change: gains 23/33/40/17% on 
>> ARM32/PPC/AArch64/AMD64
>> 
>> Testing
>> 
>> Tested with JTREG and SHA-3 Project (NIST) test vectors: run SHA3 
>> implementation over the same vectors before and after the change, and 
>> checking the results are the same.
>> The test tool: http://cr.openjdk.java.net/~bulasevich/8275914/RunKat.java
>> The test vectors compiled from the [Final Algorithm 
>> Package](https://csrc.nist.gov/Projects/Hash-Functions/SHA-3-Project):
>> http://cr.openjdk.java.net/~bulasevich/8275914/MsgKAT_224.txt
>> http://cr.openjdk.java.net/~bulasevich/8275914/MsgKAT_256.txt
>> http://cr.openjdk.java.net/~bulasevich/8275914/MsgKAT_384.txt
>> http://cr.openjdk.java.net/~bulasevich/8275914/MsgKAT_512.txt
>
> Boris Ulasevich has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   copyright fix

Looks good to me

-

Marked as reviewed by ascarpino (Reviewer).

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

Re: RFR: 8275914: SHA3: changing java implementation to help C2 create high-performance code

[Anthony Scarpino]

On Wed, 15 Dec 2021 09:20:38 GMT, Boris Ulasevich  
wrote:

> Background
> 
> The goal is to improve SHA3 implementation performance as it runs up to two 
> times slower than native (OpenSSL, measured on AMD64 and AArch6464) 
> implementation. Some hardware provides SHA3 accelerators, but most (AMD64 and 
> most AArch64) do not.
> 
> For AArch64 hardware that does support SHA3 accelerators, an intrinsic was 
> implemented for ARMv8.2 with SHA3 instructions support:
> - [JDK-8252204](https://bugs.openjdk.java.net/browse/JDK-8252204): AArch64: 
> Implement SHA3 accelerator/intrinsic
> 
> SHA3 java implementation have already been reworked to eliminate memory 
> consumption and make it work faster:
> - [JDK-8157495](https://bugs.openjdk.java.net/browse/JDK-8157495): SHA-3 Hash 
> algorithm performance improvements (~12x speedup)
> The latter issue addressed this previously, but there is still some room to 
> improve SHA3 performance with current C2 implementation, which is proposed in 
> this PR.
> 
> Option 1 (this PR)
> 
> With this change I unroll the internal cycles manually, inline it manually, 
> and use local variables (not array) for data processing. Such an approach 
> gives the best performance (see benchmark results). Without this change (with 
> current array data processing) we observe a lot of load/store operations in 
> comparison to processing in local variables, both on AMD64 and on AArch64.  
> Native implementations shows that on AArch64 (32 GPU registers) SHA-3 
> algorithm can hold all 25 data and all temporary variables in registers. C2 
> can't optimize it as well because many regisers are allocated for internal 
> usage: rscratch1, rscratch2, rmethod, rthread, etc.  With data in local 
> variables the number of excessive load/stores is much smaller and performance 
> result is much better.
> 
> Option 2 (alternative)
> 
> LINK: [the change](https://github.com/bulasevich/jdk/commit/095fc9db)
> 
> This is a more conservative change which minimizes code changes, but it has 
> lower performance improvement. Please let me know if you think this change is 
> better then main one: in this case I will replace it within this Pull Request.
> 
> With this change I unroll the internal cycles manually and use @Forceinline 
> annotation. Manual unrolling is necessary because C2 does not recognize there 
> is a counted cycle that can be completely unrolled. Instead of replacing the 
> loop with five loop bodies C2 splits the loop to pre- main- and post- loop 
> which is not good for this case. C2 works better when the array is created 
> locally, but in our case the array is created on object instantiation, so C2 
> can't prove the array length is constant. The second issue affecting 
> performance is that methods with unrolled loops get larger and C2 does not 
> inline them. It's addressed here by using @Forceinline annotation.
> 
> Benchmark results
> 
> We measured the change on four platforms: ARM32, PPC, AArch64 (with no 
> advanced SHA-3 instructions) and AMD on
>   
> [MessageDigests](https://github.com/openjdk/jdk/blob/master/test/micro/org/openjdk/bench/java/security/MessageDigests.java)
>  benchmark. Here is the result: 
> http://cr.openjdk.java.net/~bulasevich/8275914/sha3_bench.html
> - fix1 (red) is the Option 1 (this PR) change: gains 50/38/83/38% on 
> ARM32/PPC/AArch64/AMD64
> - fix2 (green) is the Option 2 (alternative) change: gains 23/33/40/17% on 
> ARM32/PPC/AArch64/AMD64
> 
> Testing
> 
> Tested with JTREG and SHA-3 Project (NIST) test vectors: run SHA3 
> implementation over the same vectors before and after the change, and 
> checking the results are the same.
> The test tool: http://cr.openjdk.java.net/~bulasevich/8275914/RunKat.java
> The test vectors compiled from the [Final Algorithm 
> Package](https://csrc.nist.gov/Projects/Hash-Functions/SHA-3-Project):
> http://cr.openjdk.java.net/~bulasevich/8275914/MsgKAT_224.txt
> http://cr.openjdk.java.net/~bulasevich/8275914/MsgKAT_256.txt
> http://cr.openjdk.java.net/~bulasevich/8275914/MsgKAT_384.txt
> http://cr.openjdk.java.net/~bulasevich/8275914/MsgKAT_512.txt

Please update the copyright date from 2020 to 2022.
I'll run regression tests to make sure things are ok.

-

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

Re: RFR: 8275914: SHA3: changing java implementation to help C2 create high-performance code

[Anthony Scarpino]

On Wed, 15 Dec 2021 09:20:38 GMT, Boris Ulasevich  
wrote:

> Background
> 
> The goal is to improve SHA3 implementation performance as it runs up to two 
> times slower than native (OpenSSL, measured on AMD64 and AArch6464) 
> implementation. Some hardware provides SHA3 accelerators, but most (AMD64 and 
> most AArch64) do not.
> 
> For AArch64 hardware that does support SHA3 accelerators, an intrinsic was 
> implemented for ARMv8.2 with SHA3 instructions support:
> - [JDK-8252204](https://bugs.openjdk.java.net/browse/JDK-8252204): AArch64: 
> Implement SHA3 accelerator/intrinsic
> 
> SHA3 java implementation have already been reworked to eliminate memory 
> consumption and make it work faster:
> - [JDK-8157495](https://bugs.openjdk.java.net/browse/JDK-8157495): SHA-3 Hash 
> algorithm performance improvements (~12x speedup)
> The latter issue addressed this previously, but there is still some room to 
> improve SHA3 performance with current C2 implementation, which is proposed in 
> this PR.
> 
> Option 1 (this PR)
> 
> With this change I unroll the internal cycles manually, inline it manually, 
> and use local variables (not array) for data processing. Such an approach 
> gives the best performance (see benchmark results). Without this change (with 
> current array data processing) we observe a lot of load/store operations in 
> comparison to processing in local variables, both on AMD64 and on AArch64.  
> Native implementations shows that on AArch64 (32 GPU registers) SHA-3 
> algorithm can hold all 25 data and all temporary variables in registers. C2 
> can't optimize it as well because many regisers are allocated for internal 
> usage: rscratch1, rscratch2, rmethod, rthread, etc.  With data in local 
> variables the number of excessive load/stores is much smaller and performance 
> result is much better.
> 
> Option 2 (alternative)
> 
> LINK: [the change](https://github.com/bulasevich/jdk/commit/095fc9db)
> 
> This is a more conservative change which minimizes code changes, but it has 
> lower performance improvement. Please let me know if you think this change is 
> better then main one: in this case I will replace it within this Pull Request.
> 
> With this change I unroll the internal cycles manually and use @Forceinline 
> annotation. Manual unrolling is necessary because C2 does not recognize there 
> is a counted cycle that can be completely unrolled. Instead of replacing the 
> loop with five loop bodies C2 splits the loop to pre- main- and post- loop 
> which is not good for this case. C2 works better when the array is created 
> locally, but in our case the array is created on object instantiation, so C2 
> can't prove the array length is constant. The second issue affecting 
> performance is that methods with unrolled loops get larger and C2 does not 
> inline them. It's addressed here by using @Forceinline annotation.
> 
> Benchmark results
> 
> We measured the change on four platforms: ARM32, PPC, AArch64 (with no 
> advanced SHA-3 instructions) and AMD on
>   
> [MessageDigests](https://github.com/openjdk/jdk/blob/master/test/micro/org/openjdk/bench/java/security/MessageDigests.java)
>  benchmark. Here is the result: 
> http://cr.openjdk.java.net/~bulasevich/8275914/sha3_bench.html
> - fix1 (red) is the Option 1 (this PR) change: gains 50/38/83/38% on 
> ARM32/PPC/AArch64/AMD64
> - fix2 (green) is the Option 2 (alternative) change: gains 23/33/40/17% on 
> ARM32/PPC/AArch64/AMD64
> 
> Testing
> 
> Tested with JTREG and SHA-3 Project (NIST) test vectors: run SHA3 
> implementation over the same vectors before and after the change, and 
> checking the results are the same.
> The test tool: http://cr.openjdk.java.net/~bulasevich/8275914/RunKat.java
> The test vectors compiled from the [Final Algorithm 
> Package](https://csrc.nist.gov/Projects/Hash-Functions/SHA-3-Project):
> http://cr.openjdk.java.net/~bulasevich/8275914/MsgKAT_224.txt
> http://cr.openjdk.java.net/~bulasevich/8275914/MsgKAT_256.txt
> http://cr.openjdk.java.net/~bulasevich/8275914/MsgKAT_384.txt
> http://cr.openjdk.java.net/~bulasevich/8275914/MsgKAT_512.txt

Ah yes.  Sorry, I saw the replied email that left that out

-

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

Re: RFR: 8275914: SHA3: changing java implementation to help C2 create high-performance code

[Anthony Scarpino]

On Wed, 15 Dec 2021 09:20:38 GMT, Boris Ulasevich  
wrote:

> Background
> 
> The goal is to improve SHA3 implementation performance as it runs up to two 
> times slower than native (OpenSSL, measured on AMD64 and AArch6464) 
> implementation. Some hardware provides SHA3 accelerators, but most (AMD64 and 
> most AArch64) do not.
> 
> For AArch64 hardware that does support SHA3 accelerators, an intrinsic was 
> implemented for ARMv8.2 with SHA3 instructions support:
> - [JDK-8252204](https://bugs.openjdk.java.net/browse/JDK-8252204): AArch64: 
> Implement SHA3 accelerator/intrinsic
> 
> SHA3 java implementation have already been reworked to eliminate memory 
> consumption and make it work faster:
> - [JDK-8157495](https://bugs.openjdk.java.net/browse/JDK-8157495): SHA-3 Hash 
> algorithm performance improvements (~12x speedup)
> The latter issue addressed this previously, but there is still some room to 
> improve SHA3 performance with current C2 implementation, which is proposed in 
> this PR.
> 
> Option 1 (this PR)
> 
> With this change I unroll the internal cycles manually, inline it manually, 
> and use local variables (not array) for data processing. Such an approach 
> gives the best performance (see benchmark results). Without this change (with 
> current array data processing) we observe a lot of load/store operations in 
> comparison to processing in local variables, both on AMD64 and on AArch64.  
> Native implementations shows that on AArch64 (32 GPU registers) SHA-3 
> algorithm can hold all 25 data and all temporary variables in registers. C2 
> can't optimize it as well because many regisers are allocated for internal 
> usage: rscratch1, rscratch2, rmethod, rthread, etc.  With data in local 
> variables the number of excessive load/stores is much smaller and performance 
> result is much better.
> 
> Option 2 (alternative)
> 
> LINK: [the change](https://github.com/bulasevich/jdk/commit/095fc9db)
> 
> This is a more conservative change which minimizes code changes, but it has 
> lower performance improvement. Please let me know if you think this change is 
> better then main one: in this case I will replace it within this Pull Request.
> 
> With this change I unroll the internal cycles manually and use @Forceinline 
> annotation. Manual unrolling is necessary because C2 does not recognize there 
> is a counted cycle that can be completely unrolled. Instead of replacing the 
> loop with five loop bodies C2 splits the loop to pre- main- and post- loop 
> which is not good for this case. C2 works better when the array is created 
> locally, but in our case the array is created on object instantiation, so C2 
> can't prove the array length is constant. The second issue affecting 
> performance is that methods with unrolled loops get larger and C2 does not 
> inline them. It's addressed here by using @Forceinline annotation.
> 
> Benchmark results
> 
> We measured the change on four platforms: ARM32, PPC, AArch64 (with no 
> advanced SHA-3 instructions) and AMD on
>   
> [MessageDigests](https://github.com/openjdk/jdk/blob/master/test/micro/org/openjdk/bench/java/security/MessageDigests.java)
>  benchmark. Here is the result: 
> http://cr.openjdk.java.net/~bulasevich/8275914/sha3_bench.html
> - fix1 (red) is the Option 1 (this PR) change: gains 50/38/83/38% on 
> ARM32/PPC/AArch64/AMD64
> - fix2 (green) is the Option 2 (alternative) change: gains 23/33/40/17% on 
> ARM32/PPC/AArch64/AMD64
> 
> Testing
> 
> Tested with JTREG and SHA-3 Project (NIST) test vectors: run SHA3 
> implementation over the same vectors before and after the change, and 
> checking the results are the same.
> The test tool: http://cr.openjdk.java.net/~bulasevich/8275914/RunKat.java
> The test vectors compiled from the [Final Algorithm 
> Package](https://csrc.nist.gov/Projects/Hash-Functions/SHA-3-Project):
> http://cr.openjdk.java.net/~bulasevich/8275914/MsgKAT_224.txt
> http://cr.openjdk.java.net/~bulasevich/8275914/MsgKAT_256.txt
> http://cr.openjdk.java.net/~bulasevich/8275914/MsgKAT_384.txt
> http://cr.openjdk.java.net/~bulasevich/8275914/MsgKAT_512.txt

Sorry this feel off my stack with the holidays and everything else going on.
I am fine proceeding change in this PR, option 1 I believe. Have you run the 
tests to make sure it passes the Known Answer Tests?
Can you also include the before and after JMH benchmarks for SHA3 in the PR?  
they are under test/micro/org/openjdk/bench/java/security/MessageDigests

Thanks

-

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

Re: RFR: JDK-8280703 CipherCore.doFinal(...) causes potentially massive byte[] allocations during decryption [v2]

[Anthony Scarpino]

On Wed, 26 Jan 2022 18:07:24 GMT, Sebastian Stenzel  
wrote:

>> Related to #411, however it turns out that for unpadded ciphers, there is no 
>> need to allocate `internalOutput`, if `output` provides sufficient capacity.
>> 
>> For padded ciphers, only the unpadded cleartext is expected to be copied to 
>> the output buffer. In this case, there is no way around the temporary buffer 
>> (without major changes).
>> 
>> While a small change, please review with care, as I might be missing some 
>> security-relevant side effect (such as: don't copy cleartext to output 
>> buffer before validating the a tag - just as an example, even if there is no 
>> authentication involved in this method).
>> 
>> I have some test failures in Tier 1 tests, but these seem to be unrelated. 
>> Tests for `com.sun.crypto` and `javax.crypto` run fine:
>> 
>> 
>> ==
>> Test summary
>> ==
>>TEST  TOTAL  PASS  FAIL ERROR 
>>   
>>jtreg:test/jdk/com/sun/crypto   141   141 0 0 
>>   
>>jtreg:test/jdk/javax/crypto  5656 0 0 
>>   
>> ==
>> TEST SUCCESS
>
> Sebastian Stenzel has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   updated copyright header

The diff passed tier1 and crypto tests.  Looks good to me

-

Marked as reviewed by ascarpino (Reviewer).

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

Re: RFR: JDK-8280703 CipherCore.doFinal(...) causes potentially massive byte[] allocations during decryption

[Anthony Scarpino]

On Wed, 26 Jan 2022 10:07:01 GMT, Sebastian Stenzel  
wrote:

> Related to #411, however it turns out that for unpadded ciphers, there is no 
> need to allocate `internalOutput`, if `output` provides sufficient capacity.
> 
> For padded ciphers, only the unpadded cleartext is expected to be copied to 
> the output buffer. In this case, there is no way around the temporary buffer 
> (without major changes).
> 
> While a small change, please review with care, as I might be missing some 
> security-relevant side effect (such as: don't copy cleartext to output buffer 
> before validating the a tag - just as an example, even if there is no 
> authentication involved in this method).
> 
> I have some test failures in Tier 1 tests, but these seem to be unrelated. 
> Tests for `com.sun.crypto` and `javax.crypto` run fine:
> 
> 
> ==
> Test summary
> ==
>TEST  TOTAL  PASS  FAIL ERROR  
>  
>jtreg:test/jdk/com/sun/crypto   141   141 0 0  
>  
>jtreg:test/jdk/javax/crypto  5656 0 0  
>  
> ==
> TEST SUCCESS

The copyright date will need to be updated from 2021 to 2022

-

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

Re: Lots of allocations in CipherCore

[Anthony Scarpino]

Hi,

I think it's a mistake. Looking at the old code I believe the if() for 
cipher.save() above that was suppose to include the new byte allocation 
and offset, but got missed.


Feel free to fix it if you like.  Let me know if you need something.

Tony

On 1/25/22 12:06 PM, Sebastian Stenzel wrote:

Hi all,

while playing around with JFR today, I stumbled upon a piece of code 
that causes a thousands of byte[] allocations. In fact it is responsible 
for 90% of the memory allocations in my application and causes GC to run 
without pause during decryption of large files.


The line in question can be found here [1]. The behaviour can be 
triggered with a simple AES-CTR cipher as in this minimal example [2].


As you can see, the comments in [1] even say that this byte[] _should_ 
only be allocated if there is insufficient space in the provided output 
buffer. However some kind of conditional statement is missing.


There used to be a `if (getMode() != GCM_MODE || outputCapacity < 
estOutSize)`, before GCM has been decoupled from CipherCore. But this 
line seems wrong to me either.


I guess this should be fixed, as I'm probably not the only one affected 
by those allocations. I'd be volunteering, unless the behaviour is intended.


Cheers!
Sebastian


[1] 
https://github.com/openjdk/jdk/blob/76fe03fe01a7c824e2e9263de95b8bcbb4b9d752/src/java.base/share/classes/com/sun/crypto/provider/CipherCore.java#L815-L818 

[2] 
https://gist.github.com/overheadhunter/f3969950c0fdbaecaa77c857b2246cc5 

Re: RFR: 8277881 Missing SessionID in TLS1.3 resumption in compatibility mode [v2]

[Anthony Scarpino]

On Thu, 23 Dec 2021 21:47:53 GMT, Daniel Jeliński  wrote:

>> All TLS 1.3 handshakes in compatibility mode must send a non-empty SessionID 
>> field. Currently TLS1.3 session resumptions are sending empty session ID. 
>> This patch fixes that problem.
>> 
>> All jdk_core tests passed. The newly added check passes with the patch, 
>> fails without it.
>
> Daniel Jeliński has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Update copyright year

Approved

-

Marked as reviewed by ascarpino (Reviewer).

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

Re: RFR: 8277881 Missing SessionID in TLS1.3 resumption in compatibility mode

[Anthony Scarpino]

On Mon, 29 Nov 2021 08:42:22 GMT, Daniel Jeliński  wrote:

> All TLS 1.3 handshakes in compatibility mode must send a non-empty SessionID 
> field. Currently TLS1.3 session resumptions are sending empty session ID. 
> This patch fixes that problem.
> 
> All jdk_core tests passed. The newly added check passes with the patch, fails 
> without it.

Please add " 2021," to the copyright of ResumeTLS13withSNI.java.
I have run all the tests and they pass.

Have you run this fix on your customer's setup or similar setup to confirm this 
fixed their problem?

-

Changes requested by ascarpino (Reviewer).

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

Re: RFR: 8255409: Support the new APIs in PKCS#11 v3.0

[Anthony Scarpino]

On Wed, 1 Dec 2021 21:42:51 GMT, Valerie Peng  wrote:

> PKCS#11 v3.0 adds the support for several new APIs. For this particular RFE, 
> it enhances SunPKCS11 provider to load PKCS#11 provider by first trying the 
> C_GetInterface (new in 3.0) before the C_GetFunctionList assuming not 
> explicitly specified in config. In addition, PKCS#11 v3.0 defines a new API 
> for cancelling session operations, so I've also updated various classes to 
> call this new API if the PKCS#11 library version is 3.0. Otherwise, these 
> classes will try to cancel by finishing off current operations as before. The 
> support for the new C_LoginUser() has not been tested, so I commented it out 
> for now. Given the current release schedule, support for other new PKCS#11 
> APIs (such as message-based ones and parameters structure) and options for 
> C_GetInterface (if needed) will be handled later. 
> 
> I validated the current changes against different NSS releases (supports 
> PKCS#11 v2.40 and v3..0 respectively) with existing regression tests.
> 
> Thanks,
> Valerie

I updated my comments because I neglected to read your initial message and went 
straight to the code review

-

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

Re: RFR: 8255409: Support the new APIs in PKCS#11 v3.0

[Anthony Scarpino]

On Wed, 1 Dec 2021 21:42:51 GMT, Valerie Peng  wrote:

> PKCS#11 v3.0 adds the support for several new APIs. For this particular RFE, 
> it enhances SunPKCS11 provider to load PKCS#11 provider by first trying the 
> C_GetInterface (new in 3.0) before the C_GetFunctionList assuming not 
> explicitly specified in config. In addition, PKCS#11 v3.0 defines a new API 
> for cancelling session operations, so I've also updated various classes to 
> call this new API if the PKCS#11 library version is 3.0. Otherwise, these 
> classes will try to cancel by finishing off current operations as before. The 
> support for the new C_LoginUser() has not been tested, so I commented it out 
> for now. Given the current release schedule, support for other new PKCS#11 
> APIs (such as message-based ones and parameters structure) and options for 
> C_GetInterface (if needed) will be handled later. 
> 
> I validated the current changes against different NSS releases (supports 
> PKCS#11 v2.40 and v3..0 respectively) with existing regression tests.
> 
> Thanks,
> Valerie

Are there no tests because there is no support for 3.0 in nss?

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11AEADCipher.java 
line 406:

> 404: token.ensureValid();
> 405: if (token.p11.getVersion().major == 3) {
> 406: long flags = (encrypt? CKF_ENCRYPT : CKF_DECRYPT);

I think this is a syntax nit with no space between "encrypt" and '?'

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java line 
449:

> 447: token.ensureValid();
> 448: if (token.p11.getVersion().major == 3) {
> 449: long flags = (encrypt? CKF_ENCRYPT : CKF_DECRYPT);

I think this is a syntax nit with no space between "encrypt" and '?'

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11KeyWrapCipher.java 
line 291:

> 289: 
> 290: if (token.p11.getVersion().major == 3) {
> 291: long flags = (opmode == Cipher.ENCRYPT_MODE? CKF_ENCRYPT :

I think this is a syntax nit with no space between MODE and '?'

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11PSSSignature.java 
line 275:

> 273: 
> 274: if (token.p11.getVersion().major == 3) {
> 275: long flags = (mode == M_SIGN? CKF_SIGN : CKF_VERIFY);

I think this is a syntax nit with no space between M_SIGN and '?'

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java 
line 285:

> 283: 
> 284: if (token.p11.getVersion().major == 3) {
> 285: long flags = (mode == M_SIGN? CKF_SIGN : CKF_VERIFY);

M_SIGN ? ...

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/PKCS11.java 
line 558:

> 556: throws PKCS11Exception;
> 557: 
> 558: ///**

Was it intentional to have the below commented out?

src/jdk.crypto.cryptoki/windows/native/libj2pkcs11/p11_md.c line 128:

> 126: }
> 127: 
> 128: #ifdef DEBUG

Is C_GetInterfaceList in DEBUG only because it's a new 3.0 function we are not 
supporting yet?

-

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

Re: RFR: 8276863: Remove test/jdk/sun/security/ec/ECDSAJavaVerify.java

[Anthony Scarpino]

On Tue, 9 Nov 2021 14:23:54 GMT, Weijun Wang  wrote:

> The test was added in JDK-8237218 to confirm that Java impl is used when 
> verifying a signature. It is useless now since the native implementation is 
> completely removed.

Look good

-

Marked as reviewed by ascarpino (Reviewer).

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

Withdrawn: 8158689: java.security.KeyPair should implement Destroyable

[Anthony Scarpino]

On Fri, 22 Oct 2021 23:50:38 GMT, Anthony Scarpino  
wrote:

> Hi,
> 
> I need a review of this change.  It makes KeyPair implement Destroyable and 
> implements the methods to call the underlying privateKey.  It also sets the 
> public and private key to 'final'.
> 
> The bug includes a CSR and Release Notes
> CSR: https://bugs.openjdk.java.net/browse/JDK-8275823
> RN: https://bugs.openjdk.java.net/browse/JDK-8275826

This pull request has been closed without being integrated.

-

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

Re: RFR: 8158689: java.security.KeyPair should implement Destroyable

[Anthony Scarpino]

On Fri, 22 Oct 2021 23:50:38 GMT, Anthony Scarpino  
wrote:

> Hi,
> 
> I need a review of this change.  It makes KeyPair implement Destroyable and 
> implements the methods to call the underlying privateKey.  It also sets the 
> public and private key to 'final'.
> 
> The bug includes a CSR and Release Notes
> CSR: https://bugs.openjdk.java.net/browse/JDK-8275823
> RN: https://bugs.openjdk.java.net/browse/JDK-8275826

Even though it has been the only option for a long time, calling 
`keyPair.getPrivateKey().destory()` is not clean and one should not have had to 
go into a class's objects to cleanup.   It's reminiscent of free memory in C.

-

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

Re: RFR: 8158689: java.security.KeyPair should implement Destroyable

[Anthony Scarpino]

On Fri, 22 Oct 2021 23:50:38 GMT, Anthony Scarpino  
wrote:

> Hi,
> 
> I need a review of this change.  It makes KeyPair implement Destroyable and 
> implements the methods to call the underlying privateKey.  It also sets the 
> public and private key to 'final'.
> 
> The bug includes a CSR and Release Notes
> CSR: https://bugs.openjdk.java.net/browse/JDK-8275823
> RN: https://bugs.openjdk.java.net/browse/JDK-8275826

You can, it's just a multi-step process that should have been available from 
KeyPair in a single step process

-

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

RFR: 8158689: java.security.KeyPair should implement Destroyable

[Anthony Scarpino]

Hi,

I need a review of this change.  It makes KeyPair implement Destroyable and 
implements the methods to call the underlying privateKey.  It also sets the 
public and private key to 'final'.

The bug includes a CSR and Release Notes
CSR: https://bugs.openjdk.java.net/browse/JDK-8275823
RN: https://bugs.openjdk.java.net/browse/JDK-8275826

-

Commit messages:
 - initial fix

Changes: https://git.openjdk.java.net/jdk/pull/6089/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6089=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8158689
  Stats: 62 lines in 2 files changed: 55 ins; 0 del; 7 mod
  Patch: https://git.openjdk.java.net/jdk/pull/6089.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/6089/head:pull/6089

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

Re: RFR: 8243585: AlgorithmChecker::check throws confusing exception when it rejects the signer key [v3]

[Anthony Scarpino]

On Wed, 20 Oct 2021 14:47:31 GMT, Sean Mullan  wrote:

>> This fix improves the exception message to better indicate when the key (and 
>> not the signature algorithm) is restricted. This change also includes a few 
>> other improvements:
>> 
>> - The constraints checking in `AlgorithmChecker.check()` has been improved. 
>> If the `AlgorithmConstraints` are an instance of 
>> `DisabledAlgorithmConstraints`, the internal `permits` methods are always 
>> called; otherwise the public `permits` methods are called. This makes the 
>> code easier to understand, and fixes at least one case where duplicate 
>> checks were being done.
>> 
>> - The above change caused some of the exception messages to be slightly 
>> different, so some tests that checked the error messages had to be updated 
>> to reflect that.
>> 
>> - AlgorithmDecomposer now stores the decomposed SHA algorithm names in a 
>> Map, which fixed a bug where "RSASSA-PSS" was not being restricted properly.
>
> Sean Mullan has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   - Skip digest alg decomposing check for algorithms that don't contain "SHA".
>   - Remove hasLoop method and fold code into decomposeName method.

looks good to me

-

Marked as reviewed by ascarpino (Reviewer).

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

Re: RFR: 8243585: AlgorithmChecker::check throws confusing exception when it rejects the signer key [v2]

[Anthony Scarpino]

On Wed, 20 Oct 2021 13:34:44 GMT, Sean Mullan  wrote:

>> src/java.base/share/classes/sun/security/util/AlgorithmDecomposer.java line 
>> 106:
>> 
>>> 104: // "SHA-256" and "SHA256" to make the right constraint 
>>> checking.
>>> 105: 
>>> 106: for (Map.Entry e : 
>>> DECOMPOSED_DIGEST_NAMES.entrySet()) {
>> 
>> If you're going to change this code, you can save me a PR if you surround 
>> this by "if (algorithm.contains("SHA") {  ...  }"
>> Its a perf change to eliminate the unnecessary map lookups when SHA isn't in 
>> the algorithm string
>
> That's a fine suggestion, although I'll note that your suggested perf 
> improvement also applies to the previous code which did not check the 
> algorithm parameter first to see if it contained `SHA`.
> Also, another small perf imp: I realized below that in the loop, if the first 
> `if` block gets executed, then the 2nd `if` block will always be false, so I 
> changed it to an if/else.

Yes, I was about to submit a PR to change the previous code, since you changed 
this code it makes more sense to ask you to do it at the same time.

-

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

Re: RFR: 8243585: AlgorithmChecker::check throws confusing exception when it rejects the signer key [v2]

[Anthony Scarpino]

On Tue, 19 Oct 2021 20:32:21 GMT, Sean Mullan  wrote:

>> This fix improves the exception message to better indicate when the key (and 
>> not the signature algorithm) is restricted. This change also includes a few 
>> other improvements:
>> 
>> - The constraints checking in `AlgorithmChecker.check()` has been improved. 
>> If the `AlgorithmConstraints` are an instance of 
>> `DisabledAlgorithmConstraints`, the internal `permits` methods are always 
>> called; otherwise the public `permits` methods are called. This makes the 
>> code easier to understand, and fixes at least one case where duplicate 
>> checks were being done.
>> 
>> - The above change caused some of the exception messages to be slightly 
>> different, so some tests that checked the error messages had to be updated 
>> to reflect that.
>> 
>> - AlgorithmDecomposer now stores the canonical algorithm names in a Map, 
>> which fixed a bug where "RSASSA-PSS" was not being restricted properly.
>
> Sean Mullan has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   - Changed names of AlgorithmDecomposer.canonicalName and 
> decomposeOneHashName
>   methods.
>   - Changed other code in AlgorithmDecomposer to use DECOMPOSED_DIGEST_NAMES
>   Map instead of hardcoding algorithm names.
>   - Changed AlgorithmChecker.trySetTrustAnchor to set trustedPubKey field so 
> that
>   constraints on the key algorithm and size are checked in the check() method 
> if
>   the constraints are an instanceof DisabledAlgorithmConstraints.

src/java.base/share/classes/sun/security/util/AlgorithmDecomposer.java line 106:

> 104: // "SHA-256" and "SHA256" to make the right constraint checking.
> 105: 
> 106: for (Map.Entry e : 
> DECOMPOSED_DIGEST_NAMES.entrySet()) {

If you're going to change this code, you can save me a PR if you surround this 
by "if (algorithm.contains("SHA") {  ...  }"
Its a perf change to eliminate the unnecessary map lookups when SHA isn't in 
the algorithm string

src/java.base/share/classes/sun/security/util/AlgorithmDecomposer.java line 158:

> 156: Set elements = decomposeImpl(algorithm);
> 157: 
> 158: for (Map.Entry e : 
> DECOMPOSED_DIGEST_NAMES.entrySet()) {

Same "if (algorithm.contains("SHA") { ... }" comment as above

src/java.base/share/classes/sun/security/util/AlgorithmDecomposer.java line 159:

> 157: 
> 158: for (Map.Entry e : 
> DECOMPOSED_DIGEST_NAMES.entrySet()) {
> 159: hasLoop(elements, e.getKey(), e.getValue());

I think it's worth merging the contents of hasLoop() into this for loop.  This 
is the only method calling hasLoop() and the extra method calls are not useful. 
 Your addition DECOMPOSED_DIGEST_NAMES makes a merger a more reasonable 
solution now than before.

-

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

Re: RFR: 8243585: AlgorithmChecker::check throws confusing exception when it rejects the signer key

[Anthony Scarpino]

On Tue, 19 Oct 2021 15:48:57 GMT, Sean Mullan  wrote:

>> src/java.base/share/classes/sun/security/util/AlgorithmDecomposer.java line 
>> 196:
>> 
>>> 194: static String canonicalName(String algorithm) {
>>> 195: return CANONICAL_NAME.getOrDefault(algorithm, algorithm);
>>> 196: }
>> 
>> I'm not sure if `canonicalName` is good. Normally, we say "SHA-1" is the 
>> standard name but this method changes it to "SHA1".
>
> Right, it's really just about using consistent message digest names so that 
> it can match for example, "SHA-1" and also "SHA1withRSA". I'll change the 
> name to something else.

Was the reason for this change that hashName("RSASSA-PSS") was returning an 
RSASSAPSS?

-

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

Re: RFR: 8264849: Add KW and KWP support to PKCS11 provider [v2]

[Anthony Scarpino]

On Mon, 11 Oct 2021 23:32:26 GMT, Valerie Peng  wrote:

>> Anyone has time to review this RFE for adding AES cipher with KW, KWP modes 
>> support to SunPKCS11 provider?
>> 
>> The main changes are in only one new class, i.e. P11KeyWrapCipher.java, 
>> which is the CipherSpi impl for the native PKCS11 key wrap mechanisms. When 
>> testing against NSS library, it seems that they only support the single part 
>> enc/dec PKCS11 APIs, so have to use a new class as existing P11Cipher class 
>> relies on the multi part enc/dec PKCS11 APIs and do not support key 
>> wrapping/unwrapping.
>> 
>> The rest are minor code refactoring and updates for the PKCS11 Exception 
>> class.
>> The new regression tests are adapted from existing key wrap regression tests 
>> for SunJCE provider.
>> 
>> Thanks,
>> Valerie
>
> Valerie Peng has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Minor update for review comments.

I have marked the CSR as reviewed

-

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

Re: RFR: 8264849: Add KW and KWP support to PKCS11 provider [v2]

[Anthony Scarpino]

On Mon, 11 Oct 2021 23:32:26 GMT, Valerie Peng  wrote:

>> Anyone has time to review this RFE for adding AES cipher with KW, KWP modes 
>> support to SunPKCS11 provider?
>> 
>> The main changes are in only one new class, i.e. P11KeyWrapCipher.java, 
>> which is the CipherSpi impl for the native PKCS11 key wrap mechanisms. When 
>> testing against NSS library, it seems that they only support the single part 
>> enc/dec PKCS11 APIs, so have to use a new class as existing P11Cipher class 
>> relies on the multi part enc/dec PKCS11 APIs and do not support key 
>> wrapping/unwrapping.
>> 
>> The rest are minor code refactoring and updates for the PKCS11 Exception 
>> class.
>> The new regression tests are adapted from existing key wrap regression tests 
>> for SunJCE provider.
>> 
>> Thanks,
>> Valerie
>
> Valerie Peng has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Minor update for review comments.

The changes look good

-

Marked as reviewed by ascarpino (Reviewer).

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

Re: RFR: 8264849: Add KW and KWP support to PKCS11 provider

[Anthony Scarpino]

On Fri, 17 Sep 2021 23:22:21 GMT, Valerie Peng  wrote:

> Anyone has time to review this RFE for adding AES cipher with KW, KWP modes 
> support to SunPKCS11 provider?
> 
> The main changes are in only one new class, i.e. P11KeyWrapCipher.java, which 
> is the CipherSpi impl for the native PKCS11 key wrap mechanisms. When testing 
> against NSS library, it seems that they only support the single part enc/dec 
> PKCS11 APIs, so have to use a new class as existing P11Cipher class relies on 
> the multi part enc/dec PKCS11 APIs and do not support key wrapping/unwrapping.
> 
> The rest are minor code refactoring and updates for the PKCS11 Exception 
> class.
> The new regression tests are adapted from existing key wrap regression tests 
> for SunJCE provider.
> 
> Thanks,
> Valerie

test/jdk/sun/security/pkcs11/Cipher/KeyWrap/TestGeneral.java line 30:

> 28:  * AES/KW/PKCS5Padding, and AES/KWP/NoPadding impls of SunPKCS11 
> provider.
> 29:  * @library /test/lib ../..
> 30:  * @run main/othervm TestGeneral

General question about all the tests.  They use othervm, did they not work with 
agentvm?

test/jdk/sun/security/pkcs11/Cipher/KeyWrap/TestGeneral.java line 74:

> 72: }
> 73: 
> 74: //System.out.println("enc out.length=" + out.length);

Lelftover debug line?

-

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

Re: RFR: 8264849: Add KW and KWP support to PKCS11 provider

[Anthony Scarpino]

On Fri, 17 Sep 2021 23:22:21 GMT, Valerie Peng  wrote:

> Anyone has time to review this RFE for adding AES cipher with KW, KWP modes 
> support to SunPKCS11 provider?
> 
> The main changes are in only one new class, i.e. P11KeyWrapCipher.java, which 
> is the CipherSpi impl for the native PKCS11 key wrap mechanisms. When testing 
> against NSS library, it seems that they only support the single part enc/dec 
> PKCS11 APIs, so have to use a new class as existing P11Cipher class relies on 
> the multi part enc/dec PKCS11 APIs and do not support key wrapping/unwrapping.
> 
> The rest are minor code refactoring and updates for the PKCS11 Exception 
> class.
> The new regression tests are adapted from existing key wrap regression tests 
> for SunJCE provider.
> 
> Thanks,
> Valerie

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11AEADCipher.java 
line 824:

> 822: } else if (e.match(CKR_ENCRYPTED_DATA_INVALID) ||
> 823: e.match(CKR_GENERAL_ERROR)) {
> 824: // CKR_GENERAL_ERROR is Solaris-specific workaround

With Solaris no longer support, this could be removed.  Are you leaving it for 
backporting?

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11KeyWrapCipher.java 
line 57:

> 55:  * doFinal() is called.
> 56:  *
> 57:  * @since   18

Is there only suppose to be one space between `@since` and 18?

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11KeyWrapCipher.java 
line 129:

> 127: if (algoParts[0].startsWith("AES")) {
> 128: // need 3 parts
> 129: if (algoParts.length != 3) {

At this point in the code, isn't it already certain to be a valid transform?  
The SunPKCS11 entries are limited to the valid transforms.   Additionally do 
you really want AssertionError?  Not NoSuchAlgorithmException?

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11KeyWrapCipher.java 
line 437:

> 435: protected byte[] engineDoFinal(byte[] in, int inOfs, int inLen)
> 436: throws IllegalBlockSizeException, BadPaddingException {
> 437: int minOutLen = doFinalLength(inLen);

nit: seems like this could be maxOutLen given it's the length used to allocate 
out[].  It can't be any larger, otherwise the operations would fail

test/jdk/sun/security/pkcs11/Cipher/KeyWrap/TestCipherKeyWrapperTest.java line 
65:

> 63: // com/sun/crypto/provider/Cipher/KeyWrap/TestCipherKeyWrapperTest.java
> 64: public class TestCipherKeyWrapperTest extends PKCS11Test {
> 65: private static final int LINIMITED_KEYSIZE = 128;

Did you mean this to be LIMITED_KEYSIZE?

-

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

Re: RFR: 8264849: Add KW and KWP support to PKCS11 provider

[Anthony Scarpino]

On Fri, 17 Sep 2021 23:22:21 GMT, Valerie Peng  wrote:

> Anyone has time to review this RFE for adding AES cipher with KW, KWP modes 
> support to SunPKCS11 provider?
> 
> The main changes are in only one new class, i.e. P11KeyWrapCipher.java, which 
> is the CipherSpi impl for the native PKCS11 key wrap mechanisms. When testing 
> against NSS library, it seems that they only support the single part enc/dec 
> PKCS11 APIs, so have to use a new class as existing P11Cipher class relies on 
> the multi part enc/dec PKCS11 APIs and do not support key wrapping/unwrapping.
> 
> The rest are minor code refactoring and updates for the PKCS11 Exception 
> class.
> The new regression tests are adapted from existing key wrap regression tests 
> for SunJCE provider.
> 
> Thanks,
> Valerie

>From a high level, why does P11KeyWrapCipher support ENCRYPT and DECRYPT 
>modes?  I expected to only see UNWRAP and WRAP mode supported.  Along those 
>same lines I expected to only see C_WrapKey and C_UnwrapKey, and not 
>encryption/decryption pkcs11 calls.  Is there some additional support here 
>that I'm not seeing?

-

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

Re: RFR: 8274330: Incorrect encoding of the DistributionPointName object in IssuingDistributionPointExtension

[Anthony Scarpino]

On Sun, 26 Sep 2021 13:27:47 GMT, Weijun Wang  wrote:

> `DistributionPointName` in `IssuingDistributionPointExtension` is a CHOICE 
> and should not be encoded as IMPLICIT.
> 
> Please note that the parsing side (at 
> https://github.com/openjdk/jdk/blob/a9db70418f7bc6b2f95afdd36a36024f865c04bf/src/java.base/share/classes/sun/security/x509/IssuingDistributionPointExtension.java#L195)
>  is aware of this and has not called `resetTag()`.

looks find to me

-

Marked as reviewed by ascarpino (Reviewer).

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

Re: [External] : Re: Understanding elliptic curve spec limitations

[Anthony Scarpino]

When I read your first message I thought you were unable to use them using 
OpenSSL. 

Yes, the curves you are most likely looking for have been removed as the CSR 
describes. 

Tony

> On Sep 28, 2021, at 8:32 AM, David Blevins  wrote:
> 
> 
>> 
>>> On Sep 28, 2021, at 12:49 AM, David Blevins  wrote:
>>> 
>>>> On Sep 27, 2021, at 3:32 PM, Anthony Scarpino 
>>>>  wrote:
>>> 
>>> On 9/27/21 2:22 PM, David Blevins wrote:
>>>> I've been putting a significant amount of work into compiling a large set 
>>>> of elliptic curve parameters/names/oids for an open source library and a 
>>>> related closed source security product we have.  We need to be able to 
>>>> support any of the curves that OpenSSL/LibreSSL support.
>>>> The trick is this is currently impossible due to hardcoding in OpenJDK 16. 
>>>>  Though you supply valid parameters via ECParameterSpec, when you attempt 
>>>> to construct an instance of ECPrivateKey or ECPublicKey you hit code in 
>>>> sun.security.util.CurveDB that does a "reverse lookup" of sorts to find 
>>>> the curve name.  If it's not a curve CurveDB knows about, you can't use it.
>>>> Is there willingness to accept contributions that would remove this 
>>>> limitation?
>>> 
>>> We haven't heard such issues since native obsolete curves were removed from 
>>> 16.  We are will to take contributions upon review.  If you're going to 
>>> formally contribute the code, you should check out 
>>> https://openjdk.java.net/contribute/
>> 
>> Thanks, Tony.
>> 
>> It also appears that of the 60 curves supported only 3 of them can be used 
>> to sign/verify.  Any insight as to why?
> 
> Found my own answer on this second question.
> 
> - https://bugs.openjdk.java.net/browse/JDK-8251547
> 
> 
> -David
> 

Re: Understanding elliptic curve spec limitations

[Anthony Scarpino]

On 9/27/21 2:22 PM, David Blevins wrote:

I've been putting a significant amount of work into compiling a large set of 
elliptic curve parameters/names/oids for an open source library and a related 
closed source security product we have.  We need to be able to support any of 
the curves that OpenSSL/LibreSSL support.

The trick is this is currently impossible due to hardcoding in OpenJDK 16.  Though you 
supply valid parameters via ECParameterSpec, when you attempt to construct an instance of 
ECPrivateKey or ECPublicKey you hit code in sun.security.util.CurveDB that does a 
"reverse lookup" of sorts to find the curve name.  If it's not a curve CurveDB 
knows about, you can't use it.

Is there willingness to accept contributions that would remove this limitation?


We haven't heard such issues since native obsolete curves were removed 
from 16.  We are will to take contributions upon review.  If you're 
going to formally contribute the code, you should check out 
https://openjdk.java.net/contribute/


TOny

Re: RFR: 8273297: AES/GCM non-AVX512+VAES CPUs suffer after 8267125 [v5]

[Anthony Scarpino]

On Wed, 22 Sep 2021 22:48:32 GMT, Smita Kamath  wrote:

>> Performance dropped up to 10% for 1k data after 8267125 for CPUs that do not 
>> support the new intrinsic. Tests run were crypto.full.AESGCMBench and 
>> crypto.full.AESGCMByteBuffer from the jmh micro benchmarks.
>> 
>> The problem is each instance of GHASH allocates 96 extra longs for the 
>> AVX512+VAES intrinsic regardless if the intrinsic is used. This extra table 
>> space should be allocated differently so that non-supporting CPUs do not 
>> suffer this penalty. This issue also affects non-Intel CPUs too.
>
> Smita Kamath has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Added htbl_entries constant to other architectures

I think it's ready to integrate

-

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

Re: RFR: 8273297: AES/GCM non-AVX512+VAES CPUs suffer after 8267125 [v2]

[Anthony Scarpino]

I'll run them.. Did you seem my comments?  They are just code-style comments

thanks

Tony

On 9/21/21 12:43 PM, Smita Kamath wrote:

On Mon, 20 Sep 2021 16:44:58 GMT, Anthony Scarpino  
wrote:


Smita Kamath has updated the pull request incrementally with one additional 
commit since the last revision:

   Added a wrapper around aes-gcm intrinsic, changed data size in TestAESMain 
and added a new constant for htbl entries


I approve the jdk changes.   You'll need a hotspot reviewer to approve the 
other changes


@ascarpino Could you please run tier 1-3 tests? We have two reviewers for the 
patch.

-

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

Re: RFR: 8273297: AES/GCM non-AVX512+VAES CPUs suffer after 8267125 [v2]

[Anthony Scarpino]

On Mon, 20 Sep 2021 05:16:16 GMT, Smita Kamath  wrote:

>> Performance dropped up to 10% for 1k data after 8267125 for CPUs that do not 
>> support the new intrinsic. Tests run were crypto.full.AESGCMBench and 
>> crypto.full.AESGCMByteBuffer from the jmh micro benchmarks.
>> 
>> The problem is each instance of GHASH allocates 96 extra longs for the 
>> AVX512+VAES intrinsic regardless if the intrinsic is used. This extra table 
>> space should be allocated differently so that non-supporting CPUs do not 
>> suffer this penalty. This issue also affects non-Intel CPUs too.
>
> Smita Kamath has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Added a wrapper around aes-gcm intrinsic, changed data size in TestAESMain 
> and added a new constant for htbl entries

I approve the jdk changes.   You'll need a hotspot reviewer to approve the 
other changes

-

Marked as reviewed by ascarpino (Reviewer).

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

Re: RFR: 8272385: Enforce ECPrivateKey d value to be in the range [1, n-1] for SunEC provider

[Anthony Scarpino]

On Wed, 1 Sep 2021 04:17:23 GMT, Jamil Nimeh  wrote:

> This fix adds an EC private key range check for the scalar value to be within 
> the range [1, n-1] (n being the order of the generator) for the SunEC ECDSA 
> Signature algorithms and ECDH KeyAgreement algorithms.  While the SunEC 
> KeyGenerator for EC keys will not generate private keys that sit outside the 
> accepted range, it is possible to create and attempt to use ECPrivateKey 
> objects that violate this range through a KeyFactory.
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8272385

Looks good to me

-

Marked as reviewed by ascarpino (Reviewer).

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

Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v12]

[Anthony Scarpino]

On Fri, 20 Aug 2021 22:43:55 GMT, Smita Kamath  wrote:

>> I would like to submit AES-GCM optimization for x86_64 architectures 
>> supporting AVX3+VAES (Evex encoded AES). This optimization interleaves AES 
>> and GHASH operations.
>> Performance gain of ~1.5x - 2x for message sizes 8k and above.
>
> Smita Kamath has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   changes to make sure that ghash_long_swap_mask and counter_mask_addr calls 
> are not duplicated

Tier 1-3 passed on linux-x64, windows-x64, macosx-x64, linux-aarch64

-

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

Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v7]

[Anthony Scarpino]

On Mon, 9 Aug 2021 18:08:53 GMT, Valerie Peng  wrote:

>> I do not understand this comment
>
> Doesn't implGCMCrypt(...) return an int telling how much bytes it has 
> processed? Then we adjust the index and remain input length with this value. 
> But here we didn't save the return value which looks wrong. Did I miss 
> something?
> 
> Never mind my second comment, I mis-read the code.

Ah.. I see.. yes, it should be included in len

-

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

Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v7]

[Anthony Scarpino]

On Fri, 6 Aug 2021 19:57:11 GMT, Valerie Peng  wrote:

>> Smita Kamath has updated the pull request incrementally with five additional 
>> commits since the last revision:
>> 
>>  - Merge branch 'aes-gcm' of github.com:smita-kamath/jdk into aes-gcm
>>  - Updates, comment and variable cleanup
>>  - merge rest
>>  - merge
>>  - fixes and code comments
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 1120:
> 
>> 1118: inOfs += r;
>> 1119: inLen -= r;
>> 1120: }
> 
> Have you considered move the "if (inLen >= PARALLEL_LEN) block" into 
> EncryptOp.update() impl (just like the Encrypt.doFinal() impl) ? Even though 
> not all op.update() calls process large data, but it'd reduce code 
> duplication and ensures that all large data processed by EncryptOp.update() 
> calls would call the intrinsified method.

There are cases where inLen is known to be smaller than PARALLEL_LEN and is a 
waste of a check, such as merging with the ibuffer to create one block.  Also 
moving it into EncryptOp would always mean an additional check and maybe an 
unnecessary jump to another method.

I did that for doFinal, because gctr/ghash.doFinal() needs to was no extra 
checks.

-

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

Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v7]

[Anthony Scarpino]

On Fri, 6 Aug 2021 20:01:00 GMT, Valerie Peng  wrote:

>> Smita Kamath has updated the pull request incrementally with five additional 
>> commits since the last revision:
>> 
>>  - Merge branch 'aes-gcm' of github.com:smita-kamath/jdk into aes-gcm
>>  - Updates, comment and variable cleanup
>>  - merge rest
>>  - merge
>>  - fixes and code comments
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 1721:
> 
>> 1719: 
>> 1720: if (inLen >= PARALLEL_LEN) {
>> 1721: len = implGCMCrypt(in, inOfs, inLen, out, outOfs, out, 
>> outOfs, gctr, ghash);
> 
> nit: exceeds 80 chars

The editor keeps reverting these for some reason.. it's very annoying

-

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

Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v7]

[Anthony Scarpino]

On Fri, 6 Aug 2021 18:40:44 GMT, Valerie Peng  wrote:

>> Smita Kamath has updated the pull request incrementally with five additional 
>> commits since the last revision:
>> 
>>  - Merge branch 'aes-gcm' of github.com:smita-kamath/jdk into aes-gcm
>>  - Updates, comment and variable cleanup
>>  - merge rest
>>  - merge
>>  - fixes and code comments
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 1473:
> 
>> 1471: if (mismatch != 0) {
>> 1472: // Clear output data
>> 1473: Arrays.fill(out, outOfs, outOfs + len, (byte) 0);
> 
> Update the javadoc of this method (line 1428-1429) with this change, i.e. 
> clears output data if tag verification fails. Same goes for line 1482-1483.

ok

-

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

Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v7]

[Anthony Scarpino]

On Fri, 6 Aug 2021 19:44:05 GMT, Valerie Peng  wrote:

>> Smita Kamath has updated the pull request incrementally with five additional 
>> commits since the last revision:
>> 
>>  - Merge branch 'aes-gcm' of github.com:smita-kamath/jdk into aes-gcm
>>  - Updates, comment and variable cleanup
>>  - merge rest
>>  - merge
>>  - fixes and code comments
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 87:
> 
>> 85: private static final int MAX_BUF_SIZE = Integer.MAX_VALUE;
>> 86: // data size when buffer is divided up to aid in intrinsics
>> 87: private static final int TRIGGERLEN = 65536;  // 64k
> 
> With this interleaved impl, is this TRIGGERLEN still needed? The 
> implGCMCrypt(byte[] in, int inOfs, int inLen,
> byte[] ct, int ctOfs, byte[] out, int outOfs, GCTR gctr, GHASH ghash) 
> method is intrinsified, would there be a difference in increasing the number 
> of gctr/ghash calls inside an already intrinsified method?

Yes, they are two different intrinsics.  The new implGCMCrypt intrinsic is 
supported by newer processors so there is no guarantee that implGCMCrypt will 
run the intrinsic.

-

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

Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v7]

[Anthony Scarpino]

On Fri, 6 Aug 2021 19:32:25 GMT, Valerie Peng  wrote:

>> Smita Kamath has updated the pull request incrementally with five additional 
>> commits since the last revision:
>> 
>>  - Merge branch 'aes-gcm' of github.com:smita-kamath/jdk into aes-gcm
>>  - Updates, comment and variable cleanup
>>  - merge rest
>>  - merge
>>  - fixes and code comments
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 1648:
> 
>> 1646: 
>> 1647: ghash.doFinal(in, inOfs, inLen);
>> 1648: return len + gctr.doFinal(in, inOfs, inLen, out, outOfs);
> 
> Why not just keep "return len + op.doFinal(in, inOfs, inLen, out, outOfs);"?

It probably got changed because I was cleaning up unused 'op' elsewhere

-

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

Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v7]

[Anthony Scarpino]

On Fri, 6 Aug 2021 19:16:39 GMT, Valerie Peng  wrote:

>> Smita Kamath has updated the pull request incrementally with five additional 
>> commits since the last revision:
>> 
>>  - Merge branch 'aes-gcm' of github.com:smita-kamath/jdk into aes-gcm
>>  - Updates, comment and variable cleanup
>>  - merge rest
>>  - merge
>>  - fixes and code comments
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 1779:
> 
>> 1777: int len = 0;
>> 1778: if (inLen >= PARALLEL_LEN) {
>> 1779: implGCMCrypt(in, inOfs, inLen, in, inOfs, out, outOfs, 
>> gctr,
> 
> Should save the return value into 'len'? For consistency sake, choose between 
> GaloisCounterMode.implGCMCrypt(...) and implGCMCrypt and not both?

I do not understand this comment

-

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

Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v7]

[Anthony Scarpino]

On Wed, 4 Aug 2021 02:24:05 GMT, Smita Kamath  wrote:

>> I would like to submit AES-GCM optimization for x86_64 architectures 
>> supporting AVX3+VAES (Evex encoded AES). This optimization interleaves AES 
>> and GHASH operations.
>> Performance gain of ~1.5x - 2x for message sizes 8k and above.
>
> Smita Kamath has updated the pull request incrementally with five additional 
> commits since the last revision:
> 
>  - Merge branch 'aes-gcm' of github.com:smita-kamath/jdk into aes-gcm
>  - Updates, comment and variable cleanup
>  - merge rest
>  - merge
>  - fixes and code comments

The latest changes I just pushed into the repo should address the remaining 
Java issues.

-

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

Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]

[Anthony Scarpino]

On Thu, 22 Jul 2021 22:41:03 GMT, Valerie Peng  wrote:

>> This is able in-place, not about two separate buffers.. zeroing happens 
>> somewhere else for all decryption bad buffers
>
> Yes, I know. Basically, we are trying to optimize performance by trying to 
> write into the supplied buffers (out) as much as we can. But then when tag 
> verification failed, the "written" bytes are erased w/ 0. Ideal case would be 
> not to touch the output buffer until after the tag verification succeeds. 
> Isn't this the previous approach? Verify the tag first and then write out the 
> plain text afterwards.

With this new intrinsic doing both ghash and gctr at the same time, I cannot do 
the that ghash check first before the gctr op.  I wish I could

-

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

Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]

[Anthony Scarpino]

On Fri, 16 Jul 2021 00:09:37 GMT, Valerie Peng  wrote:

>> Smita Kamath has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Updated AES-GCM intrinsic to match latest Java Code
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 611:
> 
>> 609: outOfs + len);
>> 610: ghash.update(ct, ctOfs, segments);
>> 611: ctOfs = len;
> 
> This does not look right when the initial value of ctOfs != 0.

Yeah that doesn't look right

-

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

Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]

[Anthony Scarpino]

On Mon, 19 Jul 2021 23:41:49 GMT, Valerie Peng  wrote:

>> If decryption fails with a bad auth tag, the in is not overwritten because 
>> it's in-place.  Encryption is not needed because there is nothing to check.  
>> I can add a comment.
>
> Hmm ok, so if it's not decryption in-place, then output buffer would still be 
> zero'ed when the auth tag failed, but this is ok?

This is able in-place, not about two separate buffers.. zeroing happens 
somewhere else for all decryption bad buffers

-

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

Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]

[Anthony Scarpino]

On Tue, 20 Jul 2021 01:35:04 GMT, Valerie Peng  wrote:

>> Smita Kamath has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Updated AES-GCM intrinsic to match latest Java Code
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 762:
> 
>> 760: 
>> 761: dst.put(out, 0, rlen);
>> 762: processed += srcLen;
> 
> It seems that callers of this implGCMCrypt() method such as 
> GCMEngine.doLastBlock() adds the returned value to the "processed" field 
> which looks like double counting? However, some caller such as 
> GCMEncrypt.doUpdate() does not. Seems inconsistent and may lead to wrong 
> value for the "processed" field?

All the callers that use GCMOperations, ie op.update(...), have the processed 
value updated.  implGCMCrypt() calls op.update() and updates the value.  It 
cannot double count 'processed' is not updated after implGCMCrypt().  I can see 
your point, but the other methods do not have access to 'processed' and would 
mean I copy that line 3 times elsewhere.  I'd rather keep it as is

-

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

Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]

[Anthony Scarpino]

On Mon, 19 Jul 2021 19:35:16 GMT, Valerie Peng  wrote:

>> Smita Kamath has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Updated AES-GCM intrinsic to match latest Java Code
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 761:
> 
>> 759: }
>> 760: 
>> 761: dst.put(out, 0, rlen);
> 
> This looks belong to the above if-block? I wonder how this have not affected 
> the operation to fail. Perhaps the existing regression tests did not cover 
> the 'rlen < blockSize' case. If the code in the above if-block is not run, 
> this outsize dst.put(...) call would put extra output bytes into the output 
> buffer.

Yes... this one and the ct offset problem earlier I would have expected the 
regression test it pick the mistake.  There should be tests that catch this.. 
I'm not sure what's up.

-

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

Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]

[Anthony Scarpino]

On Mon, 19 Jul 2021 19:22:53 GMT, Valerie Peng  wrote:

>> Smita Kamath has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Updated AES-GCM intrinsic to match latest Java Code
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 729:
> 
>> 727: 
>> 728: if (src.hasArray() && dst.hasArray()) {
>> 729: int l = rlen;
> 
> Remove this "l" variable since it's not used?

The code needs some reorganizing

-

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

Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]

[Anthony Scarpino]

On Fri, 16 Jul 2021 00:31:43 GMT, Valerie Peng  wrote:

>> Smita Kamath has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Updated AES-GCM intrinsic to match latest Java Code
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 650:
> 
>> 648: int originalOutOfs = 0;
>> 649: byte[] in;
>> 650: byte[] out;
> 
> The name "in", "out" are almost used in all calls, it's hard to tell when 
> these two are actually used. Can we rename them to make them more unique?

ok

-

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

Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]

[Anthony Scarpino]

On Tue, 20 Jul 2021 22:36:28 GMT, Valerie Peng  wrote:

>> Initializing op in abstract GCMEngine would mean another 'if(encryption)', 
>> when that would not be needed in the  GCMEncrypt() or GCMDecrypt().  I don't 
>> see why that is clearer. 
>> 
>> GaloisCounterMode.implGCMCrypt(...) is the intrinsic, so I have to use what 
>> is used by hotspot.
>
> Seems strange to have GCMOperation op defined in GCMEngine but not 
> initialized, nor used. The methods in GCMEngine which use op has an argument 
> named op anyway. Either you just use the "op" field (remove the "op" 
> argument) or the "op" argument (move the op field to GCMEncrypt/GCMDecrypt 
> class). Having both looks confusing.

Ok.. Moving it into GCMEncrypt makes sense.  Now that I look at the code 
GCMDecrypt only uses it when passed to a method.  GCMEncrypt uses it

-

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

Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]

[Anthony Scarpino]

On Fri, 16 Jul 2021 19:41:53 GMT, Valerie Peng  wrote:

>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
>> line 629:
>> 
>>> 627: GCTR gctr;
>>> 628: GHASH ghash;
>>> 629: GCMOperation op;
>> 
>> It seems clearer to initialize "op" in GCMEngine ctor since it's declared 
>> here. There is already logic in its method checking whether we are doing 
>> encryption or decryption.
>
> Now that you have GCMOperation op, but there is still if-else blocks checking 
> whether it's encryption/decryption and uses gctr and ghash instead of op. 
> Looks like a bit adhoc? Can GaloisCounterMode.implGCMCrypt(...) just take 
> GCMOperation op instead, no need for ct, ctOfs, gctr and ghash?

Initializing op in abstract GCMEngine would mean another 'if(encryption)', when 
that would not be needed in the  GCMEncrypt() or GCMDecrypt().  I don't see why 
that is clearer. 

GaloisCounterMode.implGCMCrypt(...) is the intrinsic, so I have to use what is 
used by hotspot.

-

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

Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]

[Anthony Scarpino]

On Fri, 16 Jul 2021 20:49:20 GMT, Valerie Peng  wrote:

>> Smita Kamath has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Updated AES-GCM intrinsic to match latest Java Code
>
> src/java.base/share/classes/com/sun/crypto/provider/GHASH.java line 146:
> 
>> 144: }
>> 145: state = new long[2];
>> 146: subkeyHtbl = new long[2*57]; // 48 keys for the interleaved 
>> implementation, 8 for avx-ghash implementation and one for the original key
> 
> nit: the comment is too long, i.e. > 80

Ah.. I forgot I didn't change GHASH with my changes.. but I'll fix that thanks

> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 743:
> 
>> 741: dst.array(), dst.arrayOffset() + 
>> dst.position(),
>> 742: gctr, ghash);
>> 743: }
> 
> Can we use another ByteBuffer variable to avoid almost-duplicate calls?
> 
> ByteBuffer ct = (encryption? dst : src);
> rlen -= GaloisCounterMode.implGCMCrypt(src.array(),
> src.arrayOffset() + src.position(), 
> src.remaining(),
> ct.array(), ct.arrayOffset() + ct.position(),
> dst.array(), dst.arrayOffset() + dst.position(),
> gctr, ghash);

That maybe a better choice

-

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

Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]

[Anthony Scarpino]

On Fri, 16 Jul 2021 00:10:52 GMT, Anthony Scarpino  
wrote:

>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
>> line 992:
>> 
>>> 990:  */
>>> 991: byte[] overlapDetection(byte[] in, int inOfs, byte[] out, int 
>>> outOfs) {
>>> 992: if (in == out && (!encryption || inOfs < outOfs)) {
>> 
>> So, we will always allocate an output buffer for decryption if in==out? Why 
>> just decryption? Update the javadoc for this method with the reason?
>
> If the crypto is decryption in-place, an internal output buffer is needed in 
> case the auth tag fails, otherwise the input buffer would be zero'ed.

If decryption fails with a bad auth tag, the in is not overwritten because it's 
in-place.  Encryption is not needed because there is nothing to check.  I can 
add a comment.

-

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

Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v4]

[Anthony Scarpino]

On Thu, 15 Jul 2021 22:44:05 GMT, Valerie Peng  wrote:

>> Smita Kamath has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Updated AES-GCM intrinsic to match latest Java Code
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 170:
> 
>> 168: 
>> 169: // always encrypt mode for embedded cipher
>> 170: blockCipher.init(false, key.getAlgorithm(), keyValue);
> 
> Is this change intentional? Looks like we are reverting to older version of 
> source and undo newer changes.

Nope.. unintentional

> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 472:
> 
>> 470: engine = null;
>> 471: if (encodedKey != null) {
>> 472: Arrays.fill(encodedKey, (byte)0);
> 
> Looks like another unintentional newer->older change.

I don't remember an old comment about that, dunno if that was reverted

> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 992:
> 
>> 990:  */
>> 991: byte[] overlapDetection(byte[] in, int inOfs, byte[] out, int 
>> outOfs) {
>> 992: if (in == out && (!encryption || inOfs < outOfs)) {
> 
> So, we will always allocate an output buffer for decryption if in==out? Why 
> just decryption? Update the javadoc for this method with the reason?

If the crypto is decryption in-place, an internal output buffer is needed in 
case the auth tag fails, otherwise the input buffer would be zero'ed.

-

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

Re: RFR: 8270317: Large Allocation in CipherSuite

[Anthony Scarpino]

On Wed, 14 Jul 2021 20:21:58 GMT, djelinski 
 wrote:

>> ### Benchmark results 
>> 
>> I have benchmarked 3 cases.
>> 
>> 1. The current situation. 
>> 
>> Benchmark
>> (cipherSuite)  Mode  CntScore   Error  Units
>> CipherSuiteBench.benchmarkCipherSuite   
>> TLS_AES_256_GCM_SHA384  avgt   25  124.783 ? 2.050  ns/op
>> CipherSuiteBench.benchmarkCipherSuite  
>> TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384  avgt   25  125.403 ? 0.554  ns/op
>> CipherSuiteBench.benchmarkCipherSuite  
>> TLS_DHE_DSS_WITH_AES_128_CBC_SHA256  avgt   25  127.117 ? 0.789  ns/op
>> CipherSuiteBench.benchmarkCipherSuite 
>> TLS_DHE_RSA_WITH_AES_256_CBC_SHA  avgt   25  127.869 ? 1.112  ns/op
>> 
>> 
>> 2. Use `static final array` instead of calling `CipherSuite.values` each 
>> time. 
>> 
>> Benchmark
>> (cipherSuite)  Mode  Cnt   Score   Error  Units
>> CipherSuiteBench.benchmarkCipherSuite   
>> TLS_AES_256_GCM_SHA384  avgt   25  10.146 ? 0.252  ns/op
>> CipherSuiteBench.benchmarkCipherSuite  
>> TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384  avgt   25  30.501 ? 0.207  ns/op
>> CipherSuiteBench.benchmarkCipherSuite  
>> TLS_DHE_DSS_WITH_AES_128_CBC_SHA256  avgt   25  47.375 ? 0.150  ns/op
>> CipherSuiteBench.benchmarkCipherSuite 
>> TLS_DHE_RSA_WITH_AES_256_CBC_SHA  avgt   25  55.887 ? 3.786  ns/op
>> 
>> 
>> 3. Using Hashmap for lookup instead of iterating through the array each 
>> time. (Method in this PR)
>> 
>> Benchmark
>> (cipherSuite)  Mode  Cnt   Score   Error  Units
>> CipherSuiteBench.benchmarkCipherSuite   
>> TLS_AES_256_GCM_SHA384  avgt   25  13.533 ? 0.148  ns/op
>> CipherSuiteBench.benchmarkCipherSuite  
>> TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384  avgt   25  11.269 ? 0.147  ns/op
>> CipherSuiteBench.benchmarkCipherSuite  
>> TLS_DHE_DSS_WITH_AES_128_CBC_SHA256  avgt   25  11.507 ? 0.107  ns/op
>> CipherSuiteBench.benchmarkCipherSuite 
>> TLS_DHE_RSA_WITH_AES_256_CBC_SHA  avgt   25  10.932 ? 0.146  ns/op
>> 
>> 
>> I have picked 4 cipher suite from the start of the list and are roughly 10 
>> positions apart. I have opted to go with HashMap for name and id lookup as 
>> they provide a more consistent times and benchmarks are similar for the 
>> first few cipher suits in the enum as well.
>
> src/java.base/share/classes/sun/security/ssl/CipherSuite.java line 916:
> 
>> 914: static String nameOf(int id) {
>> 915: if (maps_id.containsKey(id)) {
>> 916: return maps_id.get(id).name;
> 
> Would it make sense to skip `containsKey` and null-check the value returned 
> by `get` instead?

That's what containsKey() does, returning false of the value is null.

-

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

Integrated: 8269827: JMH tests for AES/GCM byte[] and bytebuffers

[Anthony Scarpino]

On Fri, 2 Jul 2021 19:13:48 GMT, Anthony Scarpino  wrote:

> Hi,
> 
> I need a review of these new jmh tests that run AES/GCM encryption and 
> decryption using byte[], heap bytebuffers, and direct bytebuffers as input 
> and output buffers for single and multi-part testing. 
> 
> thanks
> 
> Tony

This pull request has now been integrated.

Changeset: 58328824
Author:Anthony Scarpino 
URL:   
https://git.openjdk.java.net/jdk/commit/58328824927292927a2c6329400cde816c383ecd
Stats: 273 lines in 4 files changed: 237 ins; 17 del; 19 mod

8269827: JMH tests for AES/GCM byte[] and bytebuffers

Reviewed-by: ecaspole, weijun

-

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

Re: RFR: 8269827: JMH tests for AES/GCM byte[] and bytebuffers [v3]

[Anthony Scarpino]

On Thu, 8 Jul 2021 14:39:20 GMT, Weijun Wang  wrote:

>> Anthony Scarpino has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Add small.AESGCMByteBuffer and fix dataSize
>
> test/micro/org/openjdk/bench/javax/crypto/full/AESGCMByteBuffer.java line 50:
> 
>> 48: private int keyLength;
>> 49: 
>> 50: @Param({"" + 1024, "" + 1500, "" + 4096, "" + 16384})
> 
> You can update the line above as well.

Grr.. I'm not sure how I missed that one

-

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

Re: RFR: 8269827: JMH tests for AES/GCM byte[] and bytebuffers [v4]

[Anthony Scarpino]

> Hi,
> 
> I need a review of these new jmh tests that run AES/GCM encryption and 
> decryption using byte[], heap bytebuffers, and direct bytebuffers as input 
> and output buffers for single and multi-part testing. 
> 
> thanks
> 
> Tony

Anthony Scarpino has updated the pull request incrementally with one additional 
commit since the last revision:

  full.AESGCMByteBuffer dataSize

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4672/files
  - new: https://git.openjdk.java.net/jdk/pull/4672/files/2a7d1df6..4d43cff6

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

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

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

Re: RFR: 8269827: JMH tests for AES/GCM byte[] and bytebuffers [v3]

[Anthony Scarpino]

> Hi,
> 
> I need a review of these new jmh tests that run AES/GCM encryption and 
> decryption using byte[], heap bytebuffers, and direct bytebuffers as input 
> and output buffers for single and multi-part testing. 
> 
> thanks
> 
> Tony

Anthony Scarpino has updated the pull request incrementally with one additional 
commit since the last revision:

  Add small.AESGCMByteBuffer and fix dataSize

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4672/files
  - new: https://git.openjdk.java.net/jdk/pull/4672/files/0c7113ef..2a7d1df6

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

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

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

Re: RFR: 8269827: JMH tests for AES/GCM byte[] and bytebuffers [v2]

[Anthony Scarpino]

On Tue, 6 Jul 2021 15:40:09 GMT, Weijun Wang  wrote:

>> Anthony Scarpino has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   merge AESGCMByteArray with existing AESGCMBench
>
> test/micro/org/openjdk/bench/javax/crypto/full/AESGCMByteBuffer.java line 44:
> 
>> 42:  * benchmark operation
>> 43:  */
>> 44: 
> 
> Do you want to create a "small" version?

I debated it.. I'm still not sure, but I guess I could.

-

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

Re: RFR: 8269827: JMH tests for AES/GCM byte[] and bytebuffers [v2]

[Anthony Scarpino]

On Tue, 6 Jul 2021 15:38:20 GMT, Weijun Wang  wrote:

>> Anthony Scarpino has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   merge AESGCMByteArray with existing AESGCMBench
>
> test/micro/org/openjdk/bench/javax/crypto/full/AESGCMBench.java line 46:
> 
>> 44: private int keyLength;
>> 45: 
>> 46: @Param({"" + 1024, "" +  1500, "" + 4096, "" + 16384})
> 
> Now that you don't calculate anymore, how about just string literals like 
> `"1024"` etc?

Funny.. I didn't even notice that

-

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

Re: RFR: 8269827: JMH tests for AES/GCM byte[] and bytebuffers [v2]

[Anthony Scarpino]

On Tue, 6 Jul 2021 15:41:27 GMT, Weijun Wang  wrote:

>> Anthony Scarpino has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   merge AESGCMByteArray with existing AESGCMBench
>
> test/micro/org/openjdk/bench/javax/crypto/full/AESGCMBench.java line 58:
> 
>> 56: SecretKeySpec ks;
>> 57: GCMParameterSpec gcm_spec;
>> 58: byte[] aad;
> 
> Not an expert on this, but why no more AAD?

For a performance test there isn't any value in adding an AAD.  It's data 
sitting in a buffer that an be easily replicated with a multipart operation

-

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

Re: RFR: 8269827: JMH tests for AES/GCM byte[] and bytebuffers [v2]

[Anthony Scarpino]

> Hi,
> 
> I need a review of these new jmh tests that run AES/GCM encryption and 
> decryption using byte[], heap bytebuffers, and direct bytebuffers as input 
> and output buffers for single and multi-part testing. 
> 
> thanks
> 
> Tony

Anthony Scarpino has updated the pull request incrementally with one additional 
commit since the last revision:

  merge AESGCMByteArray with existing AESGCMBench

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4672/files
  - new: https://git.openjdk.java.net/jdk/pull/4672/files/ad463039..0c7113ef

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

  Stats: 205 lines in 4 files changed: 38 ins; 147 del; 20 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4672.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4672/head:pull/4672

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

RFR: 8269827: JMH tests for AES/GCM byte[] and bytebuffers

[Anthony Scarpino]

Hi,

I need a review of these new jmh tests that run AES/GCM encryption and 
decryption using byte[], heap bytebuffers, and direct bytebuffers as input and 
output buffers for single and multi-part testing. 

thanks

Tony

-

Commit messages:
 - cleanup
 - initial checkin

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

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

Re: RFR: 8268427: Improve AlgorithmConstraints:checkAlgorithm performance [v3]

[Anthony Scarpino]

On Fri, 18 Jun 2021 21:27:41 GMT, Anthony Scarpino  
wrote:

>> Dongbo He has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Replace HashSet with TreeSet
>
> test/micro/org/openjdk/bench/java/security/AlgorithmConstraintsPermits.java 
> line 2:
> 
>> 1: /*
>> 2:  * Copyright (c) 2021, Huawei Technologies Co., Ltd. All rights reserved.
> 
> I'm verifying if this copyright is ok or if it needs to be changed. Please 
> don't integrate until I hear back from others.

This appears to be ok with the powers a be

-

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

Re: RFR: 8268427: Improve AlgorithmConstraints:checkAlgorithm performance [v3]

[Anthony Scarpino]

On Thu, 17 Jun 2021 08:16:42 GMT, Dongbo He  wrote:

>> Now AlgorithmConstraints:checkAlgorithm uses List to check if an algorithm 
>> has been disabled. It is less efficient when there are more disabled 
>> elements in the list, we can use Set instead of List to speed up the search.
>> 
>> Patch contains a benchmark that can be run with `make test 
>> TEST="micro:java.security.AlgorithmConstraintsPermits"`.
>> Baseline results before patch:
>> 
>> Benchmark(algorithm)  Mode  CntScore 
>> Error  Units
>> AlgorithmConstraintsPermits.permitsSSLv3  avgt5   21.687 ?   
>> 1.118  ns/op
>> AlgorithmConstraintsPermits.permits  DES  avgt5  324.216 ?   
>> 6.233  ns/op
>> AlgorithmConstraintsPermits.permits NULL  avgt5  709.462 ?  
>> 51.259  ns/op
>> AlgorithmConstraintsPermits.permits   TLS1.3  avgt5  687.497 ? 
>> 170.181  ns/op
>> 
>> Benchmark results after patch:
>> 
>> Benchmark(algorithm)  Mode  CntScore
>> Error  Units
>> AlgorithmConstraintsPermits.permitsSSLv3  avgt5   46.407 ?  
>> 1.057  ns/op
>> AlgorithmConstraintsPermits.permits  DES  avgt5   65.722 ?  
>> 0.578  ns/op
>> AlgorithmConstraintsPermits.permits NULL  avgt5   43.988 ?  
>> 1.264  ns/op
>> AlgorithmConstraintsPermits.permits   TLS1.3  avgt5  399.546 ? 
>> 11.194  ns/op
>> 
>> SSLv3, DES, NULL are the first, middle and last element in 
>> `jdk.tls.disabledAlgorithms` from `conf/security/java.security`.
>> 
>> Tomcat(maxKeepAliveRequests=1, which will disable HTTP/1.0 
>> keep-alive)+Jmeter:
>> Before patch:
>> 
>> summary +  50349 in 00:00:30 = 1678.4/s Avg:   238 Min:   188 Max:   298 
>> Err: 0 (0.00%) Active: 400 Started: 400 Finished: 0
>> summary = 135183 in 00:01:22 = 1654.5/s Avg:   226 Min:16 Max:  1281 
>> Err: 0 (0.00%)
>> summary +  50240 in 00:00:30 = 1674.1/s Avg:   238 Min:   200 Max:   308 
>> Err: 0 (0.00%) Active: 400 Started: 400 Finished: 0
>> summary = 185423 in 00:01:52 = 1659.7/s Avg:   229 Min:16 Max:  1281 
>> Err: 0 (0.00%)
>> summary +  50351 in 00:00:30 = 1678.4/s Avg:   238 Min:   191 Max:   306 
>> Err: 0 (0.00%) Active: 400 Started: 400 Finished: 0
>> summary = 235774 in 00:02:22 = 1663.7/s Avg:   231 Min:16 Max:  1281 
>> Err: 0 (0.00%)
>> summary +  50461 in 00:00:30 = 1681.9/s Avg:   237 Min:   174 Max:   303 
>> Err: 0 (0.00%) Active: 400 Started: 400 Finished: 0
>> 
>> After patch:
>> 
>> summary +  59003 in 00:00:30 = 1966.6/s Avg:   203 Min:   158 Max:   272 
>> Err: 0 (0.00%) Active: 400 Started: 400 Finished: 0
>> summary = 146675 in 00:01:18 = 1884.6/s Avg:   198 Min:26 Max:   697 
>> Err: 0 (0.00%)
>> summary +  58965 in 00:00:30 = 1965.9/s Avg:   203 Min:   166 Max:   257 
>> Err: 0 (0.00%) Active: 400 Started: 400 Finished: 0
>> summary = 205640 in 00:01:48 = 1907.2/s Avg:   199 Min:26 Max:   697 
>> Err: 0 (0.00%)
>> summary +  59104 in 00:00:30 = 1969.1/s Avg:   203 Min:   157 Max:   266 
>> Err: 0 (0.00%) Active: 400 Started: 400 Finished: 0
>> summary = 264744 in 00:02:18 = 1920.7/s Avg:   200 Min:26 Max:   697 
>> Err: 0 (0.00%)
>> summary +  59323 in 00:00:30 = 1977.6/s Avg:   202 Min:   158 Max:   256 
>> Err: 0 (0.00%) Active: 400 Started: 400 Finished: 0
>> 
>> 
>> Testing: tier1, tier2
>
> Dongbo He has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Replace HashSet with TreeSet

Marked as reviewed by ascarpino (Reviewer).

test/micro/org/openjdk/bench/java/security/AlgorithmConstraintsPermits.java 
line 2:

> 1: /*
> 2:  * Copyright (c) 2021, Huawei Technologies Co., Ltd. All rights reserved.

I'm verifying if this copyright is ok or if it needs to be changed. Please 
don't integrate until I hear back from others.

-

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