Address review: security, proxy, tar, zip, subdir, SSH lock

Author: thardeck

internal/bundlereader/gitclone.go

@@ -19,6 +19,7 @@ import (
 	gitclient "github.com/go-git/go-git/v5/plumbing/transport/client"
 	httpgit "github.com/go-git/go-git/v5/plumbing/transport/http"
 	fleetssh "github.com/rancher/fleet/internal/ssh"
+	fleetgit "github.com/rancher/fleet/pkg/git"
 )
 
 // httpsProtocolMu coordinates access to go-git's process-global HTTPS
@@ -84,6 +85,7 @@ func gitDownload(ctx context.Context, dst, rawURL string, auth Auth) error {
 	cloneOpts := &gogit.CloneOptions{
 		URL:             cloneURL.String(),
 		InsecureSkipTLS: auth.InsecureSkipVerify,
+		ProxyOptions:    fleetgit.ProxyOptsFromEnvironment(cloneURL.String()),
 	}
 	if err := setGitAuth(cloneOpts, &cloneURL, sshKeyPEM, auth); err != nil {
 		return err
@@ -126,7 +128,7 @@ func gitDownload(ctx context.Context, dst, rawURL string, auth Auth) error {
 	}
 	r, err := gogit.PlainCloneContext(ctx, dst, false, cloneOpts)
 	if err != nil {
-		return fmt.Errorf("cloning %s: %w", cloneURL.String(), err)
+		return fmt.Errorf("cloning %s: %w", cloneURL.Redacted(), err)
 	}
 	h, err := r.ResolveRevision(plumbing.Revision(ref))
 	if err != nil {

internal/bundlereader/httpdownload.go

@@ -3,7 +3,6 @@ package bundlereader
 import (
 	"archive/tar"
 	"archive/zip"
-	"bytes"
 	"compress/gzip"
 	"context"
 	"errors"
@@ -141,6 +140,8 @@ func extractTar(dst string, r io.Reader) error {
 			if err := os.Symlink(hdr.Linkname, target); err != nil && !os.IsExist(err) {
 				return err
 			}
+		default:
+			return fmt.Errorf("unsupported tar entry type %d for %q", hdr.Typeflag, hdr.Name)
 		}
 	}
 	return nil
@@ -169,14 +170,33 @@ func extractGz(dst, name string, r io.Reader) error {
 	return err
 }
 
-// extractZipFromReader downloads the zip body to memory and extracts it.
-// archive/zip requires a ReaderAt, so we buffer the response first.
+// extractZipFromReader streams the zip body to a temporary file and extracts it.
+// archive/zip requires a ReaderAt, so we use an *os.File as the backing store
+// to avoid buffering the entire archive in memory.
 func extractZipFromReader(dst string, r io.Reader) error {
-	data, err := io.ReadAll(r)
+	f, err := os.CreateTemp("", "bundle-*.zip")
 	if err != nil {
-		return fmt.Errorf("reading zip body: %w", err)
+		return fmt.Errorf("creating temp file for zip: %w", err)
 	}
-	zr, err := zip.NewReader(bytes.NewReader(data), int64(len(data)))
+	defer func() {
+		f.Close()
+		os.Remove(f.Name())
+	}()
+
+	if _, err := io.Copy(f, r); err != nil {
+		return fmt.Errorf("writing zip body to temp file: %w", err)
+	}
+
+	info, err := f.Stat()
+	if err != nil {
+		return fmt.Errorf("stat temp zip file: %w", err)
+	}
+
+	if _, err := f.Seek(0, io.SeekStart); err != nil {
+		return fmt.Errorf("seeking temp zip file: %w", err)
+	}
+
+	zr, err := zip.NewReader(f, info.Size())
 	if err != nil {
 		return fmt.Errorf("opening zip: %w", err)
 	}

internal/bundlereader/loaddirectory.go

@@ -371,6 +371,27 @@ func downloadOCIChart(name, version, path string, auth Auth) (string, error) {
 	return saved, nil
 }
 
+// safeJoinSubDir joins base and sub, returning an error if sub is absolute or
+// escapes base (e.g., due to ".." components).
+func safeJoinSubDir(base, sub string) (string, error) {
+	cleanSub := filepath.Clean(filepath.FromSlash(sub))
+	if filepath.IsAbs(cleanSub) {
+		return "", fmt.Errorf("subdir must be relative, got %q", sub)
+	}
+	if cleanSub == "." {
+		return base, nil
+	}
+	joined := filepath.Join(base, cleanSub)
+	rel, err := filepath.Rel(base, joined)
+	if err != nil {
+		return "", err
+	}
+	if rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) {
+		return "", fmt.Errorf("subdir %q escapes base directory", sub)
+	}
+	return joined, nil
+}
+
 // fetchToDir resolves source (relative to pwd), downloads or copies it, and
 // places the resulting files into dst.
 func fetchToDir(ctx context.Context, dst, source, pwd string, auth Auth) error {
@@ -392,7 +413,10 @@ func fetchToDir(ctx context.Context, dst, source, pwd string, auth Auth) error {
 			return err
 		}
 
-		src := filepath.Join(td, filepath.FromSlash(si.subDir))
+		src, err := safeJoinSubDir(td, si.subDir)
+		if err != nil {
+			return fmt.Errorf("invalid subdir %q: %w", si.subDir, err)
+		}
 		if err := os.MkdirAll(dst, 0750); err != nil {
 			return err
 		}

internal/cmd/cli/gitcloner/cloner.go

@@ -43,9 +43,13 @@ var caCloneMutex sync.RWMutex
 
 // withExclusiveCA installs a custom HTTPS transport that trusts only caBundle
 // (not the system cert pool) for the duration of fn, then restores the default.
-// When caBundle is empty, fn is called under a shared lock so that default
-// HTTPS clones cannot run while a custom-CA clone holds the write lock.
-func withExclusiveCA(caBundle []byte, insecureSkipTLS bool, fn func() error) error {
+// The mutex is only taken for HTTPS URLs because only go-git's HTTPS protocol
+// registry is affected; SSH and other scheme clones can proceed concurrently.
+func withExclusiveCA(repoURL string, caBundle []byte, insecureSkipTLS bool, fn func() error) error {
+	if !strings.HasPrefix(repoURL, "https://") {
+		// Non-HTTPS clone: no protocol registry mutation, no lock needed.
+		return fn()
+	}
 	if len(caBundle) == 0 {
 		caCloneMutex.RLock()
 		defer caCloneMutex.RUnlock()
@@ -121,7 +125,7 @@ func (c *Cloner) CloneRepo(opts *GitCloner) error {
 }
 
 func cloneBranch(opts *GitCloner, auth transport.AuthMethod, caBundle []byte) error {
-	return withExclusiveCA(caBundle, opts.InsecureSkipTLS, func() error {
+	return withExclusiveCA(opts.Repo, caBundle, opts.InsecureSkipTLS, func() error {
 		r, err := plainClone(opts.Path, false, &git.CloneOptions{
 			URL:               opts.Repo,
 			Depth:             1,
@@ -147,7 +151,7 @@ func cloneBranch(opts *GitCloner, auth transport.AuthMethod, caBundle []byte) er
 }
 
 func cloneRevision(opts *GitCloner, auth transport.AuthMethod, caBundle []byte) error {
-	return withExclusiveCA(caBundle, opts.InsecureSkipTLS, func() error {
+	return withExclusiveCA(opts.Repo, caBundle, opts.InsecureSkipTLS, func() error {
 		r, err := plainClone(opts.Path, false, &git.CloneOptions{
 			URL:               opts.Repo,
 			Depth:             1,