Opened 2 years ago

Last modified 3 months ago

#5642 enhancement new

TLS endpoint wrappers

Reported by: glyph Owned by: habnabit
Priority: normal Milestone:
Component: core Keywords:
Cc: habnabit, ashfall, tobias.oberstein@… Branch: branches/tls-endpoint-wrapper-5642-2
(diff, github, buildbot, log)
Author: habnabit Launchpad Bug:

Description

If I construct my own stream client or stream server endpoint, in addition to whatever interesting routing or shuttling-around of I'm doing with the bytes, I might want to encrypt them because they go over some public network as part of the process. There should be something so I can do TLSify(someEndpoint, contextFactory) and get back a new endpoint that does TLS without necessarily doing TCP.

(One example use-case is an endpoint that forwards a port via SSH, or opens a unix socket that some routing software eventually connects to a TCP socket.)

Change History (28)

comment:1 Changed 2 years ago by glyph

  • Owner set to ashfall

Since you're working on endpoints. (It would be neat, but no worries, no particular high priority.)

comment:2 Changed 19 months ago by tom.prince

#6308 is a duplicate of this:

The current way of doing SSL with endpoints involves using listenSSL/connectSSL. This means you need one for each of TCPv4 and TCPv6, and we'll need one for the hostname endpoint too. And, you can't use it with e.g. a SOCKS endpoint.

Better would be an endpoint (or maybe I mean pair of endpoints, one client one server) that wrap an existing endpoint. Ideally this wouldn't even call transport.startTLS, but rather use the TLS protocol implementation directly.

comment:3 Changed 14 months ago by rwall

habnabit wrote this:

20:38 < rwall> https://twistedmatrix.com/pipermail/twisted-web/2013-July/005059.html
20:38 < _habnabit> aha
20:39 < _habnabit> i've written that, actually
20:39 < _habnabit> https://github.com/habnabit/txsocksx/blob/parsley/txsocksx/ssl.py
20:39 < rwall> hehe
20:39 < _habnabit> composable endpoints!
20:40 < _habnabit> so maybe there should be a ticket to add this to twisted, blocking on the ticket that adds syntax for it
20:40 < _habnabit> or three tickets? add this, add composable endpoint syntax, add a parser plugin for this
20:40 < rwall> Right I get it now.
20:40 < rwall> yes, that sounds reasonable
20:41 < _habnabit> i think the reason i haven't submitted this to twisted is i have absolutely no idea how to test it
20:42 < _habnabit> somehow having such a small amount of code makes it hard
20:46 < rwall> I don't know it that well, but I guess you pass in a MemoryReactor to the inner endpoint...for a start...and check that you get back a 
               fake port with a tls protocol attached.
20:51 < dreid> How is the tls.TLSMemoryBIOFactory tested?
20:51 < _habnabit> let's see
20:54 < _habnabit> dang, there's a bunch of stuff here
20:55 < _habnabit> it looks like some of the tests involve hooking up two of them to each other--one as a server and the other as a client

comment:4 Changed 14 months ago by rwall

A discussion about an endpoint string syntax for TLS endpoint wrappers

19:51 < rwall> idnar: I got systemd socket activation working with TLS: https://gist.github.com/wallrj/5911925
19:56 < tomprince> It'd be nice if there was an endpoint string parser that would do that.
19:58 < _habnabit> you mean have the ability to wrap endpoints in other endpoints?
19:59 < tomprince> Yes.
20:00 < _habnabit> i was kind of working on this at the pycon sprint last year; i can't remember why it stalled
20:00 < rwall> tomprince: Something like "systemd+tls:domain=INET:index=0" as the string?
20:00 < tomprince> tls:endpoint=systemd\:index\=0:cert=...
20:01 < _habnabit> that gets nasty to write pretty quickly
20:01 < _habnabit> the syntax i proposed was something like 'systemd:domain=INET:index=0;tls'
20:01 < tomprince> tls:endpoint=(systemd:index=0):cert=....
20:02 < _habnabit> hah
20:02 < _habnabit> that would mean making the endpoint string parser a bit more complicated
20:02 < tomprince> More importantly, incompatible I think.
20:03 < tomprince> Anyway, we have parsley, for writing the parser.
20:03 < _habnabit> sure
20:03 < _habnabit> ; would be incompatible too though, hmm
20:03 < _habnabit> : and = are the only reserved characters, so, :: ?
20:04 < rwall> What about IPv6 addresses?
20:04 < _habnabit> they have to be escaped now anyway
20:04 < rwall> They have to be escaped anyway I believe
20:04 < rwall> right
20:05 < _habnabit> maybe that's a good reason to allow () or something as quoting
20:06 < tomprince> I think you definitely want something that allows nesting

