WIP: Add TLS support

[belka-ew]

Hey Dragos,

I have a really crazy idea... Ok it isn't that crazy. A week ago I thought why not to write the first usable HTTP/2 implementation in D (using asynchronous and libhttp2). Since asyncio has TLS support over openssl I started a few days ago to port TLS stuff (but I'm planning to use botan for actual TLS algorithms). This pull request includes:

• a data structure NamedTuple/ExtraInfo I thought to use for passing extra information to the transports (returned by get_extra_info in asyncio). It is very similar to D's tuple but all fields should be named, so it is kind of dictionary that can hold different data types. See unit tests for examples how it can be used.
• FlowControlTransport mixin rewritten from _FlowControlMixin(Transport). It will be used later by TLS transport and protocol. I haven't really tested it (don't know how to rewrite python unit tests in D) but I will see how well it works when I'll implement another parts based on it.

I have written some more code, but it isn't in the ready state yet.

So if you like the idea, I'll continue to work on it; you can leave this PR open, then I will add commits to this request; or merge it and I'll create new PR. So I'm waiting for a feedback from you on TLS. Should I open a ticket for discussing it and maybe a checklist with an approximate plan?

[dcarp]

Hi Eugene,

this was on my plan also, but if you have the resources to add it, please do so. Actually this will be completely awesome!

It is much easier to let dub manage the dependencies, so please use Botan if possible. But, for the sake of a leaner, please try to provide an API similar to the python asyncio.

Regarding the PR I have a few comments:

1. Please use the WIP (work in progress) prefix. Example: WIP: Add TLS support. This signals that the PR is not ready yet, but feedback is welcome.
2. Is NamedTuple really necessary? Isn't this what you want?
3. FlowControlTransport will be used for defining a Transport, won't it? Maybe define it as abstract class. Start simple, later we can define it as mixin, if necessary.

[belka-ew]

Thanks for your quick answer. I always need some feedback, so we can get it in the right way :)

2. NamedTuple isn't absolutely necessary. I just tried to make its use more pleasant. For example since tuple's fields shouldn't be named, tuples are ordered, so if you have Tuple!(int, "a", int, "b", int, "c") you have to assign all fields one by one or the whole tuple at once in the right order like tuple!("a", "b", "c")(1,2,3). With this NamedTuple you can change multiple values at once and in any order:

NamedTuple!(7, "a", int, "b", int, "c") ei1;
ei1!("b", "c")(8, 9);

Will set only b and c and leave a be 7. It was the idea behind it. But since these ExtraInfo things aren't very large, maybe it doesn't matter and it would be better to use tuples, since it is in the std. I actually don't mind if I remove it. Do you think Tuples fit better here?

3. My only problem with an abstract class was that _FlowControlMixin uses self._protocol.pause_writing() and self._protocol.resume_writing(): https://github.com/python/asyncio/blob/master/asyncio/transports.py#L253, but self._protocol isn't set in _FlowControlMixin but first in _SSLProtocolTransport... Hm... I had to define protocol as a protected member of FlowControlTransport and that's all. I'll fix it and make FlowControlTransport to an abstract class.

Another question about botan. Do we need some abstraction around it or can it be used directly? I mean for libasync you created src/asynchronous/libasync. Is a kind of wrapper needed around botan? I mean asyncio supports only OpenSSL as far as I see.

[dcarp]

2. I think that Tuples are good enough: as good as a struct. Writing ei1!("b", "c")(8, 9); instead of ei1.b = 8; ei1.c = 9; is more ore less a distraction from the real work.
3. ok I think it is a good idea to wrap botan. Two reasons: 1. maybe openssl or other alternative (mocking?) will be added later; 2. easy to adopt any further changes of the botan API.

[belka-ew]

On the current state: It's definitely going in the right direction. I'm currently doing the server part and can load the certificates and pass the handshake. Now I'm working on passing the communication between the server and client through Botan. I'm testing everyting with the browser for now; will also write some tests in the second step.

[belka-ew]

Yeah, I think I have the first working server version with TLS:

import asynchronous;
import asynchronous.tls;

void main()
{
    SslContext ctx = createSslContext();
    ctx.loadCertChain("/usr/local/share/ca-certificates/cacert.crt", "/etc/ssl/private/belka.key");

    startServer(getEventLoop(),
                (StreamReader r, StreamWriter w)
                {
                w.write("HTTP/2.0 200 OK\nContent-Type: text/html\nContent-Length: 13\nServer: StillUnknownServer\n\n<html></html>");
                },
                "127.0.0.1",
                "8443",
                DEFAULT_LIMIT,
                UNSPECIFIED!AddressFamily,
                AddressInfoFlags.PASSIVE,
                null,
                100,
                ctx);

    getEventLoop().runForever();
}

and https:// should work

1. @dcarp, till now I've put everything in the single file, tls.d and I want to discuss how to organize the code. Since we have two abstractions: event loop and cryptography - I would reorganize libasync aswell (it shouldn't cause backward incompatibility). The possibilities, I see, are:

   • /src/asynchronous/events/libasync.d

     /src/asynchronous/ssl/botan.d (or /src/asynchronous/tls/botan.d or /src/asynchronous/crypto/botan.d)

   • /src/asynchronous/internal/events/libasync.d

     /src/asynchronous/internal/ssl/botan.d (and similar variants like above)

   • or what I personally like the most (short and the internal code is isolated from the public API):

     /src/asynchronous/internal/libasync.d

     /src/asynchronous/internal/botan.d

   What do you think on it? Do you have other ideas?

2. And another topic is naming conventions. Since everyone seems to use TLS instead of SSL, I would suggest do it aswell. Botan uses everywhere TLS and the old SSL was removed from C++ Botan version, vibe.d deprecated all the SSL* and uses TLS. So I would prefer to rename from the beginning SslContext to TlsContext and so on.. But maybe it isn't that important... Just my two cents.

Edit: And the common SSL API: tls.d, ssl.d, sslproto.d?

[belka-ew]

And I would move sslContext argument of startServer to the beginning of the argument list, maybe after the port number :)

[dcarp]

Thanks! This is great work!

1. When deciding the directory structure I always think about the static import names. Ex: asynchronous.libasync.events.LibasyncEventLoop your suggestions would be: a) asynchronous.events.libasync.LibasyncEventLoop b) asynchronous.internal.events.libasync.LibasyncEventLoop c) asynchronous.internal.libasync.LibasyncEventLoop

a) and b) don't look right to me: events should NOT precede libasync c) could work. At this stage is not very important, but you can do this if you find it more readable.

2. You are right TLS is the right name and we should use it. In python "ssl" is legacy. BTW the D naming convention (https://dlang.org/dstyle.html) is that acronyms are either all capital or all small case. Ex. auto tlsContext = new TLSContext(...) tls.d looks good to me.

1. Yes please, move it after service name (port). This will make it consistent with createConnection

Please also see my comments inline.

[belka-ew]

I made some cleanup. I will now go further and implement the client. And then I can write the tests where the server and the client communicate with each other. In the next step I have to check that I catch all Botan exceptions and wrap them in something like TLSException and do appropriate error handling.

I have a weired issue with the last dub and memutils unit tests (see travis logs). I've found a workaround but I think I'll open first a ticket for memutils since I'm not sure if I'm doing something wrong or there is a bug in memutils or dub (it doesn't include DebugAllocator in the unittest configuration, but tests seem to use it)

[etcimon]

Try adding "EnableDebugger" to the unittest versions

[belka-ew]

Try adding "EnableDebugger" to the unittest versions

Yes, it was the "workaround" I meant and it works. Then it is the way to go. Thanks for your feedback)