RFR: 8298420: PEM API: Implementation (Preview) [v15]
Anthony Scarpino
ascarpino at openjdk.org
Sat May 10 02:15:14 UTC 2025
On Mon, 5 May 2025 14:30:12 GMT, Sean Mullan <mullan at openjdk.org> wrote:
>> Anthony Scarpino has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 66 commits:
>>
>> - major code review comments update
>> - Merge branch 'master' into pem
>> - Merge branch 'master' into pem
>> - javadoc updates
>> - code review comments
>> - merge with master
>> - better comment and remove commented out code
>> - Merge branch 'master' into pem
>> - Merge branch 'pem-merge' into pem
>> - merge
>> - ... and 56 more: https://git.openjdk.org/jdk/compare/e2ae50d8...0c540327
>
> src/java.base/share/classes/java/security/PEMEncoder.java line 57:
>
>> 55: * that implement {@link DEREncodable} and support
>> 56: * {@linkplain PKCS8EncodedKeySpec PKCS#8} or
>> 57: * {@linkplain X509EncodedKeySpec X509} formats.
>
> The "and" in "and support {@linkplain PKCS8EncodedKeySpec PKCS#8} or {@linkplain X509EncodedKeySpec X509} formats" is too strong, ex - X509Certificate & X509CRL do not support those keyspecs. I would suggest deleting this part about "and support" as it is doesn't seem necessary.
ok
> src/java.base/share/classes/java/security/PEMEncoder.java line 64:
>
>> 62: *
>> 63: * <p> PKCS8 2.0 allows OneAsymmetricKey encoding, which may contain both private
>> 64: * and public keys in the same PEM.This is supported by using the
>
> Add space before "This".
ok
> src/java.base/share/classes/java/security/PEMEncoder.java line 118:
>
>> 116:
>> 117: /**
>> 118: * Returns a new instance of PEMEncoder.
>
> put code around PEMEncoder.
Yes
> src/java.base/share/classes/java/security/PEMEncoder.java line 120:
>
>> 118: * Returns a new instance of PEMEncoder.
>> 119: *
>> 120: * @return PEMEncoder instance
>
> s/PEMEncoder/a PEMEncoder/
>
> Use code font for PEMEncoder, also in method description.
yes
> src/java.base/share/classes/java/security/PEMEncoder.java line 122:
>
>> 120: * @return PEMEncoder instance
>> 121: */
>> 122: static public PEMEncoder of() {
>
> "public" should be before "static".
yes
> src/java.base/share/classes/java/security/PEMEncoder.java line 133:
>
>> 131: * {@code DEREncodable}.
>> 132: * @return PEM encoding in a String
>> 133: * @throws IllegalArgumentException when the passed object returns a null
>
> When would a DEREncodable return a null encoding? I think it should never return this under normal circumstances. I think we should not specifically mention this case, and be more general like "if the object cannot be encoded for some reason".
Yeah. It took me a bit to realize it meant `getEncoded()` is null.
I also think the part about encryption is unnecessary.
> src/java.base/share/classes/java/security/PEMEncoder.java line 199:
>
>> 197:
>> 198: /**
>> 199: * Encodes a given {@code DEREncodable} into PEM.
>
> Suggest:
>
> s/a given/the specified/
ok
> src/java.base/share/classes/java/security/PEMEncoder.java line 207:
>
>> 205: * configured for encryption while encoding a {@code DEREncodable} that does
>> 206: * not support encryption.
>> 207: * @throws NullPointerException when object passed is null.
>
> Wording suggestion: "if {@code de} is {@code null}"
ok
> src/java.base/share/classes/java/security/PEMEncoder.java line 215:
>
>> 213:
>> 214: /**
>> 215: * Returns a new immutable PEMEncoder instance configured to the default
>
> I don't think you need to say "immutable" as the first sentence of the class description already says PEMEncoder objects are immutable.
>
> s/configured to/configured with/
ok
> src/java.base/share/classes/java/security/PEMEncoder.java line 216:
>
>> 214: /**
>> 215: * Returns a new immutable PEMEncoder instance configured to the default
>> 216: * encryption algorithm and a given password.
>
> s/a given/the specified/
ok
> src/java.base/share/classes/java/security/PEMEncoder.java line 229:
>
>> 227: *
>> 228: * @param password sets the encryption password. The array is cloned and
>> 229: * stored in the new instance. {@code null} is a valid entry.
>
> s/entry/value/
ok
> src/java.base/share/classes/java/security/PEMEncoder.java line 230:
>
>> 228: * @param password sets the encryption password. The array is cloned and
>> 229: * stored in the new instance. {@code null} is a valid entry.
>> 230: * @return a new PEMEncoder
>
> Put code around PEMEncoder.
ok
> src/java.base/share/classes/java/security/PEMRecord.java line 53:
>
>> 51: * {@code type} or {@code pem}.
>> 52: *
>> 53: * @param type The type identifier in the PEM header. For a public key,
>
> parameter descriptions should not start with a capital letter. Same comment for the params in the ctors.
ok
> src/java.base/share/classes/java/security/PEMRecord.java line 67:
>
>> 65:
>> 66: /**
>> 67: * Return a PEMRecord instance with the given parameters.
>
> s/Return/Creates/ (same comment for all ctors)
> PEMRecord should be in code font.
ok
> src/java.base/share/classes/java/security/PEMRecord.java line 71:
>
>> 69: * <p> When {@code type} is given a properly formatted PEM header, only the
>> 70: * identifier will be set (ie: {@code PUBLIC KEY}). Otherwise, {@code type}
>> 71: * will be set to what was passed in.
>
> Put PUBLIC KEY in double quotes so it is clear this is a String.
>
> I'm not sure I understand this paragraph - will the dashes be included if the type is not understood? If not, why do we need this paragraph at all?
That paragraph needs to be removed, it was functionality that was removed during code review.
> src/java.base/share/classes/java/security/PEMRecord.java line 75:
>
>> 73: * <p> When {@code type} is given a correctly formatted PEM header, only the
>> 74: * identifier is set (for example, {@code PUBLIC KEY}). Otherwise,
>> 75: * {@code type} is set to the value that was passed in.
>
> Duplicate paragaraph as first.
Yeah. I guess I was editing it and didn't go back
> src/java.base/share/classes/java/security/PEMRecord.java line 78:
>
>> 76: *
>> 77: * @param type The type identifier in the PEM header and footer.
>> 78: * If there is no PEM data, this value will be {@code null}.
>
> Suggest rewording as "the type identifier in the PEM header and footer, or {@code null} if there is no PEM data." Same comment for other ctors.
ok
> src/java.base/share/classes/java/security/PEMRecord.java line 137:
>
>> 135: /**
>> 136: * Returns the binary encoding from the Base64 data contained in
>> 137: * {@code pem}.
>
> Should say that it returns a new copy each time it is called.
ok
> src/java.base/share/classes/java/security/PEMRecord.java line 139:
>
>> 137: * {@code pem}.
>> 138: *
>> 139: * @throws IllegalArgumentException if {@code pem} could not be decoded.
>
> s/could not/cannot/
ok
> src/java.base/share/classes/java/security/PEMRecord.java line 140:
>
>> 138: *
>> 139: * @throws IllegalArgumentException if {@code pem} could not be decoded.
>> 140: * @return binary encoding or null if {@code pem} is null.
>
> s/binary/the binary/
ok
> src/java.base/share/classes/java/security/spec/EncodedKeySpec.java line 63:
>
>> 61: * This constructor extracts the algorithm name from the encoded bytes,
>> 62: * which may be an OID if no standard algorithm name is defined. If the
>> 63: * algorithm name cannot be extracted, it is set to null.
>
> Hmm, I think this is leaking details about DER encoding into this abstract class which does not make any assumptions about the type of encoding used. Have you considered only parsing the encoding in the `X509EncodedKeySpec` and `PKCS8EncodedKeySpec` subclasses which are DER specific?
Well, [JEP 513](https://openjdk.org/jeps/513) will make it possible to limit it to those two EPSs when it integrates. Right now P8EPS and X509EPS are dependent on super() call and would require duplicating the static final variables and overriding all the methods.
What I can do right now is document it in P8EPS and X509EPS, but leave the code in EPS until the JEP integrates.
> src/java.base/share/classes/java/security/spec/PKCS8EncodedKeySpec.java line 41:
>
>> 39: * privateKeyAlgorithm PrivateKeyAlgorithmIdentifier,
>> 40: * privateKey PrivateKey,
>> 41: * attributes [0] IMPLICIT Attributes OPTIONAL,
>
> IMPLICIT has been removed in RFC 5958. Also to be consistent with 5958, I think you should change `PrivateKeyInfo` to `OneAsymmetricKey` and add an addtional line with `PrivateKeyInfo ::= OneAsymmetricKey`.
Ok.
Whats the reason `PrivateKeyInfo` have @code on it. It's not a class or a method. It's just a term.
> src/java.base/share/classes/sun/security/ec/ECKeyFactory.java line 245:
>
>> 243: throw new InvalidKeySpecException("Only ECPrivateKeySpec " +
>> 244: "and PKCS8EncodedKeySpec supported for EC private keys. " +
>> 245: keySpec.getClass().getName() + " provided.");
>
> How about using the message "keySpec.getClass().getName() + " not supported." as on lines 223-224?
Less specific.. interesting. I'm ok with that.
> src/java.base/share/classes/sun/security/pkcs/PKCS8Key.java line 79:
>
>> 77:
>> 78: /* PKCS8 version of the PEM */
>> 79: protected int version;
>
> Do these need to be protected? It doesn't seem like any subclasses need them.
I think I wanted to use it in the subclasses, but later realized the subclasses needed to parse it again anyway for algorithms-specific values. I can make it private.
> src/java.base/share/classes/sun/security/pkcs/PKCS8Key.java line 193:
>
>> 191: *
>> 192: * @param encoded the DER-encoded SubjectPublicKeyInfo value
>> 193: * @exception IOException on data format errors
>
> Change to `InvalidKeyException`.
ok
> src/java.base/share/classes/sun/security/pkcs/PKCS8Key.java line 243:
>
>> 241: }
>> 242:
>> 243: public byte[] getPrivKeyMaterial() {
>
> Do we really need this method, esp since it is the private key? I can't find any code that calls this.
It certainly was used in previous changesets. I guess it's not needed anymore.
> src/java.base/share/classes/sun/security/pkcs/PKCS8Key.java line 305:
>
>> 303: } catch (IOException e) {
>> 304: // encodedKey is still null
>> 305: throw new SecurityException(e);
>
> Should return null instead since that is what `getEncoded()` specifies.
Sure. I think some of the internal exception usage could be better. There is a lot of IOException that I may have when the API was throwing IOE. But that can be done at a later date.
> src/java.base/share/classes/sun/security/rsa/RSAKeyFactory.java line 345:
>
>> 343: } else {
>> 344: throw new InvalidKeySpecException("Only RSAPublicKeySpec "
>> 345: + "and X509EncodedKeySpec supported for RSA public keys");
>
> Update exception message to `keySpec.getClass().getName() + " not supported."` as you did in `EdDSAKeyFactory`.
ok
> src/java.base/share/classes/sun/security/rsa/RSAPrivateCrtKeyImpl.java line 362:
>
>> 360: * @throws InvalidKeyException on decoding failure
>> 361: */
>> 362:
>
> nit: remove blank line
ok
> src/java.base/share/classes/sun/security/util/Pem.java line 56:
>
>> 54: private static ObjectIdentifier PBES2OID;
>> 55:
>> 56: // Lazy initialize singleton encoder.
>
> s/initialize/initialized/
ok
> src/java.base/share/classes/sun/security/util/Pem.java line 301:
>
>> 299: * @return the string
>> 300: */
>> 301: public static String pemEncoded(String type, byte[] data) {
>
> You could refactor the common code in these two methods into a separate method and call that with the type and data.
ok
> src/java.base/share/classes/sun/security/x509/X509Key.java line 354:
>
>> 352: } catch (NoSuchAlgorithmException | InvalidKeySpecException e) {
>> 353: // Ignore and return raw key
>> 354: throw new IOException("error with encoding");
>
> How about also including the cause?
Actually that was commented out code. I think from recent change from another comment.
> src/java.base/share/conf/security/java.security line 1557:
>
>> 1555: #
>> 1556: # This property defines default Public-based Encryption algorithm for
>> 1557: # java.security.PEMEncoder is configured for encryption with `withEncryption()`.
>
> Suggested rewording:
>
> This property defines the default password-based encryption algorithm for
> java.security.PEMEncoder when configured for encryption with the withEncryption method.
ok
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2080475124
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2080496453
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2081992933
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2081993198
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2081994443
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082020210
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082026290
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082027354
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082028812
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082029044
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082371118
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082371947
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082382671
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082389619
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082394217
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082394464
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082421165
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082460595
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082460532
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082462341
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082469178
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082559879
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082722909
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082740293
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082741382
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082744160
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082751861
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082752247
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082752463
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082752768
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082759301
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082760120
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082760765
More information about the security-dev
mailing list