docs: Proposal: Source Integrity Policies#25148
Conversation
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
c256bac to
0703d13
Compare
|
I like the fact that the source verification is extensible - I've been thinking about the best way to incorporate source checking for OCI repositories, and this seems extensible enough to allow for that in the future. From a practical standpoint, how and where is this meant to be implemented? It seems the place for this would be the repo-server, where we would pass along the required attributes. Are these checks supposed to happen as a pre-cursor before running |
Do you have more details about this use case? Not to incorporate it, I just want to make sure the API design is versatile enough to support that in the future.
As I play with the implementation right now - yes, it sends the criteria to repo-server and validation results back for server to let the sync fail depending on the result. Future strategies, can do the validation there as well, but they are not required too.
Not sure yet. I intend to verify where gpg used to. Though, and this only maybe answers your question, if we send the entire criteria to repo-server, we can make the decision about (not) generating manifests and sending them back there. Although, it is going to save time and bandwidth only in error cases, so it will likely not be a huge win. |
I don't have that much detail at this time; this isn't really something I've given too much thought. The gist of it is that when we pull an OCI Image it would be good to verify the image layers by its digest. There are a few ways to do this, the most likely one I'd probably start with would be to use Sigstore to verify the digest of the downloaded OCI artifact image. An example can be found here (the example has a lot of knobs and can be simplified for our use case) From the perspective of your proposal I'd probably extend it in the style of apiVersion: argoproj.io/v1alpha1
kind: AppProject
spec:
sourceIntegrity:
oci:
policies:
- repos:
- "oci://my-docker-registry/foo/*"
sigstore:
# TODO: Add a ton of sigstore attributes |
|
@blakepettersson: Yeah, that definitely sounds implementable... |
0703d13 to
d0c5782
Compare
|
There will be some penalty. I have tested it with this repository of 10K commits, and it took about 20 seconds to verify them all (on SSD). I was quite impressed by git scalability to be honest, but I see this can be annoying. Note it only affects the most secure of the modes, where we can expect people to be ready to pay the price in exchange for the increased security. AND, they can very likely do a performance optimization by creating a seal commit (but I have not verified that it will help, and how much).
Every commit. We can think of relaxing this in the future, but I am concerned about cross-dir references, so sources from different path can influence the resulting manifests, without being checked for integrity.
It would be more secure than nothing, but trivially avoidable for motivated attacker 🤷. The great news this can be easily added after this refactor. |
|
@jannfis, @pasha-codefresh, @blakepettersson. thanks for helping me to drive this proposal thus far! There are a number of open questions that I would like to get opinions on.
sourceIntegrity:
git:
policies:
- repos:
- "https://github.com/foo/*"
# other policy attributesto sourceIntegrity:
git:
policies:
- for:
repo: "https://github.com/foo/*"
branch: "master" # This may be implemented when the need arise, but not with the current API design.
# other policy attributes
|
ebe217e to
6b786fb
Compare
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
No changes to the proposal itself. Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
- Drop progressive verification level - Change declaration syntax - Introduce sealing Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
6b786fb to
b62061f
Compare
| There needs to be a visual indicator in Argo CD CLI and UI, pointing out project repositories that do not perform GPG verification. | ||
| This is to provide an administrator with feedback on the repository matching evaluation. | ||
|
|
||
| ### Security Considerations |
There was a problem hiding this comment.
The issue of a compromised git sever or a malicious git admin should also be considered - one of the main benefits of commit signing is an extra layer of protection over existing policies in the git server of only allowing certain users to make changes etc.
The git server could perform downgrade attacks - if the ArgoCD Application is configured to deploy tag v1.1 (or HEAD), the git server could change this to point to a different (older) commit, deploying a previous version of the application that may contain known security vulnerabilities. Should ArgoCD validate that when a tag or HEAD changes, the new commit is a descendent of the previous? This may break force pushes - would require a "sealing commit"
There was a problem hiding this comment.
Compromised git server is a great scenario to showcase the value of commit signing!
When changing the target revision backwards in git history by either changing App definition, git force push or as you suggested git server forging where refs point to is something that requires thorough thought. Maybe even Argo CD rollback falls into the same category.
- I am not convinced that preventing a version with security vulnerability to be synced is something Source Integrity should address. Past commits, even when once successfully syced and run, can become obsoleted by the sole passage of time for a number of other reasons (bugfixes, compatibility with external systems that have upgraded in the meantime, legislation evolving, etc.) So as long as the commit, or the history, is correctly signed, Argo CD would sync it.
- A commit once trusted, can become invalid through key revocation, expiration or its removal from either Argo CD keyring or AppProject's declaration. So they will be prevented from syncing, which I believe is a good thing - the commits would not be considered valid if appeared on tip of the git history. The fact that given Argo CD instance used to trust them at some point, is irrelevant IMO.
Should ArgoCD validate that when a tag or HEAD changes, the new commit is a descendent of the previous?
I see how this would prevent this kind of problem (even for sources like helm or OCI) - the compromised repository serving old artifacts (commit, image, chart) pretending it is a new one. However, I see a number of legitimate use-cases that desire to deploy older version / commit that we want to preserve. Besides, for git, the effective commit might not be a descendant nor an ancestor of the previous one (force push removed previous-commit, or when jumping from branch to branch).
There was a problem hiding this comment.
@jackevans43, it seems this cannot happen when application points to commit sha or signed tag. In both cases, an attacker cannot change the commit without resigning it, or forge the signature on commits not signed by legitimate key holder.
While for HEAD or branch name, the server can maliciously server "shortened" history resembling force pushing have happened without us being able to tell...
There was a problem hiding this comment.
IMO the value of commit signing is to take the git server out of the TCB of a system. They're often complex beasts that need to be patched frequently to stay secure, and often run by another team or SaaS you might not have full visibility into their security posture (aside from a long list of compliance checkboxes).
If I could reduce my TCB to only need to trust developers, CI and the deployment environment (ArgoCD plus the Kubernetes cluster) that would be an improvement. Using Kubernetes RBAC I can control who can change the git ref of an Application, but I don't expect the git server to have free reign to do the same.
I agree that past commits are obsoleted by the passage a of time for a number of reasons, but preventing systems from being downgraded to allow an attacker to take advantage of previous versions when vulnerabilities in those versions are discovered is a common security control in e.g. mobile OS, Windows, UEFI etc. Sadly it's often used against the consumer, to prevent them downgrading to older versions so they can root their devices, but in this case we'd be using it for good!
An example - if a git admin was compromised, or there was an RCE in the git server - they cold look for apps using React Server (CVE-2025-55182), then change the tag of that app to point to a commit from the end of November. ArgoCD would then dutifully roll out that version, and the attacker would compromise the app. Once they have persistence they could swap the tag back again.
There was a problem hiding this comment.
@olivergondza I think ArgoCD currently trusts both signed and unsigned (lightweight) tags - so the git server could remove the signed tag and replace it with an unsigned one pointing to an older commit. If there was a flag to only allow signed tags (or maybe strict mode enables that), I think that would mitigate the issue when using tags 😄 (but not branches/HEAD)
There was a problem hiding this comment.
Good point that neither old nor new implementation would detect when signed tag is replaced by a lightweight one. 👍
There was a problem hiding this comment.
... so the git server could remove the signed tag and replace it with an unsigned one pointing to an older commit.
As I just tested, this can be done by using branch name that would collide with signed tag. So to force your argo to rollback your app from v2.0 to v1.0, all I need to do as a malicious admin is to create new branch named v2.0 and point it to you the commit where v1.0 is pointing. If the tag on v1.0 has a signature still considered valid, it will sync (old and new impl).
I do like this vector you have described. To address this, we would need a way to tell "the target revision must be a signed tag". I do not think we would need the inverse for HEAD or branch name, as they are the less secure variants.
There was a problem hiding this comment.
Yep, agree on the usefulness of (optionally) not allowing lightweight tags.
Yeah, agree. It's better to anticipate future potential API changes. Frankly, I find the |
That's a pretty valid concern here. I guess the earlier we ensure the integrity of the sources, the better. |
Hm. How would that look like? Do we have the AppSet controller to read the AppProject, and then supply the configuration to repository server? Anyway, I think it does make sense to make AppSet Git generator aware of SIP and have it enforce it. I'm not sure we need that in the first iteration of the feature. To me, it looks like an implementation detail instead of an architectural decision. |
Hm, not sure I understand. A simple tag usually points to a certain commit, and that commit is either signed or unsigned. It's true that a tag is not immutable, and could point to commit A today and to commit B tomorrow. But when that commit isn't signed, the policy would fail. What did I miss? EDIT: Ah, this comes from another discussion. OK, apparently I missed something. Will continue on that discussion. |
I am not sure what would it take to make AppSets part of projects, so I am not pursuing that option. One way would be to introduce apiVersion: argoproj.io/v1alpha1
kind: ApplicationSet
spec:
generators:
- git:
repoURL: https://github.com/argoproj/argo-cd.git
revision: HEAD
directories:
- path: applicationset/examples/git-generator-directory/cluster-addons/*
template:
metadata:
name: '{{.path.basename}}'
spec:
project: "my-app-project" # If templated, no source integrity enforcement for git generator - surprising for userswould become: apiVersion: argoproj.io/v1alpha1
kind: ApplicationSet
spec:
generators:
- git:
repoURL: https://github.com/argoproj/argo-cd.git
revision: HEAD
directories:
- path: applicationset/examples/git-generator-directory/cluster-addons/*
sourceIntegrity:
fromProject: "generator-project"
template:
metadata:
name: '{{.path.basename}}'
spec:
project: "my-app-project" # Can be templated without source integrity impactThe advantage is that:
|
Looks reasonable to me. However, I would strongly suggest to make this a separate change and only touch briefly upon this on the proposal. |
We can do that. Though what to do with the current logic (using
|
|
Could another option be to have it as a global option? Like having a Appsets with a hardcoded project could do the same logic as we do with credentials; attempt to look up a project-based source integrity, and if that's not available fallback to the global default (if present) |
|
@blakepettersson, it would be my preference to have a central config, not a per-project one. But due to a complexity of the config, I do not see a way to declare that inside a ConfigMap. |
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
|
Are there any other comments for this proposal? Any thoughts on #25148 (comment)? |
As per the discussion on the contributors call, this will be postponed, since the fallback to {{AppProject}}'s legacy Signing Keys, needs to be preserved either way. |
|
Currently, there are no further open questions that would not be answered. |
Signed-off-by: Oliver Gondža <ogondza@gmail.com> Co-authored-by: jannfis <jann@mistrust.net>
This is a 2.0 for an existing proposal proposed by @jannfis a while ago: #14964
I would like to receive some initial thumbs up/down on the general direction and eventual mergability for the RFE. The details might evolve as I will work on the PoC implementation...
TL;DR
Abstract current GIT+GPG verification by something more powerful:
AppProject, replace list.spec.signatureKeys([]string), by a struct at.spec.sourceIntegrity. Struct permits adding future fields to support other sources than git, and other methods of verification that GPG.WIP Implementation: #25371
Example
Changes from the original proposal by @jannfis
progressivestrategy is removed as suggested in the original proposal, as it was responsible for large part of the risks and corner cases.Checklist: