strports.endpoint

[glyph]

Once #1442 is merged, we will have a more abstract way of loading endpoints. But we will still have no way for users to specify them, or provide their own.

We should implement the syntax currently in twisted.application.strports in terms of the client and server endpoints APIs.

[glyph]

(In [29147]) Merge endpoints-1442-5: a high level connection and listening API

Author: rwall, dreid, glyph

Reviewer: radix, exarkun, glyph, jknight

Fixes: #1442

Refs: #4470 Refs: #4471 Refs: #4472 Refs: #4473 Refs: #3204

Added new "endpoint" interfaces in twisted.internet.interfaces, which abstractly describe stream transport endpoints which can be listened on or connected to. Implementations for TCP and SSL clients and servers are present in twisted.internet.endpoints. Notably, client endpoints' connect() methods return cancellable Deferreds, so code written to use them can bypass the awkward "ClientFactory.clientConnectionFailed" and "Connector.stopConnecting" methods, and handle errbacks from or cancel the returned deferred, respectively.

[exarkun]

strports.endpoint isn't really implementable the way things are now. Client and server endpoints are implemented as different objects, and strports.endpoint can probably only return one of them.

[glyph]

This is actually going to need to be 2 APIs, maybe strports.serverEndpoint and strports.clientEndpoint, because clients and servers have slightly different expectations as to what their arguments are going to be and therefore different string formats, not to mention different interfaces (IServerStreamEndpoint and IClientStreamEndpoint).

[exarkun]

(In [29258]) Branching to 'strports-endpoints-4473'

[glyph]

The  build results look pretty bad. I think the problem is that Context is a function on older versions of PyOpenSSL?

[exarkun]

(In [29274]) Compatible with older verions of pyOpenSSL

refs #4473

[exarkun]

Should be fixed now. Possibly strports could use some documentation covering its escaping rules.

[glyph]

Looks pretty good.

1. There are a bunch of top-level suites and classes separated by 2 or 1 blank lines. Might want to make it 3, especially for the classes (in the test suite).
2. The docstring should introduce (and emphasize) the endpoint parser above the other crummy stuff in strports. (I'd like to deprecate that stuff soon.)
3. serverEndpoint and clientEndpoint both scan a bit more naturally to me, because in this case, 'server' and 'client' are adjectives, and 'endpoint' is the noun they modify. (I know those aren't super great either, but If I can think of a name that isn't overly cutesy and gross, I'll file a separate ticket for changing it. I don't think we should block on that though.)

Adjust to your satisfaction and land.

[glyph]

Darn it. I started writing some documentation, and now I've got all kinds of other ideas about this branch.

My original thought was that these APIs could be named twisted.internet.endpoints.client and twisted.internet.endpoints.server, but that was dismissed as too cute. Now I think it might actually be a better choice.

Mainly, I'm noticing that strports was placed into twisted.application because the main event there is twisted.application.strports.service. However, it makes much more sense to put the API for creating an endpoint into twisted.internet; there's no reason to pull in twisted.application just to create an object purely defined in twisted.internet.

Not to belabor the obvious here, but there's a fairly compatible refactoring here, too: strports.service could be implemented in terms of a trivial EndpointService class that calls listen() on startup and shutdown, rather than using the mess of dynamically-generated classes from twisted.application.internet. strports.listen could also be implemented to just call server(reactor, description).listen(). These don't really have to be done for this branch, but (A) they're trivial and (B) they would allow us to move the private implementation details into the appropriate twisted.internet package, and not call them across a package boundary. (I don't think we want to expose anything like 'parse' in a new, good package, anyway.)

I still stand by the fact that this is OK to merge, but please take this suggestion into consideration.

[glyph]

(In [29287]) Well, it's something. As with much Twisted documentation it's hard to come up with a sufficient amount of context. Also, there isn't really a lot you can do with an endpoint by itself...

I've documented the strports stuff under the names currently used in the #4473 branch, but this will obviously have to change if they do.

refs #4473

refs #4478

[exarkun]

Darn it. I started writing some documentation, and now I've got all kinds of other ideas about this branch.

This always happens when writing documentation. It's one reason I think it's important that we write documentation closer to the time when we do implementations.

