fix(tee): make expected app hash mandatory in attestation verification

[chefsale]

VerificationResult::is_valid() used application_hash_verified.unwrap_or(true),
so a quote verified without an expected app hash (None) skipped the
application/identity binding check yet was still reported as valid — a
default-open verification path.

Make the binding mandatory end to end:

• verify_attestation / verify_mock_attestation now take a required
  &[u8; 32] expected app hash instead of Option<&[u8; 32]>, and
  VerificationResult::application_hash_verified is a plain bool.
• The specialized-node-invite flow now binds the attestation to the
  requester's public key hash on both the generation and verification
  sides (it previously passed None).
• The admin verify_quote endpoint now requires expectedApplicationHash
  in the request and reports applicationHashVerified as a bool.

Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01Q76M7k3Lf5nMxe3k9AMegm

[meroreviewer]

🤖 MeroReviewer

Reviewed by 1 agents | Quality score: 28% | Review time: 54.7s

🟡 1 warnings. See inline comments.

---

_{🤖 Generated by MeroReviewer | Review ID: review-d9e92ed6}

[meroreviewer]

🟡 Sha256::digest dereferences PublicKey — verify the deref yields the raw key bytes

The code calls Sha256::digest(*our_public_key) where our_public_key is a calimero_primitives::identity::PublicKey. The * dereference relies on Deref<Target=[u8;32]> (or similar) being implemented for PublicKey. If PublicKey serialises to a different byte representation when dereferenced (e.g. a compressed point vs. raw bytes), the hash produced here and the hash produced in handle_verification_request (line 180, Sha256::digest(*public_key)) must agree — but they operate on values that arrived over the network from different sources. If the two PublicKey values are not byte-for-byte identical (e.g. one is normalised, the other is not), the binding check will always fail. More critically, if the deref yields something other than the canonical 32-byte key material, the binding is weaker than intended. The same pattern is used in tee_attestation_admission.rs line 22. This should be verified against the PublicKey implementation.

Suggested fix:

Explicitly call the canonical serialisation method (e.g. `our_public_key.as_bytes()` or `our_public_key.to_bytes()`) rather than relying on implicit `Deref`, and add a comment confirming which byte representation is used.

[meroreviewer]

Documentation Review

The following documentation may need updates based on the changes in this PR:

• 🟡 architecture/crates/node.html: Files matching crates/node/** were changed but architecture/crates/node.html was not updated (per source_to_docs_mapping).
• 🟡 architecture/crates/server.html: Files matching crates/server/** were changed but architecture/crates/server.html was not updated (per source_to_docs_mapping).
• 🟡 architecture/crates/sync.html: Files matching crates/node/** were changed but architecture/crates/sync.html was not updated (per source_to_docs_mapping).
• 🟡 architecture/crates/tools.html: Files matching crates/merod/** were changed but architecture/crates/tools.html was not updated (per source_to_docs_mapping).
• 🟡 architecture/tee-mode.html: Files matching crates/tee-attestation/** were changed but architecture/tee-mode.html was not updated (per source_to_docs_mapping).

[cursor]

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 2 agents | Quality score: 39% | Review time: 203.6s

🟡 1 warnings. See inline comments.

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-3a970150}

[meroreviewer]

🟡 Sha256::digest dereferences PublicKey — correctness depends on Deref target

Sha256::digest(*our_public_key) dereferences PublicKey (a newtype over [u8; 32]) to get the raw 32-byte key material. The verifier side (line 180) does the same with *public_key. This is consistent, but the hash input is the raw 32-byte compressed public key bytes, not the canonical serialization (e.g. protobuf or base58). If PublicKey's Deref impl ever changes, or if a different serialization is used elsewhere for the same binding purpose, the two sides will silently diverge. A comment explaining the exact byte layout being hashed would prevent future breakage.

Suggested fix:

Add a comment: `// SAFETY: *public_key yields the raw 32-byte ed25519 public key; both sides must use the same layout.` and consider a named helper function to make the contract explicit.

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 2 agents | Quality score: 44% | Review time: 228.7s

---

🟡 Warning (4)

1. Debug format used for bool field in tracing event

File: crates/node/src/handlers/specialized_node_invite.rs (line 218-218) | Consensus: 1/2 agents

The application_hash_verified field is logged with ? (Debug format) via app_hash_verified = ?verification_result.application_hash_verified, but the field is now a plain bool (not Option<bool>). Using % (Display) is more idiomatic and consistent with the other fields in the same event (quote_verified and nonce_verified both use %-style). The ? format works but produces true/false wrapped in debug output, which is inconsistent.

Suggested fix:

Change `app_hash_verified = ?verification_result.application_hash_verified` to `app_hash_verified = verification_result.application_hash_verified` (or `%verification_result.application_hash_verified`) to match the style of the adjacent fields.

Found by: patterns-reviewer

2. app_hash_verified logged with Debug format after type change to bool

File: crates/node/src/handlers/specialized_node_invite.rs (line 218-218) | Consensus: 1/2 agents

At line 218, application_hash_verified is logged with ? (Debug) format: app_hash_verified = ?verification_result.application_hash_verified. Since application_hash_verified is now a plain bool (not Option<bool>), using ? is harmless but inconsistent — every other call site uses % or a plain field. More importantly, the field was changed from Option<bool> to bool in VerificationResult, so the ? format will print true/false with quotes in some formatters. This is a minor inconsistency but could confuse log consumers expecting a bare boolean.

Suggested fix:

Change `app_hash_verified = ?verification_result.application_hash_verified` to `app_hash_verified = verification_result.application_hash_verified` (drop the `?`).

Found by: security-reviewer

3. Error message still formats application_hash_verified with {:?} after type change to bool

File: crates/merod/src/kms/mod.rs (line 1049-1053) | Consensus: 1/2 agents

The bail! in verify_kms_attestation formats verification_result.application_hash_verified with {:?} (line ~1051: app_hash_verified={:?}). Now that the field is a plain bool rather than Option<bool>, {:?} still compiles but is inconsistent — {} or just embedding the value directly is the right choice. More importantly, the message says "app_hash_verified={:?}" which will print true/false without the Some(…) wrapper, so the format string is now misleading (it implies an optional).

Suggested fix:

Change `app_hash_verified={:?}` to `app_hash_verified={}` in the bail! format string.

Found by: patterns-reviewer

4. verify_attestation continues and returns a result even when quote parsing or collateral fetch fails

File: crates/tee-attestation/src/verify.rs (line 130-135) | Consensus: 1/2 agents

In verify_attestation, if TdxQuote::from_bytes fails the function returns an Err immediately (correct). However, if the DCAP verify() call fails (line ~105), quote_verified is set to false but execution continues: nonce and app-hash are still checked against report_data extracted from the parsed quote. The function then calls Quote::try_from(tdx_quote) and returns an Ok(VerificationResult) with quote_verified=false. A caller that checks only is_valid() will correctly reject it, but a caller that inspects individual fields (e.g. for logging or policy) receives a VerificationResult whose nonce_verified and application_hash_verified fields are derived from an unverified quote — the report_data could have been tampered with. This is pre-existing but the PR's removal of the Option wrapper makes the semantics more load-bearing.

Suggested fix:

When `quote_verified` is `false`, set `nonce_verified = false` and `application_hash_verified = false` as well (or return early with all-false fields), so callers cannot accidentally trust individual sub-fields of an unverified quote.

Found by: security-reviewer

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-fb24ed4a}