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.

Sign up for GitHub

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

Jump to bottom

coretasks: implement scram-sha-256 #2362

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

coretasks: implement scram-sha-256 #2362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bf4fc8f
coretasks: implement scram-sha-256
half-duplex Oct 23, 2022
c9d6958
more tests, handle KeyError/b64
half-duplex Oct 26, 2022
d9c0f35
more tests, test tweaks
half-duplex Oct 26, 2022
746e614
squashme: fix tests
half-duplex Aug 13, 2023
69812a6
squashme: move tests, address comments
half-duplex Nov 12, 2023
3aa7f37
squashme comments [skip ci]
half-duplex Nov 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ lint-style:
flake8

lint-type:
mypy --check-untyped-defs sopel
mypy sopel

.PHONY: test test_norecord test_novcr vcr_rerecord
test:
Expand Down
2 changes: 1 addition & 1 deletion docs/source/configuration.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,7 +576,7 @@ When :attr:`~CoreSection.server_auth_method` is defined the settings used are:
* :attr:`~CoreSection.server_auth_username`: account's username
* :attr:`~CoreSection.server_auth_password`: account's password
* :attr:`~CoreSection.server_auth_sasl_mech`: the SASL mechanism to use
(defaults to ``PLAIN``; ``EXTERNAL`` is also available)
(default is ``PLAIN``; ``EXTERNAL`` and ``SCRAM-SHA-256`` are also available)

For example, this will use NickServ ``IDENTIFY`` command and SASL mechanism::

Expand Down
11 changes: 11 additions & 0 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ dependencies = [
"importlib_metadata>=3.6",
"packaging>=23.2",
"sopel-help>=0.4.0",
"scramp>=1.4.4,<2",
]

[project.urls]
Expand All @@ -72,6 +73,16 @@ sopel-plugins = "sopel.cli.plugins:main"
[project.entry-points.pytest11]
pytest-sopel = "sopel.tests.pytest_plugin"

[tool.mypy]
check_untyped_defs = true
plugins = "sqlalchemy.ext.mypy.plugin"
show_error_codes = true

# Remove once scramp has type stubs or annotations
[[tool.mypy.overrides]]
module = 'scramp.*'
ignore_missing_imports = true
half-duplex marked this conversation as resolved.
Show resolved Hide resolved

