GNU bug report logs -
#58985
29.0.50; Have auth-source-pass behave more like other back ends
Previous Next
Reported by: "J.P." <jp <at> neverwas.me>
Date: Thu, 3 Nov 2022 13:52:02 UTC
Severity: wishlist
Tags: patch
Found in version 29.0.50
Done: "J.P." <jp <at> neverwas.me>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 58985 in the body.
You can then email your comments to 58985 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
emacs-erc <at> gnu.org, bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Thu, 03 Nov 2022 13:52:02 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
"J.P." <jp <at> neverwas.me>:
New bug report received and forwarded. Copy sent to
emacs-erc <at> gnu.org, bug-gnu-emacs <at> gnu.org.
(Thu, 03 Nov 2022 13:52:02 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Tags: patch
Hi people,
This is a belated follow-up to a brief exchange I had with Damien
earlier this year:
https://lists.gnu.org/archive/html/bug-gnu-emacs/2022-04/msg00982.html
To recap, ERC would like to include the UNIX password store in the suite
of available back ends for its auth-source integration. To do that, we'd
need auth-source-pass to either export quite a few internal functions or
offer a bit more in the way of "standard" functionality. Thinking door
#2 the likelier, I've gone ahead and attempted a POC that mainly caters
to ERC's own requirements. (Sadly, I'm not well enough acquainted with
the library to aim much wider than that.) Regardless, I'm hoping someone
more knowledgeable will be willing to give this a think at some point.
Thanks,
J.P.
In GNU Emacs 29.0.50 (build 3, x86_64-pc-linux-gnu, GTK+ Version
3.24.34, cairo version 1.17.6) of 2022-11-01 built on localhost
Repository revision: 9b098c903a2502df42e21fa0796aa35097ae2cfa
Repository branch: auth-source-pass-many
Windowing system distributor 'The X.Org Foundation', version 11.0.12014000
System Description: Fedora Linux 36 (Workstation Edition)
Configured using:
'configure --enable-check-lisp-object-type --enable-checking=yes,glyphs
'CFLAGS=-O0 -g3'
PKG_CONFIG_PATH=:/usr/lib64/pkgconfig:/usr/share/pkgconfig
CC=analyze-cc CXX=analyze-c++'
Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES NOTIFY
INOTIFY PDUMPER PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS WEBP X11 XDBE XIM XINPUT2 XPM GTK3 ZLIB
Important settings:
value of $LANG: en_US.UTF-8
value of $XMODIFIERS: @im=ibus
locale-coding-system: utf-8-unix
Major mode: Lisp Interaction
Minor modes in effect:
tooltip-mode: t
global-eldoc-mode: t
eldoc-mode: t
show-paren-mode: t
electric-indent-mode: t
mouse-wheel-mode: t
tool-bar-mode: t
menu-bar-mode: t
file-name-shadow-mode: t
global-font-lock-mode: t
font-lock-mode: t
blink-cursor-mode: t
line-number-mode: t
indent-tabs-mode: t
transient-mark-mode: t
auto-composition-mode: t
auto-encryption-mode: t
auto-compression-mode: t
Load-path shadows:
None found.
Features:
(shadow sort mail-extr emacsbug message mailcap yank-media puny dired
dired-loaddefs rfc822 mml mml-sec password-cache epa derived epg rfc6068
epg-config gnus-util text-property-search time-date subr-x mm-decode
mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader
cl-loaddefs cl-lib sendmail rfc2047 rfc2045 ietf-drums mm-util
mail-prsvr mail-utils rmc iso-transl tooltip cconv eldoc paren electric
uniquify ediff-hook vc-hooks lisp-float-type elisp-mode mwheel
term/x-win x-win term/common-win x-dnd tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode lisp-mode
prog-mode register page tab-bar menu-bar rfn-eshadow isearch easymenu
timer select scroll-bar mouse jit-lock font-lock syntax font-core
term/tty-colors frame minibuffer nadvice seq simple cl-generic
indonesian philippine cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
composite emoji-zwj charscript charprop case-table epa-hook
jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button loaddefs
theme-loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget keymap
hashtable-print-readable backquote threads dbusbind inotify lcms2
dynamic-setting system-font-setting font-render-setting cairo
move-toolbar gtk x-toolkit xinput2 x multi-tty make-network-process
emacs)
Memory information:
((conses 16 36767 7533)
(symbols 48 5118 0)
(strings 32 13166 1683)
(string-bytes 1 374788)
(vectors 16 9331)
(vector-slots 8 148593 8753)
(floats 8 21 21)
(intervals 56 341 0)
(buffers 984 11))
[0001-POC-Make-auth-source-pass-behave-more-like-other-bac.patch (text/x-patch, attachment)]
[0002-POC-Support-auth-source-pass-in-ERC.patch (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Sat, 05 Nov 2022 23:56:01 GMT)
Full text and
rfc822 format available.
Message #8 received at 58985 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
v2. Respect existing user option.
[0000-v1-v2.diff (text/x-patch, attachment)]
[0001-POC-Make-auth-source-pass-behave-more-like-other-bac.patch (text/x-patch, attachment)]
[0002-POC-Support-auth-source-pass-in-ERC.patch (text/x-patch, attachment)]
Severity set to 'wishlist' from 'normal'
Request was from
"J.P." <jp <at> neverwas.me>
to
control <at> debbugs.gnu.org.
(Sun, 06 Nov 2022 00:10:01 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Sun, 06 Nov 2022 11:24:02 GMT)
Full text and
rfc822 format available.
Message #13 received at 58985 <at> debbugs.gnu.org (full text, mbox):
"J.P." <jp <at> neverwas.me> writes:
Hi,
> v2. Respect existing user option.
I'm not familiar with the auth-source-pass.el implementation, so I
cannot speak too much about your patch. Reading it roughly, I haven't
found serious flaws, 'tho.
However :-)
--8<---------------cut here---------------start------------->8---
+(defcustom auth-source-pass-standard-search nil
+ "Whether to use more standardized search behavior.
+When nil, the password-store backend works like it always has and
+considers at most one `:user' search parameter and returns at
+most one result. With t, it tries to more faithfully mimic other
+auth-source backends."
+ :version "29.1"
+ :type 'boolean)
--8<---------------cut here---------------end--------------->8---
- The name of this user option as well as its docstring are focussed on
the current behavior. People won't know what "mimic other auth-source
backends" would mean. Please describe the effect w/o that comparison,
and pls give it a name based on its effect, and not "...-standard-search".
- I'm missing the documentation in doc/misc/auth.texi and etc/NEWS.
Best regards, Michael.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Sun, 06 Nov 2022 14:40:02 GMT)
Full text and
rfc822 format available.
Message #16 received at submit <at> debbugs.gnu.org (full text, mbox):
Hi J.P.,
thank you very much for working on auth-source-pass. I think it's fine
to break backward compatibility if it makes auth-source-pass closer to
what auth-source requires.
I don't have time to review the code though, I'm sorry.
Best
--
Damien Cassou
"Success is the ability to go from one failure to another without
losing enthusiasm." --Winston Churchill
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Mon, 07 Nov 2022 05:00:02 GMT)
Full text and
rfc822 format available.
Message #19 received at 58985 <at> debbugs.gnu.org (full text, mbox):
Hi Damien,
Damien Cassou <damien <at> cassou.me> writes:
> I think it's fine to break backward compatibility if it makes
> auth-source-pass closer to what auth-source requires.
There's some nice behavior that you introduced initially regarding
the narrowing of results, namely (from the info manual):
If several entries match, the one matching the most items (where an
"item" is one of username, port or host) is preferred. For example ...
It'd be a shame to lose that, since folks may have come to rely on it.
Perhaps it would be prudent to offer an escape hatch of some sort to
restore the existing behavior?
> I don't have time to review the code though, I'm sorry.
No worries at all.
Unfortunately, I don't use pass myself and am mostly concerned with
ERC's integration. The good news is an actual pass user, Akib (Cc'd),
has expressed some interest regarding this topic on emacs-devel, so I'm
hoping they'll step in and take over or collaborate in some fashion.
Also, I noticed that the password-store.el in zx2c4's contrib/emacs
subdir actually requires auth-source as a dependency, so I've Cc'd the
maintainer for that package as well.
Thanks,
J.P.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Mon, 07 Nov 2022 05:01:02 GMT)
Full text and
rfc822 format available.
Message #22 received at 58985 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Hi Michael,
Michael Albinus <michael.albinus <at> gmx.de> writes:
> I'm not familiar with the auth-source-pass.el implementation, so I
> cannot speak too much about your patch. Reading it roughly, I haven't
> found serious flaws, 'tho.
Thanks for taking a look!
> However :-)
>
> +(defcustom auth-source-pass-standard-search nil
> + "Whether to use more standardized search behavior.
> +When nil, the password-store backend works like it always has and
> +considers at most one `:user' search parameter and returns at
> +most one result. With t, it tries to more faithfully mimic other
> +auth-source backends."
> + :version "29.1"
> + :type 'boolean)
>
> - The name of this user option as well as its docstring are focussed on
> the current behavior. People won't know what "mimic other auth-source
> backends" would mean. Please describe the effect w/o that comparison,
> and pls give it a name based on its effect, and not "...-standard-search".
I've changed the name to `auth-source-pass-extra-query-keywords' and
updated the description to something hopefully more adequate.
> - I'm missing the documentation in doc/misc/auth.texi and etc/NEWS.
Added.
BTW, I was initially thinking it'd be better to wait for a more
comprehensive and maintainable solution, like something based around a
larger set of common functions to be shared among the various back ends
(hence the [POC] qualifier on my patches). However, I suppose such a
thing could be done later, once the desired behavior is all dialed in
(perhaps alongside addressing support for full CRUD operations, which
are still missing, AFIAK). Anyway, I really don't know enough about pass
or auth-source to commit to such an endeavor. But I've reached out to
some folks who may be able to lend a hand.
Thanks,
J.P.
[0000-v2-v3.diff (text/x-patch, attachment)]
[0001-POC-Make-auth-source-pass-behave-more-like-other-bac.patch (text/x-patch, attachment)]
[0002-POC-Support-auth-source-pass-in-ERC.patch (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Mon, 07 Nov 2022 10:35:01 GMT)
Full text and
rfc822 format available.
Message #25 received at 58985 <at> debbugs.gnu.org (full text, mbox):
"J.P." <jp <at> neverwas.me> writes:
> Hi Michael,
Hi,
>> +(defcustom auth-source-pass-standard-search nil
>> + "Whether to use more standardized search behavior.
>> +When nil, the password-store backend works like it always has and
>> +considers at most one `:user' search parameter and returns at
>> +most one result. With t, it tries to more faithfully mimic other
>> +auth-source backends."
>> + :version "29.1"
>> + :type 'boolean)
>>
>> - The name of this user option as well as its docstring are focussed on
>> the current behavior. People won't know what "mimic other auth-source
>> backends" would mean. Please describe the effect w/o that comparison,
>> and pls give it a name based on its effect, and not "...-standard-search".
>
> I've changed the name to `auth-source-pass-extra-query-keywords' and
> updated the description to something hopefully more adequate.
>
>> - I'm missing the documentation in doc/misc/auth.texi and etc/NEWS.
>
> Added.
Thanks.
> BTW, I was initially thinking it'd be better to wait for a more
> comprehensive and maintainable solution, like something based around a
> larger set of common functions to be shared among the various back ends
> (hence the [POC] qualifier on my patches). However, I suppose such a
> thing could be done later, once the desired behavior is all dialed in
> (perhaps alongside addressing support for full CRUD operations, which
> are still missing, AFIAK). Anyway, I really don't know enough about pass
> or auth-source to commit to such an endeavor. But I've reached out to
> some folks who may be able to lend a hand.
Such a change would be desirable. However, Ted, the author of
auth-source.el, isn't active these days. Personally I feel responsible
for the secrets backend, and I try bug fixing in the other auth-source
parts. That's all.
According to admin/MAINTAINERS, nobody else feels responsible for
auth-source. So I doubt that such a change will happen soon.
From my pov you could push the changes. But as you said the other
message, you'll wait for feeback fron users. That's OK, but pls take
into account that later this month an emacs-29 branch will be cut
off. Feature changes shall be pushed until then.
> Thanks,
> J.P.
Best regards, Michael.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Tue, 08 Nov 2022 13:58:02 GMT)
Full text and
rfc822 format available.
Message #28 received at 58985 <at> debbugs.gnu.org (full text, mbox):
Michael Albinus <michael.albinus <at> gmx.de> writes:
> From my pov you could push the changes. But as you said the other
> message, you'll wait for feeback fron users. That's OK, but pls take
> into account that later this month an emacs-29 branch will be cut
> off. Feature changes shall be pushed until then.
Right, good point. I guess if no one else weighs in by this time next
week, we can flip a coin or something. Thanks.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Wed, 09 Nov 2022 18:33:01 GMT)
Full text and
rfc822 format available.
Message #31 received at 58985 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Michael Albinus <michael.albinus <at> gmx.de> writes:
> "J.P." <jp <at> neverwas.me> writes:
>
> Hi,
>
>> v2. Respect existing user option.
>
> I'm not familiar with the auth-source-pass.el implementation, so I
> cannot speak too much about your patch. Reading it roughly, I haven't
> found serious flaws, 'tho.
It has a serious flaw AFAIK. I have a password entry
"akib <at> disroot.org", and this legitimate search query doesn't find it:
(auth-source-search :host "disroot.org")
But if specify the user, it finds the entry:
(auth-source-search :host "disroot.org" :user "akib")
And the entries can also be ambiguous. For example, the entry at path
"foo.org/bar.net" might be interpreted as the password of bar.net, or
as the password of the user "bar.net" on "foo.org". The current
implementation seems to interpret such entries unpredictably.
>
> However :-)
>
> +(defcustom auth-source-pass-standard-search nil
> + "Whether to use more standardized search behavior.
> +When nil, the password-store backend works like it always has and
> +considers at most one `:user' search parameter and returns at
> +most one result. With t, it tries to more faithfully mimic other
> +auth-source backends."
> + :version "29.1"
> + :type 'boolean)
>
> - The name of this user option as well as its docstring are focussed on
> the current behavior. People won't know what "mimic other auth-source
> backends" would mean. Please describe the effect w/o that comparison,
> and pls give it a name based on its effect, and not "...-standard-search".
I agree. This variable should be something like
"auth-source-pass-old-search" (or even "...-obsolete-search"). And the
default should be nil, because it fixes many bugs, and it's pointless to
disable the fixes by the default.
>
> - I'm missing the documentation in doc/misc/auth.texi and etc/NEWS.
What documentation? Of this change or anything else? I think we should
focus on the implement before writing documentation.
>
> Best regards, Michael.
>
>
>
--
Akib Azmain Turja, GPG key: 70018CE5819F17A3BBA666AFE74F0EFA922AE7F5
Fediverse: akib <at> hostux.social
Codeberg: akib
emailselfdefense.fsf.org | "Nothing can be secure without encryption."
[signature.asc (application/pgp-signature, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Wed, 09 Nov 2022 18:33:02 GMT)
Full text and
rfc822 format available.
Message #34 received at 58985 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
"J.P." <jp <at> neverwas.me> writes:
> --- a/doc/misc/erc.texi
> +++ b/doc/misc/erc.texi
> @@ -861,7 +861,8 @@ Connecting
> @code{erc-auth-source-search}. It tries to merge relevant contextual
> parameters with those provided or discovered from the logical connection
> or the underlying transport. Some auth-source back ends may not be
> -compatible; netrc, plstore, json, and secrets are currently supported.
> +compatible; netrc, plstore, json, secrets, and pass are currently
> +supported.
> @end defopt
Is pass really unsupported? I have been using pass with ERC without any
problem. Am I lucky?
--
Akib Azmain Turja, GPG key: 70018CE5819F17A3BBA666AFE74F0EFA922AE7F5
Fediverse: akib <at> hostux.social
Codeberg: akib
emailselfdefense.fsf.org | "Nothing can be secure without encryption."
[signature.asc (application/pgp-signature, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Thu, 10 Nov 2022 00:40:01 GMT)
Full text and
rfc822 format available.
Message #37 received at 58985 <at> debbugs.gnu.org (full text, mbox):
"J.P." <jp <at> neverwas.me> writes:
> Michael Albinus <michael.albinus <at> gmx.de> writes:
>
>> From my pov you could push the changes. But as you said the other
>> message, you'll wait for feeback fron users. That's OK, but pls take
>> into account that later this month an emacs-29 branch will be cut
>> off. Feature changes shall be pushed until then.
>
> Right, good point. I guess if no one else weighs in by this time next
> week, we can flip a coin or something. Thanks.
Sorry that I come a little late to this but will this mean the backend
will act less like Passwordstore.org describes or more?
I think the backend should follow the users organization of the
passwordstore folder if possible.
Br,
Björn
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Thu, 10 Nov 2022 05:24:01 GMT)
Full text and
rfc822 format available.
Message #40 received at 58985 <at> debbugs.gnu.org (full text, mbox):
Hi Akib,
Akib Azmain Turja <akib <at> disroot.org> writes:
> Michael Albinus <michael.albinus <at> gmx.de> writes:
>
>> "J.P." <jp <at> neverwas.me> writes:
>>
>> Hi,
>>
>>> v2. Respect existing user option.
>>
>> I'm not familiar with the auth-source-pass.el implementation, so I
>> cannot speak too much about your patch. Reading it roughly, I haven't
>> found serious flaws, 'tho.
>
> It has a serious flaw AFAIK. I have a password entry
> "akib <at> disroot.org", and this legitimate search query doesn't find it:
>
> (auth-source-search :host "disroot.org")
>
> But if specify the user, it finds the entry:
>
> (auth-source-search :host "disroot.org" :user "akib")
Hm, that's unfortunate. I specifically added a pair of tests just for
this, namely
auth-source-pass-extra-query-keywords--netrc-akib
auth-source-pass-extra-query-keywords--akib
Are you able to pinpoint why they're reporting a false positive by any
chance (or give a minimal repro recipe with an FS tree layout of some
~/.password-store)? Also, and I'm not trying to be insulting here, but
did you remember to rerun Make after applying the patch(es)?
> And the entries can also be ambiguous. For example, the entry at path
> "foo.org/bar.net" might be interpreted as the password of bar.net, or
> as the password of the user "bar.net" on "foo.org". The current
> implementation seems to interpret such entries unpredictably.
Sounds convincing. What do you think about deprecating the /user form?
(This may have to be spun off into a separate bug report.)
At the end of the day, I'm more concerned about consistency (and thus
predictability) than anything. IOW, I'd be okay with "foo.org/bar.net"
being parsed either way, as long as it's the *same* way every time,
which we could then document. If you're indeed finding otherwise, please
provide an MRE for this as well (with patches applied, of course).
>> - The name of this user option as well as its docstring are focussed on
>> the current behavior. People won't know what "mimic other auth-source
>> backends" would mean. Please describe the effect w/o that comparison,
>> and pls give it a name based on its effect, and not "...-standard-search".
>
> I agree. This variable should be something like
> "auth-source-pass-old-search" (or even "...-obsolete-search").
Wait, but `auth-source-pass-old-search' sounds like we're regressing to
describing a comparison rather than an effect. The name in the second
(v2) iteration, `auth-source-pass-extra-query-keywords', was an attempt
to rein in the scope of the option and convey no more than what it's
claiming to offer.
> And the default should be nil, because it fixes many bugs, and it's
> pointless to disable the fixes by the default.
Not sure I agree here, even though Damien seems to be in accord. In the
interest of minimizing churn for Melpa's pass and password-store
packages, I'd rather make this an opt-in for Emacs 29 if we end up
including it at all.
>> - I'm missing the documentation in doc/misc/auth.texi and etc/NEWS.
>
> What documentation? Of this change or anything else? I think we should
> focus on the implement before writing documentation.
Hm, (again, not trying to insult here, but) did you somehow miss the
patches attached to the email you replied to? It kind of looks that way
based on your comments. If I'm wrong, though, please forgive; I
appreciate your input regardless.
Thanks,
J.P.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Thu, 10 Nov 2022 05:26:01 GMT)
Full text and
rfc822 format available.
Message #43 received at 58985 <at> debbugs.gnu.org (full text, mbox):
Björn Bidar <bjorn.bidar <at> thaodan.de> writes:
> "J.P." <jp <at> neverwas.me> writes:
>
>> Michael Albinus <michael.albinus <at> gmx.de> writes:
>>
>>> From my pov you could push the changes. But as you said the other
>>> message, you'll wait for feeback fron users. That's OK, but pls take
>>> into account that later this month an emacs-29 branch will be cut
>>> off. Feature changes shall be pushed until then.
>>
>> Right, good point. I guess if no one else weighs in by this time next
>> week, we can flip a coin or something. Thanks.
>
> Sorry that I come a little late to this but
No worries at all.
I know this is asking a lot, but if you get a chance, please apply the
v2 patches and try them out. (Actually, you can omit the second one in
the set, which only affects ERC.)
> will this mean the backend will act less like Passwordstore.org
> describes or more?
That's a good question. My main goal thus far has been to make its query
behavior as close as possible to that of the other auth-source back
ends. Glancing at that web page, it seems auth-source-pass has taken
some liberties WRT to query features, like drilling down into the tail
of a file's contents and ascribing semantics to parts of a file name.
> I think the backend should follow the users organization of the
> passwordstore folder if possible.
From this I'll infer that the current implementation of auth-source-pass
does that sufficiently. If that's so and the changes I'm proposing
threaten to interfere with that, what's your opinion on the default
value of a knob to toggle the new behavior?
Thanks,
J.P.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Thu, 10 Nov 2022 05:27:02 GMT)
Full text and
rfc822 format available.
Message #46 received at 58985 <at> debbugs.gnu.org (full text, mbox):
Akib Azmain Turja <akib <at> disroot.org> writes:
> "J.P." <jp <at> neverwas.me> writes:
>
>> --- a/doc/misc/erc.texi
>> +++ b/doc/misc/erc.texi
>> @@ -861,7 +861,8 @@ Connecting
>> @code{erc-auth-source-search}. It tries to merge relevant contextual
>> parameters with those provided or discovered from the logical connection
>> or the underlying transport. Some auth-source back ends may not be
>> -compatible; netrc, plstore, json, and secrets are currently supported.
>> +compatible; netrc, plstore, json, secrets, and pass are currently
>> +supported.
>> @end defopt
>
> Is pass really unsupported? I have been using pass with ERC without any
> problem. Am I lucky?
It appears you are lucky. Please see toward the bottom of
https://git.savannah.gnu.org/cgit/emacs.git/tree/test/lisp/erc/erc-services-tests.el?id=ef362750#n452
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Thu, 10 Nov 2022 07:47:01 GMT)
Full text and
rfc822 format available.
Message #49 received at 58985 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
"J.P." <jp <at> neverwas.me> writes:
> Hi Michael,
>
> Michael Albinus <michael.albinus <at> gmx.de> writes:
>
>> I'm not familiar with the auth-source-pass.el implementation, so I
>> cannot speak too much about your patch. Reading it roughly, I haven't
>> found serious flaws, 'tho.
>
> Thanks for taking a look!
>
>> However :-)
>>
>> +(defcustom auth-source-pass-standard-search nil
>> + "Whether to use more standardized search behavior.
>> +When nil, the password-store backend works like it always has and
>> +considers at most one `:user' search parameter and returns at
>> +most one result. With t, it tries to more faithfully mimic other
>> +auth-source backends."
>> + :version "29.1"
>> + :type 'boolean)
>>
>> - The name of this user option as well as its docstring are focussed on
>> the current behavior. People won't know what "mimic other auth-source
>> backends" would mean. Please describe the effect w/o that comparison,
>> and pls give it a name based on its effect, and not "...-standard-search".
>
> I've changed the name to `auth-source-pass-extra-query-keywords' and
> updated the description to something hopefully more adequate.
>
>> - I'm missing the documentation in doc/misc/auth.texi and etc/NEWS.
>
> Added.
>
> BTW, I was initially thinking it'd be better to wait for a more
> comprehensive and maintainable solution, like something based around a
> larger set of common functions to be shared among the various back ends
> (hence the [POC] qualifier on my patches). However, I suppose such a
> thing could be done later, once the desired behavior is all dialed in
> (perhaps alongside addressing support for full CRUD operations, which
> are still missing, AFIAK). Anyway, I really don't know enough about pass
> or auth-source to commit to such an endeavor. But I've reached out to
> some folks who may be able to lend a hand.
>
> Thanks,
> J.P.
>
>
> From 450e2f029a26b30d583afcb44e7fdd561a95c277 Mon Sep 17 00:00:00 2001
> From: "F. Jason Park" <jp <at> neverwas.me>
> Date: Tue, 1 Nov 2022 22:46:24 -0700
> Subject: [PATCH 1/2] [POC] Make auth-source-pass behave more like other
> backends
>
> * lisp/auth-source-pass.el (auth-source-pass-extra-query-keywords): Add
> new option to bring search behavior more in line with other backends.
> (auth-source-pass-search): Add new keyword params `max' and `require'
> and consider new option `auth-source-pass-extra-query-keywords' for
> dispatch.
> (auth-source-pass--match-regexp, auth-source-pass--retrieve-parsed,
> auth-source-pass--match-parts): Add supporting variable and helpers.
> (auth-source-pass--build-result-many,
> auth-source-pass--find-match-many): Add "-many" variants for existing
> workhorse functions.
> * test/lisp/auth-source-pass-tests.el
> (auth-source-pass-extra-query-keywords--wild-port-miss-netrc,
> auth-source-pass-extra-query-keywords--wild-port-miss,
> auth-source-pass-extra-query-keywords--wild-port-hit-netrc,
> auth-source-pass-extra-query-keywords--wild-port-hit,
> auth-source-pass-extra-query-keywords--wild-port-req-miss-netrc,
> auth-source-pass-extra-query-keywords--wild-port-req-miss,
> auth-source-pass-extra-query-keywords--baseline,
> auth-source-pass-extra-query-keywords--port-type,
> auth-source-pass-extra-query-keywords--hosts-first): Add juxtaposed
> netrc and extra-query-keywords pairs to demo optional extra-compliant
> behavior.
> * doc/misc/auth.texi: Add option
> `auth-source-pass-extra-query-keywords' to auth-source-pass section.
> * etc/NEWS: Mention `auth-source-pass-extra-query-keywords' in Emacs
> 29.1 package changes section.
> ---
> doc/misc/auth.texi | 11 +++
> etc/NEWS | 8 ++
> lisp/auth-source-pass.el | 109 ++++++++++++++++++++-
> test/lisp/auth-source-pass-tests.el | 144 ++++++++++++++++++++++++++++
> 4 files changed, 271 insertions(+), 1 deletion(-)
>
> diff --git a/doc/misc/auth.texi b/doc/misc/auth.texi
> index 9dc63af6bc..222fce2058 100644
> --- a/doc/misc/auth.texi
> +++ b/doc/misc/auth.texi
> @@ -526,6 +526,8 @@ The Unix password store
> while searching for an entry matching the @code{rms} user on host
> @code{gnu.org} and port @code{22}, then the entry
> @file{gnu.org:22/rms.gpg} is preferred over @file{gnu.org.gpg}.
> +However, such filtering is not applied when the option
> +@code{auth-source-pass-extra-parameters} is set to @code{t}.
>
> Users of @code{pass} may also be interested in functionality provided
> by other Emacs packages:
> @@ -549,6 +551,15 @@ The Unix password store
> port in an entry. Defaults to @samp{:}.
> @end defvar
>
> +@defvar auth-source-pass-extra-query-keywords
> +Set this to @code{t} if you encounter problems predicting the outcome
> +of searches relative to other auth-source backends or if you have code
> +that expects to query multiple backends uniformly. This tells
> +auth-source-pass to consider the @code{:max} and @code{:require}
> +keywords as well as lists containing multiple query params (for
> +applicable keywords).
> +@end defvar
> +
The name won't make much sense to the ordinary user who don't know about
the API.
Repeating from another message, this variable should be something like
"auth-source-pass-old-search" (or even "...-obsolete-search").
> @node Help for developers
> @chapter Help for developers
>
> diff --git a/etc/NEWS b/etc/NEWS
> index 89da8aa63f..776936489f 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -1383,6 +1383,14 @@ If non-nil and there's only one matching option, auto-select that.
> If non-nil, this user option describes what entries not to add to the
> database stored on disk.
>
> +** Auth-Source
> +
> ++++
> +*** New user option 'auth-source-pass-extra-query-keywords'.
> +Whether to recognize additional keyword params, like ':max' and
> +':require', as well as accept lists of query terms paired with
> +applicable keywords.
> +
> ** Dired
>
> +++
> diff --git a/lisp/auth-source-pass.el b/lisp/auth-source-pass.el
> index 0955e2ed07..d9129667e1 100644
> --- a/lisp/auth-source-pass.el
> +++ b/lisp/auth-source-pass.el
> @@ -55,13 +55,27 @@ auth-source-pass-port-separator
> :type 'string
> :version "27.1")
>
> +(defcustom auth-source-pass-extra-query-keywords nil
> + "Whether to consider additional keywords when performing a query.
> +Specifically, when the value is t, recognize the `:max' and
> +`:require' keywords and accept lists of query parameters for
> +certain keywords, such as `:host' and `:user'. Also, wrap all
> +returned secrets in a function and forgo any further results
> +filtering unless given an applicable `:require' argument. When
> +this option is nil, do none of that, and enact the narrowing
> +behavior described toward the bottom of the Info node `(auth) The
> +Unix password store'."
> + :type 'boolean
> + :version "29.1")
> +
This should be non-nil by default, since it fixes a number of bugs. We
don't want to deprive users from these fixes, do we?
REPEAT: The name won't make much sense to the ordinary user who don't
know about the API.
Repeating from another message, this variable should be something like
"auth-source-pass-old-search" (or even "...-obsolete-search").
> (cl-defun auth-source-pass-search (&rest spec
> &key backend type host user port
> + require max
> &allow-other-keys)
> "Given some search query, return matching credentials.
>
> See `auth-source-search' for details on the parameters SPEC, BACKEND, TYPE,
> -HOST, USER and PORT."
> +HOST, USER, PORT, REQUIRE, and MAX."
> (cl-assert (or (null type) (eq type (oref backend type)))
> t "Invalid password-store search: %s %s")
> (cond ((eq host t)
> @@ -70,6 +84,8 @@ auth-source-pass-search
> ((null host)
> ;; Do not build a result, as none will match when HOST is nil
> nil)
> + (auth-source-pass-extra-query-keywords
> + (auth-source-pass--build-result-many host port user require max))
> (t
> (when-let ((result (auth-source-pass--build-result host port user)))
> (list result)))))
> @@ -89,6 +105,41 @@ auth-source-pass--build-result
> (seq-subseq retval 0 -2)) ;; remove password
> retval))))
LGTM.
>
> +(defvar auth-source-pass--match-regexp nil)
> +
> +(defun auth-source-pass--match-regexp (s)
> + (rx-to-string ; autoloaded
> + `(: (or bot "/")
> + (or (: (? (group-n 20 (+ (not (in ?\ ?/ ?@ ,s)))) "@")
> + (group-n 10 (+ (not (in ?\ ?/ ?@ ,s))))
> + (? ,s (group-n 30 (+ (not (in ?\ ?/ ,s))))))
> + (: (group-n 11 (+ (not (in ?\ ?/ ?@ ,s))))
> + (? ,s (group-n 31 (+ (not (in ?\ ?/ ,s)))))
> + (? "/" (group-n 21 (+ (not (in ?\ ?/ ,s)))))))
> + eot)
> + 'no-group))
LGTM.
> +
> +(defun auth-source-pass--build-result-many (hosts ports users require max)
> + "Return multiple `auth-source-pass--build-result' values."
> + (unless (listp hosts) (setq hosts (list hosts)))
> + (unless (listp users) (setq users (list users)))
> + (unless (listp ports) (setq ports (list ports)))
> + (let* ((auth-source-pass--match-regexp (auth-source-pass--match-regexp
> + auth-source-pass-port-separator))
> + (rv (auth-source-pass--find-match-many hosts users ports
> + require (or max 1))))
> + (when auth-source-debug
> + (auth-source-pass--do-debug "final result: %S" rv))
> + (if (eq auth-source-pass-extra-query-keywords 'test)
> + (reverse rv)
The value `test' is not documented. Is it used in tests? If it is, I
think an internal variable would be better.
> + (let (out)
> + (dolist (e rv out)
> + (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))))
> + (push e out))))))
> +
LGTM.
> ;;;###autoload
> (defun auth-source-pass-enable ()
> "Enable auth-source-password-store."
> @@ -206,6 +257,62 @@ auth-source-pass--find-match
> hosts
> (list hosts))))
>
> +(defun auth-source-pass--retrieve-parsed (seen path port-number-p)
> + (when-let ((m (string-match auth-source-pass--match-regexp path)))
> + (puthash path
> + (list :host (or (match-string 10 path) (match-string 11 path))
> + :user (or (match-string 20 path) (match-string 21 path))
> + :port (and-let* ((p (or (match-string 30 path)
> + (match-string 31 path)))
> + (n (string-to-number p)))
> + (if (or (zerop n) (not port-number-p))
> + (format "%s" p)
> + n)))
> + seen)))
LGTM.
> +
> +(defun auth-source-pass--match-parts (parts key value require)
> + (let ((mv (plist-get parts key)))
> + (if (memq key require)
> + (and value (equal mv value))
> + (or (not value) (not mv) (equal mv value)))))
LGTM.
> +
> +;; For now, this ignores the contents of files and only considers path
> +;; components when matching.
The file name contains host, user and port, so parsing contents is not
needed at all.
> +(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))
> + port-number-p
> + 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))
Can "auth-source-pass--disambiguate" return host with the protocol part?
> + (setq p nil))
> + (dolist (user (or users (list u)))
> + (dolist (port (or ports (list p)))
> + (setq port-number-p (equal 'integer (type-of port)))
Just saw the first use of "type-of". Doesn't "(integerp port)" work?
> + (dolist (e entries)
> + (when-let*
> + ((m (or (gethash e seen) (auth-source-pass--retrieve-parsed
> + seen e port-number-p)))
> + ((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))
> + (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)
LGTM.
> + (when (or (zerop (cl-decf max))
> + (null (setq entries (delete e entries))))
Can the delete call conflict with the dolist loop?
> + (throw 'done out)))))))))))
> +
> (defun auth-source-pass--disambiguate (host &optional user port)
> "Return (HOST USER PORT) after disambiguation.
> Disambiguate between having user provided inside HOST (e.g.,
> diff --git a/test/lisp/auth-source-pass-tests.el b/test/lisp/auth-source-pass-tests.el
I don't have much idea about these tests, but...
> index f5147a7ce0..718c7cf4ba 100644
> --- a/test/lisp/auth-source-pass-tests.el
> +++ b/test/lisp/auth-source-pass-tests.el
> @@ -488,6 +488,150 @@ auth-source-pass-prints-meaningful-debug-log
> (should (auth-source-pass--have-message-matching
> "found 2 entries matching \"gitlab.com\": (\"a/gitlab.com\" \"b/gitlab.com\")"))))
>
> +
> +;; FIXME move this to top of file if keeping these netrc tests
> +(require 'ert-x)
> +
> +;; No entry has the requested port, but a result is still returned.
> +
> +(ert-deftest auth-source-pass-extra-query-keywords--wild-port-miss-netrc ()
> + (ert-with-temp-file netrc-file
> + :text "\
> +machine x.com password a
> +machine x.com port 42 password b
> +"
> + (let* ((auth-sources (list netrc-file))
> + (auth-source-do-cache nil)
> + (results (auth-source-search :host "x.com" :port 22 :max 2)))
> + (dolist (result results)
> + (setf result (plist-put result :secret (auth-info-password result))))
> + (should (equal results '((:host "x.com" :secret "a")))))))
How this is testing auth-source-pass?
> +
> +(ert-deftest auth-source-pass-extra-query-keywords--wild-port-miss ()
> + (let ((auth-source-pass-extra-query-keywords 'test))
> + (auth-source-pass--with-store '(("x.com" (secret . "a"))
> + ("x.com:42" (secret . "b")))
> + (auth-source-pass-enable)
> + (should (equal (auth-source-search :host "x.com" :port 22 :max 2)
> + '((:host "x.com" :secret "a")))))))
> +
> +;; One of two entries has the requested port, both returned
> +
> +(ert-deftest auth-source-pass-extra-query-keywords--wild-port-hit-netrc ()
> + (ert-with-temp-file netrc-file
> + :text "\
> +machine x.com password a
> +machine x.com port 42 password b
> +"
> + (let* ((auth-sources (list netrc-file))
> + (auth-source-do-cache nil)
> + (results (auth-source-search :host "x.com" :port 42 :max 2)))
> + (dolist (result results)
> + (setf result (plist-put result :secret (auth-info-password result))))
> + (should (equal results '((:host "x.com" :secret "a")
> + (:host "x.com" :port "42" :secret "b")))))))
> +
> +(ert-deftest auth-source-pass-extra-query-keywords--wild-port-hit ()
> + (let ((auth-source-pass-extra-query-keywords 'test))
> + (auth-source-pass--with-store '(("x.com" (secret . "a"))
> + ("x.com:42" (secret . "b")))
> + (auth-source-pass-enable)
> + (should (equal (auth-source-search :host "x.com" :port 42 :max 2)
> + '((:host "x.com" :secret "a")
> + (:host "x.com" :port 42 :secret "b")))))))
> +
> +;; No entry has the requested port, but :port is required, so search fails
> +
> +(ert-deftest auth-source-pass-extra-query-keywords--wild-port-req-miss-netrc ()
> + (ert-with-temp-file netrc-file
> + :text "\
> +machine x.com password a
> +machine x.com port 42 password b
> +"
> + (let* ((auth-sources (list netrc-file))
> + (auth-source-do-cache nil)
> + (results (auth-source-search
> + :host "x.com" :port 22 :require '(:port) :max 2)))
> + (should-not results))))
> +
> +(ert-deftest auth-source-pass-extra-query-keywords--wild-port-req-miss ()
> + (let ((auth-source-pass-extra-query-keywords 'test))
> + (auth-source-pass--with-store '(("x.com" (secret . "a"))
> + ("x.com:42" (secret . "b")))
> + (auth-source-pass-enable)
> + (should-not (auth-source-search
> + :host "x.com" :port 22 :require '(:port) :max 2)))))
> +
> +;; Specifying a :host without a :user finds a lone entry and does not
> +;; include extra fields (i.e., :port nil) in the result
> +;; https://lists.gnu.org/archive/html/emacs-devel/2022-11/msg00130.html
> +
> +(ert-deftest auth-source-pass-extra-query-keywords--netrc-akib ()
> + (ert-with-temp-file netrc-file
> + :text "\
> +machine x.com password a
> +machine disroot.org user akib password b
> +machine z.com password c
> +"
> + (let* ((auth-sources (list netrc-file))
> + (auth-source-do-cache nil)
> + (results (auth-source-search :host "disroot.org" :max 2)))
> + (dolist (result results)
> + (setf result (plist-put result :secret (auth-info-password result))))
> + (should (equal results
> + '((:host "disroot.org" :user "akib" :secret "b")))))))
> +
> +(ert-deftest auth-source-pass-extra-query-keywords--akib ()
> + (let ((auth-source-pass-extra-query-keywords 'test))
> + (auth-source-pass--with-store '(("x.com" (secret . "a"))
> + ("akib <at> disroot.org" (secret . "b"))
> + ("z.com" (secret . "c")))
> + (auth-source-pass-enable)
> + (should (equal (auth-source-search :host "disroot.org" :max 2)
> + '((:host "disroot.org" :user "akib" :secret "b")))))))
> +
> +;; A retrieved store entry mustn't be nil regardless of whether its
> +;; path contains port or user components
> +
> +(ert-deftest auth-source-pass-extra-query-keywords--baseline ()
> + (let ((auth-source-pass-extra-query-keywords 'test))
> + (auth-source-pass--with-store '(("x.com"))
> + (auth-source-pass-enable)
> + (should-not (auth-source-search :host "x.com")))))
> +
> +;; Output port type (int or string) matches that of input parameter
> +
> +(ert-deftest auth-source-pass-extra-query-keywords--port-type ()
> + (let ((auth-source-pass-extra-query-keywords 'test))
> + (auth-source-pass--with-store '(("x.com:42" (secret . "a")))
> + (auth-source-pass-enable)
> + (should (equal (auth-source-search :host "x.com" :port 42)
> + '((:host "x.com" :port 42 :secret "a")))))
> + (auth-source-pass--with-store '(("x.com:42" (secret . "a")))
> + (auth-source-pass-enable)
> + (should (equal (auth-source-search :host "x.com" :port "42")
> + '((:host "x.com" :port "42" :secret "a")))))))
> +
> +;; The :host search param ordering more heavily influences the output
> +;; because (h1, u1, p1), (h1, u1, p2), ... (hN, uN, pN); also, exact
> +;; matches are not given precedence, i.e., matching store items are
> +;; returned in the order encountered
> +
> +(ert-deftest auth-source-pass-extra-query-keywords--hosts-first ()
> + (let ((auth-source-pass-extra-query-keywords 'test))
> + (auth-source-pass--with-store '(("x.com:42/bar" (secret . "a"))
> + ("gnu.org" (secret . "b"))
> + ("x.com" (secret . "c"))
> + ("fake.com" (secret . "d"))
> + ("x.com/foo" (secret . "e")))
> + (auth-source-pass-enable)
> + (should (equal (auth-source-search :host '("x.com" "gnu.org") :max 3)
> + ;; Notice gnu.org is never considered ^
> + '((:host "x.com" :user "bar" :port "42" :secret "a")
> + (:host "x.com" :secret "c")
> + (:host "x.com" :user "foo" :secret "e")))))))
> +
> +
> (provide 'auth-source-pass-tests)
>
> ;;; auth-source-pass-tests.el ends here
--
Akib Azmain Turja --- https://akib.codeberg.page/
GPG key: 70018CE5819F17A3BBA666AFE74F0EFA922AE7F5
Fediverse: akib <at> hostux.social, Codeberg: akib
emailselfdefense.fsf.org | "Nothing can be secure without encryption."
[signature.asc (application/pgp-signature, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Thu, 10 Nov 2022 07:47:01 GMT)
Full text and
rfc822 format available.
Message #52 received at 58985 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Akib Azmain Turja via "Bug reports for GNU Emacs, the Swiss army knife
of text editors" <bug-gnu-emacs <at> gnu.org> writes:
> Michael Albinus <michael.albinus <at> gmx.de> writes:
>
>> "J.P." <jp <at> neverwas.me> writes:
>>
>> Hi,
>>
>>> v2. Respect existing user option.
>>
>> I'm not familiar with the auth-source-pass.el implementation, so I
>> cannot speak too much about your patch. Reading it roughly, I haven't
>> found serious flaws, 'tho.
>
> It has a serious flaw AFAIK. I have a password entry
> "akib <at> disroot.org", and this legitimate search query doesn't find it:
>
> (auth-source-search :host "disroot.org")
>
> But if specify the user, it finds the entry:
>
> (auth-source-search :host "disroot.org" :user "akib")
>
> And the entries can also be ambiguous. For example, the entry at path
> "foo.org/bar.net" might be interpreted as the password of bar.net, or
> as the password of the user "bar.net" on "foo.org". The current
> implementation seems to interpret such entries unpredictably.
>
I mean, the current implementation, not the patch.
--
Akib Azmain Turja --- https://akib.codeberg.page/
GPG key: 70018CE5819F17A3BBA666AFE74F0EFA922AE7F5
Fediverse: akib <at> hostux.social, Codeberg: akib
emailselfdefense.fsf.org | "Nothing can be secure without encryption."
[signature.asc (application/pgp-signature, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Thu, 10 Nov 2022 07:48:02 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Thu, 10 Nov 2022 08:22:01 GMT)
Full text and
rfc822 format available.
Message #58 received at 58985 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
"J.P." <jp <at> neverwas.me> writes:
> Hi Akib,
>
> Akib Azmain Turja <akib <at> disroot.org> writes:
>
>> Michael Albinus <michael.albinus <at> gmx.de> writes:
>>
>>> "J.P." <jp <at> neverwas.me> writes:
>>>
>>> Hi,
>>>
>>>> v2. Respect existing user option.
>>>
>>> I'm not familiar with the auth-source-pass.el implementation, so I
>>> cannot speak too much about your patch. Reading it roughly, I haven't
>>> found serious flaws, 'tho.
>>
>> It has a serious flaw AFAIK. I have a password entry
>> "akib <at> disroot.org", and this legitimate search query doesn't find it:
>>
>> (auth-source-search :host "disroot.org")
>>
>> But if specify the user, it finds the entry:
>>
>> (auth-source-search :host "disroot.org" :user "akib")
>
> Hm, that's unfortunate. I specifically added a pair of tests just for
> this, namely
>
> auth-source-pass-extra-query-keywords--netrc-akib
> auth-source-pass-extra-query-keywords--akib
>
> Are you able to pinpoint why they're reporting a false positive by any
> chance (or give a minimal repro recipe with an FS tree layout of some
> ~/.password-store)? Also, and I'm not trying to be insulting here, but
> did you remember to rerun Make after applying the patch(es)?
>
Actually, I didn't review the patches in this email, I just commented on
the auth-source-pass in the master *right now*, not the patch. Sorry
for the trouble.
>> And the entries can also be ambiguous. For example, the entry at path
>> "foo.org/bar.net" might be interpreted as the password of bar.net, or
>> as the password of the user "bar.net" on "foo.org". The current
>> implementation seems to interpret such entries unpredictably.
>
> Sounds convincing. What do you think about deprecating the /user form?
> (This may have to be spun off into a separate bug report.)
>
> At the end of the day, I'm more concerned about consistency (and thus
> predictability) than anything. IOW, I'd be okay with "foo.org/bar.net"
> being parsed either way, as long as it's the *same* way every time,
> which we could then document. If you're indeed finding otherwise, please
> provide an MRE for this as well (with patches applied, of course).
>
>>> - The name of this user option as well as its docstring are focussed on
>>> the current behavior. People won't know what "mimic other auth-source
>>> backends" would mean. Please describe the effect w/o that comparison,
>>> and pls give it a name based on its effect, and not "...-standard-search".
>>
>> I agree. This variable should be something like
>> "auth-source-pass-old-search" (or even "...-obsolete-search").
>
> Wait, but `auth-source-pass-old-search' sounds like we're regressing to
> describing a comparison rather than an effect. The name in the second
> (v2) iteration, `auth-source-pass-extra-query-keywords', was an attempt
> to rein in the scope of the option and convey no more than what it's
> claiming to offer.
Thanks for clarification. I have written the same thing in my another
(actual) patch review email, feel free to ignore those parts.
>
>> And the default should be nil, because it fixes many bugs, and it's
>> pointless to disable the fixes by the default.
>
> Not sure I agree here, even though Damien seems to be in accord. In the
> interest of minimizing churn for Melpa's pass and password-store
> packages, I'd rather make this an opt-in for Emacs 29 if we end up
> including it at all.
>
How about communicating with them?
>>> - I'm missing the documentation in doc/misc/auth.texi and etc/NEWS.
>>
>> What documentation? Of this change or anything else? I think we should
>> focus on the implement before writing documentation.
>
> Hm, (again, not trying to insult here, but) did you somehow miss the
> patches attached to the email you replied to? It kind of looks that way
> based on your comments. If I'm wrong, though, please forgive; I
> appreciate your input regardless.
Yeah, you are right, I didn't notice those patches and just commented on
the auth-source-pass in the master *right now*, not the patch. Please
forgive for the trouble.
>
> Thanks,
> J.P.
>
>
>
--
Akib Azmain Turja --- https://akib.codeberg.page/
GPG key: 70018CE5819F17A3BBA666AFE74F0EFA922AE7F5
Fediverse: akib <at> hostux.social, Codeberg: akib
emailselfdefense.fsf.org | "Nothing can be secure without encryption."
[signature.asc (application/pgp-signature, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Thu, 10 Nov 2022 13:41:02 GMT)
Full text and
rfc822 format available.
Message #61 received at 58985 <at> debbugs.gnu.org (full text, mbox):
"J.P." <jp <at> neverwas.me> writes:
> I know this is asking a lot, but if you get a chance, please apply the
> v2 patches and try them out. (Actually, you can omit the second one in
> the set, which only affects ERC.)
I want to add I'm not an ERC user but circe user, I've got interested in
the patch as I use the backend with circe, gnus, magit, elfeed and so
on.
>> will this mean the backend will act less like Passwordstore.org
>> describes or more?
>
> That's a good question. My main goal thus far has been to make its query
> behavior as close as possible to that of the other auth-source back
> ends. Glancing at that web page, it seems auth-source-pass has taken
> some liberties WRT to query features, like drilling down into the tail
> of a file's contents and ascribing semantics to parts of a file name.
A lot of programs don't implement the full path traversal and just end
up having a static or a bogus path (e.g. those that implement
Freedesktop SecretService with pass).
So I favor a correct implementation, any progress is welcome.
>> I think the backend should follow the users organization of the
>> passwordstore folder if possible.
>
> From this I'll infer that the current implementation of auth-source-pass
> does that sufficiently. If that's so and the changes I'm proposing
> threaten to interfere with that, what's your opinion on the default
> value of a knob to toggle the new behavior?
Hm it depends if there are any backends that workaround that old behavior.
From what I see the only difference really is that you can specify
require and max.
My personal bindings for circe to auth-source currently only exist of
small functions:
;; Adopted from Ghub.el, refactored for use with Circe IRC
(defun circe--ident (username network)
(format "%s^%s" username network))
(defun circe--auth-source-get (keys &rest spec)
(declare (indent 1))
(let ((plist (car (apply #'auth-source-search
(append spec (list :max 1))))))
(mapcar (lambda (k)
(plist-get plist k))
keys)))
(defun circe-pass-get (host user &optional network)
"\fn(fn host user &optional network)"
(auth-source-forget (list :host host :user user :max 1))
(when network
(setq user (circe--ident user network)))
(let ((match (car (circe--auth-source-get (list :secret)
:host host :user user))))
(cond ((null match)
(error "Auth source empty for %s %s %s" host user network))
((functionp match)
(funcall match)) (t match))))
Which makes me wonder why ERC needs the different behavior but then I'm
not really a good lisp programmer more a novice.
Br,
Björn
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Thu, 10 Nov 2022 14:39:02 GMT)
Full text and
rfc822 format available.
Message #64 received at 58985 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Akib Azmain Turja <akib <at> disroot.org> writes:
>> +(defcustom auth-source-pass-extra-query-keywords nil
> [...]
>
> This should be non-nil by default, since it fixes a number of bugs. We
> don't want to deprive users from these fixes, do we?
If that's what everyone here agrees to, then fine by me. Hopefully end
users and dependent packages will agree.
>> +(defun auth-source-pass--build-result-many (hosts ports users require max)
>> + "Return multiple `auth-source-pass--build-result' values."
>> + (unless (listp hosts) (setq hosts (list hosts)))
>> + (unless (listp users) (setq users (list users)))
>> + (unless (listp ports) (setq ports (list ports)))
>> + (let* ((auth-source-pass--match-regexp (auth-source-pass--match-regexp
>> + auth-source-pass-port-separator))
>> + (rv (auth-source-pass--find-match-many hosts users ports
>> + require (or max 1))))
>> + (when auth-source-debug
>> + (auth-source-pass--do-debug "final result: %S" rv))
>> + (if (eq auth-source-pass-extra-query-keywords 'test)
>> + (reverse rv)
>
> The value `test' is not documented. Is it used in tests? If it is, I
> think an internal variable would be better.
We could certainly do that. I left it as something uglier and more
sentinel-like for now.
>> +
>> +;; For now, this ignores the contents of files and only considers path
>> +;; components when matching.
>
> The file name contains host, user and port, so parsing contents is not
> needed at all.
Right, but since `auth-source-pass--parse-data' impacts the code path
whenever a multiline file is encountered, I thought the reader should
know that we're consciously disregarding its findings. Anyway, I've
moved the comment somewhere more relevant and reworded it for clarity.
>> +(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))
>> + port-number-p
>> + 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))
>
> Can "auth-source-pass--disambiguate" return host with the protocol part?
No, but it downcases the host, so "Libera.Chat" becomes "libera.chat",
which may be desirable for some use cases but not for ERC's (and I
suspect those of other dependent libraries as well). If I call
`auth-source-search' with :host Libera.Chat or "ircs://irc.libera.chat",
and I get a match, I want the result to contain a host `equal' to the
one I passed in (as is the case with other back ends) and not some
normalized version, like "{,irc.}libera.chat". Likewise, for ports and
users.
>> + (setq p nil))
>> + (dolist (user (or users (list u)))
>> + (dolist (port (or ports (list p)))
>> + (setq port-number-p (equal 'integer (type-of port)))
>
> Just saw the first use of "type-of". Doesn't "(integerp port)" work?
Thanks.
>> + (when (or (zerop (cl-decf max))
>> + (null (setq entries (delete e entries))))
>
> Can the delete call conflict with the dolist loop?
In this particular case, I don't believe so, although things get
confusing when you have duplicates (which we don't). When expanded, we
basically have
(let ((tail entries))
(while tail
(let ((e (car tail)))
(cl-assert (eq (member e entries) tail)) ; invariant
(when ...
(setq entries (delete e entries)))
(setq tail (cdr tail)))))
where the CDR of the current tail may become the CDR of the previous
tail on a match, but that doesn't mutate the former. Regardless, I
suppose it's bad practice to depend on internal implementations, which
could always change, so I've swapped this out for `remove' instead. Good
question.
>> +(ert-deftest auth-source-pass-extra-query-keywords--wild-port-miss-netrc ()
>> + (ert-with-temp-file netrc-file
>> + :text "\
>> +machine x.com password a
>> +machine x.com port 42 password b
>> +"
>> + (let* ((auth-sources (list netrc-file))
>> + (auth-source-do-cache nil)
>> + (results (auth-source-search :host "x.com" :port 22 :max 2)))
>> + (dolist (result results)
>> + (setf result (plist-put result :secret (auth-info-password result))))
>> + (should (equal results '((:host "x.com" :secret "a")))))))
>
> How this is testing auth-source-pass?
It's there for comparison and to cement the behavior of the reference
implementation, netrc, as canon. Notice that those `auth-source-search'
calls for every pair of cases are identical despite having different
back ends (that's the whole point). Happy to move all the netrc variants
to test/lisp/auth-source-tests.el, but locality for juxtaposition's sake
can often be a mercy on tired eyes.
Thanks for the notes.
[0000-v3-v4.diff (text/x-patch, attachment)]
[0001-POC-Make-auth-source-pass-behave-more-like-other-bac.patch (text/x-patch, attachment)]
[0002-POC-Support-auth-source-pass-in-ERC.patch (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Thu, 10 Nov 2022 14:41:02 GMT)
Full text and
rfc822 format available.
Message #67 received at 58985 <at> debbugs.gnu.org (full text, mbox):
Björn Bidar <bjorn.bidar <at> thaodan.de> writes:
> "J.P." <jp <at> neverwas.me> writes:
>
>> I know this is asking a lot, but if you get a chance, please apply the
>> v2 patches and try them out. (Actually, you can omit the second one in
>> the set, which only affects ERC.)
>
> I want to add I'm not an ERC user but circe user, I've got interested in
> the patch as I use the backend with circe, gnus, magit, elfeed and so
> on.
All great packages!
>>> will this mean the backend will act less like Passwordstore.org
>>> describes or more?
>>
>> That's a good question. My main goal thus far has been to make its query
>> behavior as close as possible to that of the other auth-source back
>> ends. Glancing at that web page, it seems auth-source-pass has taken
>> some liberties WRT to query features, like drilling down into the tail
>> of a file's contents and ascribing semantics to parts of a file name.
>
> A lot of programs don't implement the full path traversal and just end
> up having a static or a bogus path (e.g. those that implement
> Freedesktop SecretService with pass).
Interesting. I just blindly assumed auth-source-pass would be alone in
that regard, but I guess not in the slightest.
> So I favor a correct implementation, any progress is welcome.
I don't think correctness from the passwordstore.org perspective will
butt heads with auth-source's because only the latter has any concept of
host, user, and port. Although, as you've noticed, my patch only
addresses queries and doesn't handle writes, which may be a different
animal entirely.
>>> I think the backend should follow the users organization of the
>>> passwordstore folder if possible.
>>
>> From this I'll infer that the current implementation of auth-source-pass
>> does that sufficiently. If that's so and the changes I'm proposing
>> threaten to interfere with that, what's your opinion on the default
>> value of a knob to toggle the new behavior?
>
> Hm it depends if there are any backends that workaround that old behavior.
> From what I see the only difference really is that you can specify
> require and max.
There are actually a few subtle areas where the behavior between old and
new differs and maybe one or two slightly unintuitive gotchas for folks
unfamiliar with how the other back ends operate. If you're curious,
there's a series of side-by-side comparisons added by the first patch
toward the bottom of
test/lisp/auth-source-pass-tests.el
Please let me know if you have any questions.
> My personal bindings for circe to auth-source currently only exist of
> small functions:
> ;; Adopted from Ghub.el, refactored for use with Circe IRC
> (defun circe--ident (username network)
> (format "%s^%s" username network))
> (defun circe--auth-source-get (keys &rest spec)
> (declare (indent 1))
> (let ((plist (car (apply #'auth-source-search
> (append spec (list :max 1))))))
~~~~~~
ERC would choke on this ^
> (mapcar (lambda (k)
> (plist-get plist k))
> keys)))
> (defun circe-pass-get (host user &optional network)
> "\fn(fn host user &optional network)"
> (auth-source-forget (list :host host :user user :max 1))
> (when network
> (setq user (circe--ident user network)))
> (let ((match (car (circe--auth-source-get (list :secret)
> :host host :user user))))
> (cond ((null match)
> (error "Auth source empty for %s %s %s" host user network))
> ((functionp match)
> (funcall match)) (t match))))
>
>
> Which makes me wonder why ERC needs the different behavior but then I'm
> not really a good lisp programmer more a novice.
The approach is broadly similar to what you have. But ERC uses
auth-source to query server passwords, network credentials, and channel
keys more or less transparently, without user interaction. It overloads
both :host and :user to accommodate various values based on context and
doesn't rely on auth-source for narrowing. It asks for all applicable
results and does its own thing from there.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Fri, 11 Nov 2022 03:18:02 GMT)
Full text and
rfc822 format available.
Message #70 received at 58985 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Akib Azmain Turja <akib <at> disroot.org> writes:
>> +(defun auth-source-pass--build-result-many (hosts ports users require max)
>> + "Return multiple `auth-source-pass--build-result' values."
>> + (unless (listp hosts) (setq hosts (list hosts)))
>> + (unless (listp users) (setq users (list users)))
>> + (unless (listp ports) (setq ports (list ports)))
>> + (let* ((auth-source-pass--match-regexp (auth-source-pass--match-regexp
>> + auth-source-pass-port-separator))
>> + (rv (auth-source-pass--find-match-many hosts users ports
>> + require (or max 1))))
>> + (when auth-source-debug
>> + (auth-source-pass--do-debug "final result: %S" rv))
>> + (if (eq auth-source-pass-extra-query-keywords 'test)
>> + (reverse rv)
>
> The value `test' is not documented. Is it used in tests? If it is, I
> think an internal variable would be better.
I got rid of the `test' stuff completely, so this function now always
wraps secrets.
[0000-v4-v5.diff (text/x-patch, attachment)]
[0001-POC-Make-auth-source-pass-behave-more-like-other-bac.patch (text/x-patch, attachment)]
[0002-POC-Support-auth-source-pass-in-ERC.patch (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Fri, 11 Nov 2022 17:36:02 GMT)
Full text and
rfc822 format available.
Message #73 received at 58985 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
"J.P." <jp <at> neverwas.me> writes:
>>> + (if (eq auth-source-pass-extra-query-keywords 'test)
>>> + (reverse rv)
>>
>> The value `test' is not documented. Is it used in tests? If it is, I
>> think an internal variable would be better.
>
> I got rid of the `test' stuff completely, so this function now always
> wraps secrets.
That looks good.
>
>
> From 8870cb62be1ad3ac5b9e5553e52a7f6ed7533c2f Mon Sep 17 00:00:00 2001
> From: "F. Jason Park" <jp <at> neverwas.me>
> Date: Tue, 1 Nov 2022 22:46:24 -0700
> Subject: [PATCH 1/2] [POC] Make auth-source-pass behave more like other
> backends
>
> * lisp/auth-source-pass.el (auth-source-pass-extra-query-keywords): Add
> new option to bring search behavior more in line with other backends.
> (auth-source-pass-search): Add new keyword params `max' and `require'
> and consider new option `auth-source-pass-extra-query-keywords' for
> dispatch.
> (auth-source-pass--match-regexp, auth-source-pass--retrieve-parsed,
> auth-source-pass--match-parts): Add supporting variable and helpers.
> (auth-source-pass--build-result-many,
> auth-source-pass--find-match-many): Add "-many" variants for existing
> workhorse functions.
> * test/lisp/auth-source-pass-tests.el
> (auth-source-pass-extra-query-keywords--wild-port-miss-netrc,
> auth-source-pass-extra-query-keywords--wild-port-miss,
> auth-source-pass-extra-query-keywords--wild-port-hit-netrc,
> auth-source-pass-extra-query-keywords--wild-port-hit,
> auth-source-pass-extra-query-keywords--wild-port-req-miss-netrc,
> auth-source-pass-extra-query-keywords--wild-port-req-miss,
> auth-source-pass-extra-query-keywords--netrc-akib,
> auth-source-pass-extra-query-keywords--akib,
> auth-source-pass-extra-query-keywords--netrc-host,
> auth-source-pass-extra-query-keywords--host,
> auth-source-pass-extra-query-keywords--baseline,
> auth-source-pass-extra-query-keywords--port-type,
> auth-source-pass-extra-query-keywords--hosts-first): Add juxtaposed
> netrc and extra-query-keywords pairs to demo optional extra-compliant
> behavior.
> * doc/misc/auth.texi: Add option
> `auth-source-pass-extra-query-keywords' to auth-source-pass section.
> * etc/NEWS: Mention `auth-source-pass-extra-query-keywords' in Emacs
> 29.1 package changes section. Bug#58985.
> ---
> doc/misc/auth.texi | 11 ++
> etc/NEWS | 8 ++
> lisp/auth-source-pass.el | 105 +++++++++++++++-
> test/lisp/auth-source-pass-tests.el | 184 ++++++++++++++++++++++++++++
> 4 files changed, 307 insertions(+), 1 deletion(-)
>
[...]
> +(defun auth-source-pass--build-result-many (hosts ports users require max)
> + "Return multiple `auth-source-pass--build-result' values."
> + (unless (listp hosts) (setq hosts (list hosts)))
> + (unless (listp users) (setq users (list users)))
> + (unless (listp ports) (setq ports (list ports)))
> + (let* ((auth-source-pass--match-regexp (auth-source-pass--match-regexp
> + auth-source-pass-port-separator))
> + (rv (auth-source-pass--find-match-many hosts users ports
> + require (or max 1))))
> + (when auth-source-debug
> + (auth-source-pass--do-debug "final result: %S" rv))
> + (let (out)
> + (dolist (e rv out)
> + (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))))
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)
--8<---------------cut here---------------start------------->8---
(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)))
--8<---------------cut here---------------end--------------->8---
> + (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"? I can't find any use of it in the body.
> + (puthash path
> + (list :host (or (match-string 10 path) (match-string 11 path))
> + :user (or (match-string 20 path) (match-string 21 path))
> + :port (and-let* ((p (or (match-string 30 path)
> + (match-string 31 path)))
> + (n (string-to-number p)))
> + (if (or (zerop n) (not port-number-p))
> + (format "%s" p)
> + n)))
> + seen)))
[...]
> +(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.
If you think delete is OK, go ahead and use it. If you think remove is
better, keep it. Do whatever you think right.
> + (throw 'done out)))))))))))
> +
[...]
--
Akib Azmain Turja, GPG key: 70018CE5819F17A3BBA666AFE74F0EFA922AE7F5
Fediverse: akib <at> hostux.social
Codeberg: akib
emailselfdefense.fsf.org | "Nothing can be secure without encryption."
[signature.asc (application/pgp-signature, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Sat, 12 Nov 2022 04:31:01 GMT)
Full text and
rfc822 format available.
Message #76 received at 58985 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Akib Azmain Turja <akib <at> 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 <at> 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.
>> + (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.)
>> +(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.
>
>> + (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.
[0000-v5-v6.diff (text/x-patch, attachment)]
[0001-POC-Make-auth-source-pass-behave-more-like-other-bac.patch (text/x-patch, attachment)]
[0002-POC-Support-auth-source-pass-in-ERC.patch (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Sat, 12 Nov 2022 15:27:01 GMT)
Full text and
rfc822 format available.
Message #79 received at 58985 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
"J.P." <jp <at> neverwas.me> writes:
> Akib Azmain Turja <akib <at> 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 <at> 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 <at> hostux.social
Codeberg: akib
emailselfdefense.fsf.org | "Nothing can be secure without encryption."
[signature.asc (application/pgp-signature, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Sun, 13 Nov 2022 08:16:02 GMT)
Full text and
rfc822 format available.
Message #82 received at submit <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Akib Azmain Turja via "Bug reports for GNU Emacs, the Swiss army knife
of text editors" <bug-gnu-emacs <at> gnu.org> writes:
> "J.P." <jp <at> neverwas.me> writes:
>
>> 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.
I have applied the patch the on top commit f8c11b5a, and it works fine.
I did some basic testing (manually) of auth-source-pass and the
dependent packages I use, password-store and pass, and they all seem to
be unaffected when the new option enabled. So I guess we can enable it
by default. I didn't felt the need of test with the new feature
disabled, since the patch doesn't touch any old code.
And I also found that, auth-source finds the entry "akib <at> disroot.org"
correctly with (auth-source-search :host "disroot.org") when the new
user option is set to t.
However, I haven't still installed the Emacs build with the patch
applied as my daily driver, I'm working on that. The tests were
performed on Emacs build without GUI.
--
Akib Azmain Turja, GPG key: 70018CE5819F17A3BBA666AFE74F0EFA922AE7F5
Fediverse: akib <at> hostux.social
Codeberg: akib
emailselfdefense.fsf.org | "Nothing can be secure without encryption."
[signature.asc (application/pgp-signature, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Sun, 13 Nov 2022 08:16:03 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Sun, 13 Nov 2022 15:31:01 GMT)
Full text and
rfc822 format available.
Message #88 received at 58985 <at> debbugs.gnu.org (full text, mbox):
Akib Azmain Turja <akib <at> disroot.org> writes:
> Akib Azmain Turja via "Bug reports for GNU Emacs, the Swiss army knife
> of text editors" <bug-gnu-emacs <at> gnu.org> writes:
>
>> "J.P." <jp <at> neverwas.me> writes:
>>
>>> 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.
>
> I have applied the patch the on top commit f8c11b5a, and it works fine.
>
> I did some basic testing (manually) of auth-source-pass and the
> dependent packages I use, password-store and pass, and they all seem to
> be unaffected when the new option enabled. So I guess we can enable it
> by default. I didn't felt the need of test with the new feature
> disabled, since the patch doesn't touch any old code.
Awesome. Thanks for all the work. I know it's kind of a hassle.
> And I also found that, auth-source finds the entry "akib <at> disroot.org"
> correctly with (auth-source-search :host "disroot.org") when the new
> user option is set to t.
Yeah, it's sometimes tricky to tell if the new code is even running, so
it's great that you checked that.
> However, I haven't still installed the Emacs build with the patch
> applied as my daily driver, I'm working on that. The tests were
> performed on Emacs build without GUI.
OK, nice.
You mentioned previously some potentially surprising ambiguities
surrounding the trailing /user syntax. If any realistic scenarios
present themselves, perhaps we can try to improve the situation if it's
not too far out of scope (or just document the behavior, maybe in a unit
test). Thanks again.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Mon, 14 Nov 2022 08:06:02 GMT)
Full text and
rfc822 format available.
Message #91 received at 58985 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
"J.P." <jp <at> neverwas.me> writes:
> Akib Azmain Turja <akib <at> disroot.org> writes:
>
>> Akib Azmain Turja via "Bug reports for GNU Emacs, the Swiss army knife
>> of text editors" <bug-gnu-emacs <at> gnu.org> writes:
>>
>>> "J.P." <jp <at> neverwas.me> writes:
>>>
>>>> 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.
>>
>> I have applied the patch the on top commit f8c11b5a, and it works fine.
>>
>> I did some basic testing (manually) of auth-source-pass and the
>> dependent packages I use, password-store and pass, and they all seem to
>> be unaffected when the new option enabled. So I guess we can enable it
>> by default. I didn't felt the need of test with the new feature
>> disabled, since the patch doesn't touch any old code.
>
> Awesome. Thanks for all the work. I know it's kind of a hassle.
>
>> And I also found that, auth-source finds the entry "akib <at> disroot.org"
>> correctly with (auth-source-search :host "disroot.org") when the new
>> user option is set to t.
>
> Yeah, it's sometimes tricky to tell if the new code is even running, so
> it's great that you checked that.
I'm pretty sure the new code was running, since I set
auth-source-do-cache to nil to disable cache prior doing the tests.
>
>> However, I haven't still installed the Emacs build with the patch
>> applied as my daily driver, I'm working on that. The tests were
>> performed on Emacs build without GUI.
>
> OK, nice.
>
> You mentioned previously some potentially surprising ambiguities
> surrounding the trailing /user syntax. If any realistic scenarios
> present themselves, perhaps we can try to improve the situation if it's
> not too far out of scope (or just document the behavior, maybe in a unit
> test). Thanks again.
I think it's good enough to install on master. Then more people can
test and report about it.
However, observed some behavior of the new code, here are my findings:
The new searching code seems to prefer "HOST/USER" over "USER <at> HOST".
I created the password store entry "foo.com/bar.org". Then I evaluated:
(warning: manually typed with hands)
(auth-source-search :host "bar.org")
;; => nil
(auth-source-search :host "foo.com")
;; => ((:host "foo.com" :user "bar.org" :secret ...))
I created another entry "bar.org <at> foo.com". But it returns the password
in "foo.com/bar.org".
I deleted "foo.com/bar.org", now it return the password of
"bar.org <at> foo.com".
I created "foo.com/bar.org" again, and "foo.com/bar.org" is preferred
again.
I suggest to prefer the "@" syntax over "/user" syntax.
--
Akib Azmain Turja, GPG key: 70018CE5819F17A3BBA666AFE74F0EFA922AE7F5
Fediverse: akib <at> hostux.social
Codeberg: akib
emailselfdefense.fsf.org | "Nothing can be secure without encryption."
[signature.asc (application/pgp-signature, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Mon, 14 Nov 2022 15:13:01 GMT)
Full text and
rfc822 format available.
Message #94 received at 58985 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Akib Azmain Turja <akib <at> disroot.org> writes:
> "J.P." <jp <at> neverwas.me> writes:
>
>> You mentioned previously some potentially surprising ambiguities
>> surrounding the trailing /user syntax. If any realistic scenarios
>> present themselves, perhaps we can try to improve the situation if it's
>> not too far out of scope (or just document the behavior, maybe in a unit
>> test). Thanks again.
>
> I think it's good enough to install on master. Then more people can
> test and report about it.
>
> However, observed some behavior of the new code, here are my findings:
>
> The new searching code seems to prefer "HOST/USER" over "USER <at> HOST".
That's the effect, right. I think `directory-files-recursively'
basically determines the ordering in which the entries are considered.
> I created the password store entry "foo.com/bar.org". Then I evaluated:
> (warning: manually typed with hands)
>
> (auth-source-search :host "bar.org")
> ;; => nil
>
> (auth-source-search :host "foo.com")
> ;; => ((:host "foo.com" :user "bar.org" :secret ...))
>
> I created another entry "bar.org <at> foo.com". But it returns the password
> in "foo.com/bar.org".
>
> I deleted "foo.com/bar.org", now it return the password of
> "bar.org <at> foo.com".
>
> I created "foo.com/bar.org" again, and "foo.com/bar.org" is preferred
> again.
>
> I suggest to prefer the "@" syntax over "/user" syntax.
I have tried tweaking things in that direction. But as far as
deprecating the /user form officially: that seems more like a group
decision. And then there's the question of how to express such a policy.
Should we emit a warning? At the very least, it would need to be
documented somewhere.
Anyway, this is useful analysis. Thanks again for all your help.
[0000-v6-v7.diff (text/x-patch, attachment)]
[0001-POC-Make-auth-source-pass-behave-more-like-other-bac.patch (text/x-patch, attachment)]
[0002-POC-Support-auth-source-pass-in-ERC.patch (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Mon, 14 Nov 2022 18:28:01 GMT)
Full text and
rfc822 format available.
Message #97 received at 58985 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
"J.P." <jp <at> neverwas.me> writes:
> Akib Azmain Turja <akib <at> disroot.org> writes:
>
>> "J.P." <jp <at> neverwas.me> writes:
>>
>>> You mentioned previously some potentially surprising ambiguities
>>> surrounding the trailing /user syntax. If any realistic scenarios
>>> present themselves, perhaps we can try to improve the situation if it's
>>> not too far out of scope (or just document the behavior, maybe in a unit
>>> test). Thanks again.
>>
>> I think it's good enough to install on master. Then more people can
>> test and report about it.
>>
>> However, observed some behavior of the new code, here are my findings:
>>
>> The new searching code seems to prefer "HOST/USER" over "USER <at> HOST".
>
> That's the effect, right. I think `directory-files-recursively'
> basically determines the ordering in which the entries are considered.
>
>> I created the password store entry "foo.com/bar.org". Then I evaluated:
>> (warning: manually typed with hands)
>>
>> (auth-source-search :host "bar.org")
>> ;; => nil
>>
>> (auth-source-search :host "foo.com")
>> ;; => ((:host "foo.com" :user "bar.org" :secret ...))
>>
>> I created another entry "bar.org <at> foo.com". But it returns the password
>> in "foo.com/bar.org".
>>
>> I deleted "foo.com/bar.org", now it return the password of
>> "bar.org <at> foo.com".
>>
>> I created "foo.com/bar.org" again, and "foo.com/bar.org" is preferred
>> again.
>>
>> I suggest to prefer the "@" syntax over "/user" syntax.
>
> I have tried tweaking things in that direction. But as far as
> deprecating the /user form officially: that seems more like a group
> decision. And then there's the question of how to express such a policy.
> Should we emit a warning? At the very least, it would need to be
> documented somewhere.
No, I didn't say to deprecate that syntax, the syntax makes much sense.
I'm suggesting to return "USER <at> HOST" if both "USER <at> HOST" and "HOST/USER"
are present, because the former makes more sense.
>
> Anyway, this is useful analysis. Thanks again for all your help.
>
>
>
>
When are you going to install this? It's definitely an improvement over
the one in master, and doesn't have any problems to block it.
Installing it will also expose it to more users to the change, so this
will get even more testing.
--
Akib Azmain Turja, GPG key: 70018CE5819F17A3BBA666AFE74F0EFA922AE7F5
Fediverse: akib <at> hostux.social
Codeberg: akib
emailselfdefense.fsf.org | "Nothing can be secure without encryption."
[signature.asc (application/pgp-signature, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Tue, 15 Nov 2022 03:33:02 GMT)
Full text and
rfc822 format available.
Message #100 received at 58985 <at> debbugs.gnu.org (full text, mbox):
Akib Azmain Turja <akib <at> disroot.org> writes:
>>> I suggest to prefer the "@" syntax over "/user" syntax.
>>
>> I have tried tweaking things in that direction. But as far as
>> deprecating the /user form officially: that seems more like a group
>> decision. And then there's the question of how to express such a policy.
>> Should we emit a warning? At the very least, it would need to be
>> documented somewhere.
>
> No, I didn't say to deprecate that syntax, the syntax makes much sense.
Oh, well then pardon my inferring that. But without deprecation, we'd
need to somehow "encode" the @-wins behavior into the interface with
documentation and tests, which is usually more complex than it first
appears. Otherwise, we can just treat @ favoritism as an implementation
detail not subject to preservation come some future rewrite or major
overhaul. As things stand, this patch mostly takes the latter approach
(tests aside).
> I'm suggesting to return "USER <at> HOST" if both "USER <at> HOST" and "HOST/USER"
> are present, because the former makes more sense.
Right, I guess you didn't bother trying out the latest changes attached
to my previous email, which is fine. The thing I'd like to stress here
(mainly for posterity) is that the degree to which we demote/defer
candidates of the / form is deliberate. The way I have things now gives
search order primacy over @-vs-/ contention, meaning a search tree like
h g
/ @ / @
1 2 1 2 1 2 1 2
and params like
:host '("h" "g") :port 2 :max 5
gives
@h:2, h:2/, @g:2, g:2/
whereas full demotion (not implemented) would yield
@h:2, @g:2, h:2/, g:2/
IOW, if you omit the :port 2 part, you currently get
@h:1, @h:2, h:1/, h:2/, @g:1
which is likewise expected.
Basically, the current search strategy adheres more closely to how the
other back ends operate and is thus preferred.
>> Anyway, this is useful analysis. Thanks again for all your help.
>
> When are you going to install this? It's definitely an improvement over
> the one in master, and doesn't have any problems to block it.
> Installing it will also expose it to more users to the change, so this
> will get even more testing.
I am willing to install this but am not really comfortable enabling it
by default unless the maintainers of the downstream packages (Cc. Björn)
can promise to report any problems while Emacs 29.1 is still unreleased.
Without such a pledge, I'm inclined to just leave it disabled. Thanks.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Tue, 15 Nov 2022 03:46:02 GMT)
Full text and
rfc822 format available.
Message #103 received at 58985 <at> debbugs.gnu.org (full text, mbox):
Hi Björn,
"J.P." <jp <at> neverwas.me> writes:
> Björn Bidar <bjorn.bidar <at> thaodan.de> writes:
>
>> "J.P." <jp <at> neverwas.me> writes:
>>
>>> From this I'll infer that the current implementation of auth-source-pass
>>> does that sufficiently. If that's so and the changes I'm proposing
>>> threaten to interfere with that, what's your opinion on the default
>>> value of a knob to toggle the new behavior?
>>
>> Hm it depends if there are any backends that workaround that old behavior.
>> From what I see the only difference really is that you can specify
>> require and max.
>
> There are actually a few subtle areas where the behavior between old and
> new differs and maybe one or two slightly unintuitive gotchas for folks
> unfamiliar with how the other back ends operate. If you're curious,
> there's a series of side-by-side comparisons added by the first patch
> toward the bottom of
>
> test/lisp/auth-source-pass-tests.el
>
> Please let me know if you have any questions.
I should have expressed this more clearly sooner, but I was hoping to
solicit a vote from you as to whether to enable the new, more
"standardized" behavior by default. If you choose to abstain, would you
at least commit to trying it out before 29.1 is fully released and
raising any issues that might arise as a consequence of whatever default
we go with? This would allow us (me, hopefully) to fix or revert the
changes if necessary.
Thanks,
J.P.
Reply sent
to
"J.P." <jp <at> neverwas.me>:
You have taken responsibility.
(Fri, 18 Nov 2022 14:15:03 GMT)
Full text and
rfc822 format available.
Notification sent
to
"J.P." <jp <at> neverwas.me>:
bug acknowledged by developer.
(Fri, 18 Nov 2022 14:15:03 GMT)
Full text and
rfc822 format available.
Message #108 received at 58985-done <at> debbugs.gnu.org (full text, mbox):
"J.P." <jp <at> neverwas.me> writes:
>> When are you going to install this? It's definitely an improvement over
>> the one in master, and doesn't have any problems to block it.
>> Installing it will also expose it to more users to the change, so this
>> will get even more testing.
>
> I am willing to install this but am not really comfortable enabling it
> by default unless the maintainers of the downstream packages (Cc. Björn)
> can promise to report any problems while Emacs 29.1 is still unreleased.
> Without such a pledge, I'm inclined to just leave it disabled. Thanks.
Because I am easily swayed (or maybe just a liar), I've gone ahead and
enabled it by default [1]. I've also informed Nicolas Petton of the
change. I guess Björn was too busy or annoyed by my pestering to keep
up, which is understandable.
Thanks, everyone, for your help with this (especially Akib, who I pray
will consider contributing to ERC in the future). And please remember to
complain if you encounter any related ugliness. In the meantime, I am
closing this bug.
[1] https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=2cf9e699
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Sat, 19 Nov 2022 00:36:02 GMT)
Full text and
rfc822 format available.
Message #111 received at 58985 <at> debbugs.gnu.org (full text, mbox):
Hi Kai,
Kai Tetzlaff <emacs+bug <at> tetzco.de> writes:
> This change breaks my use of `auth-source-pass' in gnus.
Thanks a lot for reporting this. And sorry about the breakage.
> I haven't had time to investigate the issue but what I can already say
> is that the problem occurs independent of the value of
> `auth-source-pass-extra-query-keywords' (`t' or `nil'). So the
> change is not backward compatible. It would (at least) be nice to
> mention this in the NEWS entry.
I'd rather not settle for "at least" if we can help it. If the user
option doesn't preserve existing behavior, that's a bug that needs
fixing.
The traditional and new code paths diverge in `auth-source-pass-search',
so without a backtrace, we should start there. (Obviously, a full
backtrace would be ideal, but I understand completely if you're not
willing to surrender one.) First off, can you try reverting the changes
to that function alone? Just eval'ing a modified version in place,
without the extra `cond' clause and the two keywords, :max and :require,
should do it. If that doesn't tell us anything (and only if you're up
for it) you could trace the function and tell me what the inputs were
(obviously after swapping out any sensitive info). A mini example of
your ~/.password-store layout might also be helpful.
According to etc/AUTHORS, you're likely much better acquainted with
Emacs than I (2009!). So, please adjust the above recommendations
accordingly and, if possible, apply some of that experience to helping
fix this bug. And apologies again for the disruption.
Thanks,
J.P.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Sat, 19 Nov 2022 03:40:01 GMT)
Full text and
rfc822 format available.
Message #114 received at 58985 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Kai Tetzlaff <emacs+bug <at> tetzco.de> writes:
> I've done some further checks and now it seems that setting
> `auth-source-pass-extra-query-keywords' to `nil' in a new emacs session
> does indeed fix the issue (maybe `auth-source' caching of the negative
> lookup caused my initial breakage to persist even after changing
> `auth-source-pass-extra-query-keywords').
Ah, right, the cache (gets me every time). BTW, it's probably still
worth mentioning the incompatibility in NEWS and the docs.
> The lookup which fails with the new code is for the following
> parameters:
>
> auth-source-search: found 0 results (max 1) matching
> (:max 1
> :host ("news6.open-news-network.org" "onn6")
> :port ("119" "nntp" "nntp" "563" "nntps" "snews"))
>
> My password store contains an entry for 'nntp/open-news-network.org'. I
> don't use the full hostname since the open news network has multiple
> servers (news1/2/3/4...) with the same domain name.
>
> Right now I don't have time for a more detailed analysis. But I will
> (hopefully) get back to it during the weekend.
Wow, thanks, this is really helpful. Based on that, I'm pretty sure
what's going on. Basically, the new behavior is geared toward blindly
replicating that of the other back ends, warts and all. But that means
some handy pass-specific features, like subdomain matching, are notably
absent. I've attached a demo patch that better illustrates this.
My main question for you is: do you think we ought to change the default
for `auth-source-pass-extra-query-keywords' to nil? What about
additionally demoting it from an option to a public variable intended
solely for use by dependent libraries instead of end users?
[0001-POC-Allow-subdomain-matching-in-auth-source-pass-fin.patch (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Sat, 19 Nov 2022 04:09:02 GMT)
Full text and
rfc822 format available.
Message #117 received at 58985 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
This is probably a(nother) bad idea, but what about making
`auth-source-pass-extra-query-keywords' a "tristate" option with a
third, hybrid value, like `match-domains', that acts like `t' except
with subdomain matching turned on?
[0000-v1-v2.diff (text/x-patch, attachment)]
[0001-POC-Allow-subdomain-matching-in-auth-source-pass-fin.patch (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Sat, 19 Nov 2022 09:26:04 GMT)
Full text and
rfc822 format available.
Message #120 received at 58985 <at> debbugs.gnu.org (full text, mbox):
This change breaks my use of `auth-source-pass' in gnus.
I haven't had time to investigate the issue but what I can already say
is that the problem occurs independent of the value of
`auth-source-pass-extra-query-keywords' (`t' or `nil'). So the
change is not backward compatible. It would (at least) be nice to
mention this in the NEWS entry.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Sat, 19 Nov 2022 09:26:04 GMT)
Full text and
rfc822 format available.
Message #123 received at 58985 <at> debbugs.gnu.org (full text, mbox):
"J.P." <jp <at> neverwas.me> writes:
Thanks for the quick reply.
>> I haven't had time to investigate the issue but what I can already say
>> is that the problem occurs independent of the value of
>> `auth-source-pass-extra-query-keywords' (`t' or `nil'). So the
>> change is not backward compatible. It would (at least) be nice to
>> mention this in the NEWS entry.
>
> I'd rather not settle for "at least" if we can help it. If the user
> option doesn't preserve existing behavior, that's a bug that needs
> fixing.
I've done some further checks and now it seems that setting
`auth-source-pass-extra-query-keywords' to `nil' in a new emacs session
does indeed fix the issue (maybe `auth-source' caching of the negative
lookup caused my initial breakage to persist even after changing
`auth-source-pass-extra-query-keywords').
The lookup which fails with the new code is for the following
parameters:
auth-source-search: found 0 results (max 1) matching
(:max 1
:host ("news6.open-news-network.org" "onn6")
:port ("119" "nntp" "nntp" "563" "nntps" "snews"))
My password store contains an entry for 'nntp/open-news-network.org'. I
don't use the full hostname since the open news network has multiple
servers (news1/2/3/4...) with the same domain name.
Right now I don't have time for a more detailed analysis. But I will
(hopefully) get back to it during the weekend.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Tue, 22 Nov 2022 15:22:02 GMT)
Full text and
rfc822 format available.
Message #126 received at 58985 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Kai Tetzlaff <emacs+bug <at> tetzco.de> writes:
> "J.P." <jp <at> neverwas.me> writes:
>
> Thanks for the quick reply.
>
>>> I haven't had time to investigate the issue but what I can already say
>>> is that the problem occurs independent of the value of
>>> `auth-source-pass-extra-query-keywords' (`t' or `nil'). So the
>>> change is not backward compatible. It would (at least) be nice to
>>> mention this in the NEWS entry.
>>
>> I'd rather not settle for "at least" if we can help it. If the user
>> option doesn't preserve existing behavior, that's a bug that needs
>> fixing.
>
> I've done some further checks and now it seems that setting
> `auth-source-pass-extra-query-keywords' to `nil' in a new emacs session
> does indeed fix the issue (maybe `auth-source' caching of the negative
> lookup caused my initial breakage to persist even after changing
> `auth-source-pass-extra-query-keywords').
Probably because auth-source was caching the result. Either set
auth-source-do-cache to nil, or do M-x auth-source-forget-all-cached to
clear cache.
>
> The lookup which fails with the new code is for the following
> parameters:
>
> auth-source-search: found 0 results (max 1) matching
> (:max 1
> :host ("news6.open-news-network.org" "onn6")
> :port ("119" "nntp" "nntp" "563" "nntps" "snews"))
>
> My password store contains an entry for 'nntp/open-news-network.org'. I
> don't use the full hostname since the open news network has multiple
> servers (news1/2/3/4...) with the same domain name.
>
> Right now I don't have time for a more detailed analysis. But I will
> (hopefully) get back to it during the weekend.
>
>
>
--
Akib Azmain Turja, GPG key: 70018CE5819F17A3BBA666AFE74F0EFA922AE7F5
Fediverse: akib <at> hostux.social
Codeberg: akib
emailselfdefense.fsf.org | "Nothing can be secure without encryption."
[signature.asc (application/pgp-signature, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Thu, 24 Nov 2022 15:03:02 GMT)
Full text and
rfc822 format available.
Message #129 received at 58985 <at> debbugs.gnu.org (full text, mbox):
Hi João,
João Távora <joaotavora <at> gmail.com> writes:
> Hi Maintainers,
>
> Commit 2cf9e699ef0fc43a4eadaf00a1ed2f876765c64d breaks my Gnus setup.
>
> Author: F. Jason Park <jp <at> neverwas.me>
> Date: Tue Nov 1 22:46:24 2022 -0700
>
> Make auth-source-pass behave more like other backends
>
> I've reached this conclusion through 'git bisect'. I.e. the commit which
> immediately precedes it is not broken.
Sorry about that. I feel not great that you spent precious man hours
bisecting on my account.
The new option `auth-source-pass-extra-query-params' is behind the
breakage you're witnessing. It tries to make auth-source-pass adhere as
closely as possible ("bug for bug") to the auth-source reference
backend, netrc (but only to the extent that the other backends already
do). The idea was to make searches closer to being backend agnostic and
thus more predictable. And auth-source-pass was the lone holdout in
terms of conforming behavior. But, alas, it's looking like the quest for
uniformity has come at the cost of usability for everyday
auth-source-pass users, which is regrettable and surely a deal breaker
for keeping it enabled by default.
> I haven't investigated why, but I do use 'pass' (www.passwordstore.org) to
> (require 'auth-source)
> (auth-source-pass-enable)
> (setq auth-sources '(password-store)) ;; don't use anything else
>
> store my passwords securely.
>
> This is my pass-related setup, which is pretty simple:
>
> After the commit, M-x gnus is unable to connect to my local imap server. There
> is very little debug information.
If we were actually gonna try and debug this, I'd probably ask you for
the names of the affected items in your ~/.password-store and the query
params passed to `auth-source-search' and maybe also whatever's printed
to *Messages* when a query is performed with `auth-source-debug' turned
on.
However, I think it's probably best to forgo all that and do what I was
leaning toward from the outset, and that's keeping the new behavior off
by default in Emacs 29. It's looking liable to cause too much churn for
too many folks [1]. Thus, unless anyone objects or has anything else to
add, I will do this in the next 24 hours or so. Apologies again for the
disruption and the time spent bisecting.
J.P.
[1] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=58985#114
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Thu, 24 Nov 2022 15:37:01 GMT)
Full text and
rfc822 format available.
Message #132 received at 58985 <at> debbugs.gnu.org (full text, mbox):
"J.P." <jp <at> neverwas.me> writes:
> If we were actually gonna try and debug this, I'd probably ask you for
> the names of the affected items in your ~/.password-store and the query
> params passed to `auth-source-search' and maybe also whatever's printed
> to *Messages* when a query is performed with `auth-source-debug' turned
> on.
The affected item is, I believe, ~/.password-store/local-gmail:imap.gpg
and likely also ~/.password-store/smtp.gmail.com:465.gpg. When I set
auth-source-debug to t, these lines appeared in *Messages*
auth-source-pass: final result: nil
auth-source-search: found 0 results (max 1) matching (:max 1 :host ("local-gmail" "localhost") :port ("imap" "imap" "143") :user "joaotavora <at> gmail.com" :require (:user :secret) :create t)
auth-source-pass: final result: nil
auth-source-search: CREATED 0 results (max 1) matching (:max 1 :host ("local-gmail" "localhost") :port ("imap" "imap" "143") :user "joaotavora <at> gmail.com" :require (:user :secret) :create t)
Opening nnimap server on local-gmail...failed:
> However, I think it's probably best to forgo all that and do what I was
> leaning toward from the outset, and that's keeping the new behavior off
> by default in Emacs 29. It's looking liable to cause too much churn for
> too many folks [1]. Thus, unless anyone objects or has anything else to
> add, I will do this in the next 24 hours or so. Apologies again for the
> disruption and the time spent bisecting.
No problem, and thanks for understanding. I think it is indeed better
if you make this opt-in. I can then opt into it and help you debug the
root cause. But in the meantime, my email won't be broken :-)
João
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Fri, 25 Nov 2022 14:25:02 GMT)
Full text and
rfc822 format available.
Message #135 received at 58985 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
João Távora <joaotavora <at> gmail.com> writes:
> The affected item is, I believe, ~/.password-store/local-gmail:imap.gpg
> and likely also ~/.password-store/smtp.gmail.com:465.gpg. When I set
> auth-source-debug to t, these lines appeared in *Messages*
>
> auth-source-pass: final result: nil
> auth-source-search: found 0 results (max 1) matching (:max 1 :host
> ("local-gmail" "localhost") :port ("imap" "imap" "143") :user
> "joaotavora <at> gmail.com" :require (:user :secret) :create t)
> auth-source-pass: final result: nil
> auth-source-search: CREATED 0 results (max 1) matching (:max 1 :host
> ("local-gmail" "localhost") :port ("imap" "imap" "143") :user
> "joaotavora <at> gmail.com" :require (:user :secret) :create t)
> Opening nnimap server on local-gmail...failed:
This was helpful, thanks. It seems
:require (:user ...)
is clashing with the absence of a "user" component in the affected file
names.
Among other things, the commit in question tries to provide a way of
honoring the `:require' keyword in a manner befitting the doc string of
`auth-source-search':
:require (A B C) means that only results that contain those
tokens will be returned. Thus for instance requiring :secret
will ensure that any results will actually have a :secret
property.
The other back ends more or less do the same. (Take a peek at the
attached examples if you're bored.) So, I guess the takeaway here, at
least as things stand, is basically this: if for some reason you really
wanted to enable the option, you'd need to rename the affected files.
Either
~/.password-store/joaotavora <at> gmail.com <at> local-gmail:imap.gpg
or
~/.password-store/local-gmail:imap/joaotavora <at> gmail.com.gpg
should do it. Alternatively, if the gnus function that calls
`auth-source-search' were somehow configurable (guessing no), you could
omit the `:require's altogether, increase the `:max' value, and
prioritize the results, which is what ERC does (or tries to do).
>> However, I think it's probably best to forgo all that and do what I was
>> leaning toward from the outset, and that's keeping the new behavior off
>> by default in Emacs 29. It's looking liable to cause too much churn for
>> too many folks [1]. Thus, unless anyone objects or has anything else to
>> add, I will do this in the next 24 hours or so. Apologies again for the
>> disruption and the time spent bisecting.
>
> No problem, and thanks for understanding.
Thank YOU for understanding. (All I did was break your email.)
> I think it is indeed better if you make this opt-in. I can then opt
> into it and help you debug the root cause. But in the meantime, my
> email won't be broken :-)
I've pushed the change, but you may need to clear your auth-source cache
or restart your session to see any effect. Please let me know if that
doesn't do it. And thanks for all your work on Emacs!
J.P.
[0001-POC-Compare-require-among-auth-source-backends.patch (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#58985; Package
emacs.
(Wed, 07 Dec 2022 14:31:02 GMT)
Full text and
rfc822 format available.
Message #138 received at 58985-done <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
"J.P." <jp <at> neverwas.me> writes:
> Because I am easily swayed (or maybe just a liar), I've gone ahead and
> enabled it by default [1]. I've also informed Nicolas Petton of the
> change. I guess Björn was too busy or annoyed by my pestering to keep
> up, which is understandable.
>
> Thanks, everyone, for your help with this (especially Akib, who I pray
> will consider contributing to ERC in the future). And please remember to
> complain if you encounter any related ugliness. In the meantime, I am
> closing this bug.
A couple updates for anyone who cares:
1. As you may have noticed, due to various complaints here on the
tracker, the new option `auth-source-pass-extra-query-keywords' is
now disabled by default.
2. The changes currently installed contain a bug involving spaces in
file names. Basically, all other back ends allow spaces in an entry's
user and host fields. The second (throwaway) patch below demonstrates
this, and the first attempts to make things right.
In my mind, item #2 is a bug that needs fixing on the release branch,
and I plan on doing so in the coming days. If there are questions or
concerns, please let them be known. Thanks.
[0001-Allow-spaces-in-auth-source-pass-match-regexp.patch (text/x-patch, attachment)]
[0002-POC-Demo-spaces-in-hosts-users-among-auth-source-bac.patch (text/x-patch, attachment)]
bug archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org.
(Thu, 05 Jan 2023 12:24:08 GMT)
Full text and
rfc822 format available.
This bug report was last modified 2 years and 222 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.