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.