Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions crates/node/src/handlers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ mod specialized_node_invite;
pub(crate) mod state_delta;
mod stream_opened;
pub(crate) mod tee_attestation_admission;
pub(crate) mod tee_attestation_throttle;

impl Handler<NodeMessage> for NodeManager {
type Result = ();
Expand Down
137 changes: 132 additions & 5 deletions crates/node/src/handlers/network_event/specialized.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
use actix::{AsyncContext, WrapFuture};
use actix::{ActorFutureExt, AsyncContext, WrapFuture};
use calimero_context_config::types::ContextGroupId;
use calimero_network_primitives::messages::NetworkEvent;
use calimero_network_primitives::specialized_node_invite::SpecializedNodeType;
use calimero_node_primitives::sync::BroadcastMessage;
use sha2::{Digest, Sha256};
use tracing::{debug, error, info, warn};

use crate::handlers::tee_attestation_throttle::Decision;
use crate::handlers::{specialized_node_invite, tee_attestation_admission};
use crate::run::NodeMode;
use crate::NodeManager;
Expand Down Expand Up @@ -143,13 +146,114 @@ pub(super) fn handle_specialized_broadcast(
"Received TEE attestation announce on namespace topic"
);

// Admission-control gates (TEE-01 / audit #48). The heavy
// `verify_attestation` path (outbound Intel-PCS fetch + DCAP
// verify) runs BEFORE any policy lookup, so an unguarded announce
// lets a malicious mesh peer amplify a 64 KiB gossip frame into a
Comment thread
xilosada marked this conversation as resolved.
// CPU verify + outbound PCS request by replaying a real quote under
// fresh nonces. Guard the spawn synchronously here, on the actor
// thread, before any verify work is scheduled.
Comment thread
xilosada marked this conversation as resolved.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 SHA-256 of the full quote bytes computed on the actor thread before throttle check

Sha256::digest(quote_bytes) hashes the entire quote (up to 64 KiB) synchronously on the NodeManager actor thread before the throttle is consulted. The throttle's Gate 1 (dedup) and Gate 2 (rate limit) are cheap map lookups that could reject the announce without needing the hash. Computing the hash first means a flood of rate-limited announces still pays the full SHA-256 cost on the actor thread, partially defeating the purpose of the throttle.

Suggested fix:

Move the `Sha256::digest` call to after the per-peer rate-limit check (Gate 2), or use a cheaper identity (e.g. the first 32 bytes of `quote_bytes` as a probabilistic key) for the dedup map and only compute the full hash when needed. Alternatively, accept the current approach as "good enough" given the actor-thread serialisation already limits concurrency, and document the trade-off.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documented at the hash site in 4f2f797. The hash can't be deferred behind the gates: quote_hash is the key for both the durable is_quote_hash_used check and the throttle's Gate 1 (dedup), so it must exist before either runs. It's bounded — quote_bytes is capped at gossipsub's 64 KiB default at the transport, and SHA-256 over ≤64 KiB is tens of microseconds, negligible beside the PCS-fetch + DCAP verify the hash gates. The comment now states this.

// `quote_hash` is the key for both the durable `is_quote_hash_used`
// check and the throttle's dedup gate, so it must be computed before

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 SHA-256 of quote bytes computed on the actor thread before throttle check — bounded but worth noting

The comment correctly notes that SHA-256 over ≤64 KiB is tens of microseconds. However, this computation happens synchronously on the NodeManager actor thread for every inbound TeeAttestationAnnounce, including those that will be immediately rejected by the throttle. A flood of 64 KiB frames (gossipsub's max) at high rate will keep the actor thread busy hashing before the cheap in-memory gates can reject. The gossipsub transport bound (64 KiB) and the actor's single-threaded nature mean this is bounded, but it is a small amplification that could be avoided by moving the hash computation to after the per-peer rate-limit check (Gate 2), since Gate 1 requires the hash but Gate 2 does not.

Suggested fix:

Restructure: check Gate 2 (per-peer rate limit) first using only `source`, then compute `quote_hash` and check Gate 1 (dedup) and Gate 3 (inflight). This eliminates the SHA-256 cost for rate-limited peers. Requires splitting `check()` into two phases or exposing a `check_rate_limit_only(source)` helper.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already documented at the hash site (commit 4f2f797). The hash can't be deferred behind the gates — it's the key for both Gate 1 (dedup) and the durable is_quote_hash_used check — and it's bounded: quote_bytes is capped at gossipsub's 64 KiB default, so SHA-256 is tens of µs, negligible beside the PCS-fetch + DCAP verify it gates. The in-code comment already states this; no change.

// either can run — it cannot be deferred behind the rate/inflight
// gates. This is acceptable: `quote_bytes` is bounded to gossipsub's
// 64 KiB default at the transport, and SHA-256 over ≤64 KiB is tens
// of microseconds, negligible beside the PCS-fetch + DCAP verify the
// hash exists to gate.
let quote_hash: [u8; 32] = Sha256::digest(quote_bytes).into();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Durable dedup check uses ContextGroupId but throttle uses raw [u8; 32] — the two dedup keys are computed from the same bytes but via different types, risking silent divergence

The durable is_quote_hash_used check constructs group_id = ContextGroupId::from(namespace_id_bytes) and passes &group_id. The in-memory throttle's check(now, source, namespace_id_bytes, quote_hash) passes the raw namespace_id_bytes: [u8; 32] directly. Both ultimately key on the same 32 bytes, but the two dedup mechanisms use different type wrappers. If ContextGroupId ever adds a transformation (e.g. a prefix or hash) to its From<[u8;32]> impl, the durable check would use a different key than the throttle, silently breaking the dedup invariant. This is a latent maintenance hazard rather than a current bug.

Suggested fix:

Pass `group_id.as_bytes()` (or equivalent) to `throttle.check` instead of `namespace_id_bytes` directly, so both checks always derive their key from the same `ContextGroupId` value. Alternatively, add a comment noting that `ContextGroupId::from` is a no-op newtype wrap and must remain so for the two keys to stay in sync.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 27c17bc — the throttle is now keyed on group_id.to_bytes(), i.e. the same ContextGroupId value the durable is_quote_hash_used check uses, so the two dedup keys are derived from one source and can't silently diverge. I confirmed ContextGroupId::from([u8;32]) is a transparent newtype wrap today (Self(Identity(value)), to_bytes returns the raw bytes), so this is behavior-preserving and just removes the latent hazard you flagged.

let group_id = ContextGroupId::from(namespace_id_bytes);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 SHA-256 of the full quote bytes computed on the actor thread before throttle check

Sha256::digest(quote_bytes) hashes the entire quote (up to 64 KiB) synchronously on the NodeManager actor thread, before the throttle gates have had a chance to reject the announce cheaply. A flood of unique-quote announces will therefore burn actor-thread CPU on hashing even when the per-peer rate limit or inflight cap would immediately reject them. Moving the hash computation to after the durable dedup check (which only needs group_id) and before Gate 1 of the in-memory throttle is the minimal fix; alternatively, pass the raw quote_bytes to the throttle and hash lazily inside check only when needed.

Suggested fix:

Reorder: perform the durable `is_quote_hash_used` check with a lazily-computed hash (compute it only once, after the topic parse succeeds), but consider whether the hash can be skipped entirely when the per-peer bucket is already empty (Gate 2 rejects before Gate 1 in the throttle). At minimum, document that the hash is computed unconditionally and is acceptable given the 64 KiB quote size.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 SHA-256 of quote_bytes computed before throttle but after info! log leaks timing

The quote_hash is computed via Sha256::digest(quote_bytes) on the actor thread (line 157), which is synchronous and O(quote_size). For a 64 KiB quote this is ~microseconds and acceptable. However, the info! log at line 148 fires unconditionally before any throttle check, meaning a flood of announces will produce an info! log entry per announce even for ones that will be immediately dropped by the durable dedup or throttle. Under a replay flood this could itself become a log-amplification vector. Consider moving the info! log to after the throttle gates pass (or downgrading it to debug! before the gates).

Suggested fix:

Move the `info!(... "Received TEE attestation announce on namespace topic")` log to after the throttle `Decision::Proceed` arm, or change it to `debug!` before the gates and `info!` only on proceed.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessed — no change. The info! is server-side structured logging, not part of any response, so it isn't an attacker-observable side channel: there's no response-timing differential exposed to the caller (the announce path returns nothing to the peer; gossip delivery is fire-and-forget). The hash itself is bounded (≤64 KiB, tens of µs) and is the required dedup key. Reordering the log wouldn't change any externally-observable timing.


// In-memory admission gates FIRST — per-group quote dedup + per-peer
// rate limit + global inflight-verify cap — so a flood is rejected on
// cheap in-memory state before we touch the store below. The returned
// permit must outlive the verify, so it is moved into the spawned
// task and held until completion. Key the throttle on
// `group_id.to_bytes()` so it and the durable check share one
// `ContextGroupId`-derived key (no divergence if the repr changes).
let now = std::time::Instant::now();
let verify_permit = match this.tee_admission_throttle.check(
now,
source,
group_id.to_bytes(),
quote_hash,
) {
Decision::Proceed(permit) => permit,
Decision::Duplicate => {
debug!(
%source,
quote_hash = %hex::encode(quote_hash),
"Dropping TeeAttestationAnnounce: recently-seen quote (dedup)"
);
return true;
}
Decision::RateLimited => {
warn!(
%source,
"Dropping TeeAttestationAnnounce: per-peer attestation rate limit exceeded"
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Durable store check consumes a throttle token even on a store-hit early return

When calimero_governance_store::is_quote_hash_used returns Ok(true), the handler returns true early (line ~200), dropping verify_permit. The inflight semaphore permit is correctly released. However, the per-peer token and the dedup entry were already committed by tee_admission_throttle.check() before this store read. A quote that is already admitted (common after a node restart with an existing fleet) will burn one token and occupy a dedup slot on every re-announce until the TTL expires, even though no verify work is done. Over a 300 s TTL with a burst of 5 tokens, a restarting fleet member that re-announces 5 times will exhaust its per-peer budget for 5 * 2 s = 10 s. This is unlikely to cause a real admission failure (the fleet-join window is 30 s and the refill rate is 1 token/2 s), but it is an unintended side-effect. Consider calling tee_admission_throttle.forget_quote (and ideally restoring the token) on the Ok(true) store-hit path, analogous to the transient-error path.

Suggested fix:

After the `Ok(true)` early return, call `this.tee_admission_throttle.forget_quote(group_id.to_bytes(), quote_hash)` so the dedup slot is freed. Optionally expose a `restore_token` method on the throttle to also return the consumed token.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessed — documented the trade-off in 60057c6; keeping the throttle-before-store order. This is the flip side of the reorder you (correctly) asked for last round: an already-admitted quote that is re-announced burns one per-peer token before the store read reveals it's admitted. It's bounded (the dedup entry the throttle just recorded suppresses the rest of the burst — subsequent re-announces hit Duplicate, no token, no store read) and rare (a node stops announcing once admitted; fleet-join breaks on admission). Your own numbers bear this out: 5 re-announces = 10s of budget inside a 30s window with 1 token/2s refill, so no real admission risk.

I deliberately did not take the forget_quote-on-admitted route: dropping the entry would re-open a store read on every following re-announce (defeating the reorder), and restoring the token has the same effect. Keeping the entry is what makes the follow-ups cheap. A dedicated AlreadyAdmitted decision variant would need the throttle to perform the store read itself, coupling it to the datastore — net worse. The one-token cost on a rare path is the right price.

return true;
}
Decision::AtCapacity => {
warn!(
%source,
"Dropping TeeAttestationAnnounce: attestation verify capacity saturated"
);
return true;
}
};

// Durable dedup (a store read), pulled earlier from `admit_tee_node`:
// a quote already admitted to this group never needs re-verifying.
// Run it only AFTER the cheap in-memory gates above, so an
// unauthenticated announce flood can't drive a store read per frame.
// A read error is non-fatal — the authoritative check still runs in
// `admit_tee_node` — so we proceed rather than drop a possibly-
// legitimate announce. On a hit we drop here: `verify_permit` is
// released as it goes out of scope, and the dedup entry the throttle
// just recorded keeps suppressing further re-announces of this quote.
//
// Trade-off of throttle-before-store: an already-admitted quote that
// is re-announced burns one per-peer token before we learn it's
// admitted. That is bounded (the dedup entry suppresses the rest of
// the burst) and rare (a node stops announcing once admitted), and it
// is the price of not doing a store read for every flood frame. We
// deliberately do NOT `forget_quote` here — keeping the entry is what
// makes the follow-up re-announces cheap — nor restore the token,
// which would just re-open the store read on the next frame.
match calimero_governance_store::is_quote_hash_used(
&this.datastore,
&group_id,
&quote_hash,
) {
Ok(true) => {
debug!(
%source,
quote_hash = %hex::encode(quote_hash),
"Dropping TeeAttestationAnnounce: quote already admitted to group"
);
return true;
}
Ok(false) => {}
Err(err) => {
warn!(
%source,
error = %err,
"Failed to read quote-hash usage; proceeding to verify"
);
}
}

let context_client = this.clients.context.clone();
let quote_bytes = quote_bytes.clone();
let public_key = *public_key;
let nonce = *nonce;
let group_key = group_id.to_bytes();
let _ignored = ctx.spawn(
async move {
if let Err(err) = tee_attestation_admission::handle_tee_attestation_announce(
// Hold the inflight permit for the lifetime of the verify so
// the global concurrency cap stays accurate; dropped here.
let _verify_permit = verify_permit;
tee_attestation_admission::handle_tee_attestation_announce(
&context_client,
source,
quote_bytes,
Expand All @@ -158,15 +262,38 @@ pub(super) fn handle_specialized_broadcast(
namespace_id_bytes,
)
.await
{
}
.into_actor(this)
.map(move |result, actor, _ctx| {
if let Err(err) = result {
warn!(
%source,
error = %err,
"Failed to handle TEE attestation announce"
);
// The verify *errored* (a transient infra failure such
// as an Intel-PCS fetch error — an invalid quote instead
// returns Ok above). Forget the dedup entry so a
// re-announce of the same quote bytes (the fleet-join
// re-announce loop reuses the identical payload) can be
// re-verified rather than being suppressed for the full
// dedup TTL — which, at TTL ≫ the 30s admission window,
// would otherwise sink the whole join on one PCS hiccup.
// A genuinely invalid quote stays deduped, and the
// per-peer rate limit + inflight cap still bound any
// replay-driven re-verification.
//
// This `.map` runs only if the spawned future completes.
// The sole way it is dropped first is the actor stopping
// — but the throttle (and its in-memory dedup map) lives
// on this actor, so it is torn down together; a stale
// entry can't outlive its map to suppress a post-restart
// re-announce (a restarted node starts with an empty map).
actor
.tee_admission_throttle
.forget_quote(group_key, quote_hash);
}
}
.into_actor(this),
}),
);
true
}
Expand Down
14 changes: 14 additions & 0 deletions crates/node/src/handlers/tee_attestation_admission.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,20 @@ pub(crate) fn public_key_binding_hash(public_key: &PublicKey) -> [u8; 32] {
///
/// Verifies the TDX quote, checks measurements against the group's TEE admission
/// policy, and publishes a `MemberJoinedViaTeeAttestation` governance op if valid.
///
/// # Error semantics (load-bearing)
///
/// Returns `Ok(())` for both a successful admission **and** a definitively
/// rejected quote (verification ran and returned an invalid result). Returns
/// `Err` only for a *transient* failure where the verify could not be completed
/// — most importantly the outbound Intel-PCS collateral fetch erroring, but also
/// store/governance publish failures. The caller in the
/// `network_event::specialized` handler relies on this distinction:
/// it clears the throttle's in-memory dedup entry on `Err` (so a legitimate
/// re-announce can be re-verified after a transient outage) but keeps it on
/// `Ok` (so a replayed/invalid quote stays suppressed). Do not start returning
/// `Err` for an invalid-but-successfully-evaluated quote without updating that
/// caller.
pub async fn handle_tee_attestation_announce(
context_client: &calimero_context_client::client::ContextClient,
source: libp2p::PeerId,
Expand Down
Loading
Loading