srvconnector fails for xmpp-client service on OS X

[jack]

SRVConnector passes the name of the service to reactor.connectTCP instead of a port numbers. This causes the port to be looked up in /etc/services, which fails because OS X is still using the deprecated 'jabber-client' name instead of 'xmpp-client'.

This means that any connections to localhost done with twisted.words.protocols.jabber.client.* just fail.

Is there a clean way to fix this in Twisted? Or is my only option to create a custom XMPPConnector that just substitutes the port number?

[jack]

This code provides a work around:

class XMPPConnector(SRVConnector):
    """Connector that replaces xmpp-client with port 5222 if SRV lookup
    fails.

    This is needed to work around buggy OSes that still use jabber-client
    in /etc/services (OS X 10.5.x, I'm looking at you).
    """

    def pickServer(self):
	host, port = SRVConnector.pickServer(self)
	if port == 'xmpp-client':
	    port = 5222
	return host, port

[exarkun]

Hm. I don't think I quite understand. Is there an XMPPConnector in Twisted? Or is this about the behavior you get when you use SRVConnector with the service 'xmpp-client'?

[jack]

Wokkel uses SRVConnector, and it's behavior is that it returns 'xmpp-client' as the port for reactor.connectTCP(). This fails on OS X because OS X /etc/services has 'jabber-client' instead of 'xmpp-client'.

XMPPConnector is a subclass of SRVConnector that I created to work around this bug. I'm not sure if this should be moved into twisted.words.protocols.jabber or if there is a better fix in the bowels of SRVConnector itself.

[exarkun]

