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