On Thu, 12 Dec 2024 19:59:05 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 with a new target base due to a > merge or a rebase. The pull request now contains 58 commits: > > - Merge branch 'pem-merge' into pem > - merge > - Merge in PEMRecord as part of the API. > - Merge branch 'pem-record' into pem-merge > > # Conflicts: > # src/java.base/share/classes/java/security/PEMDecoder.java > # src/java.base/share/classes/java/security/PEMRecord.java > # src/java.base/share/classes/sun/security/util/Pem.java > # test/jdk/java/security/PEM/PEMData.java > # test/jdk/java/security/PEM/PEMDecoderTest.java > # test/jdk/java/security/PEM/PEMEncoderTest.java > - Merge branch 'master' into pem-record > > # Conflicts: > # src/java.base/share/classes/jdk/internal/javac/PreviewFeature.java > - test fixes & cleanup > - Implement stream decoding > fix StringBuffer/Builder > X509C* changes > - Reorg tests data > minor fixes > - merge from pem branch > - Merge branch 'pem' into pem-record > > # Conflicts: > # src/java.base/share/classes/java/security/PEMEncoder.java > # src/java.base/share/classes/sun/security/provider/X509Factory.java > # src/java.base/share/classes/sun/security/util/Pem.java > # test/jdk/java/security/PEM/PEMDecoderTest.java > # test/jdk/java/security/PEM/PEMEncoderTest.java > - ... and 48 more: https://git.openjdk.org/jdk/compare/22845a77...cc952c0b src/java.base/share/classes/java/security/PEMDecoder.java line 58: > 56: * </pre> > 57: * > 58: * A specified return class must extend {@link DEREncodable} and be an nit: extend -> implement src/java.base/share/classes/java/security/PEMDecoder.java line 81: > 79: * > 80: * @apiNote > 81: * Here is an example of encoding a PrivateKey object: Nit: encoding -> decoding src/java.base/share/classes/java/security/PEMDecoder.java line 143: > 141: case Pem.PRIVATE_KEY -> { > 142: PKCS8Key p8key = new > PKCS8Key(decoder.decode(pem.pem())); > 143: KeyFactory kf = getKeyFactory(p8key.getAlgorithm()); nit: I see a lot of `p8key.getAlgorithm()` calls in here. If it's always going to be the same value, perhaps it could be assigned to a String and just reuse it throughout (though it may not be usable for the `p8.getAlgorithm()` calls further down). src/java.base/share/classes/java/security/PEMDecoder.java line 294: > 292: > 293: if (tClass.isAssignableFrom(PEMRecord.class)) { > 294: //if (PEMRecord.class.isInstance(tClass)) { nit: dead code? src/java.base/share/classes/java/security/PEMRecord.java line 48: > 46: * All values can never be null. > 47: * > 48: * During the instantiation of this record, there is no validation for the These two sentences are redundant. Either one is fine to keep though. src/java.base/share/classes/java/security/PEMRecord.java line 53: > 51: * instantiation of this record. > 52: * > 53: * @param type The type identifier in the PEM header. For a public key, Since there are specific strings that will be needed in the `type` field, is there a pointer we could add so folks looking at this could see what values might be acceptable for this field. I don't know if right here is the right place for it, but maybe a see-also or something? Or is this left open-ended to support whatever could exist in a PEM header? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1992070595 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1992074350 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1992118727 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1992132131 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1992257185 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1992263251
