Maintain secret structure when parsing (gopasspw#2433)

* Maintain secret structure when parsing

This commit introduces a new KV secret type ("AKV") that fully maintains
the secret format when parsing. As such it obsoletes the old KV and
Plain formats and the need for the core.parsing option.

Fixes gopasspw#2431

RELEASE_NOTES=[ENHANCEMENT] Maintain secret structure when parsing

Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>

* Update internal/action/edit.go

Co-authored-by: Yolan Romailler <AnomalRoil@users.noreply.github.com>

* Address review comments

This brings back the noparsing flag since we need this to cover some
corners cases.

RELEASE_NOTES=n/a

Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>

Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>
Co-authored-by: Yolan Romailler <AnomalRoil@users.noreply.github.com>

docs/config.md

@@ -87,7 +87,6 @@ This is a list of available options:
 | `core.nocolor`         | `bool`   | Do not use color. | `false` |
 | `core.nopager`         | `bool`   | Do not invoke a pager to display long lists. | `false` |
 | `core.notifications`   | `bool`   | Enable desktop notifications. | `true` |
-| `core.parsing`         | `bool`   | Enable parsing of output to have key-value and yaml secrets. | `true` |
 | `core.readonly`        | `bool`   | Disable writing to a store. Note: This is just a convenience option to prevent accidential writes. Enforcement can only happen on a central server (if repos are set up around a central one). | `false` |
 | `mounts.path`          | `string` | Path to the root store. | `$XDG_DATA_HOME/gopass/stores/root` |
 | `core.showsafecontent` | `bool`   | Only output *safe content* (i.e. everything but the first line of a secret) to the terminal. Use *copy* (`-c`) to retrieve the password in the clipboard, or *force* (`-f`) to still print it. | `false` |

internal/action/audit_test.go

@@ -35,7 +35,7 @@ func TestAudit(t *testing.T) { //nolint:paralleltest
 	}()
 
 	t.Run("expect audit complaints on very weak passwords", func(t *testing.T) { //nolint:paralleltest
-		sec := &secrets.Plain{}
+		sec := secrets.NewAKV()
 		sec.SetPassword("123")
 		assert.NoError(t, act.Store.Set(ctx, "bar", sec))
 		assert.NoError(t, act.Store.Set(ctx, "baz", sec))

internal/action/binary.go

@@ -65,16 +65,16 @@ func (s *Action) Cat(c *cli.Context) error {
 func secFromBytes(dst, src string, in []byte) gopass.Secret {
 	debug.Log("Read %d bytes from %s to %s", len(in), src, dst)
 
-	sec := secrets.NewKV()
+	sec := secrets.NewAKV()
 	if err := sec.Set("Content-Disposition", fmt.Sprintf("attachment; filename=\"%s\"", filepath.Base(src))); err != nil {
 		debug.Log("Failed to set Content-Disposition: %q", err)
 	}
-
-	_, _ = sec.Write([]byte(base64.StdEncoding.EncodeToString(in)))
 	if err := sec.Set("Content-Transfer-Encoding", "Base64"); err != nil {
 		debug.Log("Failed to set Content-Transfer-Encoding: %q", err)
 	}
 
+	_, _ = sec.Write([]byte(base64.StdEncoding.EncodeToString(in)))
+
 	return sec
 }
 

internal/action/config_test.go

@@ -45,7 +45,6 @@ core.cliptimeout = 45
 core.exportkeys = true
 core.nopager = true
 core.notifications = true
-core.parsing = true
 `
 		want += "mounts.path = " + u.StoreDir("") + "\n"
 		assert.Equal(t, want, buf.String())
@@ -86,7 +85,6 @@ core.cliptimeout = 45
 core.exportkeys = true
 core.nopager = true
 core.notifications = true
-core.parsing = true
 `
 		want += "mounts.path = " + u.StoreDir("")
 		assert.Equal(t, want, strings.TrimSpace(buf.String()), "action.printConfigValues")
@@ -119,7 +117,6 @@ core.cliptimeout
 core.exportkeys
 core.nopager
 core.notifications
-core.parsing
 mounts.path
 `
 		assert.Equal(t, want, buf.String())

internal/action/copy_test.go

@@ -90,7 +90,7 @@ func TestCopy(t *testing.T) { //nolint:paralleltest
 
 	ctx = ctxutil.WithTerminal(ctx, false)
 	assert.NoError(t, act.show(ctx, c, "zab/zab", false))
-	assert.Equal(t, "barfoo", buf.String())
+	assert.Equal(t, "barfoo\n", buf.String())
 	buf.Reset()
 }
 
@@ -178,7 +178,7 @@ func TestCopyGpg(t *testing.T) { //nolint:paralleltest
 	buf.Reset()
 
 	ctx = ctxutil.WithTerminal(ctx, false)
-	assert.NoError(t, act.show(ctx, c, "zab/zab", false))
+	assert.NoError(t, act.show(WithPasswordOnly(ctx, true), c, "zab/zab", false))
 	assert.Equal(t, "barfoo", buf.String())
 	buf.Reset()
 }

internal/action/delete_test.go

@@ -45,9 +45,10 @@ func TestDelete(t *testing.T) { //nolint:paralleltest
 	buf.Reset()
 
 	// delete foo bar
-	sec := &secrets.Plain{}
+	sec := secrets.NewAKV()
 	sec.SetPassword("123")
-	sec.WriteString("---\nbar: zab")
+	_, err = sec.Write([]byte("---\nbar: zab"))
+	require.NoError(t, err)
 	assert.NoError(t, act.Store.Set(ctx, "foo", sec))
 
 	c = gptest.CliCtx(ctx, t, "foo", "bar")

internal/action/edit.go

@@ -52,9 +52,9 @@ func (s *Action) editUpdate(ctx context.Context, name string, content, nContent
 		return nil
 	}
 
-	nSec := secrets.ParsePlain(nContent)
+	nSec := secrets.ParseAKV(nContent)
 
-	// if the secret has a password, we check it's strength.
+	// if the secret has a password, we check its strength.
 	if pw := nSec.Password(); pw != "" {
 		audit.Single(ctx, pw)
 	}

internal/action/find_test.go

@@ -94,9 +94,10 @@ func TestFind(t *testing.T) { //nolint:paralleltest
 	buf.Reset()
 
 	// add some secrets
-	sec := &secrets.Plain{}
+	sec := secrets.NewAKV()
 	sec.SetPassword("foo")
-	sec.WriteString("bar")
+	_, err = sec.Write([]byte("bar"))
+	require.NoError(t, err)
 	assert.NoError(t, act.Store.Set(ctx, "bar/baz", sec))
 	assert.NoError(t, act.Store.Set(ctx, "bar/zab", sec))
 	buf.Reset()

internal/action/generate.go

@@ -361,7 +361,7 @@ func (s *Action) generateSetPassword(ctx context.Context, name, key, password st
 	}
 
 	if content, found := s.renderTemplate(ctx, name, []byte(password)); found {
-		nSec := &secrets.Plain{}
+		nSec := secrets.NewAKV()
 		if _, err := nSec.Write(content); err == nil {
 			sec = nSec
 		} else {

internal/action/grep_test.go

@@ -40,9 +40,10 @@ func TestGrep(t *testing.T) { //nolint:paralleltest
 
 	t.Run("add some secret", func(t *testing.T) { //nolint:paralleltest
 		defer buf.Reset()
-		sec := &secrets.Plain{}
+		sec := secrets.NewAKV()
 		sec.SetPassword("foobar")
-		sec.WriteString("foobar")
+		_, err := sec.Write([]byte("foobar"))
+		require.NoError(t, err)
 		assert.NoError(t, act.Store.Set(ctx, "foo", sec))
 	})
 

internal/action/insert.go

@@ -90,21 +90,14 @@ func (s *Action) insert(ctx context.Context, c *cli.Context, name, key string, e
 }
 
 func (s *Action) insertStdin(ctx context.Context, name string, content []byte, appendTo bool) error {
-	var sec gopass.Secret
+	var sec gopass.Secret = secrets.ParseAKV(content)
+
 	if appendTo && s.Store.Exists(ctx, name) {
 		var err error
 		sec, err = s.insertStdinAppend(ctx, name, content)
 		if err != nil {
 			return err
 		}
-	} else {
-		plain := &secrets.Plain{}
-		if n, err := plain.Write(content); err != nil || n < 0 {
-			return exit.Error(exit.Aborted, err, "failed to write secret from stdin: %s", err)
-		}
-
-		sec = plain
-		debug.Log("Created new plain secret with input")
 	}
 
 	if err := s.Store.Set(ctxutil.WithCommitMessage(ctx, "Read secret from STDIN"), name, sec); err != nil {
@@ -172,7 +165,7 @@ func (s *Action) insertGetSecret(ctx context.Context, name, pw string) (gopass.S
 	}
 
 	// render template into a new secret
-	sec := &secrets.Plain{}
+	sec := secrets.NewAKV()
 	if _, err := sec.Write(content); err != nil {
 		debug.Log("failed to handle template: %s", err)
 
@@ -231,7 +224,7 @@ func (s *Action) insertMultiline(ctx context.Context, c *cli.Context, name strin
 		return exit.Error(exit.Unknown, err, "failed to start editor: %s", err)
 	}
 
-	sec := &secrets.Plain{}
+	sec := secrets.NewAKV()
 	n, err := sec.Write(content)
 	if err != nil || n < 0 {
 		out.Errorf(ctx, "WARNING: Invalid secret: %s of len %d", err, n)

internal/action/insert_test.go

@@ -21,7 +21,6 @@ func TestInsert(t *testing.T) { //nolint:paralleltest
 	ctx := context.Background()
 	ctx = ctxutil.WithAlwaysYes(ctx, true)
 	ctx = ctxutil.WithTerminal(ctx, false)
-	ctx = ctxutil.WithShowParsing(ctx, true)
 
 	act, err := newMock(ctx, u.StoreDir(""))
 	require.NoError(t, err)
@@ -52,7 +51,7 @@ func TestInsert(t *testing.T) { //nolint:paralleltest
 		buf.Reset()
 
 		assert.NoError(t, act.show(ctx, gptest.CliCtx(ctx, t), "baz", false))
-		assert.Equal(t, "foobar", buf.String())
+		assert.Equal(t, "foobar\n", buf.String())
 		buf.Reset()
 	})
 
@@ -75,23 +74,20 @@ func TestInsert(t *testing.T) { //nolint:paralleltest
 	})
 
 	t.Run("insert baz via stdin w/ k-v", func(t *testing.T) { //nolint:paralleltest
-		assert.NoError(t, act.insertStdin(ctx, "baz", []byte("foobar\ninvalid key-value\nOther: meh\nUser: name\nbody text"), false))
+		in := "foobar\ninvalid key-value\nOther: meh\nUser: name\nbody text\n"
+		assert.NoError(t, act.insertStdin(ctx, "baz", []byte(in), false))
 		buf.Reset()
 
 		assert.NoError(t, act.show(ctx, gptest.CliCtx(ctx, t), "baz", false))
-		assert.Equal(t, "foobar\nother: meh\nuser: name\ninvalid key-value\nbody text", buf.String())
-		buf.Reset()
-
-		assert.NoError(t, act.show(ctxutil.WithShowParsing(ctx, false), gptest.CliCtx(ctx, t), "baz", false))
-		assert.Equal(t, "foobar\ninvalid key-value\nOther: meh\nUser: name\nbody text", buf.String())
+		assert.Equal(t, in, buf.String())
 		buf.Reset()
 	})
 
 	t.Run("insert zab#key", func(t *testing.T) { //nolint:paralleltest
 		ctx = ctxutil.WithInteractive(ctx, false)
 		require.NoError(t, act.cfg.Set("", "core.showsafecontent", "true"))
-		assert.NoError(t, act.insertYAML(ctx, "zab", "key", []byte("foobar"), nil))
-		assert.NoError(t, act.show(ctx, gptest.CliCtx(ctx, t), "zab", false))
+		assert.NoError(t, act.insertYAML(ctx, "zabkey", "key", []byte("foobar"), nil))
+		assert.NoError(t, act.show(ctx, gptest.CliCtx(ctx, t), "zabkey", false))
 		assert.Contains(t, buf.String(), "key: foobar")
 		buf.Reset()
 	})
@@ -117,17 +113,14 @@ func TestInsert(t *testing.T) { //nolint:paralleltest
 		buf.Reset()
 	})
 
-	t.Run("insert baz via stdin w/ yaml and no input parsing", func(t *testing.T) { //nolint:paralleltest
-		ctx = ctxutil.WithShowParsing(ctx, false)
+	t.Run("insert baz via stdin w/ yaml", func(t *testing.T) { //nolint:paralleltest
 		require.NoError(t, act.cfg.Set("", "core.showsafecontent", "false"))
 		assert.NoError(t, act.insertStdin(ctx, "baz", []byte("foobar\n---\nuser: name\nother: 0123"), false))
 		buf.Reset()
 
 		assert.NoError(t, act.show(ctx, gptest.CliCtx(ctx, t), "baz", false))
-		assert.Equal(t, "foobar\n---\nuser: name\nother: 0123", buf.String())
+		assert.Equal(t, "foobar\n---\nother: 83\nuser: name\n", buf.String())
 		buf.Reset()
-
-		ctx = ctxutil.WithShowParsing(ctx, true)
 	})
 }
 

internal/action/list_test.go

@@ -48,9 +48,9 @@ func TestList(t *testing.T) { //nolint:paralleltest
 	buf.Reset()
 
 	// add foo/bar and list folder foo
-	sec := &secrets.Plain{}
+	sec := secrets.NewAKV()
 	sec.SetPassword("123")
-	assert.Error(t, sec.Set("bar", "zab"))
+	assert.NoError(t, sec.Set("bar", "zab"))
 	assert.NoError(t, act.Store.Set(ctx, "foo/bar", sec))
 	buf.Reset()
 
@@ -72,7 +72,7 @@ func TestList(t *testing.T) { //nolint:paralleltest
 	// list --folders
 
 	// add more folders and subfolders
-	sec = &secrets.Plain{}
+	sec = secrets.NewAKV()
 	sec.SetPassword("123")
 	assert.NoError(t, act.Store.Set(ctx, "foo/zen/bar", sec))
 	assert.NoError(t, act.Store.Set(ctx, "foo2/bar2", sec))
@@ -87,7 +87,7 @@ foo2/
 	buf.Reset()
 
 	// add shadowed entry
-	sec = &secrets.Plain{}
+	sec = secrets.NewAKV()
 	sec.SetPassword("123")
 	assert.NoError(t, act.Store.Set(ctx, "foo/zen", sec))
 	buf.Reset()
@@ -144,7 +144,7 @@ func TestListLimit(t *testing.T) { //nolint:paralleltest
 └── foo
 
 `
-	sec := &secrets.Plain{}
+	sec := secrets.NewAKV()
 	sec.SetPassword("123")
 	assert.NoError(t, act.Store.Set(ctx, "foo/bar", sec))
 	assert.NoError(t, act.Store.Set(ctx, "foo/zen/baz/bar", sec))

internal/action/merge.go

@@ -67,7 +67,7 @@ func (s *Action) Merge(c *cli.Context) error {
 		}
 	}
 
-	nSec := secrets.ParsePlain(newContent)
+	nSec := secrets.ParseAKV(newContent)
 
 	// if the secret has a password, we check it's strength
 	if pw := nSec.Password(); pw != "" && !c.Bool("force") {

internal/action/otp_test.go

@@ -42,9 +42,10 @@ func TestOTP(t *testing.T) { //nolint:paralleltest
 
 	t.Run("create and display valid OTP", func(t *testing.T) { //nolint:paralleltest
 		defer buf.Reset()
-		sec := &secrets.Plain{}
+		sec := secrets.NewAKV()
 		sec.SetPassword("foo")
-		sec.WriteString(twofactor.GenerateGoogleTOTP().URL("foo"))
+		_, err := sec.Write([]byte(twofactor.GenerateGoogleTOTP().URL("foo")))
+		require.NoError(t, err)
 		assert.NoError(t, act.Store.Set(ctx, "bar", sec))
 
 		assert.NoError(t, act.OTP(gptest.CliCtx(ctx, t, "bar")))

internal/action/show.go

@@ -50,7 +50,7 @@ func showParseArgs(c *cli.Context) context.Context {
 	}
 
 	if c.IsSet("noparsing") {
-		_ = config.FromContext(ctx).SetEnv("core.parsing", fmt.Sprintf("%t", !c.Bool("noparsing")))
+		ctx = ctxutil.WithShowParsing(ctx, !c.Bool("noparsing"))
 	}
 
 	if c.IsSet("chars") {
@@ -211,8 +211,6 @@ func (s *Action) showHandleOutput(ctx context.Context, name string, sec gopass.S
 		header := fmt.Sprintf("Secret: %s\n", name)
 		if HasKey(ctx) {
 			header += fmt.Sprintf("Key: %s\n", GetKey(ctx))
-		} else if config.Bool(ctx, "core.parsing") {
-			out.Warning(ctx, "Parsing is enabled. Use -n to disable.")
 		}
 		out.Print(ctx, header)
 	}
@@ -225,7 +223,7 @@ func (s *Action) showHandleOutput(ctx context.Context, name string, sec gopass.S
 
 func (s *Action) showGetContent(ctx context.Context, sec gopass.Secret) (string, string, error) {
 	// YAML key.
-	if HasKey(ctx) && config.Bool(ctx, "core.parsing") {
+	if HasKey(ctx) {
 		key := GetKey(ctx)
 		values, found := sec.Values(key)
 		if !found {
@@ -266,7 +264,7 @@ func (s *Action) showGetContent(ctx context.Context, sec gopass.Secret) (string,
 	}
 
 	// everything (default).
-	return sec.Password(), fullBody, nil
+	return pw, fullBody, nil
 }
 
 func showSafeContent(sec gopass.Secret) string {