fix(security): phase 1 hardening — log redaction, transport fixes, input safety

[iquidus]

Closes RND-559

• Redact sensitive payload from authenticated message log (handlers.go:25). Demoted to DEBUG level with only payload_len exposed, preventing key share leakage to log aggregation systems.

• Fix response body leaks in DKG/reshare commitment broadcasts (transport/client.go:205,241). Previously discarded both response and error with _, _; now closes response bodies and logs errors via structured logger.

• Add logger support to transport.Client with WithLogger() builder and nil-safe log() accessor. Replaces fmt.Printf usage with structured zap logging.

• Fix ECDSA attestation input mutation (ecdsa.go:159). Signature slice is now copied before modifying the recovery byte, preventing caller data corruption.

• Upgrade operator address mismatch on restore from warning to hard error (node.go:540). Prevents starting a node against mismatched persisted state which could cause cryptographic inconsistency.

[claude]

Claude finished @iquidus's task in 2m 57s —— View job

---

Code Review — fix(security): phase 1 hardening

• Gather context and understand PR changes
• Review pkg/attestation/ecdsa.go — input mutation fix
• Review pkg/node/handlers.go — log redaction
• Review pkg/node/node.go — address mismatch hard error
• Review pkg/transport/client.go — response body leak fixes + structured logging
• Review pkg/types/messages.go — message type changes
• Review pkg/util/util.go — utility changes
• Review client changes (kmsClient, web3signer)
• Review test coverage
• Post comprehensive review

---

Summary

This PR addresses five meaningful security-hardening issues. The fixes are well-motivated and generally well-implemented, with good test coverage. I found one silent regression that needs a fix before merge, one functional issue with the broadcast logger that needs to be wired up, and a handful of minor issues worth addressing.

---

Issues

🔴 Bug — WithLogger is defined but never called

transport.NewClient() is called at pkg/node/node.go:345 without chaining .WithLogger(n.logger):

// pkg/node/node.go:344-345
n.transport = transport.NewClient(operatorAddress, tps)
// ← missing: .WithLogger(n.logger)

This means every c.log().Sugar().Warnw(...) call added in BroadcastDKGCommitments and BroadcastReshareCommitments silently discards to the nop logger. The new structured log warnings introduced by this PR will never appear in production. The logger infra is correct; it just isn't wired up at the call site.

Fix this →

---

🟡 Bug — Duplicate "operator_address" key in two Debugw calls

pkg/clients/kmsClient/client.go has the same structured-log key twice in a single call, in two separate places:

// client.go:433-436 (CollectPartialSignatures)
c.logger.Sugar().Debugw("Collected partial signature from operator",
    "operator_index", idx,
    "operator_address", operatorAddr.Hex(),   // ← key used twice
    "operator_address", response.OperatorAddress)

// client.go:954 (collectPartialSignaturesForDecrypt)
c.logger.Sugar().Debugw("Collected partial signature",
    "operator_index", idx,
    "operator_address", operatorAddr.Hex(),   // ← key used twice
    "operator_address", response.OperatorAddress)

Zap includes both fields; most log aggregators silently drop or overwrite the first occurrence. The second "operator_address" (response.OperatorAddress) should be renamed to "response_operator_address" for clarity, consistent with the existing warn log at line 424.

Fix this →

---

🟡 Functional gap — Broadcast functions return nil on complete failure

Both BroadcastDKGCommitments and BroadcastReshareCommitments continue past per-peer failures and always return nil. If every peer rejects or times out, the caller receives no error while the DKG/reshare protocol silently stalls. The new structured logging helps, but it doesn't surface the failure to the caller.

Consider tracking a failure count and returning an error (or partial error via errors.Join) when zero or fewer-than-threshold peers accepted the broadcast:

// pkg/transport/client.go
successCount := 0
...
if resp.StatusCode == http.StatusOK {
    successCount++
}
...
if successCount == 0 {
    return fmt.Errorf("commitment broadcast failed: no peers accepted")
}