comment:5 Changed 12 months ago by itamar

This ticket should not be held up by string syntax. That's a separate feature (please open a ticket for it).

comment:6 Changed 12 months ago by habnabit

I have a slightly better and tested version of this in txsocksx now: https://github.com/habnabit/txsocksx/blob/dadcf0567f18f47ffb6d2c0e551b22c0977c0336/txsocksx/ssl.py#L26-62

My previous implementation used TLSMemoryBIOFactory, which was not compatible with e.g. Agent because of TLSMemoryBIOFactory's use of WrappingProtocol. Specifically, WrappingProtocol proxies unknown attribute accesses to the *transport* instead of the *wrapped factory*, and Agent was trying to access the request method on the Protocol it was handed back from the endpoint. Normally this would be a HTTP11ClientProtocol, but when using TLSMemoryBIOFactory, it was instead a WrappingProtocol. wrappingProtocol.request then resulted in accessing the request attribute of a tcp.Client instance, which failed.

Instead, the new implementation calls startTLS on the protocol's transport as soon as the endpoint's deferred fires with a protocol. This new approach is easier to test, certainly. It won't, however, AFAIK allow for TLS over TLS. I suppose ultimately the question is then "should the resulting endpoint wrapper support TLS over TLS?"

comment:7 Changed 12 months ago by habnabit

  • Cc habnabit added

comment:8 Changed 12 months ago by exarkun

startTLS is one of those "it would be nice to get rid of it eventually (but it's pretty old and core, who knows if we'll actually ever do that)" APIs. Still, if twisted.protocols.tls is deficient in some way compared to startTLS, let's discuss those deficiencies as bugs to be fixed.

comment:9 Changed 12 months ago by glyph

Yeah, let's at least try not to add new functionality that depends on startTLS unless it needs to. In the absence of generalized protocol switching, I think some things do still need to; but this does not seem to be one of them, since it starts the protocol as TLS.

comment:10 follow-up: Changed 12 months ago by itamar

  • Cc ashfall added

I am a bit skeptical this can't be done using twisted.protocols.tls, given that that's what startTLS uses... but maybe there's some functionality elsewhere that needs to be used as glue.

comment:11 in reply to: ↑ 10 ; follow-up: Changed 12 months ago by habnabit

Replying to itamar:

I am a bit skeptical this can't be done using twisted.protocols.tls, given that that's what startTLS uses... but maybe there's some functionality elsewhere that needs to be used as glue.

It depends on what you're referring when you say "this". Right now it's certainly possible to use twisted.protocols.tls as a wrapper—the difficult part is that then the semantics differ. Specifically, wrapping vs. not-wrapping changes what you get when your endpoint fires with a Protocol.

comment:12 in reply to: ↑ 11 ; follow-up: Changed 12 months ago by glyph

Replying to habnabit:

Replying to itamar:

It depends on what you're referring when you say "this". Right now it's certainly possible to use twisted.protocols.tls as a wrapper—the difficult part is that then the semantics differ. Specifically, wrapping vs. not-wrapping changes what you get when your endpoint fires with a Protocol.

The semantics from the observer's perspective really ought not to differ. Of course what you get for your Protocol's transport attribute might differ between these two implementation approaches, but you can very easily change what you get back from an endpoint by adding a callback to the (wrapped) endpoint that returns some other value.

Why would it not be possible to add such a callback in this case?

comment:13 in reply to: ↑ 12 Changed 12 months ago by habnabit

Replying to glyph:

The semantics from the observer's perspective really ought not to differ. Of course what you get for your Protocol's transport attribute might differ between these two implementation approaches, but you can very easily change what you get back from an endpoint by adding a callback to the (wrapped) endpoint that returns some other value.

