git: refactor authentication, checkout and verification

api/v1beta1/gitrepository_types.go

@@ -120,7 +120,6 @@ type GitRepositoryInclude struct {
 // GitRepositoryRef defines the Git ref used for pull and checkout operations.
 type GitRepositoryRef struct {
 	// The Git branch to checkout, defaults to master.
-	// +kubebuilder:default:=master
 	// +optional
 	Branch string `json:"branch,omitempty"`
 

config/crd/bases/source.toolkit.fluxcd.io_gitrepositories.yaml

@@ -91,7 +91,6 @@ spec:
                 description: The Git reference to checkout and monitor for changes, defaults to master branch.
                 properties:
                   branch:
-                    default: master
                     description: The Git branch to checkout, defaults to master.
                     type: string
                   commit:

controllers/gitrepository_controller.go

@@ -229,58 +229,47 @@ func (r *GitRepositoryReconciler) reconcile(ctx context.Context, repository sour
 	}
 	defer os.RemoveAll(tmpGit)
 
-	// determine auth method
-	auth := &git.Auth{}
+	// Configure auth options using secret
+	var authOpts *git.AuthOptions
 	if repository.Spec.SecretRef != nil {
-		authStrategy, err := strategy.AuthSecretStrategyForURL(
-			repository.Spec.URL,
-			git.CheckoutOptions{
-				GitImplementation: repository.Spec.GitImplementation,
-				RecurseSubmodules: repository.Spec.RecurseSubmodules,
-			})
-		if err != nil {
-			return sourcev1.GitRepositoryNotReady(repository, sourcev1.AuthenticationFailedReason, err.Error()), err
-		}
-
 		name := types.NamespacedName{
 			Namespace: repository.GetNamespace(),
 			Name:      repository.Spec.SecretRef.Name,
 		}
 
-		var secret corev1.Secret
-		err = r.Client.Get(ctx, name, &secret)
+		secret := &corev1.Secret{}
+		err = r.Client.Get(ctx, name, secret)
 		if err != nil {
 			err = fmt.Errorf("auth secret error: %w", err)
 			return sourcev1.GitRepositoryNotReady(repository, sourcev1.AuthenticationFailedReason, err.Error()), err
 		}
 
-		auth, err = authStrategy.Method(secret)
+		authOpts, err = git.AuthOptionsFromSecret(repository.Spec.URL, secret)
 		if err != nil {
-			err = fmt.Errorf("auth error: %w", err)
 			return sourcev1.GitRepositoryNotReady(repository, sourcev1.AuthenticationFailedReason, err.Error()), err
 		}
 	}
-
-	checkoutStrategy, err := strategy.CheckoutStrategyForRef(
-		repository.Spec.Reference,
-		git.CheckoutOptions{
-			GitImplementation: repository.Spec.GitImplementation,
-			RecurseSubmodules: repository.Spec.RecurseSubmodules,
-		},
-	)
+	checkoutOpts := git.CheckoutOptions{RecurseSubmodules: repository.Spec.RecurseSubmodules}
+	if ref := repository.Spec.Reference; ref != nil {
+		checkoutOpts.Branch = ref.Branch
+		checkoutOpts.Commit = ref.Commit
+		checkoutOpts.Tag = ref.Tag
+		checkoutOpts.SemVer = ref.SemVer
+	}
+	checkoutStrategy, err := strategy.CheckoutStrategyForImplementation(ctx,
+		git.Implementation(repository.Spec.GitImplementation), checkoutOpts)
 	if err != nil {
 		return sourcev1.GitRepositoryNotReady(repository, sourcev1.GitOperationFailedReason, err.Error()), err
 	}
 
 	gitCtx, cancel := context.WithTimeout(ctx, repository.Spec.Timeout.Duration)
 	defer cancel()
 
-	commit, revision, err := checkoutStrategy.Checkout(gitCtx, tmpGit, repository.Spec.URL, auth)
+	commit, err := checkoutStrategy.Checkout(gitCtx, tmpGit, repository.Spec.URL, authOpts)
 	if err != nil {
 		return sourcev1.GitRepositoryNotReady(repository, sourcev1.GitOperationFailedReason, err.Error()), err
 	}
-
-	artifact := r.Storage.NewArtifactFor(repository.Kind, repository.GetObjectMeta(), revision, fmt.Sprintf("%s.tar.gz", commit.Hash()))
+	artifact := r.Storage.NewArtifactFor(repository.Kind, repository.GetObjectMeta(), commit.String(), fmt.Sprintf("%s.tar.gz", commit.Hash.String()))
 
 	// copy all included repository into the artifact
 	includedArtifacts := []*sourcev1.Artifact{}
@@ -309,14 +298,17 @@ func (r *GitRepositoryReconciler) reconcile(ctx context.Context, repository sour
 			Namespace: repository.Namespace,
 			Name:      repository.Spec.Verification.SecretRef.Name,
 		}
-		var secret corev1.Secret
-		if err := r.Client.Get(ctx, publicKeySecret, &secret); err != nil {
+		var secret *corev1.Secret
+		if err := r.Client.Get(ctx, publicKeySecret, secret); err != nil {
 			err = fmt.Errorf("PGP public keys secret error: %w", err)
 			return sourcev1.GitRepositoryNotReady(repository, sourcev1.VerificationFailedReason, err.Error()), err
 		}
 
-		err := commit.Verify(secret)
-		if err != nil {
+		var keyRings []string
+		for _, v := range secret.Data {
+			keyRings = append(keyRings, string(v))
+		}
+		if _, err = commit.Verify(keyRings...); err != nil {
 			return sourcev1.GitRepositoryNotReady(repository, sourcev1.VerificationFailedReason, err.Error()), err
 		}
 	}

controllers/gitrepository_controller_test.go

@@ -23,11 +23,9 @@ import (
 	"net/http"
 	"net/url"
 	"os"
-
 	"os/exec"
 	"path"
 	"path/filepath"
-
 	"strings"
 	"time"
 
@@ -251,7 +249,7 @@ var _ = Describe("GitRepositoryReconciler", func() {
 				reference:     &sourcev1.GitRepositoryRef{SemVer: "1.2.3.4"},
 				waitForReason: sourcev1.GitOperationFailedReason,
 				expectStatus:  metav1.ConditionFalse,
-				expectMessage: "semver parse range error: improper constraint: 1.2.3.4",
+				expectMessage: "semver parse error: improper constraint: 1.2.3.4",
 			}),
 			Entry("semver no match", refTestCase{
 				reference:     &sourcev1.GitRepositoryRef{SemVer: "1.0.0"},
@@ -265,7 +263,7 @@ var _ = Describe("GitRepositoryReconciler", func() {
 				},
 				waitForReason:  sourcev1.GitOperationSucceedReason,
 				expectStatus:   metav1.ConditionTrue,
-				expectRevision: "master",
+				expectRevision: "HEAD",
 			}),
 			Entry("commit in branch", refTestCase{
 				reference: &sourcev1.GitRepositoryRef{
@@ -284,7 +282,7 @@ var _ = Describe("GitRepositoryReconciler", func() {
 				},
 				waitForReason: sourcev1.GitOperationFailedReason,
 				expectStatus:  metav1.ConditionFalse,
-				expectMessage: "git commit 'invalid' not found: object not found",
+				expectMessage: "failed to resolve commit object for 'invalid': object not found",
 			}),
 		)
 
@@ -385,7 +383,7 @@ var _ = Describe("GitRepositoryReconciler", func() {
 				reference:         &sourcev1.GitRepositoryRef{Branch: "main"},
 				waitForReason:     sourcev1.GitOperationFailedReason,
 				expectStatus:      metav1.ConditionFalse,
-				expectMessage:     "error: user rejected certificate",
+				expectMessage:     "unable to clone: user rejected certificate",
 				gitImplementation: sourcev1.LibGit2Implementation,
 			}),
 			Entry("self signed libgit2 with CA", refTestCase{

controllers/helmchart_controller.go

@@ -529,7 +529,7 @@ func (r *HelmChartReconciler) reconcileFromTarballArtifact(ctx context.Context,
 
 	v, err := semver.NewVersion(helmChart.Metadata.Version)
 	if err != nil {
-		err = fmt.Errorf("semver error: %w", err)
+		err = fmt.Errorf("semver parse error: %w", err)
 		return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
 	}
 
@@ -539,7 +539,7 @@ func (r *HelmChartReconciler) reconcileFromTarballArtifact(ctx context.Context,
 		splitRev := strings.Split(artifact.Revision, "/")
 		v, err := v.SetMetadata(splitRev[len(splitRev)-1])
 		if err != nil {
-			err = fmt.Errorf("semver error: %w", err)
+			err = fmt.Errorf("semver parse error: %w", err)
 			return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
 		}
 

docs/spec/v1beta1/gitrepositories.md

@@ -273,6 +273,21 @@ spec:
     commit: 363a6a8fe6a7f13e05d34c163b0ef02a777da20a
 ```
 
+Checkout a specific commit:
+
+```yaml
+apiVersion: source.toolkit.fluxcd.io/v1beta1
+kind: GitRepository
+metadata:
+  name: podinfo
+  namespace: default
+spec:
+  interval: 1m
+  url: https://github.com/stefanprodan/podinfo
+  ref:
+    commit: 363a6a8fe6a7f13e05d34c163b0ef02a777da20a
+```
+
 Pull a specific tag:
 
 ```yaml

go.mod

@@ -8,9 +8,10 @@ require (
 	cloud.google.com/go v0.93.3 // indirect
 	cloud.google.com/go/storage v1.16.0
 	github.com/Masterminds/semver/v3 v3.1.1
+	github.com/ProtonMail/go-crypto v0.0.0-20210428141323-04723f9f07d7
 	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
+	github.com/fluxcd/pkg/gittestserver v0.4.1
 	github.com/fluxcd/pkg/gitutil v0.1.0
 	github.com/fluxcd/pkg/helmtestserver v0.2.0
 	github.com/fluxcd/pkg/lockedfile v0.1.0

go.sum

@@ -266,8 +266,8 @@ github.com/fatih/color v1.7.0 h1:DkWD4oS2D8LGGgTQ6IvwJJXSL5Vp2ffcQg58nFV38Ys=
 github.com/fatih/color v1.7.0/go.mod h1:Zm6kSWBoL9eyXnKyktHP6abPY2pDugNf5KwzbycvMj4=
 github.com/fluxcd/pkg/apis/meta v0.10.0 h1:N7wVGHC1cyPdT87hrDC7UwCwRwnZdQM46PBSLjG2rlE=
 github.com/fluxcd/pkg/apis/meta v0.10.0/go.mod h1:CW9X9ijMTpNe7BwnokiUOrLl/h13miwVr/3abEQLbKE=
-github.com/fluxcd/pkg/gittestserver v0.3.0 h1:6aa30mybecBwBWaJ2IEk7pQzefWnjWjxkTSrHMHawvg=
-github.com/fluxcd/pkg/gittestserver v0.3.0/go.mod h1:8j36Z6B0BuKNZZ6exAWoyDEpyQoFcjz1IX3WBT7PZNg=
+github.com/fluxcd/pkg/gittestserver v0.4.1 h1:knghRrVEEPnpO0VJYjoz0H2YMc4fnKAVt5hDGsB1IHc=
+github.com/fluxcd/pkg/gittestserver v0.4.1/go.mod h1:hUPx21fe/6oox336Wih/XF1fnmzLmptNMOvATbTZXNY=
 github.com/fluxcd/pkg/gitutil v0.1.0 h1:VO3kJY/CKOCO4ysDNqfdpTg04icAKBOSb3lbR5uE/IE=
 github.com/fluxcd/pkg/gitutil v0.1.0/go.mod h1:Ybz50Ck5gkcnvF0TagaMwtlRy3X3wXuiri1HVsK5id4=
 github.com/fluxcd/pkg/helmtestserver v0.2.0 h1:cE7YHDmrWI0hr9QpaaeQ0vQ16Z0IiqZKiINDpqdY610=
@@ -984,7 +984,6 @@ golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8U
 golang.org/x/crypto v0.0.0-20200414173820-0848c9571904/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto=
 golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto=
 golang.org/x/crypto v0.0.0-20200709230013-948cd5f35899/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto=
-golang.org/x/crypto v0.0.0-20200728195943-123391ffb6de/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto=
 golang.org/x/crypto v0.0.0-20201002170205-7f63de1d35b0/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto=
 golang.org/x/crypto v0.0.0-20201203163018-be400aefbc4c/go.mod h1:jdWPYTVW3xRLrWPugEBEK3UY2ZEsg3UU495nc5E+M+I=
 golang.org/x/crypto v0.0.0-20201221181555-eec23a3978ad/go.mod h1:jdWPYTVW3xRLrWPugEBEK3UY2ZEsg3UU495nc5E+M+I=

pkg/git/git.go

@@ -17,43 +17,82 @@ limitations under the License.
 package git
 
 import (
+	"bytes"
 	"context"
+	"fmt"
+	"strings"
+	"time"
 
-	"github.com/go-git/go-git/v5/plumbing/transport"
-	git2go "github.com/libgit2/git2go/v31"
-	corev1 "k8s.io/api/core/v1"
+	"github.com/ProtonMail/go-crypto/openpgp"
 )
 
-const (
-	DefaultOrigin            = "origin"
-	DefaultBranch            = "master"
-	DefaultPublicKeyAuthUser = "git"
-	CAFile                   = "caFile"
-)
+type Implementation string
+
+type Hash []byte
 
-type Commit interface {
-	Verify(secret corev1.Secret) error
-	Hash() string
+// String returns the SHA1 Hash as a string.
+func (h Hash) String() string {
+	return string(h)
 }
 
-type CheckoutStrategy interface {
-	Checkout(ctx context.Context, path, url string, auth *Auth) (Commit, string, error)
+type Signature struct {
+	Name  string
+	Email string
+	When  time.Time
 }
 
-type CheckoutOptions struct {
-	GitImplementation string
-	RecurseSubmodules bool
+type Commit struct {
+	// Hash is the SHA1 hash of the commit.
+	Hash Hash
+	// Reference is the original reference of the commit, for example:
+	// 'refs/tags/foo'.
+	Reference string
+	// Author is the original author of the commit.
+	Author Signature
+	// Committer is the one performing the commit, might be different from
+	// Author.
+	Committer Signature
+	// Signature is the PGP signature of the commit.
+	Signature string
+	// Encoded is the encoded commit, without any signature.
+	Encoded []byte
+	// Message is the commit message, contains arbitrary text.
+	Message string
 }
 
-// TODO(hidde): candidate for refactoring, so that we do not directly
-//  depend on implementation specifics here.
-type Auth struct {
-	AuthMethod   transport.AuthMethod
-	CABundle     []byte
-	CredCallback git2go.CredentialsCallback
-	CertCallback git2go.CertificateCheckCallback
+// String returns a string representation of the Commit, composed
+// out the last part of the Reference element, and/or Hash.
+// For example: 'tag-1/a0c14dc8580a23f79bc654faa79c4f62b46c2c22',
+// for a "tag-1" tag.
+func (c *Commit) String() string {
+	if short := strings.SplitAfterN(c.Reference, "/", 3); len(short) == 3 {
+		return fmt.Sprintf("%s/%s", short[2], c.Hash)
+	}
+	return fmt.Sprintf("HEAD/%s", c.Hash)
 }
 
-type AuthSecretStrategy interface {
-	Method(secret corev1.Secret) (*Auth, error)
+// Verify the Signature of the commit with the given key rings.
+// It returns the fingerprint of the key the signature was verified
+// with, or an error.
+func (c *Commit) Verify(keyRing ...string) (string, error) {
+	if c.Signature == "" {
+		return "", fmt.Errorf("commit does not have a PGP signature")
+	}
+
+	for _, r := range keyRing {
+		reader := strings.NewReader(r)
+		keyring, err := openpgp.ReadArmoredKeyRing(reader)
+		if err != nil {
+			return "", fmt.Errorf("failed to read armored key ring: %w", err)
+		}
+		signer, err := openpgp.CheckArmoredDetachedSignature(keyring, bytes.NewBuffer(c.Encoded), bytes.NewBufferString(c.Signature), nil)
+		if err == nil {
+			return fmt.Sprintf("%X", signer.PrimaryKey.Fingerprint[12:20]), nil
+		}
+	}
+	return "", fmt.Errorf("failed to verify commit with any of the given key rings")
+}
+
+type CheckoutStrategy interface {
+	Checkout(ctx context.Context, path, url string, config *AuthOptions) (*Commit, error)
 }