[pass] [PATCH v2] Team pass: enable multiple keys and per directory

[Jason A. Donenfeld]

On Wed, Mar 19, 2014 at 5:54 PM, Matthew Cengia <mattcen at gmail.com> wrote:
>
> I'm going to be fairly brutal in the following code review, so feel free
> to take my advice or leave it. I realise that some of the code I'm
> commenting on isn't necessarily the focus of this patch


Thanks for doing this; I really appreciate it. If you'd like to read
through the entire pass source
<http://git.zx2c4.com/password-store/tree/src/password-store.sh> and
critique the hell out of it, I'd be even more appreciative.

>
>
> On 2014-03-19 17:04, Jason A. Donenfeld wrote:
> [...]
> > +     if [[ -n $PASSWORD_STORE_KEY ]]; then
> > +             for gpg_id in $PASSWORD_STORE_KEY; do
>
> The above will fail spectacularly if "$PASSWORD_STORE_KEY" contains the
> following:
>
>   PASSWORD_STORE_KEY="Matthew Cengia
>   Jason A. Donenfeld"
>
> because you're splitting it indiscriminantly on whitespace. The least
> bad way I can think of handling this, assuming PASSWORD_STORE_KEY is
> expected to be an environment variable read by pass(1) is to insist that
> each key exists in the variable on a new line, and read it as follows:
>
>   while read -r gpg_id; do
>     gpg_recipient_args+=( "-r" "$gpg_id" )
>   done <<<"$PASSWORD_STORE_KEY"

I thought about this. GPG allows specifying keys by hex ID, though. I
think this is easier to use than having the inconvenience of needing
to use newlines in an environment variable. I could be persuaded
otherwise though.

>
> > +     while read gpg_id; do
>
>   Always use read -r unless you are sure it's not what you want.

Thanks. Applied this to everywhere.

>
> > +                     -p|--path) id_path="$2"; shift; shift ;;
>
> Shift takes an argument: 'shift 2' instead of 'shift; shift;'

Did not know that. Applied.

>
> > +             id_print="$(printf "%s, " "$@" | head -c -2)"
>
> I wouldn't have used head here; I'd instead use "${id_print%, }" below.
>
> > +             git_add_file "$gpg_id" "Set GPG id to ${id_print}."

Good thinking.

>
> GPG id(s), plural?

Meh.

>
> > +                     find "$PREFIX/$id_path" -iname '*.gpg' | while read passfile; do
>
> I realise the code was already like this, and this is an extreme edge
> case, but to guard against people trying to break this by doing dumb
> things like put newlines in filenames:
>
>   find "$PREFIX/$id_path" -iname '*.gpg' -print0 | while IFS= read -rd '' passfile; do

Yea... I don't know, I guess. For whatever reason I'm sort of
hesitant, because most shell situations break when filenames get
newlines, and the overhead for fixing all of these seems quite
cumbersome. But maybe I'm foolish here?

>
> > +                             set_gpg_recipients "$(sed "s:^$PREFIX/::" <<<"$(dirname "$passfile")")"
>
> Again, I'd try to avoid external utilities if it's not too much trouble
> to do so:
>
>   passfile_dir=${passfile%/*} passfile_dir=${passfile_dir#$PREFIX}
>   set_gpg_recipients "$passfile_dir"

Thanks!

>
> > +             set_gpg_recipients "$(dirname "$path")"
>
> Another brace expansion above.

I use dirname a lot. I suppose I *could* get rid of these as you've
suggested. Would appreciate a patch I could apply for this. Or I can
just do it myself.

>
> >                       read -r -p "Enter password for $path: " -e password
> > -                     gpg2 -e -r "$ID" -o "$passfile" $GPG_OPTS <<<"$password"
> > +                     gpg2 -e "${gpg_recipient_args[@]}" -o "$passfile" $GPG_OPTS <<<"$password"
>
> Um... reading a password with read, and echoing it to the terminal?
> Shouldn't this use 'read -s'?

There are two modes for inserting a password. The default uses -s. If
you explicitly ask, though, it takes this path.

>
> I could probably keep going, as there's always more to complain about,
> but that's a reasonable first pass in my opinion, and I've got real work
> to do.

Please do keep going! Thanks for your comments.