On Wed, 24 Jan 2024 00:01:06 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 Some comments. src/java.base/share/classes/java/security/Key.java line 1: > 1: /* This file is not modified. src/java.base/share/classes/java/security/PEMEncoder.java line 88: > 86: > 87: // If non-null, encoder is configured for encryption > 88: private Cipher cipher = null; Is it worth creating `cipher` lazily? Also, why does `PEMDecoder` allows setting a factory but not here? src/java.base/share/classes/java/security/PEMEncoder.java line 224: > 222: try { > 223: os.writeBytes(Base64.getMimeEncoder(64, > Pem.LINESEPARATOR) > 224: .encode(c.getEncoded())); Should we add a `os.writeBytes(Pem.LINESEPARATOR)` like for keys here? Same with CRL. src/java.base/share/classes/java/security/cert/CRL.java line 1: > 1: /* This file is not modified. src/java.base/share/classes/java/security/cert/Certificate.java line 1: > 1: /* This file is not modified. src/java.base/share/classes/java/security/spec/EncodedKeySpec.java line 32: > 30: > 31: import java.io.IOException; > 32: import java.security.DEREncodable; Useless import. src/java.base/share/classes/java/security/spec/EncodedKeySpec.java line 71: > 69: this.encodedKey = encodedKey.clone(); > 70: try { > 71: algorithmName = > KeyUtil.getAlgorithm(this.encodedKey).getName(); What if `algorithmName` is assigned an OID in raw string? I see that `EncodedKeySpec::getAlgorithm` has not specified whether the return value is a standard algorithm name but usually we only return standard names. src/java.base/share/classes/java/security/spec/KeySpec.java line 1: > 1: /* This file is not modified. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 325: > 323: * @param key the PrivateKey object to encrypt. > 324: * @param password the password used in the PBE encryption. > 325: * @param algorithm the algorithm to encrypt with. Do you need to tell what algorithms can be used here? Is it something like "PBEwithAESandWhatever"? Do you need to add a link to somewhere in the Standard Names Doc? src/java.base/share/classes/sun/security/pkcs/PKCS8Key.java line 109: > 107: throws InvalidKeyException { > 108: this(privEncoding); > 109: pubKeyEncoded = pubEncoding; So if there is already a public key in `privEncoding`, it will be overwritten? BTW, it seems this method is not used anywhere. src/java.base/share/classes/sun/security/util/DerInputStream.java line 425: > 423: public Optional<DerValue> getOptionalConstructed(int n, byte tag) > 424: throws IOException { > 425: if (checkNextTag(t -> (t & 0x0c0) == 0x080 && (t & 0x020) == > 0x020 && is it possible to combine this with `getOptionalImplicitContextSpecific`? If I understand correctly, the CONSTRUCTED flag should be retained in the encoding even if it's IMPLICIT. Therefore, if `tag` has 0x20 then `t` should also have, vice versa. src/java.base/share/classes/sun/security/util/KeyUtil.java line 468: > 466: value = is.getDerValue(); > 467: // This route is for: RSAPublic, Encrypted RSAPrivate, EC > Public, > 468: // Encrypted EC Private, This looks a little too smart. I see it's only used by PKCS#8 and X.509 keys. Can we instead add 2 static methods in those 2 classes? src/java.base/share/classes/sun/security/util/Pem.java line 47: > 45: * Public Key PEM header & footer > 46: */ > 47: public static final byte[] PUBHEADER = "-----BEGIN PUBLIC KEY-----" Maybe add some underscores to make the names more readable? src/java.base/share/classes/sun/security/util/Pem.java line 104: > 102: public static final String DEFAULT_ALGO; > 103: static { > 104: DEFAULT_ALGO = > Security.getProperty("jdk.epkcs8.defaultAlgorithm"); Do you want to fail if the security property is not defined? src/java.base/share/classes/sun/security/util/Pem.java line 136: > 134: public static ObjectIdentifier getPBEID(String algorithm) { > 135: try { > 136: if (algorithm.contains("AES")) { Is this check reliable? src/java.base/share/classes/sun/security/util/Pem.java line 152: > 150: /** > 151: * Read the PEM text and return it in it's three components: header, > 152: * base64, and footer Add some spec on what the stream position is at after the method is called. Is it after the `-----` of footer? Is it after the newline chars after it? src/java.base/share/classes/sun/security/util/Pem.java line 303: > 301: return new Pem(header.getBytes(StandardCharsets.ISO_8859_1), > 302: data.getBytes(StandardCharsets.ISO_8859_1), > 303: footer.getBytes(StandardCharsets.ISO_8859_1)); So you read bytes and accumulate them as strings and then return as bytes at the end. Is it possible to not convert to strings in between? Or, in `PEMDecoder` you compare the header and footer to bytes arrays. Is it possible to return header and footer as strings and compare to strings there? Plus, can the data part be de-BASE64ed here? src/java.base/share/classes/sun/security/x509/AlgorithmId.java line 1: > 1: /* This file is not really modified. test/jdk/java/security/PEM/PEMCerts.java line 39: > 37: class PEMCerts { > 38: public static final String ecprivpem = """ > 39: -----BEGIN PRIVATE KEY-----\ What does the `` at the end mean? test/jdk/java/security/PEM/PEMCerts.java line 294: > 292: > 293: static String makeNoCRLF(String pem) { > 294: return Pattern.compile("/n").matcher(pem).replaceAll(""); You mean no new lines inside the PEM? Is that something legal? test/jdk/sun/security/pkcs11/ec/policy line 6: > 4: permission java.security.SecurityPermission "removeProvider.*"; > 5: permission java.security.SecurityPermission > 6: "getProperty.jdk.epkcs8.defaultAlgorithm"; Maybe we should read this property with `doPrivileged`? ------------- PR Review: https://git.openjdk.org/jdk/pull/17543#pullrequestreview-2199476754 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1691692001 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1692107049 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1692102124 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1691550299 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1691550521 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1691565445 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1691562795 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1691564145 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1692048914 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1691777319 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1692065501 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1692072184 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1692072960 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1692055705 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1692054103 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1692077332 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1692120260 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1691701936 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1692226179 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1692227437 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1692230174
