-
Notifications
You must be signed in to change notification settings - Fork 5.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for verification of GPG commit signatures #2492
Comments
@alexmt Is that something we should still work on, or is it something we will get from gitops-engine collaboration with Flux? |
Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
This feature would be great for hardened security! |
This would be really useful for us too- our intended use case is to deploy only on commits done by pre-verified people |
Just for the record, I've started working on an implementation. Hope to have it finished for the next release after v1.5. |
@jannfis awesome!! Just an idea for a more extendible approach (I'm not sure how the argo internals work so this may need to be adapted a bit): Add an "approved: false" label to the Application (or whatever the resource is that actually has the new commit sha) resource that is set to false whenever a new release is created. Now we can add our own approval workflow (through controllers), eg. one that checks for a git signature, one that checks for multisig on tags with our own custom implementation etc. etc. |
@toonsevrin Although I like the idea behind your suggestion, I'd guess it would be a whole different tier than what I had in mind for this enhancement. In the first throw, I'd like to implement syncing an application only against Git commits that where signed with a key of a configurable range of allowed keys and implement basic key management via CLI/UI. Your suggestion goes a little further, as to an approval for a sync from outside. Basically, I could imagine that there'll be a new field in the application CRD, which is used to specify whether approval is required and if yes, what kind (for example, single approval, n-of-m approval etc). Then, from the outside, you could modify the status of an application CR (i.e. This could be used in combination with GPG commit signature, because the approvers could be sure that the commit is comming from a trusted source. Or it can be used on its own. I'd be happy to discuss this further, because it could satisfy some requirements that may exist especially in heavily regulated corps. |
Hi @jannfis, you're right. Let's focus on the high value, low effort for now. The more advanced case could be implemented once there is interest for this. I don't think you have to cryptographically sign the approvals percé. I'm not sure how granular we can go with kubernetes RBAC, but RBAC could probably just be used to protect the approval flow. Also, regarding the usage in combination. The commit signature checker should just be another approver. Even if the repository can trick the other approvers with fake commits, the deployment will still fail due to this approver not approving it. Thanks for implementing this btw. |
Oh also: If there are any organizations that may need this behavior in the mean time: You can implement it yourself by always specifying the commit sha in the revision field of your Application. The commit sha acts like a checksum of your code so automatically verifies integrity. Then you can write your own approver (a controller application) that tracks your git repository, and whenever a tag or commit is made, it checks your approval workflow. If the commit is approved, you update the revision field to the commit sha of the tag/commit. Note: you will want to lock down (through rbac) argo so that this controller is the only agent that can edit the revision. |
Another argument against the advanced functionality: If you are a large organization you can create a service that signs tags for you (with a private key that is generated once by the service itself). You can then add all sorts of requirements to the signature of this tag. Once @jannfis finishes his implementation of signature validation, you simply add the public key of the service to argocd. If one of my organizations ever needs this level of security, we will open-source our implementation here. Conclusion: Let's not add the advanced modular approval functionality to the core of argocd. |
As a quick heads-up: I'm making good progress on this. So far, I've implemented all the primitives required for key management and signature validation as well as adaptions to the repository server and application controller as to enforce signature verification and handling according to its results. Currently, to enable signature verification on a project level, one has to specify a list of valid (trusted) PGP key IDs in the project spec, like so: spec:
signatureKeys:
- 4AEE18F83AFDEB23
- ... The key(s) specified must be in the GnuPG key ring known to ArgoCD (next step is to implement managing the keyring in a basic way using CLI, UI and API), otherwise signature verification will not succeed. Now, if signature verification is enabled, the application controller will instruct the repository server to run a signature verification on the revision it wants to compare the live state to, and the repository server will then run a The application controller will still use any non-signed commit for the comparison, and also will display the differences from a possible untrusted source. This was a design decision I made for no specific reason, but I'd like to hear opinions on that - it would also be possible to emit an error on the sync status and push the application into "unknown" state. @toonsevrin I like the approach of an external entity performing the final signature once you need n-of-m or more complex form of authorization. I'm looking forward to see your project coming to life! |
Awesome!! Quick note, please make the Spec spec:
signatureKeys:
- key: 4AEE18F83AFDEB23
- key: ... to follow the kubernetes CRD guidelines. This is more future-proof (eg. if you want to add a name or expiry field or whatever in the future). Regarding your note on the state with an untrusted source: OutOfSync is fine with me if that's what you are referring to. The most likely case when you are in this state is when you forgot to sign your commit/tag, which makes the resource permanently out of sync with your desired state (the unsigned commit). Anyways, this is indeed very subjective but not too important to me, as long as I can somehow find out that my resource is in a state where the signature of the latest commit is wrong. And thanks! As I am the only developer and operator in most of my companies I doubt that will be very soon though 👍 |
Thanks for this suggestion @toonsevrin, it makes total sense. Will change the spec and adapt the code so that keys are specified as array of maps, instead as array of strings. |
@jannfis array of objects*** ;) that way you can do better openapi validation |
* Add initial primitives and tests for GPG related operations * More tests and test documentation * Move gpg primitives to own module * Add initial primitives for running git verify-commit and tests * Improve and better comment test * Implement VerifyCommitSignature() primitive for metrics wrapper * More commentary * Make reposerver verify gpg signatures when generating manifests * Make signature validation optional * Forbid use of local manifests when signature verification is enabled * Introduce new signatureKeys field in project CRD * Initial support for only syncing against signed revisions * Updates to GnuPG primitives and more test cases * Move signature verification to correct place and add tests * Add signature verification result to revision metadata and display it in UI * Add more primitives and move out some stuff to common module * Add more testdata * Add key management primitives to ArgoDB * Move type GnuPGPublicKey to appsv1 package * Add const ArgoCDGPGKeysConfigMapName * Handle key operations with appsv1.GnuPGPublicKey * Add initial API for managing GPG keys * Remove deprecated code * Add primitives for adding public keys to configuration * Change semantics of ValidateGPGKeys to return more key information * Add key import functionality to public key API * Fix code quirks reported by linter * More code quirks fixes * Fix test * Add primitives for deleting keys from configuration * Add delete key operation to API and CLI * Cosmetics * Implement logic to sync configuration to keyring in repo-server * Add IsGPGEnabled() primitive and also update trustdb on ownertrust changes * Use gpg.IsGPGEnabled() instead of custom test * Remove all keyring manipulating methods from DB * Cosmetics/comments * Require grpc methods from argoproj pkg * Enable setting config path via ARGOCD_GPG_DATA_PATH * Allow "no" and any cases in ARGOCD_GPG_ENABLED * Enable GPG feature on start and start-e2e and set required environment * Cosmetics/comments * Cosmetics and commentary * Update API documentation * Fix comment * Only run GPG related operations if GPG is enabled * Allow setting ARGOCD_GPG_ENABLE from the environment * Create GPG ConfigMap resource during installation * Use function instead of constant to get the watcher path * Re-watch source path in case it gets recreated. Also, error on finish * Add End-to-End tests for GPG commit verification * Introduce SignatureKey type for AppProject CRD * Fix merge error from previous commit * Adapt test for additional manifest (argocd-gpg-keys-cm.yaml) * Fix linter issues * Adapt CircleCI configuration to enable running tests * Add wrapper scripts for git and gpg * Sigh. * Display gpg version in CircleCI * Install gnupg2 and link it to gpg in CI * Try to install gnupg2 in CircleCI image * More CircleCI tweaks * # This is a combination of 10 commits. # This is the 1st commit message: Containerize tests - test cycle # This is the commit message #2: adapt working directory # This is the commit message #3: Build before running tests (so we might have a cache) # This is the commit message #4: Test limiting parallelism # This is the commit message #5: Remove unbound variable # This is the commit message #6: Decrease parallelism to find out limit # This is the commit message #7: Use correct flag # This is the commit message #8: Update Docker image # This is the commit message #9: Remove build phase and increase parallelism # This is the commit message #10: Further increase parallelism * Dockerize toolchain * Add new targets to Makefile * Codegen * Properly handle permissions for E2E tests * Remove gnupg2 installation from CircleCI configuration * Limit parallelism of build * Fix Yarn lint * Retrigger CI for possible flaky test * Codegen * Remove duplicate target in Makefile * Pull in pager from dep ensure -v * Adapt to gitops-engine changes and codegen * Use new health package for health status constants * Add GPG methods to ArgoDB mock module * Fix possible nil pointer dereference * Fix linter issue in imports * Introduce RBAC resource type 'gpgkeys' and adapt policies * Use ARGOCD_GNUPGHOME instead of GNUPGHOME for subsystem configuration Also remove some deprecated unit tests. * Also register GPG keys API with gRPC-GW * Update from codegen * Update GPG key API * Add web UI to manage GPG keys * Lint updates * Change wording * Add some plausibility checks for supplied data on key creation * Update from codegen * Re-allow binary keys and move check for ASCII armoured to UI * Make yarn lint happy * Add editing signature keys for projects in UI * Add ability to configure signature keys for project in CLI * Change default value to use for GNUPGHOME * Do not include data section in default gpg keys CM * Adapt Docker image for GnuPG feature * Add required configuration to installation manifests * Add add-signature-key and remove-signature-key commands to project CLI * Fix typo * Add initial user documentation for GnuPG verification * Fix role name - oops * Mention required RBAC roles in docs * Support GPG verification of git annotated tags as well * Ensure CLI can build succesfully * Better support verification on tags * Print key type in upper case * Update user documentation * Correctly disable GnuPG verification if ARGOCD_GPG_ENABLE=false * Clarify that this feature is only available with Git repositories * codegen * Move verification code to own function * Remove deprecated check * Make things more developer friendly when running locally * Enable GPG feature by default, and don't require ARGOCD_GNUPGHOME to be set * Revert changes to manifests to reflect default enable state * Codegen
Initial implementation was merged in PR #3242 right now 🎉 |
Good job @jannfis !! |
Summary
Currently, ArgoCD does not support the verification of GPG commit signatures on the repositories it uses for syncing applications to clusters. ArgoCD should provide the option to verify commit signatures on a per-repository basis to enable the verification of the commits to be applied.
Motivation
In some sensitive environments, signing of Git commits using PGP/GPG and only considering the signed ones might be a security requirement.
Proposal
Since ArgoCD must know the public part of the keys used to sign commits in order to verify them, the public keys could be managed using the certificate management API introduced with v1.2. This would probably mean an additional ConfigMap and volume mount for
argocd-server
andargocd-repo-server
.As for handling unverified commits, other tools (i.e. Flux) just ignore them and don't treat them as a valid revision to sync to. We should handle it similarly, and only sync to successfully verified commits when the option is enabled in a repository.
I would be happy to implement this feature, if this is something we want to support in ArgoCD.
The text was updated successfully, but these errors were encountered: