Re Re: [14] RFR 8162628: Migrate cacerts keystore from JKS

[Michael Osipov]

> > On Aug 14, 2019, at 3:23 AM, Michael Osipov <1983-01-06 at gmx.net> wrote:
> >
> > Am 2019-08-13 um 18:23 schrieb Weijun Wang:
> >> Please take a preliminary review at
> >>
> >>    https://cr.openjdk.java.net/~weijun/8162628/webrev.00
> >>
> >> There is no test yet. I mainly want you to see if this is doable and whether there can be any unexpected compatibility impact.
> >>
> >> So, the major points are:
> >>
> >> 1. Invent a new KeyStore type named "PEM", which is a stack of PEM-format certificates. It only support X.509 certificates and is read-only (at the moment).
> >> 2. Migrate lib/security/cacerts to this format.
> >>
> >> Some details:
> >>
> >> 1. JKS/PKCS12/PEM is now aliases to each other, which means you can load a PKCS12 keystore using KeyStore.getInstance("pem"). This is an expansion of the former JKS/PKCS12 dual type.
> >> 2. PEM supports engineProbe(), and returns true as long as the first 5 bytes are readable ASCII. This is because people might put comment before "-----BEGIN CERT-----".
> >> 3. @attr can be added into comment as attributes in the comment area. cacerts will contain "@alias: aliasname". I'm still using the "[jdk]" label in the alias for jdkCA recognition.
> >
> > My comments:
> > GenerateCacerts:
> > * Why not use Files#newBufferedWriter(Path)?
>
> I like println.

OK for me ;-)

> > KeyStoreDelegator:
> > * Please avoid iterating a list like an array and using List#get(int).
>
> But I have 2 lists. It is a Pair but Java does not have it, and I don't like Map::Entry very much.

Granted, by why not use two iterators then?

> > PemKeyStore:
> > * It pretty much looks like you don't support private keys. Do you intend
> >  to deliver that in a seperate issue?
>
> To be determined, but no requirement now. Also, I don't want to invent a competitor for pkcs12.
>
> > * engineStore(): Why is that not supported? Without password of course.
>
> No requirement. I also don't want to see people losing their comments.

Granted.

> > * engineLoad():
> > ** Why not use Reader r = new Buffered...?
>
> readLine() is only in BufferedReader.

Granted.

> > ** split(":\\s*", 2): expect the unexpected: "@", kv might be null.
>
> jshell> "@".substring(1).split(":\\s*", 2)
> $1 ==> String[1] { "" }

Great!

> >
> > I will also pass a pretty large cacerts with public CA and our CAs and
> > see wether your parser doesn't choke on it.

Did you get a change to parse the file I have sent you?

> PEM is certainly slower than JKS because of text reading and de-Base64. I'll see if I can make any enhancement.

Commons Codec has a Base64InputStream. Maybe the entire file should be read as a stream with specific substreams
for the base64 parts? That is, of course, much more coding work.

Looking forward for your improvements.

Do you see a change to backport this code to 11 and 8, but not changing the default keystore type? With that
this great change could be available way earlier than Java 17 for most LTS people.

Regards,

Michael