Add support for PKCS#1 keys#6
Add support for PKCS#1 keys#6aaschmid merged 3 commits intoaaschmid:pkcs1.and.pem.bc.by.bgregos#6from
Conversation
|
I see a good bit of low-hanging fruit for cleanup, I'll try to update this soon. |
|
@bgregos: Thanks for your changes. In sum I like them pretty much. I would appreciate some tests, though. But as I haven't had some myself, I thing I first need to provide some infrastructure for testing this. |
| } catch (IOException e) { | ||
| throw new TaskwarriorKeyStoreException(e, "Could not read private key '%s' via input stream: %s", privateKeyFile, | ||
| e.getMessage()); | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
Why removed throwing the exception?
| e.printStackTrace(); | ||
| } | ||
|
|
||
| String privateKeyString = "----..."; |
There was a problem hiding this comment.
The rest should go to a separate method.
| } catch (InvalidKeySpecException e) { | ||
| throw new TaskwarriorKeyStoreException(e, "Could not generate private key for '%s': %s", privateKeyFile, e.getMessage()); | ||
| PemReader pemReader; | ||
| pemReader = new PemReader(new InputStreamReader(new ByteArrayInputStream(privKeyBytes))); |
|
|
||
| privateKeyObject = pemReader.readPemObject(); | ||
| } catch (Exception e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
I prefer throwing an exception or logging it. What's your opinion?
| e.printStackTrace(); | ||
| } | ||
| } else { | ||
| throw new RuntimeException("Unsupported key type: " + privateKeyObject.getType()); |
| ) | ||
| ); | ||
| } catch (IOException e) { | ||
| e.printStackTrace(); |
aaschmid
force-pushed
the
master
branch
2 times, most recently
from
a89e83a to
79954b2
Compare
aaschmid
left a comment
aaschmid
left a comment
There was a problem hiding this comment.
I am still not sure if I want to include such a "huge"(TM) library into my "core" application but maybe I will provide another version. There is one other possibility (see http://magnus-k-karlsson.blogspot.com/2018/05/how-to-read-pem-pkcs1-or-pkcs8-encoded.html) with the ugly tradeoff that it uses sun.* packages. Would they be available on android jdk?
|
|
||
| dependencies { | ||
| // https://mvnrepository.com/artifact/org.bouncycastle/bcprov-jdk15on | ||
| implementation group: 'org.bouncycastle', name: 'bcprov-jdk15on', version: '1.60' |
There was a problem hiding this comment.
I prefer to create a shadow jar including BC using https://imperceptiblethoughts.com/shadow/. But we could that later on as well...
|
|
||
| dependencies { | ||
| // https://mvnrepository.com/artifact/org.bouncycastle/bcprov-jdk15on | ||
| implementation group: 'org.bouncycastle', name: 'bcprov-jdk15on', version: '1.60' |
There was a problem hiding this comment.
I prefer to create a shadow jar including BC using https://imperceptiblethoughts.com/shadow/. But we could that later on as well...
|
And it would be nice to sign-off your commits as stated in https://github.com/aaschmid/taskwarrior-java-client/blob/master/CONTRIBUTING.md and agree the terms of the license agreement. Thanks @bgregos |
| @@ -23,12 +23,8 @@ task wrapper(type: Wrapper) { | |||
| dependencies { | |||
| // https://mvnrepository.com/artifact/org.bouncycastle/bcprov-jdk15on | |||
| implementation group: 'org.bouncycastle', name: 'bcprov-jdk15on', version: '1.60' | |||
There was a problem hiding this comment.
I need to use org.bouncycastle:bcpkix-jdk15on to satisfy all used classes in your code.
|
Maybe you can already rebase your changes onto https://github.com/aaschmid/taskwarrior-java-client/tree/pkcs1.and.pem.support.%232. That would be very helpful @bgregos. But please give me a hint if I should do myself :-) |
aaschmid
force-pushed
the
master
branch
2 times, most recently
from
e15ca06 to
48fc394
Compare
…ient into bgregos-master (#6) * 'master' of git://github.com/bgregos/taskwarrior-java-client: cleanup add support for PKCS#1 keys Signed-off-by: Andreas Schmid <service@aaschmid.de>
aaschmid
changed the base branch from
master
to
pkcs1.and.pem.bc.by.bgregos#6
|
@bgregos: I merged your branch into a branch of mine in order to use your changes to my code for a version use bouncycastle for support of PKCS#1 keys. I would be very nice if you could agree the terms of the license agreement as in https://github.com/aaschmid/taskwarrior-java-client/blob/master/CONTRIBUTING.md. Thanks! |
* dependencies require to use `org.bouncycastle:bcpkix-jdk15on` to
satisfy all used classes
* reformat and cleanup code
* throw exceptions rather than siliently swallow with
`e.printStackTrace()`
* fix spotbugs warning using byte stream without encoding
* NOT DONE:
* split up into separate methods -> will follow later using kept
but currently unused methods
Signed-off-by: Andreas Schmid <service@aaschmid.de>
* pkcs1.and.pem.bc.#6: refactor if-else-cascade to switch and improve method names and error messages package and publish dependencies (esp. bouncycastle) using shadow jar replace hard-coded key type with constants inline method and improve error message for PKCS#8 key generation replace properitary `sun.securtiy.*` API with bouncycastle code reuse already existing private key generation from bytes for PKSC#8 standard incorporate requested review changes from #6 document the new key probabilities in README.md use newly introduced PKCS1 and PKCS8 support for pem files (#2) add PKCS1 and PKCS8 support for pem files (#2) enhance integration tests to be able to cover all kinds of key types easily (#2) cleanup add support for PKCS#1 keys
|
Hello, apologies for the delay. I hereby agree to the terms of the Taskwarrior Java client License Agreement. Thanks for your additions to the PR as well; let me know if anything else needs done in relation to this PR. |
|
Thanks, all the things are included in the version on Maven Central. I am actually curious about your results including the published version into your Android app :-) |
2 participants
A quick and dirty implementation that allows for PKCS#1 keys. This is helpful for taskservers like inthe.am. The major downside to adding this code is that BouncyCastle is now added as a dependency, but I don't at present see a good way around it.