Race condition / Authorization bypass in provisioning job mutual exclusion

Proof-of-concept (Go-like pseudo-implementation to illustrate the race pre-fix): // This is a simplified in-memory demonstration of the race that the patch fixes. // It is not a drop-in replacement for the Grafana code but shows the exploit path. package main import ( "fmt" "time" ) const LabelJobClaim = "provisioning.grafana.app/claim" const LabelJobClaimOwner = "provisioning.grafana.app/claim-owner" type Job struct { Namespace string Name string UID string Labels map[string]string } type Store struct { jobs map[string]*Job } func key(ns, name string) string { return ns + "/" + name } func NewStore() *Store { return &Store{jobs: make(map[string]*Job)} } func (s *Store) CreateJob(ns, name, uid string) { j := &Job{Namespace: ns, Name: name, UID: uid, Labels: map[string]string{}} s.jobs[key(ns, name)] = j } // Claim stamps a claim timestamp and an owner token on the existing job. func (s *Store) Claim(ns, name string) string { j := s.jobs[key(ns, name)] if j == nil { panic("job not found on Claim") } ts := time.Now().UnixNano() if j.Labels == nil { j.Labels = make(map[string]string) } j.Labels[LabelJobClaim] = fmt.Sprintf("%d", ts) owner := fmt.Sprintf("owner-%d", ts) // deterministic within the claim j.Labels[LabelJobClaimOwner] = owner return owner } // Reclaim simulates another worker reaping the same named job and replacing it with a new UID/owner. func (s *Store) Reclaim(ns, name, newUID, newOwner string) { s.jobs[key(ns, name)] = &Job{ Namespace: ns, Name: name, UID: newUID, Labels: map[string]string{ LabelJobClaim: fmt.Sprintf("%d", time.Now().UnixNano()), LabelJobClaimOwner: newOwner, }, } } // RenewLease represents what happens when the old worker tries to renew its lease. // In the vulnerable pre-fix scenario, the code would renew as long as a claim exists, // ignoring UID/owner mismatches. func (s *Store) RenewLease(ns, name, uid, owner string) error { j := s.jobs[key(ns, name)] if j == nil { return fmt.Errorf("not found") } // Vulnerable behavior (pre-fix): only checks that a claim exists; ignores ownership/UID if j.Labels != nil && j.Labels[LabelJobClaim] != "" { // update claim timestamp to indicate renewal j.Labels[LabelJobClaim] = fmt.Sprintf("%d", time.Now().UnixNano()) return nil } return fmt.Errorf("lease not found") } func main() { s := NewStore() // Initial setup: a job in namespace ns with deterministic name s.CreateJob("ns", "deterministic-job", "uid-A") ownerA := s.Claim("ns", "deterministic-job") _ = ownerA // not used further in this toy PoC // Simulate a second worker reaping the same named job and creating a new incarnation // with a new UID and owner s.Reclaim("ns", "deterministic-job", "uid-B", "owner-B") // Worker A attempts to RenewLease using its old UID/owner // In the pre-fix vulnerable behavior, this would succeed (thereby violating mutual exclusion) // In the real patch, the code would detect UID/owner mismatch and return ErrLeaseLost. err := s.RenewLease("ns", "deterministic-job", "uid-A", ownerA) if err != nil { fmt.Println("RenewLease failed (as would be expected in patched code):", err) } else { fmt.Println("RenewLease succeeded (vulnerability demonstrated in pre-fix behavior)") } }

