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: implement scram-sha-256 #2362

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

Conversation

half-duplex
Copy link
Member

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

  • 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

@Neustradamus
Copy link

Linked to:

@dgw
Copy link
Member

dgw commented Oct 23, 2022

It adds a dependency on scramp (~29kb), which could be made optional if we're picky.

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.

@half-duplex
Copy link
Member Author

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.
If it needed to be sucked in later, it's not spectacularly complex, just nontrivial enough I don't want to eagerly NIH it.

@dgw
Copy link
Member

dgw commented Oct 23, 2022

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.

Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

I'm doing the nitpick-docs thing again, huh?

sopel/config/core_section.py Outdated Show resolved Hide resolved
sopel/coretasks.py Outdated Show resolved Hide resolved
sopel/config/core_section.py Outdated Show resolved Hide resolved
@half-duplex half-duplex force-pushed the sasl-scram branch 2 times, most recently from 7aee922 to 9d978b4 Compare October 26, 2022 05:08
dgw
dgw previously approved these changes Oct 26, 2022
Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

I'm happy, but let's give @Exirel a chance to weigh in.

@dgw dgw requested a review from Exirel October 26, 2022 05:13
@progval
Copy link

progval commented Oct 26, 2022

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 AUTHENTICATE * to abort the authentication, so it hangs on a pingpong loop

Respectively:

1666768709.393 C: CAP REQ sasl
1666768709.393 S: CAP Sopel ACK :sasl
1666768709.441 C: AUTHENTICATE SCRAM-SHA-256
1666768709.441 S: AUTHENTICATE +
[2022-10-26 09:18:29,489] sopel.coretasks      INFO     - Sending SASL SCRAM client first
1666768709.490 C: AUTHENTICATE biwsbj1qaWxsZXMscj1jOGMxNGUxZTRjOTc0Mzk5ODdiZmZlNjJjNGI5YjVmZA==
1666768709.547 S: AUTHENTICATE :cj1jOGMxNGUxZTRjOTc0Mzk5ODdiZmZlNjJjNGI5YjVmZDhkNjFiNjJiZDc2MDQ5NzQ4MzFmMTRkYjgyMTIxZGIzLHM9WlRJMVkyTmhOMk0zWmpSaU5EQTRZMkkxTVdSaFlUZG1OVFpoTnpVNVkyRT0saT00MDk2
[2022-10-26 09:18:29,623] sopel.coretasks      INFO     - Sending SASL SCRAM client final
1666768709.624 C: AUTHENTICATE Yz1iaXdzLHI9YzhjMTRlMWU0Yzk3NDM5OTg3YmZmZTYyYzRiOWI1ZmQ4ZDYxYjYyYmQ3NjA0OTc0ODMxZjE0ZGI4MjEyMWRiMyxwPVl4UWtZUmJGVkM3QjZRRERVejhOeklYcGlEczJQbWdRMEZUcUc1NTdVK289
1666768709.624 S: AUTHENTICATE :AAAA
[2022-10-26 09:18:29,668] sopel.bot            ERROR    - Unexpected KeyError ('v') from  at 2022-10-26 07:18:29.668634. Message was: AAAA
Traceback (most recent call last):
  File "/home/dev-irc/.local/lib/python3.9/site-packages/sopel/bot.py", line 648, in call_rule
    rule.execute(sopel, trigger)
  File "/home/dev-irc/.local/lib/python3.9/site-packages/sopel/plugins/rules.py", line 1203, in execute
    exit_code = self._handler(bot, trigger)
  File "/home/dev-irc/.local/lib/python3.9/site-packages/sopel/coretasks.py", line 1189, in auth_proceed
    bot._scram_client.set_server_final(server_final)
  File "/home/dev-irc/.local/lib/python3.9/site-packages/scramp/core.py", line 269, in set_server_final
    _set_server_final(message, self.server_signature)
  File "/home/dev-irc/.local/lib/python3.9/site-packages/scramp/core.py", line 562, in _set_server_final
    if server_signature != msg["v"]:
KeyError: 'v'
1666768711.920 C: PING 0.0.0.0

and

