Closed Bug 954910 Opened 10 years ago Closed 10 years ago

Handle NickServ messages when authenticating with PASS

Categories

(Chat Core :: IRC, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: clokep)

References

Details

(Whiteboard: [1.2-wanted])

Attachments

(2 files, 7 obsolete files)

*** Original post on bio 1477 at 2012-06-01 21:37:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***
Blocks: 954154
Attached patch Patch v1 (obsolete) — Splinter Review
*** Original post on bio 1477 as attmnt 1545 at 2012-06-01 21:37:00 UTC ***

We can do some nicer things by authenticating with the NickServ directly instead of forcing the PASS command to forward to it.

Related: bug 954448 (bio 1013).
Attachment #8353300 - Flags: review?(bugzilla)
Attached patch Patch v2 (obsolete) — Splinter Review
*** Original post on bio 1477 as attmnt 1546 at 2012-06-01 22:16:00 UTC ***

Fixes a bug Mic found (where we were only running ChanServ if NickServ was enabled).
Attachment #8353301 - Flags: review?(florian)
Comment on attachment 8353300 [details] [diff] [review]
Patch v1

*** Original change on bio 1477 attmnt 1545 at 2012-06-01 22:16:30 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353300 - Attachment is obsolete: true
Attachment #8353300 - Flags: review?(bugzilla)
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Comment on attachment 8353301 [details] [diff] [review]
Patch v2

*** Original change on bio 1477 attmnt 1546 at 2012-06-02 12:53:56 UTC ***

Looks good :)

Is there no way we can auto-detect if NickServ is supported?
Attachment #8353301 - Flags: review?(florian) → review+
Whiteboard: [checkin-needed]
*** Original post on bio 1477 at 2012-06-02 13:26:49 UTC ***

(In reply to comment #2)
> Comment on attachment 8353301 [details] [diff] [review] (bio-attmnt 1546) [details]
> Patch v2
> Is there no way we can auto-detect if NickServ is supported?

As far as I know...no. We could just blindly send to NickServ if we receive these messages, but that would let us potentially send a password to unknown people. I don't want to guess and auto-send someone's password to a nick posing as NickServ.

We could still send to the PASS command in both situations, but I think if we know the NickServ is in use...it makes more sense to directly send it there. Unfortunately this still wouldn't support connecting to a server with a password and then authenticating with NickServ...if that situation even exists anywhere in the wild.

There is a SERVLIST command [1], but it's an optional feature (and in fact moznet doesn't support it).

You can kind of get it when you do WHOIS, it returns:
:concrete.mozilla.org 311 clokep_js NickServ services mozilla.org * :Nickname Server
:concrete.mozilla.org 313 clokep_js NickServ :is a Network Service

Which show the "host" as "services" and that it's an IRC operator with a comment of "Network Service". I don't know how fragile this would be check.

For Freenode:
:kornbluth.freenode.net 311 clokep_js NickServ NickServ services. * :Nickname Services
:kornbluth.freenode.net 312 clokep_js NickServ services. :Atheme IRC Services

Which show the host as "NickServ" and kind of garbage after that, and then respond that it's a Server with a comment of "Atheme IRC Services".

This leads me to believe there's no "good" way to figure this out. If it makes you feel better I hope to eventually add other ways to authenticate: SASL and Ident. I think there's also a couple of other's: one called "Auth" and I think you can authenticate with an SSL certificate on some servers.

[1] http://tools.ietf.org/html/rfc2812#section-3.5.1
*** Original post on bio 1477 at 2012-06-02 13:37:49 UTC ***

Another option could be to check the user and host of the incoming messages:

:NickServ!services@mozilla.org NOTICE clokep_js :<some message>
The "services@mozilla.org" combination of user + host should mean it's a service...but how do you get the "mozilla.org" from "irc.mozilla.org"?

:NickServ!NickServ@services. NOTICE clokep_js :<some message>
This doesn't match, it has a user of NickServ and a server "services.".

This is probably the sort of information that would be good to autoconfigure on each network.
*** Original post on bio 1477 at 2012-06-02 17:09:40 UTC ***

We're going to hold off on supporting this, see if there's a way to do it without the user having to manually choose to use a NickServ.
Whiteboard: [checkin-needed]
*** Original post on bio 1477 at 2012-06-12 17:52:46 UTC ***

