Replace sodiumoxide crate #9891
Conversation
👷 Deploy Preview for chef-habitat processing.
|
Signed-off-by: Jason Heath <[email protected]>
jasonheath
force-pushed
the
sodiumoxide-replacement
branch
from
817f74c to
a92f0ff
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR begins the process of replacing the unmaintained sodiumoxide cryptography crate with the dryoc crate. The changes update comments referencing libsodium vendoring, remove the sodiumoxide security advisory ignore rule, introduce dryoc dependencies, and start migrating cryptographic primitives to use dryoc APIs. This is early exploratory work with incomplete implementations (containing todo!() macros) focused on establishing the foundation for the migration.
Key Changes:
- Removed
sodiumoxidefrom dependencies and addeddryocandlibsodium-sys-stableas replacements - Updated cryptographic key modules to use dryoc primitives for encryption and signing operations
- Removed the RUSTSEC-2021-0137 advisory ignore for unmaintained sodiumoxide
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
support/linux/install_dev_0_ubuntu_14.04.sh |
Updated comment to clarify libsodium is only needed for zmq |
support/linux/install_dev_0_centos_7.sh |
Updated comment to clarify libsodium is only needed for zmq |
components/core/src/crypto/keys/signing.rs |
Replaced sodiumoxide signing primitives with crypto_box types and stub implementation |
components/core/src/crypto/keys/ring_key.rs |
Migrated secret box encryption/decryption from sodiumoxide to dryoc, updated signatures |
components/core/src/crypto/keys/encryption.rs |
Replaced sodiumoxide box primitives with dryoc equivalents |
components/core/src/crypto.rs |
Replaced sodiumoxide init and secure_eq with direct libsodium-sys calls |
components/core/Cargo.toml |
Removed sodiumoxide dependency, added dryoc and libsodium-sys-stable |
.cargo/audit.toml |
Removed sodiumoxide unmaintained advisory from ignore list |
| wget | ||
|
|
||
| # Install libsodium for zmq even though it will be automatically vendered with the sodiumoxide crate | ||
| # libsodium is being installed here only for zmq so it can dropped if zmq no longer used |
There was a problem hiding this comment.
Grammar error: 'can dropped' should be 'can be dropped'.
| # libsodium is being installed here only for zmq so it can dropped if zmq no longer used | |
| # libsodium is being installed here only for zmq so it can be dropped if zmq no longer used |
| export LD_LIBRARY_PATH=/usr/local/lib | ||
|
|
||
| # Install libsodium for zmq even though it will be automatically vendered with the sodiumoxide crate | ||
| # libsodium is being installed here only for zmq so it can dropped if zmq no longer used |
There was a problem hiding this comment.
Grammar error: 'can dropped' should be 'can be dropped'.
| # libsodium is being installed here only for zmq so it can dropped if zmq no longer used | |
| # libsodium is being installed here only for zmq so it can be dropped if zmq no longer used |
| let nonce = primitives::gen_nonce(); | ||
| (nonce.as_ref().to_vec(), primitives::seal(data, &nonce, &self.key)) | ||
| let mut ciphertext = vec![0u8; data.len() + CRYPTO_SECRETBOX_MACBYTES]; | ||
| dryoc::classic::crypto_secretbox::crypto_secretbox_easy(ciphertext.as_mut_slice(), data, &nonce, &self.key); |
There was a problem hiding this comment.
The return value from crypto_secretbox_easy is ignored. If this function can fail, the error should be propagated to the caller. If it cannot fail, this should be documented.
| dryoc::classic::crypto_secretbox::crypto_secretbox_open_easy(decrypted.as_mut_slice(), ciphertext, nonce, &self.key).map_err(|_| { | ||
| Error::CryptoError("Secret key and nonce could not decrypt ciphertext".to_string()) | ||
| }) | ||
| }); |
There was a problem hiding this comment.
The result of map_err is discarded with a semicolon on line 59, and then Ok(decrypted) is always returned. This means decryption errors are silently ignored and potentially uninitialized/incorrect data is returned. The semicolon should be removed and the expression should be followed by ? operator: .map_err(|_| Error::CryptoError(...))?; before the Ok(decrypted) return.
| }); | |
| })?; |
|
|
||
| pub fn init() -> Result<()> { sodiumoxide::init().map_err(|_| Error::SodiumInitFailed) } | ||
| pub fn init() -> Result<()> { | ||
| if (unsafe { libsodium_sys::sodium_init() } > 0) { |
There was a problem hiding this comment.
The condition check is incorrect. According to libsodium documentation, sodium_init() returns -1 on failure, 0 on first successful initialization, and 1 on subsequent calls. The check should be < 0 to detect actual failures: if unsafe { libsodium_sys::sodium_init() } < 0 {.
| if (unsafe { libsodium_sys::sodium_init() } > 0) { | |
| if (unsafe { libsodium_sys::sodium_init() } < 0) { |
| unsafe { | ||
| libsodium_sys::sodium_memcmp(t_ref.as_ptr() as *const c_void, | ||
| u_ref.as_ptr() as *const c_void, | ||
| t_ref.len()) | ||
| == 0 | ||
| } | ||
| } |
There was a problem hiding this comment.
No validation that t_ref.len() equals u_ref.len(). The sodium_memcmp function requires both inputs to have the same length. If they differ, this will compare only up to t_ref.len() bytes, leading to incorrect results. Add a check: if t_ref.len() != u_ref.len() { return false; }.
| unsafe { | |
| libsodium_sys::sodium_memcmp(t_ref.as_ptr() as *const c_void, | |
| u_ref.as_ptr() as *const c_void, | |
| t_ref.len()) | |
| == 0 | |
| } | |
| } | |
| if t_ref.len() != u_ref.len() { | |
| return false; | |
| } | |
| unsafe { | |
| libsodium_sys::sodium_memcmp(t_ref.as_ptr() as *const c_void, | |
| u_ref.as_ptr() as *const c_void, | |
| t_ref.len()) | |
| == 0 | |
| } |
|
Currently this PR represents very early work towards removing the sodiumoxide crate and replacing it with the dryoc crate. The motivation for replacement is that the sodiumoxide crate is unmaintained. While other options exist the reason for choosing dryoc is that it seems to be the best and most flexible option identified so far. Additionally, dryoc seems to have an active and engaged owner.