client: reconnect nick changing loop because of nickserv or no support for nick related callbacks

[ludviglundgren]

Hi!

I've noticed a weird behaviour with the client that I've seen happen with InspIRCd v3 and possibly other servers. Ergo behaves a bit different as well.

When using the client with NickServ auth which I run in the AddConnectCallback it can get into a state where after a reconnect NickServ isn't immediately available (401 no such nick) and thus not being able to authenticate. It's given 20 seconds to authenticate and failing to do so it then changes the nick to some GuestXXXX variant.

It will then after X seconds retry to set the nick (built in functionality) and it will be prompted to authenticate again with NickServ but there's no way to add a callback for this it seems. And this will loop until it's fully restarted again.

1. Server disconnects for some reason
2. Client reconnects after X retries
3. ConnectCallback is run. sleep 4 seconds then try to authenticate with NickServ
4. NickServ for some reason isn't available and responds with 401 no such nick
5. After 20 seconds our nick is changed automatically
6. The client then tries to change back our nick after some interval
7. When we change back to our nick we get the prompt to NickServ IDENTIFY or our nick will be changed after 20 seconds
8. loop back to 6

I've tried to add a callback for NICK, NICKNAME_RESERVED and 433 but none of them is run when this happens.

Was this removed after the thoj fork? Here's a log of what happened.

Any ideas what to do or if something needs to be changed?

[slingamn]

If I understand correctly, adding a callback for NICK should work --- such a callback will fire whenever your nick is changed automatically.

I tested it with the following toy client:

https://gist.github.com/slingamn/7ab68ad518e88146dd050037b00941ab

I connected it to the Ergo testnet and SANICK'ed it to robot_, to which it responded as follows:

2022/06/16 14:47:08 <-- @msgid=iq3ax96hmt9vmbjf25b2kb7i4a;time=2022-06-16T18:47:08.093Z :robot!~u@m2ux9rekc7xyw.oragono NICK robot_
2022/06/16 14:47:08 --> PRIVMSG NickServ :IDENTIFY robotpassword
2022/06/16 14:47:08 <-- @time=2022-06-16T18:47:08.175Z :NickServ!NickServ@localhost NOTICE robot_ :Authentication failed: Account does not exist

(Just checking: does this network support SASL?)

[ludviglundgren]

Thanks for quick reply! It's does not as far as I'm aware.

I'll try it out again then!

[slingamn]

(That toy example uses a one-argument form of IDENTIFY, but you would need to use the two-argument form IDENTIFY <nickname> <password>, since at the time the NICK callback fires, the currently held nickname is still wrong.)

[ludviglundgren]

I've seen mixed result with that, some networks support it and others don't. Which might make this harder as well.

[ludviglundgren]

I have another question as well.

Would it be possible to get a OnDisconnect, DisconnectCallback, ReconnectCallback handler or similar?

The usecase for this would be to be able to send external notifications when the client looses connection. I can handle parts of it in the AddConnectCallback, but that would happens first when the client have connected again.

Or if you have any ideas for a solution to this? The error from Connect() is triggered only if it can't connect at the initial start, not on reconnect.

[slingamn]

Would it be possible to get a OnDisconnect, DisconnectCallback, ReconnectCallback handler or similar?

That seems reasonable, yeah. I'll look into adding this for the next release. Would a DisconnectCallback cover your use case?

Are you writing an interactive client, as opposed to a bot? There are some other cases I would want to fix for you in that case. For example, interactive users would expect an explicit Reconnect() to interrupt the ReconnectFreq delay, but right now it doesn't.

[slingamn]

re. the single-argument form of NickServ, you could try adding a callback for NOTICE, checking if the sender is NickServ, possibly doing a string match for "registered and protected" or similar (ugh), and then responding by attempting to identify. This would fire during the 20-second window where the client temporarily holds the nick.

This whole issue is somewhat fraught from a security perspective (if NickServ can be offline, could someone else be squatting the NickServ name? not sure how conventional ircds handle this)

[ludviglundgren]

That seems reasonable, yeah. I'll look into adding this for the next release. Would a DisconnectCallback cover your use case?

Are you writing an interactive client, as opposed to a bot? There are some other cases I would want to fix for you in that case. For example, interactive users would expect an explicit Reconnect() to interrupt the ReconnectFreq delay, but right now it doesn't.

Great! Yes I think that would be enough. That way it could trigger events automatically.

This is for a bot/non interactive client. The program can connect to a bunch of networks and channels and listen for messages to then further process.

It does have an ui where things can be controlled, like restarts etc. Currently I do that with a quit then full restart.

re. the single-argument form of NickServ, you could try adding a callback for NOTICE, checking if the sender is NickServ, possibly doing a string match for "registered and protected" or similar (ugh), and then responding by attempting to identify. This would fire during the 20-second window where the client temporarily holds the nick.

This whole issue is somewhat fraught from a security perspective (if NickServ can be offline, could someone else be squatting the NickServ name? not sure how conventional ircds handle this)

Hm yeah maybe that's the way. Yeah valid security concern but not sure how big of an issue it could be.

[slingamn]

Cool, let me know how it goes. If you decide to go the NOTICE-matching route, I remembered that ZNC has an implementation of this:

https://github.com/znc/znc/blob/znc-1.8.2/modules/nickserv.cpp#L103-L129

so most existing services frameworks should "work" with these heuristics. (SASL is much easier to use when it's supported!)

[ludviglundgren]

Great, will check that out!

Quassel as an example have authentication in the network setup, where you can choose SASL or NickServ - maybe built in NickServ handling could be a nice addition to the core library for the future? :wink:

[slingamn]

Ah, basically for all the reasons we've discussed in this thread, I consider NickServ to be deprecated in favor of SASL, so probably not :-) However, such code would definitely be suitable for examples/ or possibly a new contribs/ directory outside of the stable API.

[ludviglundgren]

Wow, already done?! Appreciate it a lot. Will you be making a new release soon or still have other things to include?

[slingamn]

I'm not sure --- I think I might do a release next week. But in the meantime, I would actually appreciate it if you could beta-test the new API in your application :-) You can do:

go get -v github.com/ergochat/irc-go@0187c396b8d2e2a7f583ca5728063aeed8b9a359

to use the current master branch.

BTW did you manage to solve the nickserv issues? If it turns out there is some limitation in irc-go blocking you, I would definitely to fix it in time for the next release.

[ludviglundgren]

Already pulled and tested! And it works! But it is triggered on every failed re-connect attempt. Is that intended? Ideally it would trigger once when it's disconnected and not failed retries. But I can work around that if it's intended and store when it was last called and do some time logic.

The main thing I found was that my local ergo server used the default config which held on to nicks/accounts etc which made it not work like I expected. I changed to traditional which matches more how existing networks seem to work and then part of the problem was fixed.

Also that the CurrentNick() wasn't updated when a new nick was force changed, and PreferredNick() was also a bit behind it seems :thinking:

What related limitations did you find?

[slingamn]

I'm going to close this since most of what we discussed has been addressed in #81, #82, and #85. Please do open a new issue if I missed something or if you have additional reports :-)