[Bug 2121490] New: Review Request: rust-sodiumoxide - Fast cryptographic library for Rust
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
Bug ID: 2121490 Summary: Review Request: rust-sodiumoxide - Fast cryptographic library for Rust Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: stuart@gathman.org QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://gathman.org/linux/SPECS/rust-sodiumoxide.spec SRPM URL: https://gathman.org/linux/f37/src/rust-sodiumoxide-0.2.7-1.fc37.src.rpm Description: Fast cryptographic library for Rust (bindings to libsodium). Fedora Account System Username: sdgathman
While this was built on by dev system, it is still waiting for dependencies. I am adding the review request now to document the holdup for Cjdns.
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
Stuart D Gathman stuart@gathman.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Depends On| |1991164, 1981119 | |(rust-ed25519), 2117954 Doc Type|--- |If docs needed, set a value
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=1981119 [Bug 1981119] Review Request: rust-ed25519 - Edwards Digital Signature Algorithm (EdDSA) over Curve25519 https://bugzilla.redhat.com/show_bug.cgi?id=1991164 [Bug 1991164] Review Request: rust-libsodium-sys - FFI binding to libsodium https://bugzilla.redhat.com/show_bug.cgi?id=2117954 [Bug 2117954] Review Request: rust-signature - Traits for cryptographic signature algorithms
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
Miro Hrončok mhroncok@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |2045255
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=2045255 [Bug 2045255] cjdns: FTBFS in Fedora rawhide/f36
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
--- Comment #1 from Stuart D Gathman stuart@gathman.org --- Spec URL: https://gathman.org/linux/SPECS/rust-sodiumoxide.spec SRPM URL: https://gathman.org/linux/f37/src/rust-sodiumoxide-0.2.7-2.fc37.src.rpm
One of the tests fails - a missing compile dependency:
failures: src/newtype_macros.rs - newtype_macros::new_type (line 175) src/newtype_macros.rs - newtype_macros::new_type (line 184) src/newtype_macros.rs - newtype_macros::new_type (line 194)
The newtype macros is not in scope. I don't know enough rust to debug. Since all the other tests pass, I defaulted to skip tests.
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
--- Comment #2 from Stuart D Gathman stuart@gathman.org --- Added --skip to %cargo_test
%cargo_test -- -- --skip newtype_macros::new_type
test result: ok. 21 passed; 0 failed; 0 ignored; 0 measured; 3 filtered out; finished in 2.47s
error: test failed, to rerun pass '--lib'
Caused by: process didn't exit successfully: `/builddir/build/BUILD/sodiumoxide-0.2.7/target/release/deps/sodiumoxide-730831ba5980a0f8 --skip 'newtype_macros::new_type'` (signal: 4, SIGILL: illegal instruction)
Seems like a bug in %cargo_test macro?
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
Fabio Valentini decathorpe@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |decathorpe@gmail.com
--- Comment #3 from Fabio Valentini decathorpe@gmail.com ---
Seems like a bug in %cargo_test macro?
Why would a crashing test be a bug in the %cargo_test macro?
The test runner is killed with "SIGILL" if the code of the test itself can't be executed. Recent versions of Rust / LLVM will emit "ud2" instructions on x86_64 if they encounter undefined behaviour. So I'd look at the code for the test that crashes the test runner to see how UB can happen.
https://bugzilla.redhat.com/show_bug.cgi?id=2121490 Bug 2121490 depends on bug 1991164, which changed state.
Bug 1991164 Summary: Review Request: rust-libsodium-sys - FFI binding to libsodium https://bugzilla.redhat.com/show_bug.cgi?id=1991164
What |Removed |Added ---------------------------------------------------------------------------- Status|POST |CLOSED Resolution|--- |NOTABUG
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
--- Comment #4 from Stuart D Gathman stuart@gathman.org --- It already printed the summary. All tests passed and 3 were skipped. How would you know which test got a UD?
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
--- Comment #5 from Fabio Valentini decathorpe@gmail.com ---
It already printed the summary. All tests passed and 3 were skipped. How would you know which test got a UD?
The problem is that if a test crashes instead of reporting failure, it will not be listed, since the test runner can't report back. You can try running tests with "--test-threads 1" to make things run sequentially, and then it should be obvious which test's thread crashed the test runner.
I tried reproducing the failure locally from a source checkout, and the only test (other than the "new_type" doctests failures) is: crypto::sign::ed25519::test::test_sign_verify_detached_tamper
This might or might not be the failure that causes the SIGILL in your case.
I also noticed that the upstream project was officially abandoned last month :( https://github.com/sodiumoxide/sodiumoxide
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
--- Comment #6 from Fabio Valentini decathorpe@gmail.com --- I have debugged the test failure I mentioned above. It appears that the test code calls a deprecated API in the ed25519 crate ("Signature::new"), which no longer accepts arbitrary bytes as input, and panics when given invalid data - which is what this test does.
So you can safely skip that test by using something like
# * skip a test that is not compatible with ed25519 1.3.0+: # the Signature::new method now panics when called with invalid bytes # * skip doctests with missing imports for the new_type! macro %cargo_test -- -- --skip crypto::sign::ed25519::test::test_sign_verify_detached_tamper --skip src/newtype_macros.rs
Please check if you're still seeing the SIGILL crashes when doing that, since I can't reproduce them on my local system (the package builds successfully for me with the two skipped tests).
https://bugzilla.redhat.com/show_bug.cgi?id=2121490 Bug 2121490 depends on bug 2117954, which changed state.
Bug 2117954 Summary: Review Request: rust-signature - Traits for cryptographic signature algorithms https://bugzilla.redhat.com/show_bug.cgi?id=2117954
What |Removed |Added ---------------------------------------------------------------------------- Status|POST |CLOSED Resolution|--- |RAWHIDE
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
--- Comment #7 from Stuart D Gathman stuart@gathman.org --- test result: ok. 21 passed; 0 failed; 0 ignored; 0 measured; 3 filtered out; finished in 2.37s
error: 1 target failed: `--lib`
I wonder what that means...
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
--- Comment #8 from Fabio Valentini decathorpe@gmail.com --- Hard to say without more context (i.e. the whole output of %cargo_test). I assume the actual error is further up, soccer tests are run in this order: Unit tests (--lib), doctests (--doc), and integration tests (--tests). So I assume the test result summary was for doctests, but unit tests further up failed.
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
Fabio Valentini decathorpe@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(stuart@gathman.or | |g)
--- Comment #9 from Fabio Valentini decathorpe@gmail.com --- Please follow up here if you still need this crate (looks like cjdns still needs it?). Note that the upstream project has been marked as deprecated and unmaintained, though, which probably isn't a good sign for a crypto library.
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
Stuart D Gathman stuart@gathman.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(stuart@gathman.or | |g) |
--- Comment #10 from Stuart D Gathman stuart@gathman.org --- Cjdns new version need the crate. I'm not sure what Rust programmers use if not a libsodium wrapper. I'll ask cjdns upstream about the sodiumoxide status.
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
--- Comment #11 from Stuart D Gathman stuart@gathman.org --- I have to resubmit a bunch of the dependencies to get this review finished.
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
--- Comment #12 from Fabio Valentini decathorpe@gmail.com --- I can help with submitting missing dependencies, if that would help you ...
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
--- Comment #13 from Stuart D Gathman stuart@gathman.org --- Yes, please. I just got cjdns-21.1 finally fixed (using distro flags). Caleb upstream is aware of the conflict between wrapping libsodium and rewriting crypto stuff in Rust. The Rust equivalents aren't as fast or complete (yet?). A wrapper only tracks the API, so is more stable.
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
--- Comment #14 from Fabio Valentini decathorpe@gmail.com --- Ok, that would be a package for the "ed25519" crate, and its dependencies? I will work on that as time permits.
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
Fabio Valentini decathorpe@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Depends On| |2193407
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=2193407 [Bug 2193407] Review Request: rust-ed25519 - Edwards Digital Signature Algorithm
https://bugzilla.redhat.com/show_bug.cgi?id=2121490 Bug 2121490 depends on bug 2193407, which changed state.
Bug 2193407 Summary: Review Request: rust-ed25519 - Edwards Digital Signature Algorithm https://bugzilla.redhat.com/show_bug.cgi?id=2193407
What |Removed |Added ---------------------------------------------------------------------------- Status|POST |CLOSED Resolution|--- |RAWHIDE
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
--- Comment #15 from Fabio Valentini decathorpe@gmail.com --- The ed25519 crate is now available from Fedora. Can you refresh this package so I can continue with the review?
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
--- Comment #16 from Stuart D Gathman stuart@gathman.org --- It is failing a new test: test crypto::aead::aes256gcm::aes_api::test::test_seal_open_detached ... FAILED
That is from rust-libsodium_sys
Looking into it
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
--- Comment #17 from Stuart D Gathman stuart@gathman.org --- Skipping the test gets this:
process didn't exit successfully: /builddir/build/BUILD/sodiumoxide-0.2.7/target/release/deps/sodiumoxide-be316f85c0acbe9e --skip 'crypto::sign::ed25519::test::test_sign_verify_detached_tamper' --skip src/newtype_macros.rs --skip 'crypto::aead::aes256gcm::aes_api::test::test_seal_open_detached' (signal: 4, SIGILL: illegal instruction)
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
--- Comment #18 from Fabio Valentini decathorpe@gmail.com --- This means that either the code is invalid and gets simplified down to an "ud2" x86 instruction by LLVM (i.e. "this is invalid"), or you're running code that requires a newer instruction set (like AVX2 or AVX512) on an older CPU that doesn't support it.
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
--- Comment #19 from Fabio Valentini decathorpe@gmail.com --- I tried a local build with
%cargo_test -- -- --skip crypto::sign::ed25519::test::test_sign_verify_detached_tamper --skip src/newtype_macros.rs
And the build succeeded without errors. Is it possible that the CPU feature detection in libsodium is broken and / or the CPU in your development machine is old?
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
Fabio Valentini decathorpe@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(stuart@gathman.or | |g)
--- Comment #20 from Fabio Valentini decathorpe@gmail.com --- Any progress? If you prefer, we could also close this review so somebody else can continue with a fresh one.
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
Stuart D Gathman stuart@gathman.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(stuart@gathman.or | |g) |
--- Comment #21 from Stuart D Gathman stuart@gathman.org --- Still getting the same illegal instruction error as comment 2. I'll try your suggested skip from comment 19, and then a scratch build.
Is this an "old CPU"? All my computers are discards. :-)
Vendor ID: GenuineIntel Model name: Intel(R) Core(TM) i3-2120 CPU @ 3.30GHz
I have some other machines to try a local build on.
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
--- Comment #22 from Stuart D Gathman stuart@gathman.org --- Trying local build with Model name: Intel(R) Core(TM) i5-2520M CPU @ 2.50GHz
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
--- Comment #23 from Fabio Valentini decathorpe@gmail.com --- Yeah those are "pretty old" :) Just a guess, the tests might exercise some code paths of libsodium that use AVX2 extensions. AVX2 has only been implemented in the 4000-series of Intel CPUs and newer, which might explain the problem (but just an educated guess).
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
--- Comment #24 from Stuart D Gathman stuart@gathman.org --- Spec URL: https://gathman.org/linux/SPECS/rust-sodiumoxide.spec SRPM URL: https://gathman.org/linux/SRPMS/rust-sodiumoxide-0.2.7-4.fc38.src.rpm
Builds normally on i5 cpu. I'd call that a bug in libsodium most likely.
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
Fedora Review Service fedora-review-bot@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- URL| |https://crates.io/crates/so | |diumoxide
--- Comment #25 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Copr build: https://copr.fedorainfracloud.org/coprs/build/6225986 (succeeded)
Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...
Please take a look if any issues were found.
--- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service
If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
--- Comment #26 from Fabio Valentini decathorpe@gmail.com --- Thanks for the update, I submitted a scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=104183424
The package seems to build as expected on x86_64 and i686, but it fails on non-x86 architectures (aarch64, ppc64le, s390x) because the code uses x86-specific Rust macros:
error: This macro cannot be used on the current target. You can prevent it from being used in other architectures by guarding it behind a cfg(any(target_arch = "x86", target_arch = "x86_64")). --> src/crypto/aead/aes256gcm.rs:198:17 | 198 | is_x86_feature_detected!("aes") && is_x86_feature_detected!("pclmulqdq"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this error originates in the macro `is_x86_feature_detected` (in Nightly builds, run with -Z macro-backtrace for more info) error: This macro cannot be used on the current target. You can prevent it from being used in other architectures by guarding it behind a cfg(any(target_arch = "x86", target_arch = "x86_64")). --> src/crypto/aead/aes256gcm.rs:198:52 | 198 | is_x86_feature_detected!("aes") && is_x86_feature_detected!("pclmulqdq"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this error originates in the macro `is_x86_feature_detected` (in Nightly builds, run with -Z macro-backtrace for more info)
Since this is only affecting code for a test, it should be simple to fix.
My suggestion would be to patch line 193 of src/crypto/aead/aes256gcm.rs from #[cfg(feature = "std")] to #[cfg(all(feature = "std", any(target_arch = "x86", target_arch = "x86_64")))]
Attaching a patch with this change to the spec file fixes compilation issues on aarch64, ppc64le and s390x, but reveals new test failures on these architectures: https://koji.fedoraproject.org/koji/taskinfo?taskID=104184002
---- crypto::aead::aes256gcm::aes_api::test::test_seal_open stdout ---- thread 'crypto::aead::aes256gcm::aes_api::test::test_seal_open' panicked at 'called `Result::unwrap()` on an `Err` value: ()', src/crypto/aead/aes256gcm.rs:206:40 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace ---- crypto::aead::aes256gcm::aes_api::test::test_seal_open_detached stdout ---- thread 'crypto::aead::aes256gcm::aes_api::test::test_seal_open_detached' panicked at 'called `Result::unwrap()` on an `Err` value: ()', src/crypto/aead/aes256gcm.rs:222:40 ---- crypto::aead::aes256gcm::aes_impl::test::test_vector_1 stdout ---- thread 'crypto::aead::aes256gcm::aes_impl::test::test_vector_1' panicked at 'assertion failed: `(left == right)` left: `0`, right: `54`', src/crypto/aead/aes256gcm.rs:74:13 failures: crypto::aead::aes256gcm::aes_api::test::test_seal_open crypto::aead::aes256gcm::aes_api::test::test_seal_open_detached crypto::aead::aes256gcm::aes_impl::test::test_vector_1 test result: FAILED. 248 passed; 3 failed; 0 ignored; 0 measured; 1 filtered out; finished in 17.30s
Not sure how you want to deal with these. They don't look "harmless" so it might or might not be a good idea to just skip these tests.
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
Fabio Valentini decathorpe@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(stuart@gathman.or | |g)
--- Comment #27 from Fabio Valentini decathorpe@gmail.com --- Can you follow up here please?
- Do you still need this package? - Is cjdns considering to move to a different crypto library?
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
Stuart D Gathman stuart@gathman.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(stuart@gathman.or | |g) |
--- Comment #28 from Stuart D Gathman stuart@gathman.org --- I've mentioned the unsupported status of sodiumoxide to CJD, but he is working on PKT coin (Cjdns integrated with proof of bandwidth for a blockchain currency to reward local network links for anyone).
I do need to keep working on getting sodiumoxide (and new versions of cjdns) built. Sorry I've been so busy with other stuff.
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
--- Comment #29 from Fabio Valentini decathorpe@gmail.com --- It looks like cjdns has switched to its own fork of sodiumoxide: https://github.com/CJDNS-Development-Team/CJDNS/blob/master/cjdns-crypto/Car...
This likely means that there is no more need for this package?
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
--- Comment #30 from Stuart D Gathman stuart@gathman.org --- It seems to be a bug-fix patch for the official package. Are you suggesting I should bundle the cjdns patch? I would still need all the dependencies of sodiumoxide - which is the hard part. So ideally, the sodiumoxide package could include the bugfix patches from cjdns. Also, it sounds like the bug only affects Windows - which is not relevant for Fedora.
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
--- Comment #31 from Fabio Valentini decathorpe@gmail.com ---
It seems to be a bug-fix patch for the official package.
Yes, because sodiumoxide is archived and abandoned. :)
I would still need all the dependencies of sodiumoxide - which is the hard part.
I don't think it would be hard - they're literally all already available in Fedora.
So ideally, the sodiumoxide package could include the bugfix patches from cjdns.
This is true, that would be another solution, but you'd need to patch cjdns to use it instead of the git snapshot they're referencing now.
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
Simo Sorce ssorce@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |ssorce@redhat.com
--- Comment #32 from Simo Sorce ssorce@redhat.com --- Hello, I would like to know why we would want this library in Fedora.
In general the Fedora Cyrpto team would like to avoid crypto library proliferation as much as possible, unless there is no other option and the dependent packages are a must have, and they cannot use one of the existing crypto libraries for some reason..
Also please review https://docs.fedoraproject.org/en-US/packaging-guidelines/CryptoPolicies/ and explain how this library will comply with crypto policies.
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
--- Comment #33 from Simo Sorce ssorce@redhat.com --- Please see if you can get upstream to add an option to use something like rust-openssl instead of trying to bring in more cryptographic libraries.
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
--- Comment #34 from Fabio Valentini decathorpe@gmail.com --- These are Rust bindings for libsodium, so not a *new* crypto library at all.
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
--- Comment #35 from Simo Sorce ssorce@redhat.com --- We'd still prefer to avoid usage of such a library where possible. But ok.
https://bugzilla.redhat.com/show_bug.cgi?id=2121490
--- Comment #36 from Fabio Valentini decathorpe@gmail.com --- The larger problem I see with the sodiumoxide project is that it was marked as "deprecated" by the developer, archived on GitHub in mid-2022, and is hence unmaintained since: https://github.com/sodiumoxide/sodiumoxide
That said, I also don't see any development work happening in CJDNS itself for the past two years, either: https://github.com/CJDNS-Development-Team/CJDNS
Has work on the Rust rewrite been abandoned? Or has development moved back into https://github.com/cjdelisle/cjdns ?
I see activity there, and the Rust code there seems more developed, too.
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com