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" <bug-gnu-emacs@gnu.org>]

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