-
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
feat: GPG commit signature verification (#2492) #3242
feat: GPG commit signature verification (#2492) #3242
Conversation
…t/2492-gpg-commits
PR is now feature complete. Will conduct a few more tests before declaring ready for review state. |
…t/2492-gpg-verification
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Added, several comments but I think we can resolve them later.
@@ -24,6 +24,9 @@ spec: | |||
- "20" | |||
- --operation-processors | |||
- "10" | |||
env: | |||
- name: ARGOCD_GPG_ENABLED | |||
value: "true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we enable by GPG by default and allow user to use ARGOCD_GPG_ENABLED
to disable it? In this case we don't have to touch manifests.
Additionally GPG feature is visible in UI even if feature is disabled. Why should we let user disable it at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a kill switch for the GPG feature is good for various reasons. For one, there is a slight performance impact of the feature, so people can switch it off if they won't require it. Second reason is, if there is a severe security issue found in Git's GnuPG implementation, or in the way we verify commits, disabling the feature for good by the kill switch could be a viable workaround until a fix is available.
But I agree, we can change semantics to enable the feature by default without touching the manifests. Shall I change it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have changed the default behaviour to GPG feature being enabled and reverted the manifest changes. It still can be disabled using kill switch ARGOCD_GPG_ENABLED=false
, for whatever reasons there might be.
…t/2492-gpg-verification
…t/2492-gpg-verification
// Remove files that do not exist in ConfigMap anymore | ||
err = filepath.Walk(destPath, func(path string, info os.FileInfo, err error) error { | ||
if info.IsDir() { | ||
return nil | ||
} | ||
if err != nil { | ||
log.Warnf("Error walking path %s: %v", path, err) | ||
} | ||
p := filepath.Base(path) | ||
if _, ok := cm.Data[p]; !ok { | ||
log.Infof("Removing file '%s'", path) | ||
err := os.Remove(path) | ||
if err != nil { | ||
log.Warnf("Failed to remove file %s: %v", path, err) | ||
} | ||
} | ||
return nil | ||
}) | ||
if err != nil { | ||
log.Fatalf("Error: %v", err) | ||
} | ||
// Create or update files that are specified in ConfigMap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexmt FYI, I've also added functionality to delete files from the file system which do not exist in the config map anymore. This makes local/manual testing possible.
Summary
This PR addresses #2492 and adds support for GPG signature verification on Git commits as well as basic management of GPG public keys to ArgoCD. GPG signature verification can be controlled on a per-project level by defining a list of key IDs in the project spec. The controller will only sync to those commits that are signed by one of the allowed key IDs, otherwise will refuse to sync.
Feature toggle and knobs
In order for this feature to be active, it must be explicitly switched on. To switch on GPG features, the environment variable
ARGOCD_GPG_ENABLED
must be non-empty and set to values other thanfalse
orno
. The environment needs to be set for the containers of theargocd-server
,argocd-controller
andargocd-repo-server
pods. It is not enabled by default for now.The following additional environment variables control the features' behaviour:
ARGOCD_GPG_DATA_PATH
(default:/app/config/gpg/source
) must be set to the path where the ConfigMap containing the GPG public keys is mounted at (valid only for theargocd-repo-server
pod)ARGOCD_GNUPGHOME
(default:/app/config/gpg/keys
) can be set to where the GPG keyring will be stored. This path must be available r/w and must not reside on a persistent volume.Completeness
argocd proj
for manipulating signature keys and signature verification enforcementContainerize test suite so toolchain is similar (chore: Containerize complete build & test toolchain #3245)General design principles
Functionalities of this feature have been designed to follow the least-privilege concept of ArgoCD, and are split across the API server, Repository Server and Application Controller components.
git verify-commit
on the target revision it checks out on behalf of application controller, if instructed by controller to do sogit verify-commit
) in the answer to the controllerEach repository server instance will create a GPG keyring and a private key upon initialization, both of which are transient and only valid
for the lifecycle of the pod instance. The private key is used for building the GPG trust database within the pod, and for nothing else. It is
not intended to be used elsewhere.
This design assumes trust between all ArgoCD components - especially the controller trusting the repo server - and also assumes trust into the K8s cluster ArgoCD is running in.
Introduced changes
New API for key management
The API supports the basic operations of adding, removing and listing configured GPG keys.
New gpg command in the CLI
GPG keys can be managed through the following new commands:
argocd gpg list
lists all configured GPG public keysargocd gpg get <keyid>
retrieves the configured GPG public key with key ID<keyid>
argocd gpg add --from <path>
adds one or more GPG public keys stored in file<path>
to ArgoCD. The data in<path>
can be either binary or ASCII-armored.argocd gpg rm <keyid>
removes the GPG public key from server's configurationNew ConfigMap resource
ArgoCD stores the GPG public key data in a new ConfigMap resource named
argocd-gpg-keys-cm
. The ConfigMap contains entries in the form of<KeyID>: <KeyData>
and must be mounted to theargocd-repo-server
pod, to make the public keys consumable by the repository server.New property "signatureKeys" in AppProject CRD
The property
signatureKeys
in custom resourceAppProject
is used to enforce verification of GPG commit signatures on a per-project level. The property takes an array ofSignatureKey
objects, which currently only has a single propertyKeyID
of type string. To have a project enforce signature validation, use the followingOther
New dependencies
This code uses
fsnotify
package from https://github.com/fsnotify/fsnotify, which is released under revised BSD license.Checklist: