fix(client): retain deployment TLS identity and enforce HTTPS

[chris-ricketts]

Support ExpectedTLSKeyHash and retain the first verified fingerprint across attestation refreshes and reconnects. Activate verified state and its pin atomically, preserving prior state on verification failure.

Reject plaintext requests and downgrade redirects, and prevent gRPC dial options from overriding pinned credentials. Add regression tests and document fingerprint persistence across accepted image changes.

Fixes #189

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

[arkana-ai-bot]

Arkana review — #190

Overall: The three core security fixes are correct and the test coverage is thorough. Flagging one medium usability trap and one low-priority asymmetry before merge.

---

Security analysis

✅ Atomic pin activation (client/client.go:373–389)

The pre-existing race — where setPinHash fired before PCR checks completed, leaving a bad hash pinned on PCR failure — is correctly resolved. Moving the fingerprint check and cachedState = result assignment inside a single mu.Lock() block means: (a) prior state is untouched on any verification failure, and (b) no intermediate state is visible to concurrent readers. The TOFU commit race is also correctly handled: first committer wins; later goroutines whose hash differs fail with a mismatch error rather than silently overwriting. TestConcurrentVerificationRetainsFirstCommit is deterministic (not timing-dependent) and validates this path correctly.

✅ gRPC credential ordering fix (client/grpc.go:55–58)

Old code put WithTransportCredentials first and appended caller opts; in gRPC-go, the last WithTransportCredentials wins, so callers could accidentally (or deliberately) override the pin with insecure.NewCredentials(). The three-index slice opts[:len(opts):len(opts)] both avoids mutating the caller's backing array and ensures the pinned credentials are always appended last. TestRetainedPinAcrossConnections validates the override is blocked.

✅ HTTPS enforcement on redirects (client/client.go:307–323)

httpsTransport.RoundTrip is called by net/http's redirect machinery for every hop, so a 301→http:// or 307→http:// redirect is caught before application headers or the replayed body reach the plaintext server. TestHTTPSOnly covers all five redirect status codes across all three client modes (verified, pinned, insecure-TLS), with atomic counters proving zero bytes reached the HTTP server. Correct.

✅ Defensive copies (client/client.go:245, client/client.go:350–353)

slices.Clone(opts.ExpectedPCRs) prevents the caller from mutating the PCR expectation list after New returns. maps.Clone(result.PCRs) in VerifyAttestation prevents callers from corrupting the client's internal PCR map. TestClientVerificationStateIsCallerIndependent validates both paths.

✅ extractTLSKeyHash already guards the all-zero case

verify.go:163 returns an error for all-zero hashes before reaching the lock block. There is no path by which an empty or all-zero tlsKeyHash can be committed as the TOFU anchor.

---

Findings

Medium — requireHTTPS closes caller-owned body in Do() (client/client.go:309–316, 331–333)

requireHTTPS calls req.Body.Close() on the caller's request when the URL scheme is not HTTPS. For the redirect case (called from httpsTransport.RoundTrip), closing the body is correct and necessary to prevent resource leaks. But Do() is a public API that takes an *http.Request owned by the caller:

