Skip to content

Add forward-only verification for GitRepository - #2188

Draft
ruslan-shaydullin wants to merge 3 commits into
fluxcd:mainfrom
ruslan-shaydullin:forward-only-verify-slim
Draft

ruslan-shaydullin wants to merge 3 commits into
fluxcd:mainfrom
ruslan-shaydullin:forward-only-verify-slim

Conversation

@ruslan-shaydullin

Copy link
Copy Markdown
Contributor

Depends on fluxcd/pkg#1329. The last commit points the git module at the fork that carries CloneConfig.Depth so that CI builds; it is dropped once the module is released and bumped. Draft until then.

Fixes #2010.

With .spec.verify set, the controller verifies the signature of the HEAD and/or tag object and nothing else. Anyone with push access but without a trusted signing key can point the branch at an older commit that already carries a valid signature:

git reset --hard <old signed commit> && git push --force origin main

On the next interval the GitRepository is Ready at the old revision and the artifact is rebuilt from it. The issue also describes the second step, rolling the Secret with the trusted keys back to a version where the attacker's key was still listed. A tag moved to an older commit behaves the same way.

This adds .spec.verify.forwardOnly. When set, a revision that passes signature verification is accepted only if it descends from the last verified revision, which the controller now records in .status.sourceVerificationRevision on every successful verification, with or without the option. After the change the same push gives Ready=False and SourceVerified=False with reason NonFastForwardRevision; the message names the rejected and the last verified revision, artifact and baseline stay as they were, and kustomize-controller keeps applying the last verified artifact.

