Add PKCS8 private key support #730
Conversation
Codecov Report
@@ Coverage Diff @@
## master #730 +/- ##
============================================
+ Coverage 47.48% 47.79% +0.31%
- Complexity 1002 1009 +7
============================================
Files 125 125
Lines 6596 6605 +9
Branches 847 851 +4
============================================
+ Hits 3132 3157 +25
+ Misses 3196 3177 -19
- Partials 268 271 +3
Continue to review full report at Codecov.
|
|
Thanks a lot ! I'm bit loaded right now, but will integrate the PR (which makes much sense) over the weekend hopefully. Could you do me please a favour in the meantime and sign-off the commit as described in https://github.com/fabric8io/docker-maven-plugin/blob/master/CONTRIBUTING.md ? thanks again .... |
Signed-off-by: Stas Sukhanov <stas.sukhanov@gmail.com>
|
Hopefully I did it right. Thank you. |
|
looks good ;-) thanks ! |
|
build trigger test - please ignore .... retest this please |
| return generatePrivateKey((PrivateKeyInfo) readObject); | ||
| } | ||
| } | ||
| return null; |
There was a problem hiding this comment.
I would throw an exception in that case, since a private key is mandatory for the further processing. It better to stop where the issue happens and where we have most of the error context (i.e. read object type which is wrong).
There was a problem hiding this comment.
Thanks for the comments. That would be definitely more correct to throw exception here. Will fix.
| } | ||
|
|
||
| private static void addCA(KeyStore keyStore, String caPath) throws KeyStoreException, FileNotFoundException, CertificateException { | ||
| private static void addCA(KeyStore keyStore, String caPath) throws IOException, KeyStoreException, |
There was a problem hiding this comment.
Why is FileNotFoundException not good enough here ?
| InputStream is = new FileInputStream(certPath); | ||
| Collection<? extends Certificate> certs = CertificateFactory.getInstance("X509").generateCertificates(is); | ||
| return new ArrayList<>(certs).toArray(new Certificate[certs.size()]); | ||
| private static Certificate[] loadCertificates(String certPath) throws IOException, CertificateException { |
There was a problem hiding this comment.
Same here: Why not keep FileNotFoundException as the more specific exception ?
There was a problem hiding this comment.
The problem here that try-with-resources statement automatically closes input stream and close method throws IOException and then it goes to addCA.
| PKCS8EncodedKeySpec keySpec = new PKCS8EncodedKeySpec(keyPair.getPrivateKeyInfo().getEncoded()); | ||
| return KeyFactory.getInstance("RSA").generatePrivate(keySpec); | ||
| static PrivateKey loadPrivateKey(String keyPath) throws IOException, GeneralSecurityException { | ||
| try (Reader reader = new FileReader(keyPath); |
There was a problem hiding this comment.
thanks for adding resource management here
Signed-off-by: Stas Sukhanov <stas.sukhanov@gmail.com>
|
Thanks ! Looks good to me [merge] |
PR merged! Thanks!
Hi,
This pull request adds support for PKCS8 in key.pem file (default one is PKCS1). Also I changed a bit way of working with readers and stream by using try-with-resources statement and added a few tests.
Note that I remove wrapping FileReader to BufferedReader. I think that was unnecessary because PEMParser is inherited from BufferedReader and prefetches data itself.