feat: adaugat structura de baza pentru ipfs_plugin#324
feat: adaugat structura de baza pentru ipfs_plugin#324razvanlazar2108 wants to merge 24 commits into
Conversation
1deb1f8 to
b6c159b
Compare
b6c159b to
02f0364
Compare
79efd5a to
23d95a0
Compare
|
@claude code review |
radumarias
left a comment
There was a problem hiding this comment.
Automated high-effort review (8 finder angles; every finding verified against the code — one candidate was refuted during verification and dropped). 10 findings attached as inline comments, most severe first:
enable_ipfs_plugin(&mut self)is uncallable —EncryptedFsonly exists behindArc, so the feature ships permanently disabled.- The master key crosses the API as a plain
Vec<u8>and is held forever, bypassing the documentedSecretVec/ExpireValue(mlock/zeroize, 10-min expiry) design, with no domain separation. - No key-length validation — a wrong-length key panics in ring's
UnboundKey::newand aborts the process (panic = "abort") despite theResultsignature. ChaCha20Poly1305is hardcoded, ignoring the filesystem's configured cipher (Aes256Gcmmismatch).pub ipfs_cipheris a dead field — never read anywhere — and the only public field on the core struct.cargo fmt --all -- --checkfails on this branch (verified), socheck-before-push.sh/CI can't pass.IpfsCipherre-implements thecrypto::encrypt/decryptone-shot pattern with stringly-typed errors that hide AEAD integrity failures.- The
#[path]graft makesipfs_plugin/look like a crate while compiling it unconditionally into the core library — neither a real module nor a real crate, and contains no actual IPFS functionality yet. - Each encrypt/decrypt call allocates ~256-512 KB of zeroed scratch (including an entirely unused
OpeningKey/decrypt buffer), re-expands keys, and creates a fresh OS-entropy RNG. - The Cargo.lock rustix 0.37.27→0.37.28 bump is unrelated churn and should be reverted.
Additional notes:
- Comments, rustdoc, error strings, and test messages are in Romanian in an otherwise all-English codebase — these surface in published API docs and runtime errors; please translate.
- Build status: the branch does compile (I verified with
cargo check --all-targetsusing--cfg rustix_use_libcto work around the pre-existing rustix-0.37/nightly incompatibility), with no new warnings from this PR's code. - One candidate finding — that the empty-input early-returns in
encrypt/decryptbypass AEAD authentication — was refuted during verification: the crate's own streams also produce/accept zero bytes for empty input, so those branches are merely redundant, not a new integrity gap.
Generated by Claude Code
| return ino; | ||
| } | ||
| } | ||
| pub fn enable_ipfs_plugin(&mut self, master_key: Vec<u8>) { |
There was a problem hiding this comment.
This method is uncallable — the feature is dead on arrival.
enable_ipfs_plugin takes &mut self, but the only constructor, EncryptedFs::new, returns FsResult<Arc<Self>> (and stores a self_weak clone internally, so Arc::get_mut fails too). Every consumer — the FUSE mount path, java-bridge, the rustdoc examples — holds Arc<EncryptedFs>, which never yields &mut. Any attempt to call this compiles to "cannot borrow data in an Arc as mutable", so ipfs_cipher can only ever remain None; nothing in this PR (including its test) calls it.
If core support is truly needed, this has to fit the type's ownership model: a constructor parameter, or an interior-mutability slot — though see the other comments for why the plugin probably shouldn't touch EncryptedFs at all.
Generated by Claude Code
| } | ||
| } | ||
| pub fn enable_ipfs_plugin(&mut self, master_key: Vec<u8>) { | ||
| self.ipfs_cipher = Some(IpfsCipher::new(master_key)); |
There was a problem hiding this comment.
Taking the master key as a plain Vec<u8> — and keeping it for the lifetime of the filesystem — breaks the crate's documented key-handling model.
CLAUDE.md states secrets are "held in shush_rs types (mlock/mprotect/zeroize) and dropped after inactivity", and docs/claude/encryptedfs.md documents the master key living only in ExpireValue<SecretVec<u8>> with a 10-minute expiry. This API forces the opposite on both counts:
- There is no accessor that yields the key as
Vec<u8>, so a caller would have to dokey.get().await.expose_secret().to_vec()— materializing an unprotected, non-zeroized heap copy (swappable, visible in core dumps) just to cross this boundary. - The resulting
IpfsCipherthen holds the key with no expiry for as long as the fs lives, outside theExpireValuemechanism. - It also reuses the raw master key for a second cryptographic purpose with no domain separation — a leak or break in the IPFS context compromises the entire data dir.
If the plugin needs a key, accept &SecretVec<u8> end-to-end (like every function in src/crypto.rs) and preferably derive a purpose-specific subkey from the master key rather than handing out the master key itself.
Generated by Claude Code
|
|
||
| impl IpfsCipher { | ||
| /// Inițializează plugin-ul cu o cheie sigură și cipher-ul implicit | ||
| pub fn new(secret_key: Vec<u8>) -> Self { |
There was a problem hiding this comment.
No key-length validation — a wrong-length key panics (and aborts the process in release builds) despite the Result signature.
new() accepts any Vec<u8>. The first encrypt()/decrypt() then reaches UnboundKey::new(algorithm, &key.expose_secret()).expect("unbound key") (src/crypto/write.rs:92, and the .unwrap() at read.rs:93); ring returns Err for any key that isn't exactly Cipher::key_len() bytes (32 here), so the .expect fires. With [profile.release] panic = "abort", that's a full process abort — from a safe public API whose methods promise Result<_, String>. Every existing key path avoids this because crypto::derive_key sizes the key to cipher.key_len(); this constructor is the only way to smuggle in a bad length.
Validate in new() (return Result and check secret_key.len() == self.cipher.key_len()), or better, accept SecretVec<u8> from a source that already guarantees the length.
Generated by Claude Code
| pub fn new(secret_key: Vec<u8>) -> Self { | ||
| Self { | ||
| key: SecretVec::from(secret_key), | ||
| cipher: Cipher::ChaCha20Poly1305, |
There was a problem hiding this comment.
Hardcoding ChaCha20Poly1305 ignores the cipher the filesystem was configured with.
EncryptedFs supports Aes256Gcm too and stores its configured cipher in self.cipher, but enable_ipfs_plugin doesn't pass it through — so on an AES-256-GCM filesystem, IPFS-bound data would be encrypted with a different cipher than the rest of the store, using the same master key across two algorithms. Every existing crypto entry point (create_write, create_read, crypto::encrypt/decrypt, derive_key) takes cipher: Cipher as a parameter; new() should as well, with the fs passing self.cipher.
Generated by Claude Code
| sizes_read: Mutex<HashMap<u64, AtomicU64>>, | ||
| requested_read: Mutex<HashMap<u64, AtomicU64>>, | ||
| read_only: bool, | ||
| pub ipfs_cipher: Option<IpfsCipher>, |
There was a problem hiding this comment.
Dead public field on the security-critical core type.
ipfs_cipher is never read by any code in the tree — no FUSE op, read/write path, CLI flag, or test consults it — and it's the only pub field on EncryptedFs (everything else, including the key, is private by design). Consequences:
- Being
pub, external code with&mutaccess could replace it wholesale with anIpfsCipherbuilt from an arbitrary key, bypassing all encapsulation. - It permanently commits an experimental, unused field to the crate's semver-public API surface.
- Since
IpfsCipheronly wraps the already-publiccrypto::create_write/create_read, the core struct doesn't need to know about it at all — the IPFS integration can hold its own cipher and consume the public crypto API from outside.
Suggest deleting the field (and enable_ipfs_plugin) until something actually consumes it.
Generated by Claude Code
| Ok(arc) | ||
|
|
||
|
|
||
| } |
There was a problem hiding this comment.
cargo fmt --all -- --check fails on this PR — these two stray blank lines (one contains only trailing whitespace) plus the missing trailing newline and unformatted lines in ipfs_plugin/src/lib.rs all trip rustfmt. Verified by running the check on this branch: it reports diffs in src/encryptedfs.rs and 6 locations in ipfs_plugin/src/lib.rs. That means ./scripts/check-before-push.sh (mandated by CLAUDE.md before committing) and the CI formatting gate cannot pass as-is. Please run cargo fmt --all.
| Ok(arc) | |
| } | |
| Ok(arc) | |
| } |
Generated by Claude Code
| } | ||
|
|
||
| /// Criptează un bloc de date utilizând un Cursor pentru a simula un fișier în memorie | ||
| pub fn encrypt(&self, data: &[u8]) -> Result<Vec<u8>, String> { |
There was a problem hiding this comment.
This re-implements the one-shot pattern that already exists in src/crypto.rs, with a worse error type.
crypto::encrypt/crypto::decrypt (src/crypto.rs:197-216) do exactly this — Cursor::new(vec[]) → create_write → write_all → finish → into_inner, and create_read → read_to_*. A byte-slice variant belongs next to them (e.g. encrypt_bytes/decrypt_bytes returning crypto::Result<Vec<u8>>) so future fixes to the finish/flush or nonce handling propagate to all callers instead of leaving a divergent copy here.
Related: Result<Vec<u8>, String> with format!("...: {:?}", e) discards the crate's error structure — an AEAD tag mismatch (integrity failure) becomes indistinguishable from a plain IO error, and callers can't use ?. Both underlying operations return io::Error, which crypto::Error already converts via #[from]. (The data.is_empty() early-returns are also redundant — the streams already produce/accept empty input, covered by test_encrypt_decrypt_empty_string.)
Generated by Claude Code
| } | ||
|
|
||
| #[path = "../ipfs_plugin/src/lib.rs"] | ||
| pub mod ipfs_plugin; |
There was a problem hiding this comment.
The #[path] graft gives ipfs_plugin/ the layout of a standalone crate while actually compiling it unconditionally into the core library — the isolation of neither.
ipfs_plugin/ contains only src/lib.rs (no Cargo.toml), and its use crate::crypto::... imports only resolve when grafted in here. So the directory looks like a workspace crate (inviting someone to add it to [workspace] members, which would fail), is invisible to per-package tooling, and yet ships as rencfs::ipfs_plugin — unconditional, un-feature-gated, semver-public API in every build.
The repo already has the right pattern for an integration bridge: java-bridge/ is a real separate crate with its own Cargo.toml that depends on rencfs as a library. Either do that (which also lets the plugin gain actual IPFS dependencies later without touching the core crate), or make it an honest module at src/ipfs_plugin.rs, ideally behind a #[cfg(feature = "ipfs")] gate. Worth noting: nothing here is IPFS-specific yet — no CIDs, chunking, or networking — it's a generic buffer-crypto wrapper with an aspirational name.
Generated by Claude Code
|
|
||
| // Folosim Cursor pentru a implementa Write + Seek + Read cerute de autor | ||
| let memory_file = Cursor::new(Vec::new()); | ||
| let mut writer = crypto::create_write(memory_file, self.cipher, &self.key); |
There was a problem hiding this comment.
Every encrypt() call pays ~512 KB of zeroed allocations, two ring key expansions, and an OS-entropy RNG construction — most of it dead weight.
Because Cursor<Vec<u8>> implements Write + Seek + Read, RingCryptoWrite::new takes the seek-capable branch (writer.as_write_seek_read().is_some(), src/crypto/write.rs:98) even though this is a plain sequential write: it builds a second UnboundKey, an OpeningKey, and a zeroed ~256 KB decrypt_buf (write.rs:100-104) that are never used, on top of the ~256 KB write buffer (write.rs:96) and a fresh ChaCha20Rng::from_entropy() (write.rs:93 → crypto.rs:347). decrypt() likewise re-expands the key and zeroes a ~256 KB scratch buffer per call (read.rs:91-95). For the intended use — encrypting many small IPFS chunks — the setup cost dwarfs the actual AEAD work.
Cheaper: cache reusable cipher state in IpfsCipher instead of rebuilding it per call, and give the output Vecs capacity hints (ciphertext size is computable: data.len() + 28 * ceil(len/256KiB); here Cursor::new(Vec::new()) and the Vec::new() before read_to_end at line 48 both start at zero capacity).
Generated by Claude Code
| [[package]] | ||
| name = "rustix" | ||
| version = "0.37.27" | ||
| version = "0.37.28" |
There was a problem hiding this comment.
Unrelated lockfile churn: rustix 0.37.27 → 0.37.28.
No Cargo.toml changed, rustix is only a transitive dependency, and nothing in the IPFS plugin uses it — this bump is an accidental side effect of a local cargo update/build, smuggling an unreviewed dependency change into a feature PR. (FWIW, on the repo's floating nightly toolchain the 0.37.x line currently fails to compile at all with the rustc_attrs errors — I had to build this branch with --cfg rustix_use_libc to verify it; the bump doesn't fix that.) Please revert the Cargo.lock change so the PR only contains the feature.
Generated by Claude Code
Title
IPFS plugin adding encryption #255
Description
This PR implements content encryption support for the IPFS plugin by integrating the project's native cryptographic streams. It enables secure data encryption and decryption using the existing
ChaCha20Poly1305cipher architecture.Fixes #255
Type of change
Checklist: