Correctly handle new multiline secrets #2625
Conversation
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 <dominik.schulz@gauner.org>
There was a problem hiding this comment.
I'm surprised we need such a "complex" solution, I would have expected removing the newline in NewAKV would already solve the multi-line issue, but I guess you tried it and it had side effects.
Just a few comments around the Write returns, since usually we'd return the number of bytes actually written.
pkg/gopass/secrets/akv.go
Outdated
| func (p *pwWriter) Write(buf []byte) (int, error) { | ||
| n := len(buf) | ||
| if p.written || p.a == nil { | ||
| return n, nil |
There was a problem hiding this comment.
| return n, nil | |
| return 0, nil |
Since no writes are done in that case, no?
There was a problem hiding this comment.
Actually this did raise some questions with me that I didn't have a good answer for and I ditched the MultiWriter approach for a pass-through wirter. I think that's easier to reason about.
| _, err = p.Write([]byte("baz\n")) | ||
| assert.NoError(t, err) | ||
|
|
||
| assert.Equal(t, "foobar", a.Password()) |
There was a problem hiding this comment.
Shouldn't the test also test the content of Body?
There was a problem hiding this comment.
Yes, that's more comprehensive.
pkg/gopass/secrets/akv.go
Outdated
|
|
||
| // 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 |
There was a problem hiding this comment.
I don't understand what you mean by "and similar operations will fail"?
There was a problem hiding this comment.
Like insert from STDIN. That should be affected here as well. Updated the comment.
pkg/gopass/secrets/akv.go
Outdated
| p.a.password = p.buf.String() | ||
| p.written = true | ||
|
|
||
| return n, nil |
There was a problem hiding this comment.
Shouldn't this be
| return n, nil | |
| return i, nil |
Since it only writes the pw?
There was a problem hiding this comment.
See my comment above. I think the approach was not great.
|
I did try the simpler approach first, indeed. It had side-effects for some of our tests. Unfortunately. I'll address the other comments later. |
Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>
| @@ -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) | |||
There was a problem hiding this comment.
Note that this will leak OTP codes in the debug logs, I think.
We should not be logging any of otp, topt, hotp and otpauth keys, as well as any key that satisfies action.isUnsafeKey I think.
There was a problem hiding this comment.
these should be safeString types, so they shouldn't leak I think?
But if they do we can remove these.
| @@ -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) | |||
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