API suggestions
Rolf-Smit opened this issue · 4 comments
Currently we're using Java-pinning in our Android app and we need to do some more advanced stuff. As checking the certificate expire date.
While working with the library I found some difficulties, which can easily be solved by making the API a bit more flexible.
1. Make getPinBytes() public:
We needed to create an X509Certificate object (so we can obtain the expire date) and therefor we needed the raw certificate bytes, we could either do this by decoding the plain cert HEX string to a byte array, but since we we're already using some Pin objects it's way more convenient to get the bytes from those objects. Currently I have to extend the CertPlainPin in order to make the method public.
2. Add getX509Certificate() method to the CertPlainPin class
It makes sense to add some useful methods to the different Pin classes, especially obtaining the X509Certificate seems reasonable and useful. Using the library becomes way more fun with this in-place:
new CertPlainPin(hexString).getX509Certificate().checkValidity();
3. Make Pin constructors public
There is no reason to make the constructors protected. I can imagine many cases where I don't have a string with type-prefix (like: CERTPLAIN). For example in cases where I have a X509Certificate object I wan't to be able to create a Pin like this:
new CertPlainPin(tohex(x509certificate));
4. More constructors
To make it easier to create pins from things other than HEX strings it would be nice to have extra constructors, for example:
new PublicKeyPin(PublicKey publicKey);
new Pin(byte[] bytes); //This one is especially useful. Take for example my third suggestion, it can be simplified as the toHex method is not needed anymore.
new CertPlainPin(X509Certificate certificate);
5. Create a specialized exception
Currently this library reuses the java.security.cert.CertificateException exception type, this is fine however an sub type of this exception would be better, like CertificateNotPinnedException extends CertificateException as this makes it possible to differentiate between other CertificateExceptions and not pinned exceptions.
I'm willing to create a pull-request including test cases, just let me know!
Thanks for your feedback. I'm the rest of the week on a business trip, but I try to share my thoughts on your suggestions as soon as I'm back.
I'm not found of the idea of making it public because they bytes are sometimes not sufficient to re-create the certificate. It only works for 'plain' type pins. Instead I propose we create a convinince method which exaclty fits your use case: Pin.maybeGetX509Certificate(). That would be better API design IMHO.
Having CertPlainPin.getX509Certificate() seems sensible.
I do think there is a good reason to have the Pin constructors not part of the public API. Static factory methods like java-pinning currently uses are easier to handle when extending the API and also provider other benefits. It sounds what you really want is a static method Pin.fromX509Certificate(X509Certificate).
Boils to down to adding a static method Pin.fromPublicKey(PublicKey).
Creating CertificateNotPinnedException reads like a good idea.
You are welcome to contribute. I would perfer a single PR per semantic change. And sorry that it took me so long to respond.
Thanks for your response, I'm happy to contribute. I will create a few separate PR's for this.
1-2: Lets create CertPlainPin.getX509Certificate()
3-4: I agree, lets create static factory methods.
5. +1