Hi J.P, "J.P." writes: > Hi Arsen, > > Arsen Arsenović writes: > >> "J.P." 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ć