From 8fdd4dd6746cddc45c3d414eef8ac9b1bebee809 Mon Sep 17 00:00:00 2001 From: Dominik Schulz Date: Sun, 30 Jul 2023 13:25:38 +0200 Subject: [PATCH 1/4] 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 #2614 Signed-off-by: Dominik Schulz --- internal/action/generate.go | 1 + internal/action/insert.go | 4 +++ pkg/debug/debug.go | 4 +++ pkg/debug/debug_test.go | 2 ++ pkg/fsutil/fsutil_test.go | 1 + pkg/gopass/secrets/akv.go | 53 ++++++++++++++++++++++++++++++++-- pkg/gopass/secrets/akv_test.go | 49 +++++++++++++++++++++++++++++++ 7 files changed, 112 insertions(+), 2 deletions(-) diff --git a/internal/action/generate.go b/internal/action/generate.go index 5b5fb99c5d..4a0439e061 100644 --- a/internal/action/generate.go +++ b/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) } } diff --git a/internal/action/insert.go b/internal/action/insert.go index 1a2eafa5b7..0dbe579865 100644 --- a/internal/action/insert.go +++ b/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) } diff --git a/pkg/debug/debug.go b/pkg/debug/debug.go index 968cf48d3a..de73051606 100644 --- a/pkg/debug/debug.go +++ b/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 } diff --git a/pkg/debug/debug_test.go b/pkg/debug/debug_test.go index 6b8cd0996a..10856c74d2 100644 --- a/pkg/debug/debug_test.go +++ b/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") diff --git a/pkg/fsutil/fsutil_test.go b/pkg/fsutil/fsutil_test.go index d57ecc93d0..ff113598ab 100644 --- a/pkg/fsutil/fsutil_test.go +++ b/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{ ".": "", diff --git a/pkg/gopass/secrets/akv.go b/pkg/gopass/secrets/akv.go index 0b2294d6da..ae36ffb664 100644 --- a/pkg/gopass/secrets/akv.go +++ b/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 +} diff --git a/pkg/gopass/secrets/akv_test.go b/pkg/gopass/secrets/akv_test.go index ca0cfaad32..da69b7cb02 100644 --- a/pkg/gopass/secrets/akv_test.go +++ b/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()) +} From 33dc0fce352075d7dd7b644f367f28bb34a38703 Mon Sep 17 00:00:00 2001 From: Dominik Schulz Date: Sun, 30 Jul 2023 13:29:33 +0200 Subject: [PATCH 2/4] Fix typo Signed-off-by: Dominik Schulz --- pkg/gopass/secrets/akv.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/gopass/secrets/akv.go b/pkg/gopass/secrets/akv.go index ae36ffb664..90ec72c99a 100644 --- a/pkg/gopass/secrets/akv.go +++ b/pkg/gopass/secrets/akv.go @@ -333,7 +333,7 @@ func (a *AKV) SafeStr() string { // 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 +// multiple chunks but once the first line has been completed any writes to this // writer will be silently discarded. type pwWriter struct { a *AKV From dbf3efaf86e3f4f4aa534f47315e05e3f98d567b Mon Sep 17 00:00:00 2001 From: Dominik Schulz Date: Sun, 30 Jul 2023 18:43:36 +0200 Subject: [PATCH 3/4] Ditch the MultiWriter approach in favor of a pass-through writer Signed-off-by: Dominik Schulz --- pkg/gopass/secrets/akv.go | 36 ++++++++++++++++++---------------- pkg/gopass/secrets/akv_test.go | 15 +++++++++++++- 2 files changed, 33 insertions(+), 18 deletions(-) diff --git a/pkg/gopass/secrets/akv.go b/pkg/gopass/secrets/akv.go index 90ec72c99a..8fcfb302a2 100644 --- a/pkg/gopass/secrets/akv.go +++ b/pkg/gopass/secrets/akv.go @@ -308,14 +308,15 @@ func (a *AKV) Write(buf []byte) (int, error) { var w io.Writer w = &a.raw - // If the body is empty before writing we must threat the first line + // If the body is empty before writing we must treat 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. + // and similar operations (like insert from stdin) might 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}) + w = &pwWriter{w: &a.raw, cb: func(pw string) { a.password = pw }} } return w.Write(buf) @@ -332,30 +333,31 @@ func (a *AKV) SafeStr() string { } // 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 +// then write it to the password field of the provided callback. The first line can stretch // multiple chunks but once the first line has been completed any writes to this // writer will be silently discarded. type pwWriter struct { - a *AKV - buf strings.Builder - written bool + w io.Writer + cb func(string) + buf strings.Builder } func (p *pwWriter) Write(buf []byte) (int, error) { - n := len(buf) - if p.written || p.a == nil { - return n, nil + n, err := p.w.Write(buf) + if p.cb == nil { + return n, err } 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.cb(p.buf.String()) + p.cb = nil + + return n, err } p.buf.Write(buf) - return n, nil + return n, err } diff --git a/pkg/gopass/secrets/akv_test.go b/pkg/gopass/secrets/akv_test.go index da69b7cb02..c663703b7f 100644 --- a/pkg/gopass/secrets/akv_test.go +++ b/pkg/gopass/secrets/akv_test.go @@ -344,7 +344,7 @@ func FuzzParseAKV(f *testing.F) { func TestPwWriter(t *testing.T) { a := NewAKV() - p := pwWriter{a: a} + p := pwWriter{w: &a.raw, cb: func(pw string) { a.password = pw }} // multi-chunk passwords are supported _, err := p.Write([]byte("foo")) @@ -358,4 +358,17 @@ func TestPwWriter(t *testing.T) { assert.NoError(t, err) assert.Equal(t, "foobar", a.Password()) + assert.Equal(t, "baz\n", a.Body()) +} + +func TestInvalidPwWriter(t *testing.T) { + defer func() { + r := recover() + assert.NotNil(t, r) + }() + p := pwWriter{} + + // will panic because the writer is nil + _, err := p.Write([]byte("foo")) + assert.Error(t, err) } From a2720a03bb7bfcbf5b51f47b78a6bf0d25601c5c Mon Sep 17 00:00:00 2001 From: Dominik Schulz Date: Sun, 30 Jul 2023 18:46:49 +0200 Subject: [PATCH 4/4] Format Signed-off-by: Dominik Schulz --- pkg/gitconfig/config_test.go | 2 +- pkg/pwgen/pwrules/pwrules_gen.go | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/gitconfig/config_test.go b/pkg/gitconfig/config_test.go index 8608eb5ee7..f3656a1f9a 100644 --- a/pkg/gitconfig/config_test.go +++ b/pkg/gitconfig/config_test.go @@ -3,8 +3,8 @@ package gitconfig import ( "bytes" "fmt" - "os" "math/rand" + "os" "path/filepath" "strconv" "strings" diff --git a/pkg/pwgen/pwrules/pwrules_gen.go b/pkg/pwgen/pwrules/pwrules_gen.go index 96bc5c3104..3c52285fc2 100644 --- a/pkg/pwgen/pwrules/pwrules_gen.go +++ b/pkg/pwgen/pwrules/pwrules_gen.go @@ -8,7 +8,6 @@ // https://raw.githubusercontent.com/apple/password-manager-resources/main/quirks/change-password-URLs.json // // https://raw.githubusercontent.com/apple/password-manager-resources/main/quirks/password-rules.json -// package pwrules var genAliases = map[string][]string{