<div dir="auto"><div>... try again from from my subscribed mail account...<br><div class="gmail_quote"><div dir="ltr" class="gmail_attr"><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Tony,<br>
<br>
in my jdk fork, I created a branch named pem-feedback-karl.<br>
<br>
<a href="https://github.com/KarlScheibelhofer/jdk/tree/pem-feedback-karl" rel="noreferrer noreferrer" target="_blank">https://github.com/KarlScheibelhofer/jdk/tree/pem-feedback-karl</a><br>
<br>
It is based on the pem branch of your jdk fork.<br>
In this pem-feedback-karl branch, I did some cleanup without changing<br>
the API. Your tests pass as before.<br>
<br>
My original pem-keystore implementation for the SUN provider is in this branch<br>
<br>
<a href="https://github.com/KarlScheibelhofer/jdk/tree/pem-keystore" rel="noreferrer noreferrer" target="_blank">https://github.com/KarlScheibelhofer/jdk/tree/pem-keystore</a><br>
<br>
It did not use the PEM API.<br>
<br>
In the branch<br>
<br>
<a href="https://github.com/KarlScheibelhofer/jdk/tree/pem-keystore-pem-api" rel="noreferrer noreferrer" target="_blank">https://github.com/KarlScheibelhofer/jdk/tree/pem-keystore-pem-api</a><br>
<br>
I modified the PEM keystore implementation to use the PEMDecoder and PEMEncoder.<br>
To make it pass all tests, I had to fix some issues with the PEM api:<br>
<br>
* fix missing line-breaks when encoding certificates (and CRLs)<br>
* use uniform line length for PEM encoding keys and certificates<br>
<br>
During this work, I took some notes regarding the PEM api:<br>
<br>
* Consider decoupling the raw PEM encoding and decoding from SecurityObject.<br>
This would make the API usable for general purpose PEM encoding and<br>
decoding, not just for keys and certificates, as it is now.<br>
<br>
* For this PEM keystore implementation it is essential to parse the<br>
preceding explanatory text lines.<br>
The PEM decoder should support this.<br>
As it is now, the keystore implementation must parse the PEM objects<br>
itself, including reading PEM header and footer lines.<br>
Having to handle half the work in the application diminishes the<br>
value of the PEMDecoder.<br>
It only delegates the decoding of certificates and keys to the PEMDecoder.<br>
<br>
* PEMDecoder should be able to handle or pass through unknown PEM<br>
objects gracefully.<br>
Otherwise the application has to check the PEM labels in advance<br>
itself, which does not make sense.<br>
<br>
* Though supporting InputStream/OutputStream for reading and writing<br>
makes sense,<br>
supporting Reader/Writer feels even more essential for robust<br>
support for all character encodings.<br>
Multi-Byte character encodings won't work with InputStream/OutputStream.<br>
<br>
* The standard line separator for PEM is "\r\n".<br>
For PEM files stored in a typical linux file system, "\n" is<br>
typically used, however.<br>
See the output of openssl, for example.<br>
Because there are still several command line utilities that do not<br>
work well with "\r\n" line breaks.<br>
Supporting a different line-separator than "\r\n" is a good idea in<br>
my opinion.<br>
Base64.getMimeEncoder also supports selecting a custom line separator.<br>
<br>
* The PEMEncoder encodes the predefined SecurityObjects only.<br>
There is no way to use it to PEM encoded any other type of object.<br>
Consider opening a path to generic use.<br>
<br>
* If an application has a DER encoded certificate, it has to decode<br>
and parse the certificate before<br>
it can encode it using PEMEncoder.<br>
This is inconvenient.<br>
<br>
* PEMEncoder uses 64 characters as line length for private and public keys,<br>
and 76 characters for certificates.<br>
Use the same line length for all types by default.<br>
Consider adding support for selecting a custom line length.<br>
Base64.getMimeEncoder also supports selecting a line custom length.<br>
<br>
I hope this helps advancing the PEM efforts.<br>
<br>
Best regards, Karl<br>
<br>
On Thu, Jan 25, 2024 at 10:02 PM Anthony Scarpino<br>
<<a href="mailto:anthony.scarpino@oracle.com" target="_blank" rel="noreferrer">anthony.scarpino@oracle.com</a>> wrote:<br>
><br>
><br>
> On 1/25/24 9:20 AM, Daniel Jeliński wrote:<br>
> > Hi Tony,<br>
> > Thanks for the links! The API looks very promising.<br>
> ><br>
> > Out of curiosity, why aren't you using the Base64 MIME<br>
> > encoder/decoder? They are supposed to produce/remove the newline<br>
> > characters.<br>
><br>
> I can look it over again. I had inconsistencies during testing with<br>
> expected data and returned data.<br>
><br>
> > The relationship between the byte[] and String data should be<br>
> > specified. Base64 explicitly specifies that the String APIs are<br>
> > translated to the byte[] APIs with ISO 8859-1 encoding. The PEMDecoder<br>
> > is currently using the default charset, which might produce<br>
> > interesting results if the charset is set to EBCDIC. The encoder is<br>
> > using UTF-8, which is reasonable, but given that the produced output<br>
> > will always be ASCII, ISO 8859-1 will perform better.<br>
><br>
> That's fine<br>
><br>
> > There's a disparity between the decoder and the encoder APIs; both<br>
> > work with strings, but Decoder accepts InputStream and not arrays, and<br>
> > Encoder produces byte arrays, but does not work with streams. This<br>
> > should be more uniform. I like Decoder's InputStream support (that's<br>
> > currently the only way to read multiple CA certificates from a single<br>
> > file - the String overload currently only returns the first one), so<br>
> > I'd add OutputStream support to Encoder for parity.<br>
><br>
> I don't see parity as a requirement. I see encoding and decoding as<br>
> having unique requirements. Decoding from a file or InputStream makes<br>
> sense. A decode(byte[]) method didn't seem necessary as I felt it<br>
> unlikely a user would have a single byte[] with PEM data. They were<br>
> more likely to have a String or an InputStream. The developer can wrap<br>
> it with ByteArrayInputStream or String(byte[], Charset), which is what a<br>
> public API method would do internally.<br>
><br>
> Encoding is a single operation on an object. Pass in a SecurityObject<br>
> and PEM data is returned. Returning a byte[] is the most flexible<br>
> without creating more methods. In the case where meta data is<br>
> in-between the PEM or some other data formatting.<br>
><br>
> Why should the API have an OutputStream method, something like:<br>
> pem.encodeToOutputStream(so, outputstream);<br>
> when the API as written today, the developer can use:<br>
> outputstream.writeBytes(pem.encode(obj));<br>
><br>
> I don't like to add API methods just for symmetry, they need to have a<br>
> compelling reason. I have not seen that in the OutputStream case.<br>
><br>
><br>
> > Karl's earlier<br>
> > suggestion to support Stream<SecurityObject> also makes a lot of<br>
> > sense.<br>
> I haven't counted out Stream, I just haven't seen a compelling reason.<br>
> My tests use stream() from an array list of PEM strings. But I haven't<br>
> seen the situation where the API needs stream support that couldn't be<br>
> done in a different way. This is a preview JEP, and we still have time.<br>
> If there is a compelling example, point it out to me.<br>
><br>
> > I'm not a big fan of the non-static factory methods<br>
> > withEncryption/withDecryption/withFactory. The problem with non-static<br>
> > methods that return an instance of the same type is that you need to<br>
> > check the documentation to know if the method returns a new instance<br>
> > or if it mutates the current one. Can we use static factory methods<br>
> > instead? Either that, or create a builder class.<br>
><br>
> The API states the classes are immutable which was a big requirement and<br>
> it why it's stated all over the docs. A builder class was considered<br>
> early in the API development, but it was too much complication for a few<br>
> optional cases where the developer may need to specify details like<br>
> encryption or a factory. The API has the builder idea, without the<br>
> extra builder construction methods. I don't see how a static factory<br>
> method fit here.<br>
><br>
> > I don't like the PEMEncoder.withEncryption API. It's not predictable<br>
> > enough; when encoding data, it's not consistent between writing<br>
> > unencrypted data (certificate, crl), throwing (PublicKey,<br>
> > EncryptedPrivateKeyInfo) and writing encrypted data (unencrypted<br>
> > private keys). The alternative of forcing the users to encrypt using<br>
> > EncryptedPrivateKeyInfo looks better to me.<br>
><br>
> That was a design decision to make the API easier to use. The<br>
> non-security export does not need to be burden with understanding<br>
> EncryptedPrivateKeyInfo.<br>
><br>
> The API can be consistent if you choose to only pass in<br>
> EncryptedPrivateKeyInfo and not set withEncryption(). If an<br>
> ArrayList<SecurityObject> encodes with a stream(), it is nice to<br>
> configure encryption for private keys and still encode public keys and<br>
> certificates with the same encoder.<br>
><br>
><br>
> > I'd love to see support for the OpenSSL private key formats; it seems<br>
> > that RSAPrivateCrtKeyImpl already supports PKCS#1 format, so it may be<br>
> > just a matter of exposing that functionality. Other key types like EC<br>
> > might need more work. That might be added later after the API is<br>
> > finalized.<br>
><br>
> OpenSSL 3.0 is transitioning away from their format to PKCS#8. It is an<br>
> obsoleted format. While I have thought about decoding support of RSA<br>
> PKCS#1 for compatibility, I have no intention to support OpenSSL or<br>
> PKCS#1 encoding with this PEM API.<br>
><br>
> If someone is interested in old OpenSSL or other encoding formats, that<br>
> is why the Encoder and Decoder interfaces are included. To give a<br>
> structure for developing other encoding.<br>
><br>
> ><br>
> > Thanks,<br>
> > Daniel<br>
> ><br>
> > śr., 24 sty 2024 o 22:24 Anthony Scarpino<br>
> > <<a href="mailto:anthony.scarpino@oracle.com" target="_blank" rel="noreferrer">anthony.scarpino@oracle.com</a>> napisał(a):<br>
> >><br>
> >> Hi,<br>
> >><br>
> >> The following github link is to the PEM API as it is written in the<br>
> >> draft JEP (<a href="https://openjdk.org/jeps/8300911" rel="noreferrer noreferrer" target="_blank">https://openjdk.org/jeps/8300911</a>). There has been a few<br>
> >> changes since the original posting.<br>
> >><br>
> >> <a href="https://urldefense.com/v3/__https://github.com/ascarpino/jdk/tree/pem__;!!ACWV5N9M2RV99hQ!NmZu22NrC2hxWJuqLHZ6l1C0KYVK0Qf_rV7tV-1uLqUb_5sFMJXyCCKVPjmEmGCeQ6US2RJquDm9AJqZXO46ju8Q$" rel="noreferrer noreferrer" target="_blank">https://urldefense.com/v3/__https://github.com/ascarpino/jdk/tree/pem__;!!ACWV5N9M2RV99hQ!NmZu22NrC2hxWJuqLHZ6l1C0KYVK0Qf_rV7tV-1uLqUb_5sFMJXyCCKVPjmEmGCeQ6US2RJquDm9AJqZXO46ju8Q$</a><br>
> >><br>
> >> The Encoder and PEMEncoder to now return byte[] for the encode() method.<br>
> >> A new PEMEncoder method, encodeToString(), was added. I believe these<br>
> >> make it easier for outputting data to a file and InputStreams, while<br>
> >> still supporting a method that returns a String.<br>
> >><br>
> >> For decode, InputStream has replaced Reader. There were comments<br>
> >> preferring InputStream and I found that Reader's buffering quirks were<br>
> >> problematic. Decoding from a byte[] is easy through an ByteArrayInputStream.<br>
> >><br>
> >> If you are interested in testing out the API, please download and<br>
> >> compile the repo. Then let me know how your experience went.<br>
> >><br>
> >> thanks<br>
> >><br>
> >> Tony<br>
</blockquote></div></div></div>