Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8255410: Add ChaCha20 and Poly1305 support to SunPKCS11 provider #805

Closed
wants to merge 5 commits into from

Conversation

GoeLin
Copy link
Member

@GoeLin GoeLin commented Feb 3, 2022

I backport this for parity with 11.0.15-oracle.

I had to do a row of adaptions. There are some implementation
differences, and many Java 17 usages that had to be changed.

In the original change in SunPKCS11.java, dA() is used
to give alias names of algorithms. It calls to
SecurityProviderConstants.getAliases().
In SunPKCS11.java of jdk11 neither dA() nore getAliases()
are available. It uses d() instead of da() and calls a
s() with a list of literal strings to give the alias names.
SecurityProviderConstants.java is in java.base. The jdk11
version does not contain the list of aliases that can be
found in 17.

I looked up whether there are aliases listed for
"ChaCha20-Poly1305" in 17, but found none, so I added
an empty call s() in SunPKCS11.java.

In P11AEADCipher.java I had to modify the syntax of
a switch statement.

In CK_SALSA20_CHACHA20_POLY1305_PARAMS.java
I had to replace HexFormat.of().formatHex(...)
which is a java.util class not in 11.

The tests use HexFormat heavily.
I replaced it by HexToBytes() from TestKATForGCM and
by toHexString() from TestLeadingZeroesP11.java.
To make these methods available I moved them to the
superclass PKCS11Test.java.
This way they can be used for potential later backports, too.
I also had to adapt a switch statement using '->' to syntax
know to 11.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed
  • Change requires a CSR request to be approved

Issues

  • JDK-8255410: Add ChaCha20 and Poly1305 support to SunPKCS11 provider
  • JDK-8265008: Add ChaCha20 and Poly1305 support to SunPKCS11 provider (CSR)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk11u-dev pull/805/head:pull/805
$ git checkout pull/805

Update a local copy of the PR:
$ git checkout pull/805
$ git pull https://git.openjdk.java.net/jdk11u-dev pull/805/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 805

View PR using the GUI difftool:
$ git pr show -t 805

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk11u-dev/pull/805.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 3, 2022

👋 Welcome back goetz! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot changed the title Backport 5d8c1cc8a05e0d9aedd6d54b8147d374c2290024 8255410: Add ChaCha20 and Poly1305 support to SunPKCS11 provider Feb 3, 2022
@openjdk
Copy link

@openjdk openjdk bot commented Feb 3, 2022

This backport pull request has now been updated with issue from the original commit.

@openjdk openjdk bot added backport rfr labels Feb 3, 2022
@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 3, 2022

Webrevs

Copy link
Contributor

@TheRealMDoerr TheRealMDoerr left a comment

Hi Götz, thanks for doing this large backport. Looks basically good, but I have a couple of requests. The small test code refactoring makes sense.

s(),
m(CKM_CHACHA20_POLY1305));


Copy link
Contributor

@TheRealMDoerr TheRealMDoerr Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra newline

Copy link
Member Author