Why would it not be possible to add such a callback in this case?

Ah! That would do it. I simply hadn't thought of that as an option. I'll try it now, but I suspect that's all that's necessary.

comment:14 Changed 12 months ago by habnabit

Yes! That is all that's necessary for using TLSMemoryBIOFactory as a wrapper. Here's a minimal implementation:

class TLSWrapClientEndpoint(object):
    implements(interfaces.IStreamClientEndpoint)

    def __init__(self, contextFactory, wrappedEndpoint):
        self.contextFactory = contextFactory
        self.wrappedEndpoint = wrappedEndpoint

    def connect(self, fac):
        fac = tls.TLSMemoryBIOFactory(self.contextFactory, True, fac)
        return self.wrappedEndpoint.connect(fac).addCallback(self._unwrapProtocol)

    def _unwrapProtocol(self, proto):
        return proto.wrappedProtocol

The problem remains that I'm unsure how to test this—is there a fake TLSMemoryBIOFactory, or should the test maybe do a real TLS handshake?

comment:15 follow-up: Changed 12 months ago by exarkun

You can test most of this without getting anything TLS-related involved.

"TLS endpoint wrapper" is just a particular instance of "endpoint wrapper".

You can test most of the logic with a much simpler kind of wrapping - say, a wrapper that sends every byte incremented by one.

Then "TLS endpoint wrapper" is just a particular assembly of existing, well-tested components that only needs sufficient tests to verify the assembly was done correctly.

comment:16 in reply to: ↑ 15 Changed 12 months ago by glyph

Replying to exarkun:

You can test most of the logic with a much simpler kind of wrapping - say, a wrapper that sends every byte incremented by one.

This is clearly not a realistic demonstration of a cryptographic protocol.

Now, if you were to xor every byte it with a bit-pattern like 0b01010101 (ASCII: "U") that would totally be a realistic simulation.

comment:17 Changed 9 months ago by itamar

Here are the tests I would write:

  1. connect() returns instance of protocol created with given factory.
  2. This protocol's transport is a TLSMemwWhatever.
  3. This TLSMemwWhatever is using the correct context factory.
  4. This TLSMemwWhatever's transport is the transport hooked up by the underlying wrapped endpoint.
  5. This TLSMemwWhatever is in client mode.

comment:18 Changed 9 months ago by oberstet

  • Cc tobias.oberstein@… added

comment:19 Changed 6 months ago by habnabit

  • Owner changed from ashfall to habnabit
  • Status changed from new to assigned

comment:20 Changed 5 months ago by glyph

There's a prototype of something like this in https://github.com/glyph/txsni

comment:21 Changed 5 months ago by habnabit

So, I'm trying to figure out how exactly the clientFromString for this should behave. Originally I wrote a tls: string parser that would take an endpoint description and wrap that endpoint.

However, this whole thing, of course, needs hostname validation for clients. It's a bit redundant to specify, say, tls:tcp\:example.com\:443:hostname=example.com. I'm thinking now that the tls: parser should use HostnameEndpoint underneath to establish connections. So, tls:example.com:443, and it passes the hostname both to HostnameEndpoint and CertificateOptions. There can be a separate tlswrap: endpoint type that does wrap another endpoint.

comment:22 Changed 5 months ago by habnabit

  • Author set to habnabit
  • Branch set to branches/tls-endpoint-wrapper-5642

comment:23 Changed 5 months ago by habnabit

  • Branch changed from branches/tls-endpoint-wrapper-5642 to branches/tls-endpoint-wrapper-5642-2

(In [42188]) Branching to 'tls-endpoint-wrapper-5642-2'

comment:24 Changed 5 months ago by habnabit

  • Keywords review added
  • Owner habnabit deleted
  • Status changed from assigned to new

comment:25 Changed 5 months ago by habnabit

This currently only implements client wrappers; I'm not sure if the branch should implement server wrappers as well at the same time.

comment:26 follow-up: Changed 5 months ago by tom.prince

Small branches. Small branches. Small branches.

comment:27 in reply to: ↑ 26 Changed 3 months ago by glyph

