gopasspw / gopass

The slightly more awesome standard unix password manager for teams
https://www.gopass.pw/
MIT License
5.86k stars 491 forks source link

summon integration does not work for secrets with additional information atttached #1436

Closed ckolumbus closed 4 years ago

ckolumbus commented 4 years ago

Summary

using gopass (1.9.2) as as summon provider (via linking the gopass executable into the summon provider directory) gopass outputs the whole secret content when queried instead of only the secret, as required by the protocol

Steps To Reproduce

gopass init gopass new

Creating generic secret ...
[1] Name                          []: test
[2] Generate password?            [Y/n/q]: 
[a] Human-pronounceable passphrase? (see https://xkcd.com/936/) [y/N/q]: 
[b] How long?                   [24]: 
[c] Include symbols?            [y/N/q]: 
[d] Strict rules?               [y/N/q]: 
[3] Enter zero or more key value pairs for this secret:
[a] Name (enter to quit)        []: foo
[b] Value for Key 'foo'         []: bar
[a] Name (enter to quit)        []: 

summon -p gopass --yaml 'X : !var misc/test' bash -c 'echo $X'

GxAicJQrGAA534Lo5wi5eByZ foo: bar

Expected behavior

expected output: one line with the secret

GxAicJQrGAA534Lo5wi5eByZ

Environment

ckolumbus commented 4 years ago

As addition: i can provide a PR but wanted to discuss the proper/expected behavior before implementing anything.

Options from the top of my head:

other ideas? Feedback?

dominikschulz commented 4 years ago

I think I'd like option #3 the most. This would give the greates isolation from potentially breaking changes in the gopass cli interface (expect some more). The new API - while still not fully finished - should allow a good integration point.

Option #2 would be OK, but eventually I'd like to reduce special cases in the main gopass binary, so this would be only a temporary measure.

The problem with option #1 is that it also affects all others users and use-cases and the list of (mostly undocumented) features and interactions between the different switches is already super confusing. So I'll accept changes to that only if it comes with a well written feature / flag matrix documentation.

ckolumbus commented 4 years ago

Option 3 it is :+1: Thanks for the quick feedback

ckolumbus commented 4 years ago

one more question: gopass show has a parameter

--password, -o    Display only the password (default: false)

executing gopass show -o misc/test yields the expected output

GxAicJQrGAA534Lo5wi5eByZ

but performing a search with gopass show -o test provides the full secret output.

Entry 'test' not found. Starting search...
Found exact match in 'misc/test'
GxAicJQrGAA534Lo5wi5eByZ
foo: bar

is this the expected behavior for show -o?

ckolumbus commented 4 years ago

@dominikschulz can you have a quick look at the implementation in the PR #1438 whether it's going into the right direction? I also have some problems with identifying how to unit test this (i.e. setting the Args parameter for the action context). Some advice on where to look for good examples would be appreciated.

I looked at the show_test.go but I'm not sure whether the whole action implementation is adequate for the simple show functionality within summon-provider.go

I modeled the implementation according to git-credential.go, but the test there assumestdin input andno args.

Sorry, I'm not a go master.

dominikschulz commented 4 years ago

gopass show -o: These should yield the same results, but likely the plumbing through of the -o flag to the search fallback is broken. This should be fixed.

Thanks for that PR, I left some comments.

marco-m commented 4 years ago

In my understanding since #1438 has been merged, this ticket can be closed :-)

dominikschulz commented 4 years ago

Yes, indeed. Thanks!