jdk 8298420: PEM API: Implementation (Preview)

trafficstars

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
[x] Change must not contain extraneous whitespace
[x] Commit message must refer to an issue

Integration blocker

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

Issues

JDK-8298420: Implement PEM Encodings of Cryptographic Objects (Preview) (Enhancement - P2) ⚠️ 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

Webrev

Link to Webrev Comment

Jan 24 '24 00:01 ascarpino

:wave: 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.

Jan 24 '24 00:01 bridgekeeper[bot]

@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.

Jan 24 '24 00:01 openjdk[bot]

@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:

8298420: Implement JEP 470: PEM Encodings of Cryptographic Objects (Preview)

Reviewed-by: weijun, mr, mullan, jnimeh

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

28f509317d477c5f4076658f9ae9995aa6c53631: 8356233: NMT: tty->print_cr should not be used in VirtualMemoryTracker::add_reserved_region()
cd052c72cdb62186e66c1d2ecf9216f3df61b242: 8345431: Improve jar --validate to detect duplicate or invalid entries
b2a61a9972493d67d0f1a9f3f529c11e45838d5b: 8356985: Use "stdin.encoding" in Console's read*() methods
... and 228 more: https://git.openjdk.org/jdk/compare/5e50a584744d316dd881c9404f75e65f31bb0e75...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.

Mar 13 '24 20:03 openjdk[bot]

@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

Mar 28 '24 20:03 openjdk[bot]

@ascarpino This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Jul 06 '24 04:07 bridgekeeper[bot]

/csr

Jul 24 '24 23:07 ascarpino

@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-8300911 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

Jul 24 '24 23:07 openjdk[bot]

Webrevs

Jul 25 '24 03:07 mlbridge[bot]

As I was exploring an idea for updates to the API, I came across a code optimization that I wanted to get into this code review as quickly as possible since I just announced the code review. It makes handing the internals of the PEM structures much easier.

Jul 26 '24 03:07 ascarpino

@ascarpino This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Aug 29 '24 04:08 bridgekeeper[bot]

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.

Oct 18 '24 20:10 michael-o

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.

Oct 22 '24 00:10 ascarpino

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?

Oct 30 '24 20:10 wangweij

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.

I happily accept your explanation, thanks for taking the time to dive into!

Nov 12 '24 15:11 michael-o

@ascarpino This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Dec 10 '24 18:12 bridgekeeper[bot]

⚠️ @ascarpino This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

Dec 12 '24 19:12 openjdk[bot]

@ascarpino This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

Jan 07 '25 21:01 bridgekeeper[bot]

/open

Jan 07 '25 22:01 ascarpino

@ascarpino This pull request is now open

Jan 07 '25 22:01 openjdk[bot]

@ascarpino This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Feb 05 '25 01:02 bridgekeeper[bot]

Sorry if I'm just jumping into this without having all the context, but maybe it would be a good idea to update the GenerateCacerts build tool to use this new API? That would mean free testing during build, and a chance to "dogfood" the API.

Feb 26 '25 14:02 magicus

Sorry if I'm just jumping into this without having all the context, but maybe it would be a good idea to update the GenerateCacerts build tool to use this new API? That would mean free testing during build, and a chance to "dogfood" the API.

Thanks for the comment. GenerateCacerts.java already uses a CertificateFactory method that decodes the PEM, so there is no need to change this to the PEM API. However the generateCertificate() method internally calls the PEM decoder internal methods.

Feb 26 '25 23:02 ascarpino

The code at https://github.com/openjdk/jdk/blob/ddf04617887dc389cd7667e820da7ac91eea9e8c/src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java#L449 is old. It does not cover the public key case.

Reproducer:

var pass = "changeit".toCharArray();
var kp = KeyPairGenerator.getInstance("X25519").generateKeyPair();
var sk2 = PEMDecoder.of().decode(PEMEncoder.of().encodeToString(kp), PrivateKey.class); // create a private key with public key inside
var sk2s = PEMEncoder.of().withEncryption(pass).encodeToString(sk2);
var epki = PEMDecoder.of().withDecryption(pass).decode(sk2s);

May 14 '25 15:05 wangweij

/integrate

May 28 '25 19:05 ascarpino

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

28f509317d477c5f4076658f9ae9995aa6c53631: 8356233: NMT: tty->print_cr should not be used in VirtualMemoryTracker::add_reserved_region()
cd052c72cdb62186e66c1d2ecf9216f3df61b242: 8345431: Improve jar --validate to detect duplicate or invalid entries
b2a61a9972493d67d0f1a9f3f529c11e45838d5b: 8356985: Use "stdin.encoding" in Console's read*() methods
... and 228 more: https://git.openjdk.org/jdk/compare/5e50a584744d316dd881c9404f75e65f31bb0e75...master

Your commit was automatically rebased without conflicts.

May 28 '25 19:05 openjdk[bot]

@ascarpino Pushed as commit bb2c80c0e9923385e0b6243c0ebff9afef208470.

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

May 28 '25 19:05 openjdk[bot]

jdk jdk copied to clipboard

8298420: PEM API: Implementation (Preview)

Progress

Integration blocker

Issues

Reviewing

Webrev

Webrevs

jdk
jdk copied to clipboard