Device authentication v2 for offline direct and relay Atems

[guohai]

What changed

• classify direct peers from the kernel socket address and allow owner-only loopback proof without interactive pairing
• require challenge-bound HMAC proof or explicit pairing for LAN and identity-relay Atems before registration, app messages, broadcasts, or credential sync
• share device sessions across direct and relay transports, bind sessions to stable Atem IDs, and route broadcasts to authenticated clients on both transports
• bind relay authentication and routing to server-generated socket generations, close replacements, and reject stale messages and responses
• confine direct socket state to NIO and relay auth/pairing state to main; bound and sanitize untrusted device labels
• protect identity/session/bootstrap/log files, reject unsafe session-store paths, and recursively redact structured and malformed credential payloads
• document the loopback/LAN/relay matrix, rollout contract, and remaining security work

Practical coverage

• real NIO/WebSocket loopback authentication with no network service
• forged loopback proof and LAN-token cross-scope rejection
• five concurrently authenticated loopback clients
• paired authentication through this Mac's real non-loopback LAN interface without relay
• unauthenticated sockets excluded from application broadcasts
• real Axum/WebSocket replacement with stale-response rejection and generation-bound connect/disconnect events
• cross-language HMAC vector, legacy device binding, device-label bounds, secure file migration/symlink refusal, and log redaction

Verification

