Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace libsodium with cryptology #558

Merged
merged 22 commits into from
Mar 25, 2024

Conversation

muzzammilshahid
Copy link
Contributor

@muzzammilshahid muzzammilshahid commented Feb 14, 2024

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.

Copy link
Contributor

@om26er om26er left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great and allows us to remove dependency on libsodium-jni in WAMP code at least. The XBR code still depends on libsodium-jni and quick search suggests we should be able to write a wrapper around bouncycastle to implement SecretBox as well: https://github.com/codahale/xsalsa20poly1305/blob/master/src/main/java/com/codahale/xsalsa20poly1305/SecretBox.java

@oberstet
Copy link
Contributor

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:

  • we need to make sure it continues to work .. I mean, the functionality in ABJ that actually uses libsodium stuff .. and I guess we sadly do not have much tests or even CI coverage that would allow use to have confidence in replacing libsodium
  • my other worry is: yeah, as @om26er noted, WAMP-cryptosign is one use (an important one of course), but the other indeed seems to be "XBR", or actually the experimental WAMP AP mechanisms for E2E encryption!
  • and the latter is also something I would really want to keep - I might have a chance to work on WAMP E2E, and if so, then I want to make sure "it works" on at least ABPy, ABJS, ABJ and ideally ABCPP
(.venv) oberstet@intel-nuci7:~/scm/crossbario/autobahn-java$ git log -n1
commit 23673d3dbcff869a89b2171f843b28a6306ac2f7 (HEAD -> master, origin/master, origin/HEAD)
Author: Muzzammil Shahid <[email protected]>
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
Copy link
Contributor

om26er commented Feb 15, 2024

@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
Copy link
Contributor

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
Copy link
Contributor

oberstet commented Feb 15, 2024

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
Copy link
Contributor

om26er commented Feb 15, 2024

@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
Copy link
Contributor

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
Copy link
Contributor

oberstet commented Feb 16, 2024

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

@muzzammilshahid muzzammilshahid marked this pull request as draft February 19, 2024 10:42
Change SealedBox implementation to use bouncycastle
@om26er
Copy link
Contributor

om26er commented Feb 26, 2024

@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 muzzammilshahid changed the title Use bouncycastle in Cryptosign Auth Replace libsodium with bouncycastle Feb 26, 2024
@@ -27,17 +27,15 @@ dependencies {
implementation 'org.web3j:core:5.0.0'
implementation 'org.web3j:abi:5.0.0'
implementation 'org.web3j:utils:5.0.0'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be changed to 4.11.0 as that's the latest version instead of 5.0.0 https://ethereum.stackexchange.com/questions/156931/is-web3j-version-5-0-0-discontinued

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing that also brings in the version of bouncycastle that supports Kyber and Dilithium

@muzzammilshahid
Copy link
Contributor Author

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
Copy link
Contributor

oberstet commented Feb 26, 2024

@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
Copy link
Contributor

om26er commented Feb 26, 2024

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
Copy link
Contributor

oberstet commented Feb 26, 2024

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:

  • first of all, just as "background": I do care about appropriate and precise handling of legal aspects and OSS a lot;)
  • copy-pasting any amount of code (even a single line) into AB/CB - if done at all (it should be generally avoided) - requires careful handling and must be a coordinated/communicated/consensual thing

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" ? ;)

...

@muzzammilshahid muzzammilshahid changed the title Replace libsodium with bouncycastle Replace libsodium with cryptology-java Mar 6, 2024
@muzzammilshahid muzzammilshahid changed the title Replace libsodium with cryptology-java Replace libsodium with cryptology Mar 6, 2024
@muzzammilshahid muzzammilshahid marked this pull request as ready for review March 6, 2024 14:23
@oberstet
Copy link
Contributor

oberstet commented Mar 7, 2024

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
Copy link
Contributor

om26er commented Mar 20, 2024

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
Copy link
Contributor

om26er commented Mar 25, 2024

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

@oberstet oberstet merged commit 9236159 into crossbario:master Mar 25, 2024
1 check passed
@oberstet
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants