feat(settly): config encryption-at-rest + hot-reload watcher (CFG-SOTA-001 + CFG-SOTA-002)#76
feat(settly): config encryption-at-rest + hot-reload watcher (CFG-SOTA-001 + CFG-SOTA-002)#76KooshaPari wants to merge 4 commits into
Conversation
CI on b3b65d7 still fails four jobs against stable 1.96.0. Format (rustfmt --check): - crates/pheno-config/examples/cascade.rs: collapse multi-line println!. - crates/pheno-config/examples/validation.rs: collapse multi-line format!. Two genuine fmt diffs stable rustfmt reports. (crates/settly/rustfmt.toml still uses indent_style=Block and group_imports=StdExternalCrate which are nightly-only; stable drops them silently and emits warnings — those options stay, no diff is generated for them on the current tree.) Clippy (cargo clippy -D warnings): - crates/settly/src/domain/layers.rs:103 use sort_by_key (clippy::unnecessary_sort_by). Test (cargo test): - crates/pheno-config/tests/toml_merge_test.rs: add #[allow(dead_code)] to the four PREFIX_* constants (they are pool entries for future tests, currently only some are consumed; -D warnings would otherwise fail the test compile with dead-code). cargo-deny (EmbarkStudios/cargo-deny-action 0.19.8): - deny.toml [bans]: remove six unrecognized keys (workspace-features, multiple-mains, main, unnameable-traits, unnameable-types, unknown-lints). cargo-deny >= 0.18 rejects unknown keys as a hard error; these were never documented [bans] keys and are silently dropped by 0.16 but hard-fail by 0.19.8. - deny.toml [licenses]: add BSL-1.0 to allow-list (pulled in transitively). No files deleted.
…A-001 + CFG-SOTA-002)
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
CodeAnt AI is reviewing your PR. |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
the
size:L
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| out.extend_from_slice(&self.ciphertext); | ||
| out | ||
| } | ||
|
|
||
| pub fn decode(bytes: &[u8]) -> Result<Self, ConfigCryptoError> { | ||
| let header_len = 4 + SALT_LEN + NONCE_LEN; | ||
| if bytes.len() < header_len { | ||
| return Err(ConfigCryptoError::Envelope("file too short".into())); | ||
| } | ||
| if bytes[..4] != Self::MAGIC { | ||
| return Err(ConfigCryptoError::Envelope("magic header mismatch".into())); | ||
| } | ||
| let salt: [u8; SALT_LEN] = bytes[4..4 + SALT_LEN] | ||
| .try_into() | ||
| .map_err(|_| ConfigCryptoError::Envelope("salt slice".into()))?; | ||
| let nonce: [u8; NONCE_LEN] = bytes[4 + SALT_LEN..header_len] | ||
| .try_into() | ||
| .map_err(|_| ConfigCryptoError::Envelope("nonce slice".into()))?; | ||
| let ciphertext = bytes[header_len..].to_vec(); | ||
| Ok(Self { salt, nonce, ciphertext, aad: Vec::new() }) |
There was a problem hiding this comment.
Suggestion: The on-disk envelope drops AAD during serialization/deserialization: encode() never writes self.aad, and decode() always restores aad as empty. Any payload encrypted with non-empty AAD will fail to decrypt after reading from disk, breaking encrypt_to_file/decrypt_from_file contract. Persist AAD in the file format (with a length prefix) and restore it in decode(). [incomplete implementation]
Severity Level: Major ⚠️
- ❌ File-backed decrypt fails whenever non-empty AAD used.
- ⚠️ AAD feature unusable for encrypted config integrity binding.Steps of Reproduction ✅
1. In `crates/settly/src/crypto.rs` tests module (lines 392–400), modify
`atomic_write_then_read` to call `encrypt_to_file(&path, b\"pw\", &cfg, b\"aad-1\")`
instead of using empty AAD `b\"\"` (encrypt_to_file is defined at lines 180–189).
2. Running the test calls `encrypt()` at lines 115–137 from `encrypt_to_file`, which
encrypts `cfg` with AES-256-GCM using the provided non-empty AAD and constructs
`EncryptedEnvelope { salt, nonce, ciphertext, aad: aad.to_vec() }`.
3. `encrypt_to_file` then calls `EncryptedEnvelope::encode()` at lines 74–81, which only
appends `MAGIC`, `salt`, `nonce`, and `ciphertext` to the output buffer; `self.aad` is
never written to disk.
4. `decrypt_from_file` at lines 192–198 reads the file via `read_envelope` (173–177) and
`EncryptedEnvelope::decode()` (83–99), which restores `aad` as `Vec::new()`, so
`decrypt()` at 139–155 invokes AES-GCM with empty AAD and returns
`Err(ConfigCryptoError::Decrypt(_))` instead of successfully returning `SampleCfg`,
causing the file-backed roundtrip to fail whenever non-empty AAD is used.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/settly/src/crypto.rs
**Line:** 79:98
**Comment:**
*Incomplete Implementation: The on-disk envelope drops AAD during serialization/deserialization: `encode()` never writes `self.aad`, and `decode()` always restores `aad` as empty. Any payload encrypted with non-empty AAD will fail to decrypt after reading from disk, breaking `encrypt_to_file`/`decrypt_from_file` contract. Persist AAD in the file format (with a length prefix) and restore it in `decode()`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| let (raw_tx, mut raw_rx) = tokio::sync::mpsc::unbounded_channel(); | ||
| let mut watcher = notify::recommended_watcher(move |res: notify::Result<notify::Event>| { | ||
| if let Ok(ev) = res { | ||
| let _ = raw_tx.send(ev); | ||
| } | ||
| }) |
There was a problem hiding this comment.
Suggestion: Using an unbounded MPSC channel for watcher events allows unbounded memory growth during event storms (e.g., noisy filesystems or rapid writes), which can exhaust process memory. Switch to a bounded channel and drop/coalesce excess events explicitly. [performance]
Severity Level: Major ⚠️
- ⚠️ Event storms can accumulate unlimited queued watcher events.
- ⚠️ Memory pressure possible during rapid config file updates.Steps of Reproduction ✅
1. `HotReloader::open` at lines 233–299 sets up an unbounded MPSC channel via
`tokio::sync::mpsc::unbounded_channel()` at line 249 to receive `notify::Event` values
from the filesystem watcher.
2. The `notify::recommended_watcher` callback at lines 250–253 sends every successful
filesystem event into `raw_tx` with `let _ = raw_tx.send(ev);`, which always succeeds
because the channel is unbounded and imposes no backpressure.
3. If external code rapidly rewrites the watched config file (e.g., in a tight loop),
events are produced faster than the debounce loop at 261–288 can execute
`raw_rx.recv().await` and run decryption, causing the internal queue of the unbounded
channel to grow continuously.
4. Under sustained event storms targeting this file, the process accumulates an
arbitrarily large backlog of queued `notify::Event` instances in memory, since nothing in
`HotReloader` bounds or drops events, leading to increased memory usage and potential OOM
during heavy update patterns.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/settly/src/crypto.rs
**Line:** 249:254
**Comment:**
*Performance: Using an unbounded MPSC channel for watcher events allows unbounded memory growth during event storms (e.g., noisy filesystems or rapid writes), which can exhaust process memory. Switch to a bounded channel and drop/coalesce excess events explicitly.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| let Some(_ev) = raw_rx.recv().await else { break }; | ||
| let mut latest: Option<notify::Event> = None; | ||
| loop { | ||
| match tokio::time::timeout(std::time::Duration::from_millis(250), raw_rx.recv()).await { | ||
| Ok(Some(ev)) => latest = Some(ev), | ||
| Ok(None) => return, | ||
| Err(_) => break, | ||
| } | ||
| } | ||
| if let Some(_ev) = latest { |
There was a problem hiding this comment.
Suggestion: The debounce loop ignores the first filesystem event when no second event arrives within 250ms, because latest starts as None and only gets set by subsequent events. This means a single file change can be silently dropped and never reloaded. Seed latest with the first received event before entering the timeout loop. [logic error]
Severity Level: Major ⚠️
- ❌ Single file change may never trigger config reload.
- ⚠️ Hot-reload unreliable; requires multiple events per change.Steps of Reproduction ✅
1. Use `HotReloader::<SampleCfg>::open` at lines 233–299 to watch an existing encrypted
config file on disk; this call spawns the debounce loop in the background task at lines
261–288.
2. Trigger a filesystem change that results in exactly one `notify::Event` being sent to
the callback at 250–253 for the watched file (for example, an overwrite that the
underlying `notify` backend reports as a single event).
3. The background loop at 264 consumes that first event with `let Some(_ev) =
raw_rx.recv().await`, initializes `latest` to `None` at 265, then waits up to 250ms for
additional events in the inner loop at 266–271; when `tokio::time::timeout` at 267 expires
without another event, the inner loop breaks with `latest` still `None`.
4. Because `latest` remains `None`, the guard `if let Some(_ev) = latest` at 273 fails, so
`decrypt_from_file::<T>` at 274 is never called and no `ReloadEvent` is sent, meaning a
single filesystem change can be silently dropped and configuration never reloaded.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/settly/src/crypto.rs
**Line:** 264:273
**Comment:**
*Logic Error: The debounce loop ignores the first filesystem event when no second event arrives within 250ms, because `latest` starts as `None` and only gets set by subsequent events. This means a single file change can be silently dropped and never reloaded. Seed `latest` with the first received event before entering the timeout loop.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| match decrypt_from_file::<T>(&path_clone, &passphrase_clone) { | ||
| Ok(new_cfg) => { | ||
| let event = ReloadEvent { | ||
| config: Arc::new(new_cfg), | ||
| reloaded_at: chrono::Utc::now(), | ||
| }; | ||
| let _ = tx_clone.send(event); |
There was a problem hiding this comment.
Suggestion: The background watcher publishes reload events but never updates the shared current state, so current() can return stale config forever unless reload_now() is called manually. Update the RwLock value inside the watcher success path before broadcasting. [stale reference]
Severity Level: Major ⚠️
- ❌ current() never reflects changes from filesystem reloads.
- ⚠️ Subscribers see updates while synchronous readers stay stale.Steps of Reproduction ✅
1. Create an encrypted config file with `encrypt_to_file` at lines 180–189 (similar to
`atomic_write_then_read` at 392–399), then call
`HotReloader::<SampleCfg>::open(path.clone(), b\"pw\")` at lines 233–299 to obtain
`(reloader, initial)`.
2. Inside `open`, `current` is initialized to the initial value at line 242, and a
background task is spawned at lines 261–288 that listens for filesystem events on `raw_rx`
and, on success, executes the match block at lines 274–281.
3. From another task, update the same encrypted config file using `encrypt_to_file(&path,
b\"pw\", &new_cfg, b\"\")`; the `notify` watcher callback at 250–253 sends an event into
`raw_tx`, which the background loop receives and then calls
`decrypt_from_file::<T>(&path_clone, &passphrase_clone)` at line 274 and broadcasts a
`ReloadEvent` via `tx_clone.send(event)` at line 280.
4. After this automatic reload path completes, calling `reloader.current()` at lines
306–308 still returns the original `Arc<T>` because only `reload_now` at 311–319 writes to
`self.current`, while the watcher success branch at 274–280 never updates `current`,
leaving read-based accessors permanently stale despite successful reload notifications.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/settly/src/crypto.rs
**Line:** 274:280
**Comment:**
*Stale Reference: The background watcher publishes reload events but never updates the shared `current` state, so `current()` can return stale config forever unless `reload_now()` is called manually. Update the `RwLock` value inside the watcher success path before broadcasting.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d496fb821
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| .try_into() | ||
| .map_err(|_| ConfigCryptoError::Envelope("nonce slice".into()))?; | ||
| let ciphertext = bytes[header_len..].to_vec(); | ||
| Ok(Self { salt, nonce, ciphertext, aad: Vec::new() }) |
There was a problem hiding this comment.
When encrypt_to_file(..., aad) is called with any non-empty AAD, the file cannot be read back through decrypt_from_file because the on-disk envelope drops the AAD and decode reconstructs it as empty. AES-GCM authenticates AAD, so those files always fail decryption unless callers happen to use b"", making the public file API unusable for the AAD it accepts.
Useful? React with 👍 / 👎.
| let Some(_ev) = raw_rx.recv().await else { break }; | ||
| let mut latest: Option<notify::Event> = None; | ||
| loop { | ||
| match tokio::time::timeout(std::time::Duration::from_millis(250), raw_rx.recv()).await { | ||
| Ok(Some(ev)) => latest = Some(ev), |
There was a problem hiding this comment.
For a file update that produces a single notify event, the reloader consumes that event into _ev, initializes latest to None, then times out and skips the decrypt/send path. Hot reload therefore only works when the backend emits at least two events for a write, so single-event updates leave subscribers unaware of the changed config.
Useful? React with 👍 / 👎.
| Ok(new_cfg) => { | ||
| let event = ReloadEvent { | ||
| config: Arc::new(new_cfg), | ||
| reloaded_at: chrono::Utc::now(), | ||
| }; |
There was a problem hiding this comment.
On successful external reloads this branch only broadcasts the new value; it never writes the new Arc<T> into the current RwLock. Any caller that checks HotReloader::current() after a file-system-triggered reload will continue to receive the initial config, while only manual reload_now() updates the cache.
Useful? React with 👍 / 👎.
|
|
||
| pub mod adapters; | ||
| pub mod application; | ||
| pub mod crypto; |
There was a problem hiding this comment.
Suggestion: Making the crypto module unconditional pulls crypto.rs into every build, but that file depends on crates (aes_gcm, argon2, base64ct, rand, notify, parking_lot) that are not declared in this crate's dependencies/features. This turns default builds into a hard failure instead of optional functionality. Gate the module behind the intended feature flags and/or add the required dependencies. [api mismatch]
Severity Level: Critical 🚨
- ❌ Missing crypto dependencies cause compile errors in `crypto.rs`.
- ❌ Default `settly` builds fail; encryption features unusable.Steps of Reproduction ✅
1. From `/workspace/Configra`, run `cargo build -p settly` using the package definition at
`crates/settly/Cargo.toml:1-13`.
2. The root module `crates/settly/src/lib.rs` declares `pub mod crypto;` at line 22, which
causes Rust to compile the submodule file `crates/settly/src/crypto.rs`.
3. While compiling `crates/settly/src/crypto.rs`, Rust processes external crate usages at
lines 22-31 (e.g. `use aes_gcm::{ aead::{Aead, KeyInit, Payload}, Aes256Gcm, Key, Nonce
};`, `use argon2::{Algorithm, Argon2, Params, Version};`, `use base64ct::{Base64,
Encoding};`, `use rand::{rngs::OsRng, RngCore};`, and `use tokio::sync::broadcast;`) and
later references to `notify::RecommendedWatcher` and `parking_lot::RwLock` at lines
223-225, 247-258.
4. Checking `crates/settly/Cargo.toml:14-43` and the workspace dependencies in
`Cargo.toml:26-38` shows no entries for `aes_gcm`, `argon2`, `base64ct`, `rand`, `notify`,
or `parking_lot`. Because these crates are undeclared, the `cargo build` in step 1 fails
with errors like "can't find crate `aes_gcm`" and unresolved path
`notify::RecommendedWatcher`, meaning the unconditional `pub mod crypto;` breaks default
`settly` builds instead of keeping the crypto/hot-reload functionality optional.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/settly/src/lib.rs
**Line:** 22:22
**Comment:**
*Api Mismatch: Making the `crypto` module unconditional pulls `crypto.rs` into every build, but that file depends on crates (`aes_gcm`, `argon2`, `base64ct`, `rand`, `notify`, `parking_lot`) that are not declared in this crate's dependencies/features. This turns default builds into a hard failure instead of optional functionality. Gate the module behind the intended feature flags and/or add the required dependencies.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| pub use crypto::{ | ||
| ConfigCrypto, EncryptedConfig, KeyDerivation, KEY_BYTES, NONCE_BYTES, SALT_BYTES, TAG_BYTES, | ||
| }; |
There was a problem hiding this comment.
Suggestion: This re-export list references symbols that do not exist in crates/settly/src/crypto.rs (ConfigCrypto, EncryptedConfig, KeyDerivation, KEY_BYTES, NONCE_BYTES, SALT_BYTES, TAG_BYTES). The crate API will break at compile time because pub use cannot resolve these items. Re-export the actual existing names from crypto.rs (for example the current envelope/error/types/constants) or add the missing definitions before exporting them. [api mismatch]
Severity Level: Critical 🚨
- ❌ `settly` crate fails to compile at crypto re-export.
- ❌ Workspace builds including `settly` stop at compile-time error.Steps of Reproduction ✅
1. From the repository root `/workspace/Configra`, run `cargo build -p settly` to compile
the `settly` crate defined in `crates/settly/Cargo.toml:1-13`.
2. During compilation, Rust loads the root module `crates/settly/src/lib.rs:20-32`,
including the re-export block at lines 30-32: `pub use crypto::{ ConfigCrypto,
EncryptedConfig, KeyDerivation, KEY_BYTES, NONCE_BYTES, SALT_BYTES, TAG_BYTES, };`.
3. The compiler resolves these names against the `crypto` module defined by `pub mod
crypto;` at `lib.rs:22`, which maps to `crates/settly/src/crypto.rs`.
4. Inspecting `crates/settly/src/crypto.rs:34-155, 204-327` shows existing items like
`ConfigCryptoError`, `EncryptedEnvelope`, constants `SALT_LEN` and `NONCE_LEN`, and
functions `derive_key`, `encrypt`, `decrypt`, `encrypt_to_file`, but no definitions for
`ConfigCrypto`, `EncryptedConfig`, `KeyDerivation`, `KEY_BYTES`, `NONCE_BYTES`,
`SALT_BYTES`, or `TAG_BYTES`. As a result, the `cargo build` in step 1 fails with
unresolved import errors such as "no `ConfigCrypto` in `crypto`" at `lib.rs:30-32`.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/settly/src/lib.rs
**Line:** 30:32
**Comment:**
*Api Mismatch: This re-export list references symbols that do not exist in `crates/settly/src/crypto.rs` (`ConfigCrypto`, `EncryptedConfig`, `KeyDerivation`, `KEY_BYTES`, `NONCE_BYTES`, `SALT_BYTES`, `TAG_BYTES`). The crate API will break at compile time because `pub use` cannot resolve these items. Re-export the actual existing names from `crypto.rs` (for example the current envelope/error/types/constants) or add the missing definitions before exporting them.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished reviewing your PR. |
1 participant
User description
Summary
CodeAnt-AI Description
Add encrypted config storage and file hot reload
What Changed
Impact
✅ Protected config files on disk✅ Automatic config refresh after file changes✅ Stable config precedence when layers are merged💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.