RFR: 8298420: PEM API: Implementation (Preview) [v2]

[Weijun Wang]

On Fri, 26 Jul 2024 04:02:21 GMT, Anthony Scarpino <ascarpino at openjdk.org> wrote:

>> 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?
>
> if cipher is defined this is an encrypted PEMEncoder instance, so yes it's important.
> Encoding doesn't need a factory when the object already provides the DER encoding.

Not sure if I understand. When using encryption, user can set a cipher algorithm that is not implemented in any of the builtin providers, so the `SecretKeyFactory.getInstance` and `Cipher.getInstance` might need a provider argument.

>> 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?
>
> By the spec, probably not.. but parsers are suppose to be flexible and excluding formats usually ends up is user complaints.  It's no extra work to allow it.

Not sure if it's good to be so flexible at the beginning. You won't be able to make it more restricted in the future.

>> 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`?
>
> I don't see it needing any extra permissions than others.

I meant even if you wrap the `Security.getProperty("jdk.epkcs8.defaultAlgorithm")` call in a `doPrivileged` then there is no need to add a new permission here.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1693138034
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1693139030
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1693140134