feat(source-integrity): Implement Source Integrity checking#25371
feat(source-integrity): Implement Source Integrity checking#25371olivergondza wants to merge 30 commits intoargoproj:masterfrom
Conversation
❌ Preview Environment undeployed from BunnyshellAvailable commands (reply to this comment):
|
|
This is still WIP. Filed for CI feedback... |
71850e2 to
53d5533
Compare
09dae5d to
0b618a1
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #25371 +/- ##
==========================================
+ Coverage 62.66% 62.68% +0.02%
==========================================
Files 412 414 +2
Lines 55564 55795 +231
==========================================
+ Hits 34818 34976 +158
- Misses 17424 17478 +54
- Partials 3322 3341 +19 ☔ View full report in Codecov by Sentry. |
0b618a1 to
de3d402
Compare
blakepettersson
left a comment
There was a problem hiding this comment.
All in all I think this is great, awesome work!
I've added a few questions/thoughts, LMK what you think
36c6663 to
3362313
Compare
c6aac44 to
d04c6fa
Compare
27c964a to
826199b
Compare
183de87 to
fcca2c9
Compare
|
@jannfis, @blakepettersson, @pasha-codefresh, the implementation is complete. Please review. |
96e2a51 to
707740f
Compare
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
5491ff0 to
01bacb0
Compare
| }) | ||
| } | ||
|
|
||
| func TestNamespacedSyncToUnsignedCommit(t *testing.T) { |
There was a problem hiding this comment.
Tests moved to app_management_source_integrity_test.go and extended.
| }) | ||
| } | ||
|
|
||
| func TestSyncToUnsignedCommit(t *testing.T) { |
There was a problem hiding this comment.
Tests moved to app_multiple_sources_source_integrity_test.go.
|
|
||
| The GnuPG verification requires populating the Argo CD GnuPG keyring, and configuring source integrity policies for your repositories. | ||
|
|
||
| ## Managing Argo CD GnuPG keyring |
There was a problem hiding this comment.
Moved from the old location, renamed, unchanged.
There was a problem hiding this comment.
I believe mkdocs supports redirects for dropped pages. Can you add a redirect for the old gpg verification page?
There was a problem hiding this comment.
Good point. I see we do not use the redirect plugin, and I do not want to pull it in just for this. So I kept nothing but a note on the old location informing about the eventual removal, and linked to the new approach.
e542849 to
66202dc
Compare
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
66202dc to
8f2e093
Compare
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
blakepettersson
left a comment
There was a problem hiding this comment.
I think this in general is in great shape!
I do think we need to have at least CLI support for source integrity, by adding new CLI commands, and ideally (if possible) we should migrate the existing commands to set/unset source-integrity (if the legacy signature keys are not already in use, then that should instead be used).
I'll defer the nitty-gritty of the GPG signing logic to @jannfis
| Each application can be a subject or multiple checks, and the sync will be enabled only when all criteria are met. | ||
|
|
||
| > [!NOTE] | ||
| > Source Integrity Verification is only configured through `AppProject` manifests at this point. CLI and UI are not supported. |
There was a problem hiding this comment.
I do think we should at least have CLI support for this, if not also UI (which we'd default to use source integrity if not already used)
There was a problem hiding this comment.
I agree, and CLI is something I intend to work on next. Although, I propose to add it in a separate PR once this is merged, not to add few hundreds LOC to this already giant PR. WDYT?
There was a problem hiding this comment.
I'd be fine having this in a separate PR 👍
| // Request to verify the signature when generating the manifests (only for Git repositories) | ||
| bool verifySignature = 16; | ||
| // Source integrity constrains to verify the sources before use | ||
| github.com.argoproj.argo_cd.v3.pkg.apis.application.v1alpha1.SourceIntegrity sourceIntegrity = 16; |
There was a problem hiding this comment.
This would be a breaking API change. Do we have a path to deprecate/remove the old verify style API results?
There was a problem hiding this comment.
Thanks. Let me re-add verifyResult and signatureInfo here.
There was a problem hiding this comment.
@crenshaw-dev, backward compatibility is restored.
…fication Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Implements #25148
This replaces the former GPG feature with a more general mechanism to enforce integrity. The design enables introducing various kinds of sources (OCI/Helm) and using various verification mechanisms in the future.
AppProject's.spec.signatureKeys), but it is interpreting it using the new implementation (no duplicated code, some regression risk)AppProjectGPG signing keys management is labeled deprecated from now on (UI, CLI, CR), encouraging users to switch to Source Integrity managed through project manifest.Checklist:
Presentation of an application failing Source Integrity criteria: