GNU bug report logs - #36052
26.2.50; [PATCH] Improve auth-source-pass

Previous Next

Full log

View this message in rfc822 format

From: Noam Postavsky <npostavs <at> gmail.com>
To: Damien Cassou <damien <at> cassou.me>
Cc: Magnus Henoch <magnus.henoch <at> gmail.com>, Nicolas Petton <nicolas <at> petton.fr>, Ted Zlatanov <tzz <at> lifelogs.com>, Iku Iwasa <iku.iwasa <at> gmail.com>, galaunay <gaby.launay <at> tutanota.com>, 36052 <at> debbugs.gnu.org, Keith Amidon <camalot <at> picnicpark.org>
Subject: bug#36052: 26.2.50; [PATCH] Improve auth-source-pass
Date: Thu, 06 Jun 2019 20:43:06 -0400

Damien Cassou <damien <at> cassou.me> writes:

> Magnus Henoch hasn't signed but his contribution is rather small (4
> lines of a new unit-test and 2 lines of code). See patch 0001.

His patch needs the line

    Copyright-paperwork-exempt: yes

> Subject: [PATCH 04/12] Add auth-source-pass-path option

I think auth-source-pass-filename would be the correct name to conform
with GNU naming conventions.

> Subject: [PATCH 07/12] Split out the attribute retrieval form
>  auth-source-pass-get
>
> Eliminate the need to repeatedly retrieve and parse the data for the
> entry. This is generally a good thing since it eliminates repetitions
> of the same crypto and parsing operations. It is especially valuable
> when protecting an entry with a yubikey with touch required for crypto
> operations as it eliminates the need to touch the yubikey sensor for
> each attribute retrieved.

Missing double spacing at end of sentence here.

> +(defun auth-source-pass--get-attr (key entry-data)
> +  "Return the value associated to KEY in data from an already parsed entry.
> +
> +ENTRY-DATA is the data from a parsed password-store entry.
> +The key used to retrieve the password is the symbol `secret'.
> +
> +The convention used as the format for a password-store file is
> +the following (see http://www.passwordstore.org/#organization):
> +
> +secret
> +key1: value1
> +key2: value2"

I think the end of the docstring could be replaced with

    See `auth-source-pass-get'.

It seems a little silly to duplicate this info so close by.

> Subject: [PATCH 08/12] Minimize entry parsing in auth-source-pass
>
> Prior to this commit, while searching for the most applicable entry
> password-store entries were decrypted and parsed to ensure they were
> valid. The entries were parsed in the order they were found on the
> filesystem and all applicable entries would be decrypted and parsed,
> which varied based on the contents of the password-store and the entry
> to be found.
>
> This is fine when the GPG key is cached and each entry can be
> decrypted without user interaction. However, for security some people
> have their GPG on a hardware token like a Yubikey setup so that they
> have to touch a sensor on the toke for every cryptographic operation,
> in which case it becomes inconvenient as each attempt to find an entry
> requires a variable number of touches of the hardware token.
>
> The implementation already assumes that names which contain more of
> the information in the search key should be preferred so there is an
> ordering of preference of applicable entries. If the decrypt and
> parsing is removed from the initial identification of applicable
> entries in the store then in most cases a single decrypt and parse of
> the most preferred entry will suffice, improving the experience for
> hardware token users that require interaction with the token.
>
> This commit implements that strategy. It is in spirit a refactor of
> the existing code. The core of the change is the function
> auth-source-pass--applicable-entries, which generates an ordered list
> of regular expression matchers for all possible names that could be in
> the password-store for the entry to be found and then makes a pass
> over the password-store entry names accumulating the matching entries
> in a list after the regexp that matched. This implementation ensures
> the password-store entry list still only has to be scanned once.
>
> The existing auth-source-pass--find-match-unambiguous was modified to
> use this new function to obtain candidate entries and then parse them
> one by one until an entry containing the desired information is
> located. When complete it now returns the parsed data of the entry
> instead of the entry name so that the information can be used directly
> to construct the auth-source response.
>
> * lisp/auth-source-pass.el: Private functions were refactored to
> reduce the number of decryption operations.

Double spacing, and this ChangeLog entry is a little sparse.  It looks
like the last two prose paragraphs could be easily made into ChangeLog
entries, since they're already talking about specific functions.

> +  (when (> (length name-components) 0)
> +    (cons (mapconcat 'identity name-components ".")
> +          (auth-source-pass--domains (cdr name-components)))))

I suggest instead:

    (cl-maplist (lambda (components) (mapconcat #'identity components "."))
                name-components)

> Subject: [PATCH 10/12] Refactoring of auth-source-pass
>
> * lisp/auth-source-pass.el: Refactoring.

This one's a little empty too.

> Subject: [PATCH 12/12] * etc/NEWS: Describe changes to auth-source-pass
>
> ---
>  etc/NEWS | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/etc/NEWS b/etc/NEWS
> index 975fab495a..5dcdac2668 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -1485,6 +1485,25 @@ the new variable 'buffer-auto-revert-by-notification' to a non-nil
>  value.  Auto Revert mode can use this information to avoid polling the
>  buffer periodically when 'auto-revert-avoid-polling' is non-nil.
>  
> +** auth-source-pass
> +
> +*** New customizable variable 'auth-source-pass-path' for the path to
> +the password-store.  This defaults to ~/.password-store.

It's better to have NEWS entries have the first sentence in one line.
Something like

    *** New customizable variable 'auth-source-pass-path'.
    Allows setting the path to the password-store, defaults to ~/.password-store.

> +*** New customizable variable 'auth-source-pass-port-separator' to
> +specify separator between host and port.  This defaults to colon
> +":".

    *** New customizable variable 'auth-source-pass-port-separator'.
    Specifies separator between host and port, defaults to colon ":".

> +*** auth-source-pass.el and auth-source-pass-tests.el have been
> +massively rewritten to minimize parsing of password-store entries.
> +This makes the package usable with physical tokens requiring touching
> +a sensor for every decryption.

This one puts too much emphasis on the rewrite which is an
implementation detail.

   *** Minimize the number of decryptions during password lookup.
   This makes the package usable with physical tokens requiring touching
   a sensor for every decryption.
   

> +*** 'auth-source-pass-get' has an autoload cookie now.

Maybe just say "is now autoloaded".

> +*** 'auth-source-pass-search' now correctly returns nil if no entry
> +found.

We don't put bug fixes in NEWS, so this one can be left out.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.