crossbario / autobahn-java

WebSocket & WAMP in Java for Android and Java 8
https://crossbar.io/autobahn
MIT License
1.52k stars 427 forks source link

Replace libsodium with cryptology #558

Closed muzzammilshahid closed 3 months ago

muzzammilshahid commented 5 months ago

Use cryptology in SecretBox, SealedBoxand for signing challenge and generating public,private keypair in CryptoSignAuth.

Note: Using libsodium, cryptosign authentication was not working in non-android environments. Now, it will work fine.

oberstet commented 5 months ago

yes, removing the JNI stuff would indeed be great! in general, and also when that allows to use it on non-android environments!

I have only 2 worries:

(.venv) oberstet@intel-nuci7:~/scm/crossbario/autobahn-java$ git log -n1
commit 23673d3dbcff869a89b2171f843b28a6306ac2f7 (HEAD -> master, origin/master, origin/HEAD)
Author: Muzzammil Shahid <muzzammil@xconn.io>
Date:   Wed Feb 14 07:28:11 2024 +0500

    update dependencies to latest version (#557)
(.venv) oberstet@intel-nuci7:~/scm/crossbario/autobahn-java$ find . -name "*.java" -exec grep -Hi "libsodium" {} \;
./autobahn/src/main/java/io/crossbar/autobahn/wamp/auth/CryptosignAuth.java:import org.libsodium.jni.crypto.Random;
./autobahn/src/main/java/io/crossbar/autobahn/wamp/auth/CryptosignAuth.java:import org.libsodium.jni.keys.SigningKey;
./autobahn/src/main/java/io/crossbar/autobahn/wamp/auth/CryptosignAuth.java:import org.libsodium.jni.keys.VerifyKey;
./autobahn/src/main/java/io/crossbar/autobahn/wamp/auth/CryptosignAuth.java:import static org.libsodium.jni.SodiumConstants.SECRETKEY_BYTES;
./autobahn/src/main/java/xbr/network/SimpleSeller.java:import org.libsodium.jni.crypto.Random;
./autobahn/src/main/java/xbr/network/crypto/SealedBox.java:import org.libsodium.jni.encoders.Encoder;
./autobahn/src/main/java/xbr/network/crypto/SealedBox.java:import static org.libsodium.jni.NaCl.sodium;
./autobahn/src/main/java/xbr/network/crypto/SealedBox.java:import static org.libsodium.jni.SodiumConstants.PUBLICKEY_BYTES;
./autobahn/src/main/java/xbr/network/crypto/SealedBox.java:import static org.libsodium.jni.crypto.Util.isValid;
./autobahn/src/main/java/xbr/network/crypto/SecretBox.java:import org.libsodium.jni.crypto.Random;
./autobahn/src/main/java/xbr/network/crypto/SecretBox.java:import org.libsodium.jni.crypto.Util;
./autobahn/src/main/java/xbr/network/crypto/SecretBox.java:import org.libsodium.jni.encoders.Encoder;
./autobahn/src/main/java/xbr/network/crypto/SecretBox.java:import static org.libsodium.jni.NaCl.sodium;
./autobahn/src/main/java/xbr/network/crypto/SecretBox.java:import static org.libsodium.jni.SodiumConstants.BOXZERO_BYTES;
./autobahn/src/main/java/xbr/network/crypto/SecretBox.java:import static org.libsodium.jni.SodiumConstants.XSALSA20_POLY1305_SECRETBOX_KEYBYTES;
./autobahn/src/main/java/xbr/network/crypto/SecretBox.java:import static org.libsodium.jni.SodiumConstants.XSALSA20_POLY1305_SECRETBOX_NONCEBYTES;
./autobahn/src/main/java/xbr/network/crypto/SecretBox.java:import static org.libsodium.jni.SodiumConstants.ZERO_BYTES;
./autobahn/src/main/java/xbr/network/crypto/SecretBox.java:import static org.libsodium.jni.crypto.Util.checkLength;
./autobahn/src/main/java/xbr/network/crypto/SecretBox.java:import static org.libsodium.jni.crypto.Util.isValid;
./autobahn/src/main/java/xbr/network/crypto/SecretBox.java:import static org.libsodium.jni.crypto.Util.removeZeros;
./autobahn/src/main/java/xbr/network/crypto/SecretBox.java:        byte[] msg = org.libsodium.jni.crypto.Util.prependZeros(ZERO_BYTES, message);
./autobahn/src/main/java/xbr/network/crypto/SecretBox.java:        byte[] ct = org.libsodium.jni.crypto.Util.zeros(msg.length);
./autobahn/src/main/java/xbr/network/crypto/SecretBox.java:        byte[] ct = org.libsodium.jni.crypto.Util.prependZeros(BOXZERO_BYTES, ciphertext);
./autobahn/src/main/java/xbr/network/KeySeries.java:import org.libsodium.jni.SodiumConstants;
./autobahn/src/main/java/xbr/network/KeySeries.java:import org.libsodium.jni.crypto.Random;
./autobahn/src/main/java/xbr/network/SimpleBuyer.java:import org.libsodium.jni.SodiumConstants;
./autobahn/src/main/java/xbr/network/SimpleBuyer.java:import static org.libsodium.jni.NaCl.sodium;
(.venv) oberstet@intel-nuci7:~/scm/crossbario/autobahn-java$ 
om26er commented 5 months ago

@muzzammilshahid we need to add automated tests. We should start with just adding that for CryptoSign auth, please do that in this PR, then in follow up PRs we want to test WAMPCRA and Ticket as well.

oberstet commented 5 months ago

that would be great! absolutely agreed.

fwiw, couple of comments from sideline, just what spring to my mind:

governance

changing an important dependency, and one that our crypto stuff is using in particular, is always a critical decision, one that deserves some deeper look. one important question: do we trust whoever produces that dependency? the developers and org behind. even more so since yes, it is OSS, but who has the time & know-how to read all that code?

I am not deep into Java communities these days, but I had a quick look at https://www.bouncycastle.org/about.html, and for me this "looks good & trustworthy".

the other important Q is OSS license: the license of the new dependency must be "liberal OSS" ... eg we couldn't depend on a strict GPL'ed one as this would run contrary to the liberal license of ABJ

scope

long ago, after reading some stuff by DJB, I came to the conclusion that this all makes a lot of sense, and that I trust it - I now mean the math & algorithmic level.

this was the motivation to base WAMP-cryptosign on atoms/elements of the former, and not only WAMP-cryptosign, but also the WAMP E2E capability, and also the multi-operator / federated capabilities (what was called "XBR").

from a dependency PoV, I've been using DJBs code in form of his "code snippets", then moved to NaCl, then to libsodium, also strongly driven because of Python and C++. and I tried a couple of other options, e.g. Botan https://botan-crypto.org/, which is also nice, but hey, you need to pick your weapons;)

in general, I'm still happy with libsodium, it's a very capable, easy to use, polished & maintained and pretty/proper library. however, I'm not stuck on it. specifically, since I'm really not "up to date" with Java community / dev trends and such. so I'm fine with moving to bouncycastle, I trust you guys.

as mentioned before, the only thing I really would want to stress is, WAMP-cryptosign is one app, the other is E2E and generally all current uses of libsodium should be migrated to bountycastle, and realistically, we have to do it now (within the move) because if we'd postpone migration of those other bits, it'll be done "never";) and yes, not all of E2E is "done/final", but there is a working core / pieces, and we want to keep that, and build on it, not have it swept under the rug

