bug#72441: 31.0.50; Auth-source-pass doesn't match password attributes or hosts without user when extra-query-keywords is true

["J.P." <jp@neverwas.me>]

[-- Attachment #1: Type: text/plain, Size: 8474 bytes --]

Björn Bidar via "Bug reports for GNU Emacs, the Swiss army knife of text
editors" <bug-gnu-emacs@gnu.org> writes:

> I noticed that auth-source-pass doesn't match agains password file
> attributes such as those containing :user and only matches password
> files which contain a host and a user when
> auth-source-pass-extra-query-keywords is true.

I don't use pass myself, nor have I ever, but I suppose I did add the
`auth-source-pass-extra-query-keywords' option (though mainly in a bid
to make auth-source-pass behave more like other back ends so it's usable
with ERC). Anyway, I do actually recall being somewhat aware of the
existence of the file attributes you mention. It seems I even left a
comment about the current lack of support [1].

Looking into this a bit, it seems the password store's web page
considers everything after the initial (password) line to be an opaque
text blob:

  The password store does not impose any particular schema or type of
  organization of your data, as it is simply a flat text file, which can
  contain arbitrary data. Though the most common case is storing a
  single password per entry, some power users find they would like to
  store more than just their password inside the password store, and
  additionally store answers to secret questions, website URLs, and
  other sensitive information or metadata. Since the password store does
  not impose a scheme of it's own, you can choose your own organization.
  There are many possibilities.

However, I do realize that the auth-source-pass back end without the
extra-keywords option already dips into a file's contents looking for an
attributes list like the one shown on the web page. (Whether that's wise
is pretty much moot after all these years.) Anyway, for that reason, I
suppose we _should_ attempt to at least explore doing the same when the
extra-keywords option is enabled. For me, the most important thing
remains mimicking the behavior of the other built-in back ends, which at
times is admittedly unintuitive but nevertheless consistent and thus
predictable from a mechanical POV.

> Steps to reproduce:
> 1. Setup pass with the following structure
>    WorkingTest/example.com/foo
>    FailingTest/example2.com
>    FailingTest/example3.com with user: foo in the password file
> 2. (auth-source-pass-enable)
> 3. (setq auth-source-pass-extra-query-keywords t)
> 4. (auth-source-search :host "example" :user "foo") -> works
> 5. (auth-source-search :host "example2.com") -> fails
> 6. (auth-source-search :host "example3.com" :user "foo")
>
> Auth-source-pass should be able to query the password file for
> additional attributes if one of the previous attributes such as :host
> match to it. Quering the file attributes is quite important in use
> cases where it doesn't make sense to the user to have a
> host-folder/user-file structure in cases where there's only one
> password for said host.

Currently, if you have a file in the root of your ~/.password-store
named something like "top-level-host.com", and it's contents feature a
"user: foo" attribute, both

  (auth-source-search :host "top-level-host.com")

and

  (auth-source-search :host "top-level-host.com" :user "foo")

return

  ((:host "top-level-host.com" :secret ...)).

If you're saying you want to see (:user "foo") in the results as well, I
guess we can do that (see attached patch as well as [2], below).
However, this still won't work on any of your examples, which all have
intervening path components between the root directory and the .gpg
files. The reason for this restriction is explained below.

If we do end up going with something like the attached patch, we'll need
to profile it. I can create a bunch of fake trees of varying shapes and
sizes, but I'd rather someone with real data and a sizable store assess
how much slower it is to visit (and thus decrypt) potentially every file
in the tree, which is what any attr-reading implementation must do. On
my machine, it takes roughly 0.18 seconds to decrypt a single two-line
file via `auth-source-pass--read-entry'. (This seems prohibitively
expensive to do en masse, no?) FWIW, most of this time is spent in
`epg-wait-for-status', which blocks until the subprocess exits.

> Same it should maybe also match against :host
> if no user was provided, I don't know how other sources do this thou.

While the reference implementation indeed succeeds with a plain :host
input (see test `auth-source-pass-extra-query-keywords--netrc-host'), I
believe the actual problem you perceive has more to do with the content
of the file paths, specifically, leading directory components. Still,
I'm inclined to agree that this would be nice to have. However, I do
seem to recall this being discussed on at least one occasion, with the
conclusion being that it's too complicated, if not impossible, to
disambiguate between a trailing "hostname/user" and "folder/hostname".

