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

8298420: PEM API: Implementation (Preview) #17543

Open
wants to merge 44 commits into
base: master
Choose a base branch
from

Conversation

ascarpino
Copy link
Contributor

@ascarpino ascarpino commented Jan 24, 2024

Hi all,

I need a code review of the PEM API. Privacy-Enhanced Mail (PEM) is a format for encoding and decoding cryptographic keys and certificates. It will be integrated into JDK24 as a Preview Feature. Preview features does not permanently define the API and it is subject to change in future releases until it is finalized.

Details about this change can be seen at PEM API JEP.

Thanks

Tony


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change requires CSR request JDK-8329419 to be approved
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Integration blocker

 ⚠️ Title mismatch between PR and JBS for issue JDK-8298420

Warnings

 ⚠️ Patch contains a binary file (src/java.base/share/classes/jdk/internal/icu/impl/data/icudt74b/nfc.nrm)
 ⚠️ Patch contains a binary file (src/java.base/share/classes/jdk/internal/icu/impl/data/icudt74b/nfkc.nrm)
 ⚠️ Patch contains a binary file (src/java.base/share/classes/jdk/internal/icu/impl/data/icudt74b/ubidi.icu)
 ⚠️ Patch contains a binary file (src/java.base/share/classes/jdk/internal/icu/impl/data/icudt74b/uprops.icu)
 ⚠️ Patch contains a binary file (src/java.desktop/share/classes/javax/swing/text/doc-files/Document-insert.gif)
 ⚠️ Patch contains a binary file (src/java.desktop/share/classes/sun/awt/resources/security-icon-bw16.png)
 ⚠️ Patch contains a binary file (src/java.desktop/share/classes/sun/awt/resources/security-icon-bw24.png)
 ⚠️ Patch contains a binary file (src/java.desktop/share/classes/sun/awt/resources/security-icon-bw32.png)
 ⚠️ Patch contains a binary file (src/java.desktop/share/classes/sun/awt/resources/security-icon-bw48.png)
 ⚠️ Patch contains a binary file (src/java.desktop/share/classes/sun/awt/resources/security-icon-interim16.png)
 ⚠️ Patch contains a binary file (src/java.desktop/share/classes/sun/awt/resources/security-icon-interim24.png)
 ⚠️ Patch contains a binary file (src/java.desktop/share/classes/sun/awt/resources/security-icon-interim32.png)
 ⚠️ Patch contains a binary file (src/java.desktop/share/classes/sun/awt/resources/security-icon-interim48.png)
 ⚠️ Patch contains a binary file (src/java.desktop/share/classes/sun/awt/resources/security-icon-yellow16.png)
 ⚠️ Patch contains a binary file (src/java.desktop/share/classes/sun/awt/resources/security-icon-yellow24.png)
 ⚠️ Patch contains a binary file (src/java.desktop/share/classes/sun/awt/resources/security-icon-yellow32.png)
 ⚠️ Patch contains a binary file (src/java.desktop/share/classes/sun/awt/resources/security-icon-yellow48.png)
 ⚠️ Patch contains a binary file (src/java.desktop/windows/native/libawt/windows/security_warning.ico)
 ⚠️ Patch contains a binary file (src/utils/IdealGraphVisualizer/View/src/main/resources/com/sun/hotspot/igv/view/images/hideDuplicates.png)

Issues

  • JDK-8298420: Implement PEM Encodings of Cryptographic Objects (Preview) (Enhancement - P2)(⚠️ The fixVersion in this issue is [25] but the fixVersion in .jcheck/conf is 24, a new backport will be created when this pr is integrated.) ⚠️ Title mismatch between PR and JBS.
  • JDK-8329419: PEM API: Implementation (Preview) (CSR)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17543/head:pull/17543
$ git checkout pull/17543

Update a local copy of the PR:
$ git checkout pull/17543
$ git pull https://git.openjdk.org/jdk.git pull/17543/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17543

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17543.diff

Using Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 24, 2024

👋 Welcome back ascarpino! 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
Copy link

openjdk bot commented Jan 24, 2024

@ascarpino 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 Jan 24, 2024
@openjdk
Copy link

openjdk bot commented Mar 13, 2024

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

@openjdk
Copy link

openjdk bot commented Mar 28, 2024

