bug#67937: 30.0.50; auth-source-pass relies on epa-file being enabled

[Arsen Arsenović]

Hi Michael,

Sorry for responding late.  I'm yet to be able to catch whatever is
triggering this bug.  At this point, I suspect 'let' is simply broken
somehow, and not undoing its work in some odd circumstances.

I'd love to know how to efficiently create a trace every time
file-name-handler-alist gets set to nil and never recovered without
bogging down all of Emacs, but I've not had time to hack the Emacs core
yet to do such a thing.

I've attempted:

--8<---------------cut here---------------start------------->8---
(defvar arsen--watch-file-name-handler-alist-guard nil)
(defvar arsen--watch-file-name-handler-alist-depth 0)

(defun arsen--foo-el-log-buffer ()
  (or (get-buffer "*FooEl*")
      (with-current-buffer (get-buffer-create "*FooEl*")
        (messages-buffer-mode)
        (current-buffer))))

(defun arsen--watch-file-name-handler-alist (symbol newval operation where)
  (setq arsen--watch-file-name-handler-alist-depth
        (+ arsen--watch-file-name-handler-alist-depth
           (pcase operation ('let 1) ('unlet -1) (otherwise 0))))
  (if (or (member operation '(let unlet)))
      nil
    (with-current-buffer (arsen--foo-el-log-buffer)
      (let ((inhibit-read-only t)
            (standard-output (current-buffer)))
        (goto-char (point-max))
        (if newval
            nil
          ;; (not newval), meaning it got set to nothing
          (insert "----------------- start ------------------\n")
          (insert (format
                   "dpt %S op %S wh %S\n"
                   arsen--watch-file-name-handler-alist-depth
                   operation
                   where))
          (backtrace)
          (insert "-----------------  end  ------------------\n"))))))

(add-variable-watcher 'file-name-handler-alist
                      'arsen--watch-file-name-handler-alist)
--8<---------------cut here---------------end--------------->8---

... and various variants that I lost because they all take days or weeks
to test, but to no avail (this variant isolates non un/let operations in
order to catch if something is perhaps calling setq on
file-name-handler-alist, but no useful results were hit anyway).

Simply recording all changes to file-name-handler-alist in Lisp is too
slow, of course, which is why I was thinking of hacking core to do it,
and that is also why the above is so complex.

Michael Albinus <michael.albinus@gmx.de> writes:

> Could you pls just print a backtrace when epa-fle-handler isn't found?
> Something like
>
> --8<---------------cut here---------------start------------->8---
> (message "%s" (with-output-to-string (backtrace)))
> --8<---------------cut here---------------end--------------->8---
>
> This would give us a backtrace to analyze.

I've added this to the function now:

--8<---------------cut here---------------start------------->8---
(defun auth-source-pass--read-entry (entry)
  "Return a string with the file content of ENTRY."
  (with-temp-buffer
    (let ((fname (format "%s.gpg" entry)))
      (if (not (find-file-name-handler fname 'insert-file-contents))
          (progn
            (message "%s" (with-output-to-string (backtrace)))
            (debug)))
      (insert-file-contents (expand-file-name
                             fname
                             auth-source-pass-filename))
      (buffer-substring-no-properties (point-min) (point-max)))))
--8<---------------cut here---------------end--------------->8---

I am afraid it won't find useful results.  I've checked the backtrace
after this error happens.  When it does, file-name-handler-alist is
nil and the trace is akin to:

--8<---------------cut here---------------start------------->8---
Debugger entered: nil
  funcall-interactively(debug)
  command-execute(debug record)
  execute-extended-command(nil "debug" #("debug" 0 5 (ws-butler-chg chg)))
  funcall-interactively(execute-extended-command nil "debug" #("debug" 0 5 
(ws-butler-chg chg)))
  command-execute(execute-extended-command)
--8<---------------cut here---------------end--------------->8---

... so it seems to me that this happens "outside" of any interesting
context.  I'll report back when the above fires (which, I must stress,
could take days or weeks).

>> I believe that the check utilized below is correct for the
>> check-and-error solution.
>>
>> diff --git a/lisp/auth-source-pass.el b/lisp/auth-source-pass.el
>> index 0f51755a250..4da15a65259 100644
>> --- a/lisp/auth-source-pass.el
>> +++ b/lisp/auth-source-pass.el
>> @@ -195,10 +195,13 @@ auth-source-pass--get-attr
>>  (defun auth-source-pass--read-entry (entry)
>>    "Return a string with the file content of ENTRY."
>>    (with-temp-buffer
>> -    (insert-file-contents (expand-file-name
>> -                           (format "%s.gpg" entry)
>> -                           auth-source-pass-filename))
>> -    (buffer-substring-no-properties (point-min) (point-max))))
>> +    (let ((fname (format "%s.gpg" entry)))
>> +      (if (not (find-file-name-handler fname 'insert-file-contents))
>> +          (error "auth-source-pass requires a handler for .gpg files"))
>> +      (insert-file-contents (expand-file-name
>> +                             fname
>> +                             auth-source-pass-filename))
>> +      (buffer-substring-no-properties (point-min) (point-max)))))
>>
>>  (defun auth-source-pass-parse-entry (entry)
>>    "Return an alist of the data associated with ENTRY.
>
> Nope. find-file-name-handler shows the next file name handler to be
> applied. It could be epa-file-handler, but if it is removed from
> file-name-handler-alist, another file name handler could be returned,
> like tramp-file-name-handler. So if you want to use
> find-file-name-handler, you must check something like
>
> --8<---------------cut here---------------start------------->8---
> (eq (find-file-name-handler fname 'insert-file-contents) 'epa-file-handler)
> --8<---------------cut here---------------end--------------->8---

But if we're requiring that it be specifically epa-file-handler, this
seems like a more roundabout and ineffective (because it can error for
seemingly no reason) way to do what I initially proposed, which was not
relying on epa-file at all, and just using EPA/EPG - whichever is
correct - directly (unsure if the patch is right, mind you):
https://debbugs.gnu.org/cgi/bugreport.cgi?filename=0001-auth-source-pass-don-t-rely-on-epa-file-bug-67937.patch;bug=67937;msg=23;att=1

There was concern about losing TRAMP support if the above patch is
merged.  This is a legitimate concern, and it can be supported (I just
tried it - insert-file-contents-literally will use TRAMP but not decrypt
even with epa-file enabled, so we can use (epa-decrypt-string
(buffer-substring-no-properties (point-min) (point-max))) to handle that
part of the work even with epa-file disabled, and
insert-file-contents-literally to get file contents properly).

This was argued against as the user should be able to customize how
auth-source-pass opens .gpg files, but what you propose prevents that
anyway.

I disagree with this argument for two reasons:
1) they can already just redefine the function
2) there's no other reasonable file handler for pass entries.  I'd not
   expect users to be able to change the address family of TCP sockets
   to AF_UNIX via some customizable variable either

In fact, I could've done the former and completely patched out this
issue by running the updated defun from the patch above.  I didn't,
because I suspect something else is afoot (as I mentioned initially).

I'll let you know when I get that backtrace.  In the meanwhile, I'd like
to understand your opinion on my conclusion from the above: if
epa-file-handler is the only reasonable handler for the .gpg filenames
in a pass store, there's no reason to rely on the file-name handler
system.

I think this holds, and draw the conclusion that we should use
insert-file-contents-literally and epa-decrypt-string like so:

--8<---------------cut here---------------start------------->8---
(defun auth-source-pass--read-entry (entry)
  "Return a string with the file content of ENTRY."
  (with-temp-buffer
    (let ((fname (format "%s.gpg" entry)))
      (insert-file-contents-literally (expand-file-name
                                       fname
                                       auth-source-pass-filename))
      (let ((context (epg-make-context 'OpenPGP)))
        (epg-decrypt-string context (buffer-substring-no-properties
                                     (point-min) (point-max)))))))
--8<---------------cut here---------------end--------------->8---

(the above is untested)

Thanks, have a lovely day.
-- 
Arsen Arsenović

signature.asc
Description: PGP signature