[PATCH consfigurator v2 4/6] introduce the pass data store.

[Sean Whitton]

Hello,

On Wed 09 Mar 2022 at 08:28pm -04, David Bremner wrote:

> diff --git a/src/data/pass.lisp b/src/data/pass.lisp
> new file mode 100644
> index 0000000..e216c76
> --- /dev/null
> +++ b/src/data/pass.lisp
> +
> +(defun %read-gpg (location)
> +  (stripln (gpg-file-as-string location)))
> +
> +(defun %strip-prefix (prefix string)
> +  "If STRING is prefixed by PREFIX, return the rest of STRING,
> +otherwise return NIL."
> +  (nth-value 1 (starts-with-subseq prefix string :return-suffix t)))

Doesn't need changing, but I think you prefix function names with '%' a
bit too readily.  The convention, as I understand it, is to add the '%'
for functions that are not just internal but internal and dangerous, or
to distinguish an internal function from an external one of the same
name (e.g. %CONSFIGURE and CONSFIGURE).  And in Consfigurator in
particular it is used for defprops not intended to ever be added to
hosts directly.  So simple utilities like %STRIP-PREFIX and %GPG-FILE-P
probably wouldn't be candidates.

> +
> +(defmethod register-data-source ((type (eql :pass))
> +                                 &key (location "~/.password-store"))
> +  "Provide the contents of a pass(1) store on the machine running the root
> +Lisp.  Register this data source multiple times to provide multiple stores.
> +
> +LOCATION specifies the root of the password store.
> +
> +LOCATION, IDEN1, and IDEN2 are combined to locate a file in the password
> +store.

I think I'd say 'concatenated' or 'joined' instead of just 'combined'.

> +When retrieving a password, IDEN1 should be specified as `--user-passwd-HOST'.
> +where HOST should be a valid hostname, and IDEN2 a username. Otherwise IDEN1
> +should be a valid hostname, unless prefixed by `_'. In the latter case, the
> +path without the corresponding `_' is searched if the prefixed path is not
> +found."

This is a bit difficult to read if you weren't familiar with our recent
discussions, I think.  Maybe something like "For retrieving user account
passwords, IDEN1 can be ... or ... and IDEN2 the username.  Otherwise,
IDEN1 should begin with '_' (see the ... section of the Consfigurator
user's manual).  In the latter case, if the concatenated path does not
exist in the password store then the search is tried again after
dropping the '_'.  This means that [description of usecase].  Other
forms for IDEN1 are not supported by this data source."

> +  (let ((base-path (ensure-directory-pathname location)))
> +    (unless (directory-exists-p base-path)
> +      (missing-data-source
> +       "~A does not exist, or is not a directory." base-path))
> +    (labels
> +        ((%gpg-file-p (iden1 iden2)
> +           (file-exists-p
> +            (literal-data-pathname base-path iden1 iden2 :type "gpg")))
> +         (%make-path (iden1 iden2)
> +           (acond
> +             ((%strip-prefix "--user-passwd-" iden1)
> +              (and (valid-hostname-p it) (%gpg-file-p it iden2)))
> +             ((%strip-prefix "_" iden1)
> +              (or (%gpg-file-p iden1 iden2) (%gpg-file-p it iden2)))
> +             (t
> +              (and (valid-hostname-p iden1) (%gpg-file-p iden1 iden2)))))

I like this a lot!

> +         (check (iden1 iden2)
> +           (when-let ((file-path (%make-path iden1 iden2)))
> +             (file-write-date file-path)))
> +         (extract (iden1 iden2)
> +           (when-let ((file-path (%make-path iden1 iden2)))
> +             (make-instance 'string-data
> +                            :string (%read-gpg file-path)

I think I'd be inclined to inline %READ-GPG as this is the only use, but
up to you.

-- 
Sean Whitton