Nevertheless, we could add an option to do it anyway based on one or
more heuristics-based strategy (resolving hosts for real is surely a no
go). For example, one such strategy could ignore a penultimate file-path
component that's not an FQDN, even if it's, say, LDH-only and resolvable
as a hostname, so long as the leaf component _is_ an FQDN. However, such
an option would have to be disabled by default to prevent existing
entries like "localhost/test.user" from being parsed as (:host
"test.user").

In any case, I'm happy to review patches, but I think someone who
actually uses this back end should implement the feature.


[1] https://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/auth-source-pass.el?id=423c86cb#n300

[2] The following describes details of the attached patch's logic for
    the inner (dolist (e entries) ...) loop of the primary matching
    function `auth-source-pass--find-match-many'. Hopefully it's
    somewhat sound with regard to deferring decryption for as long as
    possible.

    1. Parse the file path of each entry first and cache its results in
       a plist, the "cached entry metadata," which is filed under the
       entry's file-path in the `seen' hash table. If it doesn't match
       the basic filename format, it must not be a passwordstore file,
       so reject the entry.
    2. Check the :host field before reading the file. Unless it matches,
       reject the entry.
    3. Engage in a series of probing conditional checks for a :port
       field to match against a provided "port" query parameter, all
       while attempting to defer decryption until absolutely necessary.
       (A path-encoded :port always takes precedence over a :port in the
       file.)
       - If a `port' query parameter is not given for matching against,
         continue to the next steps for the current entry.
       - Otherwise, if a :port parsed from the file path is present and
         it doesn't match, reject the entry, meaning go to the beginning
         of the current loop, considering the next entry.
       - If a path-derived :port is absent, ensure the cached entry
         metadata contains an additional :attrs field (an alist). If the
         metadata lacks an :attrs field, the file has not yet been
         decrypted. Decrypt it now using `auth-source-pass-parse-entry',
         then add its secret and its attrs alist to the cached metadata,
         under :attrs.
       - Look in the cached entry metadata's :attrs alist for a "port"
         attr. If a "port" attr is indeed present and doesn't match the
         port query parameter, reject the entry.
       - If no such "port" attr exists and is required (meaning :port
         appears in the `require' query parameter), reject the entry.
    4. Repeat step 3 for :user. The same precedence rules apply, meaning
       any non-null path-derived :user field is immediately accepted,
       and the file is not decrypted.
    5. If we haven't yet decrypted the file, do so now and populate the
       :attrs item in the cached entry metadata. If it's already been
       decrypted at some point, :attrs will present (though possibly
       empty). In any case, add the items we care about if non-null
       (:user, :port, and :secret) to the matched results for this entry.
       However, only do so if a secret was either not required or is
       present; otherwise, reject the entry.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-POC-Match-attrs-with-auth-source-pass-extra-query-ke.patch --]
[-- Type: text/x-patch, Size: 13646 bytes --]

From a36ef10d81e2c5afb0cf2515887084b6331d38d1 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Wed, 7 Aug 2024 22:23:09 -0700
Subject: [PATCH] [POC] Match attrs with auth-source-pass-extra-query-keywords

* lisp/auth-source-pass.el (auth-source-pass--retrieve-parsed):
Remove unused function.
(auth-source-pass--cast-port): New function, a helper to match an
entry's port to the given query param's type.
(auth-source-pass--match-parts): Return non-nil when a key is required
but the value is null.  Not doing produced behavior that deviated from
the reference netrc implementation and was thus a bug.
(auth-source-pass-check-attrs-with-extra-query-keywords): New
variable, a flag to opt out of arguably expensive attribute lookups.
(auth-source-pass--find-matched-entry): New function to isolate
processing logic for a single entry.
(auth-source-pass--find-match-many): Move single-entry processing
logic to separate helper, mainly for readability.
* test/lisp/auth-source-pass-tests.el
(auth-source-pass-extra-query-keywords--akib/attr)
(auth-source-pass-extra-query-keywords--netrc-akib/require)
(auth-source-pass-extra-query-keywords--akib/attr/require)
(auth-source-pass-extra-query-keywords--netrc-baseline): New tests.
(auth-source-pass-extra-query-keywords--baseline): Reverse expected
outcome to match reference implementation.  That it didn't before was
a bug.  (Bug#72441)
---
 lisp/auth-source-pass.el            | 130 +++++++++++++++++++---------
 test/lisp/auth-source-pass-tests.el |  79 ++++++++++++++++-
 2 files changed, 165 insertions(+), 44 deletions(-)

diff --git a/lisp/auth-source-pass.el b/lisp/auth-source-pass.el
index 03fd1f35811..8982e07a6be 100644
--- a/lisp/auth-source-pass.el
+++ b/lisp/auth-source-pass.el
@@ -256,32 +256,88 @@ auth-source-pass--find-match
                 hosts
               (list hosts))))
 
-(defun auth-source-pass--retrieve-parsed (seen path port-number-p)
-  (when (string-match auth-source-pass--match-regexp path)
-    (puthash path
-             `( :host ,(or (match-string 10 path) (match-string 11 path))
-                ,@(if-let* ((tr (match-string 21 path)))
-                      (list :user tr :suffix t)
-                    (list :user (match-string 20 path)))
-                :port ,(and-let* ((p (or (match-string 30 path)
-                                         (match-string 31 path)))
-                                  (n (string-to-number p)))
-                         (if (or (zerop n) (not port-number-p))
-                             (format "%s" p)
-                           n)))
-             seen)))
-
-(defun auth-source-pass--match-parts (parts key value require)
-  (let ((mv (plist-get parts key)))
+(defun auth-source-pass--cast-port (val ref)
+  (cond ((integerp val) val)
+        ((and-let* (((integerp ref))
+                    (n (string-to-number val))
+                    ((not (zerop n))))
+           n))
+        (t (format "%s" val))))
+
+(defun auth-source-pass--match-parts (parts key reference require)
+  (let ((value (plist-get parts key)))
     (if (memq key require)
-        (and value (equal mv value))
-      (or (not value) (not mv) (equal mv value)))))
+        (or (null reference) (equal value reference))
+      (or (null reference) (null value) (equal value reference)))))
+
+(defvar auth-source-pass-check-attrs-with-extra-query-keywords t
+  "When non-nil, decrypt files to find attributes matching parameters.
+However, give precedence to fields encoded in file names.  Only applies
+when `auth-source-pass-extra-query-keywords' is non-nil.")
+
+;; This function tries to defer decryption as long as possible.  For
+;; that reason, an entry's file-path-derived :port or :user field
+;; always takes precedence over their counterparts from a decrypted
+;; file's attribute list.
+(defun auth-source-pass--find-matched-entry (host user port require seen entry)
+  "Match ENTRY against query params HOST USER PORT REQUIRE with cache SEEN."
+  (when (string-match auth-source-pass--match-regexp entry)
+    (let* ((cached (gethash entry seen))
+           (optp auth-source-pass-check-attrs-with-extra-query-keywords)
+           (suffixedp nil)
+           (h (or (and cached (plist-get cached :host))
+                  (match-string 10 entry)
+                  (match-string 11 entry)))
+           (attrs (and cached (plist-get :attrs cached)))
+           (getat (lambda (k)
+                    (save-match-data
+                      (unless attrs
+                        (setq attrs (auth-source-pass-parse-entry entry)))
+                      (auth-source-pass--get-attr k attrs))))
+           (u (cond (cached (plist-get cached :user))
+                    ((and-let* ((u (match-string 21 entry)))
+                       (setq suffixedp t)
+                       u))
+                    ((match-string 20 entry))
+                    ((and user optp) (funcall getat "user"))))
+           (p (cond (cached (plist-get cached :port))
+                    ((match-string 30 entry))
+                    ((match-string 31 entry))
+                    ((and port optp) (funcall getat "port"))))
+           ;;
+           s)
+      (when p
+        (setq p (auth-source-pass--cast-port p port)))
+      (unless cached
+        (setq cached `( :host ,h
+                        ,@(and u (list :user u))
+                        ,@(and p (list :port p))
+                        ,@(and suffixedp (list :suffix t))
+                        ,@(and attrs (list :attrs attrs))))
+        (puthash entry cached seen))
+      (when (and (equal host h)
+                 (auth-source-pass--match-parts cached :port port require)
+                 (auth-source-pass--match-parts cached :user user require)
+                 (setq s (or (funcall getat 'secret)
+                             (not (memq :secret require)))))
+        (unless (or user u)
+          (when (setq u (funcall getat "user"))
+            (setq cached (plist-put cached :user u))))
+        (unless (or port p)
+          (when (setq p (funcall getat "port"))
+            (setq p (auth-source-pass--cast-port p port)
+                  cached (plist-put cached :port p))))
+        (puthash entry (plist-put cached :attrs attrs) seen)
+        `( :host ,host
+           ,@(and u (list :user u))
+           ,@(and p (list :port p))
+           ,@(and s (not (eq s t)) (list :secret s)))))))
 
 (defun auth-source-pass--find-match-many (hosts users ports require max)
   "Return plists for valid combinations of HOSTS, USERS, PORTS."
   (let ((seen (make-hash-table :test #'equal))
         (entries (auth-source-pass-entries))
-        out suffixed suffixedp)
+        out suffixed)
     (catch 'done
       (dolist (host hosts out)
         (pcase-let ((`(,_ ,u ,p) (auth-source-pass--disambiguate host)))