Now I think it might actually be a better choice.

I think it makes some sense. One slightly annoying thing is that it means moving all of the strports parsing code into twisted.internet so that we don't have a circular dependency between twisted.internet and twisted.application. (Or we could move it somewhere else, twisted.python, but that doesn't feel quite right either.)

Anyway, I'm not going to get to this soon. How about we roll back endpoints until the 10.1 release is done, and then finish them up right for 10.2?

[glyph]

(In [29296]) Move endpoint-parsing functions to endpoints module, so they'll be in twisted.internet, as discussed on the ticket.

refs #4473

[glyph]

There's a fix for #2730, or maybe #2061, in this branch. My understanding there is fuzzy. #2730 doesn't look like a duplicate to me, it looks like it's calling out the documentation issue separately from the functional issue.

[glyph]

(In [30070]) Branching to 'strports-endpoints-4473-2'

[glyph]

 Build Results

[glyph]

The quoted portions are from a review that exarkun did offline, while we were stuck someplace without wifi.

There should be an API that does what is done to compute escapedPEMPathName at the top of twisted/internet/test/test_endpoints.py

Done, with tests.

assertEquals is preferred over assertEqual

Done.

StreamServerEndpointService class doctoring should have a @since.

Done.

Maybe StreamServerEndpointService.privilegedStartService should call the base method.

