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

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

[Arsen Arsenović via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi there!

It would seem that auth-source-pass relies on epa-file being enabled to
be able to decrypt passwords.

Repro steps:
- emacs -Q
- M-x epa-file-disable
- M-: (auth-source-pass-get 'secret "something")

You will see a GPG-encrypted data string.

epa-file-disable should not break the auth-source.

TIA, have a lovely day.

In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
 3.24.38, cairo version 1.18.0) of 2023-12-16 built on localhost
Repository revision: 2a591b228aaa8c66c27cc5b7cb3033aa6625bc0b
Repository branch: master
System Description: Gentoo Linux

Configured using:
 'configure --prefix=/usr --build=x86_64-pc-linux-gnu
 --host=x86_64-pc-linux-gnu --mandir=/usr/share/man
 --infodir=/usr/share/info --datadir=/usr/share --sysconfdir=/etc
 --localstatedir=/var/lib --datarootdir=/usr/share
 --disable-silent-rules --docdir=/usr/share/doc/emacs-30.0.9999
 --htmldir=/usr/share/doc/emacs-30.0.9999/html --libdir=/usr/lib64
 --program-suffix=-emacs-30-vcs --includedir=/usr/include/emacs-30-vcs
 --infodir=/usr/share/info/emacs-30-vcs --localstatedir=/var
 --enable-locallisppath=/etc/emacs:/usr/share/emacs/site-lisp
 --without-compress-install --without-hesiod --without-pop
 --with-file-notification=inotify --with-pdumper --enable-acl
 --enable-xattr --with-dbus --with-modules --with-gameuser=:gamestat
 --with-libgmp --with-gpm --with-native-compilation=aot --with-json
 --without-kerberos --without-kerberos5 --with-lcms2 --with-xml2
 --with-mailutils --without-selinux --without-small-ja-dic
 --with-sqlite3 --with-gnutls --with-libsystemd --with-threads
 --with-tree-sitter --without-wide-int --with-sound=alsa --with-zlib
 --with-pgtk --without-x --without-ns --with-toolkit-scroll-bars
 --without-gconf --without-gsettings --with-harfbuzz --with-libotf
 --with-m17n-flt --with-xwidgets --with-gif --with-jpeg --with-png
 --with-rsvg --with-tiff --with-webp --without-imagemagick
 --with-dumping=pdumper 'CFLAGS=-freport-bug -O3 -ggdb3 -pipe
 -fdiagnostics-color=always -march=x86-64-v2 -flto' 'LDFLAGS=-Wl,-O1
 -Wl,--as-needed -O3 -Wl,-O3 -pipe -fdiagnostics-color=always
 -Wl,--defsym=__gentoo_check_ldflags__=0 -Wl,-z,pack-relative-relocs
 -Wl,--build-id -flto''

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM HARFBUZZ JPEG JSON LCMS2
LIBOTF LIBSYSTEMD LIBXML2 MODULES NATIVE_COMP NOTIFY INOTIFY PDUMPER
PGTK PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS
TREE_SITTER WEBP XIM XWIDGETS GTK3 ZLIB

Important settings:
  value of $LC_TIME: en_GB.UTF-8
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: mu4e:main

Minor modes in effect:
  global-git-commit-mode: t
  magit-auto-revert-mode: t
  server-mode: t
  diff-hl-flydiff-mode: t
  global-jinx-mode: t
  savehist-mode: t
  save-place-mode: t
  desktop-save-mode: t
  mu4e-search-minor-mode: t
  global-hl-line-mode: t
  mu4e-update-minor-mode: t
  mu4e-context-minor-mode: t
  mu4e-modeline-mode: t
  ws-butler-global-mode: t
  ws-butler-mode: t
  corfu-popupinfo-mode: t
  global-corfu-mode: t
  corfu-mode: t
  marginalia-mode: t
  vertico-mouse-mode: t
  vertico-mode: t
  which-key-mode: t
  global-display-fill-column-indicator-mode: t
  which-function-mode: t
  electric-pair-mode: t
  global-whitespace-mode: t
  override-global-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tab-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  minibuffer-regexp-mode: t
  buffer-read-only: t
  column-number-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  overwrite-mode: overwrite-mode-binary

Load-path shadows:
/usr/share/emacs/site-lisp/desktop-entry-mode hides /usr/share/emacs/site-lisp/desktop-file-utils/desktop-entry-mode
/usr/share/emacs/site-lisp/emacs-eat/eat hides /usr/share/emacs/site-lisp/emacs-eat/term/eat
/usr/share/emacs/site-lisp/transient/transient hides /usr/share/emacs/30.0.50/lisp/transient

Features:
(shadow emacsbug help-fns radix-tree cl-print debug backtrace
network-stream nsm mailalias mm-archive sort smiley gnus-cite mail-extr
textsec uni-scripts idna-mapping ucs-normalize uni-confusable
textsec-check qp org-capture face-remap magit-bookmark git-rebase
magit-extras magit-sparse-checkout magit-gitignore magit-ediff ediff
ediff-merg ediff-mult ediff-wind ediff-diff ediff-help ediff-init
ediff-util magit-subtree magit-patch magit-submodule magit-blame
magit-stash magit-reflog magit-bisect magit-push magit-pull magit-fetch
magit-clone magit-remote magit-commit magit-sequence magit-notes
magit-worktree magit-tag magit-merge magit-branch magit-reset
magit-files magit-refs magit-status magit package url-handlers
magit-repos magit-apply magit-wip magit-log magit-diff smerge-mode
git-commit log-edit magit-core magit-autorevert magit-margin
magit-transient magit-process with-editor comp comp-cstr server
magit-mode transient magit-git magit-base magit-section cursor-sensor
crm consult-register consult misearch multi-isearch vc-hg vc-bzr
cus-edit cus-start tramp-archive tramp-gvfs tramp-cmds tramp-cache
time-stamp eat term ehelp vertico-directory add-log ffap antlr-mode view
texinfo texinfo-loaddefs autoconf-mode make-mode bug-reference epa-file
m4-mode flymake-cc flymake project compile warnings autorevert dired-aux
vc-git diff-hl-flydiff diff diff-hl log-view pcvs-util vc-dir ewoc vc
vc-dispatcher diff-mode cc-mode cc-fonts cc-guess cc-menus cc-cmds
cc-styles cc-align cc-engine cc-vars cc-defs jinx org-element
org-persist org-id org-refile avl-tree generator oc-basic ol-eww eww
url-queue mm-url ol-rmail ol-mhe ol-irc ol-info ol-gnus nnselect
ol-docview doc-view filenotify jka-compr image-mode exif ol-bibtex
bibtex ol-bbdb ol-w3m ol-doi org-link-doi ebuild-mode skeleton sh-script
smie treesit executable pcase emacs-news-mode display-line-numbers info
savehist saveplace tramp-sh tramp trampver tramp-integration files-x
tramp-message tramp-compat xdg shell tramp-loaddefs desktop frameset
cus-load mu4e mu4e-org mu4e-notification notifications mu4e-main
mu4e-view thingatpt gnus-art mm-uu mml2015 mm-view mml-smime smime
gnutls dig gnus-sum gnus-group gnus-undo gnus-start gnus-dbus dbus
comp-run comp-common gnus-cloud nnimap nnmail mail-source utf7 nnoo
parse-time iso8601 gnus-spec gnus-int gnus-range gnus-win gnus nnheader
range wid-edit mu4e-headers mu4e-compose mu4e-draft mu4e-actions
smtpmail mu4e-search mu4e-lists mu4e-bookmarks mu4e-mark mu4e-message
shr pixel-fill kinsoku url-file browse-url url url-proxy url-privacy
url-expand url-methods url-history url-cookie generate-lisp-file
url-domsuf url-util flow-fill mule-util hl-line mu4e-contacts
mu4e-update mu4e-folders mu4e-context mu4e-query-items mu4e-server
mu4e-modeline mu4e-vars mu4e-helpers mu4e-config mu4e-window bookmark pp
ido message sendmail mailcap yank-media puny dired dired-loaddefs rfc822
mml mml-sec epa derived epg rfc6068 epg-config gnus-util
text-property-search mm-decode mm-bodies mm-encode mail-parse rfc2231
rfc2047 rfc2045 mm-util ietf-drums mail-prsvr mailabbrev mail-utils
gmm-utils mailheader mu4e-obsolete auth-source-pass url-parse url-vars
auth-source eieio eieio-core password-cache ws-butler
modus-vivendi-tinted-theme modus-themes kind-icon svg-lib color svg dom
xml corfu-popupinfo corfu orderless marginalia vertico-mouse vertico
compat flycheck json map dash org ob ob-tangle ob-ref ob-lob ob-table
ob-exp org-macro org-src ob-comint org-pcomplete pcomplete comint
ansi-osc ansi-color org-list org-footnote org-faces org-entities
time-date noutline outline ob-emacs-lisp ob-core ob-eval org-cycle
org-table ol rx org-fold org-fold-core org-keys oc org-loaddefs
find-func cal-menu calendar cal-loaddefs org-version org-compat org-macs
format-spec which-key ace-window subr-x avy ring
display-fill-column-indicator disp-table which-func imenu elec-pair
icons whitespace edmacro kmacro byte-opt cl-macs gv cl-extra help-mode
cl-seq use-package use-package-ensure use-package-delight
use-package-diminish use-package-bind-key bind-key easy-mmode
use-package-core cl-loaddefs cl-lib bytecomp byte-compile site-gentoo
rmc iso-transl tooltip cconv eldoc paren electric uniquify ediff-hook
vc-hooks lisp-float-type elisp-mode mwheel term/pgtk-win pgtk-win
term/common-win pgtk-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode lisp-mode prog-mode register
page tab-bar menu-bar rfn-eshadow isearch easymenu timer select
scroll-bar mouse jit-lock font-lock syntax font-core term/tty-colors
frame minibuffer nadvice seq simple cl-generic indonesian philippine
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite emoji-zwj charscript
charprop case-table epa-hook jka-cmpr-hook help abbrev obarray oclosure
cl-preloaded button loaddefs theme-loaddefs faces cus-face macroexp
files window text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget keymap hashtable-print-readable backquote
threads xwidget-internal dbusbind inotify dynamic-setting
font-render-setting cairo gtk pgtk lcms2 multi-tty move-toolbar
make-network-process native-compile emacs)

Memory information:
((conses 16 1372904 2758834) (symbols 48 63158 173) (strings 32 238991 63972)
 (string-bytes 1 9351869) (vectors 16 143465) (vector-slots 8 3206303 1038617)
 (floats 8 758 12743) (intervals 56 99512 19143) (buffers 992 162))
<#secure method=pgpmime mode=sign>

--
Arsen Arsenović

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

[Eli Zaretskii]

> Date: Wed, 20 Dec 2023 17:57:28 +0100
> From:  Arsen Arsenović via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> It would seem that auth-source-pass relies on epa-file being enabled to
> be able to decrypt passwords.
> 
> Repro steps:
> - emacs -Q
> - M-x epa-file-disable
> - M-: (auth-source-pass-get 'secret "something")
> 
> You will see a GPG-encrypted data string.
> 
> epa-file-disable should not break the auth-source.

Please tell more about what you mean by "break".  When I try the above
in the latest master, I get nil and nothing else.  If that is deemed
"breakage", I guess I'm missing something, so I'd appreciate if you
tell more about the problem you see.

Thanks.

P.S. And apologies if what I say makes no sense because I don't use
auth-source-pass.

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

[Arsen Arsenović via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Evening Eli,

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Wed, 20 Dec 2023 17:57:28 +0100
>> From:  Arsen Arsenović via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>>
>> It would seem that auth-source-pass relies on epa-file being enabled to
>> be able to decrypt passwords.
>>
>> Repro steps:
>> - emacs -Q
>> - M-x epa-file-disable
>> - M-: (auth-source-pass-get 'secret "something")
>>
>> You will see a GPG-encrypted data string.
>>
>> epa-file-disable should not break the auth-source.
>
> Please tell more about what you mean by "break".

What I mean by that is 'You will see a GPG-encrypted data string'.  The
source returns an encrypted string rather than its contents.

This isn't auth-source-search (which is what I should be using for the
demo), but the actual search returns the same result (which I noticed
when debugging smtpmail failing to authenticate).

> When I try the above in the latest master, I get nil and nothing else.
> If that is deemed "breakage", I guess I'm missing something, so I'd
> appreciate if you tell more about the problem you see.
>
> Thanks.
>
> P.S. And apologies if what I say makes no sense because I don't use
> auth-source-pass.

Right.  "something" here is a placeholder for an actual password-store
entry (e.g. email/dev.gentoo.org:smtps), and you need a password-store
to reproduce the problem.

Apologies for the lack of clarification, I was writing in a hurry.

Have a good one!
--
Arsen Arsenović

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 251 bytes --]

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

[Eli Zaretskii]

> From: Arsen Arsenović <arsen@aarsen.me>
> Cc: 67937@debbugs.gnu.org
> Date: Wed, 20 Dec 2023 20:11:20 +0100
> 
> >> - emacs -Q
> >> - M-x epa-file-disable
> >> - M-: (auth-source-pass-get 'secret "something")
> >>
> >> You will see a GPG-encrypted data string.
> >>
> >> epa-file-disable should not break the auth-source.
> >
> > Please tell more about what you mean by "break".
> 
> What I mean by that is 'You will see a GPG-encrypted data string'.  The
> source returns an encrypted string rather than its contents.

How can it decrypt the string when you disable decryption?  What is
the replacement of epa-file that would decrypt the data string?

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

[Arsen Arsenović via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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


Eli Zaretskii <eliz@gnu.org> writes:

>> From: Arsen Arsenović <arsen@aarsen.me>
>> Cc: 67937@debbugs.gnu.org
>> Date: Wed, 20 Dec 2023 20:11:20 +0100
>>
>> >> - emacs -Q
>> >> - M-x epa-file-disable
>> >> - M-: (auth-source-pass-get 'secret "something")
>> >>
>> >> You will see a GPG-encrypted data string.
>> >>
>> >> epa-file-disable should not break the auth-source.
>> >
>> > Please tell more about what you mean by "break".
>>
>> What I mean by that is 'You will see a GPG-encrypted data string'.  The
>> source returns an encrypted string rather than its contents.
>
> How can it decrypt the string when you disable decryption?  What is
> the replacement of epa-file that would decrypt the data string?

Even with epa-disable, it could use epa-decrypt-region to decrypt the
password from the file.

For some context, I'll briefly summarize how password-store (pass)
works: pass stores credentials as one line representing the secret and
the rest being aux data (usually usernames and similar) in each file.
One file represents one set of credentials, encrypted via PGP (an
example filename is
~/.password-store/gentoo/gentoo.org/arsen@gentoo.org.gpg).

To get a given password from a given password store entry,
auth-source-pass needs to decrypt this file and get the first line of
the decrypted contents.

Currently, auth-source-pass relies on epa-file facilities to decrypt the
password entries, but those do nothing after epa-file-disable.  Instead,
it should use something like epa-decrypt-region or such (sorry, not too
familiar with EasyPG).

AIUI, epa-file-disable disables *automatic* decryption, not all forms of
decryption.

To provide some more context, I noticed auth-source-pass preventing
sending emails seemingly at random (by returning encrypted passwords
rather than the actual passwords), then noticed that it seems to start
working again following M-x epa-file-enable RET M-x
auth-source-forget-all-cached RET, and then I managed to reproduce in a
clean Emacs, then I filed this report.

I'm still unsure why epa-file gets disabled on occasion, but whether it
does or does not, auth-source-pass should either ensure its enabled or
not rely on such a facility for reading passwords.

Thanks again, have a lovely night.
--
Arsen Arsenović

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 251 bytes --]

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

[Eli Zaretskii]

> From: Arsen Arsenović <arsen@aarsen.me>
> Cc: 67937@debbugs.gnu.org
> Date: Wed, 20 Dec 2023 20:58:08 +0100
> 
> > How can it decrypt the string when you disable decryption?  What is
> > the replacement of epa-file that would decrypt the data string?
> 
> Even with epa-disable, it could use epa-decrypt-region to decrypt the
> password from the file.
> 
> For some context, I'll briefly summarize how password-store (pass)
> works: pass stores credentials as one line representing the secret and
> the rest being aux data (usually usernames and similar) in each file.
> One file represents one set of credentials, encrypted via PGP (an
> example filename is
> ~/.password-store/gentoo/gentoo.org/arsen@gentoo.org.gpg).
> 
> To get a given password from a given password store entry,
> auth-source-pass needs to decrypt this file and get the first line of
> the decrypted contents.
> 
> Currently, auth-source-pass relies on epa-file facilities to decrypt the
> password entries, but those do nothing after epa-file-disable.  Instead,
> it should use something like epa-decrypt-region or such (sorry, not too
> familiar with EasyPG).
> 
> AIUI, epa-file-disable disables *automatic* decryption, not all forms of
> decryption.

Thanks.  So it sounds like you are asking for a feature that currently
doesn't exist, AFAIU.  I added a couple of people to this discussion
who were involved with auth-source-pass, in the hope that they will
have suggestions and comments.

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

[Arsen Arsenović via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1.1: Type: text/plain, Size: 1556 bytes --]


Eli Zaretskii <eliz@gnu.org> writes:

> Thanks.  So it sounds like you are asking for a feature that currently
> doesn't exist, AFAIU.

I'm not sure I'd classify it as a new feature.  An existing interface is
broken under some conditions.

> I added a couple of people to this discussion who were involved with
> auth-source-pass, in the hope that they will have suggestions and
> comments.

Thank you.

Now, onto why I don't think this is a new feature:

Here's an example auth-source-search invocation that can demonstrate the
problem (assuming that the user has these a dev.gentoo.org secret on
port imaps with user arsen):

  (auth-info-password (car (auth-source-search :host "dev.gentoo.org"
                                               :port "imaps"
  					       :user "arsen")))

Following M-x epa-file-disable RET M-x auth-source-forget-all-cached RET
the above returns an encrypted string rather than its actual password.

This means that a current feature (auth-source-search) breaks under some
conditions.

I've worked out a fix, tested with the following:

  (require 'auth-source-pass)
  (setq auth-sources '(password-store))
  (auth-info-password (car (auth-source-search :host "dev.gentoo.org"
                                               :port "imaps"
                                               :user "arsen")))

I've attached the patch, though it lacks a regression test.  The reason
for this is that I want to spare the auth-source-pass developers some
triage, and that there's currently no regression tests for --read-entry.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: Remove epa-file reliance in auth-source-pass--read-entry --]
[-- Type: text/x-patch, Size: 1671 bytes --]

From 43e98821aa1f02abbfeea8b0b08ec6f4e31d8e9f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Arsen=20Arsenovi=C4=87?= <arsen@aarsen.me>
Date: Thu, 21 Dec 2023 12:29:55 +0100
Subject: [PATCH] auth-source-pass: don't rely on epa-file (bug#67937)

* lisp/auth-source-pass.el (epg): Require epg.
(auth-source-pass--read-entry): Use epg-decrypt-file instead of relying
on epa-file decrypting files read via insert-file-contents.
---
 lisp/auth-source-pass.el | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/lisp/auth-source-pass.el b/lisp/auth-source-pass.el
index 0f51755a250..0322de9f313 100644
--- a/lisp/auth-source-pass.el
+++ b/lisp/auth-source-pass.el
@@ -34,6 +34,7 @@
 (require 'cl-lib)
 (require 'auth-source)
 (require 'url-parse)
+(require 'epg)
 ;; Use `eval-when-compile' after the other `require's to avoid spurious
 ;; "might not be defined at runtime" warnings.
 (eval-when-compile (require 'subr-x))
@@ -194,11 +195,11 @@ 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 ((context (epg-make-context 'OpenPGP))
+	(file (expand-file-name
+	       (format "%s.gpg" entry)
+	       auth-source-pass-filename)))
+    (epg-decrypt-file context file nil)))
 
 (defun auth-source-pass-parse-entry (entry)
   "Return an alist of the data associated with ENTRY.
-- 
2.43.0


[-- Attachment #1.3: Type: text/plain, Size: 44 bytes --]


Have a lovely day!
--
Arsen Arsenović

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 251 bytes --]

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

[J.P.]

Hi Arsen,

I too don't use the password store or auth-source-pass, but a couple
dumb questions anyway (feel free to ignore):

1. Would it be possible to leverage the existing interface from
   `epa-hook' for decrypting these files? As a dirty example:

   (defun my-ensure-epa-file-name-handler (orig &rest args)
     (require 'epa-hook)
     (defvar epa-file-handler)
     (let ((file-name-handler-alist
            (cons epa-file-handler file-name-handler-alist)))
       (apply orig args)))

   (advice-add 'auth-source-pass--read-entry
               :around #'my-ensure-epa-file-name-handler)

   And if doing something like that (without the advice, obviously),
   could we somehow "weaken" the regexp of our fallback member's key so
   that `find-file-name-handlers' favors an existing, user-defined
   override? Alternatively, would it be too wasteful to first attempt to
   match the target file name against the option's current members
   before falling back on binding a modified value (or using your
   proposed hard-coded solution)? Or, wasteful or not, what about
   instead offering a new auth-source-pass option whose value is an
   alist of the same type as `file-name-handler-alist' that we use in
   place of or concatenate with the existing value at runtime?

2. How likely is it that someone actually depends on the perceived
   undesirable behavior currently on HEAD? Like, for example, could
   someone out there conceivably have a cron-like script that runs
   `epa-file-disable' before copying the encrypted secrets from the
   result of an `auth-source-search' to Nextcloud or something? If these
   weren't passwords, perhaps we could just shrug off such
   hypotheticals, but... (just saying).

Thanks,
J.P.

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

[Arsen Arsenović via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Hi J.P,

"J.P." <jp@neverwas.me> writes:

> Hi Arsen,
>
> I too don't use the password store or auth-source-pass, but a couple
> dumb questions anyway (feel free to ignore):
>
> 1. Would it be possible to leverage the existing interface from
>    `epa-hook' for decrypting these files? As a dirty example:
>
>    (defun my-ensure-epa-file-name-handler (orig &rest args)
>      (require 'epa-hook)
>      (defvar epa-file-handler)
>      (let ((file-name-handler-alist
>             (cons epa-file-handler file-name-handler-alist)))
>        (apply orig args)))
>
>    (advice-add 'auth-source-pass--read-entry
>                :around #'my-ensure-epa-file-name-handler)
>
>    And if doing something like that (without the advice, obviously),
>    could we somehow "weaken" the regexp of our fallback member's key so
>    that `find-file-name-handlers' favors an existing, user-defined
>    override? Alternatively, would it be too wasteful to first attempt to
>    match the target file name against the option's current members
>    before falling back on binding a modified value (or using your
>    proposed hard-coded solution)? Or, wasteful or not, what about
>    instead offering a new auth-source-pass option whose value is an
>    alist of the same type as `file-name-handler-alist' that we use in
>    place of or concatenate with the existing value at runtime?

I don't think ensuring the epa-hook is added here is preferable when a
solution that doesn't rely on hooks (one using epg, like in the patch I
posted) is quite short.  Unless EPA does more than EPG for this (but it
does not seem to, to my novice eyes).

I'm not sure what you mean by 'hard-coding' here.  These files are
always gpg files (that's how pass works), and they are always OpenPGP
encrypted.  The usage of epg-decrypt-file is proposed by the help of
epa-decrypt-region IIRC.

> 2. How likely is it that someone actually depends on the perceived
>    undesirable behavior currently on HEAD? Like, for example, could
>    someone out there conceivably have a cron-like script that runs
>    `epa-file-disable' before copying the encrypted secrets from the
>    result of an `auth-source-search' to Nextcloud or something? If these
>    weren't passwords, perhaps we could just shrug off such
>    hypotheticals, but... (just saying).

I do not know, but this dependency is wrong either way, and can confuse
users and the auth-source cache.

The only reason I noticed this is because *something* (and I have no
idea what as of yet) somehow unhooks epa-file.  When I noticed that, I
figured I could use epa-file-disable to provide a simpler reproducer.  I
didn't actually notice the bug by using epa-file-disable (and I have
never intentionally ran epa-file-disable).

I'll be tracking that down next, but fixing this first seemed easier.

> Thanks,
> J.P.

Have a lovely day!
--
Arsen Arsenović

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 251 bytes --]

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

[J.P.]

Hi Arsen,

Arsen Arsenović <arsen@aarsen.me> writes:

> "J.P." <jp@neverwas.me> writes:
>
>> 1. Would it be possible to leverage the existing interface from
>>    `epa-hook' for decrypting these files? As a dirty example:
>>
>>    (defun my-ensure-epa-file-name-handler (orig &rest args)
>>      (require 'epa-hook)
>>      (defvar epa-file-handler)
>>      (let ((file-name-handler-alist
>>             (cons epa-file-handler file-name-handler-alist)))
>>        (apply orig args)))
>>
>>    (advice-add 'auth-source-pass--read-entry
>>                :around #'my-ensure-epa-file-name-handler)
>>
>>    And if doing something like that (without the advice, obviously),
>>    could we somehow "weaken" the regexp of our fallback member's key so
>>    that `find-file-name-handlers' favors an existing, user-defined
>>    override? Alternatively, would it be too wasteful to first attempt to
>>    match the target file name against the option's current members
>>    before falling back on binding a modified value (or using your
>>    proposed hard-coded solution)? Or, wasteful or not, what about
>>    instead offering a new auth-source-pass option whose value is an
>>    alist of the same type as `file-name-handler-alist' that we use in
>>    place of or concatenate with the existing value at runtime?
>
> I don't think ensuring the epa-hook is added here is preferable when a
> solution that doesn't rely on hooks (one using epg, like in the patch I
> posted) is quite short.  Unless EPA does more than EPG for this (but it
> does not seem to, to my novice eyes).

I think what `epa-hook' does beyond `epg' is offer the option of opting
out of the latter by way of the `file-name-handler-alist' interface. If
that's unwise or unnecessary for some reason, we should probably explain
why, if only for posterity. And in doing so, we should maybe also
account for behavior like that of `epa-file-insert-file-contents', which
lies in the default `f-n-h-a' code path and appears to go out of its way
to provide a different user experience when it comes to error handling.
If such accommodations are unnecessary, perhaps we ought to be somewhat
explicit as to why.

> I'm not sure what you mean by 'hard-coding' here.  These files are
> always gpg files (that's how pass works), and they are always OpenPGP
> encrypted.  The usage of epg-decrypt-file is proposed by the help of
> epa-decrypt-region IIRC.

I meant "hard-coding" in the sense that the original design seems to
allow external code to potentially influence the decryption process, as
mentioned above. But from what you're saying, it sounds like there's no
legitimate use case for doing so. I wasn't privy to the original design
discussions, but it would be nice to know if there was a good reason for
going this route beyond adhering to the age-old best practice of relying
on interfaces rather than implementations. Perhaps it's worth rifling
through the archives for mention of the authors' original motivations
WRT `f-n-h-a', just for good measure?

>> 2. How likely is it that someone actually depends on the perceived
>>    undesirable behavior currently on HEAD? Like, for example, could
>>    someone out there conceivably have a cron-like script that runs
>>    `epa-file-disable' before copying the encrypted secrets from the
>>    result of an `auth-source-search' to Nextcloud or something? If these
>>    weren't passwords, perhaps we could just shrug off such
>>    hypotheticals, but... (just saying).
>
> I do not know, but this dependency is wrong either way, and can confuse
> users and the auth-source cache.

So, it seems like we're saying that protecting an unlikely minority here
should not stand in the way of simplicity and consistency. I can't argue
against that, but I think it's important to be clear about the potential
consequences of such a sacrifice.

> The only reason I noticed this is because *something* (and I have no
> idea what as of yet) somehow unhooks epa-file.  When I noticed that, I
> figured I could use epa-file-disable to provide a simpler reproducer.  I
> didn't actually notice the bug by using epa-file-disable (and I have
> never intentionally ran epa-file-disable).
>
> I'll be tracking that down next, but fixing this first seemed easier.

Tracking that down would be nice, definitely.

Thanks,
J.P.

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

[Arsen Arsenović via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Hi J.P,

"J.P." <jp@neverwas.me> writes:
[...snip...]
>> I don't think ensuring the epa-hook is added here is preferable when a
>> solution that doesn't rely on hooks (one using epg, like in the patch I
>> posted) is quite short.  Unless EPA does more than EPG for this (but it
>> does not seem to, to my novice eyes).
>
> I think what `epa-hook' does beyond `epg' is offer the option of
> opting out of the latter by way of the `file-name-handler-alist'
> interface. If that's unwise or unnecessary for some reason, we should
> probably explain why, if only for posterity.

I believe it is, because a pass entry is precisely a single OpenPGP
encrypted file.  All other pass-compatible tools and implementations
already rely on that.  If we want to allow the user to change that, we
should do so by relying on the pass CLI tool, because that way other
parts of their pass workflow allow for change.

But, I don't think even that is needed, at least for now.

> And in doing so, we should maybe also account for behavior like that
> of `epa-file-insert-file-contents', which lies in the default
> `f-n-h-a' code path and appears to go out of its way to provide a
> different user experience when it comes to error handling.  If such
> accommodations are unnecessary, perhaps we ought to be somewhat
> explicit as to why.

Indeed, stuff like this was what I was referring to.  Thanks for naming
the function that implements this, I went ahead and read it.

I believe the entire file-exists-p check is unnecessary, as the file
being read is "guaranteed" to exist, bar race conditions (which ought to
be fixed via a slightly larger refactor, by having a-s-p functions
accept either a buffer or an open file or something, then having its
user open a file literally).

That leaves us with just:

--8<---------------cut here---------------start------------->8---
         (if (setq entry (assoc file epa-file-passphrase-alist))
    	 (setcdr entry nil))
         ;; If the decryption program can't be found,
         ;; signal that as a non-file error
         ;; so that find-file-noselect-1 won't handle it.
         ;; Borrowed from jka-compr.el.
         (if (and (memq 'file-error (get (car error) 'error-conditions))
    	      (equal (cadr error) "Searching for program"))
    	 (error "Decryption program `%s' not found"
    		(nth 3 error)))
--8<---------------cut here---------------end--------------->8---

I believe the passphrase handling is also unnecessary, or at least not
very likely to matter, as 'pass' files aren't intended to be
symmetrically encrypted.

That leaves us with handling the lack of a decryption program.  Perhaps
we should extract this into some common code (I'm surprised other users
of EPG don't need it).  Perhaps the status quo is good enough as it is?
I have not tested that yet (need to run - sorry).

Overall, I don't think involving the f-n-h-a mechanism is desirable to
get one error path that could be obtained via refactoring when it ends
up also dragging in all the possible complexity of f-n-h-a where it is
not desirable (as pass entries are strictly defined).

>> I'm not sure what you mean by 'hard-coding' here.  These files are
>> always gpg files (that's how pass works), and they are always OpenPGP
>> encrypted.  The usage of epg-decrypt-file is proposed by the help of
>> epa-decrypt-region IIRC.
>
> I meant "hard-coding" in the sense that the original design seems to
> allow external code to potentially influence the decryption process,
> as mentioned above.

I believe this is accidental.  auth-source-pass authors would have to
weigh in, though.

> But from what you're saying, it sounds like there's no legitimate use
> case for doing so. I wasn't privy to the original design discussions,
> but it would be nice to know if there was a good reason for going this
> route beyond adhering to the age-old best practice of relying on
> interfaces rather than implementations.

AFAIK, epg-decrypt-file is a public interface.  epa-decrypt-region (not
epa-decrypt-file, sorry, I misrecalled in my earlier message) even
suggests using it:

  Be careful about using this command in Lisp programs!
  Since this function operates on regions, it does some tricks such
  as coding-system detection and unibyte/multibyte conversion.  If
  you are sure how the data in the region should be treated, you
  should consider using the string based counterpart
  ‘epg-decrypt-string’, or the file based counterpart
  ‘epg-decrypt-file’ instead.


> Perhaps it's worth rifling through the archives for mention of the
> authors' original motivations WRT `f-n-h-a', just for good measure?

Probably, but my intuition tells me it was accidental.  I'll try to find
relevant messages (thankfully, there wasn't too much discussion on this
topic, so that shouldn't be too hard to search :-) ).

>>> 2. How likely is it that someone actually depends on the perceived
>>>    undesirable behavior currently on HEAD? Like, for example, could
>>>    someone out there conceivably have a cron-like script that runs
>>>    `epa-file-disable' before copying the encrypted secrets from the
>>>    result of an `auth-source-search' to Nextcloud or something? If these
>>>    weren't passwords, perhaps we could just shrug off such
>>>    hypotheticals, but... (just saying).

I missed the latter part of this before, apologies.

If such a user exists, and they use any auth-sources value besides
'(password-source), their setup will already break, as this hack only
works for password-source.  In addition, due to auth-source caching,
they'd need to flush the cache twice per such a backup (once to throw
out the unencrypted password, and once to recover it).  There are
certainly more elegant solutions to getting the contents of those
encrypted files.

>>
>> I do not know, but this dependency is wrong either way, and can confuse
>> users and the auth-source cache.
>
> So, it seems like we're saying that protecting an unlikely minority here
> should not stand in the way of simplicity and consistency. I can't argue
> against that, but I think it's important to be clear about the potential
> consequences of such a sacrifice.

Sure.

>> The only reason I noticed this is because *something* (and I have no
>> idea what as of yet) somehow unhooks epa-file.  When I noticed that, I
>> figured I could use epa-file-disable to provide a simpler reproducer.  I
>> didn't actually notice the bug by using epa-file-disable (and I have
>> never intentionally ran epa-file-disable).
>>
>> I'll be tracking that down next, but fixing this first seemed easier.
>
> Tracking that down would be nice, definitely.

I will try to debug-on-variable-change f-h-n-a, but to do that I'll need
to figure out a more effective approach than hitting 'c' repeatedly in
the debugger (can debugs be conditional?), as that's getting tiring and
imprecise.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 251 bytes --]

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

[J.P.]

Hi Arsen,

Arsen Arsenović <arsen@aarsen.me> writes:

> "J.P." <jp@neverwas.me> writes:
> [...snip...]
>>
>> I think what `epa-hook' does beyond `epg' is offer the option of
>> opting out of the latter by way of the `file-name-handler-alist'
>> interface. If that's unwise or unnecessary for some reason, we should
>> probably explain why, if only for posterity.
>
> I believe it is, because a pass entry is precisely a single OpenPGP
> encrypted file.  All other pass-compatible tools and implementations
> already rely on that.  If we want to allow the user to change that, we
> should do so by relying on the pass CLI tool, because that way other
> parts of their pass workflow allow for change.
>
> But, I don't think even that is needed, at least for now.

I see. If there's essentially only one way to go about accessing and
decrypting files in these pass trees, then perhaps this is more of a bug
fix than a feature?

>> And in doing so, we should maybe also account for behavior like that
>> of `epa-file-insert-file-contents', which lies in the default
>> `f-n-h-a' code path and appears to go out of its way to provide a
>> different user experience when it comes to error handling.  If such
>> accommodations are unnecessary, perhaps we ought to be somewhat
>> explicit as to why.
>
> Indeed, stuff like this was what I was referring to.  Thanks for naming
> the function that implements this, I went ahead and read it.
>
> I believe the entire file-exists-p check is unnecessary, as the file
> being read is "guaranteed" to exist, bar race conditions (which ought to
> be fixed via a slightly larger refactor, by having a-s-p functions
> accept either a buffer or an open file or something, then having its
> user open a file literally).

Hm, I guess `expand-file-name' doesn't actually check to see if the file
name it returns exists, so I think the subprocess will ultimately be fed

  ... --decrypt -- non-existent-file.gpg

as trailing args. But I suppose that's not a concern so long as the user
can readily deduce that some kind of easily fixable pilot error has
occurred.

> That leaves us with just:
>
>          (if (setq entry (assoc file epa-file-passphrase-alist))
>     	 (setcdr entry nil))
>          ;; If the decryption program can't be found,
>          ;; signal that as a non-file error
>          ;; so that find-file-noselect-1 won't handle it.
>          ;; Borrowed from jka-compr.el.
>          (if (and (memq 'file-error (get (car error) 'error-conditions))
>     	      (equal (cadr error) "Searching for program"))
>     	 (error "Decryption program `%s' not found"
>     		(nth 3 error)))
>
> I believe the passphrase handling is also unnecessary, or at least not
> very likely to matter, as 'pass' files aren't intended to be
> symmetrically encrypted.

Makes sense. And I guess pass doesn't sign these files either, so
there's no risk of a "wrong password" failure from the agent or pinentry
ending up in that condition-case handler.

> That leaves us with handling the lack of a decryption program.  Perhaps
> we should extract this into some common code (I'm surprised other users
> of EPG don't need it).  Perhaps the status quo is good enough as it is?
> I have not tested that yet (need to run - sorry).

Again, I'd say as long as there's some way for these rare pilot errors
to reach the user, that's probably enough.

> Overall, I don't think involving the f-n-h-a mechanism is desirable to
> get one error path that could be obtained via refactoring when it ends
> up also dragging in all the possible complexity of f-n-h-a where it is
> not desirable (as pass entries are strictly defined).

Simplicity is a worthy goal, I think we can all agree.

>>> I'm not sure what you mean by 'hard-coding' here.  These files are
>>> always gpg files (that's how pass works), and they are always OpenPGP
>>> encrypted.  The usage of epg-decrypt-file is proposed by the help of
>>> epa-decrypt-region IIRC.
>>
>> I meant "hard-coding" in the sense that the original design seems to
>> allow external code to potentially influence the decryption process,
>> as mentioned above.
>
> I believe this is accidental.  auth-source-pass authors would have to
> weigh in, though.
>
>> But from what you're saying, it sounds like there's no legitimate use
>> case for doing so. I wasn't privy to the original design discussions,
>> but it would be nice to know if there was a good reason for going this
>> route beyond adhering to the age-old best practice of relying on
>> interfaces rather than implementations.
>
> AFAIK, epg-decrypt-file is a public interface.  epa-decrypt-region (not
> epa-decrypt-file, sorry, I misrecalled in my earlier message) even
> suggests using it:
>
>   Be careful about using this command in Lisp programs!
[...]

Sorry, by "relying on interfaces", I basically meant adhering to the
"dependency inversion principle," at least insofar as `epa-hook' and
`a-s-p' both rely on `f-n-h-a'. But if there's only one way to skin this
particular cat, then perhaps that's all just unwanted complexity, as you
say.

>> Perhaps it's worth rifling through the archives for mention of the
>> authors' original motivations WRT `f-n-h-a', just for good measure?
>
> Probably, but my intuition tells me it was accidental.  I'll try to find
> relevant messages (thankfully, there wasn't too much discussion on this
> topic, so that shouldn't be too hard to search :-) ).
>
>>>> 2. How likely is it that someone actually depends on the perceived
>>>>    undesirable behavior currently on HEAD? Like, for example, could
>>>>    someone out there conceivably have a cron-like script that runs
>>>>    `epa-file-disable' before copying the encrypted secrets from the
>>>>    result of an `auth-source-search' to Nextcloud or something? If these
>>>>    weren't passwords, perhaps we could just shrug off such
>>>>    hypotheticals, but... (just saying).
>
> I missed the latter part of this before, apologies.
>
> If such a user exists, and they use any auth-sources value besides
> '(password-source), their setup will already break, as this hack only
> works for password-source.  In addition, due to auth-source caching,
> they'd need to flush the cache twice per such a backup (once to throw
> out the unencrypted password, and once to recover it).  There are
> certainly more elegant solutions to getting the contents of those
> encrypted files.

I guess I was originally envisioning a headless --batch style situation
rather than an in-session background task, but regardless, what you say
about the cache makes sense in that more hurdles means more hassle,
which makes such a scenario all the more unlikely.

>>>
>>> I do not know, but this dependency is wrong either way, and can confuse
>>> users and the auth-source cache.
>>
>> So, it seems like we're saying that protecting an unlikely minority here
>> should not stand in the way of simplicity and consistency. I can't argue
>> against that, but I think it's important to be clear about the potential
>> consequences of such a sacrifice.
>
> Sure.

Don't kill me, but I have another rather unlikely scenario perhaps
worthy of passing consideration (or dismissal):

  (setopt auth-source-pass-filename "/ssh:desktop.local:.password-store")

If those Tramp addresses don't continue to work after your suggested
changes, we should probably ask Michael Albinus whether their working
currently is just an accident or something intentional and supported.

>>> The only reason I noticed this is because *something* (and I have no
>>> idea what as of yet) somehow unhooks epa-file.  When I noticed that, I
>>> figured I could use epa-file-disable to provide a simpler reproducer.  I
>>> didn't actually notice the bug by using epa-file-disable (and I have
>>> never intentionally ran epa-file-disable).
>>>
>>> I'll be tracking that down next, but fixing this first seemed easier.
>>
>> Tracking that down would be nice, definitely.
>
> I will try to debug-on-variable-change f-h-n-a, but to do that I'll need
> to figure out a more effective approach than hitting 'c' repeatedly in
> the debugger (can debugs be conditional?), as that's getting tiring and
> imprecise.

Yeah, that sounds like a pain. If you haven't tried already, perhaps
hand rolling your own `add-variable-watcher' is worth a shot?

J.P.

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

[Arsen Arsenović via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Hi J.P,

"J.P." <jp@neverwas.me> writes:

> Hi Arsen,
>
> Arsen Arsenović <arsen@aarsen.me> writes:
>
>> "J.P." <jp@neverwas.me> writes:
>> [...snip...]
>>>
>>> I think what `epa-hook' does beyond `epg' is offer the option of
>>> opting out of the latter by way of the `file-name-handler-alist'
>>> interface. If that's unwise or unnecessary for some reason, we should
>>> probably explain why, if only for posterity.
>>
>> I believe it is, because a pass entry is precisely a single OpenPGP
>> encrypted file.  All other pass-compatible tools and implementations
>> already rely on that.  If we want to allow the user to change that, we
>> should do so by relying on the pass CLI tool, because that way other
>> parts of their pass workflow allow for change.
>>
>> But, I don't think even that is needed, at least for now.
>
> I see. If there's essentially only one way to go about accessing and
> decrypting files in these pass trees, then perhaps this is more of a bug
> fix than a feature?

Yes, I consider this a bug and the patch a bug fix :-)

>>> And in doing so, we should maybe also account for behavior like that
>>> of `epa-file-insert-file-contents', which lies in the default
>>> `f-n-h-a' code path and appears to go out of its way to provide a
>>> different user experience when it comes to error handling.  If such
>>> accommodations are unnecessary, perhaps we ought to be somewhat
>>> explicit as to why.
>>
>> Indeed, stuff like this was what I was referring to.  Thanks for naming
>> the function that implements this, I went ahead and read it.
>>
>> I believe the entire file-exists-p check is unnecessary, as the file
>> being read is "guaranteed" to exist, bar race conditions (which ought to
>> be fixed via a slightly larger refactor, by having a-s-p functions
>> accept either a buffer or an open file or something, then having its
>> user open a file literally).
>
> Hm, I guess `expand-file-name' doesn't actually check to see if the file
> name it returns exists, so I think the subprocess will ultimately be fed
>
>   ... --decrypt -- non-existent-file.gpg
>
> as trailing args. But I suppose that's not a concern so long as the user
> can readily deduce that some kind of easily fixable pilot error has
> occurred.

Yes, but I think that the users of this function enumerate pass entries
before calling it, and so "never" call it with a nonexistent file
(though, that's perhaps not the case for non-a-s-p users.. unsure if
this API is public and considered stable, but I suppose it is at least
public since it's documented and lacks '--'?)

>> That leaves us with just:
>>
>>          (if (setq entry (assoc file epa-file-passphrase-alist))
>>     	 (setcdr entry nil))
>>          ;; If the decryption program can't be found,
>>          ;; signal that as a non-file error
>>          ;; so that find-file-noselect-1 won't handle it.
>>          ;; Borrowed from jka-compr.el.
>>          (if (and (memq 'file-error (get (car error) 'error-conditions))
>>     	      (equal (cadr error) "Searching for program"))
>>     	 (error "Decryption program `%s' not found"
>>     		(nth 3 error)))
>>
>> I believe the passphrase handling is also unnecessary, or at least not
>> very likely to matter, as 'pass' files aren't intended to be
>> symmetrically encrypted.
>
> Makes sense. And I guess pass doesn't sign these files either, so
> there's no risk of a "wrong password" failure from the agent or pinentry
> ending up in that condition-case handler.

It does not, no.

>> That leaves us with handling the lack of a decryption program.  Perhaps
>> we should extract this into some common code (I'm surprised other users
>> of EPG don't need it).  Perhaps the status quo is good enough as it is?
>> I have not tested that yet (need to run - sorry).
>
> Again, I'd say as long as there's some way for these rare pilot errors
> to reach the user, that's probably enough.

They should propagate normally, AFAIK.

>> Overall, I don't think involving the f-n-h-a mechanism is desirable to
>> get one error path that could be obtained via refactoring when it ends
>> up also dragging in all the possible complexity of f-n-h-a where it is
>> not desirable (as pass entries are strictly defined).
>
> Simplicity is a worthy goal, I think we can all agree.
>
>>>> I'm not sure what you mean by 'hard-coding' here.  These files are
>>>> always gpg files (that's how pass works), and they are always OpenPGP
>>>> encrypted.  The usage of epg-decrypt-file is proposed by the help of
>>>> epa-decrypt-region IIRC.
>>>
>>> I meant "hard-coding" in the sense that the original design seems to
>>> allow external code to potentially influence the decryption process,
>>> as mentioned above.
>>
>> I believe this is accidental.  auth-source-pass authors would have to
>> weigh in, though.
>>
>>> But from what you're saying, it sounds like there's no legitimate use
>>> case for doing so. I wasn't privy to the original design discussions,
>>> but it would be nice to know if there was a good reason for going this
>>> route beyond adhering to the age-old best practice of relying on
>>> interfaces rather than implementations.
>>
>> AFAIK, epg-decrypt-file is a public interface.  epa-decrypt-region (not
>> epa-decrypt-file, sorry, I misrecalled in my earlier message) even
>> suggests using it:
>>
>>   Be careful about using this command in Lisp programs!
> [...]
>
> Sorry, by "relying on interfaces", I basically meant adhering to the
> "dependency inversion principle," at least insofar as `epa-hook' and
> `a-s-p' both rely on `f-n-h-a'. But if there's only one way to skin this
> particular cat, then perhaps that's all just unwanted complexity, as you
> say.

Right, that was my perspective.

>>> Perhaps it's worth rifling through the archives for mention of the
>>> authors' original motivations WRT `f-n-h-a', just for good measure?
>>
>> Probably, but my intuition tells me it was accidental.  I'll try to find
>> relevant messages (thankfully, there wasn't too much discussion on this
>> topic, so that shouldn't be too hard to search :-) ).
>>
>>>>> 2. How likely is it that someone actually depends on the perceived
>>>>>    undesirable behavior currently on HEAD? Like, for example, could
>>>>>    someone out there conceivably have a cron-like script that runs
>>>>>    `epa-file-disable' before copying the encrypted secrets from the
>>>>>    result of an `auth-source-search' to Nextcloud or something? If these
>>>>>    weren't passwords, perhaps we could just shrug off such
>>>>>    hypotheticals, but... (just saying).
>>
>> I missed the latter part of this before, apologies.
>>
>> If such a user exists, and they use any auth-sources value besides
>> '(password-source), their setup will already break, as this hack only
>> works for password-source.  In addition, due to auth-source caching,
>> they'd need to flush the cache twice per such a backup (once to throw
>> out the unencrypted password, and once to recover it).  There are
>> certainly more elegant solutions to getting the contents of those
>> encrypted files.
>
> I guess I was originally envisioning a headless --batch style situation
> rather than an in-session background task, but regardless, what you say
> about the cache makes sense in that more hurdles means more hassle,
> which makes such a scenario all the more unlikely.

Ah, right.  That could make more sense in a batch task, but I still
doubt it for the same reason as before.

>>>>
>>>> I do not know, but this dependency is wrong either way, and can confuse
>>>> users and the auth-source cache.
>>>
>>> So, it seems like we're saying that protecting an unlikely minority here
>>> should not stand in the way of simplicity and consistency. I can't argue
>>> against that, but I think it's important to be clear about the potential
>>> consequences of such a sacrifice.
>>
>> Sure.
>
> Don't kill me, but I have another rather unlikely scenario perhaps
> worthy of passing consideration (or dismissal):
>
>   (setopt auth-source-pass-filename "/ssh:desktop.local:.password-store")
>
> If those Tramp addresses don't continue to work after your suggested
> changes, we should probably ask Michael Albinus whether their working
> currently is just an accident or something intentional and supported.

This is a worthy consideration.  It is something a user could reasonably
think to do.  Suppose:

  (let ((ctx (epg-make-context 'OpenPGP)))
    (epg-decrypt-file ctx "/ssh:..." nil))

... as you suspected, this does not work as gpg gets given the TRAMP
locator.

If the a-s-p authors think this is worthy of supporting, I will write an
alternative patch that supports this via insert-file-literally (somewhat
akin to the current code, but still explicit in decryption).  I
confirmed that insert-file-literally still supports TRAMP, so this is a
viable path forward (but I can only do that in ~hour or so - need to do
something now).

>>>> The only reason I noticed this is because *something* (and I have no
>>>> idea what as of yet) somehow unhooks epa-file.  When I noticed that, I
>>>> figured I could use epa-file-disable to provide a simpler reproducer.  I
>>>> didn't actually notice the bug by using epa-file-disable (and I have
>>>> never intentionally ran epa-file-disable).
>>>>
>>>> I'll be tracking that down next, but fixing this first seemed easier.
>>>
>>> Tracking that down would be nice, definitely.
>>
>> I will try to debug-on-variable-change f-h-n-a, but to do that I'll need
>> to figure out a more effective approach than hitting 'c' repeatedly in
>> the debugger (can debugs be conditional?), as that's getting tiring and
>> imprecise.
>
> Yeah, that sounds like a pain. If you haven't tried already, perhaps
> hand rolling your own `add-variable-watcher' is worth a shot?

I figured to try that, but have been absent all day so I have not yet.
I will do that ASAP, though.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 251 bytes --]

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

[Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

"J.P." <jp@neverwas.me> writes:

> Hi Arsen,

Hi,

> Don't kill me, but I have another rather unlikely scenario perhaps
> worthy of passing consideration (or dismissal):
>
>   (setopt auth-source-pass-filename "/ssh:desktop.local:.password-store")
>
> If those Tramp addresses don't continue to work after your suggested
> changes, we should probably ask Michael Albinus whether their working
> currently is just an accident or something intentional and supported.

I don't remember any special effort making auth-source-pass Tramp-affin,
but I might misremember. However, I wouldn't call it "accident", but
"Emacs design". If accessing auth-source-pass-filename uses the well
known primitive functions (insert-file-contents, expand-file-name
alike), there shouldn't be a problem of keeping this compatibility with
Tramp.

> J.P.

Best regards, Michael.

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

[J.P.]

Hi Michael,

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

> "J.P." <jp@neverwas.me> writes:
>
>> If those Tramp addresses don't continue to work after your suggested
>> changes, we should probably ask Michael Albinus whether their working
>> currently is just an accident or something intentional and supported.
>
> I don't remember any special effort making auth-source-pass Tramp-affin,
> but I might misremember. However, I wouldn't call it "accident", but
> "Emacs design". If accessing auth-source-pass-filename uses the well
> known primitive functions (insert-file-contents, expand-file-name
> alike), there shouldn't be a problem of keeping this compatibility with
> Tramp.

Ah, right. So deliberate by proxy (or virtue) of Emacs design, then. The
issue in this bug is that a default member of `file-name-handler-alist',
namely,

  ("\\.gpg\\(~\\|..." . epa-file-handler)

which is actually the value of the variable `epa-file-handler' added by
the file epa-hook, disappears mysteriously due to "reasons" TBD. This
breaks `auth-source-pass' because it relies on `insert-file-contents',
which calls `find-file-name-handler', to decrypt passwords. Arsen
believes this dependency a sign of unnecessary brittleness and therefore
a bug. His proposed solution is to use `insert-file-contents-literally',
which epa-hook doesn't subscribe to, as it only does

  (put 'epa-file-handler 'operations '(write-region insert-file-contents))

while `i-f-c-literally' does

  (let ((inhibit-file-name-operation 'insert-file-contents)) ...)

My initial concern was other (non-Tramp) file handlers possibly missing
out by our routing around `insert-file-contents', but without a concrete
example, perhaps that's unwarranted FUD. Anyway, thanks for weighing in.

J.P.

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

[Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

"J.P." <jp@neverwas.me> writes:

> Hi Michael,

Hi,

> The issue in this bug is that a default member of
> `file-name-handler-alist', namely,
>
>   ("\\.gpg\\(~\\|..." . epa-file-handler)
>
> which is actually the value of the variable `epa-file-handler' added by
> the file epa-hook, disappears mysteriously due to "reasons" TBD.

This happens due to the call of `epa-file-disable' mentioned in the
initial recipe.

> J.P.

Best regards, Michael.

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

[J.P.]

Hi Michael,

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

> "J.P." <jp@neverwas.me> writes:
>
>> Hi Michael,
>
> Hi,
>
>> The issue in this bug is that a default member of
>> `file-name-handler-alist', namely,
>>
>>   ("\\.gpg\\(~\\|..." . epa-file-handler)
>>
>> which is actually the value of the variable `epa-file-handler' added by
>> the file epa-hook, disappears mysteriously due to "reasons" TBD.
>
> This happens due to the call of `epa-file-disable' mentioned in the
> initial recipe.

IIUC, the call in question was only included in the recipe to simulate
the effect of the entry disappearing, and Arsen is still trying to
pinpoint the actual cause.

J.P.

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

[Arsen Arsenović via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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


"J.P." <jp@neverwas.me> writes:

>> This happens due to the call of `epa-file-disable' mentioned in the
>> initial recipe.
>
> IIUC, the call in question was only included in the recipe to simulate
> the effect of the entry disappearing, and Arsen is still trying to
> pinpoint the actual cause.

Indeed.  That's a topic for a different bug report, though ;P
--
Arsen Arsenović

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 381 bytes --]

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

[Arsen Arsenović via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1.1: Type: text/plain, Size: 998 bytes --]

Hi Michael,

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

> "J.P." <jp@neverwas.me> writes:
>
>> Hi Arsen,
>
> Hi,
>
>> Don't kill me, but I have another rather unlikely scenario perhaps
>> worthy of passing consideration (or dismissal):
>>
>>   (setopt auth-source-pass-filename "/ssh:desktop.local:.password-store")
>>
>> If those Tramp addresses don't continue to work after your suggested
>> changes, we should probably ask Michael Albinus whether their working
>> currently is just an accident or something intentional and supported.
>
> I don't remember any special effort making auth-source-pass Tramp-affin,
> but I might misremember. However, I wouldn't call it "accident", but
> "Emacs design".

A happy accident, if you will :-)

> If accessing auth-source-pass-filename uses the well known primitive
> functions (insert-file-contents, expand-file-name alike), there
> shouldn't be a problem of keeping this compatibility with Tramp.

Right.

This v2 patch restores TRAMP support.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: v2 patch --]
[-- Type: text/x-patch, Size: 2139 bytes --]

From 2097666b80c1b78462fbf454664b0017773c91d3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Arsen=20Arsenovi=C4=87?= <arsen@aarsen.me>
Date: Thu, 21 Dec 2023 12:29:55 +0100
Subject: [PATCH v2] auth-source-pass: don't rely on epa-file (bug#67937)

* lisp/auth-source-pass.el (epg): Require epg.
(auth-source-pass--read-entry): Use epg-decrypt-string and
insert-file-contents-literally instead of relying on epa-file
decrypting files read via insert-file-contents.  This avoids
interference from file-name-handler-alist, and avoids breaking
when epa-file-handler is not mong f-n-h-a.
---
 lisp/auth-source-pass.el | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/lisp/auth-source-pass.el b/lisp/auth-source-pass.el
index 0f51755a250..abfcf4b710c 100644
--- a/lisp/auth-source-pass.el
+++ b/lisp/auth-source-pass.el
@@ -34,6 +34,7 @@
 (require 'cl-lib)
 (require 'auth-source)
 (require 'url-parse)
+(require 'epg)
 ;; Use `eval-when-compile' after the other `require's to avoid spurious
 ;; "might not be defined at runtime" warnings.
 (eval-when-compile (require 'subr-x))
@@ -194,11 +195,18 @@ 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 ((context (epg-make-context 'OpenPGP))
+	(file (expand-file-name
+	       (format "%s.gpg" entry)
+	       auth-source-pass-filename)))
+    (with-temp-buffer
+      ;; Avoid file-name-handler-alist interference.  We're reading
+      ;; and decrypting a binary file here.
+      (insert-file-contents-literally file)
+      (epg-decrypt-string
+       context
+       (buffer-substring-no-properties (point-min)
+                                       (point-max))))))
 
 (defun auth-source-pass-parse-entry (entry)
   "Return an alist of the data associated with ENTRY.
-- 
2.43.0


[-- Attachment #1.3: Type: text/plain, Size: 44 bytes --]


Have a lovely day.
--
Arsen Arsenović

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 381 bytes --]

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

[Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Arsen Arsenović <arsen@aarsen.me> writes:

Hi,

>>> This happens due to the call of `epa-file-disable' mentioned in the
>>> initial recipe.
>>
>> IIUC, the call in question was only included in the recipe to simulate
>> the effect of the entry disappearing, and Arsen is still trying to
>> pinpoint the actual cause.
>
> Indeed.  That's a topic for a different bug report, though ;P

Do you have a recipe for the problem w/o calling epa-disble-file? My gut
feeling tells me that this could be the real problem, and we need to
solve this instead of bypassing the problem with another patch, which
could introduce further problems.

Hunting for this problem I recommend to use
(debug-on-variable-change 'file-name-handler-alist)

I haven't followed the whole discussion, so forgive me if this has been
discussed already.

> Arsen Arsenović

Best regards, Michael.

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

[Arsen Arsenović via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Hi Michael,

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

> Arsen Arsenović <arsen@aarsen.me> writes:
>
> Hi,
>
>>>> This happens due to the call of `epa-file-disable' mentioned in the
>>>> initial recipe.
>>>
>>> IIUC, the call in question was only included in the recipe to simulate
>>> the effect of the entry disappearing, and Arsen is still trying to
>>> pinpoint the actual cause.
>>
>> Indeed.  That's a topic for a different bug report, though ;P
>
> Do you have a recipe for the problem w/o calling epa-disble-file?

No.

This patch/bug report addresses a real problem that exists independently
of what triggered it in my case.

> My gut feeling tells me that this could be the real problem, and we
> need to solve this instead of bypassing the problem with another
> patch, which could introduce further problems.

Your gut's nearly certainly right here :-)  I am still hunting for the
cause of that issue.

Regardless, what I said initially holds true ultimately: either epa-file
should not be relied on, or a-s-p should ensure it is present.  I
gravitate towards the former, as it reduces the complexity of getting a
password-store entry.

> Hunting for this problem I recommend to use
> (debug-on-variable-change 'file-name-handler-alist)

That is too verbose.  The following appears to work well, though:

--8<---------------cut here---------------start------------->8---
(add-variable-watcher
 'file-name-handler-alist
 (lambda (symbol newval operation where)
   (cl-flet ((hefh (val)
	       (seq-some (lambda (x) (equal (cdr x) 'epa-file-handler))
			 val)))
     (let ((hb (hefh file-name-handler-alist))
	   (ha (hefh newval)))
       (cond
	((and hb (not ha))
	 (debug--implement-debug-watch symbol newval operation where))
	((and (not hb) ha)
	 (message "epa-file added")))))))
--8<---------------cut here---------------end--------------->8---

Have a lovely day!

> I haven't followed the whole discussion, so forgive me if this has been
> discussed already.
>
>> Arsen Arsenović
>
> Best regards, Michael.


--
Arsen Arsenović

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 381 bytes --]

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

[J.P.]

Arsen Arsenović <arsen@aarsen.me> writes:

> This patch/bug report addresses a real problem that exists independently
> of what triggered it in my case.
>
>> My gut feeling tells me that this could be the real problem, and we
>> need to solve this instead of bypassing the problem with another
>> patch, which could introduce further problems.
>
> Your gut's nearly certainly right here :-)  I am still hunting for the
> cause of that issue.

Perhaps it couldn't hurt to get that somewhat sorted before modifying
`auth-source-pass--read-entry'.

> Regardless, what I said initially holds true ultimately: either epa-file
> should not be relied on, or a-s-p should ensure it is present.  I
> gravitate towards the former, as it reduces the complexity of getting a
> password-store entry.
>
>> Hunting for this problem I recommend to use
>> (debug-on-variable-change 'file-name-handler-alist)
>
> That is too verbose.  The following appears to work well, though:
>
> (add-variable-watcher
>  'file-name-handler-alist
>  (lambda (symbol newval operation where)
>    (cl-flet ((hefh (val)
> 	       (seq-some (lambda (x) (equal (cdr x) 'epa-file-handler))
> 			 val)))
>      (let ((hb (hefh file-name-handler-alist))
> 	   (ha (hefh newval)))
>        (cond
> 	((and hb (not ha))
> 	 (debug--implement-debug-watch symbol newval operation where))
> 	((and (not hb) ha)
> 	 (message "epa-file added")))))))

I can't imagine

  (rassq 'epa-file-handler val)

differing from

  (car (memq epa-file-handler val)) ; w/o the quote

But if it somehow does, that could provide an insight into the cause as
well. Just a thought.

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

[Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Arsen Arsenović <arsen@aarsen.me> writes:

> Hi Michael,

Hi Arsen,

> Michael Albinus <michael.albinus@gmx.de> writes:
>
>> Arsen Arsenović <arsen@aarsen.me> writes:
>>
>> Hi,
>>
>>>>> This happens due to the call of `epa-file-disable' mentioned in the
>>>>> initial recipe.
>>>>
>>>> IIUC, the call in question was only included in the recipe to simulate
>>>> the effect of the entry disappearing, and Arsen is still trying to
>>>> pinpoint the actual cause.
>>>
>>> Indeed.  That's a topic for a different bug report, though ;P
>>
>> Do you have a recipe for the problem w/o calling epa-disble-file?
>
> No.
>
> This patch/bug report addresses a real problem that exists independently
> of what triggered it in my case.

The problem happens when epa-file-handler is removed from
file-name-handler-alist, and no other handler responsible for *.gpg
files is active. Understood.

However, in normal use cases, nobody removes this handler. If I'm wrong,
I'd like to iunderstand those use cases.

So we must document, that auth-source-pass.el depends on such a
handler. We could also add a check, that there is such a handler, and
return either nil if it is missing, or return an error. As a first step,
we could add a note in the manual, see (info "(auth) The Unix password store")

Just implementing an alternative doesn't sound the right way. This would
also increase maintainance burden, if something changes how *.gpg files
shall be handled.

As example, remote files won't work when tramp-file-name-handler is
removed from file-name-handler-alist. It would be a strange approach to
implement a Tramp alternative in packades depending on Tramp, just in case.

> Your gut's nearly certainly right here :-)  I am still hunting for the
> cause of that issue.

Good.

> Regardless, what I said initially holds true ultimately: either epa-file
> should not be relied on, or a-s-p should ensure it is present.  I
> gravitate towards the former, as it reduces the complexity of getting a
> password-store entry.

I vote for the latter, because it simplifies overall maintainability.

> Have a lovely day!
>
> Arsen Arsenović

Best regards, Michael.

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

[Arsen Arsenović via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Hi J.P,

"J.P." <jp@neverwas.me> writes:

> Arsen Arsenović <arsen@aarsen.me> writes:
>
>> This patch/bug report addresses a real problem that exists independently
>> of what triggered it in my case.
>>
>>> My gut feeling tells me that this could be the real problem, and we
>>> need to solve this instead of bypassing the problem with another
>>> patch, which could introduce further problems.
>>
>> Your gut's nearly certainly right here :-)  I am still hunting for the
>> cause of that issue.
>
> Perhaps it couldn't hurt to get that somewhat sorted before modifying
> `auth-source-pass--read-entry'.

I firmly believe that these are two separate bugs, one of which
triggered the other.  The reason for that is because I can reproduce
this bug by simply running 'epa-file-disable', without invoking the
original bug that revealed it to me.

>> Regardless, what I said initially holds true ultimately: either epa-file
>> should not be relied on, or a-s-p should ensure it is present.  I
>> gravitate towards the former, as it reduces the complexity of getting a
>> password-store entry.
>>
>>> Hunting for this problem I recommend to use
>>> (debug-on-variable-change 'file-name-handler-alist)
>>
>> That is too verbose.  The following appears to work well, though:
>>
>> (add-variable-watcher
>>  'file-name-handler-alist
>>  (lambda (symbol newval operation where)
>>    (cl-flet ((hefh (val)
>> 	       (seq-some (lambda (x) (equal (cdr x) 'epa-file-handler))
>> 			 val)))
>>      (let ((hb (hefh file-name-handler-alist))
>> 	   (ha (hefh newval)))
>>        (cond
>> 	((and hb (not ha))
>> 	 (debug--implement-debug-watch symbol newval operation where))
>> 	((and (not hb) ha)
>> 	 (message "epa-file added")))))))
>
> I can't imagine
>
>   (rassq 'epa-file-handler val)
>
> differing from
>
>   (car (memq epa-file-handler val)) ; w/o the quote
>
> But if it somehow does, that could provide an insight into the cause as
> well. Just a thought.

Interesting, I didn't realize epa-file-handler is also a variable
besides just being a function.  I also didn't know rassq exists!  Goes
to show how novice I am in elisp :-)

I will clean up that code above.

You're right WRT those possibly different being interesting, I'll try to
catch that, too.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 381 bytes --]

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

[Arsen Arsenović via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Hi Michael,

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

>> No.
>>
>> This patch/bug report addresses a real problem that exists independently
>> of what triggered it in my case.
>
> The problem happens when epa-file-handler is removed from
> file-name-handler-alist, and no other handler responsible for *.gpg
> files is active. Understood.
>
> However, in normal use cases, nobody removes this handler. If I'm wrong,
> I'd like to iunderstand those use cases.

Based on observations during the last 24h I've noticed that many Emacs
functions do, in fact, reset f-n-h-a to nil.  I'm yet to spot the
combination of calls that leaves epa-file not added back in.

I know that it happens sporadically, though, and that it does not appear
to be via a let-binding, following passwords failing to fetch correctly,
I can't open PGP-encrypted files.  The latter fact is how I initially
figured to inspect auth-source-pass.

> So we must document, that auth-source-pass.el depends on such a
> handler. We could also add a check, that there is such a handler, and
> return either nil if it is missing, or return an error. As a first step,
> we could add a note in the manual, see (info "(auth) The Unix password store")

An error is preferable.  IIRC, auth-source caches negatives too.

> Just implementing an alternative doesn't sound the right way. This would
> also increase maintainance burden, if something changes how *.gpg files
> shall be handled.

I see where you're coming from.  I propose refactoring EPA to expose a
function to insert encrypted file contents as if via i-f-c, but without
requiring f-n-h-a as a solution to that issue.

That could lead to a more consistent user experience, too.

> As example, remote files won't work when tramp-file-name-handler is
> removed from file-name-handler-alist. It would be a strange approach to
> implement a Tramp alternative in packades depending on Tramp, just in case.

Correct.

The difference here is that password store entries are by definition
PGP-encrypted files.  They are not by definition possibly remote files
exposed via TRAMP.

The latter working is a nicety of Emacs design.  The former is crucial
to interacting with the password store.

>> Your gut's nearly certainly right here :-)  I am still hunting for the
>> cause of that issue.
>
> Good.
>
>> Regardless, what I said initially holds true ultimately: either epa-file
>> should not be relied on, or a-s-p should ensure it is present.  I
>> gravitate towards the former, as it reduces the complexity of getting a
>> password-store entry.
>
> I vote for the latter, because it simplifies overall maintainability.

I disagree.  I think that involving the f-n-h-a mechanism for handling
PGP files ultimately introduces implicitly far more complexity, even if
the code is slightly briefer, precisely because of this dependency.

In addition, the user can't reasonably customize reading PGP files
substantially without breaking the contract with the password store.
This, to me, means that supporting that scenario isn't very useful,
especially in a program like Emacs, where any component can be changed
on the fly, leaving the user with the option of customizing more
directly.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 381 bytes --]

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

[Eli Zaretskii]

> From: Arsen Arsenović <arsen@aarsen.me>
> Cc: "J.P." <jp@neverwas.me>, Damien Cassou <damien@cassou.me>, Eli
>   Zaretskii <eliz@gnu.org>, 67937@debbugs.gnu.org
> Date: Sun, 24 Dec 2023 11:37:55 +0100
> 
> >> Regardless, what I said initially holds true ultimately: either epa-file
> >> should not be relied on, or a-s-p should ensure it is present.  I
> >> gravitate towards the former, as it reduces the complexity of getting a
> >> password-store entry.
> >
> > I vote for the latter, because it simplifies overall maintainability.
> 
> I disagree.  I think that involving the f-n-h-a mechanism for handling
> PGP files ultimately introduces implicitly far more complexity, even if
> the code is slightly briefer, precisely because of this dependency.

I disagree with your disagreement, and agree with Michael here.  I see
no maintainer's complexity in using file-name handlers that could be
avoided by not using them: file-name handlers are, and will always be,
an integral part of Emacs internals, so thinking about them as
"complexity" makes no more sense than, say, thinking about GC as
complexity.

P.S. Would people please not use "shorthands" like "f-n-h-a" and
"a-s-p", but instead use the full names?  Those "shorthands" make the
text harder to read, while OTOH typing them in full using M-/ is very
easy and takes only a couple of keypresses.

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

[Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Arsen Arsenović <arsen@aarsen.me> writes:

> Hi J.P,

Hi Arsen,

>> Perhaps it couldn't hurt to get that somewhat sorted before modifying
>> `auth-source-pass--read-entry'.
>
> I firmly believe that these are two separate bugs, one of which
> triggered the other.  The reason for that is because I can reproduce
> this bug by simply running 'epa-file-disable', without invoking the
> original bug that revealed it to me.

auth-source-pass.el depends on an active epa-file-handler. It should
check this, and report an error if it isn't there. There's nothing else
to do, IMHO.

> Thanks, have a lovely day!
>
> Arsen Arsenović

Best regards, Michael.

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

[Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Arsen Arsenović <arsen@aarsen.me> writes:

> Hi Michael,

Hi Arsen,

> Based on observations during the last 24h I've noticed that many Emacs
> functions do, in fact, reset f-n-h-a to nil.  I'm yet to spot the
> combination of calls that leaves epa-file not added back in.

No package in Emacs should reset file-name-handler-alist to nil. If you
find such code anywhere, please report an error.

What is possible is to let-bind file-name-handler-alist to nil.

> Thanks, have a lovely day!
>
> Arsen Arsenović

Best regards, Michael.

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

[Arsen Arsenović via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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


Eli Zaretskii <eliz@gnu.org> writes:

>> From: Arsen Arsenović <arsen@aarsen.me>
>> Cc: "J.P." <jp@neverwas.me>, Damien Cassou <damien@cassou.me>, Eli
>>   Zaretskii <eliz@gnu.org>, 67937@debbugs.gnu.org
>> Date: Sun, 24 Dec 2023 11:37:55 +0100
>>
>> >> Regardless, what I said initially holds true ultimately: either epa-file
>> >> should not be relied on, or a-s-p should ensure it is present.  I
>> >> gravitate towards the former, as it reduces the complexity of getting a
>> >> password-store entry.
>> >
>> > I vote for the latter, because it simplifies overall maintainability.
>>
>> I disagree.  I think that involving the f-n-h-a mechanism for handling
>> PGP files ultimately introduces implicitly far more complexity, even if
>> the code is slightly briefer, precisely because of this dependency.
>
> I disagree with your disagreement, and agree with Michael here.  I see
> no maintainer's complexity in using file-name handlers that could be
> avoided by not using them: file-name handlers are, and will always be,
> an integral part of Emacs internals, so thinking about them as
> "complexity" makes no more sense than, say, thinking about GC as
> complexity.

In that case, auth-source-pass should ensure it's there.  This is where
the complexity I refer to creeps in.  Now auth-source-pass needs to
alter and restore file-name-handler-alist as appropriate.  This means
that it has to get involved with global state, potentially impacting
other functions it calls.

It seems to me more reliable to alter EPA to provide an
insert-file-contents functions for direct use.  This is less composable
and elegant than file-name handlers, naturally, but it is also exactly
what a password-store read requires.

Naturally, file-name handlers are integral and highly important (and we
had an example in this thread: Tramp support /just works/ due to them!
A thing of beauty, really), but this is a rare instance in which a
file-name handler is absolutely required for correct operation, rather
than supplementing existing functionality, which is why I see this
instance differently.

> P.S. Would people please not use "shorthands" like "f-n-h-a" and
> "a-s-p", but instead use the full names?  Those "shorthands" make the
> text harder to read, while OTOH typing them in full using M-/ is very
> easy and takes only a couple of keypresses.

Sure.  These became a bad habit of mine due to Emacs handling them
often.  Apologies.
--
Arsen Arsenović

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 381 bytes --]

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

[Arsen Arsenović via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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


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

> Arsen Arsenović <arsen@aarsen.me> writes:
>
>> Hi Michael,
>
> Hi Arsen,
>
>> Based on observations during the last 24h I've noticed that many Emacs
>> functions do, in fact, reset f-n-h-a to nil.  I'm yet to spot the
>> combination of calls that leaves epa-file not added back in.
>
> No package in Emacs should reset file-name-handler-alist to nil. If you
> find such code anywhere, please report an error.
>
> What is possible is to let-bind file-name-handler-alist to nil.

This is effectively equivalent to being reset to nil for library
functions such as auth-source-search (which calls
auth-source-pass--read-entry eventually), as this is global state that
applies for called functions, no matter how deep down the call stack.

>> Thanks, have a lovely day!
>>
>> Arsen Arsenović
>
> Best regards, Michael.


--
Arsen Arsenović

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 381 bytes --]

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

[Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Arsen Arsenović <arsen@aarsen.me> writes:

Hi Arsen,

>>> I disagree.  I think that involving the f-n-h-a mechanism for handling
>>> PGP files ultimately introduces implicitly far more complexity, even if
>>> the code is slightly briefer, precisely because of this dependency.
>>
>> I disagree with your disagreement, and agree with Michael here.  I see
>> no maintainer's complexity in using file-name handlers that could be
>> avoided by not using them: file-name handlers are, and will always be,
>> an integral part of Emacs internals, so thinking about them as
>> "complexity" makes no more sense than, say, thinking about GC as
>> complexity.
>
> In that case, auth-source-pass should ensure it's there.  This is where
> the complexity I refer to creeps in.  Now auth-source-pass needs to
> alter and restore file-name-handler-alist as appropriate.  This means
> that it has to get involved with global state, potentially impacting
> other functions it calls.

No, auth-source-pass should not enable it on its own I believe. It
should fire an error, which hopefully produces a backtrace. This
backtrace would help us to understand, what's up.

> It seems to me more reliable to alter EPA to provide an
> insert-file-contents functions for direct use.  This is less composable
> and elegant than file-name handlers, naturally, but it is also exactly
> what a password-store read requires.

No. There is no reason to implement this.

> Arsen Arsenović

Best regards, Michael.

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

[Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Arsen Arsenović <arsen@aarsen.me> writes:

Hi Arsen,

>>> Based on observations during the last 24h I've noticed that many Emacs
>>> functions do, in fact, reset f-n-h-a to nil.  I'm yet to spot the
>>> combination of calls that leaves epa-file not added back in.
>>
>> No package in Emacs should reset file-name-handler-alist to nil. If you
>> find such code anywhere, please report an error.
>>
>> What is possible is to let-bind file-name-handler-alist to nil.
>
> This is effectively equivalent to being reset to nil for library
> functions such as auth-source-search (which calls
> auth-source-pass--read-entry eventually), as this is global state that
> applies for called functions, no matter how deep down the call stack.

Sure, the effect is the same. I just wanted to underline, that setting
file-name-handler-alist to nil by means of setq or alike would be
vandalism :-)

> Arsen Arsenović

Best regards, Michael.

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

[Arsen Arsenović via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Hi Michael,

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

> Arsen Arsenović <arsen@aarsen.me> writes:
>
> Hi Arsen,
>
>>>> I disagree.  I think that involving the f-n-h-a mechanism for handling
>>>> PGP files ultimately introduces implicitly far more complexity, even if
>>>> the code is slightly briefer, precisely because of this dependency.
>>>
>>> I disagree with your disagreement, and agree with Michael here.  I see
>>> no maintainer's complexity in using file-name handlers that could be
>>> avoided by not using them: file-name handlers are, and will always be,
>>> an integral part of Emacs internals, so thinking about them as
>>> "complexity" makes no more sense than, say, thinking about GC as
>>> complexity.
>>
>> In that case, auth-source-pass should ensure it's there.  This is where
>> the complexity I refer to creeps in.  Now auth-source-pass needs to
>> alter and restore file-name-handler-alist as appropriate.  This means
>> that it has to get involved with global state, potentially impacting
>> other functions it calls.
>
> No, auth-source-pass should not enable it on its own I believe. It
> should fire an error, which hopefully produces a backtrace. This
> backtrace would help us to understand, what's up.

I doubt that it would produce a useful backtrace, because I doubt a
well-behaved let-binding is causing an error (as I said, when I notice
this bug, epa-file stops working everywhere, even long after a potential
let-binding would've been unbound, implying that it gets unset via some
other means).

Nonetheless, it is worth a shot.  I will inject a check into my
currently running Emacs and see what happens.

I think erroring is an acceptable solution, though (but I do not think
the same of returning nil).

>> It seems to me more reliable to alter EPA to provide an
>> insert-file-contents functions for direct use.  This is less composable
>> and elegant than file-name handlers, naturally, but it is also exactly
>> what a password-store read requires.
>
> No. There is no reason to implement this.

It would prevent a potential error (the one suggested above) when it is
clear how a file must be read (which is always, as password-store
entries are always exactly PGP-encrypted files).

I'm also not sure how complex the heuristic for emitting this error
would be.  (memq epa-file-handler file-name-handler-alist) is not
adequate as non-EPA handlers for PGP files could be active and/or
preferred.  (assoc (car epa-file-handler) file-name-handler-alist) is
also not quite correct as regexes not exactly equal to (car
epa-file-handler) could still match PGP files.

I'm willing to implement a solution if you know a better heuristic.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 381 bytes --]

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

[Arsen Arsenović via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Hi Michael,

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

>> This is effectively equivalent to being reset to nil for library
>> functions such as auth-source-search (which calls
>> auth-source-pass--read-entry eventually), as this is global state that
>> applies for called functions, no matter how deep down the call stack.
>
> Sure, the effect is the same. I just wanted to underline, that setting
> file-name-handler-alist to nil by means of setq or alike would be
> vandalism :-)

Ah, yes, then we agree :-)

>> Arsen Arsenović
>
> Best regards, Michael.

Have a lovely day :-)
--
Arsen Arsenović

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 381 bytes --]

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

[Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Arsen Arsenović <arsen@aarsen.me> writes:

> Hi Michael,

>> No, auth-source-pass should not enable it on its own I believe. It
>> should fire an error, which hopefully produces a backtrace. This
>> backtrace would help us to understand, what's up.
>
> I doubt that it would produce a useful backtrace, because I doubt a
> well-behaved let-binding is causing an error (as I said, when I notice
> this bug, epa-file stops working everywhere, even long after a potential
> let-binding would've been unbound, implying that it gets unset via some
> other means).

But we shall try it.

> Nonetheless, it is worth a shot.  I will inject a check into my
> currently running Emacs and see what happens.
>
> I think erroring is an acceptable solution, though (but I do not think
> the same of returning nil).

Would be OK for me. Please add a hint to the error, that the user shall
contact the Emacs department about. In case your patch arrives the repository.

> I'm also not sure how complex the heuristic for emitting this error
> would be.  (memq epa-file-handler file-name-handler-alist) is not
> adequate as non-EPA handlers for PGP files could be active and/or
> preferred.

Well, it could be a starter. As you said, you have observed
file-name-handler-alist being nil, so this test would be good enough for now.

We have also (find-file-name-handler FILENAME 'insert-file-contents)
But the interpretation of the result is a little bit more tricky.

> I'm willing to implement a solution if you know a better heuristic.

Let's start with what we have. Thanks!

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

Best regards, Michael.

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

[Arsen Arsenović via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Hi Michael,

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

> Arsen Arsenović <arsen@aarsen.me> writes:
>
>> Hi Michael,
>
>>> No, auth-source-pass should not enable it on its own I believe. It
>>> should fire an error, which hopefully produces a backtrace. This
>>> backtrace would help us to understand, what's up.
>>
>> I doubt that it would produce a useful backtrace, because I doubt a
>> well-behaved let-binding is causing an error (as I said, when I notice
>> this bug, epa-file stops working everywhere, even long after a potential
>> let-binding would've been unbound, implying that it gets unset via some
>> other means).
>
> But we shall try it.

So I did.  With the diff below, I ran into an issue: the error emitted
in it is caught.

I believe that the check utilized below is correct for the
check-and-error solution.

--8<---------------cut here---------------start------------->8---
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.
--8<---------------cut here---------------end--------------->8---

>> Nonetheless, it is worth a shot.  I will inject a check into my
>> currently running Emacs and see what happens.
>>
>> I think erroring is an acceptable solution, though (but I do not think
>> the same of returning nil).
>
> Would be OK for me. Please add a hint to the error, that the user shall
> contact the Emacs department about. In case your patch arrives the repository.
>
>> I'm also not sure how complex the heuristic for emitting this error
>> would be.  (memq epa-file-handler file-name-handler-alist) is not
>> adequate as non-EPA handlers for PGP files could be active and/or
>> preferred.
>
> Well, it could be a starter. As you said, you have observed
> file-name-handler-alist being nil, so this test would be good enough for now.
>
> We have also (find-file-name-handler FILENAME 'insert-file-contents)
> But the interpretation of the result is a little bit more tricky.
>
>> I'm willing to implement a solution if you know a better heuristic.
>
> Let's start with what we have. Thanks!
>
>> Thanks, have a lovely day.
>>
>> Arsen Arsenović
>
> Best regards, Michael.


--
Arsen Arsenović

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 251 bytes --]

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

[Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Arsen Arsenović <arsen@aarsen.me> writes:

> Hi Michael,

Hi Arsen,

> So I did.  With the diff below, I ran into an issue: the error emitted
> in it is caught.

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 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---

> Arsen Arsenović

Best regards, Michael.