Package Code Review Report (some issues to fix)

[cpsource]

SPIRE Code Review

Review of spiffe/spire — findings verified against current code and git history.

---

Critical

1. World-accessible Unix Domain Sockets (os.ModePerm / 0777)

• pkg/agent/endpoints/endpoints_posix.go:30 — Workload API socket is chmod 0777, meaning any user on the system can interact with it and potentially obtain SVIDs.
• support/oidc-discovery-provider/main_posix.go:67 — Same issue on the OIDC discovery provider socket.
• Other endpoints correctly use 0770 (e.g., pkg/agent/api/endpoints_posix.go:21, pkg/server/endpoints/endpoints_posix.go:32).
• Status: STILL PRESENT — no issues filed, no commits addressing this.

2. Profiling directory created with 0777

• pkg/common/profiling/dumpers.go:161 — os.MkdirAll(profilesDir, os.ModePerm) makes profiling data world-readable/writable.
• Status: STILL PRESENT — no issues filed.

---

High

3. MySQL TLS without explicit MinVersion

• pkg/server/datastore/sqlstore/mysql.go:128 — Creates tls.Config{} without setting MinVersion, potentially allowing TLS 1.0/1.1. There's a TODO in the code acknowledging this.
• Status: STILL PRESENT — no issues filed, TODO still in code.

4. Goroutine leak in health check server

• support/oidc-discovery-provider/main.go:113-121 — Health check HTTP server started in a goroutine with no shutdown mechanism. Related feature issues (Feature Request: add health checks to oidc-discovery-provider #3121, OIDC Discovery Provider health check endpoint unavailable outside container when using virtual network #3629) are closed but didn't address the leak.
• Status: STILL PRESENT — no issues filed.

---

Medium

5. Unstructured logging with fmt.Printf

• pkg/agent/manager/manager.go:358,360 — Uses fmt.Printf for trust bundle mismatch warnings instead of the structured logger (m.c.Log). Also contains a typo: "Trust Bandle" (should be "Trust Bundle").
• Status: STILL PRESENT — introduced by rebootstrap PR Rebootstrap support - Phase 1 #5892, missed by spelling fix PR Spelling and grammar fixes #5571. No issues filed.

6. Unbuffered error channels

• pkg/server/endpoints/endpoints.go:349,391 — make(chan error) without buffer; if the receiver exits early the sending goroutine blocks forever.
• pkg/server/endpoints/bundle/server.go:72 — Same issue FIXED — now uses make(chan error, 1).
• Status: PARTIALLY FIXED — bundle/server.go corrected; endpoints.go still has 2 unbuffered channels.

7. Context misuse in shutdown handlers

• support/oidc-discovery-provider/main.go:130 — Uses context.Background() for server.Shutdown() inside a goroutine that already has the parent ctx available, so shutdown has no timeout/deadline.
• pkg/agent/plugin/nodeattestor/httpchallenge/httpchallenge.go:214 — Same pattern.
• Status: STILL PRESENT — arguably idiomatic Go, but means shutdown could hang indefinitely.

8. InsecureSkipVerify configurable in production

• pkg/server/plugin/upstreamauthority/vault/vault_client.go:281 — Vault client allows disabling TLS verification via config.
• pkg/agent/plugin/workloadattestor/k8s/k8s.go:499 — Kubelet verification can be skipped.
• Both are intentional/documented and gated behind configuration flags. Worth flagging for operational awareness.
• Status: STILL PRESENT — by design.

---

Low

9. Bare returns in named-return functions

• cmd/spire-server/cli/entry/create.go:219-233 and cmd/spire-server/cli/entry/update.go:213-230 — Use bare return with named returns, reducing clarity.

10. context.TODO() in production

• pkg/agent/agent.go:517 — Health check uses context.TODO() with an accompanying TODO comment.

11. Stale TODO comment (underlying issue already fixed)

• pkg/server/ca/manager/journal.go:337 — TODO says "stop saving the CA journal on disk in v1.10", but the disk write was already removed in PR Do not save the CA journal file anymore #5202. Only the stale comment remains.

12. ~100+ TODO/FIXME comments

• Significant technical debt markers throughout the codebase.

---

Withdrawn Findings

The following were reported in the initial review but found to be incorrect during git verification:

• Defer in loop (GCS client leak) — pkg/server/plugin/notifier/gcsbundle/gcsbundle.go — False positive. The GCS client is created once before the loop with a single defer client.Close(). No resource leak.
• RLock/Lock promotion race — pkg/server/ca/manager/manager.go:327-345 — Weaker than reported. ActivateX509CA and RotateX509CA are separate methods with separate lock acquisitions, not a lock promotion within a single call. Unlikely to be a real race.

---

Positive Observations

• No SQL injection — parameterized queries via GORM throughout
• No command injection — exec.Command used with separated args, no shell invocation
• No hardcoded secrets — credentials properly loaded from env/config
• Good mutex discipline — consistent defer unlock patterns in security-critical sections
• Test code properly isolated — no test/debug code leaking into production paths
• SHA-1 usage justified — all instances are per RFC 5280 / specification requirements

---

GitHub Issue Tracker Analysis

None of the confirmed findings are currently tracked as open issues in spiffe/spire.

#	Finding	Existing Issue?	Status
1	World-accessible UDS (0777)	None	STILL PRESENT
2	Profiling dir 0777	None	STILL PRESENT
3	MySQL TLS without MinVersion	None	STILL PRESENT
4	Goroutine leak in health check	None (related feature issues closed)	STILL PRESENT
5	fmt.Printf / "Trust Bandle" typo	None (#6593 tracks a different typo)	STILL PRESENT
6	Unbuffered error channels	None	PARTIALLY FIXED
7	Context misuse in shutdown	None	STILL PRESENT
8	InsecureSkipVerify in production	None	By design
9	CA journal stale TODO	No open issue	Underlying bug fixed in PR #5202

Notes on CA Journal History

The CA journal migration to the database was completed across several closed issues:

• Remove server dependency on on-disk data #1840 (closed, milestone 1.9.0) — "Remove server dependency on on-disk data"
• On-disk data dependency: add table to store CA journal data #4462 (closed) — "On-disk data dependency: add table to store CA journal data"
• On-disk data dependency: import journal into ca_journals table #4463 (closed, milestone 1.9.0) — "On-disk data dependency: import journal into ca_journals table"
• On-disk data dependency: populate the ca_journals table on CA rotation #4464 (closed, milestone 1.9.0) — "On-disk data dependency: populate the ca_journals table on CA rotation"
• Remove migrate journal code #4229 (closed) — "Remove migrate journal code"
• Do not save the CA journal file anymore #5202 — "Do not save the CA journal file anymore" (the actual fix)

The TODO at journal.go:337 is stale — the disk write was removed but the comment was not cleaned up.

---

Top 3 Recommendations

1. Fix the 0777 socket permissions — highest-impact issue, still unaddressed
2. Set MinVersion: tls.VersionTLS12 on the MySQL TLS config — low-effort hardening
3. Add shutdown mechanism for the OIDC discovery provider health check server — goroutine leak