security: disable HTTP redirect following on attestation, enrollment, and CLI clients [1/5]

[rsampaio]

The exporter client already has CheckRedirect hardened (#165, #168). This covers the remaining three outbound http.Client instances that still follow redirects by default:

• attestation nonce endpoint (also adds status code validation and error-field handling so a non-200 or error response is not silently accepted as a valid nonce)
• enrollment endpoint
• local CLI agent client (NewAgentHTTPClient, used by status/inject)

A compromised backend returning a 302 redirect could bounce these clients into making authenticated requests to internal services. Setting CheckRedirect to return http.ErrUseLastResponse makes the client return the redirect response as-is instead of following it.

Description

Checklist

• I am familiar with the Contributing Guidelines.
• New or existing tests cover these changes.
• The documentation is up to date with these changes.

Summary by CodeRabbit

• Bug Fixes

  • HTTP clients in attestation, endpoint, and enrollment now return redirect responses instead of following them.
  • Non-200 HTTP statuses and server-side error payloads are now treated and reported as errors.

• Tests

  • Added tests verifying redirects are not followed and that error responses and non-OK statuses are handled as errors.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: cd12d0b4-a01f-4c21-8d1a-aa51180233f7

📥 Commits

Reviewing files that changed from the base of the PR and between 30f0bbf and 7d6d5a4.

📒 Files selected for processing (6)

• internal/attestation/attestation.go
• internal/attestation/attestation_test.go
• internal/endpoint/endpoint.go
• internal/endpoint/endpoint_test.go
• internal/enrollment/enrollment.go
• internal/enrollment/enrollment_test.go

✅ Files skipped from review due to trivial changes (1)

• internal/attestation/attestation.go

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

• internal/enrollment/enrollment.go
• internal/endpoint/endpoint.go
• internal/enrollment/enrollment_test.go
• internal/endpoint/endpoint_test.go
• internal/attestation/attestation_test.go

---

📝 Walkthrough

Walkthrough

The PR disables automatic HTTP redirect following across attestation, endpoint, and enrollment HTTP clients by setting CheckRedirect to return http.ErrUseLastResponse. It also tightens attestation nonce handling to reject non-200 responses and JSON error payloads; tests were added to validate these behaviors.

Changes

Cohort / File(s)	Summary
Attestation: client behavior & nonce handling internal/attestation/attestation.go, internal/attestation/attestation_test.go	Set CheckRedirect to return http.ErrUseLastResponse in the attestation client; Manager.getNonce now errors on non-200 status and on response.Error in the JSON payload. Added tests verifying 302 redirect is not followed and server error payloads cause errors.
Endpoint: Agent HTTP client internal/endpoint/endpoint.go, internal/endpoint/endpoint_test.go	NewAgentHTTPClient now configures CheckRedirect (no-follow) for both TCP and unix-socket clients. Tests extended to assert CheckRedirect is set and that TCP client returns 302 without invoking redirect target.
Enrollment: HTTP client internal/enrollment/enrollment.go, internal/enrollment/enrollment_test.go	PerformEnrollment's HTTP client now disables redirects via CheckRedirect. Added test ensuring a 302 from the enrollment endpoint is returned as an error (contains status 302) and the redirect target is not called.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 I nibble at bugs and stop each sly trick,
No more wandering down a redirected wick,
If servers protest with a three-oh-two shout,
I pause, I report, I won’t wander about,
Hooray—secure hops, neat code, and a carrot pick! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main change: disabling HTTP redirect following on three client types for security purposes.

_{✏️ 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 security/disable-http-redirect-following

---

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

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@internal/attestation/attestation_test.go`:
- Around line 305-332: The test has a data race on the variable
redirectTargetCalled in TestManager_GetNonce_RejectsNonOKStatus because the
server handler goroutine writes it and the test goroutine reads it; change
redirectTargetCalled to a concurrency-safe atomic (e.g. a uint32) and replace
writes in the HTTP handler with sync/atomic.StoreUint32(&redirectTargetCalled,
1) and reads in the test assertion with
sync/atomic.LoadUint32(&redirectTargetCalled) == 1, and add sync/atomic to the
imports; rerun go test -race to verify.

In `@internal/endpoint/endpoint_test.go`:
- Around line 19-20: The test uses a non-thread-safe boolean
redirectTargetCalled that is written in the httptest handler goroutine and read
in the test goroutine; make it atomic by replacing the bool with var
redirectTargetCalled uint32, import "sync/atomic", change the handler write to
atomic.StoreUint32(&redirectTargetCalled, 1), and change reads/asserts to use
atomic.LoadUint32(&redirectTargetCalled) (e.g. assert.Zero(t,
atomic.LoadUint32(&redirectTargetCalled))). Ensure all occurrences in
endpoint_test.go are updated and run go test -race.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: c7011c69-82dc-4bf3-b8ff-a8b0e0d5497f

📥 Commits

Reviewing files that changed from the base of the PR and between 47dab0c and 30f0bbf.

📒 Files selected for processing (6)

• internal/attestation/attestation.go
• internal/attestation/attestation_test.go
• internal/endpoint/endpoint.go
• internal/endpoint/endpoint_test.go
• internal/enrollment/enrollment.go
• internal/enrollment/enrollment_test.go

[jingxiang-z]

Before merge, would like to see if go test -race passed.