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

Add SASL PLAIN authentication support #261

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

anisse
Copy link

@anisse anisse commented May 1, 2024

Add a new config option "sasl", which can be set to "none" or "plain". In none, behavior does not change and will be the same as, using the password config option with PASS.
When plain is selected, the new "login" option will be coupled with the existing "password" option to connect to the server with SASL PLAIN mode.

The implementation currently does blind SASL login. It does not check if the server supports sasl, sasl plain, and does not verify login errors, etc.
For simplicity, I chose not to add a state-machine that would handle verifying SASL support, login success, etc. It would need a different, async API for that, and I thought it would be overkill. It has been tested successfully with libera.chat.

This PR adds a new dependency "base64ct", a base64 crate by the RustCrypto project with no dependency (here std is enabled though); if you prefer I can add an ad-hoc licence compatible base64 encode function. I'd prefer not to guard the SASL support behind a feature if possible.

Tests were added and doc has been updated.

Related to #166

@anisse anisse force-pushed the develop branch 2 times, most recently from f8c59f3 to 660d29f Compare May 3, 2024 17:57
Add a new config option "sasl", which can be set to "none" or "plain".
In none, behavior does not change and will be the same as before, using
the password config option with PASS if present.
When plain is selected, the new "login" option will be coupled with the
existing "password" option to connect to the server with SASL PLAIN
mode.

The implementation currently does blind SASL login. It does not check if
the server supports sasl or sasl plain, and does not verify login errors,
etc. It has been successfully tested with irc.libera.chat.
@anisse
Copy link
Author

anisse commented May 3, 2024

So... I ran cargo semver-checks and technically this change is a semver violation. I wanted to add a commit to mark the Config struct as non-exhaustive, but this is an even bigger breakage. Since both new fields (sasl and login) are Option, which has a default and can be Deserialized from a config file, I think it's not to big of an issue.

Copy link

@progval progval left a comment

Choose a reason for hiding this comment

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

This is probably a bit out of scope for this PR and a niche edge case, but AUTHENTICATE messages are limited to a payload of 400 base64 characters; so long username + password combinations need to be line-wrapped.

Comment on lines +101 to 103
/// The password to connect to the server. Used with PASS or with SASL authentication
#[cfg_attr(feature = "serde", serde(skip_serializing_if = "Option::is_none"))]
pub password: Option<String>,
Copy link

Choose a reason for hiding this comment

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

You may want to make it a different configuration option. PASS's intended use is a shared server password, that is required to connect to the server at all; while SASL is to authenticate to a specific user account. That PASS can also be used as an alternative to SASL to authenticate to user account is an implementation-specific behavior of some servers.

Copy link
Author

Choose a reason for hiding this comment

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

I don't see a practical use to having both used at the same time; the "Modern IRC" document says that SASL is used as alternative to PASS, so I don't think we should see both used in practice. I don't mind adding a second, different option though.

Comment on lines +1084 to +1092
let sasl_pass = Base64::encode_string(
&format!(
"\x00{}\x00{}",
self.config().login(),
self.config().password(),
)
.bytes()
.collect::<Vec<_>>(),
);
Copy link

Choose a reason for hiding this comment

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

you might want to check the login and password don't contain null bytes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants