Bug 14202

Can anyone confirm this patch validity?
It seems invalid to me, but I'm not very familiar with this part.

It looks legitimate to me, but it really can't be accepted until someone sets up a server with XDM-AUTHORIZATION-1 turned on and tests it thoroughly.  I'll maybe get to it some day.  I'm a little confused, as I know XDM-AUTHORIZATION-1 worked when I wrote it, but we seem to have lost all pre-git history for some reason, which makes me sad, and makes it hard to track down the bug.

Reading page 21 of http://www.x.org/docs/XDMCP/xdmcp.pdf, it seems that the patch is correct actually.

I just wonder if we have another way than calling auth_info twice.

Any hint? Is that possible or the patch is actually good in this regard? :-)

Created attachment 24646 [details] [review]
patch to clean up an auth case

Gah.  Bugzilla drives me mad.

OK, the patch I just attached eliminated the extra call to getsockname(), which is I assume what you were worried about.  It does cause the code to always call getsockname(), but I think that's fine.

Bart, your patch is meant to be put on top of EL's one?

Because otherwise you don't solve the issue, we need to passe local socket information in auth information according to XDM-AUTHORIZATION-1 specs. IIUC.

Created attachment 24681 [details] [review]
Another try at getting the auth data right

OK, I'm not getting it.  Is this what is needed?  If not, please point me at some documentation or pseudocode or something, or just fix it properly yourself. :-)

Thanks for your patch Bart.

I think it's still bad, but I may be wrong, I'm going to explain what I understand in the current code:

1. We getpeername() the socket to get the socket peer address. If everything is OK, we continue.
2. We pass that peer address to get_authptr() which is responsible to find auth data.
3. We pass that authptr and this peer address to compute_auth() which compute again some authentification stuff.

So far so good.
But, in the auth information, we put the *peer* address.

The problem is that XDM-AUTHORIZATION-1 (see the document I pointed earlier) wants the *local* address (not the peer one).

So what the initial patch in this bug report do is the following:
1. Same as above.
2. Same as above.
3. Same as above.
4. If 3. fails, get the *local* address (rather than the peer one) and redo 2. and 3.

The problem I see with this solution is that it probably make things work, but it's a bit heavy.

And now, what I wonder it is what we should put ? socket's peer address or socket's local address ? I know that XDM-AUTHORIZATION-1 wants local's address which IMHO makes sense.
Puting peer's address might be required by another authentification method (like MIT-MAGIC-COOKIE), I don't know what should be put.


Finally, I took a look at libx11, since this has been sucked out from ConnDis.c.
And it has a special case in GetAuthorization()  to handle XDM-AUTHORIZATION-1 and in this case get the *local* address.

So I really don't know how to patch this correctly because it looks like a pile of spaghettis to me right now.

Created attachment 24745 [details] [review]
third try at auth patch

OK, after a phone call with Keithp I believe I now understand.  The way it's supposed to work is:

  (1) the client uses libXau to look up the authentication method in the .Xauthority file based on the *peer* address.
  (2) if the authentication type is Xdmcp, the client uses libXdmcp to wrap up the *local* address in authentication packet.

I *think* the attached, completely untested patch does this thing, handling the special case of UNIX-domain sockets on weird machines correctly and not incurring any extra system calls.

As usual, feedback welcome.

Bart,

Your patch seems ok to me, at least in theory. I've tested it and saw no regression with MIT-MAGIC-COOKIE on Unix socket.

I can't test it with XDM-AUTHORIZATION-1, but again, the implementation seems correct to me.

Would like to merge it yourself?

IMHO it would be a mistake to merge it until someone tests against XDM-AUTHENTICATION-1. I'll bug the original reporter to give it a try, because I'm lazy.  (Why do we not have an MORE_INFO_NEEDED status for bugs here?)

there's a NEEDINFO keyword.

Patch pushed and submitter confirmed it works.