Re: bug#58985: 29.0.50; Have auth-source-pass behave more like other back ends

[Akib Azmain Turja]

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

> Akib Azmain Turja <akib@disroot.org> writes:
>
>> Why the closure doesn't capture "s"?  For me, the following code
>> captures "s" (obviously with lexical binding): (just let-wrapped version
>> of your code)
>>
>> (let ((e '(:secret "topsecret")))
>>   (when-let* ((s (plist-get e :secret)) ; s not captured by closure
>>               (v (auth-source--obfuscate s)))
>>     (setf (plist-get e :secret)
>>           (lambda () (auth-source--deobfuscate v))))
>>   e)
>> ;; => (:secret
>> ;;     (closure
>> ;;         ((p #1)
>> ;;          (v . "XIcHKKIKtavKgK8J6zXP1w==-N/XAaAOqAtGcCzKGKX71og==")
>> ;;          (s . "topsecret") ;; LEAKED!!!
>> ;;          (e :secret #1)
>> ;;          t)
>> ;;         nil
>> ;;       (auth-source--deobfuscate v)))
>>
>
> Looks like you don't have:
>
>   commit 1b1ffe07897ebe06cf96ab423fad3cde9fd6c981
>   Author: Stefan Monnier <monnier@iro.umontreal.ca>
>   Date:   Mon Oct 17 17:11:40 2022 -0400
>   
>       (Ffunction): Make interpreted closures safe for space
>     
> It's easiest to just make a habit of applying patches on the latest
> HEAD. Once you do, you'll find that the output of your example changes.
> If ELPA's Compat ever takes an interest, I suppose a backported version
> could just `byte-compile' the lambda.

That's a recent commit, I'm using Emacs from a commit over two months
ago (I tried to upgrade just a few days before Eglot merged, but was
forced to revert due to native compilation errors).

>
>>> +        (push e out)))))
>>
>> [...]
>>
>>> +(defun auth-source-pass--retrieve-parsed (seen path port-number-p)
>>> +  (when-let ((m (string-match auth-source-pass--match-regexp path)))
>>
>> Why do you let-bound "m"?
>
> Because I am slow and blind, I guess.
>
>>  I can't find any use of it in the body.
>
> Go figure. (Thanks.)

I can't find any existence of "m".

>
>>> +(defun auth-source-pass--find-match-many (hosts users ports require max)
>>> +  "Return plists for valid combinations of HOSTS, USERS, PORTS.
>>> +Each plist contains, at the very least, a host and a secret."
>>> +  (let ((seen (make-hash-table :test #'equal))
>>> +        (entries (auth-source-pass-entries))
>>> +        out)
>>> +    (catch 'done
>>> +      (dolist (host hosts out)
>>> +        (pcase-let ((`(,_ ,u ,p) (auth-source-pass--disambiguate host)))
>>> +          (unless (or (not (equal "443" p)) (string-prefix-p "https://"; 
>>> host))
>>> +            (setq p nil))
>>> +          (dolist (user (or users (list u)))
>>> +            (dolist (port (or ports (list p)))
>>> +              (dolist (e entries)
>>> +                (when-let*
>>> +                    ((m (or (gethash e seen) 
>>> (auth-source-pass--retrieve-parsed
>>> +                                              seen e (integerp port))))
>>> +                     ((equal host (plist-get m :host)))
>>> +                     ((auth-source-pass--match-parts m :port port require))
>>> +                     ((auth-source-pass--match-parts m :user user require))
>>> +                     (parsed (auth-source-pass-parse-entry e))
>>> +                     ;; For now, ignore body-content pairs, if any,
>>> +                     ;; from `auth-source-pass--parse-data'.
>>> +                     (secret (or (auth-source-pass--get-attr 'secret 
>>> parsed)
>>> +                                 (not (memq :secret require)))))
>>> +                  (push
>>> +                   `( :host ,host ; prefer user-provided :host over h
>>> +                      ,@(and-let* ((u (plist-get m :user))) (list :user u))
>>> +                      ,@(and-let* ((p (plist-get m :port))) (list :port p))
>>> +                      ,@(and secret (not (eq secret t)) (list :secret 
>>> secret)))
>>> +                   out)
>>> +                  (when (or (zerop (cl-decf max))
>>> +                            (null (setq entries (remove e entries))))
>>
>> Remove will create a lot of garbage, e.g. (let ((x '(1 2 3 4 5)))
>> (eq (remove 6 x) x)) and (let ((x '(1 2 3 4 5))) (eq (remove 1 x)
>> (cdr x))) both returns nil.
>
> Since you're clearly aware that, for lists, `remove' just calls `delete'
> on a shallow copy, how could (remove thing x) ever be eq to some nthcdr
> of x so long as both are non-nil?
>
>> If you think delete is OK, go ahead and use it.  If you think remove is
>> better, keep it.  Do whatever you think right.
>
> As I tried to explain in
>
>   https://debbugs.gnu.org/cgi/bugreport.cgi?bug=58985#64
>
> I think `delete' is safe in this situation, assuming of course that, for
> ancient, core functions, the implementation can be construed as the de
> facto interface. Based on your comments, you seem to agree with this
> assumption, which seems only sane. I have thus reverted the change.
>

Any one contributing to core Emacs is almost certain more experienced
that me, so they should ignore me if they wish.

>>
>>> +                    (throw 'done out)))))))))))
>>> +
>>
>> [...]
>
> While I certainly welcome the assiduous scrutinizing of Emacs lisp
> mechanics and technique (truly), I was mainly hoping that, as an avid
> pass user, you would also help flesh out the precise effects of the
> behavior introduced by these changes and hopefully share some insights
> into how they might impact day-to-day usage for the typical pass user.
> Granted, that necessarily involves applying these patches atop your
> daily driver and living with them for a spell and, ideally, investing
> some thought into imagining common usage patterns beyond your own (plus
> any potentially problematic edge cases). If you have the energy to
> devote to (perhaps just some of) these areas, it would really help move
> this bug report forward. Thanks.
>
>
>
>

Actually, I'm not very brave, and any damage to my password-store would
be an absolute disaster.

However, I have made a backup and add the encrypted passwords to a Git
repository, and since the patch looks safe, I'm going to apply and test
it.

-- 
Akib Azmain Turja, GPG key: 70018CE5819F17A3BBA666AFE74F0EFA922AE7F5
Fediverse: akib@hostux.social
Codeberg: akib
emailselfdefense.fsf.org | "Nothing can be secure without encryption."

signature.asc
Description: PGP signature