Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

coretasks, irc: implement CertFP / SASL EXTERNAL authentication #2100

Merged
merged 1 commit into from
Jul 1, 2021

Conversation

dgw
Copy link
Member

@dgw dgw commented Jun 9, 2021

Description

Tin. Followed Libera's instructions for generating a certificate and added its fingerprint to our official Sopel bot's account in NickServ, then stuck around in my human-operated client to observe. Flawless victory:

[00:53:18] -NickServ- [email protected] has just authenticated as you (Sopel)
                                       ^ my home IP ^

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make quality and make test)
  • I have tested the functionality of the things this change touches

Note to self

@dgw pls update IRCv3 support info for Sopel when this is merged

Future plans

This lays some groundwork for expanding into SCRAM challenges, as implied by a new TODO comment, but implementing those will take some more research. I had some pseudocode for it in there, but obviously removed it before committing (otherwise linting would probably fail).

As far as actually getting SCRAM into Sopel goes, the options are:

  • choose a library — possibly scramp or tinysasl (no kidding; the whole library is ~110 lines of code), but I've barely searched for alternatives
  • implement SCRAM ourselves (oy, IRC specs are hard enough amirite)
  • steal someone else's implementation (e.g. BitBot has its own, but under GPLv2)

Using a library could be our first excuse to give Sopel setuptools extras. (pip install sopel[scram], anyone?)

Meanwhile, CertFP is what was explicitly requested on May 29 (May 30 UTC) in our IRC channel, and it turns out that was fairly easy to get done, so it might as well get reviewed and merged without waiting for SCRAM nonsense.

@dgw dgw added the Feature label Jun 9, 2021
@dgw dgw added this to the 8.0.0 milestone Jun 9, 2021
@dgw dgw requested a review from a team June 9, 2021 06:13
@dgw dgw force-pushed the certfp-support branch from 9d99b30 to b724c37 Compare June 9, 2021 06:15
Copy link
Member

@half-duplex half-duplex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with nits.
Is there a sane way in stock sopel to have a conversation through the bot? Notably, to command the bot to PRIVMSG NickServ :CERT ADD for enrollment without needing to configure a regular client or ZNC as the bot to do that. I've always just loaded a hackjob .exec plugin.

sopel/config/core_section.py Outdated Show resolved Hide resolved
sopel/config/core_section.py Outdated Show resolved Hide resolved
sopel/coretasks.py Outdated Show resolved Hide resolved
sopel/irc/__init__.py Outdated Show resolved Hide resolved
@half-duplex
Copy link
Member

References:
IRCv3.2 SASL spec
SASL EXTERNAL RFC

@dgw
Copy link
Member Author

dgw commented Jun 10, 2021

@half-duplex re: sending commands as Sopel, the .say and .me commands (in admin) could use a sibling that handles raw commands. Maybe .sendraw or just .raw?

@half-duplex
Copy link
Member

Heh, this isn't the first time this has come up, I guess it is needed.

2019-03-20 06:41:51      ma1    idk if there's a .quote or .raw or something
2019-03-20 06:43:31     +dgw    hmm, feels like there should be

@half-duplex
Copy link
Member

Actually, thinking about it... .say NickServ CERT ADD is entirely sufficient, no need for a raw message. The half of the thought that isn't currently handled gracefully is that there's no way to have the bot send NickServ's responses back to you, either over IRC or in the terminal... 🤔

Copy link
Member

@half-duplex half-duplex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstrings for auth_target and server_auth_sasl_mech should also have EXTERNAL added as a valid option. Maybe an example in configuration.rst too?

sopel/irc/backends.py Outdated Show resolved Hide resolved
@dgw
Copy link
Member Author

dgw commented Jun 10, 2021

@half-duplex Both of those comments should be addressed by the newest push.

@dgw dgw requested review from half-duplex and Exirel June 10, 2021 21:47
@half-duplex
Copy link
Member

Well, here's a fun one: sasl is considered server-based, so that example won't work.
Does SASL make sense as server-based? Why do we distinguish between "server-based" and "nick-based"? Could we just allow an arbitrary set of authentications?
If you want to leave that for another time, I guess I'd just use a single-auth example.

@dgw
Copy link
Member Author

dgw commented Jun 11, 2021

Hmm, I should probably just leave only the single-stage examples for CertFP.

Why do we distinguish between "server-based" and "nick-based"? Could we just allow an arbitrary set of authentications?

This is something Exi and I talked about fairly recently. At some point, it would make a ton of sense to rewrite Sopel's auth system to be a bit more flexible. The context was more about adding pluggability, but I imagine that simplification of the settings would come along with it. No one's going to promise that for any particular timeframe, though, so I think it's best to ship CertFP as-is once we iron out the doc details.

True, this doesn't fix the confusing auth crap, but it also doesn't make things appreciably worse. client_cert_file does its thing regardless of the SASL config, unless the network chooses to require SASL EXTERNAL before authenticating the user.

@dgw dgw force-pushed the certfp-support branch from ac87f8a to 2bcd1ed Compare June 11, 2021 00:32
@dgw dgw merged commit 9a7f9b9 into master Jul 1, 2021
@dgw dgw deleted the certfp-support branch July 1, 2021 05:18
@dgw dgw removed the request for review from Exirel July 1, 2021 05:19
dgw added a commit to dgw/ircv3.github.io that referenced this pull request Jul 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants