PAM module uses bogus username

[smoku]

Rather than just using the JID of the incoming user, authreg_pam appends the server realm to it, so you end up with PAM calls of the form <user>@<domain>. These are likely to be invalid in almost any common PAM configuration. The attached patch disables the behaviour, although before removing it, it would be interesting to find out if it ever worked properly, or if there is anyone relying on the current behaviour.

[smoku]

Patch by Simon Wilkinson <simon@…>

[smoku]

This change was submitted by Juha Heinanen and integrated in r74.

My understanding of this patch is as follows:

• if you want to use default system password database - you leave the realm string empty
• if you do enter it, you want it to be appended to the username, to be able to authenticate different realms in one PAM database

Could you please elaborate more why this change should be reverted? I do not use PAM auth and cannot decide by myself how it should work.

[smoku]

Please reopen the ticket if you think it's valid.

[sxw]

I'm not sure Trac will let me change the resolution - but here's what the problem is.

Basically, it doesn't look like a NULL realm can be set - which is what is required for this code to work correctly at sites where they wish to authenticate against their default system password database.

The problem is right at the heart of the way that configuration file parsing is performed. When dealing with attributes, an empty string is considered to be the same as NULL. Then, when attributes are retrieved, j_attrs returns 'NULL' for both an attribute with a NULL value, and an attribute which has never been defined.

There are a few ways that this could be fixed

• pstrdupx has strange behaviour when asked to strdup a 0 length string. In all other cases, it's length argument means 'length of the string, excluding the trailing NULL. When passed a 0 length, however it doesn't create a zero length string (that is a pointer to a 1 byte block containing NULL), it just returns NULL. This is arguably the core problem - but pstrdupx has a lot of callers and verifying that the safety of this behaviour change could be tricky
• Special case the realm checks - so that something other than j_attr is used to retrieve the value, and so the difference between an empty, and a not-provided string can be determined.
• Change the behaviour of the configuration file parser, so that it creates 0 length, rather than NULL strings just for configuration files.

I think the last one is the safest option. Comments?

[smoku]

Let me rephrase:

• wether you set an "" value of the attribute or do not set it at all, the config get results in NULL returned
• you need NULL as a realm returned to authenticate against default system password DB

Ergo:

• you need to set "" realm or not set it at all to auth against system password DB

Isn't this how it should work?

[sxw]

What you're outlining is the way it _should_ work - currently there's no way of setting a or empty realm that makes it past the config file parser

Currently - when you set an empty value, or do not set it all, the configuration parser sets the realm to be the same as the <id>. So, for instance ... <id>inf.ed.ac.uk</id> - results in the realm parameter of the host structure being set to "inf.ed.ac.uk". Relevant code is in c2s/main.c ...

realm = j_attr((const char **) elem->attrs[i], "realm"); [ ... ] host->realm = (realm != NULL) ? realm : pstrdup(xhash_pool(c2s->hosts), id);

As you can't (currently) set the realm attribute to an empty string due to the way that attribute parsing works, there is currently no way of setting an empty realm. Gettting rid of the check will break everyone who's not specifiying the realm, and relying on the current behaviour. I think a fix needs to be able to differentiate between when the realm isn't specified, and when it's specified as an empty string.

I have a fix for this problem which I can attach, if that would make things clearer.

Simon.

[smoku]

Ok. Now I get it.

Please do not hesitate to attach any patches. :-)

[sxw]

I've attached a patch.

However, setting realm != id does throw up some issues with the Cyrus SASL implementation. I haven't looked at the GSASL code. The problem with Cyrus is that it uses realm and id interchangably. In particular, the authzid gets set to user@REALM (rather than user@ID) which obviously only works when REALM == ID.

Providing this patch is acceptable, I'll open another bug for the Cyrus SASL issues, and fixes.

[sxw]

Updated fix

[smoku]

(In [244]) Merged nullstring config option by Simon Wilkinson. Fixes #17