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-ir extension #520

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

Add sasl-ir extension #520

wants to merge 4 commits into from

Conversation

emersion
Copy link
Contributor

Pretty similar to the IMAP SASL-IR extension.

extensions/sasl-ir.md Outdated Show resolved Hide resolved
extensions/sasl-ir.md Show resolved Hide resolved
extensions/sasl-ir.md Outdated Show resolved Hide resolved
@progval
Copy link
Contributor

progval commented Mar 21, 2023

Implemented in Matrix2051: progval/matrix2051@e03f2be

extensions/sasl-ir.md Outdated Show resolved Hide resolved
emersion and others added 2 commits March 21, 2023 19:36
Co-authored-by: dgw <dgw@technobabbl.es>
@slingamn
Copy link
Contributor

Independently of this spec, thoughts on adding a non-normative caution to the SASL spec, recommending that unrecognized mechanisms should not be logged?

@emersion
Copy link
Contributor Author

Done here: #522

@grawity
Copy link
Contributor

grawity commented Mar 24, 2023

could there be some guidance regarding mechanisms that are server-first? e.g. should the server explicitly abort if IR is used but the server's initial challenge is not empty (i.e. if it's not "AUTHENTICATE +")

I don't remember such mechanisms offhand (and the existing ones wouldn't be relevant for IRC to be fair) but afaik they do exist

extensions/sasl-ir.md Outdated Show resolved Hide resolved
Co-authored-by: Sadie Powell <sadie@witchery.services>
@slingamn
Copy link
Contributor

slingamn commented Apr 2, 2023

Summarizing some discussion from #ircv3, it seems like in order for this extension to provide a speedup, the client has to cheat slightly (by caching a previous CAP LS 302 response from the server, so that it knows that sasl-ir is available even without a CAP LS roundtrip). But if the client is willing to cheat a little more than that (by caching the availability of sasl=PLAIN), then it can get the speedup even without this extension, by preemptively sending its SASL messages without waiting for acknowledgement. (To be clear: I think this kind of cheating is good and should be encouraged in mobile clients.)

@emersion
Copy link
Contributor Author

emersion commented Apr 5, 2023

The goal of this extension is not to provide a speedup (clients can already do it), it's to make the protocol more reliable. If the client caches sasl=WHATEVER and the server happens to change their supported capabilities, then the credentials are interpreted as a SASL mechanism by the server.

@slingamn
Copy link
Contributor

If the client caches sasl=WHATEVER and the server happens to change their supported capabilities, then the credentials are interpreted as a SASL mechanism by the server.

Are we talking about a TOCTOU race between the server sending a CAP LS 302 response including sasl=PLAIN and the client attempting to use it, within the same session?

Or is the idea that a cached cross-session observation of sasl-ir is more stable than a cached observation of sasl=PLAIN? (In theory, a cached observation of sasl-ir can also become invalid.)

@emersion
Copy link
Contributor Author

The idea of this extension is to make it safer to pipeline AUTHENTICATE commands without waiting for the CAP LS reply, by making it explicit which parameters are credentials and sensitive info in an AUTHENTICATE command. Right now a server can't make the difference between the start of a SASL exchange and the first initial response sent by the client.

@DarthGandalf
Copy link
Member

Maybe stop using the same keyword AUTHENTICATE to pass both mechanism and response, and start using different keywords for them?

@emersion
Copy link
Contributor Author

If we want to make bigger changes, I'd suggest adding a * parameter indicating a continuation like CAP does:

AUTHENTICATE PLAIN
AUTHENTICATE * <base64>

@slingamn
Copy link
Contributor

The idea of this extension is to make it safer to pipeline AUTHENTICATE commands without waiting for the CAP LS reply, by making it explicit which parameters are credentials and sensitive info in an AUTHENTICATE command. Right now a server can't make the difference between the start of a SASL exchange and the first initial response sent by the client.

This issue would still exist for line continuations, right?

It seems to me that the concerns here don't really justify adding complexity to one of the most widely implemented and deployed IRCv3 specs.

(For what it's worth, the draft as written still has a performance rationale in the introduction section. I think I understand most of the robustness rationale now, but I'm still not really convinced.)

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

7 participants