klinux / klinux/gocdnext

hardening: cache write race — old signed URL can clobber a newer cache blob (generational writes)

Open
#298 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

help wanted
Dominant language
Go
Stars
7
Forks
3
Avg merge
2h 12m
Merged PRs (30d)
30

Description

Split off from #210 (Item 3). Lower priority than the artifact write-once work (#210 Item 1, PR #297) because it is fail-safe.

Problem

Caches share the signing API with artifacts but intentionally overwrite a deterministic key: CacheStorageKey = "cache/<projectID>/<sha256(key)>" (store/caches.go), and UpsertPendingCache keeps the same storage_key so re-uploads replace the blob in place (grpcsrv/cache.go, handler.go routes cache/ keys to Put, not PutCreateOnly).

That opens an "old cache URL clobbers new cache" race: two overlapping uploads for one key share one storage_key; a stale-but-valid signed PUT (15-min TTL) landing after a newer upload overwrites the newer blob, while MarkCacheReady (keyed by row id) may have recorded the newer uploader's sha → blob/metadata divergence. Nothing at the backend gates this.

Why it's lower priority

It is fail-safe: the worst outcome is a corrupted/mismatched cache entry, which a consumer treats as a cache miss and rebuilds. No artifact integrity or deploy correctness is affected (that's #210 Item 1). So this is a robustness/cleanliness fix, not a data-loss fix.

Proposed direction

Make cache writes generational instead of in-place:

  • Mint a fresh storage_key per cache upload (e.g. append a generation/uuid), and swap the ready-row to point at the new key atomically once the upload is confirmed; delete the superseded blob on a sweep.
  • OR use a conditional write (If-Generation-Match on GCS / a version guard on S3) so a stale PUT can't clobber a newer generation.

Reuse the create-only signing plumbing added in #210 (PutOption / SignedURL.Headers) where it fits.

Acceptance criteria

  • A stale signed cache PUT landing after a newer upload cannot overwrite the newer blob (or is detectably rejected).
  • Blob bytes and the recorded sha for a cache row never diverge.
  • Cache hit/restore semantics unchanged; no new backend round-trip on the hot path.
  • Tests covering the overlapping-upload race.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Trace cache key and upload handling in store/caches.go, grpcsrv/cache.go, and handler.go, including the PutOption and SignedURL plumbing from #210. Start by mapping overlapping uploads through signing, blob writes, and MarkCacheReady. Done means stale writes cannot overwrite or diverge from the recorded sha, cache restore behavior is unchanged, and a race test covers the overlap without adding a hot-path backend round trip.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.