coretasks: implement scram-sha-256 #2362
Conversation
half-duplex
force-pushed
the
sasl-scram
branch
from
8d71a45 to
d911a7a
Compare
|
Linked to: |
Avoiding/trying to eliminate dependencies in the current era of Sopel (can't speak for past maintainers) is more about future-proofing than install-size concerns. I worry whether Package X will still be releasing new versions in five years, and tend to prefer projects that are maintained by multiple users and live under a GitHub organization rather than someone's personal account. Sole-maintainer stuff has a nasty habit of going stale without warning when the author loses interest. |
|
First release 3.5 years ago, last release ...yesterday. Do you see a better lib to use? Limnoria uses https://github.com/ProgVal/pyxmpp2-scram/, which ProgVal tore out of pyxmpp2. |
|
Wasn't saying anything about this specific library, haha, just dependencies in general. Sure, I've seen ProgVal around a lot, and would probably have picked that one had I written this patch, but it's already written with the other. The important part is that there's another library we could probably switch to if needed. |
half-duplex
force-pushed
the
sasl-scram
branch
from
d911a7a to
1d5b278
Compare
half-duplex
force-pushed
the
sasl-scram
branch
2 times, most recently
from
7aee922 to
9d978b4
Compare
|
There are two uncaught tracebacks from scramp when the server returns either garbage or an invalid signature instead of the last response. In the former case, it also does not send Respectively: and |
|
Re-raising the exception (example 2) was intentional, but I suppose is unnecessary. Good catch on the scramp KeyError. |
|
This should be put on hold up until #2341 is ready and merge, because it heavily impact the code that manage SASL authentication, add lots of new tests, etc. and fixes related bugs. |
|
Hi all, maintainer of Scramp here 👋 With the latest release of Scramp 1.4.4 we now check that every message is well formed and raise a A |
|
yes |
|
Actually I just got involved because I maintain the scramp library. If any problems come up with it, just let me know. |
|
@tlocke Thanks. I saw a traceback example and assumed you were advising a fix for something still in the patch, or modifying the version range. @Exirel It might not be feasible to wait for #2341 as that's still in draft state, vs. this being practically ready to merge. Unless you have time to finish that off before mal finds the time to finalize this one. It's a race! 🏁 |
half-duplex
force-pushed
the
sasl-scram
branch
from
8796ff7 to
c754fc7
Compare
There was a problem hiding this comment.
The implementation of the SCRAM workflow looks good. However, when it comes to the appropriate checks and validation, I think more need to be done.
About the automated tests, I'm glad there are some, however the SASL related tests need to go into test/coretasks/test_coretasks_sasl.py. Maybe a part of it is a leftover from the rebase, which I assume was quite difficult from what I can remember of my own modifications.
While you are updating the doc, I think the example is wrong, as it uses server_auth_target instead of server_auth_sasl_mech, that need to be checked.
| # TODO: Implement SCRAM challenges | ||
| elif mech == "SCRAM-SHA-256": | ||
| if trigger.args[0] == "+": | ||
| bot._scram_client = ScramClient([mech], sasl_username, sasl_password) |
There was a problem hiding this comment.
I'd rather see this as a key in memory instead of an undeclared attribute on the bot.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
I still want a "has the user configured a sasl method we don't support" check but that's been annoying to add
half-duplex
force-pushed
the
sasl-scram
branch
2 times, most recently
from
b894dc0 to
ca33e00
Compare
half-duplex
force-pushed
the
sasl-scram
branch
from
ca33e00 to
69812a6
Compare
| 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), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| # TODO: Implement SCRAM challenges | ||
| elif mech == "SCRAM-SHA-256": | ||
| if trigger.args[0] == "+": | ||
| bot._scram_client = ScramClient([mech], sasl_username, sasl_password) |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| sopel_mechs = ["PLAIN", "EXTERNAL", "SCRAM-SHA-256"] | ||
| if mech not in sopel_mechs: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think that the order needs to be: "SCRAM-SHA-256", "EXTERNAL", "PLAIN"
| 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), |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.
| @@ -344,3 +353,201 @@ def test_sasl_nak(botfactory: BotFactory, tmpconfig) -> None: | |||
| 'CAP END', | |||
| 'QUIT :Error negotiating capabilities.', | |||
| ) | |||
|
|
|||
|
|
|||
| def test_sasl_bad_method(mockbot: Sopel, caplog: pytest.LogCaptureFixture): | |||
There was a problem hiding this comment.
I really don't like log capture in test. It seems something is fishy: in test, we should be able to check for exception raised instead of relying on stdout/stderr inspection.
There was a problem hiding this comment.
The exception raised is caught by Sopel and turned into a log message - I'm not sure how else I would check for it.
There was a problem hiding this comment.
This shouldn't fail silently. If there is an authentication method set, if it fails to authenticate, the bot should stop (i.e. send QUIT with the "configuration error" reason).
| assert mockbot.backend.message_sent == rawlist( | ||
| "CAP REQ :sasl", | ||
| "AUTHENTICATE PLAIN", | ||
| ) | ||
| mockbot.on_message("AUTHENTICATE +") | ||
| assert ( | ||
| len(mockbot.backend.message_sent) == 3 | ||
| and mockbot.backend.message_sent[-1] | ||
| == rawlist("AUTHENTICATE VGVzdEJvdABUZXN0Qm90AHNlY3JldA==")[0] | ||
| ) |
There was a problem hiding this comment.
Please use a different assert for such different check. Also, you can use the same technique I used previously in the SASL tests, with a n value that allows you to check not just the latest message, but all the message from that point.
| assert mockbot.backend.message_sent == rawlist( | |
| "CAP REQ :sasl", | |
| "AUTHENTICATE PLAIN", | |
| ) | |
| mockbot.on_message("AUTHENTICATE +") | |
| assert ( | |
| len(mockbot.backend.message_sent) == 3 | |
| and mockbot.backend.message_sent[-1] | |
| == rawlist("AUTHENTICATE VGVzdEJvdABUZXN0Qm90AHNlY3JldA==")[0] | |
| ) | |
| assert mockbot.backend.message_sent == rawlist( | |
| "CAP REQ :sasl", | |
| "AUTHENTICATE PLAIN", | |
| ) | |
| n = 2 | |
| mockbot.on_message("AUTHENTICATE +") | |
| assert mockbot.backend.message_sent[n:] == rawlist("AUTHENTICATE VGVzdEJvdABUZXN0Qm90AHNlY3JldA==") |
And you can adapt the rest of the test the same way.
I really dislike the [-1] approach, because it forces you to check first that you have only a predefined number of line that you have to check first, whereas with the n approach, you can directly check the difference in expected output and the actual output.
| scram_server.set_client_first( | ||
| b64decode(mockbot.backend.message_sent[-1].split(b" ")[-1]).decode("utf-8") | ||
| ) |
There was a problem hiding this comment.
The [-1] here is an assumption that I don't like. I'd rather see that there is, indeed, only the expected number of messages. And if possible, to check the content of said messages (it might not be possible, I don't know if SCRAM has a random part that we can't control in any way here).
| assert ( | ||
| len(mockbot.backend.message_sent) == 6 | ||
| and mockbot.backend.message_sent[-1] == rawlist("CAP END")[0] | ||
| ) |
There was a problem hiding this comment.
Ditto here about having only one assert here to check multiple conditions. Each needs to be on its own. And I'd even say that it's better to check the number of message sent between every on_message call, to see when messages are supposed to be sent or not.
| assert ( | ||
| len(mockbot.backend.message_sent) == 6 | ||
| and mockbot.backend.message_sent[-1] == rawlist("CAP END")[0] | ||
| ) |
There was a problem hiding this comment.
Ditto here about having only one assert here to check multiple condition. I really don't think it's good.
There was a problem hiding this comment.
Same as before: the SASL mechanism implementation looks sensible. However on the test side, I've several things I would like to see modified. A couple of nitpicks on wording (I really don't like using the word "nonsense" as it isn't precise).
Also, I noted a few changes that I don't think should be in here for now: the should be discussed in their own issue/separate PR.
half-duplex
force-pushed
the
sasl-scram
branch
from
9602028 to
3aa7f37
Compare
Description
This implements the SASL SCRAM-SHA-256 authentication mechanism.
I've tested it using irc.alphachat.net:6697 - PM for creds or BYO.
It adds a dependency on
scramp(~29kb), which could be made optional if we're picky.It adds tests for both PLAIN and SCRAM-SHA-256, which could probably be done better. Neither tests nor implementation try to catch every edge case - any big ones we care a lot about?
cc @dgw for ircv3 feature tracking, see also #971
Checklist
make qa(runsmake qualityandmake test)