From 46eb63a952e2e403aea80316514d4815fa47b0c8 Mon Sep 17 00:00:00 2001 From: Kwonunn Date: Sat, 22 Mar 2025 00:03:09 +0000 Subject: [PATCH] Fix security issues with cache by proxying access (#503) This is the forgejo-runner-side patch for a partial overhaul of the cache system to fix some access control issues with caches. This code depends on changes in act which are being reviewed here: forgejo/act#107 Co-authored-by: Michael Kriese Reviewed-on: https://code.forgejo.org/forgejo/runner/pulls/502 Reviewed-on: https://code.forgejo.org/forgejo/runner/pulls/503 Reviewed-by: Gusted Co-authored-by: Kwonunn Co-committed-by: Kwonunn --- go.mod | 2 +- go.sum | 4 +- internal/app/cmd/cache-server.go | 24 +++-- internal/app/cmd/cmd.go | 1 + internal/app/cmd/exec.go | 2 +- internal/app/run/runner.go | 122 +++++++++++++++++++----- internal/pkg/config/config.example.yaml | 9 ++ internal/pkg/config/config.go | 13 ++- 8 files changed, 139 insertions(+), 38 deletions(-) diff --git a/go.mod b/go.mod index 04de7cb..d8dfcf5 100644 --- a/go.mod +++ b/go.mod @@ -102,4 +102,4 @@ require ( gopkg.in/yaml.v2 v2.4.0 // indirect ) -replace github.com/nektos/act => code.forgejo.org/forgejo/act v1.24.1 +replace github.com/nektos/act => code.forgejo.org/forgejo/act v1.25.0 diff --git a/go.sum b/go.sum index e7db01e..ef26e6d 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,5 @@ -code.forgejo.org/forgejo/act v1.24.1 h1:kTTMA2G4+vLFOsr4oTxvvaEf4JN42lIc/tu+TwbkmmU= -code.forgejo.org/forgejo/act v1.24.1/go.mod h1:tSg5CAHnXp4WLNkMa2e9AEDSujMxKzNM4bF2pvvRCYQ= +code.forgejo.org/forgejo/act v1.25.0 h1:UfQH75ZD88GzveWZeaZ0F3h0W9qpQGgyx7pq7nbHyA0= +code.forgejo.org/forgejo/act v1.25.0/go.mod h1:tSg5CAHnXp4WLNkMa2e9AEDSujMxKzNM4bF2pvvRCYQ= code.gitea.io/actions-proto-go v0.4.0 h1:OsPBPhodXuQnsspG1sQ4eRE1PeoZyofd7+i73zCwnsU= code.gitea.io/actions-proto-go v0.4.0/go.mod h1:mn7Wkqz6JbnTOHQpot3yDeHx+O5C9EGhMEE+htvHBas= code.gitea.io/gitea-vet v0.2.3 h1:gdFmm6WOTM65rE8FUBTRzeQZYzXePKSSB1+r574hWwI= diff --git a/internal/app/cmd/cache-server.go b/internal/app/cmd/cache-server.go index 21b3352..033f91d 100644 --- a/internal/app/cmd/cache-server.go +++ b/internal/app/cmd/cache-server.go @@ -17,9 +17,10 @@ import ( ) type cacheServerArgs struct { - Dir string - Host string - Port uint16 + Dir string + Host string + Port uint16 + Secret string } func runCacheServer(ctx context.Context, configFile *string, cacheArgs *cacheServerArgs) func(cmd *cobra.Command, args []string) error { @@ -32,9 +33,10 @@ func runCacheServer(ctx context.Context, configFile *string, cacheArgs *cacheSer initLogging(cfg) var ( - dir = cfg.Cache.Dir - host = cfg.Cache.Host - port = cfg.Cache.Port + dir = cfg.Cache.Dir + host = cfg.Cache.Host + port = cfg.Cache.Port + secret = cfg.Cache.Secret ) // cacheArgs has higher priority @@ -47,11 +49,21 @@ func runCacheServer(ctx context.Context, configFile *string, cacheArgs *cacheSer if cacheArgs.Port != 0 { port = cacheArgs.Port } + if cacheArgs.Secret != "" { + secret = cacheArgs.Secret + } + + if secret == "" { + // no cache secret was specified, panic + log.Error("no cache secret was specified, exiting.") + return nil + } cacheHandler, err := artifactcache.StartHandler( dir, host, port, + secret, log.StandardLogger().WithField("module", "cache_request"), ) if err != nil { diff --git a/internal/app/cmd/cmd.go b/internal/app/cmd/cmd.go index f8987db..390c506 100644 --- a/internal/app/cmd/cmd.go +++ b/internal/app/cmd/cmd.go @@ -85,6 +85,7 @@ func Execute(ctx context.Context) { cacheCmd.Flags().StringVarP(&cacheArgs.Dir, "dir", "d", "", "Cache directory") cacheCmd.Flags().StringVarP(&cacheArgs.Host, "host", "s", "", "Host of the cache server") cacheCmd.Flags().Uint16VarP(&cacheArgs.Port, "port", "p", 0, "Port of the cache server") + cacheCmd.Flags().StringVarP(&cacheArgs.Secret, "secret", "", "", "Shared cache secret") rootCmd.AddCommand(cacheCmd) // hide completion command diff --git a/internal/app/cmd/exec.go b/internal/app/cmd/exec.go index ea1718b..404f4ae 100644 --- a/internal/app/cmd/exec.go +++ b/internal/app/cmd/exec.go @@ -352,7 +352,7 @@ func runExec(ctx context.Context, execArgs *executeArgs) func(cmd *cobra.Command } // init a cache server - handler, err := artifactcache.StartHandler("", "", 0, log.StandardLogger().WithField("module", "cache_request")) + handler, err := artifactcache.StartHandler("", "", 0, "", log.StandardLogger().WithField("module", "cache_request")) if err != nil { return err } diff --git a/internal/app/run/runner.go b/internal/app/run/runner.go index 27bdf3e..4c6d655 100644 --- a/internal/app/run/runner.go +++ b/internal/app/run/runner.go @@ -5,9 +5,12 @@ package run import ( "context" + "crypto/rand" + "encoding/hex" "encoding/json" "fmt" "path/filepath" + "strconv" "strings" "sync" "time" @@ -16,6 +19,7 @@ import ( "connectrpc.com/connect" "github.com/docker/docker/api/types/container" "github.com/nektos/act/pkg/artifactcache" + "github.com/nektos/act/pkg/cacheproxy" "github.com/nektos/act/pkg/common" "github.com/nektos/act/pkg/model" "github.com/nektos/act/pkg/runner" @@ -38,6 +42,8 @@ type Runner struct { labels labels.Labels envs map[string]string + cacheProxy *cacheproxy.Handler + runningTasks sync.Map } @@ -63,23 +69,12 @@ func NewRunner(cfg *config.Config, reg *config.Registration, cli client.Client) for k, v := range cfg.Runner.Envs { envs[k] = v } + + var cacheProxy *cacheproxy.Handler if cfg.Cache.Enabled == nil || *cfg.Cache.Enabled { - if cfg.Cache.ExternalServer != "" { - envs["ACTIONS_CACHE_URL"] = cfg.Cache.ExternalServer - } else { - cacheHandler, err := artifactcache.StartHandler( - cfg.Cache.Dir, - cfg.Cache.Host, - cfg.Cache.Port, - log.StandardLogger().WithField("module", "cache_request"), - ) - if err != nil { - log.Errorf("cannot init cache server, it will be disabled: %v", err) - // go on - } else { - envs["ACTIONS_CACHE_URL"] = cacheHandler.ExternalURL() + "/" - } - } + cacheProxy = setupCache(cfg, envs) + } else { + cacheProxy = nil } // set artifact gitea api @@ -92,14 +87,77 @@ func NewRunner(cfg *config.Config, reg *config.Registration, cli client.Client) envs["GITEA_ACTIONS_RUNNER_VERSION"] = ver.Version() return &Runner{ - name: reg.Name, - cfg: cfg, - client: cli, - labels: ls, - envs: envs, + name: reg.Name, + cfg: cfg, + client: cli, + labels: ls, + envs: envs, + cacheProxy: cacheProxy, } } +func setupCache(cfg *config.Config, envs map[string]string) *cacheproxy.Handler { + var cacheUrl string + var cacheSecret string + + if cfg.Cache.ExternalServer == "" { + // No external cache server was specified, start internal cache server + cacheSecret = cfg.Cache.Secret + + if cacheSecret == "" { + // no cache secret was specified, generate one + secretBytes := make([]byte, 64) + _, err := rand.Read(secretBytes) + if err != nil { + log.Errorf("Failed to generate random bytes, this should not happen") + } + cacheSecret = hex.EncodeToString(secretBytes) + } + + cacheServer, err := artifactcache.StartHandler( + cfg.Cache.Dir, + cfg.Cache.Host, + cfg.Cache.Port, + cacheSecret, + log.StandardLogger().WithField("module", "cache_request"), + ) + if err != nil { + log.Error("Could not start the cache server, cache will be disabled") + return nil + } + + cacheUrl = cacheServer.ExternalURL() + } else { + // An external cache server was specified, use its url + cacheSecret = cfg.Cache.Secret + + if cacheSecret == "" { + log.Error("A cache secret must be specified to use an external cache server, cache will be disabled") + return nil + } + + cacheUrl = strings.TrimSuffix(cfg.Cache.ExternalServer, "/") + } + + cacheProxy, err := cacheproxy.StartHandler( + cacheUrl, + cfg.Cache.Host, + cfg.Cache.ProxyPort, + cacheSecret, + log.StandardLogger().WithField("module", "cache_proxy"), + ) + if err != nil { + log.Errorf("cannot init cache proxy, cache will be disabled: %v", err) + } + + envs["ACTIONS_CACHE_URL"] = cacheProxy.ExternalURL() + if cfg.Cache.ActionsCacheUrlOverride != "" { + envs["ACTIONS_CACHE_URL"] = cfg.Cache.ActionsCacheUrlOverride + } + + return cacheProxy +} + func (r *Runner) Run(ctx context.Context, task *runnerv1.Task) error { if _, ok := r.runningTasks.Load(task.Id); ok { return fmt.Errorf("task %d is already running", task.Id) @@ -176,12 +234,30 @@ func (r *Runner) run(ctx context.Context, task *runnerv1.Task, reporter *report. preset.Token = t } + // Clone the runner default envs into a local envs map + runEnvs := make(map[string]string) + for id, v := range r.envs { + runEnvs[id] = v + } + giteaRuntimeToken := taskContext["gitea_runtime_token"].GetStringValue() if giteaRuntimeToken == "" { // use task token to action api token for previous Gitea Server Versions giteaRuntimeToken = preset.Token } - r.envs["ACTIONS_RUNTIME_TOKEN"] = giteaRuntimeToken + runEnvs["ACTIONS_RUNTIME_TOKEN"] = giteaRuntimeToken + + // Register the run with the cacheproxy and modify the CACHE_URL + if r.cacheProxy != nil { + timestamp := strconv.FormatInt(time.Now().Unix(), 10) + cacheRunData := r.cacheProxy.CreateRunData(preset.Repository, preset.RunID, timestamp) + cacheRunId, err := r.cacheProxy.AddRun(cacheRunData) + if err == nil { + defer r.cacheProxy.RemoveRun(cacheRunId) + baseURL := runEnvs["ACTIONS_CACHE_URL"] + runEnvs["ACTIONS_CACHE_URL"] = fmt.Sprintf("%s/%s/", baseURL, cacheRunId) + } + } eventJSON, err := json.Marshal(preset.Event) if err != nil { @@ -212,7 +288,7 @@ func (r *Runner) run(ctx context.Context, task *runnerv1.Task, reporter *report. ForceRebuild: r.cfg.Container.ForceRebuild, LogOutput: true, JSONLogger: false, - Env: r.envs, + Env: runEnvs, Secrets: task.Secrets, GitHubInstance: strings.TrimSuffix(r.client.Address(), "/"), AutoRemove: true, diff --git a/internal/pkg/config/config.example.yaml b/internal/pkg/config/config.example.yaml index bcd2117..a2fdc50 100644 --- a/internal/pkg/config/config.example.yaml +++ b/internal/pkg/config/config.example.yaml @@ -58,10 +58,19 @@ cache: # The port of the cache server. # 0 means to use a random available port. port: 0 + # The port of the cache proxy. + # 0 means to use a random available port. + proxy_port: 0 # The external cache server URL. Valid only when enable is true. # If it's specified, it will be used to set the ACTIONS_CACHE_URL environment variable. The URL should generally end with "/". # Otherwise it will be set to the the URL of the internal cache server. external_server: "" + # The shared cache secret. When communicating with a cache server, the runner uses this secret to verify the authenticity of the cache requests. + # When using an external cache server it is required to set the same secret for the runner and the cache server. + secret: "" + # Overrides the ACTIONS_CACHE_URL passed to workflow containers. This should only be used if the runner host is not reachable from the + # workflow containers, and requires further setup. + actions_cache_url_override: "" container: # Specifies the network to which the container will connect. diff --git a/internal/pkg/config/config.go b/internal/pkg/config/config.go index 3d95044..6811dd2 100644 --- a/internal/pkg/config/config.go +++ b/internal/pkg/config/config.go @@ -37,11 +37,14 @@ type Runner struct { // Cache represents the configuration for caching. type Cache struct { - Enabled *bool `yaml:"enabled"` // Enabled indicates whether caching is enabled. It is a pointer to distinguish between false and not set. If not set, it will be true. - Dir string `yaml:"dir"` // Dir specifies the directory path for caching. - Host string `yaml:"host"` // Host specifies the caching host. - Port uint16 `yaml:"port"` // Port specifies the caching port. - ExternalServer string `yaml:"external_server"` // ExternalServer specifies the URL of external cache server + Enabled *bool `yaml:"enabled"` // Enabled indicates whether caching is enabled. It is a pointer to distinguish between false and not set. If not set, it will be true. + Dir string `yaml:"dir"` // Dir specifies the directory path for caching. + Host string `yaml:"host"` // Host specifies the caching host. + Port uint16 `yaml:"port"` // Port specifies the caching port. + ProxyPort uint16 `yaml:"proxy_port"` // ProxyPort specifies the cache proxy port. + ExternalServer string `yaml:"external_server"` // ExternalServer specifies the URL of external cache server + ActionsCacheUrlOverride string `yaml:"actions_cache_url_override"` // Allows the user to override the ACTIONS_CACHE_URL passed to the workflow containers + Secret string `yaml:"secret"` // Shared secret to secure caches. } // Container represents the configuration for the container.