Add TLS handling updates

[sangwa]

• feat: make private TLS certificate optional for public-cert-only deployments
• feat: verify dependency server TLS certificate against attestation report
• fix: return 500 instead of 503 for TLS certificate verification failures
• fix: preserve handler-controlled error messages for 5xx responses

Summary by CodeRabbit

• New Features

  • HTTPS dependency verification now additionally binds the observed server TLS leaf certificate fingerprint to the dependency report (skipped for plain HTTP).

• Bug Fixes

  • TLS certificate verification failures map to non-transient server errors (500) rather than transient upstream errors (503).
  • Server startup emits a warning when no private TLS certificate is configured.
  • TLS validation requires at least one cert; private certs are required when dependency endpoints exist.

• Documentation

  • Clarified conditional TLS requirements, dependency attestation checks, and error-message preservation for handler-controlled errors.

[coderabbitai]

Caution

Review failed

Pull request was closed or merged during review

Walkthrough

This pull request adds HTTPS server-leaf-certificate binding to dependency attestation: when a dependency is fetched over HTTPS the server computes SHA-256(resp.TLS.PeerCertificates[0].Raw) and requires it to equal report.data.tls.private; the check is skipped for plain HTTP (no TLS state). TLS config validation was made conditional: a private TLS cert/key/CA is required when dependency endpoints are configured or when no public cert is present; public/private validations run only when their paths are provided. TLS certificate verification errors are treated as non-transient (mapped to the non-503 error path). Error handler behavior was changed to preserve handler-controlled fiber.NewError messages for 5xx responses. Documentation and tests were updated.

Sequence Diagram(s)

sequenceDiagram
    participant Client as Attestation Client
    participant Server as Attestation Server
    participant Dep as Dependency Service
    participant TLS as TLS Layer

    Client->>Server: Request attestation / trigger dependency fetch
    Server->>Dep: HTTP(S) request to dependency
    alt HTTPS
        Note right of Dep: TLS handshake
        Dep-->>TLS: Send leaf certificate
        TLS->>Server: Provide resp.TLS with PeerCertificates[0].Raw
        Server->>Server: Compute SHA-256(resp.TLS.PeerCertificates[0].Raw) -> serverCertFP
        Dep-->>Server: Attestation report (report.data.tls.private + data.tls.client + evidence)
        Server->>Server: Require report.data.tls.private == serverCertFP
    else HTTP (no TLS)
        Dep-->>Server: Attestation report (no server TLS state)
        Server->>Server: Skip serverCertFP check
    end
    Server->>Server: Verify client cert fingerprint (data.tls.client) and nonce and crypto evidence
    alt all checks pass
        Server-->>Client: Success (verified)
    else any e2e or crypto failure
        Server-->>Client: E2E verification error (opaque to caller)
    end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐇 I hopped the TLS meadow, sniffed the leaf's small print,
A hexed little fingerprint, a trustworthy mint.
If hashes match true, I dance and I cheer,
If not, I thump my paw — "no link here, my dear!"

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title 'Add TLS handling updates' is vague and generic, using non-descriptive language that does not convey meaningful information about the specific changes.	Use a more specific title that highlights the primary change, such as 'Make private TLS certificate optional and verify dependency server certificates' or 'Add server certificate verification for dependency HTTPS connections'.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/tls-handling-updates

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

internal/server.go (1)

126-128: Clarify warning scope to include HTTPS dependency server-cert binding

Nice addition. Consider mentioning that HTTPS dependency server-certificate binding is also unavailable in this mode, to fully reflect the new verification model.

Suggested wording tweak

-		logger.Warn("running without private TLS certificate, dependency mTLS and XFCC client certificate proof are unavailable")
+		logger.Warn("running without private TLS certificate, dependency mTLS/XFCC client-certificate proof and HTTPS dependency server-certificate binding are unavailable")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/server.go` around lines 126 - 128, The warning in the Private TLS
cert check (the if block testing cfg.PrivateTLSCertPath == "") is incomplete;
update the logger.Warn message in internal/server.go to explicitly include that
HTTPS dependency server-certificate binding is also unavailable in this mode
(alongside dependency mTLS and XFCC client certificate proof), so the warning
fully reflects the verification model change.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@internal/server.go`:
- Around line 126-128: The warning in the Private TLS cert check (the if block
testing cfg.PrivateTLSCertPath == "") is incomplete; update the logger.Warn
message in internal/server.go to explicitly include that HTTPS dependency
server-certificate binding is also unavailable in this mode (alongside
dependency mTLS and XFCC client certificate proof), so the warning fully
reflects the verification model change.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: fb88d5b0-ee0a-4952-be35-de0fe05ea71b

📥 Commits

Reviewing files that changed from the base of the PR and between 57cf4f7 and 42dd053.

📒 Files selected for processing (11)

• CLAUDE.md
• README.md
• docs/architecture.md
• docs/security.md
• docs/verification.md
• internal/attestation_test.go
• internal/dependencies.go
• internal/dependencies_test.go
• internal/server.go
• internal/tls.go
• internal/tls_test.go

[coderabbitai]

🧹 Nitpick comments (1)

internal/dependencies_test.go (1)

211-306: Consider extracting a small helper for report setup in server-cert tests.

The repeated construction of AttestationReportData/AttestationReport is a bit verbose; a helper would reduce duplication and make intent even clearer.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/dependencies_test.go` around lines 211 - 306, Extract a small test
helper to build AttestationReport/AttestationReportData to remove duplication in
the server-cert tests: create a function (e.g., makeReportWithTLS or
newTestReport) that accepts nonceHex, clientFP, privateFP (allowing empty
string), and blob bytes and returns (*AttestationReport, *AttestationReportData
or its JSON); then replace repeated inline constructions in
TestVerifyDependencyReport_ServerCertSkippedWhenEmpty,
TestVerifyDependencyReport_ServerCertMismatch,
TestVerifyDependencyReport_ServerCertMissing, and
TestVerifyDependencyReport_ServerCertMatch with calls to that helper and reuse
reportDataWithClientCert where appropriate; ensure the returned report.Data is
set to json.RawMessage of the marshaled report data and the Evidence contains
the same fake blob used in the tests so existing verifyDependencyReportOnly
calls continue to behave the same.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@internal/dependencies_test.go`:
- Around line 211-306: Extract a small test helper to build
AttestationReport/AttestationReportData to remove duplication in the server-cert
tests: create a function (e.g., makeReportWithTLS or newTestReport) that accepts
nonceHex, clientFP, privateFP (allowing empty string), and blob bytes and
returns (*AttestationReport, *AttestationReportData or its JSON); then replace
repeated inline constructions in
TestVerifyDependencyReport_ServerCertSkippedWhenEmpty,
TestVerifyDependencyReport_ServerCertMismatch,
TestVerifyDependencyReport_ServerCertMissing, and
TestVerifyDependencyReport_ServerCertMatch with calls to that helper and reuse
reportDataWithClientCert where appropriate; ensure the returned report.Data is
set to json.RawMessage of the marshaled report data and the Evidence contains
the same fake blob used in the tests so existing verifyDependencyReportOnly
calls continue to behave the same.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 04b75eb5-6b9c-410a-af1d-02045f777bf4

📥 Commits

Reviewing files that changed from the base of the PR and between 42dd053 and d2855c9.

📒 Files selected for processing (3)

• docs/security.md
• internal/dependencies.go
• internal/dependencies_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• internal/dependencies.go

[sangwa]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.