@ascarpino this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout pem
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Mar 28, 2024
@openjdk openjdk bot changed the title JDK-8300911: PEM API (Preview) 8300911: PEM API (Preview) Apr 10, 2024
@openjdk openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Apr 22, 2024
Fixed P8v2 0xA0
Test: Use EKPI with PBEParameters to match EPK8 base64
cleanup
Copy link
Member

Choose a reason for hiding this comment

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

Update copyright year.

Copy link
Member

Choose a reason for hiding this comment

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

Update copyright year, if needed.

* AlgorithmParameterSpec of that provider.
*
* @param key The PrivateKey object to encrypt.
* @param password the password used in the PBE encryption.
Copy link
Member

Choose a reason for hiding this comment

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

Consider indicating that a clone happens.

@@ -81,6 +81,9 @@ public enum Feature {
STREAM_GATHERERS,
@JEP(number=476, title="Module Import Declarations", status="Preview")
MODULE_IMPORTS,
//XXX Number will change when assigned
@JEP(number=999, title="PEM API", status="Preview")
Copy link
Member

Choose a reason for hiding this comment

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

No JEP # assigned yet?

Copy link
Member

Choose a reason for hiding this comment

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

I just checked, and there isn't one yet, so maybe you can use this comment as a reminder to update later. :)

Copy link
Member

Choose a reason for hiding this comment

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

Update copyright year, if required.

Copy link
Member

Choose a reason for hiding this comment

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

Update copyright year, if required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

date is correct

Copy link
Member

Choose a reason for hiding this comment

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

Consider undoing the diff/change to remove the newline, or technically, you'll need to update the copyright year, I suppose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll restore the line

Copy link
Member

Choose a reason for hiding this comment

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

Update copyright year, if required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

has correct year

@michael-o
Copy link

This JEP is misnamed. The RFC clearly says

   For reasons that basically boil down to non-coordination or
   inattention, many PKIX, PKCS, and CMS libraries implement a text-
   based encoding that is similar to -- but not identical with -- PEM
   encoding. 
...
Unlike legacy PEM encoding [[RFC1421](https://www.rfc-editor.org/rfc/rfc1421)], OpenPGP ASCII armor, and the
   OpenSSH key file format, textual encoding does *not* define or permit
   headers to be encoded alongside the data.  Empty space can appear
   between the pre-encapsulation boundary and the base64, but generators
   SHOULD NOT emit such any such spacing.  (The provision for this empty
   area is a throwback to PEM, which defined an "encapsulated header
   portion".)

So this RFC is clearly not PEM and this JEP shouldn't be named as such, hence class names neither.

@ascarpino
Copy link
Contributor Author

ascarpino commented Oct 22, 2024

This JEP is misnamed. The RFC clearly says

   For reasons that basically boil down to non-coordination or
   inattention, many PKIX, PKCS, and CMS libraries implement a text-
   based encoding that is similar to -- but not identical with -- PEM
   encoding. 
...
Unlike legacy PEM encoding [[RFC1421](https://www.rfc-editor.org/rfc/rfc1421)], OpenPGP ASCII armor, and the
   OpenSSH key file format, textual encoding does *not* define or permit
   headers to be encoded alongside the data.  Empty space can appear
   between the pre-encapsulation boundary and the base64, but generators
   SHOULD NOT emit such any such spacing.  (The provision for this empty
   area is a throwback to PEM, which defined an "encapsulated header
   portion".)

So this RFC is clearly not PEM and this JEP shouldn't be named as such, hence class names neither.

PEM has evolved over time as the RFC states, but that doesn't change that PEM is the established term for this textual format. RFC1421 was not added to the JEP because it does not need to explain the history. To quote the whole paragraph:

The tradition within the RFC series can be traced back to Privacy-
Enhanced Mail (PEM) [[RFC1421](https://www.rfc-editor.org/rfc/rfc1421)],
 based on a proposal by Marshall Rose in Message Encapsulation 
[[RFC934](https://www.rfc-editor.org/rfc/rfc934)].  Originally called 
"PEM encapsulation mechanism", "encapsulated PEM message", or 
(arguably) "PEM printable encoding", today the format is sometimes 
referred to as "PEM encoding".  Variations include OpenPGP ASCII 
armor [[RFC4880](https://www.rfc-editor.org/rfc/rfc4880)] and 
OpenSSH key file format [[RFC4716](https://www.rfc-editor.org/rfc/rfc4716)].

The JEP is clear that PKCS#8 and X.509 are supported. Other variations could be added to the PEM API in the future or by a different API.

OpenSSL use the "PEM" for -inform, -outform, and many other examples. BouncyCastle has PEMReader, PEMWriter, and PEMParser. Even wikipedia states that "The PEM format was eventually formalized by the IETF in RFC 7468". The Java API using a different term would lead to unnecessary confusion.

// KeyType not relevant here. We just want KeyFactory
if ((PKCS8EncodedKeySpec.class).isAssignableFrom(tClass)) {
getKeyFactory(key.getAlgorithm()).
getKeySpec(key, PKCS8EncodedKeySpec.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you forget to assign above to so?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, and that may address a bug found by testing a few days ago.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe rename to PEMData. There are non-cert here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, makes sense

Copy link
Contributor

@wangweij wangweij left a comment

Choose a reason for hiding this comment

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

Some comments on EncryptedPrivateKeyInfo.

* PKCS#8 ASN.1 encoding.
* @param encoded the ASN.1 encoding to be parsed.
* @throws NullPointerException if {@code encoded} is {@code null}.
* @throws IOException if error occurs when parsing the ASN.1 encoding.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change the old spec? There seems to be no problem. Especially, why remove the "array are copied" sentence?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can re-add the cloning. As far as the others, parses was implied by from an ASN.1 encoding. and I thought being more specific about what type of ASN.1 encoding was better

* @return an EncryptedPrivateKeyInfo.
* @throws IllegalArgumentException when an argument causes an
* initialization error.
* @throws SecurityException on a cryptographic errors.
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I didn't notice this before. Have we decided to repurpose SecurityException for this usage now that there will be no more Security Manager?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if it was exclusively a Security Manager exception, but @seanjmullan had said previously that SecurityException was used elsewhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

It was used when JarInputStream::read encountered a wrong hash or signature. But, it was mainly used for permission violation.

}

/**
* Return a PrivateKey from the object's encrypted data with a KeyFactory
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be SecretKeyFactory. Same in @param provider below.

Also, "Return a key" is not clear. Existing getKeySpec methods use "Extract". You maybe also use "Recover".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think "Return" is very clear and I'm not a fan of "Extract" or "Recover". The other methods are overly descriptive of the method's internals rather than the just stating the purpose. Even with that, the other methods end with "returns it."

}

/**
* Creates and encrypts an `EncryptedPrivateKeyInfo` from a given PrivateKey
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, you should use {@code} here. Same in @impNote below. Also, there are some other class names that should be in enclosed in {@code}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

* @exception NoSuchAlgorithmException if cannot find appropriate
* cipher to decrypt the encrypted data.
* @exception NullPointerException if {@code decryptKey} is {@code null}.
* @exception NoSuchAlgorithmException Cannot find appropriate cipher to
Copy link
Contributor

Choose a reason for hiding this comment

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

Use "if".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

* from the given Provider.
*
* @param password the password
* @param provider the KeyFactory provider used to generate the key.
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you allow it to be null, mention it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Copy link
Contributor

Choose a reason for hiding this comment

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

New encrypt and decrypt methods are all password-based and work on keys directly. Old methods uses a decryption key and works on key specs. For completeness; have you thought about more combinations? Maybe at least encryption with a key? I assume an EncryptedPrivateKeyInfo is not only encrypted with a password.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could add some Key related methods.

@wangweij
Copy link
Contributor

wangweij commented Oct 30, 2024

Can you please support the read-public-key-from-pkcs8 feature in NamedKeyFactory::engineGeneratePublic method? It could be something like

        } else if (keySpec instanceof PKCS8EncodedKeySpec p8spec) {
            try {
                var p8key = new PKCS8Key(p8spec.getEncoded());
                var pubEncoding = p8key.getPubKeyEncoded();
                if (pubEncoding == null) {
                    throw new InvalidKeySpecException(
                            "This PKCS8EncodedKeySpec does not contain a public key");
                }
                return fromX509(p8key.getPubKeyEncoded());
            } catch (InvalidKeyException e) {
                throw new InvalidKeySpecException(e);
            }

BTW, I see in your other KeyFactory updates you haven't checked if getPubKeyEncoded() returns null. Will it throw NPE instead of IKSE?

Copy link
Contributor

@wangweij wangweij left a comment

Choose a reason for hiding this comment

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

For PEMEncoder.

Also, do you want to update the PKCS8EncodedKeySpec class with a new ASN.1 grammar and a link to version 2.0?

* on a PEMEncoder instance returned by {@link #withEncryption(char[])} or
* by passing an {@link EncryptedPrivateKeyInfo} object into the encode methods.
* <p>
* PKCS8 v2.0 allows OneAsymmetric encoding, which is a private and public
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a link to PKCS 8 2.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add an external link? I don't believe that is allowed

*
* @apiNote
* Here is an example of encoding a PrivateKey object:
* <pre>
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to code snippet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

* @param de a cryptographic object to be PEM encoded that implements
* DEREncodable.
* @return PEM encoding in a String
* @throws IllegalArgumentException when the passed object returns a null
Copy link
Contributor

Choose a reason for hiding this comment

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

What does "returns a null binary encoding" mean? There is no other method talking about this. I think we can just say "if configured for encryption but object does not support" since this looks like the only reason.

Also, how about IllegalStateException?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getEncoded() returns null.
I think your suggestion would still need me to explain why the object doesn't support encryption.
I don't think IllegalStateException makes sense when object passed does not provide the needed data.

yield pemEncoded(new PEMRecord(
PEMRecord.ENCRYPTED_PRIVATE_KEY, epki.getEncoded()));
} catch (IOException e) {
throw new SecurityException(e);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you really want to use SecurityException? This only happens when the AlgorithmParameters inside EPKI is not initialized. I would say this is a very good candidate for an IllegalStateException.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For consistency, InvalidArgumentException is better than SecurityException

* {@link #encode(DEREncodable)}.
*
* @param password sets the encryption password. The array is cloned and
* stored in the new instance.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can password be empty? I vaguely remember some algorithms might not like it, or, is it just that SecretKeySpec does not like an empty key?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PBEKeySpec allows null and empty passwords and I hope the provider/algorithm would throw an error if that was a problem.
I changed this to allow null. I realized EKPI allowed null, but PEM didn't. It would be consistent to just allow what PBEKeySpec allows.

Copy link
Contributor

@wangweij wangweij left a comment

Choose a reason for hiding this comment

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

Comments for PEMDecoder.

* is useful for algorithm-agnostic methods, {@code ECPublicKey} for
* algorithm-specific operations, or {@code X509EncodedKeySpec} if the
* X.509 binary encoding is desired instead of a Key object. An IOException
* will be thrown if the class is incorrect for the given PEM data.
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no IOE in this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep

}

/**
* Configures and returns a new {@code PEMDecoder} instance from the
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you going to be more specific on what kind of factories will be involved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

* Configures and returns a new {@code PEMDecoder} instance from the
* current instance that will use Factory classes from the specified
* {@link Provider}. Any errors using the {@code provider} will occur
* during decoding.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean errors will happen during decoding? Do you want to be clear on what exceptions will be thrown?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Errors will be thrown during decoding, when decode() is called. This method sets the provider in the PEMDecoder instance. This method throws no exceptions.

* the default provider configuration.
*
* @param provider the Factory provider.
* @return a new PEM decoder instance.
Copy link
Contributor

Choose a reason for hiding this comment

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

The return spec for this method and the next one should be using a consistent wording.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep

throw new IllegalArgumentException("No PEM data found.");
}

DEREncodable so = decode(pem);
Copy link
Contributor

@wangweij wangweij Oct 30, 2024

Choose a reason for hiding this comment

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

The line above could throw IOE. Shall we wrap it in an IAE? I mean the same error in the other decode-from-string method is an IAE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that would be consistent

if (next.isContextSpecific((byte)0)) {

// OPTIONAL Context tag 0 for Attributes for PKCS8 v1 & v2
// Uses 0xA0 constructed define-length or 0x80 constructed
Copy link
Member

Choose a reason for hiding this comment

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

Minor nit: 0xA0 = context-specific/constructed, 0x80 = context-specific/primitive. Definite length vs. indefinite length is not defined by the tag itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok


}

if (pubKeyEncoded != null) {
Copy link
Member

@jnimeh jnimeh Oct 31, 2024

Choose a reason for hiding this comment

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

Looking back at an earlier conversation between you and Weijun, I think I read that pubKeyEncoded will be set/overwritten if the private key encoding holds a public key. So when consuming a PKCS#8 EC key, where the private key is itself a SEC1-v2 formatted key encoding with a pubkey, wouldn't the version be set to 0 (v1), but the pubKeyEncoded is also non-null?
I ask only because upon running this method, wouldn't you end up making the output a v2 OneAsymmetricKey, still with the SEC1-v2 private key (with pub key) and also have it in the public key section?