I added it, but the base method has no behavior (it's a 'pass'). What is one to test in a situation like this? As it is, the line is covered, but there are no new tests for it, because it does nothing. But the point of calling it would be so that the overridden method could possibly do something in the future... but we can't rely on that doing anything useful.

The news fragment talks about "client" and "server" but the APIs aren't named that anymore. Likewise some test docstrings talk about "client" and "server".

I think I've fixed this everywhere. Let me know if you spot anywhere.

Some version numbers - in @since docs and in deprecations - might need to become 10.2.

Done, I think.

There needs to be a ticket describing the problems with the SSL support in these APIs (the privateKey/certKey names, the issues with specifying which certificates to trust, the missing additional SSL parameters) and describing the fixes we discussed (all that time ago - uh, what were those fixes anyway?).

The reason that there were problems with the SSL support is that OpenSSLCertificateOptions takes OpenSSL objects to its constructor, not twisted.internet.ssl objects. We should have a version that takes twisted.internet.ssl objects ( #4655 ). However, it's perfectly easy to get from a twisted.internet.ssl object to an OpenSSL object, by accessing some attributes. Even though that's a little gross (those attributes should have been private in the first place, but oh well, too late!), that's what I'm doing for now.

Plus, we can construct twisted.internet.ssl objects from OpenSSL objects by using their constructors directly rather than fromXXX classmethods, and once we have those, it's relatively easy to write tests, because we get nice operations like == and != being defined. I've made the tests verify more stuff about the SSL objects returned, by looking at the factory rather than at the almost-completely-opaque Context.

The thing in the branch should now be functional enough to use for just about any client. The only part it doesn't do is automatically do certificate validation for you based on the hostname, but that starts getting into more serious UI anyway.

I still dislike TCP being the default scheme (ie, the handler for "80"). I think that logic belongs at a different layer.

You already convinced me. I like brevity, but the presence of 'tcp:' in all examples will indicate to the reader of example documentation that something else might come before that colon, which is a useful thing to be curious about.

So, the idea was that this behavior was deprecated. It's there for compatibility with any scripts that might be specifying port numbers to existing strports-using code. There were tests for the deprecation of the parameter that let you select a default already, but I've added some code to deprecate passing it at all, and I fixed the documentation to not mention the deprecated syntax. It also raises now, if you call the new API without passing an endpoint type explicitly in the description.

Docstrings for the parser functions in endpoints.py, maybe

Done, possibly to a comical degree.

[exarkun]

• The deprecation tests fail in an SVN checkout. Version("twisted") is somewhat unpleasantly different than Version("Twisted"). :/ The Fedora builders caught this because they still use svn to get Twisted, but none of the builders converted to bzr did because twisted.python.versions has no bzr support.
• twisted.application.internet.StreamServerEndpointService
  • stopService doesn't set running to False
  • factory and endpoint attributes are undocumented
• twisted.application.strports
  • How careful do we want to be about the deprecation of the default parameter to service? service(description, factory, None, reactor) won't currently emit a deprecation warning, but if we ever delete default it will, of course, break.
• At the top of twisted.application.test.test_internet there's a zope.interface import after the Twisted imports.
• For twisted.internet.endpoints.serverFromString and clientFromString I would like to see a complete list of supported keyword arguments somewhere. Doesn't need to be in the docstring for those functions, but it should probably at least be linked from there. Addressing this in #4478 is probably fine, so if you just want to add a comment there about it... Actually, this is basically end-user facing documentation, so having it in a howto (carefully separated from more technical information) makes more sense.
• The caCertsDir keyword argument to twisted.internet.endpoints._parseClientSSL isn't documented (whereas certKey and privateKey are).
• There are some issues with how this argument is implemented:
  • There's no guarantee all of the PEMs in the directory will be readable. For example, the ipop3d package in Ubuntu apparently drops a file in /etc/certs/ssl which is only readable by root. This means clientFromString can't reliably use /etc/certs/ssl on Debian or Ubuntu. There also may be symlinks created by c_rehash to files which have since been deleted, resulting in dead links.
  • You can't add the same certificate to a Context multiple times. You'll get an OpenSSL.SSL.Error from X509_STORE_add_cert saying cert already in hash table. c_rehash automatically creates a duplicate of every certificate in a directory, so you won't be able to share a directory between clientFromString and, say, curl.
  • There's no guarantee that every file in the directory will be a certificate, or even that every entry in the directory will be a file. In particular, twisted.internet.test.test_endpoints.SSLClientStringTests.test_ssl fails in an SVN checkout because of the .svn directory it encounters.
  • I feel like there's still something mis-implemented in OpenSSLCertificateOptions' support of caCerts, but the trivial case works so for now maybe I'll try to forget about that, and if people report problems then we can just fix it in OpenSSLCertificateOptions (the trivial case works, at least, once I work around the above mentioned problems).
  • Oh, but, right, all those other problems. Uh. I was expecting that the caCertsDir option would be supported via Context.load_verify_locations(None, caCertsDir). Did you avoid this because it means not using CertificateOptions? That API avoids largely these problems (although it requires the use of `c_rehash) because it only reads certificates it needs (which you still might not have permission on, but that failure makes more sense, since it's a file you actually need, instead of a file that just happens to be there) and the hashing avoids adding any duplicates.
• Another ticket that would be nice is introducing a more coherent alias for certKey at least, and perhaps privateKey as well.
• twisted.internet.test.test_endpoints.ParserTestCase.test_impliedEscape is missing a docstring.
• I think that to be confident OpenSSL is doing what we want, there need to be a few tests that attempt actual connections. It sucks, but I'm not sure how else to trust the security of this code on even a superficial level (apart from writing our own inspectable, well-behaved, well-tested (probably by attempting actual connections) context class, and then using that).
• The PEMs should have better filenames, some text explaining their roles, relationships to each other, and how they were generated. Also, I'm sort of a fan of keeping this material in strings in the test file, but I'm not going to argue the point.

Sorry for all that SSL crap.

[glyph]

Replying to exarkun:

* The deprecation tests fail in an SVN checkout. Version("twisted") is somewhat unpleasantly different than Version("Twisted"). :/ The Fedora builders caught this because they still use svn to get Twisted, but none of the builders converted to bzr did because twisted.python.versions has no bzr support.

Fixed

* twisted.application.internet.StreamServerEndpointService

Fixed

* stopService doesn't set running to False

Fixed

* factory and endpoint attributes are undocumented

Fixed

* twisted.application.strports * How careful do we want to be about the deprecation of the default parameter to service? service(description, factory, None, reactor) won't currently emit a deprecation warning, but if we ever delete default it will, of course, break.

Fixed

* At the top of twisted.application.test.test_internet there's a zope.interface import after the Twisted imports.

Fixed

* For twisted.internet.endpoints.serverFromString and clientFromString I would like to see a complete list of supported keyword arguments somewhere. Doesn't need to be in the docstring for those functions, but it should probably at least be linked from there. Addressing this in #4478 is probably fine, so if you just want to add a comment there about it... Actually, this is basically end-user facing documentation, so having it in a howto (carefully separated from more technical information) makes more sense.

Addressing this in #4478

* The caCertsDir keyword argument to twisted.internet.endpoints._parseClientSSL isn't documented (whereas certKey and privateKey are).

Fixed

* There are some issues with how this argument is implemented: * There's no guarantee all of the PEMs in the directory will be readable. For example, the ipop3d package in Ubuntu apparently drops a file in /etc/certs/ssl which is only readable by root. This means clientFromString can't reliably use /etc/certs/ssl on Debian or Ubuntu. There also may be symlinks created by c_rehash to files which have since been deleted, resulting in dead links.

Fixed

* You can't add the same certificate to a Context multiple times. You'll get an OpenSSL.SSL.Error from X509_STORE_add_cert saying cert already in hash table. c_rehash automatically creates a duplicate of every certificate in a directory, so you won't be able to share a directory between clientFromString and, say, curl.

Fixed

* There's no guarantee that every file in the directory will be a certificate, or even that every entry in the directory will be a file. In particular, twisted.internet.test.test_endpoints.SSLClientStringTests.test_ssl fails in an SVN checkout because of the .svn directory it encounters.

Fixed

* I feel like there's still something mis-implemented in OpenSSLCertificateOptions' support of caCerts, but the trivial case works so for now maybe I'll try to forget about that, and if people report problems then we can just fix it in OpenSSLCertificateOptions (the trivial case works, at least, once I work around the above mentioned problems).

There are tests for OpenSSLCertificateOptions, and that isn't really what this branch is about. File a ticket if you can describe this more clearly, plz :)

* Oh, but, right, all those other problems. Uh. I was expecting that the caCertsDir option would be supported via Context.load_verify_locations(None, caCertsDir). Did you avoid this because it means not using CertificateOptions? That API avoids largely these problems (although it requires the use of `c_rehash) because it only reads certificates it needs (which you still might not have permission on, but that failure makes more sense, since it's a file you actually need, instead of a file that just happens to be there) and the hashing avoids adding any duplicates.

load_verify_locations is not available in all supported versions of PyOpenSSL and I don't want to deal with the buildbot version skew nightmare right now. Besides, this should be in twisted.internet.ssl somewhere, not in the string-parsing code that just uses it.

* Another ticket that would be nice is introducing a more coherent alias for certKey at least, and perhaps privateKey as well.

I think privateKey mostly makes sense, although it could be better explained. #4694 for certKey.

* twisted.internet.test.test_endpoints.ParserTestCase.test_impliedEscape is missing a docstring.

Fixed

* I think that to be confident OpenSSL is doing what we want, there need to be a few tests that attempt actual connections. It sucks, but I'm not sure how else to trust the security of this code on even a superficial level (apart from writing our own inspectable, well-behaved, well-tested (probably by attempting actual connections) context class, and then using that).

We're checking that it's an OpenSSLCertificateOptions. There are tests for that. If they're insufficient, then that shouldn't be this branch's problem.

* The PEMs should have better filenames, some text explaining their roles, relationships to each other, and how they were generated. Also, I'm sort of a fan of keeping this material in strings in the test file, but I'm not going to argue the point.

The filenames are pretty arbitrary because they're fairly arbitrary themselves. They each contain what I guess OpenSSL calls a "comment" (i.e. some text that just randomly isn't part of the certificate) explaining their purpose and how they were created.

I think that's just about it.

[glyph]

Added #4695 for plugin stuff.

[glyph]

Description updated so it doesn't say anything about pluggability any more.

[glyph]

(In [30155]) twisted.internet.endpoints now has 'serverFromString' and 'clientFromString' APIs for constructing endpoints from descriptive strings.

Author: glyph, exarkun

Reviewer: exarkun, glyph

Fixes #4473

In addition to providing new APIs to construct endpoints, this also retrofits the twisted.application.strports.service API to return a simple service wrapping an endpoint.