*** Original post on bio 1573 at 2012-07-04 22:32:00 UTC *** *** Due to BzAPI limitations, the initial description is in comment 1 ***

*** Original post on bio 1573 as attmnt 1731 at 2012-07-04 22:32:00 UTC *** Some IRC servers (some freenode servers) require SASL to connect. This, in turn, requires supporting IRC Client Capabilities Negotiation (CAP). The attached WIP is very rough (it ignores being able to send the password using PASS) and does a few other funky things. (Mostly that it will only send the CAP END command if SASL is used...if you don't send this command the CAP session never ends and we never go past the client authentication stage). We need a way to have things register that they want to stay in the client authentication stage and then a way to know when to send CAP END. I wanted to attach this somewhere and get feedback.

*** Original post on bio 1573 at 2012-07-04 22:34:11 UTC *** The docs: SASL: http://hg.atheme.org/atheme/raw-file/tip/doc/SASL CAP: http://tools.ietf.org/html/draft-mitchell-irc-capabilities-01 http://www.leeh.co.uk/ircd/client-cap.txt http://www.leeh.co.uk/draft-mitchell-irc-capabilities-02.html http://tools.ietf.org/html/draft-baudis-irc-capab-00

*** Original post on bio 1573 at 2012-07-05 21:38:39 UTC *** (In reply to comment #1) > http://www.leeh.co.uk/ircd/client-cap.txt > http://www.leeh.co.uk/draft-mitchell-irc-capabilities-02.html These are 404 :(

Comment on attachment 8353490 [details] [diff] [review] WIP v1 *** Original change on bio 1573 attmnt 1731 at 2012-07-07 22:55:27 UTC *** So, I won't pretend I fully understand the specs... some questions (I know this is mostly just not implemented yet) - When sending CAP LS at the beginning of an IRC session, will we wait with everything else until we receive an error message or a confirmation? Should there be a timeout as well? - 421 also seems a possible error response to CAP LS (eg unreal) - Are we going to have an internal ranking of authentification mechanisms (something like (SASL BLOWFISH > SASL PLAIN) > AUTH > PASS > NickServ?) Is the order we would like to try them in compatible with the order in which we need to send the requisite messages? I see a potential problem if the SASL examples are accurate in that NOTICE AUTH gets all the way to "NOTICE AUTH :*** No Ident response" before the server sends "CAP * LS :sasl" - How do Nickserv and SASL interact? Does it replace NickServ IDENTIFY, or is SASL purely for connecting to the server? - btoa does not work on Unicode strings, is this a problem? workaround here https://developer.mozilla.org/en/DOM/window.btoa - The examples in http://hg.atheme.org/atheme/raw-file/tip/doc/SASL suggests you will run into bug 955002 (bio 1571) for AUTHENTICATE too - "// Some servers send" ...? >+function capMessage(aMessage) { >+ // Some servers send >+ let parameters = aMessage.params.slice(-1)[0].trim().split(" "); >+ aMessage.cap = { >+ subcommand: aMessage.params[aMessage.params.length == 3 ? 1 : 0] >+ }; >+ >+ return parameters.map(function(aParameter) { >+ // Clone the original object. >+ let message = JSON.parse(JSON.stringify(aMessage)); >+ let matches = aParameter.match(capParameterExp); >+ >+ message.cap.modifier = matches[1]; >+ // The names are case insensitive, arbitrarily choose lowercase. >+ message.cap.parameter = matches[2].toLowerCase(); >+ message.cap.disable = matches[1] == "-"; >+ message.cap.sticky = matches[1] == "="; >+ message.cap.ack = matches[1] == "~"; >+ >+ return message; >+ }); Why do you need to clone the aMessage? What do the CAP message handlers need it for? Wouldn't it be cleaner to return "capMessage objects" to which all the needed info is copied? I ask because it seems most of the relevant parsed info is in message.cap.* (so why not drop the .cap and copy across what is missing - it can't be much, right?) I'm not asking this question because I dislike JSON.parse(JSON.stringify()), that is fine with me ;)

*** Original post on bio 1573 at 2012-07-08 23:11:08 UTC *** (In reply to comment #3) > Comment on attachment 8353490 [details] [diff] [review] (bio-attmnt 1731) [details] > WIP v1 > > So, I won't pretend I fully understand the specs... some questions (I know this > is mostly just not implemented yet) I was more interested in the general feel of the code and handling of the CAP messages than of whether or not the SASL code looks reasonable FWIW. > - When sending CAP LS at the beginning of an IRC session, will we wait with > everything else until we receive an error message or a confirmation? Should > there be a timeout as well? When you send CAP LS, if the server understands that it WILL NOT send 001 until CAP END is received (and will respond to the LS request, of course). If a server doesn't understand it...well...it'll be ignored and we'll receive no response to the LS and a 001 after we send NICK and USER. > - 421 also seems a possible error response to CAP LS (eg unreal) Yes, that would be if the server doesn't understand CAP. :) > - Are we going to have an internal ranking of authentification mechanisms > (something like (SASL BLOWFISH > SASL PLAIN) > AUTH > PASS > NickServ?) Is the > order we would like to try them in compatible with the order in which we need > to send the requisite messages? > I see a potential problem if the SASL examples are accurate in that NOTICE AUTH > gets all the way to "NOTICE AUTH :*** No Ident response" before the server > sends "CAP * LS :sasl" Right so...I don't think we have to worry about this until we support Ident. We should attempt to use Blowfish first, and if it's not supported then Plain for SASL. We must always send a password as part of PASS before we do anything. So I guess we'd: 1. Send it as PASS. 2. If the server supports SASL, try SASL Blowfish, if not, try SASL PLAIN. Unfortunately we don't know if PASS worked until after we're fully logged in, so we need to also send the data for SASL. > - How do Nickserv and SASL interact? Does it replace NickServ IDENTIFY, or is > SASL purely for connecting to the server? It replaces NickServ identify, yes. It is NOT for connecting to the server, it is for logging into a particular nick. > - btoa does not work on Unicode strings, is this a problem? workaround here > https://developer.mozilla.org/en/DOM/window.btoa I'm not sure how we would handle this, but good catch. > - The examples in http://hg.atheme.org/atheme/raw-file/tip/doc/SASL suggests > you will run into bug 955002 (bio 1571) for AUTHENTICATE too Why? That would only happen if we try to use the source/nickname I think, but we don't in the handler. > - "// Some servers send" ...? I don't know...I need to look at it again. > Why do you need to clone the aMessage? What do the CAP message handlers need it > for? Wouldn't it be cleaner to return "capMessage objects" to which all the > needed info is copied? I ask because it seems most of the relevant parsed info > is in message.cap.* (so why not drop the .cap and copy across what is missing - > it can't be much, right?) > I'm not asking this question because I dislike JSON.parse(JSON.stringify()), > that is fine with me ;) This is the same way we handle ALL messages (we take the standard IRC message and add fields to it). The reason we have to clone it is because we're returning a map of them, if you don't clone it...then you just end up modifying the same object over and over again and continually referencing it.

*** Original post on bio 1573 at 2012-07-09 16:42:51 UTC *** Thanks for clarifying things. > > - 421 also seems a possible error response to CAP LS (eg unreal) > Yes, that would be if the server doesn't understand CAP. :) OK, so one can treat 421 or 001 as indicating "don't have CAP" if they occur 'early'. > > - The examples in http://hg.atheme.org/atheme/raw-file/tip/doc/SASL suggests > > you will run into bug 955002 (bio 1571) for AUTHENTICATE too > Why? That would only happen if we try to use the source/nickname I think, but > we don't in the handler. True, just something to watch for when/if sending a system message. > > Why do you need to clone the aMessage? What do the CAP message handlers need it > > for? Wouldn't it be cleaner to return "capMessage objects" to which all the > > needed info is copied? I ask because it seems most of the relevant parsed info > > is in message.cap.* (so why not drop the .cap and copy across what is missing - > > it can't be much, right?) > > I'm not asking this question because I dislike JSON.parse(JSON.stringify()), > > that is fine with me ;) > This is the same way we handle ALL messages (we take the standard IRC message > and add fields to it). The reason we have to clone it is because we're > returning a map of them, if you don't clone it...then you just end up modifying > the same object over and over again and continually referencing it. Sure, I understood that part. However, the set of messages returned by capMessage get handled by ircHandlers.handleCAPMessage() and there is no reason I can see why that function should require the "usual" message object rather than a 'specialized' CAP message object. It seems odd to me to split up an incoming message into separate CAP messages, but to then have those still contain the full incoming message with all the differentiation happening in a subobject. You can just make a bunch of new objects I think rather than having to base them on clones of the original message. But if you prefer to do it that way, fair enough...

*** Original post on bio 1573 at 2012-08-17 21:24:02 UTC *** (In reply to comment #5) > Sure, I understood that part. However, the set of messages returned by > capMessage get handled by ircHandlers.handleCAPMessage() and there is no reason > I can see why that function should require the "usual" message object rather > than a 'specialized' CAP message object. It seems odd to me to split up an > incoming message into separate CAP messages, but to then have those still > contain the full incoming message with all the differentiation happening in a > subobject. You can just make a bunch of new objects I think rather than having > to base them on clones of the original message. But if you prefer to do it that > way, fair enough... Right so this is how we handle all messages in the IRC code. I.e. ISUPPORT messages inherit from IRC messages, etc. Note that the only one this really matters on is CTCP, which legitimately needs to inherit from IRC and DCC, which needs to inherit from CTCP. The reason we keep all the information is to keep a full trail of where the message came from, which is necessary in many situations (i.e. DCC you need to know who the request came from, etc.) I agree it's overkill for CAP, but I like symmetry.

*** Original post on bio 1573 as attmnt 2009 at 2012-10-23 23:16:00 UTC *** OK, this is pretty similar to the last WIP, as far as I know. Known issues with it: After authentication I get an "you're already authenticated" messages from NickServ (this is Freenode being really stupid and taking the PASS command but not returning the "already authenticated" response for SASL...wtf). I'm tempting to just handle this message in NickServ and throw it away.

Comment on attachment 8353490 [details] [diff] [review] WIP v1 *** Original change on bio 1573 attmnt 1731 at 2012-10-23 23:16:01 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8353769 [details] [diff] [review] WIP v2 *** Original change on bio 1573 attmnt 2009 at 2012-10-28 21:46:51 UTC *** >> - btoa does not work on Unicode strings, is this a problem? workaround here >> https://developer.mozilla.org/en/DOM/window.btoa >I'm not sure how we would handle this, but good catch. There is now example code for how to handle this on the mdn page :) >After authentication I get an "you're already authenticated" messages from >NickServ (this is Freenode being really stupid and taking the PASS command but >not returning the "already authenticated" response for SASL...wtf). >I'm tempting to just handle this message in NickServ and throw it away. We discussed this a bit on IRC - the fact that we send PASS at all when SASL is available removes some of the benefits of SASL. So the alternative to ignoring the message would be attempting SASL when first connecting to any given server, and only sending PASS if this fails (and remembering that for the future). This is a bit of a hack of course, the question is whether it can be implemented reasonably nicely without being fragile. Flo mentioned that we would have to warn the user if a server which had previously supported SASL suddenly doesn't. The alternative would be adding something to the advanced protocol options I suppose :( I wonder if some of the WARN messages (those that are not failures of the code or completely unexpected responses), as well as a message of the sort "Successfully authentificated the nick with SASL" should be written to the server tab rather than the error console, to make it more easily visible to the user. Otherwise this looks good!

*** Original post on bio 1573 at 2012-10-29 10:35:43 UTC *** One legible protocol option might be "Require encryption for passwords" (this would allow PASS but only over SSL, and SASL)

*** Original post on bio 1573 as attmnt 2021 at 2012-10-29 22:32:00 UTC *** After much discussion on IRC, this is v1 of this patch. It explicitly requires an option to enable SASL in the advanced configuration of the account.

Comment on attachment 8353769 [details] [diff] [review] WIP v2 *** Original change on bio 1573 attmnt 2009 at 2012-10-29 22:32:31 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8353781 [details] [diff] [review] Patch v1 *** Original change on bio 1573 attmnt 2021 at 2012-10-31 21:41:50 UTC *** Looks good - mainly nits and UI stuff. >+options.auth=Authentication method Maybe "Nick authentification method" to clarify to the less-IRC-savvy what exactly is being authentified? (Or even "NickServ..." but I suppose that may not always be correct?) If we can remove a potential source of confusion, we should. >+ // Functions for keeping track of whether the Client Capabilities is done. >+ _caps: [], >+ _capTimeout: null, >+ addCAP: function(aCAP) { This comment could be clearer ;) > "451": function(aMessage) { // ERR_NOTREGISTERED > // :You have not registered >+ // If the server doesn't understand CAP it might return this error. >+ if (aMessage.params[0] == "CAP") >+ return true; > // TODO > return false; > }, Shouldn't we write a system message somewhere ('Server doesn't support SASL') to tell the user to switch to PASS auth? We might need a similar handler for 421. Do we also need to check (somewhere) for the case where the user wants to authenticate with SASL, and the server has CAP, but no SASL? >+const capParameterExp = /^([\-=~])?((?:(?:[A-Z][A-Z0-9\-]*\.)*[A-Z][A-Z0-9\-]*\/)?[A-Z][A-Z0-9\-]*)$/i; >+function capMessage(aMessage) { Maybe call this parseCapMessage ? >+ // Some servers send ??? I still don't know what this comment means ;) >+ "CAP": function(aMessage) { >+ // [* | <nick>] <subcommand> :<parameters> >+ let messages = capMessage(aMessage); >+ ERROR(JSON.stringify(messages)); Was this ERROR for debugging purposes? >+ // An authentication identity, authorization identity and password are >+ // used, separated by null. >+ let data = [this._requestedNickname, this._requestedNickname, >+ this.imAccount.password].join("\0"); >+ this.sendMessage("AUTHENTICATE", btoa(data)); I know IRC nicks can't be unicode, are you sure about passwords? >+ "903": function(aMessage) { >+ // Authentication was successful. >+ return true; >+ }, Some form of user feedback would be nice (server tab). If the user gets to select the auth method, they should also be able to find out whether it succeeded or failed without using the error console. >+ "904": function(aMessage) { >+ // AUTHENTICATE message failed. >+ // Only PLAIN is currently supported, so fail here. >+ WARN("The server does not support SASL PLAIN authentication."); Server tab? >+ "905": function(aMessage) { >+ ERROR("Authentication failed."); Server tab ("Password incorrect", or what else could it be?)

*** Original post on bio 1573 as attmnt 2028 at 2012-11-02 02:14:00 UTC *** > >+function capMessage(aMessage) { > Maybe call this parseCapMessage ? None of the other methods that parse a message do this, why would we name this one differently? > >+ // Some servers send ??? > I still don't know what this comment means ;) Me neither. :) > >+ "903": function(aMessage) { > >+ "904": function(aMessage) { > >+ "905": function(aMessage) { I disagree with adding any of these to the server tab. The server tab already has too much stuff going to it. IRC doesn't fully require the password and they should get prompted by NickServ, we don't need to add to that noise. The error console is good enough. Note that this does things a bit differently. We attempt SASL, if this doesn't work we'll try the IDENTIFY command once logged on. If that doesn't work we attempt "NICKSERV IDENTIFY", if that doesn't work...oh well, the user needs to authenticate themselves. This works on Freenode and moznet (it works on Freenode with and without SASL, by the way). The NickServ messages still come up, but we catch those already. I have a good feeling about this one! :)

Comment on attachment 8353781 [details] [diff] [review] Patch v1 *** Original change on bio 1573 attmnt 2021 at 2012-11-02 02:14:07 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 1573 as attmnt 2030 at 2012-11-02 10:49:00 UTC *** Fixes bitrot from bug 955140 (bio 1712). Also marks the user as authenticated when NickServ responds. This patch will probably not check in cleanly without the patch in bug 955126 (bio 1698).

Comment on attachment 8353788 [details] [diff] [review] Patch v2 *** Original change on bio 1573 attmnt 2028 at 2012-11-02 10:49:38 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 1573 as attmnt 2031 at 2012-11-02 10:57:00 UTC *** Remove a debug statement in ircCAP.js.

Comment on attachment 8353790 [details] [diff] [review] Patch v2.1 *** Original change on bio 1573 attmnt 2030 at 2012-11-02 10:57:09 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 1573 at 2012-11-02 20:56:43 UTC *** Comment on attachment 8353791 [details] [diff] [review] (bio-attmnt 2031) Patch v2.2 It's great you managed to get rid of - PASS - the auth account option - and with it the need to write stuff to the server tab etc :) Nits only remain. >+ // Whether the user has successfully logged into the server with their nick. >+ isAuthenticated: false, This comment could be misleading, as you can log in without authenticating your nick. Something like "Whether the user has successfully authenticated their nick with NickServ" Should we add a flag "allow plaintext transmission of password" to the IRC options (similar to XMPP)? (Not in this patch of course, as it is isomorphic to SSL until we have SASL Blowfish) > "421": function(aMessage) { // ERR_UNKNOWNCOMMAND > // <command> :Unknown command >- // TODO This shouldn't occur >+ // IDENTIFY failed, try NICKSERV IDENTIFY. >+ if (aMessage.params[1] == "IDENTIFY" && !this.isAuthenticated) { Should check for this.imAccount.password too, just in case it wasn't our code that sent IDENTIFY? Unlikely, but... >+ this.sendMessage("NICKSERV", ["IDENTIFY", this.imAccount.password], >+ "NICKSERV IDENTIFY <password not logged>"); >+ } We should not return false if aMessage.params[1] == "NICKSERV", since we sent it. I wonder if these two possible 421 responses shouldn't be handled in ircServices?

Comment on attachment 8353791 [details] [diff] [review] Patch v2.2 *** Original change on bio 1573 attmnt 2031 at 2012-11-02 20:57:38 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 1573 as attmnt 2034 at 2012-11-02 21:35:00 UTC *** (In reply to comment #15) > >+ // Whether the user has successfully logged into the server with their nick. > >+ isAuthenticated: false, > This comment could be misleading, as you can log in without authenticating your > nick. Something like "Whether the user has successfully authenticated their > nick with NickServ" I changed it, got the r+ on that over IRC. ;) > Should we add a flag "allow plaintext transmission of password" to the IRC > options (similar to XMPP)? (Not in this patch of course, as it is isomorphic to > SSL until we have SASL Blowfish) Possibly, I need wnayes' Blowfish code to land first. > > "421": function(aMessage) { // ERR_UNKNOWNCOMMAND [...] > >+ if (aMessage.params[1] == "IDENTIFY" && !this.isAuthenticated) { > Should check for this.imAccount.password too, just in case it wasn't our code > that sent IDENTIFY? Unlikely, but... It should, good catch! > I wonder if these two possible 421 responses shouldn't be handled in > ircServices? The 421 & 001 handlers were moved to ircServices, I think this makes it a lot clearer that they go together.

Comment on attachment 8353791 [details] [diff] [review] Patch v2.2 *** Original change on bio 1573 attmnt 2031 at 2012-11-02 21:35:37 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 1573 as attmnt 2035 at 2012-11-02 22:13:00 UTC *** Some review comments received on IRC.

Comment on attachment 8353794 [details] [diff] [review] Patch v3 *** Original change on bio 1573 attmnt 2034 at 2012-11-02 22:13:14 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment on attachment 8353795 [details] [diff] [review] Patch v4 *** Original change on bio 1573 attmnt 2035 at 2012-11-02 22:17:48 UTC *** Looking forward to having this in the nightlies! :) Thanks for working on this!

*** Original post on bio 1573 at 2012-11-03 04:24:05 UTC *** Checked in as http://hg.instantbird.org/instantbird/rev/e841a2bb2254