From a3aedba3f16c12072073cd5b26b9472d7efb2024 Mon Sep 17 00:00:00 2001 From: Mathieu Fenniak Date: Fri, 5 Sep 2025 06:01:49 +0000 Subject: [PATCH] refactor: remove duplicate computeMac function (#936) It was raised during embargo review of #925 that there are two implementations of `computeMac`; this PR fixes that. As all the tests for `computeMac` were in the `artifactcache` package, it made more sense to keep the method there. That required reversing the dependency `artifactcache->cacheproxy` package dependency -- it makes more sense to me for the proxy to depend on the cache, rather than vice-versa. - other - [PR](https://code.forgejo.org/forgejo/runner/pulls/936): refactor: remove duplicate computeMac function Reviewed-on: https://code.forgejo.org/forgejo/runner/pulls/936 Reviewed-by: Michael Kriese Co-authored-by: Mathieu Fenniak Co-committed-by: Mathieu Fenniak --- act/artifactcache/handler.go | 13 +++++++++--- act/artifactcache/handler_test.go | 2 +- act/artifactcache/mac.go | 8 +++----- act/artifactcache/mac_test.go | 19 ++++++++--------- act/cacheproxy/handler.go | 34 ++++++------------------------- 5 files changed, 29 insertions(+), 47 deletions(-) diff --git a/act/artifactcache/handler.go b/act/artifactcache/handler.go index 801cd93d..54087746 100644 --- a/act/artifactcache/handler.go +++ b/act/artifactcache/handler.go @@ -20,7 +20,6 @@ import ( "github.com/timshannon/bolthold" "go.etcd.io/bbolt" - "code.forgejo.org/forgejo/runner/v9/act/cacheproxy" "code.forgejo.org/forgejo/runner/v9/act/common" ) @@ -689,8 +688,16 @@ func parseContentRange(s string) (uint64, uint64, error) { return start, stop, nil } -func runDataFromHeaders(r *http.Request) cacheproxy.RunData { - return cacheproxy.RunData{ +type RunData struct { + RepositoryFullName string + RunNumber string + Timestamp string + RepositoryMAC string + WriteIsolationKey string +} + +func runDataFromHeaders(r *http.Request) RunData { + return RunData{ RepositoryFullName: r.Header.Get("Forgejo-Cache-Repo"), RunNumber: r.Header.Get("Forgejo-Cache-RunNumber"), Timestamp: r.Header.Get("Forgejo-Cache-Timestamp"), diff --git a/act/artifactcache/handler_test.go b/act/artifactcache/handler_test.go index c9e01ba3..cc0b443c 100644 --- a/act/artifactcache/handler_test.go +++ b/act/artifactcache/handler_test.go @@ -821,7 +821,7 @@ func overrideWriteIsolationKey(writeIsolationKey string) func() { originalMac := httpClientTransport.OverrideDefaultMac httpClientTransport.WriteIsolationKey = writeIsolationKey - httpClientTransport.OverrideDefaultMac = computeMac("secret", cacheRepo, cacheRunnum, cacheTimestamp, httpClientTransport.WriteIsolationKey) + httpClientTransport.OverrideDefaultMac = ComputeMac("secret", cacheRepo, cacheRunnum, cacheTimestamp, httpClientTransport.WriteIsolationKey) return func() { httpClientTransport.WriteIsolationKey = originalWriteIsolationKey diff --git a/act/artifactcache/mac.go b/act/artifactcache/mac.go index faefe2d7..5b8ca3fb 100644 --- a/act/artifactcache/mac.go +++ b/act/artifactcache/mac.go @@ -10,19 +10,17 @@ import ( "errors" "strconv" "time" - - "code.forgejo.org/forgejo/runner/v9/act/cacheproxy" ) var ErrValidation = errors.New("validation error") -func (h *handler) validateMac(rundata cacheproxy.RunData) (string, error) { +func (h *handler) validateMac(rundata RunData) (string, error) { // TODO: allow configurable max age if !validateAge(rundata.Timestamp) { return "", ErrValidation } - expectedMAC := computeMac(h.secret, rundata.RepositoryFullName, rundata.RunNumber, rundata.Timestamp, rundata.WriteIsolationKey) + 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,7 +38,7 @@ func validateAge(ts string) bool { return true } -func computeMac(secret, repo, run, ts, writeIsolationKey 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(">")) diff --git a/act/artifactcache/mac_test.go b/act/artifactcache/mac_test.go index a790bc1d..f51461e3 100644 --- a/act/artifactcache/mac_test.go +++ b/act/artifactcache/mac_test.go @@ -5,7 +5,6 @@ import ( "testing" "time" - "code.forgejo.org/forgejo/runner/v9/act/cacheproxy" "github.com/stretchr/testify/require" ) @@ -19,8 +18,8 @@ func TestMac(t *testing.T) { run := "1" ts := strconv.FormatInt(time.Now().Unix(), 10) - mac := computeMac(handler.secret, name, run, ts, "") - rundata := cacheproxy.RunData{ + mac := ComputeMac(handler.secret, name, run, ts, "") + rundata := RunData{ RepositoryFullName: name, RunNumber: run, Timestamp: ts, @@ -37,8 +36,8 @@ func TestMac(t *testing.T) { run := "1" ts := "9223372036854775807" // This should last us for a while... - mac := computeMac(handler.secret, name, run, ts, "") - rundata := cacheproxy.RunData{ + mac := ComputeMac(handler.secret, name, run, ts, "") + rundata := RunData{ RepositoryFullName: name, RunNumber: run, Timestamp: ts, @@ -54,7 +53,7 @@ func TestMac(t *testing.T) { run := "1" ts := strconv.FormatInt(time.Now().Unix(), 10) - rundata := cacheproxy.RunData{ + rundata := RunData{ RepositoryFullName: name, RunNumber: run, Timestamp: ts, @@ -72,12 +71,12 @@ func TestMac(t *testing.T) { run := "42" ts := "1337" - mac := computeMac(secret, name, run, ts, "") - expectedMac := "4754474b21329e8beadd2b4054aa4be803965d66e710fa1fee091334ed804f29" // * 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) - mac = computeMac(secret, name, run, ts, "refs/pull/12/head") - expectedMac = "9ca8f4cb5e1b083ee8cd215215bc00f379b28511d3ef7930bf054767de34766d" // * Precomputed, anytime the computeMac function changes this needs to be recalculated + 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) }) } diff --git a/act/cacheproxy/handler.go b/act/cacheproxy/handler.go index 2b2b3707..a9610a02 100644 --- a/act/cacheproxy/handler.go +++ b/act/cacheproxy/handler.go @@ -4,9 +4,6 @@ package cacheproxy import ( - "crypto/hmac" - "crypto/sha256" - "encoding/hex" "errors" "fmt" "io" @@ -22,6 +19,7 @@ import ( "github.com/julienschmidt/httprouter" "github.com/sirupsen/logrus" + "code.forgejo.org/forgejo/runner/v9/act/artifactcache" "code.forgejo.org/forgejo/runner/v9/act/common" ) @@ -46,17 +44,9 @@ type Handler struct { runs sync.Map } -type RunData struct { - RepositoryFullName string - RunNumber string - Timestamp string - RepositoryMAC string - WriteIsolationKey string -} - -func (h *Handler) CreateRunData(fullName, runNumber, timestamp, writeIsolationKey string) RunData { - mac := computeMac(h.cacheSecret, fullName, runNumber, timestamp, writeIsolationKey) - return RunData{ +func (h *Handler) CreateRunData(fullName, runNumber, timestamp, writeIsolationKey string) artifactcache.RunData { + mac := artifactcache.ComputeMac(h.cacheSecret, fullName, runNumber, timestamp, writeIsolationKey) + return artifactcache.RunData{ RepositoryFullName: fullName, RunNumber: runNumber, Timestamp: timestamp, @@ -142,7 +132,7 @@ func (h *Handler) newReverseProxy(targetHost string) (*httputil.ReverseProxy, er h.logger.Warn(fmt.Sprintf("Tried starting a cache proxy with id %s, which does not exist.", id)) return } - runData := data.(RunData) + runData := data.(artifactcache.RunData) uri := matches[2] r.SetURL(targetURL) @@ -170,7 +160,7 @@ func (h *Handler) ExternalURL() string { // Informs the proxy of a workflow run that can make cache requests. // The RunData contains the information about the repository. // The function returns the 32-bit random key which the run will use to identify itself. -func (h *Handler) AddRun(data RunData) (string, error) { +func (h *Handler) AddRun(data artifactcache.RunData) (string, error) { for range 3 { key := common.MustRandName(4) _, loaded := h.runs.LoadOrStore(key, data) @@ -211,15 +201,3 @@ func (h *Handler) Close() error { } return retErr } - -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)) -}