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

8264849: Add KW and KWP support to PKCS11 provider #5569

Closed
wants to merge 2 commits into from

Conversation

valeriepeng
Copy link

@valeriepeng valeriepeng commented Sep 17, 2021

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


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8264849: Add KW and KWP support to PKCS11 provider

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5569/head:pull/5569
$ git checkout pull/5569

Update a local copy of the PR:
$ git checkout pull/5569
$ git pull https://git.openjdk.java.net/jdk pull/5569/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5569

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5569.diff

@valeriepeng
Copy link
Author

valeriepeng commented Sep 17, 2021

/csr

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 17, 2021

👋 Welcome back valeriep! 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 added rfr Pull request is ready for review csr Pull request needs approved CSR before integration labels Sep 17, 2021
@openjdk
Copy link

openjdk bot commented Sep 17, 2021

@valeriepeng has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@valeriepeng please create a CSR request for issue JDK-8264849. This pull request cannot be integrated until the CSR request is approved.

@openjdk
Copy link

openjdk bot commented Sep 17, 2021

@valeriepeng The following label will be automatically applied to this pull request:

  • security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the security security-dev@openjdk.org label Sep 17, 2021
@mlbridge
Copy link

mlbridge bot commented Sep 17, 2021

Webrevs

@valeriepeng
Copy link
Author

valeriepeng commented Sep 28, 2021

Please also review CSR at: https://bugs.openjdk.java.net/browse/JDK-8274174
Thanks.

@ascarpino
Copy link
Contributor

ascarpino commented Sep 30, 2021

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?

@valeriepeng
Copy link
Author

valeriepeng commented Sep 30, 2021

The corresponding PKCS11 mechanisms impl in NSS supports enc/dec/wrap/unwrap. In NIST spec 800-38F, these two modes, i.e. KW and KWP, are approved for both key wrapping as well as the protection of general data. Thus, they are implemented to support both enc/dec and wrap/unwrap.

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?

