libsodium-jni icon indicating copy to clipboard operation
libsodium-jni copied to clipboard

Add SealedBox support

Open om26er opened this issue 6 years ago • 6 comments

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.

om26er avatar Mar 13 '19 20:03 om26er

great to see support for SealedBox is coming (we will use it to add XBR support to AutobahnJava, a WebSocket/WAMP library). quick comments:

  • regarding constructors: the sender side only needs the public key of the receiver, and the receiver only needs its own private key - from which the public key is derived. so the constructors could be "cleaned up" based on this (eg look at https://pynacl.readthedocs.io/en/stable/public/#nacl-public-sealedbox) - having said that, I don't know about the interface rules/design libsodium-jni follow, and we can certainly use the current ctors
  • rgd test coverage: here is how pynacl treats that: https://pynacl.readthedocs.io/en/stable/vectors/sealedbox_vectors/ - they use a C based tool to generate test vector. since this library is wrapping the underlying libsodium, I don't think it is super important (testing the underlying libsodium is obviously done in upstream) .. the bugs that could creep in in the wrapping code here are more about actually calling the right libsodium stuff and not messing up this calling and forwarding of parameters/data from java to libsodium (C), so I do think tests at a higher level (as in, using this library) would make more sense - if so.

oberstet avatar Mar 23 '19 09:03 oberstet

@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

oberstet avatar Apr 02 '19 07:04 oberstet

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?

joshjdevl avatar Apr 27 '19 11:04 joshjdevl

I agree with @joshjdevl the ephemeral key should not be a constructor parameter (provided externally), but IMO should be created newly each time encrypt is called, so that a single SealedBox object can be (safely) reused for multiple encrypt operations using the same recipient public key.

fwiw, pynacl has this API: https://pynacl.readthedocs.io/en/stable/public/#nacl-public-sealedbox

oberstet avatar Apr 27 '19 12:04 oberstet

I agree with @joshjdevl the ephemeral key should not be a constructor parameter (provided externally), but IMO should be created newly each time encrypt is called, so that a single SealedBox object can be (safely) reused for multiple encrypt operations using the same recipient public key.

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 decrypt method to be a static, which takes ciphertext, publicKey and privateKey as parameters.

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: message and publicKey.

om26er avatar Apr 27 '19 18:04 om26er

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 decrypt interface is still slightly "wrong". It takes a private key as a parameter, while that should come from the ctor (stored in private member attribute).

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 ..

oberstet avatar Apr 29 '19 04:04 oberstet