From e58b9b4357393162d5943718f7affc20c2f2d5e2 Mon Sep 17 00:00:00 2001 From: user-fPoq4eSbDgj__95j Date: Wed, 28 Oct 2026 02:16:09 +0000 Subject: [PATCH] fix(security): harden server-side LFS snapshots --- ...{.git-lfs-3.7.1.pkg => .git-lfs-3.8.1.pkg} | 0 bin/git-lfs | 2 +- docker/Dockerfile | 14 ++++- internal/strategy/apiv1.go | 10 ++- internal/strategy/apiv1_test.go | 22 +++++++ internal/strategy/git/git.go | 37 ++++++++++- internal/strategy/git/lfs_security_test.go | 62 +++++++++++++++++++ internal/strategy/git/snapshot.go | 35 ++++++++++- 8 files changed, 176 insertions(+), 6 deletions(-) rename bin/{.git-lfs-3.7.1.pkg => .git-lfs-3.8.1.pkg} (100%) create mode 100644 internal/strategy/git/lfs_security_test.go diff --git a/bin/.git-lfs-3.7.1.pkg b/bin/.git-lfs-3.8.1.pkg similarity index 100% rename from bin/.git-lfs-3.7.1.pkg rename to bin/.git-lfs-3.8.1.pkg diff --git a/bin/git-lfs b/bin/git-lfs index 09e12ebf..e75ca93f 120000 --- a/bin/git-lfs +++ b/bin/git-lfs @@ -1 +1 @@ -.git-lfs-3.7.1.pkg \ No newline at end of file +.git-lfs-3.8.1.pkg \ No newline at end of file diff --git a/docker/Dockerfile b/docker/Dockerfile index 734804f5..d5812c17 100644 --- a/docker/Dockerfile +++ b/docker/Dockerfile @@ -2,11 +2,23 @@ FROM alpine:3.23 ARG TARGETARCH +ARG GIT_LFS_VERSION=3.8.1 SHELL ["/bin/sh", "-o", "pipefail", "-c"] # Install runtime dependencies for git operations and TLS -RUN apk add --no-cache 'ca-certificates>=20261020-r0' curl git git-daemon git-lfs tzdata zstd && \ +RUN apk add --no-cache 'ca-certificates>=20261020-r0' curl git git-daemon tzdata zstd && \ + case "${TARGETARCH}" in \ + amd64) git_lfs_sha=e72cf723a4a660438862282e8aecf3885fc8123e88bfcad534cf6b941a354b75 ;; \ + arm64) git_lfs_sha=67895dd77b63329796bc5efdd47b57f218e1f7b98717ebaf936a8238dbcaec0f ;; \ + *) echo "unsupported architecture: ${TARGETARCH}" >&2; exit 1 ;; \ + esac && \ + curl -fsSLo /tmp/git-lfs.tar.gz \ + "https://github.com/git-lfs/git-lfs/releases/download/v${GIT_LFS_VERSION}/git-lfs-linux-${TARGETARCH}-v${GIT_LFS_VERSION}.tar.gz" && \ + echo "${git_lfs_sha} /tmp/git-lfs.tar.gz" | sha256sum -c - && \ + tar -xzf /tmp/git-lfs.tar.gz -C /tmp "git-lfs-${GIT_LFS_VERSION}/git-lfs" && \ + mv "/tmp/git-lfs-${GIT_LFS_VERSION}/git-lfs" /usr/local/bin/git-lfs && \ + chmod 0755 /usr/local/bin/git-lfs && \ addgroup -g 1000 cachew && \ adduser -D -u 1000 -G cachew cachew diff --git a/internal/strategy/apiv1.go b/internal/strategy/apiv1.go index e05561ea..e5875aad 100644 --- a/internal/strategy/apiv1.go +++ b/internal/strategy/apiv1.go @@ -119,7 +119,7 @@ func (d *APIV1) putObject(w http.ResponseWriter, r *http.Request) { } // Extract and filter headers from request - headers := httputil.FilterHeaders(r.Header, httputil.TransportHeaders...) + headers := apiObjectHeaders(r.Header) var opts []cache.Option if etag := headers.Get(cache.ETagKey); etag != "" { rawETag, err := cache.RawETagFromHeader(etag) @@ -149,6 +149,14 @@ func (d *APIV1) putObject(w http.ResponseWriter, r *http.Request) { } } +func apiObjectHeaders(headers http.Header) http.Header { + filtered := httputil.FilterHeaders(headers, httputil.TransportHeaders...) + filtered.Del("Authorization") + filtered.Del("Cookie") + filtered.Del("Proxy-Authorization") + return filtered +} + func (d *APIV1) deleteObject(w http.ResponseWriter, r *http.Request) { namespace, err := cache.ParseNamespace(r.PathValue("namespace")) if err != nil { diff --git a/internal/strategy/apiv1_test.go b/internal/strategy/apiv1_test.go index e0d1bbb1..bb73a29b 100644 --- a/internal/strategy/apiv1_test.go +++ b/internal/strategy/apiv1_test.go @@ -77,6 +77,28 @@ func apiPut(ctx context.Context, t *testing.T, handler http.Handler, key cache.K return etag } +func TestPutObjectDoesNotPersistCredentialHeaders(t *testing.T) { + handler, ctx := testAPISetup(t) + key := cache.NewKey("credential-headers") + req := httptest.NewRequest(http.MethodPost, "/api/v1/object/test/"+key.String(), strings.NewReader("test data")) + req.Header.Set("Authorization", "Bearer secret") + req.Header.Set("Cookie", "session=secret") + req.Header.Set("Proxy-Authorization", "Basic secret") + req.Header.Set("X-Custom", "kept") + w := httptest.NewRecorder() + handler.ServeHTTP(w, req.WithContext(ctx)) + assert.Equal(t, http.StatusOK, w.Code) + + req = httptest.NewRequest(http.MethodGet, "/api/v1/object/test/"+key.String(), nil) + w = httptest.NewRecorder() + handler.ServeHTTP(w, req.WithContext(ctx)) + assert.Equal(t, http.StatusOK, w.Code) + assert.Zero(t, w.Header().Get("Authorization")) + assert.Zero(t, w.Header().Get("Cookie")) + assert.Zero(t, w.Header().Get("Proxy-Authorization")) + assert.Equal(t, "kept", w.Header().Get("X-Custom")) +} + func TestConditionalGetIfNoneMatch(t *testing.T) { handler, ctx := testAPISetup(t) key := cache.NewKey("cond-get") diff --git a/internal/strategy/git/git.go b/internal/strategy/git/git.go index 46f36efc..d47fdb95 100644 --- a/internal/strategy/git/git.go +++ b/internal/strategy/git/git.go @@ -24,6 +24,7 @@ import ( "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" "go.opentelemetry.io/otel/trace" + "golang.org/x/mod/semver" "github.com/block/cachew/internal/cache" "github.com/block/cachew/internal/gitclone" @@ -38,6 +39,11 @@ import ( //nolint:gochecknoglobals // OTel tracer instances are package-scoped by convention var tracer = otel.Tracer("github.com/block/cachew/internal/strategy/git") +const minimumGitLFSVersion = "v3.8.1" + +//nolint:gochecknoglobals // The executable and its version are process-wide. +var gitLFSVersionCheck = sync.OnceValue(checkGitLFSVersion) + func Register(r *strategy.Registry, scheduler jobscheduler.Provider, cloneManagerProvider gitclone.ManagerProvider, tokenManagerProvider githubapp.TokenManagerProvider) { strategy.Register(r, "git", "Caches Git repositories, including tarball snapshots.", func(ctx context.Context, config Config, cache cache.Cache, mux strategy.Mux) (*Strategy, error) { return New(ctx, config, scheduler, cache, mux, cloneManagerProvider, tokenManagerProvider) @@ -108,8 +114,8 @@ func New( logger := logging.FromContext(ctx) - if _, err := exec.LookPath("git-lfs"); err != nil { - return nil, errors.New("git-lfs is required but not found in PATH") + if err := gitLFSVersionCheck(); err != nil { + return nil, err } // Get GitHub App token manager if configured @@ -226,6 +232,33 @@ func New( return s, nil } +func gitLFSVersionSupported(output []byte) bool { + _, versionText, ok := strings.Cut(string(output), "git-lfs/") + if !ok { + return false + } + fields := strings.Fields(versionText) + if len(fields) == 0 { + return false + } + version := "v" + fields[0] + return semver.IsValid(version) && semver.Compare(version, minimumGitLFSVersion) >= 0 +} + +func checkGitLFSVersion() error { + if _, err := exec.LookPath("git-lfs"); err != nil { + return errors.New("git-lfs is required but not found in PATH") + } + gitLFSVersion, err := exec.CommandContext(context.Background(), "git-lfs", "version").CombinedOutput() + if err != nil { + return errors.Wrapf(err, "run git-lfs version: %s", string(gitLFSVersion)) + } + if !gitLFSVersionSupported(gitLFSVersion) { + return errors.Errorf("git-lfs %s or newer is required", strings.TrimPrefix(minimumGitLFSVersion, "v")) + } + return nil +} + var _ strategy.Strategy = (*Strategy)(nil) var _ strategy.Readier = (*Strategy)(nil) var _ strategy.MetadataConsumer = (*Strategy)(nil) diff --git a/internal/strategy/git/lfs_security_test.go b/internal/strategy/git/lfs_security_test.go new file mode 100644 index 00000000..3f2a67e5 --- /dev/null +++ b/internal/strategy/git/lfs_security_test.go @@ -0,0 +1,62 @@ +package git //nolint:testpackage // white-box testing is required for process-boundary hardening + +import ( + "os" + "path/filepath" + "runtime" + "testing" + + "github.com/alecthomas/assert/v2" +) + +func TestGitLFSVersionSupported(t *testing.T) { + assert.False(t, gitLFSVersionSupported([]byte("git-lfs/3.7.1 (GitHub; linux amd64; go 1.24.4)"))) + assert.False(t, gitLFSVersionSupported([]byte("unexpected output"))) + assert.True(t, gitLFSVersionSupported([]byte("git-lfs/3.8.1 (GitHub; linux amd64; go 1.27.3)"))) + assert.True(t, gitLFSVersionSupported([]byte("git-lfs/4.0.0 (GitHub; linux amd64; go 1.28)"))) +} + +func TestLFSEndpointForUpstream(t *testing.T) { + tests := []struct { + upstream string + expected string + }{ + {"https://github.com/org/repo", "https://github.com/org/repo.git/info/lfs"}, + {"https://github.com/org/repo.git", "https://github.com/org/repo.git/info/lfs"}, + } + for _, test := range tests { + actual, err := lfsEndpointForUpstream(test.upstream) + assert.NoError(t, err) + assert.Equal(t, test.expected, actual) + } +} + +func TestLFSEndpointForUpstreamRejectsUnsafeURL(t *testing.T) { + for _, upstream := range []string{ + "http://127.0.0.1/org/repo", + "https://user@example.com/org/repo", + "https://example.com/org/repo?endpoint=http://127.0.0.1", + "https://example.com/org/repo#x", + } { + _, err := lfsEndpointForUpstream(upstream) + assert.Error(t, err) + } +} + +func TestReplaceRepositoryLFSConfigDoesNotFollowSymlink(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink setup requires privileges on Windows") + } + outside := filepath.Join(t.TempDir(), "outside") + assert.NoError(t, os.WriteFile(outside, []byte("preserve"), 0o600)) + workDir := t.TempDir() + assert.NoError(t, os.Symlink(outside, filepath.Join(workDir, ".lfsconfig"))) + + assert.NoError(t, replaceRepositoryLFSConfig(workDir)) + contents, err := os.ReadFile(outside) + assert.NoError(t, err) + assert.Equal(t, "preserve", string(contents)) + info, err := os.Lstat(filepath.Join(workDir, ".lfsconfig")) + assert.NoError(t, err) + assert.False(t, info.Mode()&os.ModeSymlink != 0) +} diff --git a/internal/strategy/git/snapshot.go b/internal/strategy/git/snapshot.go index e8b5fd4b..0c7d35bc 100644 --- a/internal/strategy/git/snapshot.go +++ b/internal/strategy/git/snapshot.go @@ -6,6 +6,7 @@ import ( "io" "math/rand/v2" "net/http" + "net/url" "os" "os/exec" "path/filepath" @@ -1573,6 +1574,9 @@ func (s *Strategy) generateAndUploadLFSSnapshot(ctx context.Context, repo *gitcl return errors.Wrap(err, "rev-parse HEAD for LFS snapshot") } commit = headSHA + if err := replaceRepositoryLFSConfig(workDir); err != nil { + return err + } // Set up LFS in the snapshot clone. cloneForSnapshot already restores // remote.origin.url to the upstream URL, so LFS will fetch from GitHub. @@ -1588,7 +1592,14 @@ func (s *Strategy) generateAndUploadLFSSnapshot(ctx context.Context, repo *gitcl // token can't expire mid-fetch and trigger a retry storm. fetchStart := time.Now() fetchCtx, cancel := context.WithTimeout(ctx, lfsFetchTimeout) - fetchCmd, err := repo.GitCommand(fetchCtx, "-C", workDir, "lfs", "fetch", "origin", "HEAD") + lfsURL, err := lfsEndpointForUpstream(upstream) + if err != nil { + cancel() + s.metrics.recordLFSPhase(ctx, upstream, "fetch", "error", time.Since(fetchStart)) + return errors.Wrap(err, "derive LFS endpoint") + } + fetchCmd, err := repo.GitCommand(fetchCtx, "-c", "lfs.url="+lfsURL, "-c", "lfs.basictransfersonly=true", + "-c", "lfs.standalonetransferagent=", "-C", workDir, "lfs", "fetch", "origin", "HEAD") if err != nil { cancel() s.metrics.recordLFSPhase(ctx, upstream, "fetch", "error", time.Since(fetchStart)) @@ -1643,6 +1654,28 @@ func (s *Strategy) generateAndUploadLFSSnapshot(ctx context.Context, repo *gitcl return commit, nil } +func lfsEndpointForUpstream(upstream string) (string, error) { + parsed, err := url.Parse(upstream) + if err != nil { + return "", errors.Wrap(err, "parse upstream URL") + } + if parsed.Scheme != "https" || parsed.Hostname() == "" || parsed.User != nil || parsed.RawQuery != "" || + parsed.Fragment != "" { + return "", errors.Errorf("unsupported upstream URL %q", upstream) + } + parsed.Path = strings.TrimSuffix(strings.TrimSuffix(parsed.Path, "/"), ".git") + ".git/info/lfs" + parsed.RawPath = "" + return parsed.String(), nil +} + +func replaceRepositoryLFSConfig(workDir string) error { + lfsConfigPath := filepath.Join(workDir, ".lfsconfig") + if err := os.RemoveAll(lfsConfigPath); err != nil { + return errors.Wrap(err, "remove repository LFS configuration") + } + return errors.Wrap(os.WriteFile(lfsConfigPath, nil, 0o600), "replace repository LFS configuration") +} + // dirSizeBytes returns the total size in bytes of regular files under root. // Per-entry stat or walk errors are deliberately swallowed so a transient // failure (e.g. a file removed mid-walk during snapshot prep) doesn't fail