Hello folks, I'm a sysadmin at W3C, and we're looking at using ngIRCd to serve our private staff IRC channel, requiring users to authenticate through PAM. Our initial tests with this have been encouraging, and I really like how clean ngIRCd is, but we have hit a couple of corner cases with authentication I wanted to ask about. First, users who have passwords with spaces in them can't connect. If I've done my homework right, this seems to be a limitation of the IRC protocol itself: when clients connect, they provide all their important information (nick, username, password, etc.) on a space-separated line, so the server has no choice but to parse a password with spaces as multiple fields. Do I have that right? I'd be happy to be told I'm wrong about this if it means I can accommodate these users better. Second, ngIRCd allocates 21 bytes for the password buffer, which is too short for some of our users' passwords. As far as I can tell, a maximum password length isn't defined in the RFC or similar, so I'm guessing this limitation is largely historical. I've patched ngIRCd locally to #define CLIENT_PASS_LEN to 64, which seems to be working with no ill effects. Would you be willing to consider a patch like this upstream? We'd love to be running stock ngIRCd without any patches, even one as trivial as this. Thank you, -- Brett Smith
Hi Brett! Am 14.08.2012 um 16:31 schrieb Brett Smith <brett@w3.org>:
First, users who have passwords with spaces in them can't connect. If I've done my homework right, this seems to be a limitation of the IRC protocol itself: when clients connect, they provide all their important information (nick, username, password, etc.) on a space-separated line, so the server has no choice but to parse a password with spaces as multiple fields. Do I have that right? I'd be happy to be told I'm wrong about this if it means I can accommodate these users better.
No: the PASS command takes one parameter, the syntax is "PASS <password>". But in the IRC protocol, one parameter containing spaces can be passed to the server: you have to prefix it with the ":" character. So the following is completely "legal": PASS :a nice password And I don't think that there is a limitation in ngIRCd preventing passwords with spaces from working "per se", but I didn't test … On the other hand, I easily can imagine that this is a client bug, and the client is sending something line "PASS a nice password" which is … wrong. Which client are you using?
Second, ngIRCd allocates 21 bytes for the password buffer, which is too short for some of our users' passwords. As far as I can tell, a maximum password length isn't defined in the RFC or similar, so I'm guessing this limitation is largely historical. I've patched ngIRCd locally to #define CLIENT_PASS_LEN to 64, which seems to be working with no ill effects.
The only "ill" effect is that each client in the network now requires 43 bytes more RAM. This may be a problem or not.
Would you be willing to consider a patch like this upstream? We'd love to be running stock ngIRCd without any patches, even one as trivial as this.
I'm not sure if "just make the buffer bigger" is the right approach: on the one hand, ngIRCd only needs the password while registering the client in the network, and lots of clients not even send any. And by looking at the code I wonder why we save the password in the CLIENT structure at all: I don't think that a local daemon ever gets a password from a remote client!? So I think the best would be to move the password file into the CONN structure, and probably make it dynamic using malloc() when needed … Regards Alex
On 08/15/2012 10:20 AM, Alexander Barton wrote:
Am 14.08.2012 um 16:31 schrieb Brett Smith <brett@w3.org>:
First, users who have passwords with spaces in them can't connect. If I've done my homework right, this seems to be a limitation of the IRC protocol itself: when clients connect, they provide all their important information (nick, username, password, etc.) on a space-separated line, so the server has no choice but to parse a password with spaces as multiple fields. Do I have that right? I'd be happy to be told I'm wrong about this if it means I can accommodate these users better.
No: the PASS command takes one parameter, the syntax is "PASS <password>". But in the IRC protocol, one parameter containing spaces can be passed to the server: you have to prefix it with the ":" character. So the following is completely "legal":
PASS :a nice password
And I don't think that there is a limitation in ngIRCd preventing passwords with spaces from working "per se", but I didn't test …
On the other hand, I easily can imagine that this is a client bug, and the client is sending something line "PASS a nice password" which is … wrong.
Which client are you using?
I was testing with irssi. When I said /connect irc.example.com 6667 "a nice password" it would correctly treat the quoted string as one password, but would write it verbatim in the PASS line, so the IRCD sees it as multiple arguments. Fortunately, at least it's possible to do the right thing by hand: /connect irc.example.com 6667 ":a nice password" works as expected.
Would you be willing to consider a patch like this upstream? We'd love to be running stock ngIRCd without any patches, even one as trivial as this.
I'm not sure if "just make the buffer bigger" is the right approach: on the one hand, ngIRCd only needs the password while registering the client in the network, and lots of clients not even send any.
And by looking at the code I wonder why we save the password in the CLIENT structure at all: I don't think that a local daemon ever gets a password from a remote client!?
So I think the best would be to move the password file into the CONN structure, and probably make it dynamic using malloc() when needed …
My boss has given me the green light to spend some time on this, so hopefully I'll have a patch for you soon—later next week, if things go well. Thanks for all your help. Best regards, -- Brett Smith
On 08/17/2012 10:17 AM, Brett Smith wrote:
On 08/15/2012 10:20 AM, Alexander Barton wrote:
I'm not sure if "just make the buffer bigger" is the right approach: on the one hand, ngIRCd only needs the password while registering the client in the network, and lots of clients not even send any.
And by looking at the code I wonder why we save the password in the CLIENT structure at all: I don't think that a local daemon ever gets a password from a remote client!?
So I think the best would be to move the password file into the CONN structure, and probably make it dynamic using malloc() when needed …
My boss has given me the green light to spend some time on this, so hopefully I'll have a patch for you soon—later next week, if things go well.
I've done this. My work is at <https://gitorious.org/~bretts/ngircd-irc-daemon/bretts-ngircd/commits/move-connection-password>. If it'd be more convenient for me to provide it as a single patch or whatever else, just let me know. I also apologize in advance for my rusty and unstylish C. :) But at least it works correctly with the basic tests I've been running on password support for our new server. If this isn't quite right, though, please don't hesitate to let me know. Thanks again, -- Brett Smith
Hi Brett! Am 23.08.2012 um 21:07 schrieb Brett Smith <brett@w3.org>:
My boss has given me the green light to spend some time on this, so hopefully I'll have a patch for you soon—later next week, if things go well.
I've done this. My work is at <https://gitorious.org/~bretts/ngircd-irc-daemon/bretts-ngircd/commits/move-connection-password>.
Great, thanks a lot!
If it'd be more convenient for me to provide it as a single patch or whatever else, just let me know.
No, not at all: sending or referencing individual patches is the best. And using GIT directly or using "git format-patch" (or something similar) is the easiest workflow for me. Thanks.
I also apologize in advance for my rusty and unstylish C. :)
Hehe, but the one to blame here is … me. I wrote all the crappy "endless" lines full of silly whitespaces that clutter our code today. But ok, it's old code. Today we pretty much adhere to the coding style of the Linux kernel – with some exceptions to make really old compilers happy. And there is a wrapper script for the GNU indent tool, that produces such output: contrib/ngindent. But there is no need to convert all ugly code at one, just let's fix it as we move on … The uglies thing today are all the silly "function( parameter )" whitespaces ;-)
But at least it works correctly with the basic tests I've been running on password support for our new server. If this isn't quite right, though, please don't hesitate to let me know.
I tested it as well and found no errors. Its merged in our master branch now. Regards Alex
Teilnehmer (2)
-
Alexander Barton -
Brett Smith