Full codebase review by 4 independent agents (security, architecture,
code quality, correctness) identified ~80 findings. This commit fixes 40
of them across all workspace crates.
Critical fixes:
- Federation service: validate origin against mTLS cert CN/SAN (C1)
- WS bridge: add DM channel auth, size limits, rate limiting (C2)
- hpke_seal: panic on error instead of silent empty ciphertext (C3)
- hpke_setup_sender_and_export: error on parse fail, no PQ downgrade (C7)
Security fixes:
- Zeroize: seed_bytes() returns Zeroizing<[u8;32]>, private_to_bytes()
returns Zeroizing<Vec<u8>>, ClientAuth.access_token, SessionState.password,
conversation hex_key all wrapped in Zeroizing
- Keystore: 0o600 file permissions on Unix
- MeshIdentity: 0o600 file permissions on Unix
- Timing floors: resolveIdentity + WS bridge resolve_user get 5ms floor
- Mobile: TLS verification gated behind insecure-dev feature flag
- Proto: from_bytes default limit tightened from 64 MiB to 8 MiB
Correctness fixes:
- fetch_wait: register waiter before fetch to close TOCTOU window
- MeshEnvelope: exclude hop_count from signature (forwarding no longer
invalidates sender signature)
- BroadcastChannel: encrypt returns Result instead of panicking
- transcript: rename verify_transcript_chain → validate_transcript_structure
- group.rs: extract shared process_incoming() for receive_message variants
- auth_ops: remove spurious RegistrationRequest deserialization
- MeshStore.seen: bounded to 100K with FIFO eviction
Quality fixes:
- FFI error classification: typed downcast instead of string matching
- Plugin HookVTable: SAFETY documentation for unsafe Send+Sync
- clippy::unwrap_used: warn → deny workspace-wide
- Various .unwrap_or("") → proper error returns
Review report: docs/REVIEW-2026-03-04.md
152 tests passing (72 core + 35 server + 14 E2E + 1 doctest + 30 P2P)
143 lines
4.4 KiB
Rust
143 lines
4.4 KiB
Rust
use std::{
|
|
collections::HashMap,
|
|
fs,
|
|
path::{Path, PathBuf},
|
|
sync::RwLock,
|
|
};
|
|
|
|
use openmls_traits::key_store::{MlsEntity, OpenMlsKeyStore};
|
|
|
|
/// A disk-backed key store implementing `OpenMlsKeyStore`.
|
|
///
|
|
/// In-memory when `path` is `None`; otherwise flushes the entire map to disk on
|
|
/// every store/delete so HPKE init keys survive process restarts.
|
|
///
|
|
/// # Serialization
|
|
///
|
|
/// Uses bincode for both individual MLS entity values and the outer HashMap
|
|
/// container. This is required because OpenMLS types use bincode-compatible
|
|
/// serialization, and `HashMap<Vec<u8>, Vec<u8>>` requires a binary format
|
|
/// (JSON mandates string keys).
|
|
///
|
|
/// # Persistence security
|
|
///
|
|
/// When `path` is set, file permissions are restricted to owner-only (0o600)
|
|
/// on Unix platforms, since the store may contain HPKE private keys.
|
|
#[derive(Debug)]
|
|
pub struct DiskKeyStore {
|
|
path: Option<PathBuf>,
|
|
values: RwLock<HashMap<Vec<u8>, Vec<u8>>>,
|
|
}
|
|
|
|
#[derive(thiserror::Error, Debug, PartialEq, Eq)]
|
|
pub enum DiskKeyStoreError {
|
|
#[error("serialization error")]
|
|
Serialization,
|
|
#[error("io error: {0}")]
|
|
Io(String),
|
|
}
|
|
|
|
impl DiskKeyStore {
|
|
/// In-memory keystore (no persistence).
|
|
pub fn ephemeral() -> Self {
|
|
Self {
|
|
path: None,
|
|
values: RwLock::new(HashMap::new()),
|
|
}
|
|
}
|
|
|
|
/// Persistent keystore backed by `path`. Creates an empty store if missing.
|
|
pub fn persistent(path: impl AsRef<Path>) -> Result<Self, DiskKeyStoreError> {
|
|
let path = path.as_ref().to_path_buf();
|
|
let values = if path.exists() {
|
|
let bytes = fs::read(&path).map_err(|e| DiskKeyStoreError::Io(e.to_string()))?;
|
|
if bytes.is_empty() {
|
|
HashMap::new()
|
|
} else {
|
|
bincode::deserialize(&bytes)
|
|
.map_err(|_| DiskKeyStoreError::Serialization)?
|
|
}
|
|
} else {
|
|
HashMap::new()
|
|
};
|
|
|
|
let store = Self {
|
|
path: Some(path),
|
|
values: RwLock::new(values),
|
|
};
|
|
|
|
// Set restrictive file permissions on the keystore file.
|
|
store.set_file_permissions()?;
|
|
|
|
Ok(store)
|
|
}
|
|
|
|
fn flush(&self) -> Result<(), DiskKeyStoreError> {
|
|
let Some(path) = &self.path else {
|
|
return Ok(());
|
|
};
|
|
let values = self.values.read().map_err(|_| DiskKeyStoreError::Io("lock poisoned".into()))?;
|
|
let bytes = bincode::serialize(&*values).map_err(|_| DiskKeyStoreError::Serialization)?;
|
|
if let Some(parent) = path.parent() {
|
|
fs::create_dir_all(parent).map_err(|e| DiskKeyStoreError::Io(e.to_string()))?;
|
|
}
|
|
fs::write(path, &bytes).map_err(|e| DiskKeyStoreError::Io(e.to_string()))?;
|
|
self.set_file_permissions()?;
|
|
Ok(())
|
|
}
|
|
|
|
/// Restrict file permissions to owner-only (0o600) on Unix.
|
|
#[cfg(unix)]
|
|
fn set_file_permissions(&self) -> Result<(), DiskKeyStoreError> {
|
|
use std::os::unix::fs::PermissionsExt;
|
|
if let Some(path) = &self.path {
|
|
if path.exists() {
|
|
let perms = std::fs::Permissions::from_mode(0o600);
|
|
fs::set_permissions(path, perms)
|
|
.map_err(|e| DiskKeyStoreError::Io(format!("set permissions: {e}")))?;
|
|
}
|
|
}
|
|
Ok(())
|
|
}
|
|
|
|
#[cfg(not(unix))]
|
|
fn set_file_permissions(&self) -> Result<(), DiskKeyStoreError> {
|
|
Ok(())
|
|
}
|
|
}
|
|
|
|
impl Default for DiskKeyStore {
|
|
fn default() -> Self {
|
|
Self::ephemeral()
|
|
}
|
|
}
|
|
|
|
impl OpenMlsKeyStore for DiskKeyStore {
|
|
type Error = DiskKeyStoreError;
|
|
|
|
fn store<V: MlsEntity>(&self, k: &[u8], v: &V) -> Result<(), Self::Error> {
|
|
let value = bincode::serialize(v).map_err(|_| DiskKeyStoreError::Serialization)?;
|
|
let mut values = self.values.write().map_err(|_| DiskKeyStoreError::Io("lock poisoned".into()))?;
|
|
values.insert(k.to_vec(), value);
|
|
drop(values);
|
|
self.flush()
|
|
}
|
|
|
|
fn read<V: MlsEntity>(&self, k: &[u8]) -> Option<V> {
|
|
let values = match self.values.read() {
|
|
Ok(v) => v,
|
|
Err(_) => return None,
|
|
};
|
|
values
|
|
.get(k)
|
|
.and_then(|bytes| bincode::deserialize(bytes).ok())
|
|
}
|
|
|
|
fn delete<V: MlsEntity>(&self, k: &[u8]) -> Result<(), Self::Error> {
|
|
let mut values = self.values.write().map_err(|_| DiskKeyStoreError::Io("lock poisoned".into()))?;
|
|
values.remove(k);
|
|
drop(values);
|
|
self.flush()
|
|
}
|
|
}
|