Correctly handle new multiline secrets

This commit fixes as small issue in how multi-line secrets are handled.
Before they were always written in to the secret body completly ignoring
the first line that contains the password. Now we do respect that
correctly. To implement that properly we need to have some additional
code to satisfy the io.Writer assumptions around the AKV secret type.

Also this fixes some non-hermetic tests that showed up during testing of
this change.

Fixes gopasspw#2614

Signed-off-by: Dominik Schulz <[email protected]>

internal/action/generate.go

@@ -436,6 +436,7 @@ func (s *Action) generateReplaceExisting(ctx context.Context, name, key, passwor
 
 func setMetadata(sec gopass.Secret, kvps map[string]string) {
 	for k, v := range kvps {
+		debug.Log("setting %s to %s", k, v)
 		_ = sec.Set(k, v)
 	}
 }

internal/action/insert.go

@@ -185,6 +185,7 @@ func (s *Action) insertGetSecret(ctx context.Context, name, pw string) (gopass.S
 
 // insertYAML will overwrite existing keys.
 func (s *Action) insertYAML(ctx context.Context, name, key string, content []byte, kvps map[string]string) error {
+	debug.Log("insertYAML: %s - %s -> %s", name, key, content)
 	if ctxutil.IsInteractive(ctx) {
 		pw, err := termio.AskForString(ctx, name+":"+key, "")
 		if err != nil {
@@ -200,12 +201,15 @@ func (s *Action) insertYAML(ctx context.Context, name, key string, content []byt
 		if err != nil {
 			return exit.Error(exit.Encrypt, err, "failed to set key %q of %q: %s", key, name, err)
 		}
+		debug.Log("using existing secret %s", name)
 	} else {
 		sec = secrets.New()
+		debug.Log("creating new secret %s", name)
 	}
 
 	setMetadata(sec, kvps)
 
+	debug.Log("setting %s to %s", key, string(content))
 	if err := sec.Set(key, string(content)); err != nil {
 		return exit.Error(exit.Usage, err, "failed set key %q of %q: %q", key, name, err)
 	}

pkg/debug/debug.go

@@ -44,6 +44,10 @@ func initDebug() bool {
 		return false
 	}
 
+	// we need to explicitly set logSecrets to false in case tests run under an environment
+	// where GOPASS_DEBUG_LOG_SECRETS is true. Otherwise setting it to false in the test
+	// context won't have any effect.
+	logSecrets = false
 	if sv := os.Getenv("GOPASS_DEBUG_LOG_SECRETS"); sv != "" && sv != "false" {
 		logSecrets = true
 	}

pkg/debug/debug_test.go

@@ -51,6 +51,7 @@ func TestDebug(t *testing.T) {
 
 	fn := filepath.Join(td, "gopass.log")
 	t.Setenv("GOPASS_DEBUG_LOG", fn)
+	t.Setenv("GOPASS_DEBUG_LOG_SECRETS", "false")
 
 	// it's been already initialized, need to re-init
 	assert.True(t, initDebug())
@@ -64,6 +65,7 @@ func TestDebug(t *testing.T) {
 
 	logStr := string(buf)
 	assert.Contains(t, logStr, "foo")
+	assert.NotEqual(t, "true", os.Getenv("GOPASS_DEBUG_LOG_SECRETS"))
 	assert.NotContains(t, logStr, "secret")
 	assert.NotContains(t, logStr, "toolong")
 	assert.Contains(t, logStr, "shorter")

pkg/fsutil/fsutil_test.go

@@ -28,6 +28,7 @@ func TestCleanFilename(t *testing.T) {
 
 func TestCleanPath(t *testing.T) {
 	tempdir := t.TempDir()
+	t.Setenv("GOPASS_HOMEDIR", "")
 
 	m := map[string]string{
 		".":                                 "",

pkg/gopass/secrets/akv.go

@@ -4,6 +4,7 @@ import (
 	"bufio"
 	"bytes"
 	"fmt"
+	"io"
 	"strings"
 
 	"github.com/gopasspw/gopass/internal/set"
@@ -28,7 +29,6 @@ func NewAKV() *AKV {
 		kvp: make(map[string][]string),
 		raw: strings.Builder{},
 	}
-	a.raw.WriteString("\n")
 
 	return a
 }
@@ -92,6 +92,9 @@ func (a *AKV) Set(key string, value any) error {
 	// if the key does exist we must make sure to update only
 	// the first instance and leave all others intact.
 
+	if a.raw.Len() == 0 {
+		a.raw.WriteString("\n")
+	}
 	s := bufio.NewScanner(strings.NewReader(a.raw.String()))
 	a.raw = strings.Builder{}
 
@@ -143,6 +146,10 @@ func (a *AKV) Add(key string, value any) error {
 	sv := fmt.Sprintf("%s", value)
 	a.kvp[key] = append(a.kvp[key], sv)
 
+	// we must not accidentially write the KVP into the password field
+	if a.raw.Len() == 0 {
+		a.raw.WriteString("\n")
+	}
 	a.raw.WriteString(fmt.Sprintf("%s: %s\n", key, sv))
 
 	return nil
@@ -298,7 +305,20 @@ func (a *AKV) Body() string {
 
 // Write appends the buffer to the secret's body.
 func (a *AKV) Write(buf []byte) (int, error) {
-	return a.raw.Write(buf)
+	var w io.Writer
+	w = &a.raw
+
+	// If the body is empty before writing we must threat the first line
+	// of the newly written content as the password or multi-line insert
+	// and similar operations will fail. For regular command line usage
+	// this is actually a non-issue since the gopass process will exit
+	// after writing and when reading the secret it will be (correctly)
+	// parsed. But for tests, library and maybe REPL usage this is an issue.
+	if a.raw.Len() == 0 {
+		w = io.MultiWriter(&a.raw, &pwWriter{a: a})
+	}
+
+	return w.Write(buf)
 }
 
 // FromMime returns whether this secret was converted from a Mime secret of not.
@@ -310,3 +330,32 @@ func (a *AKV) FromMime() bool {
 func (a *AKV) SafeStr() string {
 	return "(elided)"
 }
+
+// pwWriter is a io.Writer that will extract the first line of the input stream and
+// then write it to the password field of the provided AKV. The first line can stretch
+// multiple chunks but once the first line hass been completed any writes to this
+// writer will be silently discarded.
+type pwWriter struct {
+	a       *AKV
+	buf     strings.Builder
+	written bool
+}
+
+func (p *pwWriter) Write(buf []byte) (int, error) {
+	n := len(buf)
+	if p.written || p.a == nil {
+		return n, nil
+	}
+
+	i := bytes.Index(buf, []byte("\n"))
+	if i > 0 {
+		p.buf.Write(buf[0:i])
+		p.a.password = p.buf.String()
+		p.written = true
+
+		return n, nil
+	}
+	p.buf.Write(buf)
+
+	return n, nil
+}

pkg/gopass/secrets/akv_test.go

@@ -84,6 +84,37 @@ zab: 123
 
 	sec := ParseAKV([]byte(in))
 	assert.Equal(t, in, string(sec.Bytes()))
+	assert.Equal(t, "passw0rd", sec.Password())
+}
+
+func TestMultilineInsertAKV(t *testing.T) {
+	t.Parallel()
+
+	in := `passw0rd
+foo: baz
+foo: bar
+zab: 123
+`
+
+	sec := NewAKV()
+	_, err := sec.Write([]byte(in))
+	assert.NoError(t, err)
+	assert.Equal(t, in, string(sec.Bytes()))
+	assert.Equal(t, "passw0rd", sec.Password())
+
+	_, err = sec.Write([]byte("more text"))
+	assert.NoError(t, err)
+	assert.Equal(t, "passw0rd", sec.Password())
+}
+
+func TestSetKeyValuePairToEmptyAKV(t *testing.T) {
+	t.Parallel()
+
+	sec := NewAKV()
+	assert.NoError(t, sec.Set("foo", "bar"))
+	v, found := sec.Get("foo")
+	assert.True(t, found)
+	assert.Equal(t, "bar", v)
 }
 
 func TestParseAKV(t *testing.T) {
@@ -310,3 +341,21 @@ func FuzzParseAKV(f *testing.F) {
 		ParseAKV(in)
 	})
 }
+
+func TestPwWriter(t *testing.T) {
+	a := NewAKV()
+	p := pwWriter{a: a}
+
+	// multi-chunk passwords are supported
+	_, err := p.Write([]byte("foo"))
+	assert.NoError(t, err)
+
+	_, err = p.Write([]byte("bar\n"))
+	assert.NoError(t, err)
+
+	// but anything after the first line is discarded
+	_, err = p.Write([]byte("baz\n"))
+	assert.NoError(t, err)
+
+	assert.Equal(t, "foobar", a.Password())
+}