• swift test (148 passed)
• cd relay-server && cargo test (180 passed)
• node --test --test-concurrency=1 .github/scripts/*.test.mjs (32 passed)
• git diff --check

Coordinated rollout

Requires Agora-Build/Atem#15. Merge both repositories before releasing either client. Old clients that send a session ID without proof cannot authenticate against v2.

Security status

Direct LAN still uses plaintext ws://. HMAC prevents session-ID-only impersonation, but it does not protect first pairing or application traffic from LAN inspection/MITM. Do not call LAN production-ready until WSS certificate pinning lands. Relay-owner authentication, authenticated Voice/LLM/Vault/RTC APIs, direct unauthenticated connection/pairing admission bounds, and device revocation/rotation remain tracked blockers.

Generated with SMT smt@agora.build

[github-actions]

Claude Code Review

Code Review: Device Authentication v2

Overall this is a well-structured, security-focused change with strong test coverage. Below are concrete findings.

Security / Logic Findings

1. Loopback scope classification ignores IPv4-mapped and other loopback forms inconsistently

Sources/Menubar/AstationWebSocketServer.swift — DirectConnectionScope.init

case "127.0.0.1", "::1", "0:0:0:0:0:0:0:1", "::ffff:127.0.0.1":
    self = .loopback

The IPv4 loopback range is 127.0.0.0/8, not just 127.0.0.1. A connection whose kernel peer address is e.g. 127.0.0.2 (a legitimate loopback address on macOS) is classified as .lan, not .loopback. This is a fail-closed direction (over-restrictive, not a bypass), so it is not a vulnerability, but it is a correctness/edge-case bug that could break legitimate same-Mac clients bound to non-.1 loopback addresses. Consider matching the full 127.0.0.0/8 block and ::ffff:127.0.0.0/8.

More importantly, peerAddress comes from channel.remoteAddress?.ipAddress. If it is ever nil, the default branch yields .lan, which is correctly fail-closed. Good.

2. pushCredentials sends credentials to relay clients without re-checking auth at send time

Sources/Menubar/AstationHubManager.swift — pushCredentials / sendMessage

sendMessage(_:to:expectedRelayConnectionId:) guards relay sends against a connection generation mismatch, but does not re-verify isAuthenticated. The credential push relies on addClient having verified authentication earlier. Since credentials (credentialSync) are the most sensitive payload, and the relay send path in sendHandler does gate on isAuthenticated(...) || isRelayAuthenticationControl(message), credentialSync is not an auth-control message and will be correctly gated there. This is consistent — but the double path (sendMessage for connection-generation, sendHandler for auth) makes the invariant non-obvious. Worth a comment or a single choke point to avoid a future regression where a caller bypasses sendHandler's gate.

3. Pairing dialog runs a modal NSAlert on the main thread while holding relay state assumptions

Sources/Menubar/AstationHubManager.swift — handleIdentityRelayAuthentication

let alert = NSAlert()
...
guard alert.runModal() == .alertFirstButtonReturn else { ... }
guard identityRelayAuthentication.connectionId(for: clientId) == connectionId else { ... }

runModal() blocks the main queue synchronously. During that block, no other main-queue relay events (connect/disconnect/new hello) can be processed, so the post-modal connectionId re-check can only observe state as it was before the modal opened — meaning a connection that is replaced during the modal will not be detected until after approval completes, but since the re-check runs on the same serialized queue after the modal returns and events are still queued behind it, the replacement event is processed after this handler finishes. The re-check therefore cannot catch an in-flight replacement. The finishIdentityRelayAuthentication authenticate(...) call does re-validate connectionIds[clientId] == connectionId, so a stale approval is ultimately rejected there. This is safe, but the intermediate connectionId guard is effectively dead code given the serialized queue — confirm the intended threading model. (The direct-socket path in showPairingDialog correctly re-dispatches to ws.eventLoop and re-checks connectedClients[clientId].)

4. deviceLabel truncation can split a grapheme cluster / combining sequence

Sources/Menubar/DeviceAuthentication.swift — deviceLabel

Truncation is done per-unicodeScalar with a byte budget. A multi-scalar grapheme (emoji with modifiers, combining marks) can be cut mid-cluster, producing a mangled label. This is cosmetic (labels are already sanitized and non-security-critical), but if labels are ever displayed in the pairing dialog (they are: "Device: \(hostname)"), a maliciously crafted label could still render oddly. Not a vulnerability given control chars are stripped; noting for robustness.

5. isRelayAuthenticationControl allows status == "auth" messages to be sent to unauthenticated relay clients

Sources/Menubar/AstationHubManager.swift

return status == "auth_required" || status == "authenticated" || status == "auth" || status == "error"

"auth" is the client→server proof message name, yet it is whitelisted for server→client sends to unauthenticated relay clients. Astation never legitimately sends a status: "auth" message outbound (it sends auth_required, authenticated, error, or .auth(...)). Including "auth" here is harmless today but broadens the pre-auth send surface unnecessarily. Recommend removing "auth" unless there is a concrete outbound use.

Performance

6. getConnectedClientsCount() blocks with .wait() off the event loop

Sources/Menubar/AstationWebSocketServer.swift

return (try? eventLoop.submit { self.connectedClients.count }.wait()) ?? 0

If this is ever called from the state event loop through an indirect path, .wait() would deadlock. The inEventLoop check guards the direct case, but any nested call originating on that loop but not detected as inEventLoop (it will be detected) is fine. Low risk, but .wait() on a shared single-thread EventLoopGroup is fragile if this method is called from status-bar refresh timers frequently — it serializes UI polling against network I/O. Consider caching the count in an atomic updated on the loop.

Code Quality / Maintainability

7. Redaction regex for unstructured text may miss keys and is order-dependent

Sources/Menubar/NetworkDebugLogger.swift — sanitizeUnstructuredText

The sensitiveKeys set (used for JSON) and the regex alternation (used for text) are maintained separately and can drift. For example, authorization, cookie, credential, appcertificate, encryptionkey are in sensitiveKeys but not in the unstructured-text regex alternation. A malformed/non-JSON log line containing authorization: <secret> or cookie=<secret> would not be redacted by sanitizeUnstructuredText. Given the truncated-JSON test case exists specifically because malformed payloads fall through to this path, this is a real redaction gap. Recommend deriving both from a single source or adding the missing keys to the regex.

8. sanitizedPayload on large non-JSON strings runs two regex passes over full text before truncating

Sources/Menubar/NetworkDebugLogger.swift

sanitizeUnstructuredText runs regex substitution over the entire (potentially large) payload and truncates only at the end. For very large binary-ish strings that decode as UTF-8 but aren't JSON, this is O(n) regex work per log call. Minor, but debug logging should avoid unbounded work; consider truncating before regex where correctness allows.

Tests

9. No test for LAN cross-scope rejection in the reverse direction

Tests/AstationTests/DirectConnectionTests.swift

There is testLANSessionTokenCannotAuthenticateAsLoopback (LAN token used on loopback scope) but no test asserting a loopback bootstrap-token proof is rejected on the LAN scope (i.e., a session_id/atem_id/proof built from the bootstrap token cannot pass sessionStore.authenticate on a LAN connection). The description claims "LAN-token cross-scope rejection"; the inverse direction strengthens the matrix and is cheap to add.

10. No test covering 127.0.0.2-style loopback classification

Given finding #1, a test that a 127.0.0.0/8 non-.1 address is (or intentionally is not) treated as loopback would document the intended behavior.

Non-findings / Positives

• Constant-time proof comparison (constantTimeEqual) and strict hex-proof validation are correct.
• File-permission hardening (0600/0700), symlink refusal, and owner checks in LocalBootstrapStore/SessionStore are thorough and well-tested.
• Connection-generation binding (IdentityRelayAuthenticationState) with challenge invalidation on reconnect is sound and tested.
• Broadcasts correctly restricted to authenticatedClients on the direct path.

Most findings are hardening/robustness rather than exploitable bypasses. Findings #1 and #7 are the most actionable: the loopback CIDR narrowing (correctness/regression risk for legitimate clients) and the unstructured-text redaction gap (real secret-leak risk in malformed debug logs).

[github-actions]

Codex Code Review

Findings

• Medium: Sources/Menubar/AstationWebSocketServer.swift:222 allows unbounded direct pairing prompts. Any LAN client can open many unauthenticated sockets and send a syntactically valid statusUpdate(auth) with pairing_code, hostname, and atem_id; each connection inserts its own clientId into pairingClients and schedules a modal NSAlert on the main thread. pairingClients only prevents duplicate prompts per socket, not total pending prompts or rate, so a LAN attacker can flood/block the UI and make Astation unusable. Add a global pending-pairing cap/rate limit for direct sockets, expire pending pairing attempts, and close/reject excess attempts before dispatching UI.

Residual Risk

• I only found one concrete issue in the supplied patch context. Relay/LAN plaintext and relay-owner authentication are documented as remaining blockers, so I did not count them as PR regressions.