Rewrite registration code (Implement SASL, separate acct/server passwords)

[xyzshantaram]

As far as I can tell, Libera requires certain IP ranges (such as those used by DigitalOcean) to authenticate via SASL. This is troublesome for bots, so I've put together a quick draft of SASL PLAIN support for this library. I've extended the existing registration.ts code with a boolean option "useSasl". If this is set to true, the client tries SASL auth first and failing that reverts to using PASS. Would love to hear thoughts.

[jeromeludmann]

As far as I can tell, Libera requires certain IP ranges (such as those used by DigitalOcean) to authenticate via SASL. This is troublesome for bots, so I've put together a quick draft of SASL PLAIN support for this library. I've extended the existing registration.ts code with a boolean option "useSasl". If this is set to true, the client tries SASL auth first and failing that reverts to using PASS. Would love to hear thoughts.

@xyzshantaram Thanks a lot for your work 👍

Do you think you could add a little test in registration_test.ts to cover the trySasl sequence?

[jeromeludmann]

Could you also run make fmt after to make fmt tool happy?

[jeromeludmann]

@xyzshantaram I made a change about the CAP sequence in order to avoid sending CAP END twice.

I think my current design related to CAP is not convenient. Helper in cap.ts is not really useful, especially client.utils.sendCapabilities(). For now, all CAPs are sent in one shot. But I think they should be sent with more flexibility. What do you think?

Could you test the latest commit on Libera and say me if it works as expected? I will merge after :)

[xyzshantaram]

Hi, just saw this.
Thanks for the feedback and review! A few questions and some information:

1) My apologies for not sending it in with tests, I was just running a basic script on my end for testing purposes and didn't want to put too much into the PR before we ironed out the way the API would look. (For example, should the SASL be a separate plugin of its own, etc.) 2) I'll run make fmt before committing any changes from now on. 3) re: CAP messages, IRCv3 says that after sending CAP REQ sasl, we cannot send CAP END until SASL auth has either failed or succeeded, so that the server does not finalize connection registration before the SASL process is complete.

From https://ircv3.net/specs/extensions/sasl-3.1:

  If the client completes registration (with CAP END, NICK, USER 
  and any other necessary messages) while the SASL authentication 
  is still in progress, the server SHOULD abort it and send a 906 numeric,
  then register the client without authentication.

