docs: [Proposal] Source Verification Policies #14964
Conversation
Signed-off-by: jannfis <jann@mistrust.net>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #14964 +/- ##
=======================================
Coverage 49.91% 49.91%
=======================================
Files 262 262
Lines 45002 45002
=======================================
Hits 22464 22464
Misses 20329 20329
Partials 2209 2209 ☔ View full report in Codecov by Sentry. |
|
|
||
| So as it seems from that explanation of the impact of verification level, a newly Application could only ever be synced with a verification level of `head`. | ||
|
|
||
| ### Detailed examples |
There was a problem hiding this comment.
What would happen with a multi-source application? Would we sync just one source while not syncing the source that fails the verification level check? Or will we sync the application only if all the sources succeeded the verification level checks?
There was a problem hiding this comment.
If any policy fails validation, the application will not be synced. One advantage of the new verification policies is that you can define a unique policy on a per-source basis. If one of the sources of a multi-source application is not covered by a policy, it will be automatically a "pass".
|
|
||
| Also, the current approach is an "all-or-nothing" approach for any given AppProject. The reality however is, that different repositories might have different trust levels, and different people working on them and being trusted. Especially with the advent of multi-source applications, the current approach of defining the trusted signers for all Applications in a given AppProject is not viable anymore. | ||
|
|
||
| ### Goals |
There was a problem hiding this comment.
Should there be an additional goal of support GPG in ApplicationSet git generator?
There was a problem hiding this comment.
IMHO, ApplicationSet is out of scope for this proposal. ApplicationSets are not bound to AppProjects afaik.
| * `progressive`: This verification level will verify all commits between the application's target revision and the revision it was last synced to, but not beyond that. The revision that the application is currently synced to does not necessarily need to be signed, so that a migration path from level `head` to the more secure method `progressive` is possible. However, if the application hasn't synced before (i.e. there is no previous revision in the application's sync history), the `progressive` level will inhibit the same behaviour as the `strict` verification level (see below). | ||
| * `strict`: This verification level will always verify all commits in the history of the repository that led to the application's target revision. The `strict` level effectively enforces that all commits in your repository need to be signed from the beginning of your repository's history, without allowing _any_ unsigned commit in the history. This is the most secure policy. | ||
|
|
||
| For the `strict` and `progressive` verification levels, if the target revision is an annotated tag, then that tag's signature will also be verified in addition to the specific commits verified as defined by the respective level. |
There was a problem hiding this comment.
Currently either a signed tag or a lightweight tag are accepted. In these modes, should only signed tags be allowed? A malicious user, or git server, could move lightweight tags to an older commit to perform a downgrade attack (e.g. deploy and older version of an app with known vulnerabilities)
There was a problem hiding this comment.
Hm, that's a good catch. I haven't thought much about this scenario. However, there will be another attack vector here, which is the Application CR. The same downgrade attack would be possible by just modifying the source in the Application to resolve a previous tag. But yeah, I agree, that this would add up to a defense-in-depth principle.
What do you think about making this another option for a policy? For example, enforceSignedTag: <bool> or allowSignedTagsOnly: <bool>?
There was a problem hiding this comment.
Yes, I think an additional option to force tags to be signed would solve the issue.
I was worried about something going bad in git - I think if a malicious user has access to the Application CR then it's probably game over anyway?
There was a problem hiding this comment.
OK, basically, someone with write access to the Application CR can modify the targetRevision to sync against.
The scope of the verification policies is to only allow sync against previously vetted (e.g. signed) revisions. To my knowledge, neither PGP nor Git knows about the concept of signature revocation, except when revoking the private key used to make the signature. I think preventing downgrade attacks becomes rather futile on this level.
But let's keep thinking about it.
There was a problem hiding this comment.
My concern was not really signature revocation, it was more what mischief a malicious git server could achieve. When the Application is tracking a branch it should only accept revisions that are descendants of the currently synced revision and if it's tracking a tag it should be a signed tag, both to prevent the git server moving back to a previous (but signed) vulnerable commit. If a user has the ability to change the Application CR, then they can legitimately point targetRevision where they like, as long as it's signed
|
|
||
| * `none`: As the name implies, verification will be disabled at this verification level. | ||
| * `head`: This verification level will verify the commit that is pointed to by the HEAD of the target revision. In case target revision is an annotated tag, only the signature of the tag will be verified. | ||
| * `progressive`: This verification level will verify all commits between the application's target revision and the revision it was last synced to, but not beyond that. The revision that the application is currently synced to does not necessarily need to be signed, so that a migration path from level `head` to the more secure method `progressive` is possible. However, if the application hasn't synced before (i.e. there is no previous revision in the application's sync history), the `progressive` level will inhibit the same behaviour as the `strict` verification level (see below). |
There was a problem hiding this comment.
Should it be an explicit design goal to prevent downgrade attacks? e.g. a malicious user or git server could present HEAD as an older (signed) commit, pointing to an older, vulnerable version of the app? When syncing, argocd should only move to commits that are a descendent of the currently synced commit?
There was a problem hiding this comment.
Now I'm wondering if a malicious user in Git could modify the ancestry tree without affecting valid signatures on commits.
The strict mode already checks the ancestry (i.e. only moves forward, not backwards). However, an attacker that has access to both, the Application CR and control over the Git server could potentially work around that.
There was a problem hiding this comment.
No, I think because the git signature covers both the contents of the commit and the digest of the parent commit, it's not possible to move commits around and retain a valid signature.
I think it's game over if an attacker can modify the Application CR or ArgoCD config in general. But I think that's fine - it would be nice to only have to trust ArgoCD and the cluster it runs on, but not the git server.
There was a problem hiding this comment.
I think it's game over if an attacker can modify the Application CR
Can you please elaborate a little on this? The Application CR (as opposed to the AppProject CR) is eventually a user-modifiable resource, and a modification to it must not affect the security of the system.
There was a problem hiding this comment.
Sorry I wasn't very clear. If a user has a signing key, write access to any branch of a repo on the AppProject allowlist, and write access to the Application CR, they can get ArgoCD to deploy arbitrary resources (within the bounds of what the "best" AppProject allows as they can choose which one to be bound by). If people don't want that happen, I think the only solution is separation of duties so nobody has all 3 roles (or careful push rules on the git repos)
|
|
||
| #### Verification levels | ||
|
|
||
| Each source verification policy currently knows about four levels of verification: |
There was a problem hiding this comment.
Could there be an additional mode that restricts ArgoCD to deploying git refs that are digests - i.e. some external system is doing verification, so it's unnecessary for ArgoCD to do any validation, apart from preventing mistakes by restricting to digests?
There was a problem hiding this comment.
Thinking about this, and about my answer in #14964 (comment) (reading and answering top to bottom here 😅 ), maybe what I mentioned could be amended as allowedTargets: <type>, where <type> is one of st (signed tag) or digest (commit digest).
| ## Open Questions [optional] | ||
|
|
||
| * Do we really need `progressive` verification level (see below)? | ||
| * Should repository pattern matching be regexp based instead of shell-glob? |
There was a problem hiding this comment.
Most stuff in AppProject is glob. And I think source templates are also defined with globs. So I'd favor that strategy until we have a strong reason to move to regex.
| A source verification policy has the following properties: | ||
|
|
||
| * A repository pattern (shell-glob type) that is matched against the URL of the source (mandatory) | ||
| * A repository type that is matched against the application's repository type (mandatory) |
There was a problem hiding this comment.
Does this field need to be added to the list above?
| * A repository pattern (shell-glob type) that is matched against the URL of the source (mandatory) | ||
| * A repository type that is matched against the application's repository type (mandatory) | ||
| * A verification level that describes with which level the verification should be performed (mandatory) | ||
| * A verification methods that describes how to the source should be verified (mandatory) | ||
| * A list of identities of trusted signers (optional, if omitted, any known signer is trusted) |
There was a problem hiding this comment.
I feel like this list is a duplicate of the one above - could probably merge them.
| * A verification methods that describes how to the source should be verified (mandatory) | ||
| * A list of identities of trusted signers (optional, if omitted, any known signer is trusted) | ||
|
|
||
| Additionally, there are some optional properties you can set for a policy that depend on the verification level, method and respository type. |
There was a problem hiding this comment.
The list above already contains an optional field... are there more optional fields which are not listed here?
There was a problem hiding this comment.
For now, we only have this one optional property. This could change in the future when there are more supported verification methods, or when Argo CD supports additional repository types.
|
|
||
| 2. The `progressive` level has a "bootstrap" feature to allow creating an `Application` in `progressive` level. Obviously, when an `Application` is created, it does not have any history information unless it is synced initially. The bootstrap feature defines a time window, the `bootstrapPeriod`, in which an `Application` without history can temporarily sync using the `head` level (i.e. only with the HEAD of `targetRevision` being signed). The bootstrap feature assumes that the `.metadata.creationTimestamp` of the `Application` resource is immutable, or rather be reset by the Kubernetes API server upon modification. During the timeframe in the `bootstrapPeriod`, an adversary could delete the `Application`'s history and sync to any signed commit in the repository. | ||
|
|
||
| 3. The `progressive` level doesn't allow roll-backs, as it verifies the commits between the previous synced state and the target revision. If the application is on `F`, and wants to roll back to `C`, it cannot find any revision history for this path (because `C` existed prior to `F`). This must be properly documented. For `head` and `full`, this is not a concern as long as the level's requirements are met in the repository. |
There was a problem hiding this comment.
It sounds like there are several downsides to progressive. Should we document the migration path from progressive to strict? i.e. should we be asking people to create a new orphan branch, copy over contents, and sign all commits on a the new branch so that they can move to strict?
| - # taken from .spec.signatureKeys | ||
| ``` | ||
|
|
||
| If a user wants to migrate from the current implementation to the new source verification policies, they will first have to remove `.spec.signatureKeys` and can then go ahead and define desired policies in `.spec.sourceVerificationPolicies`. |
There was a problem hiding this comment.
What's the behavior if both fields are set at the same time?
Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Signed-off-by: Jann Fischer <jann@mistrust.net>
Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Signed-off-by: Jann Fischer <jann@mistrust.net>
Proposal for Source Verification Policies.
Note on DCO:
If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.
Checklist:
Please see Contribution FAQs if you have questions about your pull-request.