[PATCH consfigurator 3/3] introduce the pass data store.

[Sean Whitton]

Hello,

On Sat 19 Feb 2022 at 03:43pm -04, David Bremner wrote:

> In this initial version, only IDEN1 of the form '--user-passwd-HOST' is
> supported.
> ---
>  consfigurator.asd  |  1 +
>  src/data/pass.lisp | 60 ++++++++++++++++++++++++++++++++++++++++++++++
>  src/package.lisp   |  3 +++
>  3 files changed, 64 insertions(+)
>  create mode 100644 src/data/pass.lisp
>
> diff --git a/src/data/pass.lisp b/src/data/pass.lisp
> new file mode 100644
> index 0000000..fea1c3a
> --- /dev/null
> +++ b/src/data/pass.lisp
> +(named-readtables:in-readtable :consfigurator)
> +
> +(define-constant =prefix= "--user-passwd-" :test #'equal)

The CL convention for constants is to surround names with '+' not '='.

> +(defun %read-gpg (location)
> +  (string-right-trim
> +   '(#\Return #\Newline)
> +   (util:gpg-file-as-string location)))

There is STRIPLN for this.  Maybe GPG-FILE-AS-STRING should always
STRIPLN what it returns?

> +(defmethod register-data-source ((type (eql :pass)) &key (location "~/.password-store"))

Please reduce the line length -- I'd suggest a newline before the whole
lambda list.  Emacs indents that pretty readably.

> +  "Provide the contents of a pass(1)  on the machine running the root

... a pass(1) store ...

> +Lisp.  Register this data source multiple times to provide multiple stores.
> +
> +LOCATION specifies the root of the password store.
> +
> +When retrieving a password, IDEN1 should be specified as `--user-passwd-HOST'
> +and IDEN2 as username. HOST does not need to be an actual hostname and is
> +interpreted as a directory in the password store. In particular HOST may
> +contain one or more occurences of `/'."

Previously I had been thinking that it would be okay for
--user-passwd-HOST,USER to be primary in some sense, such that
iden1,iden2 pairs not beginning with '--' would get converted.  The idea
would be that pass(1) is primarily about storing username/password
pairs.  But requiring an explicit --user-passwd- is likely better for
extensibility, so let's stick with that.

I'm wary of letting HOST not be a hostname.  It breaks with the
documented convention, and might clash with possible extensions of this
data source.  How about for extensibility safety, we validate that HOST
is an actual hostname and otherwise return nil?

> +  (let ((base-path (ensure-directory-pathname (uiop:native-namestring location))))

I think we should import NATIVE-NAMESTRING into CONSFIGURATOR, like we
already do for UNIX-NAMESTRING, if it is to be used here.  However, I
don't think this code can be correct, because you're converting to a
namestring and then immediately back into a pathname.  Does
(ensure-directory-pathname location) not produce the same results?

> +    (unless (directory-exists-p base-path)
> +      (missing-data-source
> +       "~A does not exist, or is not a directory." base-path))
> +    (labels ((%make-path (iden1 iden2)
> +               (when-let ((host (and (string-prefix-p =prefix= iden1)
> +                                     (subseq iden1 (length =prefix=)))))
> +                 (util:data-file-path base-path host iden2 :type "gpg")))

There's this Lisp convention to use WHEN for side effects and AND for
returning values.  Additionally, SUBSEQ can never return nil.  So I find
this a bit tricky to read.  How about

    (and (string-prefix-p +prefix+ iden1)
         (let ((host (subseq iden1 (length +prefix+))))
           (data-file-path base-path host iden2 :type "gpg")))

though I'd be inclined to inline HOST.

> +             (check (iden1 iden2)
> +               (when-let ((file-path (%make-path iden1 iden2)))
> +                 (informat 4 "~&checking pass ~a ~a ~a" iden1 iden2 file-path)
> +                 (and (file-exists-p file-path)
> +                      (file-write-date file-path))))
> +             (extract (iden1 iden2)
> +               (when-let ((file-path (%make-path iden1 iden2)))
> +                 (informat 4 "~&extracting from pass ~a ~a ~a" iden1 iden2 file-path)
> +                 (and (file-exists-p file-path)
> +                      (make-instance 'string-data
> +                                     :string (%read-gpg file-path)
> +                                     :iden1 iden1
> +                                     :iden2 iden2
> +                                     :version (file-write-date file-path))))))

I find these uses of WHEN-LET fine fwiw.  There are some quite long
lines here though; can you reflow a bit, please?

> diff --git a/src/package.lisp b/src/package.lisp
> index e8464a7..c4d834e 100644
> --- a/src/package.lisp
> +++ b/src/package.lisp
> @@ -1022,5 +1022,8 @@
>
>    (package :consfigurator.data.local-file)
>
> +  (package :consfigurator.data.pass
> +           (:local-nicknames (#:util  #:consfigurator.data.util)))
> +
>    (package :consfigurator.data.files-tree
>             (:local-nicknames (#:util  #:consfigurator.data.util))))

Same comment about the UTIL nickname.

-- 
Sean Whitton