Qabel / qabel-core

(B2C) :cloud: Implementation of Qabel-Core in Java
Other
20 stars 17 forks source link

Private key operations in KeyPair Class #442

Open roeslpa opened 8 years ago

roeslpa commented 8 years ago

In order to ensure that the private key is never misused I would suggest to process all operations with the private key in the QblECKeyPair Class and thus remove the public method getPrivateKey(). As far as I know only the computation of the index DM name uses the private key directly. Any arguments against?

roeslpa commented 8 years ago

@jmt85 why was this issue closed? It might be implemented in the future, but in my view it is relevant. If you disagree, please provide a reason.

audax commented 8 years ago

The issue of accidently misusing the private key is tackled by code reviews. Moving all operations with the private key into the class couples it very deeply with all other modules.

See julians answer: https://github.com/Qabel/qabel-core/pull/467#issuecomment-194487288

We are currently refactoring contacts, identities and all that stuff, including the persistance. After that refactoring a clean solution for how to couple the private key to modules that use the private key may present itself.

roeslpa commented 8 years ago

From my point of view there are only very few use cases for the private key like decryption, signing, being input for a finger print (or hash sum for e.g., the prefix) and being exported. Since all this can be implemented totally independent from modules inside the private key class, there would not be a need for code review of external modules to ensure that it is not misused.

I am not writing this to complicate the implementation but there are several attack vectors which could arise by using the key (e.g.: timing issues) and by preventing the usage outside the class a review would be much easier.

julianseeger commented 8 years ago

If it keeps independent of lower level concepts ( unlinke #467 ) I'm totally ok with pulling the actions into one key class as long as is represents a single responsibility (*crypting / hashing /... stuff).

Don't forget that we have to persist and export the private key => some parts just need to get it. And it might give a wrong impression of security because you can always use reflections like

Field key = QblEcKeyPair.class.getField("privateKey");
key.setAccessible(true);
byte[] owned = key.getValue(keyPair);   // or similar...