Replying to tom.prince:

Small branches. Small branches. Small branches.

Yes.

comment:28 Changed 3 months ago by glyph

  • Keywords review removed
  • Owner set to habnabit

Thanks very much habnabit.

  1. TLSWrapperClientEndpoint should probably just be called TLSClientEndpoint.
  2. the fac parameter to should be protocolFactory, so sayeth the interface. (So say we all.) But also don't have names like "fac".
  3. Pretty sure you don't have to do stuff like this:
    +    @ivar prefix: See
    +        L{IStreamClientEndpointStringParserWithReactor.prefix}.
    
    properties should be the same as methods in this regard. If pydoctor fills in the information correctly like it does with methods, please just remove it. (Although if you got hassled by twistedchecker or pydoctor about this, please file a bug on the appropriate tool.)
  4. _parseClient is very ugly. This is a bug, or at the very least a missing feature, in zope.interface. Obviously it should be left in place to satisfy the verifyObject tests or whatever, but please file a bug against ZI and link to it from the docstring.
  5. TLSWrapperClientEndpoint is actually a totally general wrapper endpoint - to the point where its test cases actually verify its generality via the upper-casing wrapper factory - made specific only by the fact that it curries only a single argument into its _wrapper callable. If it's any significant amount of work at all, don't bother with this, but it seems like you could trivially convert it into a WrapperEndpoint and then just have a wrapTLSEndpoint function that used it with TLSMemoryBIOFactory and did the currying with a lambda rather than something that required documentation as an argument and an ivar and so on. If you decide not to do this then please file a ticket for generalizing it to a wrapper endpoint to facilitate other kinds of composition.
  6. test_endpoints.FakeEndpoint
    1. There are a couple of other bits of code within Twisted which already do parts of this; twisted.conch.test.test_endpoints.SingleUseMemoryEndpoint, some stuff in twisted.internet.test.fakeendpoint. As we create fakes like this we should always be careful to clean things up, to centralize them, to test them and to make them public. Consider putting this somewhere like proto_helpers, but at least eliminate the duplication with conch.
    2. Consider splitting the fake into two objects so that the system under test doesn't have access to endpoint.deferred without typing at least a few underscores. It would obviously be bad to end up testing that. (This is a general design thing we should get better at with test doubles, too.)
  7. Don't create new old-style classes, even in tests :-). UppercaseWrapperFactory and NetstringTracker and NetstringFactory should all inherit from object.
  8. NetstringFactory shouldn't be a ClientFactory, endpoints don't even use ClientFactorys.
  9. Except don't bother even having NetstringFactory. Kill several lines of docstrings and just use Factory.forProtocol.
  10. Assert something useful about the error message in test_noSSLSupport. (Like, that it contains the word "SSL" and ideally something about installing pyOpenSSL). We need to get better about considering the usability of such messages.
  11. test_utf8Encoding's docstring is a little confusing because it uses the word "or". It passes the correct type to each underlying API, it should just say what it passes where.
  12. test_defaultSSLOptions reveals some unfortunate things about the implementation of this API. Since the tls: prefix is new code, we don't have any compatibility expectations for it, so it should be as secure as possible by default. That means that, being client TLS, it should always verify by default, using the default platformTrust. Turning off verification, while it should still be possible, should be ugly and highly visible on the command line. absolutelyNoSecurity:True is better than verify:False. This means you can't use the caCerts and verify parameters (which should probably be deprecated anyway) - use the trustRoot argument to CertificateOptions.
  13. As a corrolary to the previous point, tlswrap: should take a mandatory hostname as its first parameter, which can optionally be like * or something if you really really don't want to pass a hostname. (This is extra nice because shell globbing will helpfully screw it up and you'll have to quote the whole thing in order to get it to work.)
  14. Given that tlswrap: is a bit more esoteric and is also a bunch of additional code, it would make sense to split this up. Especially given the length of this review ;-).
  15. In the endpoints.rst:
    1. under "TLS", contextEndpoint is unused and contextFactory is undefined.
    2. in the "TLS wrapper" section, you use tls: instead of tlswrap: for the example.
Note: See TracTickets for help on using tickets.