-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8360564: Implement JEP 524: PEM Encodings of Cryptographic Objects (Second Preview) #27147
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
Conversation
|
👋 Welcome back ascarpino! A progress list of the required criteria for merging this PR into |
|
@ascarpino 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: 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 98 new commits pushed to the
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 |
|
@ascarpino The following label will be automatically applied to this pull request:
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. |
|
/csr |
|
@ascarpino has indicated that a compatibility and specification (CSR) request is needed for this pull request. @ascarpino please create a CSR request for issue JDK-8360564 with the correct fix version. This pull request cannot be integrated until the CSR request is approved. |
Webrevs
|
wangweij
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments.
| String p = pem.content().replaceAll("(.{64})", "$1\r\n"); | ||
| return pemEncoded(pem.type(), p); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it correct to put the 2 new methods here? Seems not closely related to PEM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are needed by PEM and EKPI, so this seems like the right place.
| * @param pair set to true for returning a KeyPair, if possible. Otherwise, | ||
| * return a PrivateKey | ||
| * @param provider KeyFactory provider | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is very similar to PKCS8Key::parseKey. Maybe we should combine them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before the code review, I intentionally split these apart. I didn't like how the code looked and that returning a DEREncodable complicated parseKey(). It made PKCS8Key much more complicated with typecasting and passing in pair.
If I redo PKCS8Key.java, this would be on the list to find a more elegant way to merge them.
| // Store key material for subclasses to parse | ||
| privKeyMaterial = val.data.getOctetString(); | ||
|
|
||
| // Special check and parsing for ECDSA's SEC1v2 format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So there are 2 ways to extract public key from PKCS8. Are you going to check whether they match?
Also, can we add a test case for this format?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this method, there is only one path for SEC1v2 requires the version to be 1, PKCS8 requires the version to be 2. The two should not intersect in this method.
It is true that a version 2 OneAsymmetricKey could contain a SEC1v2 private key encoding. At that point a KeyFactory needs to generate both keys to verify. I think that goes beyond what is necessary for what amounts to a "user error" in putting the wrong public and private key pair together.
I would like to explore redoing PKCS8Key.java as it has taken on many more tasks with encoding & decoding different algorithms. But I don't want that to be tied to a JEP, rather a bug or RFE.
| Arrays.fill(oct, (byte) 0x0); | ||
| if (seq.data.available() != 0) { | ||
| DerValue derValue = seq.data.getDerValue(); | ||
| if (derValue.isContextSpecific((byte) 1)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If any of these ifs is false null is returned. Would you rather throw an IAE?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see there could be a
parameters [0] ECDomainParameters {{ SECGCurveNames }} OPTIONAL,
Shall we skip it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only checks if one is available in the private key material. If there is none, null is fine.
The domain parameters are kept as part of private key SEC1v2 encoding and can be read when generating a private key with a KeyFactory. Translating the encoding could be error-prone, and maybe incompatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I meant if both [0] and [1] are there you can skip [0] and read [1]. Currently you just check for [1].
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, I didn't add a skip.
| * CERTIFICATE, CERTIFICATE REQUEST, ATTRIBUTE CERTIFICATE, X509 CRL, PKCS7, CMS, | ||
| * PRIVATE KEY, ENCRYPTED PRIVATE KEY, RSA PRIVATE KEY, or PUBLIC KEY. | ||
| * | ||
| * <p> {@code leadingData} may be null if no non-PEM data preceded PEM header |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest rewording as "... if there was no data preceding the PEM header ..."
Why do you allow null when there is a ctor that does not have a leadingData parameter - is there a reason for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it was more friendly to specify a constructor when leadingData was known to be not needed. The other constructor is used by PEMDecoder decoding is not known until decoding the stream occurs.
wangweij
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor comments.
src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java
Outdated
Show resolved
Hide resolved
wangweij
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some tiny comments.
src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java
Outdated
Show resolved
Hide resolved
koushikthirupattur
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
| * @return a decoded byte array | ||
| * @throws IllegalArgumentException if decoding fails | ||
| */ | ||
| final public byte[] decode() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method doesn't seem to be covered by the tests. I have created a ticket to add this https://bugs.openjdk.org/browse/JDK-8371574.
It also covers the 2 methods in PEM.java in utils, which could be covered further
|
/integrate |
|
Going to push as commit ad3dfaf.
Your commit was automatically rebased without conflicts. |
|
@ascarpino Pushed as commit ad3dfaf. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Hi
Please review the Second Preview for the PEM API. The most significant changes from JEP 470 are:
PEMRecordclass toPEM.encryptKeymethods of theEncryptedPrivateKeyInfoclass to acceptDEREncodableobjects rather than justPrivateKeyobjects so that cryptographic objects with public keys, i.e.,KeyPairandPKCS8EncodedKeySpec, can also be encrypted.PEMEncoderandPEMDecoderclasses to support the encryption and decryption ofKeyPairandPKCS8EncodedKeySpecobjects.thanks
Tony
Progress
Issues
Reviewers
Reviewers without OpenJDK IDs
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27147/head:pull/27147$ git checkout pull/27147Update a local copy of the PR:
$ git checkout pull/27147$ git pull https://git.openjdk.org/jdk.git pull/27147/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 27147View PR using the GUI difftool:
$ git pr show -t 27147Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27147.diff
Using Webrev
Link to Webrev Comment