On Wed, 1 Oct 2025 20:13:45 GMT, Weijun Wang <[email protected]> wrote:
>> Anthony Scarpino has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> missed some decoder comments
>
> src/java.base/share/classes/java/security/PEMDecoder.java line 374:
>
>> 372: * the PEM footer or the end of the stream. If an I/O error occurs,
>> 373: * the read position in the stream may become inconsistent.
>> 374: * It is recommended to perform no further decoding operations
>
> Three lines above. Is it better to say "until the end of the first PEM
> footer"? Same with the other decode-from-stream method.
"first" sounds awkward to me, I can change it to "a PEM footer".
> src/java.base/share/classes/java/security/PEMEncoder.java line 69:
>
>> 67: * {@link KeyPair} objects passed to the {@code encode} or
>> 68: * {@code encodeToString} methods are encoded as a
>> 69: * OneAsymmetricKey structure using the "PRIVATE KEY" type.
>
> Should "OneAsymmetricKey" be fixed-width `OneAsymmetricKey`? Also, 4 lines
> above here.
I think it would be confusing to have @code around it or other monospaced font
when there is no class with that name. If that were the case, then PKCS #8 and
ASN.1 would need it to and I think it would become harder to read with all the
font changes.
> src/java.base/share/classes/java/security/PEMEncoder.java line 95:
>
>> 93: * <li>{@code PKCS8EncodedKeySpec} (if configured with encryption) :
>> 94: * ENCRYPTED PRIVATE KEY</li>
>> 95: * <li>{@code PEM} : {@code PEM.type()}</li>
>
> `X509EncodedKeySpec` missing.
line 91?
> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 1:
>
>> 1: /*
>
> Since you added `getKey` and `getKeyPair` with a password argument, can we
> also add a `getKeySpec` with the same argument to be consistent?
>
> Also, if you add this method, can it be used instead of
> `Pem.decryptEncoding`? That method is called inside EPKI and it creates
> another EPKI which looks wasteful and dangerously recursive.
Hmm.. I had not planned to add any to getKeySpec as I view that as legacy. We
can consider that in a future update.
I can probably change the internal method to take the `this` instead.
> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line
> 459:
>
>> 457: public static EncryptedPrivateKeyInfo encryptKey(DEREncodable de,
>> 458: Key encryptKey, String algorithm, AlgorithmParameterSpec params,
>> 459: Provider provider, SecureRandom random) {
>
> Technically, must `encryptKey` be a PBE key? Can it be any key? I notice that
> PBE is not mentioned in `getKey`.
No, I should clean that up.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2411999544
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2412463817
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2396287482
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2414636177
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2417915297
