emacs-erc
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Add support for TLS client certificates to 'erc-tls'


From: Amin Bandali
Subject: Re: Add support for TLS client certificates to 'erc-tls'
Date: Thu, 22 Apr 2021 20:45:29 -0400
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux)

J.P. writes:

> Amin Bandali <bandali@gnu.org> writes:
>
>>> It reconnected successfully with no hiccups, so I think that's one
>>> for the win column.
>
> This continues to be the case with the latest changes applied; ditto
> when using the authinfo variant of the new :client-certificate param.

Great!

>>> 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.
>
> Nice job. This sort of thing can be a real time sink (at least for me).

Thanks, and agreed; but it's good to take the time and do it anyway. :-)

> One question. And stop me if I've confused myself here, but the doc
> string and the tex-info section for `erc-tls' both offer the same
> example (borrowed from their plain `erc' counterparts) in which the
> function `erc-compute-full-name' is said to be consulted despite the
> :full-name "Harry S. Truman" keyword arg being present. While it's
> (technically) true that `erc-compute-full-name' always runs, I suspect
> its inclusion in the original example was inadvertent (or something
> about it has changed since 2007). Would it perhaps make sense to omit
> `erc-compute-full-name' from the Truman example?

Yeah that's fair!  I've tried to clarify/remedy that in the latest
revision.

[...]
>
> Thanks, but I shouldn't have proposed that tweak without a concrete use
> case in mind. In fact, I've only ever needed :nowait to be nil when
> using a custom opener. If my suggestion made things more confusing or
> distracting, my apologies. Perhaps it's best to just revert to what you
> had originally or even just force it to t. (Not that you should waste
> another second on this relative nothingburger.)

No worries :-).  And per our short discussion on #erc the other day,
there could be a legitimate use-case for that.  So I think it's safe to
say that all in all it was a net positive change.

>> 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.
>
> The interface change is a welcome improvement, IMO. Although dealing
> with a list may feel slightly more cumbersome to some, the multiple
> helpful examples make that a nonissue. (And sparing contributors an
> extra param to worry about is a nice bonus as well.) But I guess the
> real win is in accommodating the authinfo facility, which seems a rather
> obvious and essential inclusion in retrospect. Great work!
>

Awesome.  Yeah I couldn't think of a more straightforward and/or less
cumbersome way of doing it, except for using the same shape of argument
as expected by `open-network-stream'.  Per usual, I would be open to
reconsideration if a better way of doing it is suggested.

For now, I went ahead and finally installed the attached patch on the
master branch.  Thanks for all the feedback/suggestions. :-)

Attachment: 0001-Add-support-for-using-a-TLS-client-certificate-with-.patch
Description: Text Data


reply via email to

[Prev in Thread] Current Thread [Next in Thread]