errorCode == CKR_GENERAL_ERROR) {
} else if (e.match(CKR_ENCRYPTED_DATA_INVALID) ||
e.match(CKR_GENERAL_ERROR)) {
// CKR_GENERAL_ERROR is Solaris-specific workaround
Copy link
Contributor

@ascarpino ascarpino Sep 30, 2021

Choose a reason for hiding this comment

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

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

Copy link
Author

@valeriepeng valeriepeng Oct 1, 2021

Choose a reason for hiding this comment

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

Yes, thought that it may be useful in case this got backported.

* For multi-part encryption/decryption, this class has to buffer data until
* doFinal() is called.
*
* @since 18
Copy link
Contributor

@ascarpino ascarpino Sep 30, 2021

Choose a reason for hiding this comment

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

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

Copy link
Author

@valeriepeng valeriepeng Oct 1, 2021

Choose a reason for hiding this comment

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

Sure, will fix.

// adapted from
// com/sun/crypto/provider/Cipher/KeyWrap/TestCipherKeyWrapperTest.java
public class TestCipherKeyWrapperTest extends PKCS11Test {
private static final int LINIMITED_KEYSIZE = 128;
Copy link
Contributor

@ascarpino ascarpino Sep 30, 2021

Choose a reason for hiding this comment

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

Did you mean this to be LIMITED_KEYSIZE?

Copy link
Author

@valeriepeng valeriepeng Oct 1, 2021

Choose a reason for hiding this comment

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

Not sure, the tests are adapted from the existing tests under com/sun/crypto/provider/Cipher/KeyWrap/ and I only do minimum edits to make them test against SunPKCS11 provider.


if (algoParts[0].startsWith("AES")) {
// need 3 parts
if (algoParts.length != 3) {
Copy link
Contributor

@ascarpino ascarpino Sep 30, 2021

Choose a reason for hiding this comment

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

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?

Copy link
Author

@valeriepeng valeriepeng Oct 1, 2021

Choose a reason for hiding this comment

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

Hmm, you are right, no need to check again as there are code in javax.crypto.Cipher class which handles this. I will remove it.

// see JCE spec
protected byte[] engineDoFinal(byte[] in, int inOfs, int inLen)
throws IllegalBlockSizeException, BadPaddingException {
int minOutLen = doFinalLength(inLen);
Copy link
Contributor

@ascarpino ascarpino Sep 30, 2021

Choose a reason for hiding this comment

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

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

Copy link
Author

@valeriepeng valeriepeng Oct 1, 2021

Choose a reason for hiding this comment

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

Sure, I will rename it to maxOutLen

throw new RuntimeException("Failed enc output len check");
}

//System.out.println("enc out.length=" + out.length);
Copy link
Contributor

@ascarpino ascarpino Oct 1, 2021

Choose a reason for hiding this comment

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

Lelftover debug line?

Copy link
Author

@valeriepeng valeriepeng Oct 1, 2021

Choose a reason for hiding this comment

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

Yes, will remove.

* @summary Verify general properties of the AES/KW/NoPadding,
* AES/KW/PKCS5Padding, and AES/KWP/NoPadding impls of SunPKCS11 provider.
* @library /test/lib ../..
* @run main/othervm TestGeneral
Copy link
Contributor

@ascarpino ascarpino Oct 1, 2021

Choose a reason for hiding this comment

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

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

Copy link
Author

@valeriepeng valeriepeng Oct 1, 2021

Choose a reason for hiding this comment

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

Hmm, good question. There are system provider manipulation code in PKCS11Test class, i.e. Security.addProvider(), Security.removeProvider() calls as well as setting/getting system properties. I suppose if we can be sure that things are back to a clean state after each test, then should be ok to use agentvm instead of othervm.

Copy link
Contributor

@ascarpino ascarpino left a comment

The changes look good

@valeriepeng
Copy link
Author

valeriepeng commented Oct 12, 2021

The changes look good

Thanks much for review! Could you please also review the CSR as well https://bugs.openjdk.java.net/browse/JDK-8274174?

@ascarpino
Copy link
Contributor

ascarpino commented Oct 13, 2021

I have marked the CSR as reviewed

@valeriepeng
Copy link
Author

valeriepeng commented Oct 13, 2021

I have marked the CSR as reviewed

Thanks!

@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Oct 15, 2021
@openjdk
Copy link

openjdk bot commented Oct 15, 2021

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

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

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

8264849: Add KW and KWP support to PKCS11 provider

Reviewed-by: ascarpino

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 358 new commits pushed to the master branch:

  • 831802d: 8275163: Deflater::deflate methods omit javadoc for ReadOnlyBufferException
  • ad9e234: 8275145: file.encoding system property has an incorrect value on Windows
  • f178185: 8275249: Suppress warnings on non-serializable array component types in jdk.jlink
  • ee64ce9: 8274937: Revert the timeout setting for DynamicLoaderConstraintsTest
  • 8c4da9c: 8275013: Improve discussion of serialization method declarations in java.io.Object{Input, Output}Stream
  • da8da3a: 8269698: Specification for methods of java.awt.im.InputContext should mention that they do nothing
  • 8e02064: 8049520: FileCredentialsCache loads cache once and is never refreshed
  • 172aed1: 8274338: com/sun/jdi/RedefineCrossEvent.java failed "assert(m != __null) failed: NULL mirror"
  • ced7909: 8275286: Check current thread when calling JRT methods that expect it
  • c0f3e1d: 8271071: accessibility of a table on macOS lacks cell navigation
  • ... and 348 more: https://git.openjdk.java.net/jdk/compare/7e92abe7a4bd2840fed19826fbff0285732f1765...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 Pull request is ready to be integrated label Oct 15, 2021
@valeriepeng
Copy link
Author

valeriepeng commented Oct 19, 2021

/integrate

@openjdk
Copy link

openjdk bot commented Oct 19, 2021

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

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Oct 19, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 19, 2021
@openjdk
Copy link

openjdk bot commented Oct 19, 2021

@valeriepeng Pushed as commit e63c148.

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

@valeriepeng valeriepeng deleted the JDK-8264849 branch Oct 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated security security-dev@openjdk.org
2 participants