On Thu, 17 Apr 2025 15:51:09 GMT, Anthony Scarpino <ascarp...@openjdk.org> wrote:
>> 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](https://bugs.openjdk.org/browse/JDK-8300911). >> >> Thanks >> >> Tony > > Anthony Scarpino has updated the pull request incrementally with two > additional commits since the last revision: > > - javadoc updates > - code review comments Some comments on the `PEMxyz` classes. src/java.base/share/classes/java/security/PEMDecoder.java line 60: > 58: * A specified return class must implement {@link DEREncodable} and be an > 59: * appropriate JCE object class for the PEM; otherwise an > 60: * {@link IllegalArgumentException} is thrown. Do we need to document somewhere what "appropriate" JCE classes are for each PEM type? src/java.base/share/classes/java/security/PEMDecoder.java line 213: > 211: Objects.requireNonNull(str); > 212: try { > 213: return decode(new ByteArrayInputStream(str.getBytes())); Does `getBytes` require a charset argument? The only thing I'm concerned is someone put UTF-8 chars before the PEM block. I think we've seen certificates containing subject or issuer names there and they are not ASCII. Shall we document the encoding of leading data in `PEMRecord`? src/java.base/share/classes/java/security/PEMDecoder.java line 398: > 396: * Returns a new {@code PEMDecoder} instance from the current > instance > 397: * configured to decrypt encrypted PEM data with given password. > 398: * Non-encrypted PEM may still be decoded from this instance. I assume that if users want to use a non-default cipher provider they should decode as `EncryptedPrivateKeyInfo` first and it has more sophisticated methods there. Do we need to document about this here? src/java.base/share/classes/java/security/PEMEncoder.java line 120: > 118: > 119: /** > 120: * Returns an instance of PEMEncoder. Do we need to say "simple" or "normal" or "raw" `PEMEncoder` here? Just to be clear that it does not do any encryption. src/java.base/share/classes/java/security/PEMEncoder.java line 134: > 132: private String pemEncoded(PEMRecord pem) { > 133: StringBuilder sb = new StringBuilder(1024); > 134: sb.append("-----BEGIN ").append(pem.type()).append("-----"); So you throw away the leading data? Shall we document this somewhere? src/java.base/share/classes/java/security/PEMEncoder.java line 175: > 173: throw new IllegalArgumentException("KeyPair does not > " + > 174: "contain PrivateKey."); > 175: } Do we really need to care about whether the private part or the public part is null? src/java.base/share/classes/java/security/PEMEncoder.java line 235: > 233: */ > 234: public byte[] encode(DEREncodable de) { > 235: return encodeToString(de).getBytes(StandardCharsets.ISO_8859_1); Which operation is lighter? Have you considered letting `pemEncoded` to return a byte array and converting it into a string in `encodeToString()`? src/java.base/share/classes/java/security/PEMEncoder.java line 279: > 277: if (keySpec != null) { > 278: // For thread safety > 279: lock.lock(); How much does this lock buy? If someone provides a password, I assume they will use it anyway. src/java.base/share/classes/java/security/PEMEncoder.java line 287: > 285: keySpec = null; > 286: } catch (GeneralSecurityException e) { > 287: throw new SecurityException("Security property " + Let's discuss whether to use `SecurityException` here. I would use `ProviderException`. src/java.base/share/classes/java/security/PEMEncoder.java line 300: > 298: // If `key` is non-null, this is an encoder ready to encrypt. > 299: if (key != null) { > 300: if (privateBytes == null || publicBytes != null) { `publicBytes` cannot be null here. You rejected both being null at the beginning of this method. src/java.base/share/classes/java/security/PEMRecord.java line 61: > 59: * RFC 7468: Textual Encodings of PKIX, PKCS, and CMS Structures > 60: */ > 61: public record PEMRecord(String type, String pem, byte[] leadingData) How about using the raw byte data instead of the `pem` string? This would automatically reject all format problems, like extra newline at the end, too wide string, and invalid Base64 chars. Also, two `PEMRecord` should equal to each other even if they are encoded differently (for example, different width). Also, how about forcing all fields to be non null? If there is no leading bytes, use an empty array. I cannot imagine how data could be empty. src/java.base/share/classes/java/security/PEMRecord.java line 79: > 77: * @param pem The data between the PEM header and footer. > 78: * @param leadingData Any non-PEM data read during the decoding > process > 79: * before the PEM header. This value maybe {@code > null}. Do we need to say if `leadingData` contains the final newline char? src/java.base/share/classes/java/security/PEMRecord.java line 95: > 93: // including lowercase. The onus is on the caller. > 94: if (type != null && type.startsWith("-----")) { > 95: // Remove PEM headers syntax if present. I don't think we need to be so tolerant at the beginning. Just reject it. src/java.base/share/classes/java/security/PEMRecord.java line 135: > 133: /** > 134: * Returns the binary encoding from the Base64 data contained in > 135: * {@code pem}. The name does not sound correct to me. PEM encodes binary data to an ASCII string, so "encoding" is the ASCII string. How about just `getData`? ------------- PR Review: https://git.openjdk.org/jdk/pull/17543#pullrequestreview-2792041040 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2059012734 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2059031256 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2059036057 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2058988622 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2058978573 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2058995881 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2058985486 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2058997407 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2059003315 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2059001002 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2058954734 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2058957229 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2058962946 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2058953881