test coverage

the WAMP spec has a pretty good definition / description of WAMP-cryptosign

https://wamp-proto.org/wamp_latest_ietf.html#name-cryptosign-based-authentica

this includes test vectors!

these could be used for unit tests.

now, for integration testing, the best / most polished bits I'm aware of are here:

https://github.com/crossbario/crossbar-examples/tree/master/authentication

this is working driven from makefiles, I never had the time to fully make it CI / lights-out in Crossbar.io or in any of the Autobahns, but it covers all the flavors and is pretty comprehensive

for example, it also covers WAMP-cryptosign with TLS channel binding (that is when running TLS) - which is sth people slowly recognize as important (importance channel binding for any authentication)

oberstet commented 5 months ago

rgd unit tests, another good source is ABPy:

https://github.com/crossbario/autobahn-python/blob/master/autobahn/wamp/test/test_wamp_cryptosign.py

also in general, unfiltered, but still on point:

find autobahn-python/autobahn -name "*.py" -exec grep -Hi "cryptosign" {} \;
find crossbar -name "*.py" -exec grep -Hi "cryptosign" {} \;
find crossbar-examples -name "*.py" -exec grep -Hi "cryptosign" {} \;

rgd E2E ("cryptobox" .. also using libsodium) the situation sadly is worse ... fwiw, I still have the sovereigntechfund application running.

