browserpass / browserpass-native

Browserpass native client app
ISC License
394 stars 50 forks source link

Support for passage #127

Open sebastianblunt opened 2 years ago

sebastianblunt commented 2 years ago

Does browserpass want to add support for passage?

Passage is mostly similar to pass. The only difference is the actual data files are put in an extra subdirectory in the store (~/.passage/store), the file extension is different, and the command to decrypt is different. I fairly easily managed to get browserpass working with passage just by hacking the host application (https://github.com/sebastianblunt/browserpass-native/commit/dd761eafcabe4a0c1d26d4dabb9bf99c8e3ba48d).

For proper support, I figure we could have two types of custom stores. Add an extra kind field to the store configuration (with a value of either gpg or age, if field is missing it defaults to gpg). Then just add an extra global option agePath (corresponding to gpgPath). Keep the default store using gpg and require passage users to set up a custom age store.

Any thoughts? Would PRs for this be accepted?

maximbaz commented 2 years ago

Hello! Thanks for the link, interesting project!

An honest answer is that I don't know whether we want to support it, we did design browserpass with gpg in mind (including relying for example on gpg infrastructure for entering (pinentry) and caching (gpg-agent) master password, or supporting hardware tokens and smartcards (pcscd)) which significantly reduces dependencies and work for us.

I do agree that there's no doubt that it's possible to introduce the support without breaking changes, I'm just wondering if we should first see more interest from the community to use age before doing so.

Finally if we go ahead with this, it is worth to consider generalizing this entire code instead of having hardcoded support for two specific tools - we are striving to have as few releases for browserpass-native as possible, because the upgrade process is often manual and annoying, compared to browser extension.

sebastianblunt commented 2 years ago

Makes sense.

To generalize it, as it works now, we would only really need to make the decryption arguments and the file extension configurable, in addition to the already configurable decryption program and store path.

One thing to consider is that making the arguments fully configurable would mean giving the browser extension (or anything capable of writing to the extension's local storage) arbitrary code execution capabilities.

Another option could be to support configuring a passPath instead of the host application using gpg (or age) directly. I.e. the host application would call pass foo to perform the decryption rather than calling out to gpg.

x70b1 commented 2 years ago

gopass added passage support already: https://github.com/gopasspw/gopass/issues/2059.

uninsane commented 2 years ago

i'm about to apply a similar patch as Sebastian. my own store uses Mozilla's sops infrastructure, where passwords are encoded to an age key derived from my ssh key.

browserpass' gpgAgent is already configurable, so i plan to just point that at a shim around sops. the only thing i expect to patch out is the part where secret listing globs on *.gpg and secret decryption enforces that file extension.

anyway, count me in for both:

I'm just wondering if we should first see more interest from the community to use age before doing so.

and, more preferably,

if we go ahead with this, it is worth to consider generalizing this entire code instead of having hardcoded support for two specific tools.

in the meantime, is there any pluggable browser password filler i should look into? or is my best bet to locally patch browserpass?

edit: here's my hack to get browserpass speaking to sops (ignore nix.flake and nix.lock). i remove the .gpg extension enforcement and then outside the package i inject a gpg-lookalike on the PATH which just calls into sops.

maximbaz commented 2 years ago

Thanks to all of you who express support for age and share your usecases, I feel like I'm sold on the idea to generalize browserpass in terms of extensions and decryption commands.

Is anyone up for creating a PR? I think we can strive for not breaking existing setups and at the same time allow configuring what is necessary to make configurable to support age, without hardcoding things for new tools.

We tend to prefer to release native component as less frequent as possible, and there's already another PR in active development which requires changes in native host, it would be ideal to include the support for age in the same version, i.e. in the next release.

sebastianblunt commented 2 years ago

One thought, would we want it to be globally configured as the gpgPath is now, or per-store? I could imagine some people may want to run different kinds of stores simultaneously.

maximbaz commented 2 years ago

Per-store makes sense, in fact I can see an inconsistency in this repo docs and in extension docs, where the latter seems to suggest it already is per store. I can't recall at the moment what is right, but we should definitely align the docs.

uninsane commented 2 years ago

Is anyone up for creating a PR?

@maximbaz i could whip something up that introduces some configurable interface between browserpass-native and the thing actually listing/fetching passwords. to do this right i think i'll also have to make a small (hopefully) change to the browserpass-extension so that it doesn't forcibly append .gpg to the password filename before passing it into the native component.

i'd be inclined to introduce two user-facing settings: one which names a command to run for listing secrets ("list_command"), and another to load them from disk and decrypt them ("fetch_command"). browserpass-native would just exec.Command these, passing in the store path and secret path where appropriate. the existing gpg implementation would be split out into some browserpass-gpg binary (could be just a shell script) that would be shipped alongside the existing browserpass binary and browserpass-gpg list ..., browserpass-gpg fetch ... would be invoked by default when list_command or fetch_command are unset. i'd leave the actual passage implementation to someone else (with the goal that that part's easy after this work).

i can hammer that out & we reconcile the minor stuff during PR time if that sounds reasonable enough, or we can hash out the details in advance if you're particular about this. is there some chatroom you or other devs around here use to coordinate?

maximbaz commented 2 years ago

Thanks for your offer and willingness to help!

Is there some chatroom you or other devs around here use to coordinate?

This would be the best place I think 👍

to do this right i think i'll also have to make a small (hopefully) change to the browserpass-extension so that it doesn't forcibly append .gpg to the password filename before passing it into the native component.

Correct, although it shouldn't be too hard, the real "origin" of where the file extension is being hardcoded to .gpg is actually in the native host in list request, the extension could easily determine the correct file suffix from the list response, if that's even necessary (more on this below).

i'd be inclined to introduce two user-facing settings: one which names a command to run for listing secrets ("list_command"), and another to load them from disk and decrypt them ("fetch_command")

We should also at least think about tree/save/delete requests, although that PR has been sitting way too long in the draft state, it's a requirement to complete https://github.com/browserpass/browserpass-extension/pull/290, so we will have to finalize it anyway before the release.

the existing gpg implementation would be split out into some browserpass-gpg binary (could be just a shell script) that would be shipped alongside the existing browserpass binary

I think it's a reasonable proposal, I have some initial suggestions, none seem to be conflicting with your vision:

  1. Instead of having a way to configure a file extension, we could make the list return all files, not filtered by any extension (for most people this would probably return some extra hidden files, and we can hide those in popup, as well as files without extension).
  2. Perhaps just like you had in mind, we could make the fetch look up the file extension of the file that the browser wants decrypted, and exec.Command a binary called browserpass-<file-extension> from $PATH or folder where browserpass binary is stored.
  3. Just as you propose, we can move all gpg-related code into browserpass-gpg binary, since gpg is going to remain the mainly supported tool which we ship by default. It might be easier to compile a second Go binary rather than creating a shell script (there's a code pattern of doing this in Go), both because shell script would cause problems on Windows, and also because we have all the code already written in Go, it should just be a matter of importing and calling it in another place.
  4. Some people have gpgPath configured, we should find a way to migrate this config, say to convert it to fetch_command=browserpass-gpg fetch --bin /custom/path/to/gpg or something like that. We can furthermore document that fetch subcommand receives for example two positional arguments (store and file paths), and there might be zero or more optional arguments (like --bin) which browserpass-age might want to simply ignore.

I'm not sure if we necessarily want browserpass-age implementation in this repo (since it isn't tested / vetted by us, since we don't want to tag new releases in this repo too often when that script is getting updated, etc.), but I am open to have my mind changed, and/or to explore creating contrib/ folder or even a browserpass-contrib repo, if there is such desire 🙂

cc @erayd, if you want to share some thoughts on this feature request?

erayd commented 2 years ago

@maximbaz I'm open to it, but would prefer it to happen after the MV3 migration. We're on a deadline for that, which is approaching far too quickly, and this isn't the only extension I need to migrate. Could we revisit this after that is done?

Instead of having a way to configure a file extension, we could make the list return all files, not filtered by any extension (for most people this would probably return some extra hidden files, and we can hide those in popup, as well as files without extension).

Happy with this, but we should allow the extension to pass a list of exclusions (if we don't already; IIRC I think we already made that a feature). Skipping stuff like the .git folder on the host will be a lot faster than traversing it and then having the extension drop it.

Something else to consider is that, if we're allowing more backends, we may want to also allow the host to simply call plugin binaries. Some backends may do things like encrypting file & folder names, and trying to natively handle all that is probably not the greatest idea. Allowing the native host to call out to some other configured binary that will provide listings & content is probably better IMO. Plus that way implementations can update their own stuff independently of our own native host, and still maintain compatibility.

I'm not sure if we necessarily want browserpass-age implementation in this repo...

I agree. Keep it external.

maximbaz commented 2 years ago

Could we revisit this after that is done?

Sure thing 👍

Happy with this, but we should allow the extension to pass a list of exclusions (if we don't already; IIRC I think we already made that a feature). Skipping stuff like the .git folder on the host will be a lot faster than traversing it and then having the extension drop it.

I'm not sure that we have it right now, I think we simply glob recursively for a hardcoded **/*.gpg today... Unless glob library we use already skips hidden folders like .git, it's something to consider 👍

Something else to consider is that, if we're allowing more backends, we may want to also allow the host to simply call plugin binaries.

Yeah! I think it's what we also had in mind with browserpass-gpg binary, native host would call an external plugin binary browserpass-gpg list to provide listing, and for age it would similarly call browserpass-age list.

Although now that I think about, my idea to avoid having to configure file extension and instead extract it from file entry wouldn't work, as we'd have to know the extension already on the list stage (to handle encrypted folder names, as you mentioned) 🤔. Oh well, I guess there's no way around adding a new configuration for the encryption method (like gpg or age), which would both be used to decide which binary plugin to execute.