Add Wycheproof test vectors

[nickray]

https://github.com/google/wycheproof/blob/master/testvectors/eddsa_test.json

like https://github.com/RustCrypto/elliptic-curves/issues/232

[enrikb]

Hi, I started playing with serde and macros. This is the current status: https://github.com/enrikb/salty/tree/feature/project_wycheproof_eddsa There is still a lot of room for improvements, of course.

$ cargo run --release
    Finished release [optimized] target(s) in 0.03s
     Running `qemu-system-arm -cpu cortex-m33 -machine musca-b1 -nographic -semihosting-config enable=on,target=native -s -kernel target/thumbv7em-none-eabi/release/wycheproof`
running tests..................................................................
tc063 FAIL (expected INVALID, but isn't).
tc064 FAIL (expected INVALID, but isn't).
tc065 FAIL (expected INVALID, but isn't).
tc066 FAIL (expected INVALID, but isn't)...............................................................................
done.

[nickray]

Very cool! I'd be happy to collaborate on merging this (and fixing the bugs it finds).

Wondering if wycheproof/eddsa_test.json should be in the repo itself, or fetched by some command?

[enrikb]

Thanks for the feedback! I have reworked the stuff a little bit and besides other things, the JSON file will be downloaded from the Project Wycheproof repository. (Just master as of now.) Currently, this is realized in the Makefile. Maybe there's a better way. The tests are now integrated both as proper Rust tests running on the build host (make test in salty) and as qemu/on-target tests (make test in qemu-tests). I'm aware that these tests are not fully equivalent due to the different code base, but sometimes a test on the build host can be handy, as well. The four failing tests fail on the host, too. I think these are malleable signatures that might fail with TweetNaCl, too. But I haven't checked in detail, so far. BTW, all tests pass using ed25519-dalek's verify_strict. Please let me know if I should make a pull request from this stuff, or if I should e.g. handle the additional crates separately from this repo. Any suggestions welcome.

[nickray]

In case you're still interested in this @enrikb, I've started to add X25519 as well (to enable a native recipient for age, an eventual WireGuard interface, and some other use cases). So there's even more things to test now :)

[nickray]

@enrikb this is really nice work! I took the liberty of creating a PR myself https://github.com/ycrypto/salty/pull/11, with disposition to merge. Very much agree that testing on host is useful, the difference in implementation should just be the elliptic base field, with most logic shared, so the HIL test should run after the host test, and then "just" checks there's no issue in swapping out the base field implementation. I think it's fine to have the wycheproof* libraries locally, it would only make sense to pull them out if they're somehow usable by other libraries as well (and perhaps offer All The Wycheproof Tests...). It's just unfortunate there seem to be no actual signature correctness tests? Just signature verification?

I'm going to try and add the RFC 7758/8032 tests and the Wycheproof X25519 tests.

[enrikb]

@nickray, you're too fast for me, are you still on adrenaline_v2? ;-) It's absolutely fine for me if you work on this, too. I planned to have a look at the stuff again maybe over the weekend.

[nickray]

I'm sorry :) Indeed excited hehe. I have to change the API a little to accomodate both Ed255 and X255, so if I merge your status quo now, you can continue to build on that more easily.

[nickray]

Regarding malleability of signatures that Wycheproof Ed25519 tests complain about: https://docs.rs/ed25519-dalek/1.0.1/ed25519_dalek/struct.PublicKey.html#method.verify_strict. I'm not too hot about adding malleability checks by default, as it's not clear to me what security this actually provides (and in many situations, speed is a competing need, e.g. NFC timeout - though we should measure, not speculate, cf. https://github.com/ycrypto/salty/issues/10), besides satisfying "strict testing" requirements of certain protocols. (Note this refers to verification, for signature generation, we should absolutely reduce, cf. https://github.com/ycrypto/salty/issues/3).

[nickray]

I'd like to do a new release. The problem is that the wycheproof* dependency libraries aren't published (and probably shouldn't currently be), so salty can't be published. But without them cargo test does not work.

[enrikb]

As far as I can see, everything is available in the salty repository. Should we maybe rename the wycheproof* stuff for a release to stay out of a potential 'official' wycheproof namespace from the salty side? (Not sure if this is your main concern.)

[nickray]

The thing is if you publish a library on crates.io, all its dependencies need to be published as well (no git or in-repo dependencies). I guess I could publish the two as salty-wycheproof*? Not sure about the long-term testing strategy, so the concern is just claiming more names on crates.io that we may later abandon. The goal is clear: a new release :)

[nickray]

Not sure why this doesn't fix it actually: https://github.com/rust-lang/cargo/pull/7333

[enrikb]

The goal is clear: a new release :)

If there is no more decent workaround, you could make a release-branch having the wycheproof stuff disabled for the release - or the other way round, move it to a feature branch, if the release should be from main. I'm fine with all solutions that unblock the release.

[nickray]

Alright, did the release branch thing and published 0.1.0-alpha.2 as first release with X25519. UPDATE: https://github.com/ycrypto/salty/pull/13

[enrikb]

Meanwhile, made some progress with the X25519 test integration. Will have to do some cleanup, but expect an update soon. When using the agreement functions, I have found two issues, so far:

1. I can't access the contents of agreement::SharedSecret() from outside. Am I missing something or is SharedSecret() missing something like to_bytes / as_bytes? (I added a to_bytes method to be able to test the testing.)
2. The implementation of fn from(bytes: [u8; 32]) -> PublicKey in agreement.rs can panic. I suppose this should not be the case, but I'm still a Rust beginner... (I'm currently using panic::catch_unwind as workaround.)

Ah, nearly forgot the good news: all tests from x25519_test.json pass, given above workarounds.

[nickray]

Thanks! Yeah we shouldn't panic (fallible try_from) and I've noticed API holes too. Feel free to adjust what you need in your PR (for 1. AsRef or AsSlice might be useful); we could also stay closer to dalek in some places.