Add SealedBox support #127
Conversation
| } | ||
|
|
||
| public byte[] encrypt(byte[] message) { | ||
| if (publicKey == null) |
There was a problem hiding this comment.
We need to avoid waiting until encrypt time to determine that the public key passed at initialization is incorrect.
There was a problem hiding this comment.
Ok, moved that check into the constructor, now throwing IllegalArgumentException though that also inherits RuntimeException. Couldn't think of an appropriate checked exception here (if we want to go that way, we could do a custom exception though).
| } | ||
|
|
||
| public byte[] decrypt(byte[] ciphertext) { | ||
| if (privateKey == null) |
There was a problem hiding this comment.
We need to avoid waiting until decrypt time to determine that the public key passed at initialization is incorrect.
|
great to see support for
|
|
@om26er ok, I forked this to our org at https://github.com/crossbario/libsodium-jni - could you pls continue there and merge above - we need to continue work and are blocked on this, so lets use our fork from now on |
|
I'm sorry, though I don't see how this matches the libsodium api for sealed box. The proposed constructor is expecting the keys to be generated outside the constructor. Why are the keys being generated outside the constructor? What is the use case? |
|
|
||
| public byte[] encrypt(byte[] message) { | ||
| byte[] ct = new byte[message.length + SEAL_BYTES]; | ||
| isValid(sodium().crypto_box_seal( |
There was a problem hiding this comment.
The Util class is marked deprecated and should not be used.
isValid is supposed to return a boolean, though throws a RuntimeException.
There was a problem hiding this comment.
Ok, I have removed the usage of isValid and have moved the check internally.
What's the planned approach to deal with all the deprecated classes ?
There was a problem hiding this comment.
Planning to remove the deprecated classes in a major release.
The deprecated classes haven't been removed yet as it's unclear impact to downstream developers.
|
I agree with @joshjdevl the ephemeral key should not be a constructor parameter (provided externally), but IMO should be created newly each time
|
Ok, it seems the private key was not needed to encrypt, as that was taken care by libsodium internally, it was used to actually decrypt the message, which didn't really make sense, so now I have changed the I think a similar approach could also be used for the encrypt method i.e. make that a static as well, which takes two parameters: |
|
rgd deprecated classes: not sure what is meant here (what classes?). if sth is deprecated in the future, lets fix it then - unless the deprecated classes would be in the interface of this new class. my 2cts my main remaining itch would be: IMO the as a sender, I want to create a sealed box using the receiver public key provided in ctor, and then reuse that box to encrypt many messages - this is now the case. as a receiver, I want to create a sealed box using my private key provided in ctor, and then reuse that box to decrypt many messages - this is not yet the case (I have to store my private key outside, and then provide it each time I decrypt a message). this interface design is asymmetric (rgd where pub/priv key is provided - once in ctor, once in method). again, pynacl has got this right, in as: the interface is minimal and intuitive .. |
Code is mostly taken from https://github.com/abstractj/kalium/blob/master/src/main/java/org/abstractj/kalium/crypto/SealedBox.java, also doesn't currently have tests and is mostly untested.
I assume we would want to change the method signatures to throw exceptions before merging.