Ah, okay, thanks for the explanation. I'm not sure if it makes sense to have XMPPConnector in twisted.words.protocols.jabber - could be, I'll leave that up to someone else. I don't think there's anything to be done to SRVConnector - it's doing what it's supposed to do (except it probably should use a non-blocking API for service -> port translation, but that's another matter). It's not SRVConnector's job to know about the mapping, that's the platform's job.

[jack]

In general I agree with you, but if a major platform is broken, it seems wrong to leave those users out in the cold, or make every application developer rediscover how to work around the problem. A simple optional parameter to SRVConnector to take the port number if a mapping isn't otherwise found seems to hurt nothing and would at least make the workaround far simpler.

[ralphm]

A similar implementation has been in the XMPP client example for about two years now. At that time the service was also missing from FreeBSD's services file. I filed a bug report for FreeBSD and it has since been fixed.

I am a bit ambivalent about this issue. In principle, it is IANA, not the platforms, that decide on the mapping between (SRV) service names and the ports they are assigned. It is very common that services files in platforms are not up to date, and I don't even know how Windows platforms deal with this. There is a likelyhood of 0 that these mappings will change at any point in the future, and as such I don't think it hurts to provide a way to pass a default port in case the service name has not been (locally) found and there is no SRV record either.

[exarkun]

What is the purpose of ever using the platform-service mapping if the correct value is known and present in the code? Should we just forget about /etc/services completely?

[ralphm]

Looking over requisites for moving over wokkel.client to Twisted Words, this came back to my radar.

Replying to exarkun:

What is the purpose of ever using the platform-service mapping if the correct value is known and present in the code? Should we just forget about /etc/services completely?

That is a good question indeed.

For OS X 10.7 (Lion), that file still does not list xmpp-client or xmpp-server as services. The file appears to be unchanged since imported from the FreeBSD userland in 2002.

I think we can go two ways here:

1. Introduce a new keyword argument to SRVConnect.__init__ called fallbackPort. It would hold the port number to use when SRV records are not found for the named service at the specified domain.
2. Augment or replace the call to socket.getservbyname in twisted.internet.tcp.Connector. As mentioned in comment:4 this call is potentially blocking (it could even hit NIS, for example). We could carry our own list for  IANA registered ports.

I tend to favor option 1.

Opinions?

[exarkun]

By keyword argument I think you mean optional argument?

I was quite confused by this issue until I realized that SRVConnector is not XMPPConnector. XMPPConnector knows that the port to use is 5222 if there is no contradictory SRV record. Why don't we leave SRVConnector alone and just teach XMPPConnector to use port 5222 if there is no SRV record?

[ralphm]

Having read up on this some more, I feel obliged to provide some background information.

The section titled 'The Port number' in  RFC defining SRV RRs reads:

Currently, the translation from service name to port number happens at the client, often using a file such as /etc/services.

Moving this information to the DNS makes it less necessary to update these files on every single computer of the net every time a new service is added, and makes it possible to move standard services out of the "root-only" port range on unix.

It further specifies the procedure for resolving such records. Failing that it recommends retrieving the address record for the target (A or AAAA RR) and connecting to the service there. Twisted's implementation of SRVConnector does just that. In pickServer, when there are no records found, it returns (host, port), with port the service name. This is then passed on to connectTCP, which indirectly calls socket.getservbyname to resolve the service name into a port number before attempting to connect to the target in host.

As long as socket.getservbyname returns a port number, this works great. It turns out that since an update to the  IANA Procedures for the Management of the Service Name and Transport Protocol Port Number Registry, it is suggested newly designed protocols make use of SRV records, without requiring a dedicated port number.

XMPP however, does have registered port numbers for the service names xmpp-client and xmpp-server. Other protocols (LDAP comes to mind) do the same. To work around outdated /etc/services files, I think that for Twisted implementations of such protocols it is ok to have to know about the default port number.

Replying to exarkun:

I was quite confused by this issue until I realized that SRVConnector is not XMPPConnector. XMPPConnector knows that the port to use is 5222 if there is no contradictory SRV record. Why don't we leave SRVConnector alone and just teach XMPPConnector to use port 5222 if there is no SRV record?

The only purpose of XMPPConnector, as in the example from comment:1, is to provide a default port number when no SRV records are found for the target domain and service. It doesn't do anything else. Wokkel currently has one for clients and one for outgoing server connections. When initiating a connection it instantiates the connector and calls connect on it. That doesn't seem proper to me.

Given all of the above, I think my earlier option 2 isn't going to work at all, and I think there should be a way to provide an optional default port number to SRVConnector. Not just for XMPP, but for other services as well.

A future development could be to extend twisted.internet.endpoints.clientFromString to be able to work with SRVConnector. It could reuse the port component as the default port to connect to, if SRV lookup fails.

[ralphm]

(In [33979]) Branching to 'srvconnect-default-port-3456'

[ralphm]

This should do it. Please review.

[dreid]

If I've understood the problem correctly this seems like a reasonable solution. Hopefully a little movement will cause someone more knowledgable to look at this branch for further review.

A couple of nits.

• defaultPort probably shouldn't be public.
• @type default: C{int} should be @type defaultPort: C{int}
• pickServer should document it's return types.
• The pickServer docstring could be more clear if it specified the possible return values in the order of priority.
• The patch has a lot of unrelated changes caused by reformatting docstrings of functions not touched to make the changes.
• The SRVConnector class docstring is better but still incomplete. I don't think it is necessary to make it 100% complete but it might be valuable to at least document the variables that affect the behavior of pickServer.
• I assume pickServer is not a part of the public API? Perhaps now would be a good time to deprecate it and move it's behavior to a private function.

[ralphm]

I made a start with addressing the review comments.

Replying to dreid:

If I've understood the problem correctly this seems like a reasonable solution. Hopefully a little movement will cause someone more knowledgable to look at this branch for further review. A couple of nits. * defaultPort probably shouldn't be public. * @type default: C{int} should be @type defaultPort: C{int}

Fixed.

* pickServer should document it's return types. * The pickServer docstring could be more clear if it specified the possible return values in the order of priority. * The SRVConnector class docstring is better but still incomplete. I don't think it is necessary to make it 100% complete but it might be valuable to at least document the variables that affect the behavior of pickServer.

It turns out that the current implementation is at least partially incorrect. E.g. the random value is not actually used. This should probably done in another ticket.

* The patch has a lot of unrelated changes caused by reformatting docstrings of functions not touched to make the changes.

I didn't consider that cleaning of the formatting of these docstrings was superfluous. I'll revert those changes.

* I assume pickServer is not a part of the public API? Perhaps now would be a good time to deprecate it and move it's behavior to a private function.

I am not sure if this should be private. Although the RFC recommends a particular algorithm, it is not a MUST.

[ralphm]

I reverted the unrelated comment and spacing changes.

As far as the incorrect workings of picking servers in the case of more than one SRV record, its scope is a bit larger than just pickServer. The SRV specification also requires iterating over each possible server until a connection can be established or they are exhausted. Ticket #4859 seems to cover that and probably requires a different interface to SRV lookups altogether.

Please review.

[dreid]

OK, yeah it seems like at when #4859 is done SRVConnector itself can be deprecated in it's entirety. So I wouldn't worry about deprecating pickServer specifically.

• pickServer should use @returns to document it's return value
• port = self._defaultPort or self.service will cause self._defaultPort to always be used. The name defaultPort implies that it would be the default if no other port is specified. Perhaps preferredPortNumber, you want it to raise the question what is this preferred to and also you want to be distinct from strports descriptions.

[ralphm]

After some sprint discussion, we decided to first attempt connecting with the service name as the port. If that service name is found to be unknown, a connection will be attempted to the default port, if given. As such the whole implementation was done from scratch.

I also adjusted the XMPP client example to match the new situation.

Please review, maybe using log:branches/srvconnect-default-port-3456.

[therve]

1.

+        if not self._defaultPort:
+            return failure

Instead of doing that, I would only add _ebServiceUnknown if defaultPort is defined.

2.

+        If there is no default port, connecting fails.

It's missing a mention of the error: "If there is no default port and that service is not defined..."

3.

+        connector = SRVConnector(reactor, 'xmpp-client', client_jid.host, f)

It looks like it's missing the default port argument?

Thanks!

[ralphm]

(In [34865]) Add _ebServiceUnkown only if _defaultPort is set, fix example, docs.

Re: #3456.

[ralphm]

Addressed all review points by therve. Please review.

[therve]

Looks good, please merge!

[ralphm]

(In [34878]) Merge srvconnect-default-port-3456: Accept a fallback port to SRVConnector.

Author: ralphm. Reviewer: dreid, therve. Fixes: #3456.

SRVConnector will resolve DNS SRV records to determine which host and port to connect to, given a domain and service name. If there are no SRV records and the service name is unknown to the local system (e.g. not present in /etc/services, like xmpp-client in current versions of OS X), the new defaultPort argument is used as the fallback port. It should hold the port number registered for the service name by IANA.