diff --git a/internal/gitutil/gitutil.go b/internal/gitutil/gitutil.go index 2a6ce25add..646c0d8c34 100644 --- a/internal/gitutil/gitutil.go +++ b/internal/gitutil/gitutil.go @@ -23,11 +23,14 @@ import ( "encoding/hex" "fmt" "io" + "maps" "os" "os/exec" "path/filepath" "regexp" "runtime" + "slices" + "sort" "strings" "time" @@ -39,6 +42,32 @@ import ( // for remote repos. Defaults to UserHomeDir/.kpt/repos if unspecified. const RepoCacheDirEnv = "KPT_CACHE_DIR" +type GitArgKind string + +const ( + GitArgRef GitArgKind = "reference" + GitArgCommit GitArgKind = "commit" + GitArgRepoURI GitArgKind = "repo URI" +) + +// validateGitArg guards against git argument injection. Values such as refs, +// tags, refspecs, commit SHAs and repo URIs are frequently attacker-controlled +// (e.g. read from the upstream block of a remote sub-package's Kptfile) and are +// passed to git as positional arguments. Git never accepts a ref, refspec, +// commit, or URI that begins with a dash, so any such value would instead be +// interpreted as a command-line option (for example `--output=` for +// `git show`, or `--upload-pack=` for `git fetch`). Rejecting values that +// begin with a dash prevents these option-injection attacks without breaking +// any legitimate input. +func validateGitArg(kind GitArgKind, value string) error { + const op errors.Op = "gitutil.validateGitArg" + if strings.HasPrefix(value, "-") { + return errors.E(op, errors.InvalidParam, fmt.Errorf( + "invalid git %s %q: must not begin with '-'", kind, value)) + } + return nil +} + // NewLocalGitRunner returns a new GitLocalRunner for a local package. func NewLocalGitRunner(pkg string) (*GitLocalRunner, error) { const op errors.Op = "gitutil.NewLocalGitRunner" @@ -139,25 +168,15 @@ func (g *GitLocalRunner) run(ctx context.Context, verbose bool, command string, }, nil } -type NewGitUpstreamRepoOption func(*GitUpstreamRepo) - -func WithFetchedRefs(a map[string]bool) NewGitUpstreamRepoOption { - return func(g *GitUpstreamRepo) { - g.fetchedRefs = a - } -} - // NewGitUpstreamRepo returns a new GitUpstreamRepo for an upstream package. -func NewGitUpstreamRepo(ctx context.Context, uri string, opts ...NewGitUpstreamRepoOption) (*GitUpstreamRepo, error) { +func NewGitUpstreamRepo(ctx context.Context, uri string) (GitUpstreamRepo, error) { const op errors.Op = "gitutil.NewGitUpstreamRepo" - g := &GitUpstreamRepo{ - URI: uri, - } - for _, opt := range opts { - opt(g) + if err := validateGitArg(GitArgRepoURI, uri); err != nil { + return nil, errors.E(op, errors.Repo(uri), err) } - if g.fetchedRefs == nil { - g.fetchedRefs = map[string]bool{} + g := &gitUpstreamRepoBroker{ + uri: uri, + fetchedRefs: map[string]bool{}, } if err := g.updateRefs(ctx); err != nil { return nil, errors.E(op, errors.Repo(uri), err) @@ -165,57 +184,90 @@ func NewGitUpstreamRepo(ctx context.Context, uri string, opts ...NewGitUpstreamR return g, nil } -// GitUpstreamRepo runs git commands in a local git repo. -type GitUpstreamRepo struct { - URI string +type GitUpstreamRepo interface { + Uri() string + Heads() []string + Tags() []string + GetFetchedRefs() []string + GetRepo(ctx context.Context, refs []string) (string, error) + GetDefaultBranch(ctx context.Context) (string, error) + ResolveBranch(branch string) (string, bool) + ResolveTag(tag string) (string, bool) + ResolveRef(ref string) string +} - // Heads contains all head refs in the upstream repo as well as the - // each of the are referencing. - Heads map[string]string +// gitUpstreamRepoBroker runs git commands in a local git repo. +type gitUpstreamRepoBroker struct { + uri string - // Tags contains all tag refs in the upstream repo as well as the - // each of the are referencing. - Tags map[string]string + // commitByHead contains all head refs in the upstream repo + commitByHead map[string]string - // fetchedRefs keeps track of refs already fetched from remote + // commitByTag contains all tag refs in the upstream repo + commitByTag map[string]string + + // fetchedRefs keeps track of the commits already fetched from remote. It is + // keyed by the resolved commit SHA (not the ref name) so that a branch or + // tag that has moved to a new commit is fetched again rather than skipped. fetchedRefs map[string]bool } -func (gur *GitUpstreamRepo) GetFetchedRefs() []string { - fetchedRefs := make([]string, 0, len(gur.fetchedRefs)) +func (gur *gitUpstreamRepoBroker) Uri() string { + return gur.uri +} + +func (gur *gitUpstreamRepoBroker) Heads() []string { + heads := slices.Sorted(maps.Keys(gur.commitByHead)) + if heads == nil { + return []string{} + } + return heads +} + +func (gur *gitUpstreamRepoBroker) Tags() []string { + tags := slices.Sorted(maps.Keys(gur.commitByTag)) + if tags == nil { + return []string{} + } + return tags +} + +func (gur *gitUpstreamRepoBroker) GetFetchedRefs() []string { + refs := make([]string, 0, len(gur.fetchedRefs)) for ref := range gur.fetchedRefs { - fetchedRefs = append(fetchedRefs, ref) + refs = append(refs, ref) } - return fetchedRefs + sort.Strings(refs) + return refs } // updateRefs fetches all refs from the upstream git repo, parses the results -// and caches all refs and the commit they reference. Not that this doesn't +// and caches all refs and the commit they reference. Note that this doesn't // download any objects, only refs. -func (gur *GitUpstreamRepo) updateRefs(ctx context.Context) error { +func (gur *gitUpstreamRepoBroker) updateRefs(ctx context.Context) error { const op errors.Op = "gitutil.updateRefs" - repoCacheDir, err := gur.cacheRepo(ctx, gur.URI, []string{}, []string{}) + repoCacheDir, err := gur.cacheRepo(ctx, nil) if err != nil { - return errors.E(op, errors.Repo(gur.URI), err) + return errors.E(op, errors.Repo(gur.uri), err) } gitRunner, err := NewLocalGitRunner(repoCacheDir) if err != nil { - return errors.E(op, errors.Repo(gur.URI), err) + return errors.E(op, errors.Repo(gur.uri), err) } rr, err := gitRunner.Run(ctx, "ls-remote", "--heads", "--tags", "--refs", "origin") if err != nil { AmendGitExecError(err, func(e *GitExecError) { - e.Repo = gur.URI + e.Repo = gur.uri }) // TODO: This should only fail if we can't connect to the repo. We should // consider exposing the error message from git to the user here. - return errors.E(op, errors.Repo(gur.URI), err) + return errors.E(op, errors.Repo(gur.uri), err) } - heads := make(map[string]string) - tags := make(map[string]string) + commitByHead := make(map[string]string) + commitByTag := make(map[string]string) re := regexp.MustCompile(`^([a-z0-9]+)\s+refs/(heads|tags)/(.+)$`) scanner := bufio.NewScanner(bytes.NewBufferString(rr.Stdout)) @@ -225,64 +277,92 @@ func (gur *GitUpstreamRepo) updateRefs(ctx context.Context) error { if len(res) == 0 { continue } - switch res[2] { + commit := res[1] + kind := res[2] + name := res[3] + switch kind { case "heads": - heads[res[3]] = res[1] + if err := validateGitArg(GitArgRef, name); err != nil { + return errors.E(op, errors.Repo(gur.uri), err) + } + if err := validateGitArg(GitArgCommit, commit); err != nil { + return errors.E(op, errors.Repo(gur.uri), err) + } + commitByHead[name] = commit case "tags": - tags[res[3]] = res[1] + if err := validateGitArg(GitArgRef, name); err != nil { + return errors.E(op, errors.Repo(gur.uri), err) + } + if err := validateGitArg(GitArgCommit, commit); err != nil { + return errors.E(op, errors.Repo(gur.uri), err) + } + commitByTag[name] = commit } } if err := scanner.Err(); err != nil { - return errors.E(op, errors.Repo(gur.URI), errors.Git, + return errors.E(op, errors.Repo(gur.uri), errors.Git, fmt.Errorf("error parsing response from git: %w", err)) } - gur.Heads = heads - gur.Tags = tags + gur.commitByHead = commitByHead + gur.commitByTag = commitByTag return nil } // GetRepo fetches all the provided refs and the objects. It will fetch it // to the cache repo and returns the path to the local git clone in the cache // directory. -func (gur *GitUpstreamRepo) GetRepo(ctx context.Context, refs []string) (string, error) { +func (gur *gitUpstreamRepoBroker) GetRepo(ctx context.Context, refs []string) (string, error) { const op errors.Op = "gitutil.GetRepo" - dir, err := gur.cacheRepo(ctx, gur.URI, refs, []string{}) + for _, ref := range refs { + if err := validateGitArg(GitArgRef, ref); err != nil { + return "", errors.E(op, errors.Repo(gur.uri), err) + } + } + // Refresh our view of the upstream refs so that a branch (or tag) that has + // moved to a new commit since this broker was created resolves to its + // current commit. Together with keying fetchedRefs on the resolved commit + // in cacheRepo, this ensures a moved ref is fetched again instead of being + // served the stale commit cached at construction time. + if err := gur.updateRefs(ctx); err != nil { + return "", errors.E(op, errors.Repo(gur.uri), err) + } + dir, err := gur.cacheRepo(ctx, refs) if err != nil { - return "", errors.E(op, errors.Repo(gur.URI), err) + return "", errors.E(op, errors.Repo(gur.uri), err) } return dir, nil } // GetDefaultBranch returns the name of the branch pointed to by the // HEAD symref. This is the default branch of the repository. -func (gur *GitUpstreamRepo) GetDefaultBranch(ctx context.Context) (string, error) { +func (gur *gitUpstreamRepoBroker) GetDefaultBranch(ctx context.Context) (string, error) { const op errors.Op = "gitutil.GetDefaultBranch" - cacheRepo, err := gur.cacheRepo(ctx, gur.URI, []string{}, []string{}) + cacheRepo, err := gur.cacheRepo(ctx, nil) if err != nil { - return "", errors.E(op, errors.Repo(gur.URI), err) + return "", errors.E(op, errors.Repo(gur.uri), err) } gitRunner, err := NewLocalGitRunner(cacheRepo) if err != nil { - return "", errors.E(op, errors.Repo(gur.URI), err) + return "", errors.E(op, errors.Repo(gur.uri), err) } rr, err := gitRunner.Run(ctx, "ls-remote", "--symref", "origin", "HEAD") if err != nil { AmendGitExecError(err, func(e *GitExecError) { - e.Repo = gur.URI + e.Repo = gur.uri }) - return "", errors.E(op, errors.Repo(gur.URI), err) + return "", errors.E(op, errors.Repo(gur.uri), err) } if rr.Stdout == "" { - return "", errors.E(op, errors.Repo(gur.URI), + return "", errors.E(op, errors.Repo(gur.uri), fmt.Errorf("unable to detect default branch in repo")) } re := regexp.MustCompile(`ref: refs/heads/([^\s/]+)\s*HEAD`) match := re.FindStringSubmatch(rr.Stdout) if len(match) != 2 { - return "", errors.E(op, errors.Repo(gur.URI), errors.Git, + return "", errors.E(op, errors.Repo(gur.uri), errors.Git, fmt.Errorf("unexpected response from git when determining default branch: %s", rr.Stdout)) } return match[1], nil @@ -291,43 +371,38 @@ func (gur *GitUpstreamRepo) GetDefaultBranch(ctx context.Context) (string, error // ResolveBranch resolves the branch to a commit SHA. This happens based on the // cached information about refs in the upstream repo. If the branch doesn't exist // in the upstream repo, the last return value will be false. -func (gur *GitUpstreamRepo) ResolveBranch(branch string) (string, bool) { +func (gur *gitUpstreamRepoBroker) ResolveBranch(branch string) (string, bool) { branch = strings.TrimPrefix(branch, "refs/heads/") - for head, commit := range gur.Heads { - if head == branch { - return commit, true - } - } - return "", false + commit, ok := gur.commitByHead[branch] + return commit, ok } // ResolveTag resolves the tag to a commit SHA. This happens based on the // cached information about refs in the upstream repo. If the tag doesn't exist // in the upstream repo, the last return value will be false. -func (gur *GitUpstreamRepo) ResolveTag(tag string) (string, bool) { +func (gur *gitUpstreamRepoBroker) ResolveTag(tag string) (string, bool) { tag = strings.TrimPrefix(tag, "refs/tags/") - for t, commit := range gur.Tags { - if t == tag { - return commit, true - } - } - return "", false + commit, ok := gur.commitByTag[tag] + return commit, ok } // ResolveRef resolves the ref (either tag or branch) to a commit SHA. If the // ref doesn't exist in the upstream repo, the last return value will be false. -func (gur *GitUpstreamRepo) ResolveRef(ref string) (string, bool) { - commit, found := gur.ResolveBranch(ref) - if found { - return commit, true +func (gur *gitUpstreamRepoBroker) ResolveRef(ref string) string { + if commit, found := gur.ResolveBranch(ref); found { + return commit + } + if commit, found := gur.ResolveTag(ref); found { + return commit } - return gur.ResolveTag(ref) + + return ref } // getRepoDir returns the cache directory name for a remote repo // This takes the md5 hash of the repo uri and then base32 (or hex for Windows to shorten dir) // encodes it to make sure it doesn't contain characters that isn't legal in directory names. -func (gur *GitUpstreamRepo) getRepoDir(uri string) string { +func (gur *gitUpstreamRepoBroker) getRepoDir(uri string) string { if runtime.GOOS == "windows" { var hash = md5.Sum([]byte(uri)) return strings.ToLower(hex.EncodeToString(hash[:])) @@ -337,7 +412,7 @@ func (gur *GitUpstreamRepo) getRepoDir(uri string) string { } // getRepoCacheDir -func (gur *GitUpstreamRepo) getRepoCacheDir() (string, error) { +func (gur *gitUpstreamRepoBroker) getRepoCacheDir() (string, error) { const op errors.Op = "gitutil.getRepoCacheDir" var err error dir := os.Getenv(RepoCacheDirEnv) @@ -354,9 +429,15 @@ func (gur *GitUpstreamRepo) getRepoCacheDir() (string, error) { return filepath.Join(dir, ".kpt", "repos"), nil } -// cacheRepo fetches a remote repo to a cache location, and fetches the provided refs. -func (gur *GitUpstreamRepo) cacheRepo(ctx context.Context, uri string, requiredRefs []string, optionalRefs []string) (string, error) { +// cacheRepo fetches the remote repo, and fetches the provided refs. +func (gur *gitUpstreamRepoBroker) cacheRepo(ctx context.Context, requiredRefs []string) (string, error) { const op errors.Op = "gitutil.cacheRepo" + // preventing argument injection. + for _, ref := range requiredRefs { + if err := validateGitArg(GitArgRef, ref); err != nil { + return "", errors.E(op, errors.Repo(gur.uri), err) + } + } kptCacheDir, err := gur.getRepoCacheDir() if err != nil { return "", errors.E(op, err) @@ -369,21 +450,21 @@ func (gur *GitUpstreamRepo) cacheRepo(ctx context.Context, uri string, requiredR // create the repo directory if it doesn't exist yet gitRunner, err := NewLocalGitRunner(kptCacheDir) if err != nil { - return "", errors.E(op, errors.Repo(uri), err) + return "", errors.E(op, errors.Repo(gur.uri), err) } - uriSha := gur.getRepoDir(uri) + uriSha := gur.getRepoDir(gur.uri) repoCacheDir := filepath.Join(kptCacheDir, uriSha) if _, err := os.Stat(repoCacheDir); os.IsNotExist(err) { if _, err := gitRunner.Run(ctx, "init", uriSha); err != nil { AmendGitExecError(err, func(e *GitExecError) { - e.Repo = uri + e.Repo = gur.uri }) return "", errors.E(op, errors.Git, fmt.Errorf("error running `git init`: %w", err)) } gitRunner.Dir = repoCacheDir - if _, err = gitRunner.Run(ctx, "remote", "add", "origin", uri); err != nil { + if _, err = gitRunner.Run(ctx, "remote", "add", "origin", gur.uri); err != nil { AmendGitExecError(err, func(e *GitExecError) { - e.Repo = uri + e.Repo = gur.uri }) return "", errors.E(op, errors.Git, fmt.Errorf("error adding origin remote: %w", err)) } @@ -391,23 +472,29 @@ func (gur *GitUpstreamRepo) cacheRepo(ctx context.Context, uri string, requiredR gitRunner.Dir = repoCacheDir } -loop: for i := range requiredRefs { - s := requiredRefs[i] + requiredRef := requiredRefs[i] // Check if we can verify the ref. This will output a full commit sha if // either the ref (short commit, tag, branch) can be resolved to a full // commit sha, or if the provided ref is already a valid full commit sha (note // that this will happen even if the commit doesn't exist in the local repo). // We ignore the error here since an error just means the ref didn't exist, // which we detect by checking the output to stdout. - rr, _ := gitRunner.Run(ctx, "rev-parse", "--verify", "-q", s) + rr, _ := gitRunner.Run(ctx, "rev-parse", "--verify", "-q", requiredRef) // If the output is the same as the ref, then the ref was already a full // commit sha. - validFullSha := s == strings.TrimSpace(rr.Stdout) - _, resolved := gur.ResolveRef(s) - // check if ref was previously fetched - // we use the ref s as the cache key - _, fetched := gur.fetchedRefs[s] + validFullSha := requiredRef == strings.TrimSpace(rr.Stdout) + resolvedCommit := gur.ResolveRef(requiredRef) + resolved := resolvedCommit != requiredRef + // Use the resolved commit SHA as the cache key rather than the ref name. + // Branches and tags are mutable and may point at a different commit than + // they did on a previous fetch; keying on the immutable commit ensures a + // moved ref is fetched again instead of being skipped as already-fetched. + cacheKey := requiredRef + if resolved { + cacheKey = resolvedCommit + } + _, fetched := gur.fetchedRefs[cacheKey] switch { case fetched: // skip refetching if previously fetched @@ -415,51 +502,39 @@ loop: case resolved || validFullSha: // If the ref references a branch or a tag, or is a valid commit // sha and has not already been fetched, we can fetch just a single commit. - if _, err := gitRunner.RunVerbose(ctx, "fetch", "origin", "--depth=1", s); err != nil { + if _, err := gitRunner.RunVerbose(ctx, "fetch", "origin", "--depth=1", requiredRef); err != nil { AmendGitExecError(err, func(e *GitExecError) { - e.Repo = uri + e.Repo = gur.uri e.Command = "fetch" - e.Ref = s + e.Ref = requiredRef }) return "", errors.E(op, errors.Git, fmt.Errorf( - "error running `git fetch` for ref %q: %w", s, err)) + "error running `git fetch` for ref %q: %w", requiredRef, err)) } - gur.fetchedRefs[s] = true + gur.fetchedRefs[cacheKey] = true default: // In other situations (like a short commit sha), we have to do // a full fetch from the remote. if _, err := gitRunner.RunVerbose(ctx, "fetch", "origin"); err != nil { AmendGitExecError(err, func(e *GitExecError) { - e.Repo = uri + e.Repo = gur.uri e.Command = "fetch" }) return "", errors.E(op, errors.Git, fmt.Errorf( "error running `git fetch` for origin: %w", err)) } - if _, err = gitRunner.Run(ctx, "show", s); err != nil { + if _, err = gitRunner.Run(ctx, "show", requiredRef); err != nil { AmendGitExecError(err, func(e *GitExecError) { - e.Repo = uri - e.Ref = s + e.Repo = gur.uri + e.Ref = requiredRef }) return "", errors.E(op, errors.Git, fmt.Errorf( "error verifying results from fetch: %w", err)) } - gur.fetchedRefs[s] = true - // If we did a full fetch, we already have all refs, so we can just - // exit the loop. - break loop + gur.fetchedRefs[cacheKey] = true + return repoCacheDir, nil } } - var found bool - for _, s := range optionalRefs { - if _, err := gitRunner.Run(ctx, "fetch", "origin", s); err == nil { - found = true - } - } - if !found && len(optionalRefs) > 0 { - return "", errors.E(op, errors.Git, fmt.Errorf("unable to find any refs %s", - strings.Join(optionalRefs, ","))) - } return repoCacheDir, nil } diff --git a/internal/gitutil/gitutil_test.go b/internal/gitutil/gitutil_test.go index 0ae5ebb4fa..60c03c5d7b 100644 --- a/internal/gitutil/gitutil_test.go +++ b/internal/gitutil/gitutil_test.go @@ -115,8 +115,8 @@ func TestNewGitUpstreamRepo_noRefs(t *testing.T) { if !assert.NoError(t, err) { t.FailNow() } - assert.Equal(t, 0, len(gur.Heads)) - assert.Equal(t, 0, len(gur.Tags)) + assert.Equal(t, 0, len(gur.Heads())) + assert.Equal(t, 0, len(gur.Tags())) } func TestNewGitUpstreamRepo(t *testing.T) { @@ -158,6 +158,8 @@ func TestNewGitUpstreamRepo(t *testing.T) { } for tn, tc := range testCases { + sort.Strings(tc.expectedHeads) + sort.Strings(tc.expectedTags) t.Run(tn, func(t *testing.T) { repoContent := map[string][]testutil.Content{ testutil.Upstream: tc.repoContent, @@ -172,8 +174,8 @@ func TestNewGitUpstreamRepo(t *testing.T) { if !assert.NoError(t, err) { t.FailNow() } - assert.EqualValues(t, tc.expectedHeads, toKeys(gur.Heads)) - assert.EqualValues(t, tc.expectedTags, toKeys(gur.Tags)) + assert.EqualValues(t, tc.expectedHeads, gur.Heads()) + assert.EqualValues(t, tc.expectedTags, gur.Tags()) }) } } @@ -359,11 +361,7 @@ func TestGitUpstreamRepo_GetRepo(t *testing.T) { t.FailNow() } for _, r := range refs { - sha, found := gur.ResolveRef(r) - if !found { - // Assume the ref is a commit... - sha = r - } + sha := gur.ResolveRef(r) _, err := runner.Run(fake.CtxWithDefaultPrinter(), "reset", "--hard", sha) assert.NoError(t, err) } @@ -392,8 +390,13 @@ func TestGitUpstreamRepo_GetRepo_multipleUpdates(t *testing.T) { g, _, clean := testutil.SetupReposAndWorkspace(t, repoContent) defer clean() - firstRepoDir := getRepoAndVerify(t, g[testutil.Upstream].RepoDirectory, branchName) - _, err := os.Stat(filepath.Join(firstRepoDir, "deployment.yaml")) + gur, err := internalgitutil.NewGitUpstreamRepo(fake.CtxWithDefaultPrinter(), g[testutil.Upstream].RepoDirectory) + if !assert.NoError(t, err) { + t.FailNow() + } + + firstRepoDir := getRepoAndVerify(t, gur, branchName) + _, err = os.Stat(filepath.Join(firstRepoDir, "deployment.yaml")) if !assert.NoError(t, err) { t.FailNow() } @@ -402,7 +405,7 @@ func TestGitUpstreamRepo_GetRepo_multipleUpdates(t *testing.T) { t.FailNow() } - secondRepoDir := getRepoAndVerify(t, g[testutil.Upstream].RepoDirectory, branchName) + secondRepoDir := getRepoAndVerify(t, gur, branchName) _, err = os.Stat(filepath.Join(secondRepoDir, "configmap.yaml")) if !assert.NoError(t, err) { t.FailNow() @@ -411,11 +414,7 @@ func TestGitUpstreamRepo_GetRepo_multipleUpdates(t *testing.T) { assert.Equal(t, firstRepoDir, secondRepoDir) } -func getRepoAndVerify(t *testing.T, repo, branchName string) string { - gur, err := internalgitutil.NewGitUpstreamRepo(fake.CtxWithDefaultPrinter(), repo) - if !assert.NoError(t, err) { - t.FailNow() - } +func getRepoAndVerify(t *testing.T, gur internalgitutil.GitUpstreamRepo, branchName string) string { dir, err := gur.GetRepo(fake.CtxWithDefaultPrinter(), []string{branchName}) if !assert.NoError(t, err) { t.FailNow() @@ -433,11 +432,238 @@ func getRepoAndVerify(t *testing.T, repo, branchName string) string { return dir } -func toKeys(m map[string]string) []string { - keys := make([]string, 0) - for k := range m { - keys = append(keys, k) +// TestGitUpstreamRepo_GetRepo_rejectsOptionInjection makes sure that a ref that +// git would interpret as a command-line option (e.g. one read from an +// attacker-controlled remote sub-package Kptfile) is rejected rather than +// passed to git, preventing argument injection. +func TestGitUpstreamRepo_GetRepo_rejectsOptionInjection(t *testing.T) { + repoContent := map[string][]testutil.Content{ + testutil.Upstream: { + { + Pkg: pkgbuilder.NewRootPkg(). + WithResource(pkgbuilder.DeploymentResource), + Branch: "main", + }, + }, + } + g, _, clean := testutil.SetupReposAndWorkspace(t, repoContent) + defer clean() + + gur, err := internalgitutil.NewGitUpstreamRepo(fake.CtxWithDefaultPrinter(), g[testutil.Upstream].RepoDirectory) + if !assert.NoError(t, err) { + t.FailNow() + } + + _, err = gur.GetRepo(fake.CtxWithDefaultPrinter(), []string{"--output=../../../.bashrc"}) + if !assert.Error(t, err) { + t.FailNow() + } + assert.Contains(t, err.Error(), "must not begin with '-'") +} + +// TestNewGitUpstreamRepo_rejectsOptionInjectionURI makes sure that a repo URI +// that git would interpret as a command-line option (e.g. one read from an +// attacker-controlled Kptfile upstream block) is rejected before it is ever +// handed to git. Without this guard a value like `--upload-pack=` would be +// treated as an option to `git fetch`/`git ls-remote` and could execute an +// arbitrary command. +func TestNewGitUpstreamRepo_rejectsOptionInjectionURI(t *testing.T) { + testCases := map[string]string{ + "upload-pack argument injection": "--upload-pack=touch /tmp/kpt-pwned", + "short option": "-o", + "long option with value": "--output=/tmp/kpt-pwned", + } + + for tn, uri := range testCases { + t.Run(tn, func(t *testing.T) { + _, err := internalgitutil.NewGitUpstreamRepo(fake.CtxWithDefaultPrinter(), uri) + if !assert.Error(t, err) { + t.FailNow() + } + assert.Contains(t, err.Error(), "must not begin with '-'") + }) + } +} + +// TestGitUpstreamRepo_GetRepo_rejectsOptionInjectionVariants exercises a range +// of option-injection payloads (including one hidden after a legitimate-looking +// ref) to ensure every ref is validated before any of them reaches git. +func TestGitUpstreamRepo_GetRepo_rejectsOptionInjectionVariants(t *testing.T) { + testCases := map[string]struct { + refs []string + }{ + "upload-pack argument injection": { + refs: []string{"--upload-pack=touch /tmp/kpt-pwned"}, + }, + "short option injection": { + refs: []string{"-o"}, + }, + "output file write via show": { + refs: []string{"--output=/tmp/kpt-injection"}, + }, + "malicious ref after a valid one": { + refs: []string{"main", "--upload-pack=evil"}, + }, + } + + repoContent := map[string][]testutil.Content{ + testutil.Upstream: { + { + Pkg: pkgbuilder.NewRootPkg(). + WithResource(pkgbuilder.DeploymentResource), + Branch: "main", + }, + }, + } + + for tn, tc := range testCases { + t.Run(tn, func(t *testing.T) { + g, _, clean := testutil.SetupReposAndWorkspace(t, repoContent) + defer clean() + + gur, err := internalgitutil.NewGitUpstreamRepo(fake.CtxWithDefaultPrinter(), g[testutil.Upstream].RepoDirectory) + if !assert.NoError(t, err) { + t.FailNow() + } + + _, err = gur.GetRepo(fake.CtxWithDefaultPrinter(), tc.refs) + if !assert.Error(t, err) { + t.FailNow() + } + assert.Contains(t, err.Error(), "must not begin with '-'") + }) + } +} + +// TestGitUpstreamRepo_GetRepo_optionInjectionHasNoSideEffect verifies that a +// rejected `--output=` payload never causes git to write the file, proving +// the option was never passed to git rather than merely surfacing an error. +func TestGitUpstreamRepo_GetRepo_optionInjectionHasNoSideEffect(t *testing.T) { + repoContent := map[string][]testutil.Content{ + testutil.Upstream: { + { + Pkg: pkgbuilder.NewRootPkg(). + WithResource(pkgbuilder.DeploymentResource), + Branch: "main", + }, + }, + } + g, _, clean := testutil.SetupReposAndWorkspace(t, repoContent) + defer clean() + + gur, err := internalgitutil.NewGitUpstreamRepo(fake.CtxWithDefaultPrinter(), g[testutil.Upstream].RepoDirectory) + if !assert.NoError(t, err) { + t.FailNow() + } + + victim := filepath.Join(t.TempDir(), "victim.txt") + _, err = gur.GetRepo(fake.CtxWithDefaultPrinter(), []string{"--output=" + victim}) + if !assert.Error(t, err) { + t.FailNow() + } + assert.Contains(t, err.Error(), "must not begin with '-'") + + _, statErr := os.Stat(victim) + assert.True(t, os.IsNotExist(statErr), + "injected --output must not have caused git to create a file") +} + +// TestGitUpstreamRepo_GetRepo_staleBranchRefetch verifies that when a branch is +// moved to a new commit upstream after the broker was created, a subsequent +// GetRepo fetches the new commit rather than serving the stale commit that the +// branch pointed to at construction time. It also asserts that fetchedRefs is +// keyed on the resolved commit SHA, so both the old and new commits are tracked. +func TestGitUpstreamRepo_GetRepo_staleBranchRefetch(t *testing.T) { + branchName := "kpt-test-stale" + repoContent := map[string][]testutil.Content{ + testutil.Upstream: { + { + Pkg: pkgbuilder.NewRootPkg(). + WithResource(pkgbuilder.DeploymentResource), + Branch: branchName, + }, + { + Pkg: pkgbuilder.NewRootPkg(). + WithResource(pkgbuilder.ConfigMapResource), + Branch: branchName, + }, + }, + } + g, _, clean := testutil.SetupReposAndWorkspace(t, repoContent) + defer clean() + + gur, err := internalgitutil.NewGitUpstreamRepo(fake.CtxWithDefaultPrinter(), g[testutil.Upstream].RepoDirectory) + if !assert.NoError(t, err) { + t.FailNow() } - sort.Strings(keys) - return keys + + firstRepoDir := getRepoAndVerify(t, gur, branchName) + firstSha, ok := gur.ResolveBranch(branchName) + if !assert.True(t, ok) { + t.FailNow() + } + // The first commit's content should be present, the second's should not. + _, err = os.Stat(filepath.Join(firstRepoDir, "deployment.yaml")) + assert.NoError(t, err) + // fetchedRefs is keyed by the resolved commit SHA, not the branch name. + assert.Equal(t, []string{firstSha}, gur.GetFetchedRefs()) + + // Move the branch to a new commit upstream. + if !assert.NoError(t, testutil.UpdateRepos(t, g, repoContent)) { + t.FailNow() + } + + secondRepoDir := getRepoAndVerify(t, gur, branchName) + secondSha, ok := gur.ResolveBranch(branchName) + if !assert.True(t, ok) { + t.FailNow() + } + // The branch must now resolve to a different commit than before. + assert.NotEqual(t, firstSha, secondSha) + // The cache dir is reused, but it must now contain the new commit's content. + assert.Equal(t, firstRepoDir, secondRepoDir) + _, err = os.Stat(filepath.Join(secondRepoDir, "configmap.yaml")) + assert.NoError(t, err) + + // Both the stale and current commits should be recorded as fetched, proving + // the moved branch was fetched again instead of skipped as already-fetched. + fetched := gur.GetFetchedRefs() + assert.Len(t, fetched, 2) + assert.Contains(t, fetched, firstSha) + assert.Contains(t, fetched, secondSha) +} + +// TestGitUpstreamRepo_GetRepo_idempotentFetch verifies that fetching the same +// unchanged branch twice does not re-fetch it or add a duplicate entry to +// fetchedRefs, i.e. the commit-keyed cache is honored when nothing has moved. +func TestGitUpstreamRepo_GetRepo_idempotentFetch(t *testing.T) { + branchName := "kpt-test-idempotent" + repoContent := map[string][]testutil.Content{ + testutil.Upstream: { + { + Pkg: pkgbuilder.NewRootPkg(). + WithResource(pkgbuilder.DeploymentResource), + Branch: branchName, + }, + }, + } + g, _, clean := testutil.SetupReposAndWorkspace(t, repoContent) + defer clean() + + gur, err := internalgitutil.NewGitUpstreamRepo(fake.CtxWithDefaultPrinter(), g[testutil.Upstream].RepoDirectory) + if !assert.NoError(t, err) { + t.FailNow() + } + + getRepoAndVerify(t, gur, branchName) + firstFetched := gur.GetFetchedRefs() + if !assert.Len(t, firstFetched, 1) { + t.FailNow() + } + + // A second GetRepo of the same, unchanged branch must not add a new entry. + getRepoAndVerify(t, gur, branchName) + secondFetched := gur.GetFetchedRefs() + assert.Equal(t, firstFetched, secondFetched) + assert.Len(t, secondFetched, 1) } diff --git a/pkg/lib/update/update.go b/pkg/lib/update/update.go index 138f56b1f6..291955833b 100644 --- a/pkg/lib/update/update.go +++ b/pkg/lib/update/update.go @@ -100,7 +100,7 @@ type Command struct { Strategy kptfilev1.UpdateStrategyType // cachedUpstreamRepos is an upstream repo already fetched for a given repoSpec CloneRef - cachedUpstreamRepos map[string]*internalgitutil.GitUpstreamRepo + cachedUpstreamRepos map[string]internalgitutil.GitUpstreamRepo } func GetUpdater(strategy string) updatetypes.Updater { @@ -146,7 +146,7 @@ func (u *Command) Run(ctx context.Context) error { return errors.E(op, u.Pkg.UniquePath, err) } if u.cachedUpstreamRepos == nil { - u.cachedUpstreamRepos = make(map[string]*internalgitutil.GitUpstreamRepo) + u.cachedUpstreamRepos = make(map[string]internalgitutil.GitUpstreamRepo) } packageCount := 0 @@ -197,7 +197,7 @@ func (u *Command) Run(ctx context.Context) error { } // GetCachedUpstreamRepos returns repos cached during update -func (u Command) GetCachedUpstreamRepos() map[string]*internalgitutil.GitUpstreamRepo { +func (u Command) GetCachedUpstreamRepos() map[string]internalgitutil.GitUpstreamRepo { return u.cachedUpstreamRepos } diff --git a/pkg/lib/update/update_test.go b/pkg/lib/update/update_test.go index 67ff220c90..3642671e35 100644 --- a/pkg/lib/update/update_test.go +++ b/pkg/lib/update/update_test.go @@ -87,10 +87,10 @@ func TestCommand_Run_noRefChanges(t *testing.T) { cachedGURs := cmd.GetCachedUpstreamRepos() assert.Equal(t, 1, len(cachedGURs)) for _, gur := range cachedGURs { - assert.Equal(t, upstreamRepo.RepoDirectory, gur.URI) + assert.Equal(t, upstreamRepo.RepoDirectory, gur.Uri()) // Expect two fetched refs // 1 for upstream ref (master) and 1 for previous upstream lock - assert.ElementsMatch(t, []string{masterBranch, upstreamRepo.Commits[0]}, gur.GetFetchedRefs()) + assert.ElementsMatch(t, upstreamRepo.Commits, gur.GetFetchedRefs()) } // Expect the local package to have Dataset2 @@ -3774,15 +3774,14 @@ func TestMultiUpdateCache(t *testing.T) { // Assert cached refs for each upstream repo upstreamRepo := repos[testutil.Upstream].RepoDirectory blueprintsRepo := repos[blueprintsUpstream].RepoDirectory - blueprintsRepoCommit := repos[blueprintsUpstream].Commits for cr, gur := range cachedGURs { switch cr { case upstreamRepo: // Expect only upstream ref master to be cached as there was no lock - assert.ElementsMatch(t, []string{masterBranch}, gur.GetFetchedRefs()) + assert.ElementsMatch(t, repos[testutil.Upstream].Commits, gur.GetFetchedRefs()) case blueprintsRepo: // Expect both upstream ref master and upstream lock commit to be cached - assert.ElementsMatch(t, []string{masterBranch, blueprintsRepoCommit[0]}, gur.GetFetchedRefs()) + assert.ElementsMatch(t, repos[blueprintsUpstream].Commits, gur.GetFetchedRefs()) default: t.Fatalf("Unexpected upstream repo %s", cr) } diff --git a/pkg/lib/util/fetch/fetch.go b/pkg/lib/util/fetch/fetch.go index 6785718c97..ba9456b8b3 100644 --- a/pkg/lib/util/fetch/fetch.go +++ b/pkg/lib/util/fetch/fetch.go @@ -99,12 +99,12 @@ type Cloner struct { repoSpec *gitutil.RepoSpec // cachedRepos - cachedRepo map[string]*internalgitutil.GitUpstreamRepo + cachedRepo map[string]internalgitutil.GitUpstreamRepo } type NewClonerOption func(*Cloner) -func WithCachedRepo(r map[string]*internalgitutil.GitUpstreamRepo) NewClonerOption { +func WithCachedRepo(r map[string]internalgitutil.GitUpstreamRepo) NewClonerOption { return func(c *Cloner) { c.cachedRepo = r } @@ -118,7 +118,7 @@ func NewCloner(r *gitutil.RepoSpec, opts ...NewClonerOption) *Cloner { opt(c) } if c.cachedRepo == nil { - c.cachedRepo = make(map[string]*internalgitutil.GitUpstreamRepo) + c.cachedRepo = make(map[string]internalgitutil.GitUpstreamRepo) } return c } @@ -202,10 +202,7 @@ func (c *Cloner) ClonerUsingGitExec(ctx context.Context) error { // Find the commit SHA for the ref that was just fetched. We need the SHA // rather than the ref to be able to do a hard reset of the cache repo. - commit, found := upstreamRepo.ResolveRef(c.repoSpec.Ref) - if !found { - commit = c.repoSpec.Ref - } + commit := upstreamRepo.ResolveRef(c.repoSpec.Ref) // Reset the local repo to the commit we need. Doing a hard reset instead of // a checkout means we don't create any local branches so we don't need to diff --git a/pkg/lib/util/parse/parse.go b/pkg/lib/util/parse/parse.go index c1b5efc8e3..7ef16eeecd 100644 --- a/pkg/lib/util/parse/parse.go +++ b/pkg/lib/util/parse/parse.go @@ -200,11 +200,7 @@ func getRepoBranches(ctx context.Context, repo string) ([]string, error) { if err != nil { return nil, err } - branches := make([]string, 0, len(gur.Heads)) - for head := range gur.Heads { - branches = append(branches, head) - } - return branches, nil + return gur.Heads(), nil } // isAmbiguousBranch checks if a given branch name is similar to other branch names.