[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Add support for TLS client certificates to 'erc-tls'
From: |
J.P. |
Subject: |
Re: Add support for TLS client certificates to 'erc-tls' |
Date: |
Thu, 15 Apr 2021 05:44:03 -0700 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux) |
Amin Bandali <bandali@gnu.org> writes:
> Fellow ERC user 'neverwas' was kind enough to take my patch for a test
> drive and provide feedback. I have done some work on addressing their
> feedback, but for now I'm attaching my very first draft here which I'd
> sent to neverwas, and will attach the next version of my patch (along
> with a proper commit message, NEWS entry, etc) shortly after neverwas's
> reply with their feedback.
Hey 'bandali' :-)
Below is what I sent you previously. I look forward to trying out the
latest changes!
-------------------- Start of forwarded message --------------------
From: "J.P." <jp@neverwas.me>
To: bandali@gnu.org
Subject: ERC TLS client certificate
Date: Sat, 10 Apr 2021 07:41:56 -0700
As always, these are just the impressions of an average dummy, so feel
free to dismiss them.
Since my setup is probably the most common (GNU/Linux, x86_64, 28.0.50),
you likely already have it pretty well covered. A few environmental
details I'll mention anyway just in case they add anything:
1. patch applied atop
commit 0db2126d7176b0bd1b13d4b0d1cd958c8cf55714
Author: Dmitry Gutov <dgutov@yandex.ru>
Date: Sat Apr 10 01:51:39 2021 +0300
with debugging symbols present and no optimization
2. Emacs invoked with -Q -nw and piloted manually thereafter
3. single file used for both key and cert (combined PEM format)
4. isolated network namespace
sh:~$ ss -tpn
Local Address:Port Peer Address:Port Process
127.0.0.1:54982 127.0.0.1:6670 users:(("emacs",pid=151118,fd=8))
127.0.0.1:34874 127.0.0.1:6697 users:(("socat",pid=160299,fd=5))
127.0.0.1:6670 127.0.0.1:54982 users:(("socat",pid=160299,fd=6))
[::1]:42176 [::1]:6667
[::1]:42178 [::1]:6667
[::1]:6667 [::1]:42178 users:(("oragono",pid=147008,fd=9))
ff:127.0.0.1]:6697 ff:127.0.0.1]:34874 users:(("oragono",pid=147008,fd=11))
[::1]:6667 [::1]:42176 users:(("oragono",pid=147008,fd=10))
Oragono is the server accepting TLS on 6697 and talking to the two
non-Emacs clients (bots) on 6667 (non-TLS). The socat proc is
listening on 6670 and forwarding to 6697; its purpose is explained
below.
Notes
~~~~~
The buffer-local vars you've introduced follow existing conventions in
that they're basically there for recording the options during
entry-point invocation to aid later in reconnecting. For example,
erc-session-connector remembers the initial stream opener, etc.
I've tested this persistence aspect by pulling the plug on an active
session with healthy traffic (hence the socat proxy). It reconnected
successfully with no hiccups, so I think that's one for the win column.
You've probably gamed out the trade-offs more than I have, but seems to
me that mulling over all the ways of specifying TLS connection params
short of explicitly passing them around as you do is kind of moot. I
couldn't think of any that (1) don't buck existing library trends and
(2) make things any easier to maintain/debug. So might as well stick to
the status quo, I guess.
In terms of preserving extensibility and leaving existing escape hatches
intact, it's hard to be sure without test cases or protracted trials,
but if I had to guess, it looks like you're pretty safe in that
department as well. For example, I can't see how folks with their own
erc-server-connect-function would experience any churn from these
changes.
I see you've updated the doc string for the plain `erc' entry point to
include the two new params. Maybe it's also worth mentioning that
specifying them won't magically enable TLS. Users will still need to use
`erc-tls' (right?).
Beyond that, users may appreciate a mention of the new additions in the
info manual and maybe the wiki as well (instead of just NEWS).
Lastly, in the function erc-open-tls-stream, do you maybe want
plist-member instead of plist-get?
(let ((p (plist-put parameters :type 'tls))
args)
(unless (plist-get p :nowait)
;; ^~~~~~~~~~~
(setq p (plist-put p :nowait t)))
(setq args `(,name ,buffer ,host ,port ,@p))
As is, I think it basically enforces non-nil (unless that's the idea).
In general, I think these changes lower the barrier to entry by getting
folks connected faster OOTB, which can only help with adoption and
mind/market share.
J.P.
-------------------- End of forwarded message --------------------