Cherry-pick bug fixes from reconciler refactor branch

controllers/storage.go

@@ -29,6 +29,7 @@ import (
 	"strings"
 	"time"
 
+	securejoin "github.com/cyphar/filepath-securejoin"
 	"github.com/go-git/go-git/v5/plumbing/format/gitignore"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
@@ -83,8 +84,7 @@ func (s Storage) SetArtifactURL(artifact *sourcev1.Artifact) {
 	artifact.URL = fmt.Sprintf("http://%s/%s", s.Hostname, artifact.Path)
 }
 
-// SetHostname sets the hostname of the given URL string to the current Storage.Hostname
-// and returns the result.
+// SetHostname sets the hostname of the given URL string to the current Storage.Hostname and returns the result.
 func (s Storage) SetHostname(URL string) string {
 	u, err := url.Parse(URL)
 	if err != nil {
@@ -106,8 +106,7 @@ func (s *Storage) RemoveAll(artifact sourcev1.Artifact) error {
 	return os.RemoveAll(dir)
 }
 
-// RemoveAllButCurrent removes all files for the given v1beta1.Artifact base dir,
-// excluding the current one.
+// RemoveAllButCurrent removes all files for the given v1beta1.Artifact base dir, excluding the current one.
 func (s *Storage) RemoveAllButCurrent(artifact sourcev1.Artifact) error {
 	localPath := s.LocalPath(artifact)
 	dir := filepath.Dir(localPath)
@@ -132,8 +131,7 @@ func (s *Storage) RemoveAllButCurrent(artifact sourcev1.Artifact) error {
 	return nil
 }
 
-// ArtifactExist returns a boolean indicating whether the v1beta1.Artifact exists in storage
-// and is a regular file.
+// ArtifactExist returns a boolean indicating whether the v1beta1.Artifact exists in storage and is a regular file.
 func (s *Storage) ArtifactExist(artifact sourcev1.Artifact) bool {
 	fi, err := os.Lstat(s.LocalPath(artifact))
 	if err != nil {
@@ -142,14 +140,13 @@ func (s *Storage) ArtifactExist(artifact sourcev1.Artifact) bool {
 	return fi.Mode().IsRegular()
 }
 
-// ArchiveFileFilter must return true if a file should not be included
-// in the archive after inspecting the given path and/or os.FileInfo.
+// ArchiveFileFilter must return true if a file should not be included in the archive after inspecting the given path
+// and/or os.FileInfo.
 type ArchiveFileFilter func(p string, fi os.FileInfo) bool
 
-// SourceIgnoreFilter returns an ArchiveFileFilter that filters out
-// files matching sourceignore.VCSPatterns and any of the provided
-// patterns. If an empty gitignore.Pattern slice is given, the matcher
-// is set to sourceignore.NewDefaultMatcher.
+// SourceIgnoreFilter returns an ArchiveFileFilter that filters out files matching sourceignore.VCSPatterns and any of
+// the provided patterns.
+// If an empty gitignore.Pattern slice is given, the matcher is set to sourceignore.NewDefaultMatcher.
 func SourceIgnoreFilter(ps []gitignore.Pattern, domain []string) ArchiveFileFilter {
 	matcher := sourceignore.NewDefaultMatcher(ps, domain)
 	if len(ps) > 0 {
@@ -163,10 +160,10 @@ func SourceIgnoreFilter(ps []gitignore.Pattern, domain []string) ArchiveFileFilt
 	}
 }
 
-// Archive atomically archives the given directory as a tarball to the
-// given v1beta1.Artifact path, excluding directories and any
-// ArchiveFileFilter matches. If successful, it sets the checksum and
-// last update time on the artifact.
+// Archive atomically archives the given directory as a tarball to the given v1beta1.Artifact path, excluding
+// directories and any ArchiveFileFilter matches. While archiving, any environment specific data (for example,
+// the user and group name) is stripped from file headers.
+// If successful, it sets the checksum and last update time on the artifact.
 func (s *Storage) Archive(artifact *sourcev1.Artifact, dir string, filter ArchiveFileFilter) (err error) {
 	if f, err := os.Stat(dir); os.IsNotExist(err) || !f.IsDir() {
 		return fmt.Errorf("invalid dir path: %s", dir)
@@ -220,6 +217,16 @@ func (s *Storage) Archive(artifact *sourcev1.Artifact, dir string, filter Archiv
 		}
 		header.Name = relFilePath
 
+		// We want to remove any environment specific data as well, this
+		// ensures the checksum is purely content based.
+		header.Gid = 0
+		header.Uid = 0
+		header.Uname = ""
+		header.Gname = ""
+		header.ModTime = time.Time{}
+		header.AccessTime = time.Time{}
+		header.ChangeTime = time.Time{}
+
 		if err := tw.WriteHeader(header); err != nil {
 			return err
 		}
@@ -341,9 +348,8 @@ func (s *Storage) Copy(artifact *sourcev1.Artifact, reader io.Reader) (err error
 	return nil
 }
 
-// CopyFromPath atomically copies the contents of the given path to the path of
-// the v1beta1.Artifact. If successful, the checksum and last update time on the
-// artifact is set.
+// CopyFromPath atomically copies the contents of the given path to the path of the v1beta1.Artifact.
+// If successful, the checksum and last update time on the artifact is set.
 func (s *Storage) CopyFromPath(artifact *sourcev1.Artifact, path string) (err error) {
 	f, err := os.Open(path)
 	if err != nil {
@@ -353,10 +359,10 @@ func (s *Storage) CopyFromPath(artifact *sourcev1.Artifact, path string) (err er
 	return s.Copy(artifact, f)
 }
 
-// CopyToPath copies the contents of the given artifact to the path.
+// CopyToPath copies the contents in the (sub)path of the given artifact to the given path.
 func (s *Storage) CopyToPath(artifact *sourcev1.Artifact, subPath, toPath string) error {
 	// create a tmp directory to store artifact
-	tmp, err := os.MkdirTemp("", "flux-include")
+	tmp, err := os.MkdirTemp("", "flux-include-")
 	if err != nil {
 		return err
 	}
@@ -371,7 +377,7 @@ func (s *Storage) CopyToPath(artifact *sourcev1.Artifact, subPath, toPath string
 	defer f.Close()
 
 	// untar the artifact
-	untarPath := filepath.Join(tmp, "tar")
+	untarPath := filepath.Join(tmp, "unpack")
 	if _, err = untar.Untar(f, untarPath); err != nil {
 		return err
 	}
@@ -382,15 +388,17 @@ func (s *Storage) CopyToPath(artifact *sourcev1.Artifact, subPath, toPath string
 	}
 
 	// copy the artifact content to the destination dir
-	fromPath := filepath.Join(untarPath, subPath)
+	fromPath, err := securejoin.SecureJoin(untarPath, subPath)
+	if err != nil {
+		return err
+	}
 	if err := fs.RenameWithFallback(fromPath, toPath); err != nil {
 		return err
 	}
 	return nil
 }
 
-// Symlink creates or updates a symbolic link for the given v1beta1.Artifact
-// and returns the URL for the symlink.
+// Symlink creates or updates a symbolic link for the given v1beta1.Artifact and returns the URL for the symlink.
 func (s *Storage) Symlink(artifact sourcev1.Artifact, linkName string) (string, error) {
 	localPath := s.LocalPath(artifact)
 	dir := filepath.Dir(localPath)
@@ -427,13 +435,16 @@ func (s *Storage) Lock(artifact sourcev1.Artifact) (unlock func(), err error) {
 	return mutex.Lock()
 }
 
-// LocalPath returns the local path of the given artifact (that is: relative to
-// the Storage.BasePath).
+// LocalPath returns the secure local path of the given artifact (that is: relative to the Storage.BasePath).
 func (s *Storage) LocalPath(artifact sourcev1.Artifact) string {
 	if artifact.Path == "" {
 		return ""
 	}
-	return filepath.Join(s.BasePath, artifact.Path)
+	path, err := securejoin.SecureJoin(s.BasePath, artifact.Path)
+	if err != nil {
+		return ""
+	}
+	return path
 }
 
 // newHash returns a new SHA1 hash.

go.mod

@@ -6,7 +6,6 @@ replace github.com/fluxcd/source-controller/api => ./api
 
 require (
 	github.com/Masterminds/semver/v3 v3.1.1
-	github.com/blang/semver/v4 v4.0.0
 	github.com/cyphar/filepath-securejoin v0.2.2
 	github.com/fluxcd/pkg/apis/meta v0.10.0
 	github.com/fluxcd/pkg/gittestserver v0.3.0

go.sum

@@ -113,10 +113,7 @@ github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6r
 github.com/bgentry/speakeasy v0.1.0/go.mod h1:+zsyZBPWlz7T6j88CTgSN5bM796AkVf0kBD4zp0CCIs=
 github.com/bitly/go-simplejson v0.5.0/go.mod h1:cXHtHw4XUPsvGaxgjIAn8PhEWG9NfngEKAMDJEczWVA=
 github.com/bketelsen/crypt v0.0.3-0.20200106085610-5cbc8cc4026c/go.mod h1:MKsuJmJgSg28kpZDP6UIiPt0e0Oz0kqKNGyRaWEPv84=
-github.com/blang/semver v3.5.1+incompatible h1:cQNTCjp13qL8KC3Nbxr/y2Bqb63oX6wdnnjpJbkM4JQ=
 github.com/blang/semver v3.5.1+incompatible/go.mod h1:kRBLl5iJ+tD4TcOOxsy/0fnwebNt5EWlYSAyrTnjyyk=
-github.com/blang/semver/v4 v4.0.0 h1:1PFHFE6yCCTv8C1TeyNNarDzntLi7wMI5i/pzqYIsAM=
-github.com/blang/semver/v4 v4.0.0/go.mod h1:IbckMUScFkM3pff0VJDNKRiT6TG/YpiHIM2yvyW5YoQ=
 github.com/bmizerany/assert v0.0.0-20160611221934-b7ed37b82869/go.mod h1:Ekp36dRnpXw/yCqJaO+ZrUyxD+3VXMFFr56k5XYrpB4=
 github.com/bshuster-repo/logrus-logstash-hook v0.4.1 h1:pgAtgj+A31JBVtEHu2uHuEx0n+2ukqUJnS2vVe5pQNA=
 github.com/bshuster-repo/logrus-logstash-hook v0.4.1/go.mod h1:zsTqEiSzDgAa/8GZR7E1qaXrhYNDKBYy5/dWPTIflbk=

pkg/git/gogit/checkout.go

@@ -193,7 +193,7 @@ func (c *CheckoutSemVer) Checkout(ctx context.Context, path, url string, auth *g
 
 	tags := make(map[string]string)
 	tagTimestamps := make(map[string]time.Time)
-	_ = repoTags.ForEach(func(t *plumbing.Reference) error {
+	if err = repoTags.ForEach(func(t *plumbing.Reference) error {
 		revision := plumbing.Revision(t.Name().String())
 		hash, err := repo.ResolveRevision(revision)
 		if err != nil {
@@ -207,7 +207,9 @@ func (c *CheckoutSemVer) Checkout(ctx context.Context, path, url string, auth *g
 
 		tags[t.Name().Short()] = t.Strings()[1]
 		return nil
-	})
+	}); err != nil {
+		return nil, "", err
+	}
 
 	var matchedVersions semver.Collection
 	for tag, _ := range tags {

pkg/git/libgit2/checkout.go

@@ -19,8 +19,11 @@ package libgit2
 import (
 	"context"
 	"fmt"
+	"sort"
+	"time"
 
-	"github.com/blang/semver/v4"
+	"github.com/Masterminds/semver/v3"
+	"github.com/fluxcd/pkg/version"
 	git2go "github.com/libgit2/git2go/v31"
 
 	"github.com/fluxcd/pkg/gitutil"
@@ -168,7 +171,7 @@ type CheckoutSemVer struct {
 }
 
 func (c *CheckoutSemVer) Checkout(ctx context.Context, path, url string, auth *git.Auth) (git.Commit, string, error) {
-	rng, err := semver.ParseRange(c.semVer)
+	verConstraint, err := semver.NewConstraint(c.semVer)
 	if err != nil {
 		return nil, "", fmt.Errorf("semver parse range error: %w", err)
 	}
@@ -186,28 +189,61 @@ func (c *CheckoutSemVer) Checkout(ctx context.Context, path, url string, auth *g
 		return nil, "", fmt.Errorf("unable to clone '%s', error: %w", url, err)
 	}
 
-	repoTags, err := repo.Tags.List()
-	if err != nil {
-		return nil, "", fmt.Errorf("git list tags error: %w", err)
-	}
+	tags := make(map[string]string)
+	tagTimestamps := make(map[string]time.Time)
+	if err := repo.Tags.Foreach(func(name string, id *git2go.Oid) error {
+		tag, err := repo.LookupTag(id)
+		if err != nil {
+			return nil
+		}
 
-	svTags := make(map[string]string)
-	var svers []semver.Version
-	for _, tag := range repoTags {
-		v, _ := semver.ParseTolerant(tag)
-		if rng(v) {
-			svers = append(svers, v)
-			svTags[v.String()] = tag
+		commit, err := tag.Peel(git2go.ObjectCommit)
+		if err != nil {
+			return fmt.Errorf("can't get commit for tag %s: %w", name, err)
 		}
+		c, err := commit.AsCommit()
+		if err != nil {
+			return err
+		}
+		tagTimestamps[tag.Name()] = c.Committer().When
+		tags[tag.Name()] = name
+		return nil
+	}); err != nil {
+		return nil, "", err
 	}
 
-	if len(svers) == 0 {
+	var matchedVersions semver.Collection
+	for tag, _ := range tags {
+		v, err := version.ParseVersion(tag)
+		if err != nil {
+			continue
+		}
+		if !verConstraint.Check(v) {
+			continue
+		}
+		matchedVersions = append(matchedVersions, v)
+	}
+	if len(matchedVersions) == 0 {
 		return nil, "", fmt.Errorf("no match found for semver: %s", c.semVer)
 	}
 
-	semver.Sort(svers)
-	v := svers[len(svers)-1]
-	t := svTags[v.String()]
+	// Sort versions
+	sort.SliceStable(matchedVersions, func(i, j int) bool {
+		left := matchedVersions[i]
+		right := matchedVersions[j]
+
+		if !left.Equal(right) {
+			return left.LessThan(right)
+		}
+
+		// Having tag target timestamps at our disposal, we further try to sort
+		// versions into a chronological order. This is especially important for
+		// versions that differ only by build metadata, because it is not considered
+		// a part of the comparable version in Semver
+		return tagTimestamps[left.String()].Before(tagTimestamps[right.String()])
+	})
+	v := matchedVersions[len(matchedVersions)-1]
+	t := v.Original()
 
 	ref, err := repo.References.Dwim(t)
 	if err != nil {