Enhancing GCM ByteBuffer Implementation

[samasri]

Fixing OpenJ9's Issue 11390. Specifically, some of the AEAD tests were failing in the jtreg test harness. This PR fixes them by:

• Adding necessary functions to NativeGaloisCounterMode to support processing ByteBuffer input and output.
• Editing the jtreg test GCMBufferTest.java to not check for the output length after cipher.update. This is because cipher.update doesn't have to return the encrypted input length. In our JDK, the cipher.update behaves differently depending on whether the machine used does have native crypto or not:
  • If native crypto is used, the cipher.update processes the input by adding it to a buffer, which will be processed when cipher.doFinal is called. Hence, the cipher.update returns zero.
  • However, when native crypto is not used, cipher.update returns 16.

[pshipton]

Pls update the IBM copyrights in the files changed.

[keithc-ca]

Please also remove all trailing whitespace in any changes.

[keithc-ca]

I think you should use the same author for all commits (eventually squashed?). So far we have two:

• Samer Almasri almasri@ualberta.ca
• Samer AL Masri samasri@ibm.com

[pshipton]

Checking what testing is resolved and what still fails with this change.

[pshipton]

jenkins test sanity.openjdk zlinux jdknext

[pshipton]

PR build is https://ci.eclipse.org/openj9/job/PullRequest-OpenJDKnext/72/

[pshipton]

[2021-01-19T14:12:47.871Z] /home/jenkins/workspace/Build_JDKnext_s390x_linux_Personal/closed/src/java.base/share/classes/com/sun/crypto/provider/NativeGaloisCounterMode.java:403: error: variable dst_arr is already defined in method encryptFinal(ByteBuffer,ByteBuffer)
[2021-01-19T14:12:47.871Z]             byte[] dst_arr = dst.array();
[2021-01-19T14:12:47.871Z]                    ^
[2021-01-19T14:12:47.872Z] /home/jenkins/workspace/Build_JDKnext_s390x_linux_Personal/closed/src/java.base/share/classes/com/sun/crypto/provider/NativeGaloisCounterMode.java:405: error: variable dst_arr is already defined in method encryptFinal(ByteBuffer,ByteBuffer)
[2021-01-19T14:12:47.872Z]             byte[] dst_arr = new byte[dst.capacity()];
[2021-01-19T14:12:47.872Z]                    ^

[samasri]

Yes, I have a commit for this ready, but I realized that some tests are not passing, so I'm fixing them. Will update with a fix here soon.

[samasri]

The source code should compile and tests should be passing again

[pshipton]

jenkins test sanity.openjdk zlinux jdknext

[pshipton]

https://ci.eclipse.org/openj9/job/PullRequest-OpenJDKnext/73/

[pshipton]

test/jdk/com/sun/crypto/provider/Cipher/AEAD/GCMBufferTest.java needs to have an IBM copyright added.

[keithc-ca]

test/jdk/com/sun/crypto/provider/Cipher/AEAD/GCMBufferTest.java needs to have an IBM copyright added.

I'd prefer that we don't modify the test: that file should be reverted.

[pshipton]

I'd prefer that we don't modify the test: that file should be reverted.

Agreed that solution is preferable.

[pshipton]

com/sun/crypto/provider/Cipher/AEAD/GCMBufferTest.java is failing.

11:14:50  java.lang.Exception: Wrong update return len (BYTE):  rlen=0, expected output len=16
11:14:50  	at GCMBufferTest.crypto(GCMBufferTest.java:423)
11:14:50  	at GCMBufferTest.encrypt(GCMBufferTest.java:303)
11:14:50  	at GCMBufferTest.test(GCMBufferTest.java:281)
11:14:50  	at GCMBufferTest.main(GCMBufferTest.java:631)

Also com/sun/crypto/provider/Cipher/ChaCha20/ChaCha20Poly1305ParamTest.java I think is a new problem, probably not related to this PR.

11:15:04  java.lang.NullPointerException: Cannot read the array length because "this.nonce" is null
11:15:04  	at java.base/com.sun.crypto.provider.NativeChaCha20Cipher.engineGetParameters(NativeChaCha20Cipher.java:217)
11:15:04  	at java.base/javax.crypto.Cipher.getParameters(Cipher.java:1060)
11:15:04  	at ChaCha20Poly1305ParamTest.main(ChaCha20Poly1305ParamTest.java:240)

[pshipton]

I don't think the PR testing is running the correct test material.

[pshipton]

Created a new issue eclipse-openj9/openj9#11689 for the ChaCha20Poly1305ParamTest failure.

[pshipton]

If GCMBufferTest.java continues to be modified, it needs to be validated via grinder.
https://ci.eclipse.org/openj9/view/Test/job/Grinder/1434/ - passed

[pshipton]

I would be good to get this finalized and merged before the code split occurs this weekend.

[pshipton]

Once it is finalized, it needs to be backported to jdk16.

[pshipton]

Note the openjdk tests failing due to this problem have been excluded, and will need to be re-included when this is ready to merge.
adoptium/aqa-tests#2249

[keithc-ca]

Please squash and rebase on the head of the openj9 branch.

[keithc-ca]

Please fix the commit message: the author and committer should be the same.
You may want to use git commit --amend --author='Samer AL Masri <samasri@ibm.com>'.
Please also remove the extra #Signed-off-by: lines.

[samasri]

Sorry about that, done.

[keithc-ca]

Jenkins test sanity,extended plinux jdknext

[keithc-ca]

Jenkins test sanity.openjdk zlinux jdknext

[pshipton]

Tested the previously failing (and excluded) openjdk tests via grinders, using the PR zlinux build https://ci.eclipse.org/openj9/job/Pipeline_Build_Test_JDKnext_s390x_linux/834/. All passed.

test/jdk/com/sun/crypto/provider/Cipher/AEAD/Encrypt.java
https://ci.eclipse.org/openj9/view/Test/job/Grinder/1520 - passed

com/sun/crypto/provider/Cipher/AEAD/GCMBufferTest.java
https://ci.eclipse.org/openj9/view/Test/job/Grinder/1521 - passed

com/sun/crypto/provider/Cipher/AEAD/OverlapByteBuffer.java
https://ci.eclipse.org/openj9/view/Test/job/Grinder/1522 - passed

com/sun/crypto/provider/Cipher/AEAD/SameBuffer.java
https://ci.eclipse.org/openj9/view/Test/job/Grinder/1523 - passed

Create PR to un-exclude the tests
adoptium/aqa-tests#2266

Created PRs to port this change to jdk16, assuming the current commit is the final one. In draft state until this is confirmed.
ibmruntimes/openj9-openjdk-jdk16#25
ibmruntimes/openj9-openjdk-jdk16#26

[keithc-ca]

The sanity.openjdk failure is eclipse-openj9/openj9#11930.

[keithc-ca]

Thanks, @samasri for being persistent with this.