test(noise): replace libsodium-sys-stable with pure-Rust dryoc

[thomaseizinger]

Description

We are currently relying on libsodium-sys-stable for some testing of the libp2p-noise crypto. That library has a build-script which downloads the latest libsodium-sys library. Build-scripts slow down the compilation process and having them perform network calls is not great either.

dryoc implements a libsodium compatible API in pure Rust and is recommended here: https://github.com/The-DevX-Initiative/RCIG_Coordination_Repo/blob/main/Awesome_Rust_Cryptography.md#collections-of-cryptographic-primitives

This also removes two unsafe blocks because we no longer need to perform FFI.

Notes

@pinkforest You contributed the change to libsodium-sys-stable previously. Any feedback on why this wouldn't be a good idea?

Links to any relevant issues

Open Questions

Change checklist

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• A changelog entry has been made in the appropriate crates

[pinkforest]

So the tests were about testing that some conversion works.

This is why I chose whole different implementations and between C vs Rust.

dryoc also is only compatible but is not really the libsodium as the point was to test against libsodium as reference

sodiumoxide also previously used called the libsodium C -

Like you might find the callpath from dryoc goes to the same stuff that is being tested against.

They both use sha2 crate SHA-512 and dalek's CompressedEdwardsY under the hood.

Like if you look at dryoc eventually you find what is it essentially being tested with:

use curve25519_dalek::edwards::{CompressedEdwardsY, EdwardsPoint};
use sha2::{Digest as DigestImpl, Sha512 as Sha512Impl};

Related to functions you're re-wiring to in dryoc:

crypto_sign_ed25519_sk_to_curve25519

// Converts an Ed25519 secret key `ed25519_secret_key` into an X25519 secret                                                                                                                                          
/// key key, placing the result into `x25519_secret_key`.                                                                                                                                                             
///                                                                                                                                                                                                                   
/// Compatible with libsodium's `crypto_sign_ed25519_sk_to_curve25519`                                                                                                                                                
pub fn crypto_sign_ed25519_sk_to_curve25519(
    x25519_secret_key: &mut [u8; CRYPTO_SCALARMULT_CURVE25519_BYTES],
    ed25519_secret_key: &SecretKey,
) {
    let hash: [u8; CRYPTO_HASH_SHA512_BYTES] = Sha512::compute(&ed25519_secret_key[..32]);
    let mut scalar = clamp_hash(hash);
    x25519_secret_key.copy_from_slice(&scalar);
    scalar.zeroize()
}

crypto_sign_ed25519_pk_to_curve25519

//                                                                                                                                                                                                                    
/// Compatible with libsodium's `crypto_sign_ed25519_pk_to_curve25519`                                                                                                                                                
pub fn crypto_sign_ed25519_pk_to_curve25519(
    x25519_public_key: &mut [u8; CRYPTO_SCALARMULT_CURVE25519_BYTES],
    ed25519_public_key: &PublicKey,
) -> Result<(), Error> {
    let ep = CompressedEdwardsY(*ed25519_public_key)
        .decompress()
        .ok_or_else(|| dryoc_error!("failed to convert to Edwards point"))?;
    x25519_public_key.copy_from_slice(ep.to_montgomery().as_bytes());

    Ok(())
}

Then if we look at what is being tested against at transports/noise/src/protocol/x25519.rs

use sha2::{Digest, Sha512};
use curve25519_dalek::edwards::CompressedEdwardsY;

If the need is to avoid the C-library as whole - then could use some mininifcation strategies.

Then could just c2rust the bits needed from libsodium instead of bringing the whole library and turn it into Rust crate.

This was done in anger with with some fiat crypto via ring-xous fork of ring compatible with rustls 0.16.20 and being pure Rust meaning it will work a joy in wasm.
https://www.crowdsupply.com/sutajio-kosagi/precursor/updates/bringing-up-tls-on-precursor

If you want I could work on doing that - but I would not replace with dryoc if the aim is to integration test the impls.

[thomaseizinger]

Ah I should have looked into that in more detail. That obviously doesn't make sense then. Thanks for the good explanation!

I wonder if we need these tests in the first place. This isn't our code so it is not really our responsibility to test this. @libp2p/rust-libp2p-maintainers What do you think? I think removing these tests bears little risk.

[thomaseizinger]

Closing for now. Might re-open a new PR to remove the test!

[pinkforest]

It is a relatively standard practice to test crypto like this given the context and where it is intended for.

I can do a direct-rustified-minified libsodium for testing purposes that contain those two fn's,

So it would be just a crate instead of pulling a C library - testing integration purposes it's fine.

[thomaseizinger]

It is a relatively standard practice to test crypto like this given the context and where it is intended for.

Oh, I absolutely understand that! The reason I am questioning it is because we are just users of ed25519_dalek and x25519_dalek. We don't actually implement any of the cryptography ourselves.

Is every user expected to test that ed25519_dalek and x25519_dalek behave correctly? It just seems out of place and something that should happen upstream.

[thomaseizinger]

I just saw that we do implement a bit of crypto here. Let's see if we can get rid of that ...

[thomaseizinger]

All of this code is only used within the non-libp2p spec compliant version of the noise protocol. I am not sure if anyone is using that but I am tempted to remove it because it is not interoperable with any other libp2p implementation.

[thomaseizinger]

All of this code is only used within the non-libp2p spec compliant version of the noise protocol. I am not sure if anyone is using that but I am tempted to remove it because it is not interoperable with any other libp2p implementation.

I opened a PR here: #3227