should it be granted, I will have time to really polish all of it! in the spec, and in ABPy and CB and so on.

and not only E2E, but also how clustering and federation specifically works together with E2E.

think of: Alice wants to call a procedure provided by Bob, and both Alice and Bob are end-users (WAMP clients) which connect to router by different ops, e.g. Fred and Carol.

so there are 4 parties, and Alice wants to trust Bob, but neither trusts Fred nor Carol other than for "availability" of the routing service

anyways, would be great, let's see what happens;)

om26er commented 5 months ago

@oberstet regarding bouncycastle, yes it looks like many "famous" projects use that. It is already an indirect dependency in autobahn-java because web3j makes use of that for its encryption stuff.

We might pause this PR and add basic test infrastructure first, then changing libs should be simpler. And I agree we should aim for the removal of libsodium-jni in a single PR.

oberstet commented 5 months ago

fantastic, absolutely agreed!

It is already an indirect dependency in autobahn-java because web3j makes use of that for its encryption stuff.

ah right! I forgot;)

luckily, the WAMP spec refers all important primitives needed for WAMP-cryptosign and E2E already!


grafik


yes, "keccak256" .. why on earth .. it's just what Ethereum (and others) decided on long ago ... finally, my EIP665 to add curve25519 to Ethereum went nowhere:( and Ethereum-WASM is still "W.I.P." as far as I know. Anyways, just complaining.

oberstet commented 5 months ago

one more comment rgd changing underlying crypto utils dep: besides getting rid of JNI, the other important motivation / factor we should require IMO is: post quantum crypto

concretely, does bountycastle support Dilithium+Kyber?


currently, this isn't supported in any AB* or in CB, or in fact in WAMP spec level, but if I at least take up work again on those, then this will be one of the first areas to work on, and ABPy / CB will then get Dilithium+Kyber support ... and then ABJ could profit from having a crypto utils lib already that makes this easy to add there as well

om26er commented 4 months ago

@oberstet Bouncycastle does Indeed support Dillithium and Kyber https://www.bouncycastle.org/releasenotes.html. This PR is now updated to fully supported SealedBox and SecretBox using bouncycastle.

We right now ship HSalsa20.java from another project. It needs to be changed to be based on bouncycastle as well.

muzzammilshahid commented 4 months ago

HSalsa20 implementation based on bouncy castle is taken from https://github.com/codahale/xsalsa20poly1305/blob/master/src/main/java/com/codahale/xsalsa20poly1305/HSalsa20.java

oberstet commented 4 months ago

@muzzammilshahid great progress!! thanks a lot, this is really cool, your contributions are very welcome=) @om26er thanks for reviewing/guidance and watching over! this is of course as important/welcome as well=)


just one quick note, I haven't looked in detail (Omer is much better than me with that, I trust you guys ..): rgd copy-pasting code from somewhere else into this repo: that isn't a problem in itself, we just need to handle OSS/license/legal aspects properly.

so in general, the approach with ABJ (and the other Autobahn's) is:

  1. adding an external dependency to the library (eg another library or what) requires the dep to have an OSS license compatible with the liberal MIT license of Autobahn, and the dep and its license should be "documented", eg in ABPy https://github.com/crossbario/autobahn-python/blob/359f868f9db410586cf01c071220994d8d7f165a/setup.py#L91.
  2. for Crossbar.io, the same applies, but in an even stricter form (!), as the Crossbar.io build process produces a complete dependency file including licenses https://github.com/crossbario/crossbar/blob/master/LICENSES-OSS, and the Crossbar.io executable / CLI allows easy access to that as well!
  3. ultimately, a "fully verified repeatable (to the bit)" build would be great, but it is complicated with Python .. or "only" very hard with C++ ... but it would be possible with Rust
  4. when copy-pasting code directly into one of AB's or CB, for the legal aspects see above, but then it is not an external dependency, but literal code in our repos, and then we need to make sure the license headers for the respective files (and only for those files that were copied / pasted into) properly reflect both the original author as well as us
  5. PLUS: if the license of copy-pasted code is MIT itself, then "the" license of ABJ remains MIT (purely) as well, so np at all!
  6. FINALLY: if the license of the copy-pasted code is "only compatible" with the liberal MIT license (but a different license nevertheless), then "the" license of ABJ itself is no longer only MIT, but a mix. which is a medium-level worry/annoyment .. not sure how to best handle it. comments welcome.
om26er commented 4 months ago

The copied file is Apache 2. If mixing the license is a worry, we could create a new repo under (https://github.com/xconnio), implement the new code there (SealedBox, SecretBox etc) and import that in this project. Doing that with a proper name might make it more visible for other people to use this code as well.

Thoughts?

oberstet commented 4 months ago

oh yes, haven't thought about that option! this is definitely sth to consider!

couple of thoughts. those do not imply a specific path/option we chose, just general thoughts:

I would distinguish these cases:

  1. it is only about a "small" amount of code or only a portion copied out from a single source file
  2. it is only copy-pasting a single or some complete files from a source repo "as is"
  3. it is copy-pasting (almost) all of the files from a source repo (or a distinguished submodule within that source repo)
  4. it is copy-pasting the complete source repo or tree

and then consider:

rgd "4.": this should be avoided almost always. as long as "the source repo" still has "some hope", or "some non-code content" such as issues/discussion/... we should contribute upstream. if unavoidable, kindly ask to "take over" and transfer the repo. e.g. I did this with Scour (the original author explicitly didn't want to maintain the code nor the repo .. and the ownership transfer was what he though himself as "best")

rgd "3.": sometimes it will be better to follow this, eg when the source repo transfer would still suck because no CI or no issues to keep alive and such exists, and the project would make sense to have/offer in a separate, reusable form for others to use as well! I think this would be a good case for what @om26er hinted at. OSS is about "reuse" ... and being good OSS citizens we should not only consider our own immediate needs, but others as well (to a degree .. we can't "fix the world" just because others would welcome that ... I'd welcome that too! but where do I file my bug "world is broken" ? ;)

...

oberstet commented 4 months ago

fantastic! fwiw, just from a casual look, the code/changes look quite good=)

just noting 2 minor aspects that popped to my mind while looking at the code .. nothing important, just noting so it's not lost:

  1. sourceCompatibility = JavaVersion.VERSION_17 : I have no clue what's the "latest/current/reasonable" choice, but fwiw, my own personal opinion is: people should run "the latest" Android / Java or upgrade, so I tend to not be overly bothered / slowed down by wanting/having to support old run-times or what. of course we need to be reasonable ... but bumping this in general is totally fine for me;)
  2. I noticed you added a bunch of new unit tests for the crypto touched! this is absolutely great, love it. you could add links in comments to where the test vectors in those tests originally come from (eg the spec section) and/or where these test vectors can also be found (eg ABPy) ... this is helping other developers .. also our self in the future (at least me, I tend to forget things) ... cross-referencing is a good thing;)
om26er commented 4 months ago

sourceCompatibility = JavaVersion.VERSION_17 is only changed for non-android environment which is fine because web3j dependency bump requires that.

The automated tests as basic to verify we can sign and verify the challenges. I think once we land this work, we should be able to iterate on things real quick.

We have progressive call results coming in as well

om26er commented 3 months ago

@oberstet I can't land the PR. Can you please merge this?

oberstet commented 3 months ago

done. thanks @om26er and of course many thanks / congrats to @muzzammilshahid !