mirror of https://github.com/ospab/ostp.git
fix: quiet hot-path logging and stop logging access keys verbatim
Two classes of issue:
- Hot-path/attacker-triggerable events logged at info/error with internal
detail: a per-handshake info! byte dump (raw_vec[0..6]) and a per-packet
error! on session-id mismatch that dumped expected/got session ids.
Both are log-flood + info-leak surfaces; downgraded to debug and
stripped of the sensitive detail. Close/Resume frame handling likewise
moved from info to debug.
- The access key (a shared secret) was written to logs verbatim in three
places (session drop, key-created UI event, API create-user) and as an
8-char prefix in one. Added key_fp() — a short SHA-256 fingerprint — and
routed all key logging through it so operators can still correlate
events without the secret ever hitting the log.
This commit is contained in:
parent
f904695760
commit
b5735fe8c2
|
|
@ -236,7 +236,9 @@ impl ProtocolMachine {
|
||||||
|
|
||||||
let session_id = u32::from_be_bytes([raw_vec[0], raw_vec[1], raw_vec[2], raw_vec[3]]);
|
let session_id = u32::from_be_bytes([raw_vec[0], raw_vec[1], raw_vec[2], raw_vec[3]]);
|
||||||
if session_id != self.session_id {
|
if session_id != self.session_id {
|
||||||
tracing::error!("session id mismatch! expected={:#010x}, got={:#010x}, is_handshake={}, raw_len={}", self.session_id, session_id, is_handshake, raw_vec.len());
|
// Per-packet, attacker-triggerable event: keep at debug and don't
|
||||||
|
// dump internal session ids (log-flood + info-leak surface).
|
||||||
|
tracing::debug!("session id mismatch (is_handshake={})", is_handshake);
|
||||||
return Err(ProtocolError::State("session id mismatch".to_string()));
|
return Err(ProtocolError::State("session id mismatch".to_string()));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -262,8 +264,7 @@ impl ProtocolMachine {
|
||||||
noise_len, raw_vec.len() - 6
|
noise_len, raw_vec.len() - 6
|
||||||
)));
|
)));
|
||||||
}
|
}
|
||||||
tracing::info!("handle_inbound: raw_vec.len()={}, noise_len={}, raw_vec[0..6]={:?}", raw_vec.len(), noise_len, &raw_vec[0..6]);
|
|
||||||
|
|
||||||
let mut read_out = vec![0_u8; 1024];
|
let mut read_out = vec![0_u8; 1024];
|
||||||
let n = self.noise.read_handshake(&raw_vec[6..6 + noise_len], &mut read_out).map_err(|e| {
|
let n = self.noise.read_handshake(&raw_vec[6..6 + noise_len], &mut read_out).map_err(|e| {
|
||||||
ProtocolError::Crypto(format!("noise-read: {:?} (raw_len={}, noise_len={})", e, raw_vec.len(), noise_len))
|
ProtocolError::Crypto(format!("noise-read: {:?} (raw_len={}, noise_len={})", e, raw_vec.len(), noise_len))
|
||||||
|
|
@ -362,11 +363,11 @@ impl ProtocolMachine {
|
||||||
}
|
}
|
||||||
FrameKind::Resume => {
|
FrameKind::Resume => {
|
||||||
// 0-RTT: treat early data as application data
|
// 0-RTT: treat early data as application data
|
||||||
tracing::info!("0-RTT Resume frame received, processing early data");
|
tracing::debug!("0-RTT Resume frame received, processing early data");
|
||||||
ProtocolAction::DeliverApp(packet.header.stream_id, packet.payload)
|
ProtocolAction::DeliverApp(packet.header.stream_id, packet.payload)
|
||||||
}
|
}
|
||||||
FrameKind::Close => {
|
FrameKind::Close => {
|
||||||
tracing::info!("Received Close frame, terminating session");
|
tracing::debug!("Received Close frame, terminating session");
|
||||||
self.state = OstpState::Closed;
|
self.state = OstpState::Closed;
|
||||||
ProtocolAction::Noop
|
ProtocolAction::Noop
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -633,7 +633,7 @@ async fn handle_create_user(
|
||||||
return api_error::<String>("failed to save configuration");
|
return api_error::<String>("failed to save configuration");
|
||||||
}
|
}
|
||||||
|
|
||||||
tracing::info!("API: created user key {}", &key[..8.min(key.len())]);
|
tracing::info!("API: created user key (fp={})", crate::dispatcher::key_fp(&key));
|
||||||
(StatusCode::OK, ApiResponse::success(key))
|
(StatusCode::OK, ApiResponse::success(key))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -106,6 +106,15 @@ pub struct Dispatcher {
|
||||||
/// capping flood-driven trial work at TRIAL_RATE × num_keys crypto ops/sec.
|
/// capping flood-driven trial work at TRIAL_RATE × num_keys crypto ops/sec.
|
||||||
const TRIAL_RATE: f64 = 100.0;
|
const TRIAL_RATE: f64 = 100.0;
|
||||||
|
|
||||||
|
/// Short, non-reversible fingerprint of an access key for logs. The access key
|
||||||
|
/// is a shared secret, so it must never be written to logs verbatim; this lets
|
||||||
|
/// an operator correlate events without exposing the key itself.
|
||||||
|
pub(crate) fn key_fp(access_key: &str) -> String {
|
||||||
|
use sha2::{Digest, Sha256};
|
||||||
|
let h = Sha256::digest(access_key.as_bytes());
|
||||||
|
format!("{:02x}{:02x}{:02x}", h[0], h[1], h[2])
|
||||||
|
}
|
||||||
|
|
||||||
#[allow(dead_code)]
|
#[allow(dead_code)]
|
||||||
impl Dispatcher {
|
impl Dispatcher {
|
||||||
pub fn new(machine_config: ProtocolConfig, access_keys: Arc<RwLock<HashMap<String, crate::api::UserMeta>>>) -> Self {
|
pub fn new(machine_config: ProtocolConfig, access_keys: Arc<RwLock<HashMap<String, crate::api::UserMeta>>>) -> Self {
|
||||||
|
|
@ -289,7 +298,7 @@ impl Dispatcher {
|
||||||
let user_stats = self.get_or_create_user_stats(&access_key);
|
let user_stats = self.get_or_create_user_stats(&access_key);
|
||||||
if !key_valid || user_stats.is_over_limit() {
|
if !key_valid || user_stats.is_over_limit() {
|
||||||
tracing::info!("Dropping session {} for key {} (valid={}, over_limit={})",
|
tracing::info!("Dropping session {} for key {} (valid={}, over_limit={})",
|
||||||
session_id, access_key, key_valid, user_stats.is_over_limit());
|
session_id, key_fp(&access_key), key_valid, user_stats.is_over_limit());
|
||||||
self.drop_session(session_id);
|
self.drop_session(session_id);
|
||||||
return Ok(DispatchOutcome::Unauthorized);
|
return Ok(DispatchOutcome::Unauthorized);
|
||||||
}
|
}
|
||||||
|
|
@ -467,7 +476,7 @@ impl Dispatcher {
|
||||||
|
|
||||||
// Check traffic limit before accepting
|
// Check traffic limit before accepting
|
||||||
if user_stats.is_over_limit() {
|
if user_stats.is_over_limit() {
|
||||||
tracing::warn!("User {} exceeded traffic limit, rejecting handshake from {}", candidate_key, peer);
|
tracing::warn!("User {} exceeded traffic limit, rejecting handshake from {}", key_fp(&candidate_key), peer);
|
||||||
return Ok(DispatchOutcome::Unauthorized);
|
return Ok(DispatchOutcome::Unauthorized);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -303,7 +303,8 @@ pub async fn run_server(
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
UiEvent::KeyCreated { key } => {
|
UiEvent::KeyCreated { key } => {
|
||||||
tracing::info!("Access key created: {key}");
|
// Never log the access key verbatim — it's a shared secret.
|
||||||
|
tracing::info!("Access key created (fp={})", crate::dispatcher::key_fp(&key));
|
||||||
}
|
}
|
||||||
UiEvent::UnauthorizedProbe { peer, bytes } => {
|
UiEvent::UnauthorizedProbe { peer, bytes } => {
|
||||||
if debug {
|
if debug {
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue