From 5ea5326959eade11b46815efd62e32304218056e Mon Sep 17 00:00:00 2001 From: Dominik Schulz Date: Sun, 18 Feb 2018 21:21:52 +0100 Subject: [PATCH] Implement a simple, robust and predictable KV parser --- .travis.yml | 2 +- store/secret/kv.go | 80 ++++++++++++++++++++++++++++++ store/secret/kv_test.go | 43 ++++++++++++++++ store/secret/secret.go | 98 +++++-------------------------------- store/secret/secret_test.go | 19 ++++--- store/secret/yaml.go | 88 +++++++++++++++++++++++++++++++++ store/secret/yaml_test.go | 46 ++++++++--------- 7 files changed, 254 insertions(+), 122 deletions(-) create mode 100644 store/secret/kv.go create mode 100644 store/secret/kv_test.go create mode 100644 store/secret/yaml.go diff --git a/.travis.yml b/.travis.yml index 889fc41386..ecd3a90b6e 100644 --- a/.travis.yml +++ b/.travis.yml @@ -8,7 +8,7 @@ before_install: - if [ $TRAVIS_OS_NAME = linux ]; then sudo apt-get install git gnupg2; else brew install git gnupg || true; fi go: - - 1.9 + - "1.10" script: - make all diff --git a/store/secret/kv.go b/store/secret/kv.go new file mode 100644 index 0000000000..cd1f84873b --- /dev/null +++ b/store/secret/kv.go @@ -0,0 +1,80 @@ +package secret + +import ( + "bufio" + "sort" + "strings" +) + +func (s *Secret) decodeKV() error { + mayBeYAML := false + scanner := bufio.NewScanner(strings.NewReader(s.body)) + data := make(map[string]interface{}, strings.Count(s.body, "\n")) + for scanner.Scan() { + line := scanner.Text() + if strings.HasPrefix(line, "---") { + mayBeYAML = true + } + parts := strings.SplitN(line, ":", 2) + if len(parts) < 1 { + continue + } + if len(parts) == 1 && strings.HasPrefix(parts[0], " ") { + mayBeYAML = true + } + for i, part := range parts { + parts[i] = strings.TrimSpace(part) + } + // preserve key only entries + if len(parts) < 2 { + data[parts[0]] = "" + continue + } + if strings.HasPrefix(parts[1], "|") { + mayBeYAML = true + } + data[parts[0]] = parts[1] + } + if mayBeYAML { + docSep, err := s.decodeYAML() + if docSep && err == nil && s.data != nil { + return nil + } + } + s.data = data + return nil +} + +func (s *Secret) encodeKV() error { + if s.data == nil { + return nil + } + keys := make([]string, 0, len(s.data)) + for key := range s.data { + keys = append(keys, key) + } + sort.Strings(keys) + var buf strings.Builder + mayBeYAML := false + for _, key := range keys { + sv, ok := s.data[key].(string) + if !ok { + mayBeYAML = true + continue + } + _, _ = buf.WriteString(key) + _, _ = buf.WriteString(": ") + _, _ = buf.WriteString(sv) + _, _ = buf.WriteString("\n") + if strings.Contains(sv, "\n") { + mayBeYAML = true + } + } + if mayBeYAML { + if err := s.encodeYAML(); err == nil { + return nil + } + } + s.body = buf.String() + return nil +} diff --git a/store/secret/kv_test.go b/store/secret/kv_test.go new file mode 100644 index 0000000000..651ae5de1c --- /dev/null +++ b/store/secret/kv_test.go @@ -0,0 +1,43 @@ +package secret + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestYAMLFromHereDoc(t *testing.T) { + t.Logf("Parse K/V w/ HereDoc as YAML, not K/V") + mlValue := `somepw +--- +foo: | + bar + baz +key: value +` + s, err := Parse([]byte(mlValue)) + assert.NoError(t, err) + assert.NotNil(t, s) + v, err := s.Value("foo") + assert.NoError(t, err) + assert.Equal(t, "bar\nbaz\n", v) +} + +func TestKVContentFromInvalidYAML(t *testing.T) { + t.Logf("Retrieve content from invalid YAML (#375)") + mlValue := `somepasswd +--- +Test / test.com +username: myuser@test.com +password: somepasswd +url: http://www.test.com/` + s, err := Parse([]byte(mlValue)) + assert.NoError(t, err) + assert.NotNil(t, s) + v, err := s.Value("Test / test.com") + assert.NoError(t, err) + assert.Equal(t, "", v) + + // read back key + assert.Equal(t, mlValue, s.String()) +} diff --git a/store/secret/secret.go b/store/secret/secret.go index a171da7dc1..ebd7b1f3d6 100644 --- a/store/secret/secret.go +++ b/store/secret/secret.go @@ -2,13 +2,8 @@ package secret import ( "bytes" - "fmt" "strings" "sync" - - "github.com/justwatchcom/gopass/store" - - yaml "gopkg.in/yaml.v2" ) // Secret is a decoded secret @@ -38,41 +33,12 @@ func Parse(buf []byte) (*Secret, error) { if len(lines) > 1 { s.body = string(bytes.TrimSpace(lines[1])) } - if _, err := s.decodeYAML(); err != nil { + if err := s.decode(); err != nil { return s, err } return s, nil } -// decodeYAML attempts to decode an optional YAML part of a secret -func (s *Secret) decodeYAML() (bool, error) { - if !strings.HasPrefix(s.body, "---\n") && s.password != "---" { - return false, nil - } - d := make(map[string]interface{}) - err := yaml.Unmarshal([]byte(s.body), &d) - if err != nil { - return true, err - } - s.data = d - return true, nil -} - -func (s *Secret) encodeYAML() (err error) { - defer func() { - if r := recover(); r != nil { - err = fmt.Errorf("panic: %s", r) - } - }() - // update body - yb, err := yaml.Marshal(s.data) - if err != nil { - return err - } - s.body = "---\n" + string(yb) - return err -} - // Bytes encodes an secret func (s *Secret) Bytes() ([]byte, error) { buf := &bytes.Buffer{} @@ -86,7 +52,7 @@ func (s *Secret) Bytes() ([]byte, error) { // String encodes and returns a string representation of a secret func (s *Secret) String() string { - buf := &bytes.Buffer{} + var buf strings.Builder _, _ = buf.WriteString(s.password) _, _ = buf.WriteString("\n") _, _ = buf.WriteString(s.body) @@ -135,60 +101,10 @@ func (s *Secret) SetBody(b string) error { s.body = b s.data = nil - _, err := s.decodeYAML() + err := s.decode() return err } -// Value returns the value of the given key if the body contained valid -// YAML -func (s *Secret) Value(key string) (string, error) { - s.Lock() - defer s.Unlock() - - if s.data == nil { - if !strings.HasPrefix(s.body, "---\n") { - return "", store.ErrYAMLNoMark - } - if _, err := s.decodeYAML(); err != nil { - return "", err - } - } - if v, found := s.data[key]; found { - if sv, ok := v.(string); ok { - return sv, nil - } - return "", store.ErrYAMLValueUnsupported - } - return "", store.ErrYAMLNoKey -} - -// SetValue sets a key to a given value. Will fail if an non-empty body exists -func (s *Secret) SetValue(key, value string) error { - s.Lock() - defer s.Unlock() - - if s.body == "" && s.data == nil { - s.data = make(map[string]interface{}, 1) - } - if s.data == nil { - return store.ErrYAMLNoMark - } - s.data[key] = value - return s.encodeYAML() -} - -// DeleteKey key will delete a single key from an decoded map -func (s *Secret) DeleteKey(key string) error { - s.Lock() - defer s.Unlock() - - if s.data == nil { - return store.ErrYAMLNoMark - } - delete(s.data, key) - return s.encodeYAML() -} - // Equal returns true if two secrets are equal func (s *Secret) Equal(other *Secret) bool { if s == nil && other == nil { @@ -224,3 +140,11 @@ func (s *Secret) Equal(other *Secret) bool { return true } + +func (s *Secret) encode() error { + return s.encodeKV() +} + +func (s *Secret) decode() error { + return s.decodeKV() +} diff --git a/store/secret/secret_test.go b/store/secret/secret_test.go index 23bdc8fe8e..3f2018537b 100644 --- a/store/secret/secret_test.go +++ b/store/secret/secret_test.go @@ -34,18 +34,18 @@ func TestNew(t *testing.T) { // delete non-existing key assert.NoError(t, sec.DeleteKey("some-key")) - // set invalid YAML - assert.Error(t, sec.SetBody("---\nkey-only\n")) + // set invalid YAML, should parse as K/V + assert.NoError(t, sec.SetBody("---\nkey-only\n")) assert.Equal(t, "---\nkey-only\n", sec.Body()) // non-YAML body assert.NoError(t, sec.SetBody("key-only\n")) // try to set value on non-YAML body - assert.EqualError(t, sec.SetValue("key", "value"), store.ErrYAMLNoMark.Error()) + assert.NoError(t, sec.SetValue("key", "value")) // delete non-existing key - assert.EqualError(t, sec.DeleteKey("some-key"), store.ErrYAMLNoMark.Error()) + assert.NoError(t, sec.DeleteKey("some-key")) } func TestEqual(t *testing.T) { @@ -173,7 +173,10 @@ key2: value2`), "key1": "value1", "key2": "value2", }, - Fail: true, + Body: `--- + key1: value1 +key2: value2`, + Fail: false, }, { Desc: "missing YAML marker", @@ -187,14 +190,14 @@ key2: value2`, } { sec, err := Parse(tc.In) if tc.Fail { - assert.Error(t, err) + assert.Error(t, err, tc.Desc) continue } else if err != nil { assert.NoError(t, err) continue } - assert.Equal(t, tc.Password, sec.Password()) - assert.Equal(t, tc.Body, sec.Body()) + assert.Equal(t, tc.Password, sec.Password(), tc.Desc) + assert.Equal(t, tc.Body, sec.Body(), tc.Desc) for k, v := range tc.Data { rv, err := sec.Value(k) assert.NoError(t, err) diff --git a/store/secret/yaml.go b/store/secret/yaml.go new file mode 100644 index 0000000000..37cdd07131 --- /dev/null +++ b/store/secret/yaml.go @@ -0,0 +1,88 @@ +package secret + +import ( + "fmt" + "strings" + + "github.com/justwatchcom/gopass/store" + yaml "gopkg.in/yaml.v2" +) + +// Value returns the value of the given key if the body contained valid +// YAML +func (s *Secret) Value(key string) (string, error) { + s.Lock() + defer s.Unlock() + + if s.data == nil { + if !strings.HasPrefix(s.body, "---\n") { + return "", store.ErrYAMLNoMark + } + if err := s.decode(); err != nil { + return "", err + } + } + if v, found := s.data[key]; found { + if sv, ok := v.(string); ok { + return sv, nil + } + return "", store.ErrYAMLValueUnsupported + } + return "", store.ErrYAMLNoKey +} + +// SetValue sets a key to a given value. Will fail if an non-empty body exists +func (s *Secret) SetValue(key, value string) error { + s.Lock() + defer s.Unlock() + + if s.body == "" && s.data == nil { + s.data = make(map[string]interface{}, 1) + } + if s.data == nil { + return store.ErrYAMLNoMark + } + s.data[key] = value + return s.encode() +} + +// DeleteKey key will delete a single key from an decoded map +func (s *Secret) DeleteKey(key string) error { + s.Lock() + defer s.Unlock() + + if s.data == nil { + return store.ErrYAMLNoMark + } + delete(s.data, key) + return s.encode() +} + +// decodeYAML attempts to decode an optional YAML part of a secret +func (s *Secret) decodeYAML() (bool, error) { + if !strings.HasPrefix(s.body, "---\n") && s.password != "---" { + return false, nil + } + d := make(map[string]interface{}) + err := yaml.Unmarshal([]byte(s.body), &d) + if err != nil { + return true, err + } + s.data = d + return true, nil +} + +func (s *Secret) encodeYAML() (err error) { + defer func() { + if r := recover(); r != nil { + err = fmt.Errorf("panic: %s", r) + } + }() + // update body + yb, err := yaml.Marshal(s.data) + if err != nil { + return err + } + s.body = "---\n" + string(yb) + return err +} diff --git a/store/secret/yaml_test.go b/store/secret/yaml_test.go index 74c7950786..b047f7c1b7 100644 --- a/store/secret/yaml_test.go +++ b/store/secret/yaml_test.go @@ -2,10 +2,11 @@ package secret import ( "fmt" + "sort" "strconv" + "strings" "testing" - "github.com/justwatchcom/gopass/store" "github.com/stretchr/testify/assert" ) @@ -58,7 +59,7 @@ func TestYAMLKeyToEmptySecret(t *testing.T) { // read back whole entry buf, err := s.Bytes() assert.NoError(t, err) - want := "\n---\n" + yamlKey + ": " + yamlValue + "\n" + want := "\n" + yamlKey + ": " + yamlValue + "\n" assert.Equal(t, want, string(buf)) } @@ -96,21 +97,20 @@ func TestYAMLKeyToPWOnlySecret(t *testing.T) { // read back whole entry bv, err := s.Bytes() assert.NoError(t, err) - want := yamlPassword + "\n---\nbar: baz\n" + want := yamlPassword + "\nbar: baz\n" assert.Equal(t, want, string(bv)) } func TestBareYAMLReadKey(t *testing.T) { t.Logf("Bare YAML - no document marker - read key") - in := "bar: baz\nzab: 123\n" + in := "\nbar: baz\nzab: 123\n" s, err := Parse([]byte(in)) assert.NoError(t, err) // read back a key _, err = s.Value(yamlKey) - if err != store.ErrYAMLNoMark { - t.Fatalf("Should fail to read YAML without document marker") - } + assert.NoError(t, err) + // read back whole entry content, err := s.Bytes() assert.NoError(t, err) @@ -122,8 +122,11 @@ func TestYAMLSetMultipleKeys(t *testing.T) { s, err := Parse([]byte(yamlPassword)) assert.NoError(t, err) - want := yamlPassword + "\n---\n" + var b strings.Builder + _, _ = b.WriteString(yamlPassword) + _, _ = b.WriteString("\n") numKey := 100 + keys := make([]string, 0, numKey) for i := 0; i < numKey; i++ { // set key key := fmt.Sprintf("%s-%d", yamlKey, i) @@ -131,7 +134,14 @@ func TestYAMLSetMultipleKeys(t *testing.T) { t.Fatalf("Failed to write new key: %s", err) continue } - want += key + ": " + yamlValue + "\n" + keys = append(keys, key) + } + sort.Strings(keys) + for _, key := range keys { + _, _ = b.WriteString(key) + _, _ = b.WriteString(": ") + _, _ = b.WriteString(yamlValue) + _, _ = b.WriteString("\n") } // read back the password @@ -150,7 +160,7 @@ func TestYAMLSetMultipleKeys(t *testing.T) { // read back whole entry content, err := s.Bytes() assert.NoError(t, err) - assert.Equal(t, want, string(content)) + assert.Equal(t, b.String(), string(content)) } func TestYAMLMultilineWithDashes(t *testing.T) { @@ -170,22 +180,6 @@ ccc assert.Equal(t, mlValue, string(content)) } -func TestYAMLContentFromInvalidYAML(t *testing.T) { - t.Logf("Retrieve content from invalid YAML (#375)") - mlValue := `somepasswd ---- -Test / test.com -username: myuser@test.com -password: somepasswd -url: http://www.test.com/` - s, err := Parse([]byte(mlValue)) - assert.Error(t, err) - assert.NotNil(t, s) - - // read back key - assert.Equal(t, mlValue, s.String()) -} - func TestYAMLDocMarkerAsPW(t *testing.T) { t.Logf("Document Marker as Password (#398)") mlValue := `---`