From 285f4dc8a1bf5e2c774fe48ae0daee2ac1511e6d Mon Sep 17 00:00:00 2001 From: Uwe Daub Date: Wed, 8 Jul 2020 09:00:34 +0200 Subject: [PATCH 1/4] Fix tests --- config/config_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/config_test.go b/config/config_test.go index 9f8710d48..3a5afb191 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -372,9 +372,9 @@ func TestLoadConfigFileWithVaultDestinationRules(t *testing.T) { conf, err := parseDestinationRuleForFile(parseConfigFile(sampleConfigWithVaultDestinationRules, t), "vault-v2/barfoo", nil) assert.Nil(t, err) assert.NotNil(t, conf.Destination) - assert.Equal(t, "http://127.0.0.1:8200/v1/secret/data/foobar/barfoo", conf.Destination.Path("barfoo")) + assert.Equal(t, "https://127.0.0.1:8200/v1/secret/data/foobar/barfoo", conf.Destination.Path("barfoo")) conf, err = parseDestinationRuleForFile(parseConfigFile(sampleConfigWithVaultDestinationRules, t), "vault-v1/barfoo", nil) assert.Nil(t, err) assert.NotNil(t, conf.Destination) - assert.Equal(t, "http://127.0.0.1:8200/v1/kv/barfoo/barfoo", conf.Destination.Path("barfoo")) + assert.Equal(t, "https://127.0.0.1:8200/v1/kv/barfoo/barfoo", conf.Destination.Path("barfoo")) } From 46babd098dda9e09e24a3687b35730daf52f02e0 Mon Sep 17 00:00:00 2001 From: Uwe Daub Date: Wed, 8 Jul 2020 13:45:03 +0200 Subject: [PATCH 2/4] Fix endless loop in x/crypto/openpgp func ReadMessage This fixes https://github.com/mozilla/sops/issues/665 See also https://github.com/golang/go/issues/28786 In some strange situations it can happen, that openpgp.ReadMessage() runs into a endless loop. This seems to be triggered by a slightly inconsistency in key settings. It happened to me, but I wasn't able to reproduce it with a fresh key. A proposed solution from the x/crypto community was, to break this loop in the callback passphrasePrompt. --- pgp/keysource.go | 67 +++++++++++++++++++++++++++--------------------- 1 file changed, 38 insertions(+), 29 deletions(-) diff --git a/pgp/keysource.go b/pgp/keysource.go index 153709220..78f89171d 100644 --- a/pgp/keysource.go +++ b/pgp/keysource.go @@ -216,7 +216,7 @@ func (key *MasterKey) decryptWithCryptoOpenpgp() ([]byte, error) { if err != nil { return nil, fmt.Errorf("Armor decoding failed: %s", err) } - md, err := openpgp.ReadMessage(block.Body, ring, key.passphrasePrompt, nil) + md, err := openpgp.ReadMessage(block.Body, ring, key.passphrasePrompt(), nil) if err != nil { return nil, fmt.Errorf("Reading PGP message failed: %s", err) } @@ -318,39 +318,48 @@ func (key *MasterKey) fingerprintMap(ring openpgp.EntityList) map[string]openpgp return fps } -func (key *MasterKey) passphrasePrompt(keys []openpgp.Key, symmetric bool) ([]byte, error) { - conn, err := gpgagent.NewConn() - if err == gpgagent.ErrNoAgent { - log.Infof("gpg-agent not found, continuing with manual passphrase " + - "input...") - fmt.Print("Enter PGP key passphrase: ") - pass, err := gopass.GetPasswd() - if err != nil { - return nil, err - } - for _, k := range keys { - k.PrivateKey.Decrypt(pass) +func (key *MasterKey) passphrasePrompt() func(keys []openpgp.Key, symmetric bool) ([]byte, error) { + callCounter := 0 + maxCalls := 3 + return func(keys []openpgp.Key, symmetric bool) ([]byte, error) { + if callCounter >= maxCalls { + return nil, fmt.Errorf("function passphrasePrompt called more than once") } - return pass, err - } - if err != nil { - return nil, fmt.Errorf("Could not establish connection with gpg-agent: %s", err) - } - defer conn.Close() - for _, k := range keys { - req := gpgagent.PassphraseRequest{ - CacheKey: k.PublicKey.KeyIdShortString(), - Prompt: "Passphrase", - Desc: fmt.Sprintf("Unlock key %s to decrypt sops's key", k.PublicKey.KeyIdShortString()), + callCounter++ + + conn, err := gpgagent.NewConn() + if err == gpgagent.ErrNoAgent { + log.Infof("gpg-agent not found, continuing with manual passphrase " + + "input...") + fmt.Print("Enter PGP key passphrase: ") + pass, err := gopass.GetPasswd() + if err != nil { + return nil, err + } + for _, k := range keys { + k.PrivateKey.Decrypt(pass) + } + return pass, err } - pass, err := conn.GetPassphrase(&req) if err != nil { - return nil, fmt.Errorf("gpg-agent passphrase request errored: %s", err) + return nil, fmt.Errorf("Could not establish connection with gpg-agent: %s", err) + } + defer conn.Close() + for _, k := range keys { + req := gpgagent.PassphraseRequest{ + CacheKey: k.PublicKey.KeyIdShortString(), + Prompt: "Passphrase", + Desc: fmt.Sprintf("Unlock key %s to decrypt sops's key", k.PublicKey.KeyIdShortString()), + } + pass, err := conn.GetPassphrase(&req) + if err != nil { + return nil, fmt.Errorf("gpg-agent passphrase request errored: %s", err) + } + k.PrivateKey.Decrypt([]byte(pass)) + return []byte(pass), nil } - k.PrivateKey.Decrypt([]byte(pass)) - return []byte(pass), nil + return nil, fmt.Errorf("No key to unlock") } - return nil, fmt.Errorf("No key to unlock") } // ToMap converts the MasterKey into a map for serialization purposes From 2baef3dae5f215b255684cb1d870c1f04650d302 Mon Sep 17 00:00:00 2001 From: Uwe Daub Date: Thu, 9 Jul 2020 11:23:03 +0200 Subject: [PATCH 3/4] Revert "Fix tests" This reverts commit 285f4dc8a1bf5e2c774fe48ae0daee2ac1511e6d. --- config/config_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/config_test.go b/config/config_test.go index 3a5afb191..9f8710d48 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -372,9 +372,9 @@ func TestLoadConfigFileWithVaultDestinationRules(t *testing.T) { conf, err := parseDestinationRuleForFile(parseConfigFile(sampleConfigWithVaultDestinationRules, t), "vault-v2/barfoo", nil) assert.Nil(t, err) assert.NotNil(t, conf.Destination) - assert.Equal(t, "https://127.0.0.1:8200/v1/secret/data/foobar/barfoo", conf.Destination.Path("barfoo")) + assert.Equal(t, "http://127.0.0.1:8200/v1/secret/data/foobar/barfoo", conf.Destination.Path("barfoo")) conf, err = parseDestinationRuleForFile(parseConfigFile(sampleConfigWithVaultDestinationRules, t), "vault-v1/barfoo", nil) assert.Nil(t, err) assert.NotNil(t, conf.Destination) - assert.Equal(t, "https://127.0.0.1:8200/v1/kv/barfoo/barfoo", conf.Destination.Path("barfoo")) + assert.Equal(t, "http://127.0.0.1:8200/v1/kv/barfoo/barfoo", conf.Destination.Path("barfoo")) } From 5ff1968dc21b375a0dc8cdf56c102a8a6a6d6611 Mon Sep 17 00:00:00 2001 From: Uwe Daub Date: Thu, 9 Jul 2020 11:26:04 +0200 Subject: [PATCH 4/4] Improve error description https://github.com/mozilla/sops/pull/690#discussion_r451630193 --- pgp/keysource.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pgp/keysource.go b/pgp/keysource.go index 78f89171d..5f111df2c 100644 --- a/pgp/keysource.go +++ b/pgp/keysource.go @@ -323,7 +323,7 @@ func (key *MasterKey) passphrasePrompt() func(keys []openpgp.Key, symmetric bool maxCalls := 3 return func(keys []openpgp.Key, symmetric bool) ([]byte, error) { if callCounter >= maxCalls { - return nil, fmt.Errorf("function passphrasePrompt called more than once") + return nil, fmt.Errorf("function passphrasePrompt called too many times") } callCounter++