diff --git a/pkg/registry/apis/provisioning/jobs/driver.go b/pkg/registry/apis/provisioning/jobs/driver.go index 83d104eb5b8a..e23007d2a750 100644 --- a/pkg/registry/apis/provisioning/jobs/driver.go +++ b/pkg/registry/apis/provisioning/jobs/driver.go @@ -294,9 +294,20 @@ func (d *jobDriver) leaseRenewalLoop(ctx context.Context, logger logging.Logger, if err != nil { consecutiveFailures++ - if apierrors.IsNotFound(err) || - strings.Contains(err.Error(), "job no longer exists") { - logger.Error("job no longer exists - lease expired", "error", err) + // Both cases below are terminal: continuing to run would mean two workers + // process the same job. Abort immediately rather than retrying, which would + // only stomp the new owner's claim. + + // Another worker now owns the claim (job reaped and re-claimed on the same name). + if errors.Is(err, ErrLeaseLost) { + logger.Error("lease taken over by another worker - aborting job", "error", err) + close(leaseExpired) + return + } + + // The job no longer exists in the store (deleted or reaped before renewal). + if apierrors.IsNotFound(err) || strings.Contains(err.Error(), "job no longer exists") { + logger.Error("job no longer exists - aborting job", "error", err) close(leaseExpired) return } diff --git a/pkg/registry/apis/provisioning/jobs/persistentstore.go b/pkg/registry/apis/provisioning/jobs/persistentstore.go index 0a1e5c4be4e0..5f1f9a78341e 100644 --- a/pkg/registry/apis/provisioning/jobs/persistentstore.go +++ b/pkg/registry/apis/provisioning/jobs/persistentstore.go @@ -20,6 +20,7 @@ import ( client "github.com/grafana/grafana/apps/provisioning/pkg/generated/clientset/versioned/typed/provisioning/v0alpha1" "github.com/grafana/grafana/pkg/apimachinery/identity" "github.com/grafana/grafana/pkg/infra/tracing" + "github.com/grafana/grafana/pkg/util" "github.com/prometheus/client_golang/prometheus" ) @@ -28,12 +29,24 @@ const ( // The label must be formatted as milliseconds from Epoch. This grants a natural ordering, allowing for less-than operators in label selectors. // The natural ordering would be broken if the number rolls over into 1 more digit. This won't happen before Nov, 2286. LabelJobClaim = "provisioning.grafana.app/claim" + // LabelJobClaimOwner is a token unique to a single claim, identifying which worker owns it. + // Job names are deterministic (repository + action), so a reaped job and its re-created + // namesake share a name. Without an owner token, a worker cannot tell its own claim apart + // from a fresh claim placed by another worker on the same name, and would renew/complete a + // job it no longer owns -- leading to two workers running the same job. The token lets + // RenewLease and Complete verify the claim in the store is still the one we placed. + LabelJobClaimOwner = "provisioning.grafana.app/claim-owner" // LabelRepository contains the repository name as a label. This allows for label selectors to find the archived version of a job. LabelRepository = "provisioning.grafana.app/repository" // LabelJobOriginalUID contains the Job's original uid as a label. This allows for label selectors to find the archived version of a job. LabelJobOriginalUID = "provisioning.grafana.app/original-uid" ) +// ErrLeaseLost indicates the job's claim in the store is no longer the one we placed: +// the job was reaped and re-created, or another worker has taken it over. A worker that +// sees this must stop processing immediately so two workers do not run the same job. +var ErrLeaseLost = errors.New("job lease lost: claimed by another worker") + var ErrNoJobs = &apierrors.StatusError{ ErrStatus: metav1.Status{ Status: metav1.StatusFailure, @@ -137,6 +150,7 @@ func (s *persistentStore) Claim(ctx context.Context) (job *provisioning.Job, rol job.Labels = make(map[string]string) } job.Labels[LabelJobClaim] = strconv.FormatInt(s.clock().UnixMilli(), 10) + job.Labels[LabelJobClaimOwner] = util.GenerateShortUID() s.queueMetrics.RecordWaitTime(string(job.Spec.Action), s.clock().Sub(job.CreationTimestamp.Time).Seconds()) // Set up the provisioning identity for this namespace @@ -193,9 +207,19 @@ func (s *persistentStore) Claim(ctx context.Context) (job *provisioning.Job, rol return } + // Only roll back if the job in the store is still the one we claimed. Job names are + // deterministic, so this same name may now be a re-created job claimed by another + // worker. Stripping its claim would hand that worker's job to a third one and + // reintroduce duplicate execution, so leave it alone. + if refetched.UID != updatedJob.UID || refetched.Labels[LabelJobClaimOwner] != updatedJob.Labels[LabelJobClaimOwner] { + logger.Info("claim no longer owned by this worker - skipping rollback") + return + } + // Rollback the claim. refetchedJob := refetched.DeepCopy() delete(refetchedJob.Labels, LabelJobClaim) + delete(refetchedJob.Labels, LabelJobClaimOwner) refetchedJob.Status.State = provisioning.JobStatePending timeoutCtx, cancel = context.WithTimeout(ctx, 5*time.Second) @@ -306,23 +330,44 @@ func (s *persistentStore) Complete(ctx context.Context, job *provisioning.Job) e return apifmt.Errorf("failed to get provisioning identity for '%s': %w", job.GetNamespace(), err) } + // Verify we still own the job before deleting it. Job names are deterministic, so a + // reaped job and its re-created namesake share a name. Deleting purely by name would let + // a worker that has lost its lease delete the job another worker is now running. A matching + // UID proves it is the same object we claimed, and a matching owner token proves the claim + // is still ours. If the job is gone or a different incarnation exists, report NotFound so + // callers treat it as already cleaned up. + latest, err := s.client.Jobs(job.GetNamespace()).Get(ctx, job.GetName(), metav1.GetOptions{}) + if err != nil { + span.RecordError(err) + return apifmt.Errorf("failed to get job '%s' in '%s' for completion: %w", job.GetName(), job.GetNamespace(), err) + } + if latest.UID != job.UID || latest.Labels[LabelJobClaimOwner] != job.Labels[LabelJobClaimOwner] { + logger.Info("job no longer owned by this worker - skipping completion") + return apierrors.NewNotFound(provisioning.JobResourceInfo.GroupResource(), job.GetName()) + } + // Delete the job from the active job store. // Callers are responsible for writing the job to history after calling this. // - // We will assume that the caller is the claimant. If this is not true, an error is returned. - // This is a best-effort operation; if the job is not in the claimed state, we will still attempt to delete it. - err = s.client.Jobs(job.GetNamespace()).Delete(ctx, job.GetName(), metav1.DeleteOptions{}) + // The UID precondition makes the delete atomic against the ownership check above: if the + // object is replaced by a namesake between the Get and the Delete, the precondition fails. + uid := job.UID + err = s.client.Jobs(job.GetNamespace()).Delete(ctx, job.GetName(), metav1.DeleteOptions{ + Preconditions: &metav1.Preconditions{UID: &uid}, + }) if err != nil { span.RecordError(err) return apifmt.Errorf("failed to delete job '%s' in '%s': %w", job.GetName(), job.GetNamespace(), err) } logger.Debug("deleted job from job store") - // We need to remove the claim label before moving the job to the historic job store. + // We need to remove the claim labels before moving the job to the historic job store, + // so the per-claim owner token does not leak into the archived object. if job.Labels == nil { job.Labels = make(map[string]string) } delete(job.Labels, LabelJobClaim) + delete(job.Labels, LabelJobClaimOwner) s.queueMetrics.DecreaseQueueSize(string(job.Spec.Action)) logger.Debug("complete job complete") @@ -419,14 +464,23 @@ func (s *persistentStore) RenewLease(ctx context.Context, job *provisioning.Job) return apifmt.Errorf("failed to fetch job for lease renewal '%s' in '%s': %w", job.GetName(), job.GetNamespace(), err) } - // Verify we still own the lease - if latestJob.Labels == nil || latestJob.Labels[LabelJobClaim] == "" { - err := apifmt.Errorf("lease lost for job '%s' in '%s': no longer claimed", job.GetName(), job.GetNamespace()) + // Verify we still own the lease. Checking that the job is claimed is not enough: + // job names are deterministic, so the claim in the store may belong to a different + // worker that took over after ours was reaped. A matching UID proves it is the same + // object we claimed (a re-created namesake gets a fresh UID), and a matching owner + // token proves the claim is still the one we placed. If either differs, we have lost + // the lease and must not renew it. + owner := job.Labels[LabelJobClaimOwner] + if latestJob.Labels == nil || + latestJob.Labels[LabelJobClaim] == "" || + latestJob.Labels[LabelJobClaimOwner] != owner || + latestJob.UID != job.UID { + err := apifmt.Errorf("lease lost for job '%s' in '%s': %w", job.GetName(), job.GetNamespace(), ErrLeaseLost) span.RecordError(err) return err } - // Update the claim timestamp to current time + // Update the claim timestamp to current time, preserving our owner token. updatedJob := latestJob.DeepCopy() updatedJob.Labels[LabelJobClaim] = strconv.FormatInt(s.clock().UnixMilli(), 10) diff --git a/pkg/registry/apis/provisioning/jobs/persistentstore_test.go b/pkg/registry/apis/provisioning/jobs/persistentstore_test.go index 27357724c8f3..cb3c5755532a 100644 --- a/pkg/registry/apis/provisioning/jobs/persistentstore_test.go +++ b/pkg/registry/apis/provisioning/jobs/persistentstore_test.go @@ -2,11 +2,14 @@ package jobs import ( "context" + "errors" "testing" "time" + "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" provisioning "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1" @@ -15,6 +18,223 @@ import ( "github.com/grafana/grafana/pkg/apimachinery/identity" ) +func newTestStore(client provisioningv0alpha1.ProvisioningV0alpha1Interface) *persistentStore { + return &persistentStore{ + client: client, + clock: time.Now, + expiry: 30 * time.Second, + queueMetrics: RegisterQueueMetrics(prometheus.NewPedanticRegistry()), + } +} + +// TestClaim_StampsOwnerToken verifies that claiming a job stamps a unique owner +// token alongside the claim timestamp, so ownership can be verified later. +func TestClaim_StampsOwnerToken(t *testing.T) { + fakeClient := newTestClientset() + store := newTestStore(fakeClient) + + ctx, _, err := identity.WithProvisioningIdentity(context.Background(), "stacks-123") + require.NoError(t, err) + + _, err = fakeClient.Jobs("stacks-123").Create(ctx, &provisioning.Job{ + ObjectMeta: metav1.ObjectMeta{Name: "test-job", Namespace: "stacks-123"}, + Spec: provisioning.JobSpec{Repository: "test-repo", Action: provisioning.JobActionPull}, + }, metav1.CreateOptions{}) + require.NoError(t, err) + + claimed, rollback, err := store.Claim(ctx) + require.NoError(t, err) + require.NotNil(t, claimed) + defer rollback() + + assert.NotEmpty(t, claimed.Labels[LabelJobClaim], "claim timestamp should be set") + assert.NotEmpty(t, claimed.Labels[LabelJobClaimOwner], "claim owner token should be set") +} + +// TestClaim_RollbackSkipsJobOwnedByAnother verifies that the claim rollback does not +// strip the claim from a job that is now owned by another worker. Job names are +// deterministic, so by the time we roll back, the same name may be a re-created job +// another worker is running; clearing its claim would reintroduce duplicate execution. +func TestClaim_RollbackSkipsJobOwnedByAnother(t *testing.T) { + fakeClient := newTestClientset() + store := newTestStore(fakeClient) + + ctx, _, err := identity.WithProvisioningIdentity(context.Background(), "stacks-123") + require.NoError(t, err) + + _, err = fakeClient.Jobs("stacks-123").Create(ctx, &provisioning.Job{ + ObjectMeta: metav1.ObjectMeta{Name: "test-job", Namespace: "stacks-123"}, + Spec: provisioning.JobSpec{Repository: "test-repo", Action: provisioning.JobActionPull}, + }, metav1.CreateOptions{}) + require.NoError(t, err) + + _, rollback, err := store.Claim(ctx) + require.NoError(t, err) + + // Simulate another worker taking over the same job name after our claim. + taken, err := fakeClient.Jobs("stacks-123").Get(ctx, "test-job", metav1.GetOptions{}) + require.NoError(t, err) + taken.Labels[LabelJobClaimOwner] = "another-worker" + _, err = fakeClient.Jobs("stacks-123").Update(ctx, taken, metav1.UpdateOptions{}) + require.NoError(t, err) + + // Rolling back our (now lost) claim must not disturb the other worker's job. + rollback() + + after, err := fakeClient.Jobs("stacks-123").Get(ctx, "test-job", metav1.GetOptions{}) + require.NoError(t, err) + assert.Equal(t, "another-worker", after.Labels[LabelJobClaimOwner], "rollback must not strip another worker's claim") + assert.NotEmpty(t, after.Labels[LabelJobClaim], "rollback must not clear the active claim timestamp") +} + +// TestRenewLease_LostToAnotherOwner verifies that a worker cannot renew a claim +// that now belongs to a different owner (same job name, different owner token). +// This is the core protection against two workers processing the same job: once +// a job is reaped and re-claimed by another worker, the original worker's renewal +// must fail with ErrLeaseLost so it aborts instead of continuing to run. +func TestRenewLease_LostToAnotherOwner(t *testing.T) { + fakeClient := newTestClientset() + store := newTestStore(fakeClient) + + ctx, _, err := identity.WithProvisioningIdentity(context.Background(), "stacks-123") + require.NoError(t, err) + + // The store's job is claimed by owner-B. + created, err := fakeClient.Jobs("stacks-123").Create(ctx, &provisioning.Job{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-job", + Namespace: "stacks-123", + UID: "uid-1", + Labels: map[string]string{ + LabelJobClaim: "1000000000000", + LabelJobClaimOwner: "owner-B", + }, + }, + Spec: provisioning.JobSpec{Repository: "test-repo", Action: provisioning.JobActionPull}, + }, metav1.CreateOptions{}) + require.NoError(t, err) + + // We hold a claim with owner-A on the same job name/UID. + ours := created.DeepCopy() + ours.Labels[LabelJobClaimOwner] = "owner-A" + + err = store.RenewLease(ctx, ours) + require.Error(t, err) + assert.True(t, errors.Is(err, ErrLeaseLost), "expected ErrLeaseLost, got %v", err) +} + +// TestRenewLease_LostToReincarnatedJob verifies that a matching owner token is not +// enough: if the object was deleted and re-created under the same name (new UID), +// renewal must still fail. +func TestRenewLease_LostToReincarnatedJob(t *testing ... [truncated]