fix(security): prevent on: pull_request actions from mutating caches of other workflow events (#925)

Reviewed-on: https://code.forgejo.org/forgejo/runner/pulls/925
This commit is contained in:
earl-warren 2025-09-01 12:24:30 +00:00
commit 57efbac055
No known key found for this signature in database
GPG key ID: F128CBE6AB3A7201
8 changed files with 451 additions and 44 deletions

View file

@ -181,11 +181,19 @@ func (h *Handler) find(w http.ResponseWriter, r *http.Request, params httprouter
}
defer db.Close()
cache, err := findCache(db, repo, keys, version)
cache, err := findCache(db, repo, keys, version, rundata.WriteIsolationKey)
if err != nil {
h.responseJSON(w, r, 500, err)
return
}
// If read was scoped to WriteIsolationKey and didn't find anything, we can fallback to the non-isolated cache read
if cache == nil && rundata.WriteIsolationKey != "" {
cache, err = findCache(db, repo, keys, version, "")
if err != nil {
h.responseJSON(w, r, 500, err)
return
}
}
if cache == nil {
h.responseJSON(w, r, 204)
return
@ -236,6 +244,7 @@ func (h *Handler) reserve(w http.ResponseWriter, r *http.Request, params httprou
cache.CreatedAt = now
cache.UsedAt = now
cache.Repo = repo
cache.WriteIsolationKey = rundata.WriteIsolationKey
if err := insertCache(db, cache); err != nil {
h.responseJSON(w, r, 500, err)
return
@ -275,6 +284,10 @@ func (h *Handler) upload(w http.ResponseWriter, r *http.Request, params httprout
h.responseJSON(w, r, 500, fmt.Errorf("cache repo is not valid"))
return
}
if cache.WriteIsolationKey != rundata.WriteIsolationKey {
h.responseJSON(w, r, 403, fmt.Errorf("cache authorized for write isolation %q, but attempting to operate on %q", rundata.WriteIsolationKey, cache.WriteIsolationKey))
return
}
if cache.Complete {
h.responseJSON(w, r, 400, fmt.Errorf("cache %v %q: already complete", cache.ID, cache.Key))
@ -326,6 +339,10 @@ func (h *Handler) commit(w http.ResponseWriter, r *http.Request, params httprout
h.responseJSON(w, r, 500, fmt.Errorf("cache repo is not valid"))
return
}
if cache.WriteIsolationKey != rundata.WriteIsolationKey {
h.responseJSON(w, r, 403, fmt.Errorf("cache authorized for write isolation %q, but attempting to operate on %q", rundata.WriteIsolationKey, cache.WriteIsolationKey))
return
}
if cache.Complete {
h.responseJSON(w, r, 400, fmt.Errorf("cache %v %q: already complete", cache.ID, cache.Key))
@ -386,6 +403,11 @@ func (h *Handler) get(w http.ResponseWriter, r *http.Request, params httprouter.
h.responseJSON(w, r, 500, fmt.Errorf("cache repo is not valid"))
return
}
// reads permitted against caches w/ the same isolation key, or no isolation key
if cache.WriteIsolationKey != rundata.WriteIsolationKey && cache.WriteIsolationKey != "" {
h.responseJSON(w, r, 403, fmt.Errorf("cache authorized for write isolation %q, but attempting to operate on %q", rundata.WriteIsolationKey, cache.WriteIsolationKey))
return
}
if err := h.useCache(id); err != nil {
h.responseJSON(w, r, 500, fmt.Errorf("cache useCache: %w", err))
@ -417,7 +439,7 @@ func (h *Handler) middleware(handler httprouter.Handle) httprouter.Handle {
}
// if not found, return (nil, nil) instead of an error.
func findCache(db *bolthold.Store, repo string, keys []string, version string) (*Cache, error) {
func findCache(db *bolthold.Store, repo string, keys []string, version, writeIsolationKey string) (*Cache, error) {
cache := &Cache{}
for _, prefix := range keys {
// if a key in the list matches exactly, don't return partial matches
@ -425,6 +447,7 @@ func findCache(db *bolthold.Store, repo string, keys []string, version string) (
bolthold.Where("Repo").Eq(repo).Index("Repo").
And("Key").Eq(prefix).
And("Version").Eq(version).
And("WriteIsolationKey").Eq(writeIsolationKey).
And("Complete").Eq(true).
SortBy("CreatedAt").Reverse()); err == nil || !errors.Is(err, bolthold.ErrNotFound) {
if err != nil {
@ -441,6 +464,7 @@ func findCache(db *bolthold.Store, repo string, keys []string, version string) (
bolthold.Where("Repo").Eq(repo).Index("Repo").
And("Key").RegExp(re).
And("Version").Eq(version).
And("WriteIsolationKey").Eq(writeIsolationKey).
And("Complete").Eq(true).
SortBy("CreatedAt").Reverse()); err != nil {
if errors.Is(err, bolthold.ErrNotFound) {
@ -644,5 +668,6 @@ func runDataFromHeaders(r *http.Request) cacheproxy.RunData {
RunNumber: r.Header.Get("Forgejo-Cache-RunNumber"),
Timestamp: r.Header.Get("Forgejo-Cache-Timestamp"),
RepositoryMAC: r.Header.Get("Forgejo-Cache-MAC"),
WriteIsolationKey: r.Header.Get("Forgejo-Cache-WriteIsolationKey"),
}
}

View file

@ -22,26 +22,35 @@ const (
cacheRepo = "testuser/repo"
cacheRunnum = "1"
cacheTimestamp = "0"
cacheMac = "c13854dd1ac599d1d61680cd93c26b77ba0ee10f374a3408bcaea82f38ca1865"
cacheMac = "bc2e9167f9e310baebcead390937264e4c0b21d2fdd49f5b9470d54406099360"
)
var handlerExternalURL string
type AuthHeaderTransport struct {
T http.RoundTripper
T http.RoundTripper
WriteIsolationKey string
OverrideDefaultMac string
}
func (t *AuthHeaderTransport) RoundTrip(req *http.Request) (*http.Response, error) {
req.Header.Set("Forgejo-Cache-Repo", cacheRepo)
req.Header.Set("Forgejo-Cache-RunNumber", cacheRunnum)
req.Header.Set("Forgejo-Cache-Timestamp", cacheTimestamp)
req.Header.Set("Forgejo-Cache-MAC", cacheMac)
if t.OverrideDefaultMac != "" {
req.Header.Set("Forgejo-Cache-MAC", t.OverrideDefaultMac)
} else {
req.Header.Set("Forgejo-Cache-MAC", cacheMac)
}
req.Header.Set("Forgejo-Cache-Host", handlerExternalURL)
if t.WriteIsolationKey != "" {
req.Header.Set("Forgejo-Cache-WriteIsolationKey", t.WriteIsolationKey)
}
return t.T.RoundTrip(req)
}
var (
httpClientTransport = AuthHeaderTransport{http.DefaultTransport}
httpClientTransport = AuthHeaderTransport{T: http.DefaultTransport}
httpClient = http.Client{Transport: &httpClientTransport}
)
@ -88,7 +97,7 @@ func TestHandler(t *testing.T) {
content := make([]byte, 100)
_, err := rand.Read(content)
require.NoError(t, err)
uploadCacheNormally(t, base, key, version, content)
uploadCacheNormally(t, base, key, version, "", content)
})
t.Run("clean", func(t *testing.T) {
@ -380,7 +389,7 @@ func TestHandler(t *testing.T) {
_, err := rand.Read(content)
require.NoError(t, err)
uploadCacheNormally(t, base, key, version, content)
uploadCacheNormally(t, base, key, version, "", content)
// Perform the request with the custom `httpClient` which will send correct MAC data
resp, err := httpClient.Get(fmt.Sprintf("%s/cache?keys=%s&version=%s", base, key, version))
@ -416,7 +425,7 @@ func TestHandler(t *testing.T) {
for i := range contents {
_, err := rand.Read(contents[i])
require.NoError(t, err)
uploadCacheNormally(t, base, keys[i], version, contents[i])
uploadCacheNormally(t, base, keys[i], version, "", contents[i])
time.Sleep(time.Second) // ensure CreatedAt of caches are different
}
@ -454,13 +463,98 @@ func TestHandler(t *testing.T) {
assert.Equal(t, contents[except], content)
})
t.Run("find can't match without WriteIsolationKey match", func(t *testing.T) {
defer func() { httpClientTransport.WriteIsolationKey = "" }()
version := "c19da02a2bd7e77277f1ac29ab45c09b7d46a4ee758284e26bb3045ad11d9d20"
key := strings.ToLower(t.Name())
uploadCacheNormally(t, base, key, version, "TestWriteKey", make([]byte, 64))
func() {
defer overrideWriteIsolationKey("AnotherTestWriteKey")()
resp, err := httpClient.Get(fmt.Sprintf("%s/cache?keys=%s&version=%s", base, key, version))
require.NoError(t, err)
require.Equal(t, 204, resp.StatusCode)
}()
{
resp, err := httpClient.Get(fmt.Sprintf("%s/cache?keys=%s&version=%s", base, key, version))
require.NoError(t, err)
require.Equal(t, 204, resp.StatusCode)
}
func() {
defer overrideWriteIsolationKey("TestWriteKey")()
resp, err := httpClient.Get(fmt.Sprintf("%s/cache?keys=%s&version=%s", base, key, version))
require.NoError(t, err)
require.Equal(t, 200, resp.StatusCode)
}()
})
t.Run("find prefers WriteIsolationKey match", func(t *testing.T) {
version := "c19da02a2bd7e77277f1ac29ab45c09b7d46a4ee758284e26bb3045ad11d9d21"
key := strings.ToLower(t.Name())
// Between two values with the same `key`...
uploadCacheNormally(t, base, key, version, "TestWriteKey", make([]byte, 64))
uploadCacheNormally(t, base, key, version, "", make([]byte, 128))
// We should read the value with the matching WriteIsolationKey from the cache...
func() {
defer overrideWriteIsolationKey("TestWriteKey")()
resp, err := httpClient.Get(fmt.Sprintf("%s/cache?keys=%s&version=%s", base, key, version))
require.NoError(t, err)
require.Equal(t, 200, resp.StatusCode)
got := struct {
ArchiveLocation string `json:"archiveLocation"`
}{}
require.NoError(t, json.NewDecoder(resp.Body).Decode(&got))
contentResp, err := httpClient.Get(got.ArchiveLocation)
require.NoError(t, err)
require.Equal(t, 200, contentResp.StatusCode)
content, err := io.ReadAll(contentResp.Body)
require.NoError(t, err)
// Which we finally check matches the correct WriteIsolationKey's content here.
assert.Equal(t, make([]byte, 64), content)
}()
})
t.Run("find falls back if matching WriteIsolationKey not available", func(t *testing.T) {
version := "c19da02a2bd7e77277f1ac29ab45c09b7d46a4ee758284e26bb3045ad11d9d21"
key := strings.ToLower(t.Name())
uploadCacheNormally(t, base, key, version, "", make([]byte, 128))
func() {
defer overrideWriteIsolationKey("TestWriteKey")()
resp, err := httpClient.Get(fmt.Sprintf("%s/cache?keys=%s&version=%s", base, key, version))
require.NoError(t, err)
require.Equal(t, 200, resp.StatusCode)
got := struct {
ArchiveLocation string `json:"archiveLocation"`
}{}
require.NoError(t, json.NewDecoder(resp.Body).Decode(&got))
contentResp, err := httpClient.Get(got.ArchiveLocation)
require.NoError(t, err)
require.Equal(t, 200, contentResp.StatusCode)
content, err := io.ReadAll(contentResp.Body)
require.NoError(t, err)
assert.Equal(t, make([]byte, 128), content)
}()
})
t.Run("case insensitive", func(t *testing.T) {
version := "c19da02a2bd7e77277f1ac29ab45c09b7d46a4ee758284e26bb3045ad11d9d20"
key := strings.ToLower(t.Name())
content := make([]byte, 100)
_, err := rand.Read(content)
require.NoError(t, err)
uploadCacheNormally(t, base, key+"_ABC", version, content)
uploadCacheNormally(t, base, key+"_ABC", version, "", content)
{
reqKey := key + "_aBc"
@ -494,7 +588,7 @@ func TestHandler(t *testing.T) {
for i := range contents {
_, err := rand.Read(contents[i])
require.NoError(t, err)
uploadCacheNormally(t, base, keys[i], version, contents[i])
uploadCacheNormally(t, base, keys[i], version, "", contents[i])
time.Sleep(time.Second) // ensure CreatedAt of caches are different
}
@ -545,7 +639,7 @@ func TestHandler(t *testing.T) {
for i := range contents {
_, err := rand.Read(contents[i])
require.NoError(t, err)
uploadCacheNormally(t, base, keys[i], version, contents[i])
uploadCacheNormally(t, base, keys[i], version, "", contents[i])
time.Sleep(time.Second) // ensure CreatedAt of caches are different
}
@ -581,9 +675,166 @@ func TestHandler(t *testing.T) {
require.NoError(t, err)
assert.Equal(t, contents[expect], content)
})
t.Run("upload across WriteIsolationKey", func(t *testing.T) {
defer overrideWriteIsolationKey("CorrectKey")()
key := strings.ToLower(t.Name())
version := "c19da02a2bd7e77277f1ac29ab45c09b7d46a4ee758284e26bb3045ad11d9d20"
content := make([]byte, 256)
var id uint64
// reserve
{
body, err := json.Marshal(&Request{
Key: key,
Version: version,
Size: int64(len(content)),
})
require.NoError(t, err)
resp, err := httpClient.Post(fmt.Sprintf("%s/caches", base), "application/json", bytes.NewReader(body))
require.NoError(t, err)
assert.Equal(t, 200, resp.StatusCode)
got := struct {
CacheID uint64 `json:"cacheId"`
}{}
require.NoError(t, json.NewDecoder(resp.Body).Decode(&got))
id = got.CacheID
}
// upload, but with the incorrect write isolation key relative to the cache obj created
func() {
defer overrideWriteIsolationKey("WrongKey")()
req, err := http.NewRequest(http.MethodPatch,
fmt.Sprintf("%s/caches/%d", base, id), bytes.NewReader(content))
require.NoError(t, err)
req.Header.Set("Content-Type", "application/octet-stream")
req.Header.Set("Content-Range", "bytes 0-99/*")
resp, err := httpClient.Do(req)
require.NoError(t, err)
assert.Equal(t, 403, resp.StatusCode)
}()
})
t.Run("commit across WriteIsolationKey", func(t *testing.T) {
defer overrideWriteIsolationKey("CorrectKey")()
key := strings.ToLower(t.Name())
version := "c19da02a2bd7e77277f1ac29ab45c09b7d46a4ee758284e26bb3045ad11d9d20"
content := make([]byte, 256)
var id uint64
// reserve
{
body, err := json.Marshal(&Request{
Key: key,
Version: version,
Size: int64(len(content)),
})
require.NoError(t, err)
resp, err := httpClient.Post(fmt.Sprintf("%s/caches", base), "application/json", bytes.NewReader(body))
require.NoError(t, err)
assert.Equal(t, 200, resp.StatusCode)
got := struct {
CacheID uint64 `json:"cacheId"`
}{}
require.NoError(t, json.NewDecoder(resp.Body).Decode(&got))
id = got.CacheID
}
// upload
{
req, err := http.NewRequest(http.MethodPatch,
fmt.Sprintf("%s/caches/%d", base, id), bytes.NewReader(content))
require.NoError(t, err)
req.Header.Set("Content-Type", "application/octet-stream")
req.Header.Set("Content-Range", "bytes 0-99/*")
resp, err := httpClient.Do(req)
require.NoError(t, err)
assert.Equal(t, 200, resp.StatusCode)
}
// commit, but with the incorrect write isolation key relative to the cache obj created
func() {
defer overrideWriteIsolationKey("WrongKey")()
resp, err := httpClient.Post(fmt.Sprintf("%s/caches/%d", base, id), "", nil)
require.NoError(t, err)
assert.Equal(t, 403, resp.StatusCode)
}()
})
t.Run("get across WriteIsolationKey", func(t *testing.T) {
defer func() { httpClientTransport.WriteIsolationKey = "" }()
version := "c19da02a2bd7e77277f1ac29ab45c09b7d46a4ee758284e26bb3045ad11d9d21"
key := strings.ToLower(t.Name())
uploadCacheNormally(t, base, key, version, "", make([]byte, 128))
keyIsolated := strings.ToLower(t.Name()) + "_isolated"
uploadCacheNormally(t, base, keyIsolated, version, "CorrectKey", make([]byte, 128))
// Perform the 'get' without the right WriteIsolationKey for the cache entry... should be OK for `key` since it
// was written with WriteIsolationKey "" meaning it is available for non-isolated access
func() {
defer overrideWriteIsolationKey("WhoopsWrongKey")()
resp, err := httpClient.Get(fmt.Sprintf("%s/cache?keys=%s&version=%s", base, key, version))
require.NoError(t, err)
require.Equal(t, 200, resp.StatusCode)
got := struct {
ArchiveLocation string `json:"archiveLocation"`
}{}
require.NoError(t, json.NewDecoder(resp.Body).Decode(&got))
contentResp, err := httpClient.Get(got.ArchiveLocation)
require.NoError(t, err)
require.Equal(t, 200, contentResp.StatusCode)
httpClientTransport.WriteIsolationKey = "CorrectKey" // reset for next find
}()
// Perform the 'get' without the right WriteIsolationKey for the cache entry... should be 403 for `keyIsolated`
// because it was written with a different WriteIsolationKey.
{
got := func() struct {
ArchiveLocation string `json:"archiveLocation"`
} {
defer overrideWriteIsolationKey("CorrectKey")() // for test purposes make the `find` successful...
resp, err := httpClient.Get(fmt.Sprintf("%s/cache?keys=%s&version=%s", base, keyIsolated, version))
require.NoError(t, err)
require.Equal(t, 200, resp.StatusCode)
got := struct {
ArchiveLocation string `json:"archiveLocation"`
}{}
require.NoError(t, json.NewDecoder(resp.Body).Decode(&got))
return got
}()
func() {
defer overrideWriteIsolationKey("WhoopsWrongKey")() // but then access w/ the wrong key for `get`
contentResp, err := httpClient.Get(got.ArchiveLocation)
require.NoError(t, err)
require.Equal(t, 403, contentResp.StatusCode)
}()
}
})
}
func uploadCacheNormally(t *testing.T, base, key, version string, content []byte) {
func overrideWriteIsolationKey(writeIsolationKey string) func() {
originalWriteIsolationKey := httpClientTransport.WriteIsolationKey
originalMac := httpClientTransport.OverrideDefaultMac
httpClientTransport.WriteIsolationKey = writeIsolationKey
httpClientTransport.OverrideDefaultMac = computeMac("secret", cacheRepo, cacheRunnum, cacheTimestamp, httpClientTransport.WriteIsolationKey)
return func() {
httpClientTransport.WriteIsolationKey = originalWriteIsolationKey
httpClientTransport.OverrideDefaultMac = originalMac
}
}
func uploadCacheNormally(t *testing.T, base, key, version, writeIsolationKey string, content []byte) {
if writeIsolationKey != "" {
defer overrideWriteIsolationKey(writeIsolationKey)()
}
var id uint64
{
body, err := json.Marshal(&Request{

View file

@ -22,7 +22,7 @@ func (h *Handler) validateMac(rundata cacheproxy.RunData) (string, error) {
return "", ErrValidation
}
expectedMAC := computeMac(h.secret, rundata.RepositoryFullName, rundata.RunNumber, rundata.Timestamp)
expectedMAC := computeMac(h.secret, rundata.RepositoryFullName, rundata.RunNumber, rundata.Timestamp, rundata.WriteIsolationKey)
if hmac.Equal([]byte(expectedMAC), []byte(rundata.RepositoryMAC)) {
return rundata.RepositoryFullName, nil
}
@ -40,12 +40,14 @@ func validateAge(ts string) bool {
return true
}
func computeMac(secret, repo, run, ts string) string {
func computeMac(secret, repo, run, ts, writeIsolationKey string) string {
mac := hmac.New(sha256.New, []byte(secret))
mac.Write([]byte(repo))
mac.Write([]byte(">"))
mac.Write([]byte(run))
mac.Write([]byte(">"))
mac.Write([]byte(ts))
mac.Write([]byte(">"))
mac.Write([]byte(writeIsolationKey))
return hex.EncodeToString(mac.Sum(nil))
}

View file

@ -19,7 +19,7 @@ func TestMac(t *testing.T) {
run := "1"
ts := strconv.FormatInt(time.Now().Unix(), 10)
mac := computeMac(handler.secret, name, run, ts)
mac := computeMac(handler.secret, name, run, ts, "")
rundata := cacheproxy.RunData{
RepositoryFullName: name,
RunNumber: run,
@ -37,7 +37,7 @@ func TestMac(t *testing.T) {
run := "1"
ts := "9223372036854775807" // This should last us for a while...
mac := computeMac(handler.secret, name, run, ts)
mac := computeMac(handler.secret, name, run, ts, "")
rundata := cacheproxy.RunData{
RepositoryFullName: name,
RunNumber: run,
@ -72,9 +72,12 @@ func TestMac(t *testing.T) {
run := "42"
ts := "1337"
mac := computeMac(secret, name, run, ts)
expectedMac := "f666f06f917acb7186e152195b2a8c8d36d068ce683454be0878806e08e04f2b" // * Precomputed, anytime the computeMac function changes this needs to be recalculated
mac := computeMac(secret, name, run, ts, "")
expectedMac := "4754474b21329e8beadd2b4054aa4be803965d66e710fa1fee091334ed804f29" // * Precomputed, anytime the computeMac function changes this needs to be recalculated
require.Equal(t, expectedMac, mac)
require.Equal(t, mac, expectedMac)
mac = computeMac(secret, name, run, ts, "refs/pull/12/head")
expectedMac = "9ca8f4cb5e1b083ee8cd215215bc00f379b28511d3ef7930bf054767de34766d" // * Precomputed, anytime the computeMac function changes this needs to be recalculated
require.Equal(t, expectedMac, mac)
})
}

View file

@ -24,12 +24,13 @@ func (c *Request) ToCache() *Cache {
}
type Cache struct {
ID uint64 `json:"id" boltholdKey:"ID"`
Repo string `json:"repo" boltholdIndex:"Repo"`
Key string `json:"key"`
Version string `json:"version"`
Size int64 `json:"cacheSize"`
Complete bool `json:"complete"`
UsedAt int64 `json:"usedAt"`
CreatedAt int64 `json:"createdAt"`
ID uint64 `json:"id" boltholdKey:"ID"`
Repo string `json:"repo" boltholdIndex:"Repo"`
Key string `json:"key"`
Version string `json:"version"`
Size int64 `json:"cacheSize"`
Complete bool `json:"complete"`
UsedAt int64 `json:"usedAt"`
CreatedAt int64 `json:"createdAt"`
WriteIsolationKey string `json:"writeIsolationKey"`
}

View file

@ -51,15 +51,17 @@ type RunData struct {
RunNumber string
Timestamp string
RepositoryMAC string
WriteIsolationKey string
}
func (h *Handler) CreateRunData(fullName, runNumber, timestamp string) RunData {
mac := computeMac(h.cacheSecret, fullName, runNumber, timestamp)
func (h *Handler) CreateRunData(fullName, runNumber, timestamp, writeIsolationKey string) RunData {
mac := computeMac(h.cacheSecret, fullName, runNumber, timestamp, writeIsolationKey)
return RunData{
RepositoryFullName: fullName,
RunNumber: runNumber,
Timestamp: timestamp,
RepositoryMAC: mac,
WriteIsolationKey: writeIsolationKey,
}
}
@ -152,6 +154,9 @@ func (h *Handler) newReverseProxy(targetHost string) (*httputil.ReverseProxy, er
r.Out.Header.Set("Forgejo-Cache-Timestamp", runData.Timestamp)
r.Out.Header.Set("Forgejo-Cache-MAC", runData.RepositoryMAC)
r.Out.Header.Set("Forgejo-Cache-Host", h.ExternalURL())
if runData.WriteIsolationKey != "" {
r.Out.Header.Set("Forgejo-Cache-WriteIsolationKey", runData.WriteIsolationKey)
}
},
}
return proxy, nil
@ -207,12 +212,14 @@ func (h *Handler) Close() error {
return retErr
}
func computeMac(secret, repo, run, ts string) string {
func computeMac(secret, repo, run, ts, writeIsolationKey string) string {
mac := hmac.New(sha256.New, []byte(secret))
mac.Write([]byte(repo))
mac.Write([]byte(">"))
mac.Write([]byte(run))
mac.Write([]byte(">"))
mac.Write([]byte(ts))
mac.Write([]byte(">"))
mac.Write([]byte(writeIsolationKey))
return hex.EncodeToString(mac.Sum(nil))
}