@GoeLin GoeLin Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@@ -626,6 +630,11 @@ private static void register(Descriptor d) {
d(AGP, "GCM", "sun.security.util.GCMParameters",
m(CKM_AES_GCM));

d(AGP, "ChaCha20-Poly1305",
"com.sun.crypto.provider.ChaCha20Poly1305Parameters",
s(),
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aliases should be taken from KnowsOIDs: CHACHA20_POLY1305("1.2.840.113549.1.9.16.3.18", "CHACHA20-POLY1305"),

@@ -709,6 +720,11 @@ private static void register(Descriptor d) {
d(CIP, "Blowfish/CBC/PKCS5Padding", P11Cipher,
m(CKM_BLOWFISH_CBC));

d(CIP, "ChaCha20-Poly1305", P11AEADCipher,
s(),
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like above

Copy link
Member Author

@GoeLin GoeLin Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both fixed.

} else {
sb.append("0x");
for (byte b: nonce) {
sb.append(String.format("0%02X", b));
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

other code uses String.format("%02x", b & 0xff). I believe it gets converted to int with sign extend otherwise. Did you check the output?

Copy link
Member Author

@GoeLin GoeLin Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The formatter should get a java.lang.Byte. A simple test shows it's the same as HexFormat.of().formatHex(). I need to remove the leading 0, though :)

} else {
sb.append("0x");
for (byte b: aad) {
sb.append(String.format("0%02X", b));
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like above

@GoeLin GoeLin force-pushed the goetz_backport_8255410 branch from 9541e0f to e5dea49 Compare Feb 10, 2022
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr left a comment

Please consider my proposal. Otherwise, good. Thanks for the backport work!

@@ -626,6 +630,11 @@ private static void register(Descriptor d) {
d(AGP, "GCM", "sun.security.util.GCMParameters",
m(CKM_AES_GCM));

d(AGP, "ChaCha20-Poly1305",
"com.sun.crypto.provider.ChaCha20Poly1305Parameters",
s("1.2.840.113549.1.9.16.3.18", "CHACHA20-POLY1305"),
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've looked a bit more through the code and it seems like the algorithm name is compared case insensitive (algorithm = algorithm.toUpperCase(Locale.ENGLISH); in P11SecretKeyFactory.java). So, it seems like "CHACHA20-POLY1305" is redundant because it matches the original name (case insensitive). Other places have an extra entry starting with "OID". I'd use: s("1.2.840.113549.1.9.16.3.18", "OID.1.2.840.113549.1.9.16.3.18") which looks consistent with the remaining code. Maybe another reviewer can confirm this.

@@ -709,6 +720,10 @@ private static void register(Descriptor d) {
d(CIP, "Blowfish/CBC/PKCS5Padding", P11Cipher,
m(CKM_BLOWFISH_CBC));

d(CIP, "ChaCha20-Poly1305", P11AEADCipher,
s("1.2.840.113549.1.9.16.3.18", "CHACHA20-POLY1305"),
m(CKM_CHACHA20_POLY1305));
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like above

@openjdk
Copy link

@openjdk openjdk bot commented Feb 10, 2022

@GoeLin This change now passes all automated pre-integration checks.

After integration, the commit message for the final commit will be:

8255410: Add ChaCha20 and Poly1305 support to SunPKCS11 provider

Reviewed-by: mdoerr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 24 new commits pushed to the master branch:

  • 80919eb: 8279669: test/jdk/com/sun/jdi/TestScaffold.java uses wrong condition
  • ba6c4c1: 8261107: ArrayIndexOutOfBoundsException in the ICC_Profile.getInstance(InputStream)
  • 80a2777: 8282372: [11] build issue on MacOS/aarch64 12.2.1 using Xcode 13.1: call to 'log2_intptr' is ambiguous
  • 68c6320: 8214004: Missing space between compiler thread name and task info in hs_err
  • 16cbd32: 8250750: JDK-8247515 fix for OSX pc_to_symbol() lookup fails with some symbols
  • 6f9d287: 8277488: Add expiry exception for Digicert (geotrustglobalca) expiring in May 2022
  • ffa5ae8: 8247515: OSX pc_to_symbol() lookup does not work with core files
  • 4378844: 8254085: javax/swing/text/Caret/TestCaretPositionJTextPane.java failed with "RuntimeException: Wrong caret position"
  • 26e599d: 8247272: SA ELF file support has never worked for 64-bit causing address to symbol name mapping to fail
  • 259bbf5: 8233986: ProblemList javax/swing/plaf/basic/BasicTextUI/8001470/bug8001470.java for windows-x64
  • ... and 14 more: https://git.openjdk.java.net/jdk11u-dev/compare/f4e14a685ef4c8eb7c3f861b9c0bd981e5544c36...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Feb 10, 2022
@GoeLin
Copy link
Member Author

@GoeLin GoeLin commented Feb 11, 2022

Hi, I would appreciate a second review. Does anyone have an opinion on the aliases?
@martinuy If I remember correctly you have made changes to the security files in the past.
Could you have a look? Or @shipilev or @zhengyu123, would you find time? Thanks!

@shipilev
Copy link
Contributor

@shipilev shipilev commented Feb 11, 2022

Hi, I would appreciate a second review. Does anyone have an opinion on the aliases? @martinuy If I remember correctly you have made changes to the security files in the past. Could you have a look? Or @shipilev or @zhengyu123, would you find time? Thanks!

@martinuy is our security expert, please wait for him to reply :)

@TheRealMDoerr
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr commented Feb 11, 2022

For the 2nd reviewer: I had proposed to use s("1.2.840.113549.1.9.16.3.18", "OID.1.2.840.113549.1.9.16.3.18") to be more consistent with jdk11. Please check.

@GoeLin
Copy link
Member Author

@GoeLin GoeLin commented Feb 23, 2022

@martinuy, will you find time to look at this change this week? Next Tuesday rampdown begins, would be nice to have this pushed by then.
Thanks, Goetz.

@GoeLin
Copy link
Member Author

@GoeLin GoeLin commented Mar 1, 2022

Hi @shipilev, @TheRealMDoerr
I would like to push this today before the start of rampdown.
@martinuy did not react yet. What do you think, is this ok to go anyways?
I think we should not miss it in 11.0.15. In case the aliases are not good, we can still make
a follow up change.

@TheRealMDoerr
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr commented Mar 1, 2022

I'd prefer to push it after updating the aliases. Some jdk11 code may use the "OID." prefix and the algorithm would not be found. I agree with that this backport should go into 11.0.15. Ideally before ramp down or alternatively as critical backport.

@theRealAph
Copy link
Contributor

@theRealAph theRealAph commented Mar 1, 2022

I'd prefer to push it after updating the aliases. Some jdk11 code may use the "OID." prefix and the algorithm would not be found. I agree with that this backport should go into 11.0.15. Ideally before ramp down or alternatively as critical backport.

I agree too, but let's wait for a comment from Martin.

@GoeLin
Copy link
Member Author

@GoeLin GoeLin commented Mar 1, 2022

I agree too, but let's wait for a comment from Martin.

You mean Martin Balao, right? He didn't react for 18 days now.
I'd rather have it pushed so that it reaches any downstream testing,
than wait and do a critical push on last notice.

I updated the aliases as recommended by Martin Doerr.

Copy link
Contributor

@TheRealMDoerr TheRealMDoerr left a comment

Thanks for the update. Good to go IMHO.

@theRealAph
Copy link
Contributor

@theRealAph theRealAph commented Mar 1, 2022

I agree too, but let's wait for a comment from Martin.

You mean Martin Balao, right? He didn't react for 18 days now.

I'm sure he never saw it. He's working on it at the moment.

@martinuy
Copy link
Contributor

@martinuy martinuy commented Mar 1, 2022

Hi all,

I'm reviewing the backport now and will provide my comments today.

Martin.-

@GoeLin
Copy link
Member Author

@GoeLin GoeLin commented Mar 1, 2022

I'm sure he never saw it. He's working on it at the moment.

Great, thank you!
Reaching out to somebody is a bit problematic since github: Too many channels (PR, JBS, Mail), and too many mail notifications. I missed things, too.

@martinuy
Copy link
Contributor

@martinuy martinuy commented Mar 1, 2022

Hi @GoeLin,

Thanks for proposing this backports. Looks good to me and I agree with all the previous review comments and fixes.

One minor thing: it looks to me that HexToBytes does not exactly reflect HexFormat::parseHex behavior when it comes to handling null strings. While HexFormat::parseHex requires a non-null value (or throws a NPE) [1], HexToBytes will return an array of length 0. Returning an array of length 0, for HexFormat::parseHex, is reserved for the case in which the string is empty (non-null) only. There might be some performance differences too. Given that this is only used for tests, I believe that no changes are needed to the current PR. Something I was thinking is that HexFormat looks pretty self-contained so I wondered if it is worth moving it to an internal package of java.base (which could be for example jdk.internal.util) and exporting it to modules (such as jdk.crypto.cryptoki) on demand as backports use it. This would allow to use it even beyond SunPKCS11 in the future.

Martin.-

--
[1] - https://github.com/openjdk/jdk/blob/jdk-19+11/src/java.base/share/classes/java/util/HexFormat.java#L527

@GoeLin
Copy link
Member Author

@GoeLin GoeLin commented Mar 1, 2022

Thanks Martin for looking at this so ad-hoc!
/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Mar 1, 2022

Going to push as commit 5cad68f.
Since your change was applied there have been 25 commits pushed to the master branch:

  • c0effc2: 8211333: AArch64: Fix another build failure after JDK-8211029
  • 80919eb: 8279669: test/jdk/com/sun/jdi/TestScaffold.java uses wrong condition
  • ba6c4c1: 8261107: ArrayIndexOutOfBoundsException in the ICC_Profile.getInstance(InputStream)
  • 80a2777: 8282372: [11] build issue on MacOS/aarch64 12.2.1 using Xcode 13.1: call to 'log2_intptr' is ambiguous
  • 68c6320: 8214004: Missing space between compiler thread name and task info in hs_err
  • 16cbd32: 8250750: JDK-8247515 fix for OSX pc_to_symbol() lookup fails with some symbols
  • 6f9d287: 8277488: Add expiry exception for Digicert (geotrustglobalca) expiring in May 2022
  • ffa5ae8: 8247515: OSX pc_to_symbol() lookup does not work with core files
  • 4378844: 8254085: javax/swing/text/Caret/TestCaretPositionJTextPane.java failed with "RuntimeException: Wrong caret position"
  • 26e599d: 8247272: SA ELF file support has never worked for 64-bit causing address to symbol name mapping to fail
  • ... and 15 more: https://git.openjdk.java.net/jdk11u-dev/compare/f4e14a685ef4c8eb7c3f861b9c0bd981e5544c36...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated label Mar 1, 2022
@openjdk openjdk bot closed this Mar 1, 2022
@openjdk openjdk bot removed ready rfr labels Mar 1, 2022
@openjdk
Copy link

@openjdk openjdk bot commented Mar 1, 2022

@GoeLin Pushed as commit 5cad68f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@GoeLin GoeLin deleted the goetz_backport_8255410 branch Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport integrated
5 participants