From c63520ca4df47d058382d811e20cce6065721c02 Mon Sep 17 00:00:00 2001 From: Dominik Schulz Date: Wed, 21 Feb 2018 15:47:23 +0100 Subject: [PATCH] Implement a simple, robust and predictable KV parser (#659) Fixes #559 Fixes #638 --- action/fix.go | 72 ---------------------- action/fix_test.go | 68 --------------------- commands.go | 18 ------ commands_test.go | 2 +- store/secret/kv.go | 93 +++++++++++++++++++++++++++++ store/secret/kv_test.go | 43 +++++++++++++ store/secret/secret.go | 116 ++++++------------------------------ store/secret/secret_test.go | 19 +++--- store/secret/yaml.go | 88 +++++++++++++++++++++++++++ store/secret/yaml_test.go | 46 +++++++------- 10 files changed, 274 insertions(+), 291 deletions(-) delete mode 100644 action/fix.go delete mode 100644 action/fix_test.go 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/action/fix.go b/action/fix.go deleted file mode 100644 index bbbc112a80..0000000000 --- a/action/fix.go +++ /dev/null @@ -1,72 +0,0 @@ -package action - -import ( - "context" - "errors" - "strings" - - "github.com/justwatchcom/gopass/store/sub" - "github.com/justwatchcom/gopass/utils/out" - "github.com/justwatchcom/gopass/utils/termio" - "github.com/urfave/cli" -) - -// Fix checks the secrets for fixable inconsistencies -func (s *Action) Fix(ctx context.Context, c *cli.Context) error { - if c.IsSet("force") { - ctx = sub.WithFsckForce(ctx, c.Bool("force")) - } - check := c.Bool("check") - - if !termio.AskForConfirmation(ctx, "Do you want to introduce YAML document separators to all secrets that can be parsed as valid YAML?") { - return errors.New("user aborted") - } - - t, err := s.Store.Tree(ctx) - if err != nil { - return exitError(ctx, ExitList, err, "failed to list store: %s", err) - } - - pwList := t.List(0) - num := 0 - - for _, name := range pwList { - sec, err := s.Store.Get(ctx, name) - if err != nil { - out.Red(ctx, "Failed to decode secret %s: %s", name, err) - continue - } - if sec.Data() != nil { - // valid YAML already - out.Debug(ctx, "%s - not fixing - valid YAML", name) - continue - } - body := sec.Body() - if strings.HasPrefix(body, "---\n") { - // invalid YAML, but doc-sep, avoid duplicating the doc-sep - out.Debug(ctx, "%s - not fixing - invalid YAML w/ doc-sep", name) - continue - } - if strings.Trim(body, "\n\r\t ") == "" { - // body contains only whitespaces - out.Debug(ctx, "%s - not fixing - only whitepaces", name) - continue - } - out.Debug(ctx, "Secret %s before fix: %s", name, sec.String()) - if err := sec.SetBody("---\n" + body); err != nil { - out.Red(ctx, "Failed to add doc-sep to %s: %s", name, err) - continue - } - out.Debug(ctx, "Secret after fix: %s", sec.String()) - num++ - if check { - continue - } - if err := s.Store.Set(ctx, name, sec); err != nil { - out.Red(ctx, "Failed to save secret %s: %s", name, err) - } - } - out.Green(ctx, "Fixed %d secrets", num) - - return nil -} diff --git a/action/fix_test.go b/action/fix_test.go deleted file mode 100644 index 4f12ba4ac2..0000000000 --- a/action/fix_test.go +++ /dev/null @@ -1,68 +0,0 @@ -package action - -import ( - "bytes" - "context" - "flag" - "os" - "testing" - - "github.com/justwatchcom/gopass/store/secret" - "github.com/justwatchcom/gopass/tests/gptest" - "github.com/justwatchcom/gopass/utils/ctxutil" - "github.com/justwatchcom/gopass/utils/out" - "github.com/stretchr/testify/assert" - "github.com/urfave/cli" -) - -func TestFix(t *testing.T) { - u := gptest.NewUnitTester(t) - defer u.Remove() - - ctx := context.Background() - ctx = ctxutil.WithAlwaysYes(ctx, true) - act, err := newMock(ctx, u) - assert.NoError(t, err) - - buf := &bytes.Buffer{} - out.Stdout = buf - defer func() { - out.Stdout = os.Stdout - }() - - assert.NoError(t, act.Store.Set(ctx, "yaml/valid", secret.New("foo", "---\nbar: baz"))) - assert.NoError(t, act.Store.Set(ctx, "yaml/invalid1", secret.New("foo", "---\nbar"))) - assert.NoError(t, act.Store.Set(ctx, "yaml/invalid2", secret.New("foo", "bar:"))) - - app := cli.NewApp() - - // fix - fs := flag.NewFlagSet("default", flag.ContinueOnError) - c := cli.NewContext(app, fs, nil) - - assert.NoError(t, act.Fix(ctx, c)) - - // fix --force - fs = flag.NewFlagSet("default", flag.ContinueOnError) - sf := cli.BoolFlag{ - Name: "force", - Usage: "force", - } - assert.NoError(t, sf.ApplyWithError(fs)) - assert.NoError(t, fs.Parse([]string{"--force"})) - c = cli.NewContext(app, fs, nil) - - assert.NoError(t, act.Fix(ctx, c)) - - // fix --check - fs = flag.NewFlagSet("default", flag.ContinueOnError) - sf = cli.BoolFlag{ - Name: "check", - Usage: "check", - } - assert.NoError(t, sf.ApplyWithError(fs)) - assert.NoError(t, fs.Parse([]string{"--check=true"})) - c = cli.NewContext(app, fs, nil) - - assert.NoError(t, act.Fix(ctx, c)) -} diff --git a/commands.go b/commands.go index 25b0eb05d5..2d32cd3997 100644 --- a/commands.go +++ b/commands.go @@ -339,24 +339,6 @@ func getCommands(ctx context.Context, action *ap.Action, app *cli.App) []cli.Com }, }, }, - { - Name: "fix", - Usage: "Upgrade secrets", - Before: func(c *cli.Context) error { return action.Initialized(withGlobalFlags(ctx, c), c) }, - Action: func(c *cli.Context) error { - return action.Fix(withGlobalFlags(ctx, c), c) - }, - Flags: []cli.Flag{ - cli.BoolFlag{ - Name: "check, c", - Usage: "Only report", - }, - cli.BoolFlag{ - Name: "force, f", - Usage: "Auto-correct any errors, do not ask", - }, - }, - }, { Name: "generate", Usage: "Generate a new password", diff --git a/commands_test.go b/commands_test.go index 611aa71cdb..887f6316ca 100644 --- a/commands_test.go +++ b/commands_test.go @@ -89,7 +89,7 @@ func TestGetCommands(t *testing.T) { c := cli.NewContext(app, fs, nil) commands := getCommands(ctx, act, app) - assert.Equal(t, 31, len(commands)) + assert.Equal(t, 30, len(commands)) prefix := "" testCommands(t, c, commands, prefix) diff --git a/store/secret/kv.go b/store/secret/kv.go new file mode 100644 index 0000000000..aebcfc749e --- /dev/null +++ b/store/secret/kv.go @@ -0,0 +1,93 @@ +package secret + +import ( + "bufio" + "fmt" + "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 debug { + fmt.Printf("[DEBUG] decodeKV() - mayBeYAML - err: %s\n", err) + } + if docSep && err == nil && s.data != nil { + return nil + } + } + if debug { + fmt.Printf("[DEBUG] decodeKV() - simple KV\n") + } + 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 { + if debug { + fmt.Printf("[DEBUG] encodeKV() - mayBeYAML - OK\n") + } + return nil + } + } + if debug { + fmt.Printf("[DEBUG] encodeKV() - simple KV\n") + } + 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..eb63bb049f 100644 --- a/store/secret/secret.go +++ b/store/secret/secret.go @@ -2,14 +2,18 @@ package secret import ( "bytes" - "fmt" + "os" "strings" "sync" +) - "github.com/justwatchcom/gopass/store" +var debug bool - yaml "gopkg.in/yaml.v2" -) +func init() { + if gdb := os.Getenv("GOPASS_DEBUG"); gdb != "" { + debug = true + } +} // Secret is a decoded secret type Secret struct { @@ -38,41 +42,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 +61,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 +110,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 { @@ -209,18 +134,13 @@ func (s *Secret) Equal(other *Secret) bool { return false } - buf, err := s.Bytes() - if err != nil { - return false - } - bufOther, err := other.Bytes() - if err != nil { - return false - } + return true +} - if len(buf) != len(bufOther) { - return false - } +func (s *Secret) encode() error { + return s.encodeKV() +} - return true +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 := `---`