[tool.pytest.ini_options]
# NOTE: sopel/ is included here to include dynamically-generated tests
testpaths = ["test", "sopel"]
Expand Down
4 changes: 0 additions & 4 deletions setup.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,3 @@ exclude =
contrib/*,
conftest.py
no-accept-encodings = True

[mypy]
plugins = sqlalchemy.ext.mypy.plugin
show_error_codes = True
10 changes: 7 additions & 3 deletions sopel/config/core_section.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1191,12 +1191,16 @@ def homedir(self):

:default: ``PLAIN``

``EXTERNAL`` is also supported, e.g. for using :attr:`client_cert_file` to
authenticate via CertFP.
Supported mechanisms are:

* ``PLAIN``, to authenticate by sending a plaintext password
* ``EXTERNAL``, to authenticate using a TLS client certificate
(see :attr:`client_cert_file`)
* ``SCRAM-SHA-256``, for password-based challenge-response authentication

.. versionadded:: 7.0
.. versionchanged:: 8.0
Added support for SASL EXTERNAL mechanism.
Added support for SASL EXTERNAL and SCRAM-SHA-256 mechanisms.
"""

server_auth_username = ValidatedAttribute('server_auth_username')
Expand Down
60 changes: 55 additions & 5 deletions sopel/coretasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
from __future__ import annotations

import base64
from binascii import Error as BinasciiError
import collections
import copy
from datetime import datetime, timedelta, timezone
Expand All @@ -32,6 +33,9 @@
import time
from typing import Callable, Optional, TYPE_CHECKING

from scramp import ScramClient, ScramException
from scramp.core import ClientStage as ScramClientStage

from sopel import config, plugin
from sopel.irc import isupport, utils
from sopel.tools import events, jobs, SopelMemory, target
Expand Down Expand Up @@ -106,7 +110,6 @@ def _handle_sasl_capability(
'cannot authenticate with SASL.',
)
return plugin.CapabilityNegotiation.ERROR

# Check SASL configuration (password is required)
password, mech = _get_sasl_pass_and_mech(bot)
if not password:
Expand All @@ -118,9 +121,18 @@ def _handle_sasl_capability(
cap_info = bot.capabilities.get_capability_info('sasl')
cap_params = cap_info.params

available_mechs = cap_params.split(',') if cap_params else []
server_mechs = cap_params.split(',') if cap_params else []

if available_mechs and mech not in available_mechs:
sopel_mechs = ["PLAIN", "EXTERNAL", "SCRAM-SHA-256"]
if mech not in sopel_mechs:
Comment on lines +126 to +127
Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree with that change: technically, a plugin can interact with the SASL authentication outside coretasks, as long as it operates within the confine of the capability request framework (i.e. properly setting CAP negotiation DONE).

At least, I disagree with this change being in this PR.

Copy link

Choose a reason for hiding this comment

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

I think that the order needs to be: "SCRAM-SHA-256", "EXTERNAL", "PLAIN"

Copy link
Member Author

Choose a reason for hiding this comment

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

Why, @Neustradamus?

raise config.ConfigurationError(
'SASL mechanism "{mech}" is not supported by Sopel; '
'available mechanisms are: {available}.'.format(
mech=mech,
available=', '.join(sopel_mechs),
)
)
if server_mechs and mech not in server_mechs:
# Raise an error if configured to use an unsupported SASL mechanism,
# but only if the server actually advertised supported mechanisms,
# i.e. this network supports SASL 3.2
Expand All @@ -129,11 +141,13 @@ def _handle_sasl_capability(
# by the sasl_mechs() function

# See https://github.com/sopel-irc/sopel/issues/1780 for background

common_mechs = set(sopel_mechs) & set(server_mechs)
raise config.ConfigurationError(
'SASL mechanism "{mech}" is not advertised by this server; '
'available mechanisms are: {available}.'.format(
mech=mech,
available=', '.join(available_mechs),
available=', '.join(common_mechs),
Comment on lines +145 to +150
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I like the set intersection here, but it feels like it warrants changing the wording of the message to indicate that we are listing only remote mechanisms that we support. I would propose something like mechanisms in common with server are: {available}

I'm also not sure how I feel about the (admittedly narrow) edge case where there are no mechanisms in common, the error message will just contain an empty set. Might warrant its own check to issue an error that more plainly states that there are no mechanisms in common.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather see the 3 different values: the mech from config, the available mech from the server, and the supported mech from Sopel, all displayed in the same config error for clarity.

)
)

Expand Down Expand Up @@ -1250,7 +1264,43 @@ def auth_proceed(bot, trigger):
bot.write(('AUTHENTICATE', '*'))
return

# TODO: Implement SCRAM challenges
elif mech == "SCRAM-SHA-256":
if trigger.args[0] == "+":
bot._scram_client = ScramClient([mech], sasl_username, sasl_password)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather see this as a key in memory instead of an undeclared attribute on the bot.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we have a line on what belongs in bot.memory vs an attribute? To me, bot.memory is meant for users to touch, so the scram client belongs as a _attribute. On the other hand, I'd rather leave the scramp dependency in coretasks.py only...

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there's any clearly-defined line, but the core does use bot.memory for some stateful things ('join_events_queue' in particular) so I tend to agree that this would be better stored in memory.

I could see a case for having an attribute on the bot that contains a generalized authentication client of some sort, but I can see and agree with the objection to the definition given here, especially since the attribute isn't guaranteed to exist.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, at some point, maybe have a fully extendable authentication system within Sopel so plugin can actually hook into it properly... but at this time, this isn't it.

The bot offer a memory object exactly for this type of purpose: plugin specific piece of in-memory data. In this case, this is even a throwaway piece of data, because once the scram challenge is complete, you could delete it from memory.

client_first = bot._scram_client.get_client_first()
LOGGER.info("Sending SASL SCRAM client first")
send_authenticate(bot, client_first)
elif bot._scram_client.stage == ScramClientStage.get_client_first:
try:
server_first = base64.b64decode(trigger.args[0]).decode("utf-8")
bot._scram_client.set_server_first(server_first)
except (BinasciiError, KeyError, ScramException) as e:
LOGGER.error("SASL SCRAM server_first failed: %r", e)
bot.write(("AUTHENTICATE", "*"))
return
if bot._scram_client.iterations < 4096:
LOGGER.warning(
"SASL SCRAM iteration count is insecure, continuing anyway"
)
elif bot._scram_client.iterations >= 4_000_000:
LOGGER.warning(
"SASL SCRAM iteration count is very high, this will be slow..."
)
client_final = bot._scram_client.get_client_final()
LOGGER.info("Sending SASL SCRAM client final")
send_authenticate(bot, client_final)
elif bot._scram_client.stage == ScramClientStage.get_client_final:
try:
server_final = base64.b64decode(trigger.args[0]).decode("utf-8")
bot._scram_client.set_server_final(server_final)
except (BinasciiError, KeyError, ScramException) as e:
LOGGER.error("SASL SCRAM server_final failed: %r", e)
bot.write(("AUTHENTICATE", "*"))
return
LOGGER.info("SASL SCRAM succeeded")
bot.write(("AUTHENTICATE", "+"))
bot._scram_client = None
Comment on lines +1273 to +1302
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at this, I find myself wondering how much of these 30 lines could be pushed into ScramClient, so that the elif clauses could be folded away behind something roughly like else: bot._scram_client.proceed()

I'm not sure if it's really worth pushing across the class boundary, but maybe it would still make sense to define a separate method on the bot.

Copy link
Contributor

Choose a reason for hiding this comment

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

Like @SnoopJ I think this is a bit too big for the if/elif. However, I'd rather see a function (such as _handle_sasl_scram(...) of the coretasks plugin. I don't see why this should be a bot's method.

return


def _make_sasl_plain_token(account, password):
Expand Down
Loading