// client.go
func (c *Client) Do(ctx context.Context, req *http.Request) (*Response, error) {
    if err := requireHTTPS(req); err != nil {
        return nil, err  // req.Body has already been closed here
    }

If a caller passes an HTTP URL by mistake (a programming error, but a plausible one during development), req.Body is closed before the error is returned. The caller cannot safely retry with a corrected request URL. Consider not closing the body in the Do() guard — only close it in the transport-layer call where the request is owned by the redirect machinery — or document the closure behaviour explicitly.

Low — GRPCConn snapshots the pin at call time; HTTP reads it live (client/grpc.go:44–48)

// GRPCConn: pin is captured at call time
VerifyPeerCertificate: func(rawCerts [][]byte, _ [][]*x509.Certificate) error {
    return verifyLeafCertPin(rawCerts, attest.TLSKeyHash) // snapshot
},

// HTTP mainTransport: pin is read live on every handshake
VerifyPeerCertificate: func(rawCerts [][]byte, _ [][]*x509.Certificate) error {
    return verifyLeafCertPin(rawCerts, c.currentTLSHash()) // live read
},

Both produce the same value in this design (the deployment TLS key is immutable after TOFU enrollment), so there is no functional difference today. However, the asymmetry is likely to confuse future readers and could become a subtle bug if coordinated key rotation is ever added. A one-line comment in GRPCConn explaining that attest.TLSKeyHash is intentionally snapshotted at connection time (and why this is equivalent to the live read) would make the intent durable.

---

Minor / informational

CLI per-invocation TOFU (cmd/enclave/curl.go:116–120): The CLI creates a client without ExpectedTLSKeyHash, so each invocation re-enrolls. The README acknowledges this ("The CLI currently establishes first-use trust per invocation"). PCR0 still guards the bootstrap attestation on each call, so this is not a regression — but it means CLI users get weaker deployment-identity guarantees than library users who persist the fingerprint. Worth a follow-up issue if CLI fingerprint persistence is ever desired.

CacheTTL documentation fix: The old docstring ("Set to 0 to verify on every request") was incorrect — 0 already defaulted to 60 s. The new documentation ("Zero defaults to 60 s; a negative duration verifies on every request") matches the code. No behaviour change, but the correction is welcome.

url.Parse normalization: New now validates the base URL via url.Parse and stores endpoint.String() rather than the raw input. This canonicalizes the URL (e.g., trailing slashes, encoding). GRPCConn then strips https:// via strings.TrimPrefix, which is correct now that the scheme is guaranteed to be https. No issue.

Cross-repo impact: go-sdk and other SDK repos do not import ArkLabsHQ/enclave/client directly. cmd/enclave/curl.go calls client.New without the new options (TOFU-per-invocation, acceptable for CLI use). No cross-repo breakage.

vendorHash change (flake.nix:68): Consistent with the new grpc/health test dependency (subpackage of google.golang.org/grpc, no separate go.mod entry required).

---

Summary

The atomic state fix, gRPC credential ordering fix, and HTTPS redirect enforcement are all correctly implemented and tested. The requireHTTPS body-close side effect in the public Do() API is worth fixing before merge. The gRPC snapshot comment is low-priority but would pay for itself the first time someone touches that code path.

[arkana-ai-bot]

Follow-up pass — 9a8cbf1 (one commit since 13569dc)

One trivial infrastructure commit; 0 prior issues fixed; 2 still open.

---

Incremental diff (13569dc → 9a8cbf1)

Single commit 9a8cbf1 ("chore: enclave-cli vendorHash") — flake.nix:68 only:

-vendorHash = "sha256-UpYTFzOGPnETUaftmz0SwC/gDKbeUWj5zeYULCAsyf8=";
+vendorHash = "sha256-g9ywOTYQX+FzU+6+Pa4SaOtqreM++CGHxIfmvXAqVsU=";

This is the expected follow-on to the grpc/health test-dependency addition in the main commit. The hash covers the enclave-cli Nix package's Go vendor directory; updating it after a go.sum change is mandatory and correct. No functional code change.

---

Prior-finding status

Medium — requireHTTPS closes caller-owned body in Do() (client/client.go:309–316, 331–333)
Still open. client/client.go was not touched in this increment. The same body-close side effect on the public Do() API remains. This is the only finding I'd want resolved before merge.

Low — GRPCConn snapshots pin at call time; HTTP reads it live (client/grpc.go:44–48)
Still open. client/grpc.go was not touched. No functional impact in current design, but the explanatory comment is still absent.

---

No new findings

The vendorHash change introduces no logic, no new API surface, and no new dependencies beyond what was already reviewed. No cross-repo impact.

[arkana-ai-bot]

Review: fix(client): retain deployment TLS identity and enforce HTTPS

Security-relevant client library fix — please ensure a human reviews the TLS pinning model before this ships to consumers.

What it fixes

1. TLS fingerprint not retained across reconnects/refreshes. Previously, cachedState and pinHash were separate; the pin was updated even on failed verification. Now the whole cachedState (including TLSKeyHash) is activated atomically with the verified state — failed verification leaves the prior state unchanged.

2. HTTP downgrade not rejected. The client now refuses plaintext http:// base URLs at construction, and rejects redirects that downgrade to HTTP or change the host, including POST-body-replaying redirects. PinnedHTTPClient enforces the same requirement.

3. gRPC dial options could override pinned credentials. Caller-supplied dial options are now prevented from replacing the pinned TLS credentials.

4. New ExpectedTLSKeyHash option. Allows callers to persist and supply the deployment TLS public-key fingerprint, preserving deployment identity across client lifetimes even when accepting a successor PCR0. Empty = TOFU (trusts first verified attestation). Mutually exclusive with InsecureTLS.

Implementation notes

• New() validates the base URL is a proper HTTPS URL at construction time — good fail-fast.
• ExpectedTLSKeyHash and InsecureTLS are mutually exclusive and enforced at construction.
• opts.ExpectedPCRs = slices.Clone(...) prevents callers from mutating the slice after construction — good defensive copy.
• Minor semantic change: CacheTTL == 0 now defaults to 60s rather than verifying every request; negative durations verify every request. Existing callers who explicitly set CacheTTL: 0 expecting per-request verification will silently get 60s instead. Worth a note in the changelog.

Verdict

The changes are all security improvements. The TOFU model for ExpectedTLSKeyHash is documented clearly. Looks ready to merge pending human eyes on the atomic state-activation logic and the CacheTTL=0 semantic change.

[arkana-ai-bot]

Arkana review — sha 9a8cbf1e

Summary: Client library gains ExpectedTLSKeyHash — callers can pin the deployment's TLS public-key fingerprint (SHA-256 of DER SubjectPublicKeyInfo, hex). On first use the fingerprint is learned from the verified attestation and retained for the client's lifetime. On subsequent runs callers supply the saved value. HTTP/gRPC enforce HTTPS throughout (plain-HTTP requests and downgrade redirects are rejected before headers/body are sent).

Assessment: security improvement, looks ready. One question.

What's good:

• Trust-on-first-use is the right default (PCR0 alone doesn't distinguish deployments of the same image).
• Fingerprint is retained immutably for the client's lifetime (cache refreshes / reconnects cannot replace it).
• Failed verification leaves prior state unchanged — correct.
• HTTPS enforcement including rejection of downgrade redirects (POST-replay risk closed).
• gRPC reconnects retain the fingerprint — consistent with HTTP.
• InsecureTLS is incompatible with ExpectedTLSKeyHash — good, prevents silent downgrade.
• Clear upgrade path in the README example.

One question:

The CacheTTL doc now says "Zero defaults to 60s; a negative duration verifies on every request" — was the previous behaviour (zero = verify on every request) intentional or a bug? If callers were relying on zero-TTL for eagerly-verified connections, this is a subtle breaking change. Worth a comment in the changelog or a migration note.

[arkana-ai-bot]

This PR has been open for 11 days without review. @chris-ricketts is anyone looking at the client TLS identity fix?

[arkana-ai-bot]

Follow-up pass — a6ae86f (one commit since 9a8cbf1)

1 commit added; 0 prior issues fixed; 2 still open. Informational CLI-TOFU finding now addressed.

---

Incremental diff (9a8cbf1 → a6ae86f)

Single commit a6ae86f1 — feat(cli): add attestation enrollment and saved TLS key verification:

File	Change
cmd/enclave/attest.go	New: enclave attest command
cmd/enclave/verification.go	New: shared verificationOptions + addFlags/newClient
cmd/enclave/curl.go	Refactored onto verificationOptions; stderr/stdout switched to cobra I/O
cmd/enclave/main.go	Registers newAttestCommand(); adds SilenceUsage/SilenceErrors
nix/tests/e2e.py	Enrollment, replica/migration/cert-change/restart continuity, and failure-path tests
README.md	Documents attest, --expected-tls-key-hash, enrollment workflow

---

Prior-finding status

Medium — requireHTTPS closes caller-owned body in Do() (client/client.go:220–228, 261)
Still open. client/client.go untouched. The same code path remains:

// client/client.go:220–228
func requireHTTPS(req *http.Request) error {
    if req == nil || req.URL == nil || req.URL.Scheme != "https" {
        if req != nil && req.Body != nil {
            _ = req.Body.Close()  // closes caller's request body
        }
        return fmt.Errorf("request URL must use HTTPS")
    }
    return nil
}

A caller who passes an HTTP URL to Do() gets their req.Body silently closed before the error is returned. Still the only finding I'd want resolved before merge.

Low — GRPCConn snapshots pin at call time; HTTP reads it live (client/grpc.go:44–48)
Still open. client/grpc.go untouched. No functional consequence in current design; the missing explanatory comment is still absent.

Informational — CLI per-invocation TOFU
Fixed (this commit). enclave attest prints the verified fingerprint to stdout for one-time enrollment; both attest and curl accept --expected-tls-key-hash for all subsequent invocations. README updated accordingly. The CLI is no longer limited to weak TOFU-per-invocation.

---

Review of new code

cmd/enclave/attest.go — correct

attest calls opts.newClient() then c.VerifyAttestation(), writes result.TLSKeyHash via fmt.Fprintln(cmd.OutOrStdout(), ...), and returns on any error before writing. Stdout therefore contains exactly the fingerprint (64 hex chars + newline) on success, and is empty on failure. The E2E test asserts test ! -s /tmp/candidate-fingerprint on the 503 case, confirming this invariant holds in the packaged binary. ✓

cmd/enclave/verification.go — correct

Shared flags are registered once; newClient() prints the InsecureSkipCOSEVerify warning to cmd.ErrOrStderr() before calling client.New(). This means the warning is emitted even if construction fails (e.g., bad URL), which is slightly odd but not harmful. No security issue.

cmd/enclave/main.go — correct

SilenceErrors: true + SilenceUsage: true prevents Cobra from double-printing errors that Execute() already forwards to stderr manually. The combination is correct; no error is silently discarded.

nix/tests/e2e.py — thorough, one microscopic note

The failure-path loop tests six error classes for both attest and curl /test/env/E2E_SIGNING_KEY, including the all-zeros hash case. The expected error substring for '00' * 32 is "64 hex characters"; validateTLSKeyHash in client/client.go returns "TLS public-key fingerprint must be a nonzero SHA-256 hash (64 hex characters)" for that input, so the substring match holds. ✓

Microscopic: wrong_tls_key_hash = f"{int(tls_key_hash, 16) ^ 1:064x}" would produce all-zeros if the real hash were 0x...0001, yielding a different error than expected. Cryptographically impossible for a real TLS public-key SHA-256, but a one-line comment would make the assumption explicit for future readers.

README — accurate

The enrollment workflow (enroll once with attest, reuse with --expected-tls-key-hash, retain across successor PCR0 transitions) is correctly documented and matches the implementation. The flag table is complete. ✓

---

Summary

The new CLI code is clean and correctly separates stdout (fingerprint only) from stderr (warnings/errors). The E2E coverage is the most complete this PR has seen. The one outstanding blocker is the pre-existing requireHTTPS body-close in the public Do() API (client/client.go); everything else is ready.