4) I agree that CAPs should allow for more flexibility, since there might be other stuff down the road that has a similar model. Maybe we can send the CAP [cmd] messages first and then the CAP END afterward, maintaining a list of CAPs that need to be sent which plugins can modify. 5) I'm not sure what the intention of removing the rpl_saslsuccess listener is. Do you want the client to listen for it themselves? The problem is that if we don't do this we have no idea if SASL registration succeeded and so we can't degrade gracefully down to doing PASS auth instead. If this was a deliberate decision I understand, but I wanted to bring it to your notice in case it wasn't. (As an aside, we can't use the same username for SASL and PASS, they can be different, apparently.) 6) Also, now that we have two authentication mechanisms we might need to add an event type that fires an event when auth is complete and the bot can start doing stuff, but this is a conversation for another day.

[xyzshantaram]

The code as it stands does seem to connect successfully to Libera, but I believe they just aren't enforcing the CAP END thing strictly. A spec-conforming client SHOULD deny the connection, so we should fix this before it goes into the library.

[jeromeludmann]

3. re: CAP messages, IRCv3 says that after sending CAP REQ sasl, we cannot send CAP END until SASL auth has either failed or succeeded, so that the server does not finalize connection registration before the SASL process is complete.

From https://ircv3.net/specs/extensions/sasl-3.1:

If the client completes registration (with CAP END, NICK, USER 
and any other necessary messages) while the SASL authentication 
is still in progress, the server SHOULD abort it and send a 906 numeric,
then register the client without authentication.

You are right, thank you for explaining 👍 I reverted these commits and just kept missing unit tests.

Unfortunately, for now I don't have time to dig deeper about what you said, but if you think some other changes should be made, I will be happy to merge them 😃

should the SASL be a separate plugin of its own

About this point, due to the event driven way, I think it would be complicated to manage SASL in an other place than registration.ts 🤔

[xyzshantaram]

What do you think needs to be done so we can merge basic SASL functionality? I'll try to make another commit by this evening with whatever you need done.

[jeromeludmann]

If it looks good for you for SASL basic feature, so I think we can merge it soon.

Some refactoring could be made later when additional SASL features will brought.

[xyzshantaram]

Hi, made a couple of changes right now. I made our impl comply with the spec by chunking the AUTHENTICATE message into 400-byte pieces and de-promisified trySasl. I have to ask, are you okay with falling back to silently sending the PASS registration or should we make it error out explicitly?

[xyzshantaram]

On second thought, I think it's better to let registration fail and not do anything in the SASL error case than fail silently.

[jeromeludmann]

and de-promisified trySasl

Just to know, why did you change trySasl to remove its promise?

I have to ask, are you okay with falling back to silently sending the PASS registration or should we make it error out explicitly?

A fallback could be an appropriate behavior but I wonder if there are cases where I do want to use SASL and not classical registration? Or another cases where I want to be notified that SASL failed?

On second thought, I think it's better to let registration fail and not do anything in the SASL error case than fail silently.

Yes I am more confortable with throwing an error instead of silently fallback to classical registration, at least as a default behavior. Generally I think it's always better to force the developer to change its own configuration. Doing this in a strict way avoids potential errors.

But I think also both behaviors seem useful:

• the default one should be "fails if SASL error" to prevent unwanted things,
• and the optional behavior would have a fallback to classical registration when SASL fails

The second point should be explicitly provided by the developer as an option.

For now, the default behavior looks good to me and would allow to add the second feature later.

[xyzshantaram]

Well I removed the Promise returned by trySasl because it makes the code a little neater and I realised I could listen for success/fail outside trySasl anyway. The Promise was just because there was no straightforward way to return fail/success information from trySasl (because of the callback-based API of the library).

I agree strongly on

force the developer to change its own configuration. Doing this in a strict way avoids potential errors. I will implement the fallbackToPASSReg flag later today. I wanted to ask, is there a way to force the client to immediately error out on raw:err_saslfail or raw:err_saslabort?

I wasn't sure if I can call emitError in plugin code, and I couldn't find another way to force an error. Currently it fails not when the SASL reg fails but when the server issues a "Closing Link".

Once we're done ironing out the fallback and error behaviour I think we should be good to merge this in.

[jeromeludmann]

Well I removed the Promise returned by trySasl because it makes the code a little neater and I realised I could listen for success/fail outside trySasl anyway. The Promise was just because there was no straightforward way to return fail/success information from trySasl (because of the callback-based API of the library).

👍

I wanted to ask, is there a way to force the client to immediately error out on raw:err_saslfail or raw:err_saslabort?

I wasn't sure if I can call emitError in plugin code, and I couldn't find another way to force an error. Currently it fails not when the SASL reg fails but when the server issues a "Closing Link".

emitError is indeed the current way to force an error from plugins. I think you could do something like following:

// plugins/registration.ts

const onSaslFail = (payload) => {
  // ... do something like client.disconnect() if needed

  // emits properly an error
  // note: "read" is the type of the error, the second arg the message bound
  // and the third arg is only here to have a clearer call stack.
  client.emitError("read", "ERROR: SASL failed", onSaslFail)  
}

// run onSaslFail on "raw:err_saslfail" AND "raw:err_saslabort"
client.on(["raw:err_saslfail", "raw:err_saslabort"], onSaslFail);

// or

// run onSaslFail on "raw:err_saslfail" OR "raw:err_saslabort"
client.once(["raw:err_saslfail", "raw:err_saslabort"], onSaslFail);
// but you have to ensure that listener gets added again on each connection

You can find existing examples of emitError usage in:

• plugins/throw_on_error.ts (which throws an new Error(...) on IRC server ERROR)
• plugins/ping_timeout.ts

Once we're done ironing out the fallback and error behaviour I think we should be good to merge this in.

👍

---

Nothing to do with this PR, but I just realized the current "types" of throwable errors by emitError are focused on TCP sockets ("read", "write", "connect", "close") and I find they are not very useful to help developers to handle these specific errors 🤔

[xyzshantaram]

Okay, I've implemented the tryPassOnSaslFail flag (might need to come up with a better name for it). It seems to work fine according to my (admittedly limited) testing. We might also need to separate the PASS password from the SASL password because they could be different.

Re:

Nothing to do with this PR, but I just realized the current "types" of throwable errors by emitError are focused on TCP sockets ("read", "write", "connect", "close") and I find they are not very useful to help developers to handle these specific errors 🤔

The error type could just be a generic string with probably a defined format and a "strong recommendation" not to define too many weird error types.

[jeromeludmann]

We might also need to separate the PASS password from the SASL password because they could be different.

I agree, if both default password / SASL password can be different, we have to use specific variable.

[jeromeludmann]

Okay, I've implemented the tryPassOnSaslFail flag (might need to come up with a better name for it)

Just a thought, rather than having useSasl and tryPassOnSaslFail we could have:

registrationMode: 'default' | 'sasl' | 'saslOtherwiseDefault'

What do you think? 🤔

I don't know if it would be tricky to force existing value for password and saslPassword, depending on whether we use default or sasl mode.

[xyzshantaram]

registrationMode: 'default' | 'sasl' | 'saslOtherwiseDefault'

Definitely works. Also, the PASS command is for a connection password and not a NickServ password, so we probably also need to modify the code for that. (It does work on Libera, but this is nonstandard).

I'd say we should make this whole thing a separate auth plugin, and then have registrationMode: 'NickServ' | 'sasl' | 'saslThenNickServ', and rename the current password field to serverPassword, freeing the password field for use as an account password.

This way

• we can use the PASS command to send a server password independent of the user account
• We can use SASL to authenticate the user (using the newly freed password field).
• Failing that, we can use NickServ auth as usual (/msg NickServ username password). Again, password would be the password field supplied in options.

[glguy]

• we can use the PASS command to send a server password independent of the user account

This is an important improvement, because it's almost certain that a connection that uses a server password will use a different SASL password.

[xyzshantaram]

I've separated the PASS command from NickServ / SASL PLAIN auth as described in https://github.com/jeromeludmann/deno-irc/pull/6#issuecomment-1729812153. Please let me know what you think. I haven't split off this stuff into an auth plugin though since I realised connection registration cannot occur without SASL completing.

Should this code perhaps be a helper? (As in, client.helpers.authenticate?)

[jeromeludmann]

I've separated the PASS command from NickServ / SASL PLAIN auth as described in #6 (comment). Please let me know what you think. I haven't split off this stuff into an auth plugin though since I realised connection registration cannot occur without SASL completing.

Really better, thank you for pointing out this (and adding NickServ authentication). I find the distinction between serverPassword and password more obvious now.

Should this code perhaps be a helper? (As in, client.helpers.authenticate?)

Not sure, not convinced for now by a dedicated helper to store this logic. I think this internal mechanism should stay in registration.ts plugin. But maybe you have a good reason to create an helper?

In all cases, thank you for the good work 🙏 Let me know when you will be OK to merge.

[xyzshantaram]

Really better, thank you for pointing out this (and adding NickServ authentication). I find the distinction between serverPassword and password more obvious now.

Just fyi, here's what I was told about it today (by none other than @glguy from above actually, thank you glguy and others in #libera!)

<glguy> shantaram: PASS gets used either for a server password (in which case it will not be forwarded to NickServ) or if it's not used for anything else then it's forward to NickServ (the important thing is that it's not a guarantee)

Apparently it's a hack for historical reasons, not really standards-compliant. In all honesty, NickServ isn't part of a standard either, but it's a de facto one at this point so we should support it.

Re: merging, I think this is good enough to merge as it stands. Let me know if there are any more housekeeping tasks (like docs, testing) that need doing which I missed.

As for the code going in a helper, I was just wondering if it might be better to separate auth from connection registration. I suppose our hands are tied though because of the way SASL works.

[jeromeludmann]

Thank you for these infos. Some things are sometimes strange about IRC history.

In all honesty, NickServ isn't part of a standard either, but it's a de facto one at this point so we should support it.

Yea, at first I did not want NickServ to be included since it was not related to protocol... but after all I think the most important thing is that this feature is useful for as many people as possible, even if it's not a strict standard feature of IRC.

Re: merging, I think this is good enough to merge as it stands. Let me know if there are any more housekeeping tasks (like docs, testing) that need doing which I missed.

I will merge it this weekend.

As for the code going in a helper, I was just wondering if it might be better to separate auth from connection registration. I suppose our hands are tied though because of the way SASL works.

Ideally, I would like to have:

• server password and SASL in registration.ts,
• and NickServ in another plugin nickserv.ts.

But nickserv.ts should be aware of the registration sequence (SASL or not) and the event driven style is probably not appropriate for such a case. It seems complicated to me to do that with two distinct plugins rather than just one, without adding unwanted complexity.

[glguy]

While you're hacking on this, something that is worth remembering is that SASL isn't specifically a registration-time transaction. There are cases for doing it after registration, too. Services might not have been up during registration. You might have logged out and want to log back in at connection time. Combined with an authorization identity, SASL can be used to switch identities at runtime.

[jeromeludmann]

@glguy Very interesting, thank you for sharing 🙏