@@ -289,28 +345,18 @@ auth-source-pass--find-match-many
             (setq p nil))
           (dolist (user (or users (list u)))
             (dolist (port (or ports (list p)))
-              (dolist (e entries)
-                (when-let*
-                    ((m (or (gethash e seen) (auth-source-pass--retrieve-parsed
-                                              seen e (integerp port))))
-                     ((equal host (plist-get m :host)))
-                     ((auth-source-pass--match-parts m :port port require))
-                     ((auth-source-pass--match-parts m :user user require))
-                     (parsed (auth-source-pass-parse-entry e))
-                     ;; For now, ignore body-content pairs, if any,
-                     ;; from `auth-source-pass--parse-data'.
-                     (secret (or (auth-source-pass--get-attr 'secret parsed)
-                                 (not (memq :secret require)))))
-                  (push
-                   `( :host ,host ; prefer user-provided :host over h
-                      ,@(and-let* ((u (plist-get m :user))) (list :user u))
-                      ,@(and-let* ((p (plist-get m :port))) (list :port p))
-                      ,@(and secret (not (eq secret t)) (list :secret secret)))
-                   (if (setq suffixedp (plist-get m :suffix)) suffixed out))
-                  (unless suffixedp
-                    (when (or (zerop (cl-decf max))
-                              (null (setq entries (delete e entries))))
-                      (throw 'done out)))))
+              (dolist (entry entries)
+                (let* ((result (auth-source-pass--find-matched-entry
+                                host user port require seen entry))
+                       ;;
+                       suffixedp)
+                  (when result
+                    (setq suffixedp (plist-get (gethash entry seen) :suffix))
+                    (push result (if suffixedp suffixed out))
+                    (unless suffixedp
+                      (when (or (zerop (cl-decf max))
+                                (null (setq entries (delete entry entries))))
+                        (throw 'done out))))))
               (setq suffixed (nreverse suffixed))
               (while suffixed
                 (push (pop suffixed) out)
diff --git a/test/lisp/auth-source-pass-tests.el b/test/lisp/auth-source-pass-tests.el
index 6455c3393d5..2ce5d12a6bc 100644
--- a/test/lisp/auth-source-pass-tests.el
+++ b/test/lisp/auth-source-pass-tests.el
@@ -601,6 +601,73 @@ auth-source-pass-extra-query-keywords--akib
       (should (equal results
                      '((:host "disroot.org" :user "akib" :secret "b")))))))
 
+(ert-deftest auth-source-pass-extra-query-keywords--akib/attr ()
+  (auth-source-pass--with-store '(("x.com" (secret . "a"))
+                                  ("disroot.org" (secret . "b")
+                                   ("user" . "akib") ("port" . "42"))
+                                  ("z.com" (secret . "c")))
+    (auth-source-pass-enable)
+    (let* ((auth-source-pass-extra-query-keywords t)
+           results)
+
+      ;; Non-matching query param.
+      (setq results (auth-source-search :host "disroot.org" :user "?" :max 2))
+      (should-not results)
+
+      ;; No query params matching attrs.
+      (setq results (auth-source-search :host "disroot.org" :max 2))
+      (dolist (result results)
+        (setf (plist-get result :secret) (auth-info-password result)))
+      (should (equal results
+                     '(( :host "disroot.org" :user "akib"
+                         :port "42" :secret "b"))))
+
+      ;; Matching user query param.
+      (setq results (auth-source-search :host "disroot.org" :user "akib"))
+      (dolist (result results)
+        (setf (plist-get result :secret) (auth-info-password result)))
+      (should (equal results
+                     '(( :host "disroot.org" :user "akib"
+                         :port "42" :secret "b"))))
+
+      ;; Matching port typed query param.
+      (setq results (auth-source-search :host "disroot.org" :port 42))
+      (dolist (result results)
+        (setf (plist-get result :secret) (auth-info-password result)))
+      (should (equal results
+                     '(( :host "disroot.org" :user "akib"
+                         :port 42 :secret "b")))))))
+
+(ert-deftest auth-source-pass-extra-query-keywords--netrc-akib/require ()
+  (ert-with-temp-file netrc-file
+    :text "\
+machine x.com password a
+machine disroot.org user akib password b
+machine z.com password c
+"
+    (let* ((auth-sources (list netrc-file))
+           (auth-source-do-cache nil)
+           (results (auth-source-search :host "disroot.org"
+                                        :require '(:user) :max 2)))
+      (dolist (result results)
+        (setf (plist-get result :secret) (auth-info-password result)))
+      (should (equal results
+                     '((:host "disroot.org" :user "akib" :secret "b")))))))
+
+(ert-deftest auth-source-pass-extra-query-keywords--akib/attr/require ()
+  (auth-source-pass--with-store '(("x.com" (secret . "a"))
+                                  ("disroot.org" (secret . "b")
+                                   ("user" . "akib"))
+                                  ("z.com" (secret . "c")))
+    (auth-source-pass-enable)
+    (let* ((auth-source-pass-extra-query-keywords t)
+           (results (auth-source-search :host "disroot.org"
+                                        :require '(:user) :max 2)))
+      (dolist (result results)
+        (setf (plist-get result :secret) (auth-info-password result)))
+      (should (equal results
+                     '((:host "disroot.org" :user "akib" :secret "b")))))))
+
 ;; Searches for :host are case-sensitive, and a returned host isn't
 ;; normalized.
 
@@ -632,11 +699,19 @@ auth-source-pass-extra-query-keywords--host
 ;; A retrieved store entry mustn't be nil regardless of whether its
 ;; path contains port or user components.
 
+(ert-deftest auth-source-pass-extra-query-keywords--netrc-baseline ()
+  (ert-with-temp-file netrc-file
+    :text "machine foo\n"
+    (let* ((auth-sources (list netrc-file))
+           (auth-source-do-cache nil)
+           (results (auth-source-search :host "foo")))
+      (should (equal results '((:host "foo")))))))
+
 (ert-deftest auth-source-pass-extra-query-keywords--baseline ()
   (let ((auth-source-pass-extra-query-keywords t))
-    (auth-source-pass--with-store '(("x.com"))
+    (auth-source-pass--with-store '(("foo"))
       (auth-source-pass-enable)
-      (should-not (auth-source-search :host "x.com")))))
+      (should (equal (auth-source-search :host "foo") '((:host "foo")))))))
 
 ;; Output port type (int or string) matches that of input parameter.
 
-- 
2.45.2