On Fri, 26 Jul 2024 04:02:21 GMT, Anthony Scarpino <ascarp...@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
