Missing support for TLS SNI (easy)

[FreeFull]

SNI support can be tested here: ​https://sni.velox.ch/? This results in some https websites triggering self signed certificate errors, although in any browser supporting SNI (such as QupZilla), the certificate will be considered valid.

Haiku version: x86_gcc2 hrev49658

[pulkomandy]

It seems this is a one line change on OpenSSL level: ​http://stackoverflow.com/questions/5113333/how-to-implement-server-name-indication-sni

However, BSecureSocket does not know the hostname (it gets the already resolved BNetworkAddress).

I'm in vacation starting today and until next week, so if someone wants to have a look, feel free to.

[markh]

I had a look at this and I came up with two ways to fix it.

• Allow passing the hostname to the socket, either in the constructor or in the Connect call.
• Store the hostname in BNetworkAddress

I gave the first option a try and it was more work than I expected and it crashes in the Connect call of BSecureSocket now. I added the SSL_set_tlsext_host_name call there and perhaps I am doing something wrong here. What is the best way to figure out what is going wrong? Attach a patch with my changes?

Implementing the first option gave me the idea that the second option would be better, but I don't know the network code good enough to say for certain that it is better.

As both of the proposed changes are on the Haiku side, we perhaps also need to take into account backwards compatibility.

[pulkomandy]

The second solution would be preferred.

As you said, the changes are on Haiku side which means this can be tested more easily with a small test application just doing one HTTPS request. Said app can then be put into Debugger for a closer analysis.

Note: this is the same problem that currently prevents accessing freelists.org from Web+.

As for backward compatibility, the BUrlRequest family of classes isn't considered stable yet. I don't think BNetworkAddress is, either (BeOS compatibility is ensured with the BNetAddress class). The changes do require some coordination still, probably they will need a rebuild of WebKit as they are delivered to the repo.

[markh]

I finally had time to try the second solution and got something working. It even works without needing a new version of Web+ to be built. I added a private variable fHostName to BNetworkAddress and filled it in when the SetTo call contained a host. This does mean that if you make a call to a SetTo function without a host, it will not enable the SNI support. Not sure what to do about that. Thought about adding a host parameter to those SetTo functions, but decided against it for now. I used the existing HostName function to return fHostName, but left the TODO in place. Not sure if we want to do something extra in case fHostName is empty. Made some changes in SecureSocket to get the host name from the BNetworkAddress and use it to make the SSL_set_tlsext_host_name call. Only did it for Connect, but perhaps Accept needs to be changed as well.

Tested some sites (freelists, slashdot) that didn't work without it and confirmed that with my changes they work fine. Also tested some other sites that were already working to make sure I didn't break anything and it seems fine.

[pulkomandy]

Looks good in general, I see some possible changes/improvements (this is open to discussion):

• In SecureSocket, _SetupConnect is checking for NULL, I think it should also check for an empty string as that's what String() will return (as used in Connect).
• When there is no hostname, the given IP address could be converted to a string and used ("127.0.0.1" or whatever). I'm not sure if this is of any use.
• This should also be tested outside Web+, in particular when 302 redirects are involved, because Web+ handles those on its own and does not use the HttpRequest code for that. We should make sure that the behavior is still correct in that case (the redirected host should be used for the second connection).

[markh]

• Good point regarding the NULL check, I will add it
• Won't the IP address be used automatically as that is what will be passed in as host to the SetTo functions? I don't think it will be of any use as that is not the intent of the SNI support.
• Is there some documentation on the BHTTPRequest class? I have never done any coding with it and it I can't seem to reach www.haiku-os.org to check for documentation.

[pulkomandy]

It seems the website is currently offline.

For examples of use of BHttpRequest you can have a look in src/tests/kits/network (and if possible, adding more tests there for the new features would be nice).

Yes, giving the IP address to SNI may not be a very good idea. I'm not sure any certificate provider would deliver a cert for an IP without domain, anyway.

[markh]

How do I compile and run those tests? Regarding the 302 redirect, I see that the test website used in those unit tests can do a redirect, but to which url would we try the redirect? If it is a unit test, it would be convenient if it always kept working, so I'm not sure redirecting to freelists is a good idea.

[pulkomandy]

jam -q UnitTests will build the tests, then you get an UnitTester command which is the test harness for running them (IIRC in generated/tests/bin or somewhere around there, have a look at the articles on www.haiku-os.org about "unit tests").

The tests with online services are not the best solution, as a first step we could redirect to freelists, but ideally we should run these with a local HTTP server. I don't know if PoorMan is up to the task or if we should use lighttpd or some other available server. This will require a lot more work than just fixing this particular bug, so maybe we can keep that for a later stage.

[markh]

Thanks, I will give that a try. I will fix the NULL check and update the current patch and I will have a look at the tests. I noticed that th test website has a https version that fails to load with the current version of Web+, while it works if I apply my patch, so at least that should be a good test.

[markh]

Updated the patch with a fix for the NULL check. Will look at the unit tests next. I'm thinking of making a separate patch for that.

[waddlesplash]

Ping. Can this just be merged, or are we waiting for the unit tests?

[pulkomandy]

I think I see a bug in the patch. Sorry for not reviewing earlier.

In BNetworkAddress::SetTo(const char* host, const char* service, uint32 flags) , you changed the error handling. I think you forgot to change == B_OK to != B_OK as in the other variant. This is something that unit tests would probably catch.

With this issue fixed the patch is ok to merge as is. I don't want to put unit test burden on people trying to contribute to Haiku, we should fix our own mess there...

[markh]

I fixed the bug you found and also fixed another problem when compiling without openssl. I am looking into the unit tests as well, but it is fine to do this later. I ran into a bit more trouble than I expected (see #12799 for example).

[jessicah]

Regarding the style in the SetTo() functions; the previous style is preferable. Errors with early return, etc., make the code easier to follow and also reduces indentation.

[waddlesplash]

Applied in hrev50368 -- sorry, jessicah, I saw your comment after committing.

[markh]

That's not what the code was doing. It was returning early on success, which meant that any value that needed to be done on success needed to be done in multiple places. This was already happening with fStatus and adding fHostName multiple times seemed to me to make it worse.

Also, in general I dislike multiple exit points in a function, but if the coding style is to prefer this then I will keep it in mind for other patches.

[pulkomandy]

There is some disagreement on the way this was done on the mailing list after the change was pushed (I don't know why people waited for this to happen before reviewing...). Adding the link here in case you don't read haiku-commits daily:

​https://www.freelists.org/post/haiku-commits/haiku-hrev50368-srckitsnetworklibnetapi-headersosnet,8