1666768717.764 C: CAP REQ sasl
1666768717.764 S: CAP Sopel ACK :sasl
1666768717.821 C: AUTHENTICATE SCRAM-SHA-256
1666768717.821 S: AUTHENTICATE +
[2022-10-26 09:18:37,864] sopel.coretasks      INFO     - Sending SASL SCRAM client first
1666768717.865 C: AUTHENTICATE biwsbj1qaWxsZXMscj05YjJkYWZjOTRlNzM0NTNlOGMxNDM2MjFlNDEwNzRiMw==
1666768717.915 S: AUTHENTICATE :cj05YjJkYWZjOTRlNzM0NTNlOGMxNDM2MjFlNDEwNzRiMzA1NGE5MzU3NTFjMjQ2NmQ4YjM1MjIwZTAxMTEzZTM4LHM9WVdJellqVmlNemN6WW1FME5ESmxZamd6WWpnMk56TmtPR014WXpRd09UWT0saT00MDk2
[2022-10-26 09:18:37,993] sopel.coretasks      INFO     - Sending SASL SCRAM client final
1666768717.994 C: AUTHENTICATE Yz1iaXdzLHI9OWIyZGFmYzk0ZTczNDUzZThjMTQzNjIxZTQxMDc0YjMwNTRhOTM1NzUxYzI0NjZkOGIzNTIyMGUwMTExM2UzOCxwPUNhQXFCZlladjJWSFZzU3U2cUdnZHo3M0dUODNuZFZ4NkJDejNaNTdkMFk9
1666768717.994 S: AUTHENTICATE :dj1ubU1mM1FIV2NKUWk5cE1ndHFLU0tQclZueUk2c3FOTzZJN3BFLzBveUdjPQ==
[2022-10-26 09:18:38,040] sopel.coretasks      ERROR    - SASL SCRAM failed: ScramException("The server signature doesn't match.")
[2022-10-26 09:18:38,040] sopel.bot            ERROR    - Unexpected ScramException (The server signature doesn't match. other-error) from  at 2022-10-26 07:18:38.040819. Message was: dj1ubU1mM1FIV2NKUWk5cE1ndHFLU0tQclZueUk2c3FOTzZJN3BFLzBveUdjPQ==
Traceback (most recent call last):
  File "/home/dev-irc/.local/lib/python3.9/site-packages/sopel/bot.py", line 648, in call_rule
    rule.execute(sopel, trigger)
  File "/home/dev-irc/.local/lib/python3.9/site-packages/sopel/plugins/rules.py", line 1203, in execute
    exit_code = self._handler(bot, trigger)
  File "/home/dev-irc/.local/lib/python3.9/site-packages/sopel/coretasks.py", line 1193, in auth_proceed
    raise e
  File "/home/dev-irc/.local/lib/python3.9/site-packages/sopel/coretasks.py", line 1189, in auth_proceed
    bot._scram_client.set_server_final(server_final)
  File "/home/dev-irc/.local/lib/python3.9/site-packages/scramp/core.py", line 269, in set_server_final
    _set_server_final(message, self.server_signature)
  File "/home/dev-irc/.local/lib/python3.9/site-packages/scramp/core.py", line 563, in _set_server_final
    raise ScramException(
scramp.core.ScramException: The server signature doesn't match. other-error
1666768718.042 C: AUTHENTICATE *

@half-duplex
Copy link
Member Author

half-duplex commented Oct 26, 2022

Re-raising the exception (example 2) was intentional, but I suppose is unnecessary. Good catch on the scramp KeyError.
Also added try/catch for bad server_first & b64

@Exirel
Copy link
Contributor

Exirel commented Oct 27, 2022

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.

@tlocke
Copy link

tlocke commented Nov 5, 2022

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 ScramException if not. So in the example above:

  File "/home/dev-irc/.local/lib/python3.9/site-packages/sopel/coretasks.py", line 1189, in auth_proceed
    bot._scram_client.set_server_final(server_final)
  File "/home/dev-irc/.local/lib/python3.9/site-packages/scramp/core.py", line 269, in set_server_final
    _set_server_final(message, self.server_signature)
  File "/home/dev-irc/.local/lib/python3.9/site-packages/scramp/core.py", line 562, in _set_server_final
    if server_signature != msg["v"]:
KeyError: 'v'

A ScramException with a descriptive message is raised instead. Thanks to @half-duplex for the patch 👍 Let me know if you come across any other problems.

@dgw
Copy link
Member

dgw commented Jan 2, 2023

@progval @tlocke Have your concerns all been addressed?

I'm going through some of our older open PRs and this one looks like it should be either ready or very nearly so.

@progval
Copy link

progval commented Jan 2, 2023

yes

@tlocke
Copy link

tlocke commented Jan 3, 2023

Actually I just got involved because I maintain the scramp library. If any problems come up with it, just let me know.

@dgw
Copy link
Member

dgw commented Jan 3, 2023

@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! 🏁

@Exirel Exirel modified the milestones: 8.0.0, 8.1.0 Jul 20, 2023
Copy link
Contributor

@Exirel Exirel left a comment

Choose a reason for hiding this comment

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

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.

test/test_coretasks.py Outdated Show resolved Hide resolved
# 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.

sopel/coretasks.py Outdated Show resolved Hide resolved
sopel/coretasks.py Outdated Show resolved Hide resolved
sopel/coretasks.py Outdated Show resolved Hide resolved
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 half-duplex force-pushed the sasl-scram branch 2 times, most recently from b894dc0 to ca33e00 Compare November 12, 2023 02:33
Comment on lines +145 to +150
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),
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.

# 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 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.

Comment on lines +1273 to +1302
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
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.

pyproject.toml Show resolved Hide resolved
Comment on lines +126 to +127
sopel_mechs = ["PLAIN", "EXTERNAL", "SCRAM-SHA-256"]
if mech not in sopel_mechs:
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.

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.

Comment on lines +145 to +150
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),
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.

Comment on lines +1273 to +1302
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
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.

@@ -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):
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@half-duplex half-duplex Nov 13, 2023

Choose a reason for hiding this comment

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

The exception raised is caught by Sopel and turned into a log message - I'm not sure how else I would check for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Comment on lines +379 to +388
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]
)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Suggested change
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.

Comment on lines +419 to +421
scram_server.set_client_first(
b64decode(mockbot.backend.message_sent[-1].split(b" ")[-1]).decode("utf-8")
)
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Comment on lines +442 to +445
assert (
len(mockbot.backend.message_sent) == 6
and mockbot.backend.message_sent[-1] == rawlist("CAP END")[0]
)
Copy link
Contributor

@Exirel Exirel Nov 13, 2023

Choose a reason for hiding this comment

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

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.

Comment on lines +442 to +445
assert (
len(mockbot.backend.message_sent) == 6
and mockbot.backend.message_sent[-1] == rawlist("CAP END")[0]
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto here about having only one assert here to check multiple condition. I really don't think it's good.

Copy link
Contributor

@Exirel Exirel left a comment

Choose a reason for hiding this comment

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

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.

test/coretasks/test_coretasks_sasl.py Outdated Show resolved Hide resolved
test/coretasks/test_coretasks_sasl.py Outdated Show resolved Hide resolved
test/coretasks/test_coretasks_sasl.py Outdated Show resolved Hide resolved
test/test_coretasks.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SASL EXTERNAL incorrectly requires a password to be set
7 participants