-
Notifications
You must be signed in to change notification settings - Fork 7.7k
feat(hydrator): sign hydrated commits (Alpha) (#28239) #28271
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
Open
mladjan-gadzic
wants to merge
18
commits into
argoproj:master
Choose a base branch
from
mladjan-gadzic:sign-unsigned-commit
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
8be2a7f
feat(hydrator): sign hydrated commits
mladjan-gadzic f96d82a
fix: address review comment - deprecate function
mladjan-gadzic af132c6
fix: address review comments - verify path at startup
mladjan-gadzic ef38c31
fix: address review comments - improve cleanup
mladjan-gadzic a9aed54
fix: address review comment - keyData zeroing
mladjan-gadzic 2c97384
fix: address review comments - replace wrapper
mladjan-gadzic f3b80f3
fix: address review comments - remove deprecated code
mladjan-gadzic 30420c6
fix: address review comments - improve tests, add locking for atomic …
mladjan-gadzic c66fea8
fix: consolidate test helpers and add full-flow test
mladjan-gadzic 23e93a7
fix: increase test coverage and refactor
mladjan-gadzic 3e7a41f
chore: merge master and resolve conflicts
mladjan-gadzic 9bb106e
fix: ci
mladjan-gadzic ffd1927
chore: merge upstream/master and resolve
mladjan-gadzic e1ebcd4
fix: tests
mladjan-gadzic a1ee32c
fix: ci failure
mladjan-gadzic 8ca6784
fix: address review comment - add example docs
mladjan-gadzic efa3a62
fix: address review comment - fix wording
mladjan-gadzic 9f62820
Merge remote-tracking branch 'upstream/master' into sign-unsigned-commit
mladjan-gadzic File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
165 changes: 165 additions & 0 deletions
165
cmd/argocd-commit-server/commands/argocd_commit_server_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,165 @@ | ||
| package commands | ||
|
|
||
| import ( | ||
| "os" | ||
| "os/exec" | ||
| "path/filepath" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
|
|
||
| "github.com/argoproj/argo-cd/v3/common" | ||
| "github.com/argoproj/argo-cd/v3/util/gpgsign/gpgsigntest" | ||
| ) | ||
|
|
||
| func TestValidateSigningFlags(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| keyPath string | ||
| passphraseFile string | ||
| wantErr bool | ||
| }{ | ||
| { | ||
| name: "no key and no passphrase is valid (signing disabled)", | ||
| keyPath: "", | ||
| passphraseFile: "", | ||
| wantErr: false, | ||
| }, | ||
| { | ||
| name: "key without passphrase is valid (unprotected key)", | ||
| keyPath: "/app/config/gpg/signing/signingKey", | ||
| passphraseFile: "", | ||
| wantErr: false, | ||
| }, | ||
| { | ||
| name: "key with passphrase is valid (protected key)", | ||
| keyPath: "/app/config/gpg/signing/signingKey", | ||
| passphraseFile: "/app/config/gpg/signing/passphrase", | ||
| wantErr: false, | ||
| }, | ||
| { | ||
| name: "passphrase without key is rejected", | ||
| keyPath: "", | ||
| passphraseFile: "/app/config/gpg/signing/passphrase", | ||
| wantErr: true, | ||
| }, | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| err := validateSigningFlags(tt.keyPath, tt.passphraseFile) | ||
| if tt.wantErr { | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "signing-key-path") | ||
| } else { | ||
| require.NoError(t, err) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| // setSharedGnuPGHome points ARGOCD_GNUPGHOME at a throwaway dir so | ||
| // setupSigningKey can initialize a real keyring without touching the host's. | ||
| func setSharedGnuPGHome(t *testing.T) { | ||
| t.Helper() | ||
| t.Setenv(common.EnvGnuPGHome, gpgsigntest.ShortTempDir(t)) | ||
| } | ||
|
|
||
| func TestSetupSigningKey(t *testing.T) { | ||
| t.Run("imports an unprotected key and returns its config", func(t *testing.T) { | ||
| keyData, wantFP := gpgsigntest.GenerateSigningKey(t, "") | ||
| setSharedGnuPGHome(t) | ||
|
|
||
| keyPath := filepath.Join(gpgsigntest.ShortTempDir(t), "signingKey") | ||
| require.NoError(t, os.WriteFile(keyPath, keyData, 0o600)) | ||
|
|
||
| cfg, err := setupSigningKey(keyPath, "") | ||
| require.NoError(t, err) | ||
| require.NotNil(t, cfg) | ||
| assert.Equal(t, wantFP, cfg.Fingerprint) | ||
| assert.Equal(t, wantFP[len(wantFP)-16:], cfg.KeyID) | ||
| }) | ||
|
|
||
| t.Run("imports a passphrase-protected key and presets the passphrase", func(t *testing.T) { | ||
| const passphrase = "s3cret" | ||
| keyData, wantFP := gpgsigntest.GenerateSigningKey(t, passphrase) | ||
| setSharedGnuPGHome(t) | ||
|
|
||
| dir := gpgsigntest.ShortTempDir(t) | ||
| keyPath := filepath.Join(dir, "signingKey") | ||
| require.NoError(t, os.WriteFile(keyPath, keyData, 0o600)) | ||
| // File-editors love a trailing newline; setupSigningKey must strip it. | ||
| passphraseFile := filepath.Join(dir, "passphrase") | ||
| require.NoError(t, os.WriteFile(passphraseFile, []byte(passphrase+"\n"), 0o600)) | ||
|
|
||
| cfg, err := setupSigningKey(keyPath, passphraseFile) | ||
| // gpg-preset-passphrase, and the gpgconf used to locate it, ship with the | ||
| // gpg-agent package but may be absent in minimal environments; skip there | ||
| // rather than fail. | ||
| if err != nil && (strings.Contains(err.Error(), "gpg-preset-passphrase not found") || | ||
| strings.Contains(err.Error(), "failed to locate gnupg libexec dir")) { | ||
| t.Skipf("gpg-preset-passphrase unavailable: %v", err) | ||
| } | ||
| require.NoError(t, err) | ||
| require.NotNil(t, cfg) | ||
| assert.Equal(t, wantFP, cfg.Fingerprint) | ||
| }) | ||
|
|
||
| t.Run("fails when the key file does not exist", func(t *testing.T) { | ||
| // gpg is still required because setupSigningKey initializes GnuPG before | ||
| // it ever reads the key file. | ||
| if _, err := exec.LookPath("gpg"); err != nil { | ||
| t.Skip("gpg not available") | ||
| } | ||
| setSharedGnuPGHome(t) | ||
|
|
||
| _, err := setupSigningKey(filepath.Join(gpgsigntest.ShortTempDir(t), "does-not-exist"), "") | ||
| require.Error(t, err) | ||
| assert.ErrorContains(t, err, "failed to read signing key") | ||
| }) | ||
|
|
||
| t.Run("fails when the key file is empty", func(t *testing.T) { | ||
| if _, err := exec.LookPath("gpg"); err != nil { | ||
| t.Skip("gpg not available") | ||
| } | ||
| setSharedGnuPGHome(t) | ||
|
|
||
| keyPath := filepath.Join(gpgsigntest.ShortTempDir(t), "empty") | ||
| require.NoError(t, os.WriteFile(keyPath, []byte(" \n"), 0o600)) | ||
|
|
||
| _, err := setupSigningKey(keyPath, "") | ||
| require.Error(t, err) | ||
| assert.ErrorContains(t, err, "is empty") | ||
| }) | ||
|
|
||
| t.Run("fails when the passphrase file does not exist", func(t *testing.T) { | ||
| if _, err := exec.LookPath("gpg"); err != nil { | ||
| t.Skip("gpg not available") | ||
| } | ||
| setSharedGnuPGHome(t) | ||
|
|
||
| // The passphrase is read before the key is imported, so a non-empty | ||
| // placeholder key is enough to reach the passphrase-read error. | ||
| keyPath := filepath.Join(gpgsigntest.ShortTempDir(t), "signingKey") | ||
| require.NoError(t, os.WriteFile(keyPath, []byte("not-a-real-key"), 0o600)) | ||
|
|
||
| _, err := setupSigningKey(keyPath, filepath.Join(gpgsigntest.ShortTempDir(t), "no-passphrase")) | ||
| require.Error(t, err) | ||
| assert.ErrorContains(t, err, "failed to read signing key passphrase") | ||
| }) | ||
|
|
||
| t.Run("fails when the key data is not a valid GPG key", func(t *testing.T) { | ||
| if _, err := exec.LookPath("gpg"); err != nil { | ||
| t.Skip("gpg not available") | ||
| } | ||
| setSharedGnuPGHome(t) | ||
|
|
||
| keyPath := filepath.Join(gpgsigntest.ShortTempDir(t), "signingKey") | ||
| require.NoError(t, os.WriteFile(keyPath, []byte("not-a-real-key"), 0o600)) | ||
|
|
||
| _, err := setupSigningKey(keyPath, "") | ||
| require.Error(t, err) | ||
| assert.ErrorContains(t, err, "failed to import signing key") | ||
| }) | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Do you think the code should empty
keydata, for both security and memory issues?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.
keyDatais gc-eligable so no memory issues that way. however, from security perspective, it can be zeroed as in-depth defense.