Fix this →

---

🟡 Debuggability — Non-OK broadcast response body is not read

When a peer returns a non-200 status in BroadcastDKGCommitments/BroadcastReshareCommitments, only the status code is logged. The rejection reason (which is usually in the response body) is discarded:

if resp.StatusCode != http.StatusOK {
    c.log().Sugar().Warnw("DKG commitment rejected",
        "peer", op.OperatorAddress.Hex(), "status", resp.StatusCode)
    // ← no body read; peer's error message lost
}
_ = resp.Body.Close()

Reading a bounded slice of the error body (e.g., io.ReadAll(io.LimitReader(resp.Body, 256))) would significantly improve incident response.

Fix this →

---

🟢 Minor — fmt.Sscanf with %s for challenge parsing is subtly fragile

parseChallenge in ecdsa.go:202 uses fmt.Sscanf(challenge, "%d-%s", ...). The %s verb stops at any whitespace, so a challenge containing a space before the nonce would silently truncate — the nonce length check would then catch it, but the error message would be "invalid nonce length" rather than "invalid challenge format", which is misleading.

A more explicit split would be:

// pkg/attestation/ecdsa.go
parts := strings.SplitN(challenge, "-", 2)
if len(parts) != 2 { ... }
timestamp, err := strconv.ParseInt(parts[0], 10, 64)
nonce = parts[1]

This is low risk in practice (nonces are hex-encoded), but makes intent clearer.

---

🟢 Minor — validateAuthenticatedMessage always returns nil for its third return value

pkg/node/handlers.go:43 has signature (...) (*AuthenticatedMessage, *OperatorSetPeer, interface{}, error) and always returns nil for the interface{} slot. All call sites use _, _, to discard it. This dead return slot is pre-existing but could be removed in a future cleanup to reduce confusion.

---

What's Well Done

ECDSA mutation fix (ecdsa.go:159-163) — copy(signature, request.Attestation) before modifying the recovery byte is exactly right. The TestVerifyDoesNotMutateAttestation and TestECDSAVerifyIdempotent tests directly validate both the fix and its invariant.

Log redaction (handlers.go:49) — Demoting authMsg.Payload to DEBUG and replacing it with payload_len is the correct approach. It preserves debugging utility (you can see traffic volume) without risking key share leakage to log aggregators that operate at INFO level.

Hard error on address mismatch (node.go:536-539) — The upgrade from Warnw to a hard error is the right call for a cryptographic safety check. The error message includes both addresses and a remediation hint. The common.HexToAddress() normalization on both sides before comparison (line 536) prevents EIP-55 case differences from causing false positives. Both the mismatch case and the case-insensitive success case have dedicated tests.

ValidateAppID (util/util.go:152-163) — Clean implementation: single compiled regex, informative errors, and sensible 255-byte cap. Applied consistently in both /secrets and /app/sign handlers.

Response body size limits (kmsClient/client.go, web3signer/client.go) — maxSuccessBodySize + io.LimitReader on the success path, and MaxErrorBodySize on the error path, prevents OOM from malicious large responses.

DeserializeFr range check (types/messages.go:68) — Catching values at or above the field modulus before they reach SetBigInt is important; gnark-crypto would silently reduce them mod p, corrupting share arithmetic.

---

Test Coverage Assessment

Coverage is strong for the new changes:

• TestVerifyDoesNotMutateAttestation + TestECDSAVerifyIdempotent — ECDSA mutation fix ✓
• TestRestoreState_OperatorAddressMismatch + TestRestoreState_OperatorAddressCaseInsensitive — address mismatch hard error ✓
• TestDeserializeFr — field element range validation ✓
• TestNodePersistence suite — persistence infrastructure ✓

Gap: No test exercises BroadcastDKGCommitments / BroadcastReshareCommitments with a failing peer to verify the new warning paths fire or that the WithLogger wiring works end-to-end.

---