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.

<!--start release-notes-assistant-->
<!--URL:https://code.forgejo.org/forgejo/runner-->
- other
  - [PR](https://code.forgejo.org/forgejo/runner/pulls/936): <!--number 936 --><!--line 0 --><!--description cmVmYWN0b3I6IHJlbW92ZSBkdXBsaWNhdGUgY29tcHV0ZU1hYyBmdW5jdGlvbg==-->refactor: remove duplicate computeMac function<!--description-->
<!--end release-notes-assistant-->

Reviewed-on: https://code.forgejo.org/forgejo/runner/pulls/936
Reviewed-by: Michael Kriese <michael.kriese@gmx.de>
Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net>
Co-committed-by: Mathieu Fenniak <mathieu@fenniak.net>

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 {
+type RunData struct {
-	return cacheproxy.RunData{
+	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"),

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

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(">"))

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, "")
+		mac := ComputeMac(handler.secret, name, run, ts, "")
-		rundata := cacheproxy.RunData{
+		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, "")
+		mac := ComputeMac(handler.secret, name, run, ts, "")
-		rundata := cacheproxy.RunData{
+		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, "")
+		mac := ComputeMac(secret, name, run, ts, "")
-		expectedMac := "4754474b21329e8beadd2b4054aa4be803965d66e710fa1fee091334ed804f29" // * Precomputed, anytime the computeMac function changes this needs to be recalculated
+		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")
+		mac = ComputeMac(secret, name, run, ts, "refs/pull/12/head")
-		expectedMac = "9ca8f4cb5e1b083ee8cd215215bc00f379b28511d3ef7930bf054767de34766d" // * Precomputed, anytime the computeMac function changes this needs to be recalculated
+		expectedMac = "9ca8f4cb5e1b083ee8cd215215bc00f379b28511d3ef7930bf054767de34766d" // * Precomputed, anytime the ComputeMac function changes this needs to be recalculated
 		require.Equal(t, expectedMac, mac)
 	})
 }

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 {
+func (h *Handler) CreateRunData(fullName, runNumber, timestamp, writeIsolationKey string) artifactcache.RunData {
-	RepositoryFullName string
+	mac := artifactcache.ComputeMac(h.cacheSecret, fullName, runNumber, timestamp, writeIsolationKey)
-	RunNumber          string
+	return artifactcache.RunData{
-	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{
 		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))
-}