So to summarize the issues here:
1. We don't want to send passwords to someone unless we're sure they are "NickServ".
2. We don't want to make it confusing by adding other advanced options which aren't really necessary (e.g. if we can automatically find the NickServ, don't ask the user if we should use it).
3. We want to handle annoying NickServ messages without ever showing them to the user.

My proposal:
1. Continue sending the password with the PASS command always.
2. When we receive a message asking us to authenticate, we send the password to the NickServ (after doing some basic checks that might tell us it's a service).
3. We eat the message saying that we're already identified (if the pass command worked).
4. A hidden pref should be added to get the NickServ's name (services.nickserv), if this is user set and blank, it will match no one and no NickServ would be used. Otherwise it is used as the NickServ name. (This would be done for ChanServ, etc. as well.) These would, of course, default to NickServ, ChanServ, etc.

This keeps us from doing weird timers and things like that (it makes everything stateless). Thoughts?
*** Original post on bio 1477 at 2012-06-12 19:16:59 UTC ***

This sounds like a good plan overall.

Could you swap the order of 2) and 3), to avoid sending the password twice when it's not necessary? Or does that run into trouble with timeouts (or is there no response if PASS fails)?

I don't understand 4) - it sounds like l10n, but I guess that's not what you mean.

There should probably be an advanced account option to select the authentification method. If set to NickServ, you would do without PASS I suppose?
*** Original post on bio 1477 at 2012-06-12 19:22:13 UTC ***

(In reply to comment #7)
> This sounds like a good plan overall.
> 
> Could you swap the order of 2) and 3), to avoid sending the password twice when
> it's not necessary?
This doesn't make any sense, how would you get the message saying you've already identified if you haven't sent the password a second time?

> Or does that run into trouble with timeouts (or is there no
> response if PASS fails)?
If PASS fails and a password is required to connect you are kicked off the network. If it's used just for NickServ authentication, then the NickServ might send us a message, I'm not sure.

> I don't understand 4) - it sounds like l10n, but I guess that's not what you
> mean.
A network might rename the NickServ to something else. This might be unnecessary though until we know it's necessary.

> There should probably be an advanced account option to select the
> authentification method. If set to NickServ, you would do without PASS I
> suppose?
The goal of this was to avoid the advanced option for authentication method. If you included the option, then this is pretty much what my current patch does.
*** Original post on bio 1477 at 2012-06-12 19:33:36 UTC ***

(In reply to comment #8)
> This doesn't make any sense, how would you get the message saying you've
> already identified if you haven't sent the password a second time?

Ah, I misunderstood what you meant by that message. I thought there was a confirmation message sent when NickServ identified you, but unfortunately there is no such thing.

> > I don't understand 4) - it sounds like l10n, but I guess that's not what you
> > mean.
> A network might rename the NickServ to something else. This might be
> unnecessary though until we know it's necessary.

OK, so this is an account-specific pref.

> > There should probably be an advanced account option to select the
> > authentification method. If set to NickServ, you would do without PASS I
> > suppose?
> The goal of this was to avoid the advanced option for authentication method. If
> you included the option, then this is pretty much what my current patch does.

This suggestion was just meant as an override for rare cases. But I suppose it's not needed in practice unless there really are servers that behave strangely. Maybe it should wait until it actually causes problems for someone...
*** Original post on bio 1477 at 2012-06-12 19:42:57 UTC ***

(In reply to comment #9)
> (In reply to comment #8)
> > This doesn't make any sense, how would you get the message saying you've
> > already identified if you haven't sent the password a second time?
> 
> Ah, I misunderstood what you meant by that message. I thought there was a
> confirmation message sent when NickServ identified you, but unfortunately there
> is no such thing.
There is, but it's received after the query for a password. See below:
--> PASS <password>
--> NICK <nickname>
--> USER <username>
<-- 001 ...
...
<-- :NickServ PRIVMSG <you> :This nick is owned by someone else.  Please choose another.
[...possible other messages telling us how to identify...]
<-- :NickServ PRIVMSG <you> :Password accepted - you are now recognized.

If we were to send the following after already being identified:
--> PRIVMSG NickServ :IDENTIFY <password>
<-- :NickServ PRIVMSG <you> :You are already identified.

Hopefully that clears up what is happening.
Attached patch Alternate patch v1 (obsolete) — Splinter Review
*** Original post on bio 1477 as attmnt 1698 at 2012-06-26 01:51:00 UTC ***

This is an alternate patch which attempts to use a timer to queue messages until we receive a successful login message.
Attachment #8353455 - Flags: review?(florian)
Comment on attachment 8353455 [details] [diff] [review]
Alternate patch v1

*** Original change on bio 1477 attmnt 1698 at 2012-06-26 01:52:09 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353455 - Flags: review?(bugzilla)
Comment on attachment 8353455 [details] [diff] [review]
Alternate patch v1

*** Original change on bio 1477 attmnt 1698 at 2012-06-26 08:20:48 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353455 - Flags: review?(florian)
Attachment #8353455 - Flags: review-
Attachment #8353455 - Flags: feedback+
*** Original post on bio 1477 at 2012-06-26 08:20:48 UTC ***

Comment on attachment 8353455 [details] [diff] [review] (bio-attmnt 1698)
Alternate patch v1

Overall I like this approach better than the previous patches I've seen, but the code is not ready.

>diff --git a/chat/protocols/irc/ircServices.jsm b/chat/protocols/irc/ircServices.jsm

>+ * Since the "protocol" behind services is really just text-based, human
>+ * readable messages, attempt to parse them, but if always fall back to just
>+ * showing the message to the user if we're unsure what to do.

"if always fall back" looks like you forgot to remove or reorder some words ;).


>+    "NickServ": function(aMessage) {
>+      let text = aMessage.params[1];
>+
>+      // If we sent something bogus, always show this to the user.
>+      // Unknown command \x02<command>\x02. "/msg NickServ HELP" for help.
>+      if (text.indexOf("Unknown command") == 0)
>+        return false;
>+
>+      // Uh-oh, we used the wrong password.
>+      if (text == "Password incorrect.") {

The same 'Password incorrect' message is handled further down. Did you forget to remove some code after doing some refactoring?

>+        WARN("Password not accepted by NickServ, authenticate manually.");
>+        return false;
>+      }
>+
>+      // The second time around we need to check if this message is in the queue.

I don't understand that comment.

>+      ERROR("HERE " + JSON.stringify(aMessage) + "\n" + this.waitingForPassCommand);

Remove this?

>+      if (this.waitingForPassCommand)

This variable name is confusing. After reading the whole patch, I believe when it's true it means the automatic authentication with the PASS command isn't supported by the server, and we decided the user will deal with nickserv directly.

>+          return false;

Fix the indent.

>+
>+      // NickServ wants us to identify.
>+      if (text == "This nick is owned by someone else.  Please choose another." || // Anope.
>+          text == "This nickname is registered. Please choose a different nickname, or identify via \x02/msg NickServ identify <password>\x02.") { // Atheme.
>+        // This is the first time we've seen this authentication request.
>+        LOG("Authentication requested by NickServ.");
>+
>+        // Wait one second before showing the message to the user (giving the
>+        // the server time to process the PASS command).
>+        this.nickServMessageQueue = [aMessage];
>+        this.nickservAuthTimeout =

nickServ or nickserv in variable name? Any way, consistency is better ;).

>+          setTimeout(function() {
>+            this.waitingForPassCommand = true;
>+            this.nickServMessageQueue.every(function(aMessage)
>+              ircHandlers.handleMessage(this, aMessage), this);
>+            delete this.waitingForPassCommand;
>+          }.bind(this), 1000);
>+        return true;
>+      }
>+
>+      // The following two messages are sent by Anope after informing us we need
>+      // to identify. Just eat them.
>+      if (text == "(If this is your nick, type \x02/msg NickServ IDENTIFY \x1Fpassword\x1F\x02.)" ||
>+          text == "If you do not change within one minute, I will change your nick.") {
>+        this.nickServMessageQueue.push(aMessage);
>+        return true;
>+      }
>+
>+      // We need to queue invalid passwords here too so that the messages appear
>+      // in the right order.
>+      if (text == "Password incorrect." || // Anope.
>+          text.indexOf("Invalid password for ") == 0) { // Atheme.
>+        this.nickServMessageQueue.push(aMessage);

In this case, shouldn't we call ircHandlers.handleMessage for all queued messages, and then delete the queue, and cancel and delete the timer, like you do for successful auth?

>+        return true;
>+      }
>+
>+      // Password successfully accepted by NickServ.
>+      if (text == "Password accepted - you are now recognized." || // Anope.
>+          text == "You are now identified for \x02" + aMessage.params[0] + "\x02.") { // Atheme.
>+        LOG("Successfully authenticated with NickServ.");
>+        // If we're waiting for an authentication (and have a queue of messages
>+        // waiting), then the PASS command succeeded and we don't need to
>+        // display those messages.
>+        if (this.nickServMessageQueue) {
>+          clearTimeout(this.nickservAuthTimeout);
>+          delete this.nickServMessageQueue;
>+          delete this.nickservAuthTimeout;

Maybe swap these two lines, so that the deletion of the timer is immediately after canceling it? :)
Attached patch Alternate patch v2 (obsolete) — Splinter Review
*** Original post on bio 1477 as attmnt 1709 at 2012-06-27 00:44:00 UTC ***

This patch is done slightly differently -- it doesn't attempt to match every possible message we can get and queue them after receiving a login request...it just queues EVERYTHING, and then prints them all out after 1 second. I think this is a bit more robust.

It also takes into account the review comments from florian.
Attachment #8353467 - Flags: review?(florian)
Comment on attachment 8353455 [details] [diff] [review]
Alternate patch v1

*** Original change on bio 1477 attmnt 1698 at 2012-06-27 00:44:06 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353455 - Attachment is obsolete: true
Attachment #8353455 - Flags: review?(bugzilla)
Blocks: 953609
Attached patch Alternate patch v3 (obsolete) — Splinter Review
*** Original post on bio 1477 as attmnt 1741 at 2012-07-14 15:02:00 UTC ***

Updated patch with dead code removed.
Attachment #8353501 - Flags: review?(florian)
Comment on attachment 8353301 [details] [diff] [review]
Patch v2

*** Original change on bio 1477 attmnt 1546 at 2012-07-14 15:02:49 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353301 - Attachment is obsolete: true
Comment on attachment 8353467 [details] [diff] [review]
Alternate patch v2

*** Original change on bio 1477 attmnt 1709 at 2012-07-14 15:02:49 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353467 - Attachment is obsolete: true
Attachment #8353467 - Flags: review?(florian)
Attached patch Alternate patch v4 (obsolete) — Splinter Review
*** Original post on bio 1477 as attmnt 1742 at 2012-07-14 15:29:00 UTC ***

Fixes some nits from flo and aleth (changed a name and some spacing). As well as fixes the order of handling different messages.

To be explicit: this patch does NOT attempt to authenticate to NickServ, it just gracefully handles the NickServ messages received when the PASS command authentication works.
Attachment #8353502 - Flags: review?(florian)
Comment on attachment 8353501 [details] [diff] [review]
Alternate patch v3

*** Original change on bio 1477 attmnt 1741 at 2012-07-14 15:29:22 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353501 - Attachment is obsolete: true
Attachment #8353501 - Flags: review?(florian)
Summary: Authenticate with NickServ → Handle NickServ messages when authenticating with PASS
Attached patch Alternate patch v4.3 (obsolete) — Splinter Review
*** Original post on bio 1477 as attmnt 1743 at 2012-07-14 15:55:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353503 - Flags: review?(florian)
Comment on attachment 8353502 [details] [diff] [review]
Alternate patch v4

*** Original change on bio 1477 attmnt 1742 at 2012-07-14 15:55:35 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353502 - Attachment is obsolete: true
Attachment #8353502 - Flags: review?(florian)
*** Original post on bio 1477 as attmnt 1744 at 2012-07-14 16:31:00 UTC ***

Updated comments.
Attachment #8353504 - Flags: review?(florian)
Comment on attachment 8353503 [details] [diff] [review]
Alternate patch v4.3

*** Original change on bio 1477 attmnt 1743 at 2012-07-14 16:31:22 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353503 - Attachment is obsolete: true
Attachment #8353503 - Flags: review?(florian)
Comment on attachment 8353504 [details] [diff] [review]
Alternate patch v4.4

*** Original change on bio 1477 attmnt 1744 at 2012-07-14 20:50:49 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353504 - Flags: review?(florian) → review+
*** Original post on bio 1477 at 2012-07-14 21:02:52 UTC ***

https://hg.instantbird.org/instantbird/rev/f0507769cd2b

Thanks for this huge improvement for all identified IRC users who hadn't discovered nickserv killer! (And you just made the libpurple IRC prpl a bit less usable per comparison ;))
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2
*** Original post on bio 1477 at 2012-07-16 16:41:54 UTC ***

This doesn't work for me on irc.mozilla.org (gravel.mozilla.org)

The Nickserv text is
06:34:23 PM - NickServ: This nickname is registered and protected. If it is your
06:34:23 PM - NickServ: nick, type /msg NickServ IDENTIFY password. Otherwise,
06:34:23 PM - NickServ: please choose a different nick.
06:34:23 PM - NickServ: Password accepted - you are now recognized.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
*** Original post on bio 1477 at 2012-07-16 16:46:38 UTC ***

(In reply to comment #19)
> This doesn't work for me on irc.mozilla.org (gravel.mozilla.org)
> 
> The Nickserv text is
> 06:34:23 PM - NickServ: This nickname is registered and protected. If it is
> your
> 06:34:23 PM - NickServ: nick, type /msg NickServ IDENTIFY password. Otherwise,
> 06:34:23 PM - NickServ: please choose a different nick.
> 06:34:23 PM - NickServ: Password accepted - you are now recognized.

Are you on the latest build? Are there errors in the error console? Can you turn on logging and include the actual messages (and times). Thanks.

I don't think this text matches what I see, I wonder if the text was changed slightly for this server?
No longer blocks: 953609
*** Original post on bio 1477 at 2012-07-16 17:11:17 UTC ***

(In reply to comment #20)
> Are you on the latest build? Are there errors in the error console? Can you
> turn on logging and include the actual messages (and times). Thanks.

It's the latest build, no errors or warnings - it's just that you don't test for that particular NickServ text.

> I don't think this text matches what I see, I wonder if the text was changed
> slightly for this server?

irc: :NickServ!services@mozilla.org NOTICE aleth :This nickname is registered and protected.  If it is your                                                                                      
irc: :NickServ!services@mozilla.org NOTICE aleth :nick, type /msg NickServ IDENTIFY password.  Otherwise,                                                                                         
irc: :NickServ!services@mozilla.org NOTICE aleth :please choose a different nick.                

irc: :NickServ!services@mozilla.org NOTICE aleth :If you do not change within one minute, I will change your nick.                  


The other variant (if the IP isn't recognised?) is

irc: :NickServ!services@mozilla.org NOTICE clokep :This nick is owned by someone else.  Please choose another. 
irc: :NickServ!services@mozilla.org NOTICE clokep :(If this is your nick, type /msg NickServ IDENTIFY password.)                                                                       
irc: :NickServ!services@mozilla.org NOTICE clokep :If you do not change within one minute, I will change your nick.
*** Original post on bio 1477 at 2012-07-16 17:19:11 UTC ***

Btw /msg NickServ IDENTIFY is in bold, and password is bold and underlined, so some control codes are being lost here.
Whiteboard: [1.2-wanted]
*** Original post on bio 1477 at 2012-07-20 12:31:48 UTC ***

What's left to do here? Do we just need to add a third version of the nickserv message that we detect?
*** Original post on bio 1477 at 2012-07-20 12:43:38 UTC ***

(In reply to comment #23)
> What's left to do here? Do we just need to add a third version of the nickserv
> message that we detect?

I think so. By trawling their source and by playing with NickServ:
8:42:09 AM - NickServ: Syntax: SET SECURE {ON | OFF}
8:42:09 AM - NickServ: Turns NickServ's security features on or off for your
8:42:09 AM - NickServ: nick. With SECURE set, you must enter your password
8:42:09 AM - NickServ: before you will be recognized as the owner of the nick,
8:42:09 AM - NickServ: regardless of whether your address is on the access
8:42:09 AM - NickServ: list. However, if you are on the access list, NickServ
8:42:09 AM - NickServ: will not auto-kill you regardless of the setting of the
8:42:09 AM - NickServ: KILL option.

And [1] refers to the place in the source where this line is. I'll put up a patch adding this.

[1] http://anope.git.sourceforge.net/git/gitweb.cgi?p=anope/anope;a=blob;f=lang/en_us.l;h=018b004b081eed4bab6a2b2297ebf82b15fda49a;hb=HEAD#l329
Attached patch Followup v1Splinter Review
*** Original post on bio 1477 as attmnt 1752 at 2012-07-21 12:44:00 UTC ***

Handle the extra anope case.
Attachment #8353513 - Flags: review?(bugzilla)
Comment on attachment 8353513 [details] [diff] [review]
Followup v1

*** Original change on bio 1477 attmnt 1752 at 2012-07-21 13:32:25 UTC ***

Thanks for fixing this! :)
Attachment #8353513 - Flags: review?(bugzilla) → review+
Whiteboard: [1.2-wanted] → [1.2-wanted][checkin-needed]
*** Original post on bio 1477 at 2012-07-22 04:24:08 UTC ***

Fixed for real this time? ;) http://hg.instantbird.org/instantbird/rev/5263623bebd1
Whiteboard: [1.2-wanted][checkin-needed] → [1.2-wanted]
*** Original post on bio 1477 at 2012-07-22 04:25:29 UTC ***

I need to supply a comment to close this. :(
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.