RFR: 8298420: PEM API: Implementation (Preview)
Weijun Wang
weijun at openjdk.org
Thu Jul 25 22:44:40 UTC 2024
On Wed, 24 Jan 2024 00:01:06 GMT, Anthony Scarpino <ascarpino at 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
More information about the security-dev
mailing list