Skip to content
This repository has been archived by the owner on Feb 6, 2021. It is now read-only.

Kalium 1.0.0? #48

Closed
wants to merge 24 commits into from
Closed

Kalium 1.0.0? #48

wants to merge 24 commits into from

Conversation

bendoerr
Copy link
Contributor

I started off with the goal of adding all of the currently documented Libsodium interfaces to Kalium. I may have gotten a bit carried away cleaning up code though. Anyway this is my progress so far, no need to merge yet.

Goals

No code changes, just moved declarations around and added comments to
assist as the number of declarations begin to grow.
I cannot think of a reason to not do this at a minimum.
Java arrays are always indexed by ints and thus limited to Integer.MAX
elements, in theory. I was worried that the array length values were
defined as longs for a performance reason with jnr-ffi but in testing I
couldn't see any difference.

It is better to define them as ints here to be consistent with the Java
language, I think.
 * Add the full gambit of constants for crypto_secretbox
 * Deprecated the two constants that were not "CRYPTO" prefixed
 * Add the easy and detached crypto_secretbox APIs
 * Update SecretBox to use the easy API
 * Add SecretBox.encryptDetached and SecretBox.decryptDetached
 * Add ConstantTest to verify constants against exposed libsodium values
 * Deprecate the APIs that use the primitive names in favor of the
   documented names
 * Add tests against the constant values
 * Update AuthenticationKey to use the documented APIs
There are quite a few changes here, all related to crypto_box.
 * Added all of the easy, detached, and afternm native interfaces.
 * Deprecated the NaCl crypto_box native interfaces and switched the
   kalium implementations to the libsodium versions.
 * Added a way to generate a KeyPair from a seed.
 * Adds all of the currently documented crypto_sign APIs including the
   detached and ed25519 utility methods
 * Adds all of the crypto_sign constants.
 * Simplify SigningKey and VerifyKey to use the detached API.
 * Adds the documented crypto_generichash
 * Adds all of the crypto_generichash constants
 * Adds the interactive APIs
@abstractj
Copy link
Owner

@bendoerr I'd say that would be better to split documentation and improvements in small PRs, when possible, it's better for visibility. Anyway is not a big deal, it will just take some time for a full review.

Regarding Kalium 1.0.0 or not. Let's leave it as is for now, more testing is required.

@bendoerr
Copy link
Contributor Author

Sure, I'll split this up as much as I can.

I haven't had much time to work on it recently since I added the majority of the functionality that I needed. If I recall, I was blocked in one use case waiting for a bug fix to be merge into jnr-ffi/jffi... which looks like it has been merged and released now, so I'll include that in a future pull-request.

@abstractj
Copy link
Owner

@bendoerr thanks a million Ben

@abstractj
Copy link
Owner

abstractj commented May 14, 2016

@bendoerr here's my suggestion to get it merged. First, I really do appreciate your changes, but would like to do into this way.

1 I saw that you added several interfaces from libsodium and this is cool, at the same time I'd like to see the respective testing vectors for each interface (we can do it together in small steps). Otherwise Kalium would become just a Java library exposing plain C, the goal here is make it easy for people willing to consume crypto without having to care about what happens under the covers with libsodium.

Let's move to another PR the following commits please:

  • Add SealedBox
  • Update/Add crypto_secretbox APIs
  • Update/Add crypto_auth APIs
  • Add crypto_aead_chacha20poly1305
  • Update/Add crypto_box APIs
  • Update/Adds crypto_sign APIs
  • Add/Update crypto_generichash APIs
  • Add crypto_shorthash API
  • Add AEAD AES256-GCM support
  • Add back original pwhash_scryptsalsa208sha256 constants
  • Add crypto_hash_sha[256,512] multi-part interfaces
  • Add all of the crypto_auth_* interfaces.
  • Add crypto_onetimeauth interfaces.
  • Add key agreement over X25519
  • Add the crypto_stream_* interfaces
  • Add ed25519 to curve25519 interfaces

2 Let's get only the interfaces that requires an update and put in a single PR. Because from what I noticed we don't have the testing vectors for the new methods.

  • Update crypto_secretbox APIs
  • Update crypto_auth APIs
  • Update crypto_box APIs
  • Update crypto_sign APIs
  • Update crypto_generichash APIs

3 Let's add the remaining interfaces with testing vectors

  • Add SealedBox
  • Add crypto_aead_chacha20poly1305
  • Add/Update crypto_generichash APIs
  • Add crypto_shorthash API
  • Add AEAD AES256-GCM support
  • Add back original pwhash_scryptsalsa208sha256 constants
  • Add crypto_hash_sha[256,512] multi-part interfaces
  • Add all of the crypto_auth_* interfaces.
  • Add crypto_onetimeauth interfaces.
  • Add key agreement over X25519
  • Add the crypto_stream_* interfaces
  • Add ed25519 to curve25519 interfaces

Sorry if it sounds frustrating, I could just cherry-pick your commits, but I really would like to preserve what you did. If all that I said make some sense to you, let's move forward.

@bendoerr
Copy link
Contributor Author

@abstractj not frustrating at all. Your suggestion sounds like a good approach and I appreciate the guidance. I'll get new PR's up later today.

@bendoerr bendoerr mentioned this pull request May 18, 2016
@bendoerr
Copy link
Contributor Author

bendoerr commented May 18, 2016

@abstractj How do you feel about 733d3e6? I think it would resolve #37 and keep things more clear for those of us that are familiar with libsodium. Edit. See PR #58.

@bendoerr
Copy link
Contributor Author

@abstractj I've added the two noted PR's let's figure out where to go with those, then I'll start popping up the rest.

@abstractj
Copy link
Owner

I'm closing it, because I believe this PR is out of date.

@abstractj abstractj closed this Jul 11, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants