Em Mon, Mar 17, 2014 at 10:29:12AM +0100, intrigeri escreveu: > Silvio Rhatto wrote (14 Mar 2014 20:43:23 GMT) : > > I am looking for a sponsor for release 0.3.2-1 of "keyringer". > > Great! Here's a quick code review of the changes since 0.2.9. Thanks for the review :) > I'm not convinced that the key expiry check does not introduce more > severe regressions than it improves the UX. Let's see: > > * Does it support the case when I have multiple keys for a given > recipient in my keyring, and e.g. one is expired, but the one that > should be used is not? Yes. Iteration happens per key fingerprint and not by uid address according to what is defined on each keyring recipient file. > * It seems that all subkeys (regardless of their intended usage) must > *not* be expired, else keyringer errors out. This won't work with > people who change their encryption subkey on a regular basis, > will it? I don't think it will, except if there's a bug in the code. At least one subkey must not be expired. > * Why try to check this ourselves at all, while GnuPG will do it > anyway (and likely in a more correct way)? While GnuPG already has expiration checks, it doesn't have any feature to warn users that they key will expire in advance. One application for "keyringer <keyring> check" would be a cronjob with it's output piped to an email to help a group of recipients make sure that all keys are ready to use. In my experience with OpenPGP it's very frustrating to fail encrypting to an expired recipient. Sometimes it's not just the case to refresh keys as some people tend to forget to renew, create new keys or keep their key expirations updated. > > + candidates=(`keyringer_exec find "$BASEDIR" | grep -i "$1" | grep -e '.asc$'`) > This seems fragile to me. How about passing find something like > '"-name "*$i*.asc"' instead? > > Same concerns in lib/keyringer/actions/find. I changed find action to use "-iname" on commit c77565e. > > + echo "Could not find exact match \"$1\", please chose one of the following secrets:" > > s/exact match/exact match for/ > s/chose/choose/ > > > +Alis to clip action. > >+: Alis to clip action. > > s/Alis/Alias/ > > > +If you're using debian `jessie` or `unstable`, just run > > s/debian/Debian/ > > > +# If you run it more than that interval, no canary will > > +# be updated. > > "more than once in this time span", maybe? > > > +# - Can optinally be included in another git repository > > +# (encrypted or plain+signed), commited and pushed > > Ask your preferred spell checker :) > > > +# - Then, add the following at your ~/.profile or wherever you want your canary > > +# be called from: "keyringer <keyring> canary". > > s/at your/to your/ All fixed, thanks :) > It's unclear to me what "calling a canary" means in this context. > Clarify? It's out of context: the canary code was moved to a custom branch and is not part of the current release. But yeah, it's poorly redacted and that should be fixed. > In a few places, s/GPG/GnuPG/ > > > + echo "Please check for this key or fix the recipient file." > > s/check for/retrieve this key yourself/, maybe? Done. > > + gpg --batch --refresh-keys "$recipient" > > I'm not sure this really does what you mean here. At least the GnuPG > manpage does not talk about it. Perhaps use --recv-keys for > better clarity? > > > + keyringer_exec git "$BASEDIR" gc --prune=all > > Potentially losing user's data feels quite wrong, in an action called > "check", and documented as "Run maintenance checks in a keyring". I removed --prune. > > + echo "Fatal: please set the full GPG signature hash for key ID $recipient:" > > I'm afraid "GPG signature hash" is unclear to most people. > Just say "OpenPGP fingerprint" instead? Fixed. I'll release a new version once there's no remaining issues in this thread. -- rhatto at riseup.net pubkey 66CA01CE2BF2C9B7E8D64E340546823964E39FCA please bring your fingerprint when meeting me
Attachment:
signature.asc
Description: Digital signature