[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#47788: Add support for TLS client certificates to 'erc-tls'
From: |
Amin Bandali |
Subject: |
bug#47788: Add support for TLS client certificates to 'erc-tls' |
Date: |
Sun, 18 Apr 2021 15:23:18 -0400 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux) |
J.P. writes:
[...]
>
> Hey 'bandali' :-)
Hey :-)
> Below is what I sent you previously. I look forward to trying out the
> latest changes!
Thanks again for your feedback! I'll address each point inline, and
attach an updated version at the end of the message.
> From: "J.P." <jp@neverwas.me>
> Subject: ERC TLS client certificate
> Date: Sat, 10 Apr 2021 10:41:56 -0400 (5 days, 9 hours, 28 minutes ago)
> To: bandali@gnu.org
>
> As always, these are just the impressions of an average dummy, so feel
> free to dismiss them.
Please be kinder to yourself :-). All feedback and impressions are
welcome and very much appreciated.
[...]
>
> 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.
Indeed.
> 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.
Oh great, thanks for testing that scenario specifically!
> 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.
Right, I gave it some thought and I thought this would be the most
straightforward and "idiomatic" (w.r.t. the rest of the ERC codebase)
way of doing it. That said, I'd welcome ideas for simplifying and/or
improving the implementation as part of this patch, or the status quo in
general in separate patches.
> 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.
Right, yeah I made sure to not change the existing parameters of any of
the functions and/or their order and only add new parameters at the end,
so as to not break any existing configurations. But indeed, having a
test suite would've been nice to help give more assurance about that.
> 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?).
Indeed, very much so! Thanks for noticing that; amended in v2.
> 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).
Certainly; done in v2.
> 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).
Indeed :-). My intention was to enforce non-nil there, to always make
the connection in an asynchronous/non-blocking way, similar to its dual,
`erc-open-network-stream'. I think passing :nowait nil could be useful
for debugging, but off top of my head I can't think of any other reason
one would want to do it. And for debugging, one could easily redefine
the function completely. On the other hand, I don't see any harm in
allowing/respecting :nowait nil if it's explicitly set. So I've changed
the above plist-get to plist-member in v2.
> 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.
Great! It certainly scratches an itch for me personally and greatly
simplifies the use-case of authenticating with client certificates.
Hopefully others will find this useful as well. :-)
* * *
Robert Pluim writes:
[...]
> It would be nice if you could arrange for an option to pass
> ':client-certificate t' to 'open-network-stream' so it would leverage
> 'auth-source'
Thanks for the suggestion. After some thought, I decided to change the
interface of erc-tls from the two separate :client-key and :client-crt
arguments in v1 of the patch to just one :client-certificate in v2,
matching that of `open-network-stream'. I tested the change with
`:client-certificate t' and confirm that it works fine for me.
I've attached v2 below, and if there are no other comments or feedback
I'll commit it to emacs.git in a few days. Main changes from v1 include
using just one :client-certificate argument like `open-network-stream',
adding a NEWS entry, and documenting erc-tls in the ERC manual.
* * *
0001-Add-support-for-using-a-TLS-client-certificate-with-.patch
Description: Text Data
bug#47788: Add support for TLS client certificates to 'erc-tls', Robert Pluim, 2021/04/16