From 95cddf616ce5e662341227a06cfd8160b9945dc6 Mon Sep 17 00:00:00 2001 From: Matheus Pimenta Date: Mon, 21 Sep 2026 16:06:14 +0100 Subject: [PATCH] Reverify unchanged GitRepository on key rotation The optimized same-revision checkout returns a partial commit and, when the content config and verification mode are unchanged, returns before verifySignature. Since the verification policy was not part of the decision, rotating spec.verify.secretRef or its key material kept an existing SourceVerified=True condition for the new generation, allowing a revoked signing key to remain trusted for the advertised artifact. Record a fingerprint of the verification public keys in the status on success, and require a full checkout and re-verification when the fingerprint of the keys in the referenced Secret changes. The fingerprint depends only on the key material, not on the Secret name or the data key names. Signed-off-by: Matheus Pimenta Assisted-by: opencode/deepseek-v4.1-flash --- api/v1/gitrepository_types.go | 7 + api/v1beta1/zz_generated.deepcopy.go | 2 +- ...rce.toolkit.fluxcd.io_gitrepositories.yaml | 7 + docs/api/v1/source.md | 15 ++ docs/spec/v1/gitrepositories.md | 10 + .../controller/gitrepository_controller.go | 79 +++++-- .../gitrepository_controller_test.go | 218 ++++++++++++++++++ 7 files changed, 324 insertions(+), 14 deletions(-) diff --git a/api/v1/gitrepository_types.go b/api/v1/gitrepository_types.go index eb8be2d32..569c6c91d 100644 --- a/api/v1/gitrepository_types.go +++ b/api/v1/gitrepository_types.go @@ -288,6 +288,13 @@ type GitRepositoryStatus struct { // +optional ObservedSparseCheckout []string `json:"observedSparseCheckout,omitempty"` + // SourceVerificationFingerprint is the fingerprint of the public keys used + // to verify the signature of the Git object(s) for the current Artifact. + // It is used to detect changes to the verification policy, such as a key + // rotation, that require the current revision to be verified again. + // +optional + SourceVerificationFingerprint string `json:"sourceVerificationFingerprint,omitempty"` + // SourceVerificationMode is the last used verification mode indicating // which Git object(s) have been verified. // +optional diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go index 10be7301e..1bc9d6296 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -24,7 +24,7 @@ import ( "github.com/fluxcd/pkg/apis/acl" "github.com/fluxcd/pkg/apis/meta" "k8s.io/apimachinery/pkg/apis/meta/v1" - runtime "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime" ) // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. diff --git a/config/crd/bases/source.toolkit.fluxcd.io_gitrepositories.yaml b/config/crd/bases/source.toolkit.fluxcd.io_gitrepositories.yaml index 5fa5cf33c..dfb8628a8 100644 --- a/config/crd/bases/source.toolkit.fluxcd.io_gitrepositories.yaml +++ b/config/crd/bases/source.toolkit.fluxcd.io_gitrepositories.yaml @@ -477,6 +477,13 @@ spec: items: type: string type: array + sourceVerificationFingerprint: + description: |- + SourceVerificationFingerprint is the fingerprint of the public keys used + to verify the signature of the Git object(s) for the current Artifact. + It is used to detect changes to the verification policy, such as a key + rotation, that require the current revision to be verified again. + type: string sourceVerificationMode: description: |- SourceVerificationMode is the last used verification mode indicating diff --git a/docs/api/v1/source.md b/docs/api/v1/source.md index a3ea61377..871f7a196 100644 --- a/docs/api/v1/source.md +++ b/docs/api/v1/source.md @@ -2413,6 +2413,21 @@ produce the current Artifact.

+sourceVerificationFingerprint
+ +string + + + +(Optional) +

SourceVerificationFingerprint is the fingerprint of the public keys used +to verify the signature of the Git object(s) for the current Artifact. +It is used to detect changes to the verification policy, such as a key +rotation, that require the current revision to be verified again.

+ + + + sourceVerificationMode
diff --git a/docs/spec/v1/gitrepositories.md b/docs/spec/v1/gitrepositories.md index 4c9d244af..a2148f380 100644 --- a/docs/spec/v1/gitrepositories.md +++ b/docs/spec/v1/gitrepositories.md @@ -1342,6 +1342,16 @@ mode in spec](#verification). The verification status is applicable only to the latest Git repository revision used to successfully build and store an artifact. +### Source Verification Fingerprint + +The source-controller reports a fingerprint of the public keys it used to verify +the Git object(s) in the GitRepository's +`.status.sourceVerificationFingerprint`. The fingerprint is derived from the key +material in the referenced Secret and does not depend on the Secret name or the +Secret data key names. It is used by the controller to detect a change in the +verification policy, such as a key rotation, that requires the current revision +to be verified again even when its revision did not change. + ### Observed Generation The source-controller reports an [observed generation][typical-status-properties] diff --git a/internal/controller/gitrepository_controller.go b/internal/controller/gitrepository_controller.go index a3c27c9b6..df3051fff 100644 --- a/internal/controller/gitrepository_controller.go +++ b/internal/controller/gitrepository_controller.go @@ -23,6 +23,7 @@ import ( "net/url" "os" "path/filepath" + "sort" "strings" "time" @@ -33,6 +34,7 @@ import ( "github.com/fluxcd/pkg/runtime/logger" "github.com/fluxcd/pkg/runtime/secrets" "github.com/go-git/go-git/v5/plumbing/transport" + "github.com/opencontainers/go-digest" ssh "golang.org/x/crypto/ssh" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" @@ -592,7 +594,9 @@ func (r *GitRepositoryReconciler) reconcileSource(ctx context.Context, sp *patch // reconciliation can be skipped if other configurations have not changed. if !git.IsConcreteCommit(*commit) { // Check if the content config contributing to the artifact has changed. - if !gitContentConfigChanged(obj, includes) { + // A change to the verification policy (e.g. a key rotation) also + // requires a new verification of the unchanged revision. + if !gitContentConfigChanged(obj, includes) && !r.verificationPolicyChanged(ctx, obj) { ge := serror.NewGeneric( fmt.Errorf("no changes since last reconciliation: observed revision '%s'", commitReference(obj, commit)), sourcev1.GitOperationSucceedReason, @@ -1112,6 +1116,64 @@ func (r *GitRepositoryReconciler) fetchIncludes(ctx context.Context, obj *source return &artifacts, nil } +// verificationKeys returns the PGP key rings and SSH authorized keys contained +// in the given Secret. Data entries with an SSH public key suffix are treated as +// authorized keys, entries with a PGP public key suffix (or no known suffix) as +// PGP key rings. +func verificationKeys(secret *corev1.Secret) (keyRings, authorizedKeys []string) { + for k, v := range secret.Data { + switch { + case strings.HasSuffix(k, publicKeySSHSuffix): + authorizedKeys = append(authorizedKeys, string(v)) + case strings.HasSuffix(k, publicKeyPGPSuffix): + keyRings = append(keyRings, string(v)) + default: + // Provide fallback to support previous undocumented behavior + keyRings = append(keyRings, string(v)) + } + } + return keyRings, authorizedKeys +} + +// verificationFingerprint returns a stable fingerprint of the public keys in +// the given Secret, independent of the Secret name or the data key names. It is +// used to detect a change in the verification policy, e.g. a key rotation, that +// requires the current revision to be verified again. +func verificationFingerprint(secret *corev1.Secret) string { + keyRings, authorizedKeys := verificationKeys(secret) + sort.Strings(keyRings) + sort.Strings(authorizedKeys) + + var b strings.Builder + for _, k := range keyRings { + b.WriteString("pgp:") + b.WriteString(k) + b.WriteByte(0) + } + for _, k := range authorizedKeys { + b.WriteString("ssh:") + b.WriteString(k) + b.WriteByte(0) + } + return digest.Canonical.FromString(b.String()).String() +} + +// verificationPolicyChanged returns true if the public keys trusted for +// verification differ from the ones used for the last successful verification, +// or if the current policy can not be determined. A changed policy requires the +// current revision to be verified again, even if it did not change. +func (r *GitRepositoryReconciler) verificationPolicyChanged(ctx context.Context, obj *sourcev1.GitRepository) bool { + if obj.Spec.Verification == nil || obj.Spec.Verification.Mode == "" { + return false + } + secret, err := r.getSecret(ctx, obj.Spec.Verification.SecretRef.Name, obj.GetNamespace()) + if err != nil { + // Return true so the full reconciliation surfaces the error. + return true + } + return verificationFingerprint(secret) != obj.Status.SourceVerificationFingerprint +} + // verifySignature verifies the signature of the given Git commit and/or its referencing tag // depending on the verification mode specified on the object. // If the signature can not be verified or the verification fails, it records @@ -1124,6 +1186,7 @@ func (r *GitRepositoryReconciler) verifySignature(ctx context.Context, obj *sour // observations if there is none if obj.Spec.Verification == nil || obj.Spec.Verification.Mode == "" { obj.Status.SourceVerificationMode = nil + obj.Status.SourceVerificationFingerprint = "" conditions.Delete(obj, sourcev1.SourceVerifiedCondition) return sreconcile.ResultSuccess, nil } @@ -1143,18 +1206,7 @@ func (r *GitRepositoryReconciler) verifySignature(ctx context.Context, obj *sour return sreconcile.ResultEmpty, e } - var keyRings []string - var authorizedKeys []string - for k, v := range secret.Data { - if strings.HasSuffix(k, publicKeySSHSuffix) { - authorizedKeys = append(authorizedKeys, string(v)) - } else if strings.HasSuffix(k, publicKeyPGPSuffix) { - keyRings = append(keyRings, string(v)) - } else { - // Provide fallback to support previous undocumented behavior - keyRings = append(keyRings, string(v)) - } - } + keyRings, authorizedKeys := verificationKeys(secret) var message strings.Builder if obj.Spec.Verification.VerifyTag() { @@ -1217,6 +1269,7 @@ func (r *GitRepositoryReconciler) verifySignature(ctx context.Context, obj *sour reason := meta.SucceededReason mode := obj.Spec.Verification.GetMode() obj.Status.SourceVerificationMode = &mode + obj.Status.SourceVerificationFingerprint = verificationFingerprint(secret) conditions.MarkTrue(obj, sourcev1.SourceVerifiedCondition, reason, "%s", message.String()) r.eventLogf(ctx, obj, eventv1.EventTypeTrace, reason, "%s", message.String()) return sreconcile.ResultSuccess, nil diff --git a/internal/controller/gitrepository_controller_test.go b/internal/controller/gitrepository_controller_test.go index 84b2074a8..d5b3b7c2f 100644 --- a/internal/controller/gitrepository_controller_test.go +++ b/internal/controller/gitrepository_controller_test.go @@ -2149,6 +2149,7 @@ func TestGitRepositoryReconciler_verifySignature(t *testing.T) { name: "Source verification mode in status is unset if there's no verification in spec", beforeFunc: func(obj *sourcev1.GitRepository) { obj.Status.SourceVerificationMode = ptrToVerificationMode(sourcev1.ModeGitHEAD) + obj.Status.SourceVerificationFingerprint = "stale-fingerprint" obj.Spec.Verification = nil }, want: sreconcile.ResultSuccess, @@ -2789,8 +2790,12 @@ func TestGitRepositoryReconciler_verifySignature(t *testing.T) { g.Expect(got).To(Equal(tt.want)) if tt.wantSourceVerificationMode != nil { g.Expect(*obj.Status.SourceVerificationMode).To(Equal(*tt.wantSourceVerificationMode)) + if tt.secret != nil { + g.Expect(obj.Status.SourceVerificationFingerprint).To(Equal(verificationFingerprint(tt.secret))) + } } else { g.Expect(obj.Status.SourceVerificationMode).To(BeNil()) + g.Expect(obj.Status.SourceVerificationFingerprint).To(BeEmpty()) } }) } @@ -4084,6 +4089,219 @@ func Test_requiresVerification(t *testing.T) { } } +func Test_verificationFingerprint(t *testing.T) { + g := NewWithT(t) + + pgpKey := armoredKeyRingFixture + sshKey := "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIExampleKeyForFingerprintTests" + + // The same key material stored under a different data key must yield the + // same fingerprint, as the policy is defined by the keys, not their names. + base := &corev1.Secret{Data: map[string][]byte{"foo": []byte(pgpKey)}} + sameKey := &corev1.Secret{Data: map[string][]byte{"bar.asc": []byte(pgpKey)}} + g.Expect(verificationFingerprint(base)).To(Equal(verificationFingerprint(sameKey))) + + // The fingerprint must be stable regardless of the map iteration order. + mixed := &corev1.Secret{Data: map[string][]byte{ + "a.asc": []byte(pgpKey), + "b.sshpub": []byte(sshKey), + "c.sshpub": []byte(sshKey + "\n"), + }} + g.Expect(verificationFingerprint(mixed)).To(Equal(verificationFingerprint(mixed))) + + // Adding an SSH key changes the fingerprint. + g.Expect(verificationFingerprint(mixed)).ToNot(Equal(verificationFingerprint(base))) + + // Changing the key material changes the fingerprint. + changed := &corev1.Secret{Data: map[string][]byte{"foo": []byte(pgpKey + "\n")}} + g.Expect(verificationFingerprint(changed)).ToNot(Equal(verificationFingerprint(base))) +} + +func TestGitRepositoryReconciler_verificationPolicyChanged(t *testing.T) { + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "keys", Namespace: "default"}, + Data: map[string][]byte{"foo": []byte(armoredKeyRingFixture)}, + } + rotatedSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "keys", Namespace: "default"}, + Data: map[string][]byte{"foo": []byte(armoredKeyRingFixture + "\n")}, + } + + newObj := func(mode sourcev1.GitVerificationMode, fingerprint string) *sourcev1.GitRepository { + obj := &sourcev1.GitRepository{ + ObjectMeta: metav1.ObjectMeta{Name: "repo", Namespace: "default"}, + Spec: sourcev1.GitRepositorySpec{ + Verification: &sourcev1.GitRepositoryVerification{ + Mode: mode, + SecretRef: meta.LocalObjectReference{Name: "keys"}, + }, + }, + } + obj.Status.SourceVerificationFingerprint = fingerprint + return obj + } + + tests := []struct { + name string + obj *sourcev1.GitRepository + secret *corev1.Secret + want bool + }{ + { + name: "no verification configured", + obj: &sourcev1.GitRepository{}, + want: false, + }, + { + name: "verification without mode", + obj: &sourcev1.GitRepository{ + Spec: sourcev1.GitRepositorySpec{ + Verification: &sourcev1.GitRepositoryVerification{ + SecretRef: meta.LocalObjectReference{Name: "keys"}, + }, + }, + }, + want: false, + }, + { + name: "missing secret requires verification", + obj: newObj(sourcev1.ModeGitHEAD, ""), + want: true, + }, + { + name: "unchanged keys do not require verification", + obj: newObj(sourcev1.ModeGitHEAD, verificationFingerprint(secret)), + secret: secret, + want: false, + }, + { + name: "rotated keys require verification", + obj: newObj(sourcev1.ModeGitHEAD, verificationFingerprint(secret)), + secret: rotatedSecret, + want: true, + }, + { + name: "missing observed fingerprint requires verification", + obj: newObj(sourcev1.ModeGitHEAD, ""), + secret: secret, + want: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + clientBuilder := fakeclient.NewClientBuilder().WithScheme(testEnv.GetScheme()) + if tt.secret != nil { + clientBuilder = clientBuilder.WithObjects(tt.secret) + } + r := &GitRepositoryReconciler{Client: clientBuilder.Build()} + g.Expect(r.verificationPolicyChanged(ctx, tt.obj)).To(Equal(tt.want)) + }) + } +} + +func TestGitRepositoryReconciler_reconcileSource_verificationPolicyChange(t *testing.T) { + g := NewWithT(t) + + server, err := gittestserver.NewTempGitServer() + g.Expect(err).NotTo(HaveOccurred()) + defer os.RemoveAll(server.Root()) + server.AutoCreate() + g.Expect(server.StartHTTP()).To(Succeed()) + defer server.StopHTTP() + + repoPath := "/test.git" + localRepo, err := initGitRepo(server, "testdata/git/repository", git.DefaultBranch, repoPath) + g.Expect(err).NotTo(HaveOccurred()) + + headRef, err := localRepo.Head() + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(remoteBranchForHead(localRepo, headRef, "staging")).To(Succeed()) + + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "keys", Namespace: "default"}, + Data: map[string][]byte{"foo": []byte(armoredKeyRingFixture)}, + } + + tests := []struct { + name string + fingerprint string + wantVerification bool + }{ + { + name: "unchanged policy skips verification of the unchanged revision", + fingerprint: verificationFingerprint(secret), + }, + { + name: "changed policy reverifies the unchanged revision", + fingerprint: "stale", + wantVerification: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + obj := &sourcev1.GitRepository{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "verification-policy-", + Namespace: "default", + Generation: 1, + }, + Spec: sourcev1.GitRepositorySpec{ + Interval: metav1.Duration{Duration: interval}, + Timeout: &metav1.Duration{Duration: timeout}, + URL: server.HTTPAddress() + repoPath, + Reference: &sourcev1.GitRepositoryRef{ + Branch: "staging", + }, + Verification: &sourcev1.GitRepositoryVerification{ + Mode: sourcev1.ModeGitHEAD, + SecretRef: meta.LocalObjectReference{Name: "keys"}, + }, + }, + Status: sourcev1.GitRepositoryStatus{ + Artifact: &meta.Artifact{ + Revision: "staging@sha1:" + headRef.Hash().String(), + Path: randStringRunes(10), + }, + SourceVerificationMode: ptrToVerificationMode(sourcev1.ModeGitHEAD), + SourceVerificationFingerprint: tt.fingerprint, + }, + } + conditions.MarkTrue(obj, sourcev1.ArtifactInStorageCondition, meta.SucceededReason, "foo") + + c := fakeclient.NewClientBuilder(). + WithScheme(testEnv.GetScheme()). + WithObjects(obj, secret). + WithStatusSubresource(&sourcev1.GitRepository{}). + Build() + r := &GitRepositoryReconciler{ + Client: c, + EventRecorder: record.NewFakeRecorder(32), + Storage: testStorage, + patchOptions: getPatchOptions(gitRepositoryReadyCondition.Owned, "sc"), + } + + var commit git.Commit + var includes artifactSet + sp := patch.NewSerialPatcher(obj, c) + _, err := r.reconcileSource(ctx, sp, obj, &commit, &includes, t.TempDir()) + g.Expect(err).To(HaveOccurred()) + + if tt.wantVerification { + // The full checkout is performed, but the unsigned test commit + // fails verification. + g.Expect(err.Error()).To(ContainSubstring("signature verification of commit")) + } else { + g.Expect(err.Error()).To(ContainSubstring("no changes since last reconciliation")) + } + }) + } +} + func ptrToVerificationMode(mode sourcev1.GitVerificationMode) *sourcev1.GitVerificationMode { return &mode }