How the history is obtained. With a baseline recorded, the controller clones the last 100 commits of the reference instead of the tip and walks the parents breadth first, so that a shallow boundary on one chain of a merge does not hide the baseline on the other (Commit.IsAncestor of go-git stops at the first missing parent, go-git#2409). If the baseline is not within reach, the temporary directory is wiped and the reference is cloned again with 500 commits; beyond that the revision is rejected. The signature is verified before the history is inspected, so an unsigned HEAD never costs a second clone, and it is verified again after the second clone if the remote moved in between. A fetch into the existing clone was ruled out by the rule in client.go of fluxcd/pkg and by fluxcd/pkg#1289; a full clone was ruled out because a reference pointed at an old commit would then cost a full clone on every retry. The two depths are constants, not API, sized from measurements of go-git on argoproj/argo-cd (below). The variants in the analysis by @bb-Ricardo in #2010, a configurable depth next to the boolean or a blobless partial clone, were the starting point; partial clones are not available in go-git v5, and the depth stays internal until there is a reason to expose it.

The baseline lives in a status field of its own rather than in artifact.revision because reconcileStorage clears status.artifact when the artifact is missing from storage, and the protection must not disappear at that moment. An object with the option enabled and no baseline yet is verified once in full to establish it; for objects verified before this change sourceVerificationFingerprint is empty too, so the first reconciliation after the upgrade is a full one anyway. The first baseline is announced with an event. While the remote reference still points at a rejected revision, retries repeat the rejection without inspecting the history again.

There is one rule and one way around it. A branch switch to a non-descendant, a commit pinned backwards, a tag moved backwards and a rebased refs/pull/* reference are all rejected the same way and all accepted the same way: by a forced reconciliation with reconcile.fluxcd.io/forceAt set together with requestedAt, which verifies the revision on the remote at that moment and makes it the new baseline. meta.ForceRequestStatus is embedded in the status for that, as in HelmRelease. Pinning .spec.ref.commit does not work for a flux-system GitRepository that is itself applied from the rejected history, and a kubectl patch would be reverted by kustomize-controller. A legitimate rollback with git revert produces a signed descendant and needs nothing.

Not in this PR: .spec.ref.semver (release tags cut from release branches are frequently not descendants of each other; the combination stalls the object with InvalidVerificationMode), a --force flag for flux reconcile source git, a warn-only mode, a depth field in the API. The option does not replace code review and does not protect against a compromised trusted key: any signed descendant is accepted, including a work-in-progress commit of a trusted author that someone fast-forwarded the branch to, and the docs say so. Branch protection on the hosting side does not cover tags on every host, self-hosted servers or a compromised hosting account; .spec.verify already assumes an untrusted Git server, and this option completes that model.

Tests: 31 scenarios in TestGitRepositoryReconciler_reconcileSource_forwardOnly (the rollback accepted with the option off, rejected with it on, the rejection repeated without a clone, the return to the verified revision, signed and unsigned descendants, a merge with the baseline on either parent, a baseline 150 commits back found by the second clone, an unsigned HEAD that costs no second clone, a baseline 501 commits back rejected, a missing artifact, the first baseline and its bootstrap, a branch switch with and without the force request and the request handled once, a tag moved backwards and forwards, a pinned commit backwards and forwards, a pull request reference, an invalid baseline, semver), unit tests of the history walk on an in-memory repository, and new cases in the signature and policy tests. make test passes locally apart from a pre-existing resolver failure in internal/oci/notation on my machine.

Measurements on argoproj/argo-cd (11.7k commits, go-git v5.19.3, M4 Pro)
clone time peak RSS .git
depth 1 4.9 s 0.75 GiB 30 MB
depth 100 6.4 s 0.74 to 0.96 GiB 55 MB
depth 500 16 to 17 s 1.6 to 1.8 GiB 62 MB
depth 1000 15.9 s 2.1 to 2.2 GiB 82 MB
full 25.2 s 1.1 GiB 174 MB

A full reconciliation with the option on: baseline 5 commits behind, 6.1 s and one clone; 150 behind, 22 s and two clones. On configuration repositories the difference is not measurable. The memory of a shallow clone in go-git grows with the depth and exceeds that of a full clone from about 150 on, which is why the second depth is 500 and not larger.

Signature verification checks the HEAD and/or tag object of the
checked out revision and nothing else. Anyone with push access can
point a branch or tag at an older commit that carries a valid
signature of a trusted author, and the controller builds an Artifact
from it.

Add .spec.verify.forwardOnly, which requires a new revision to descend
from the last verified one, recorded in the new
.status.sourceVerificationRevision. Embed meta.ForceRequestStatus so
that a revision that does not descend from the baseline can be
accepted deliberately with reconcile.fluxcd.io/forceAt, and add the
accessors meta.ShouldHandleForceRequest needs. Document the field, the
baseline, the history inspection and the force request in the spec.

Signed-off-by: Ruslan Shaydullin <shaydullin.r.d@outlook.com>
With forwardOnly set and a baseline recorded, clone the last 100
commits of the reference instead of only its tip and search them for
the baseline, walking the parents breadth-first so that a shallow
boundary on one chain of a merge does not hide the baseline on the
other. If the baseline is not within reach, discard the clone and
clone the last 500 commits into a fresh directory; a baseline further
away is rejected. The signature is verified before the history is
inspected, and again after the second clone if the remote HEAD moved.

A rejected revision sets SourceVerified=False with reason
NonFastForwardRevision, leaves the Artifact and the baseline untouched,
and is cached per object so that retries repeat the rejection without
inspecting the history while the remote reference, the object and the
baseline are unchanged. A force request skips the check and makes the
verified revision the new baseline. The baseline is recorded on every
successful verification; an object with forwardOnly and no baseline is
verified in full once to establish it. Semver references are not
supported with forwardOnly and stall the object.

The history is obtained with a bounded clone rather than a fetch into
the existing clone, which the Git client does not support reliably on
all providers, and rather than a full clone, which large repositories
cannot afford on every rejected attempt.

Signed-off-by: Ruslan Shaydullin <shaydullin.r.d@outlook.com>
Temporary: point the git module at the fork that carries
CloneConfig.Depth so that CI builds until the module is released and
the dependency is bumped. To be dropped before merge.

Signed-off-by: Ruslan Shaydullin <shaydullin.r.d@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature Request: add "forward only" option for GitRepository with signed commits

1 participant