Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#12388 closed bug (fixed)

Missing support for TLS SNI (easy)

Reported by: FreeFull Owned by: axeld
Priority: normal Milestone: Unscheduled
Component: Kits/Network Kit Version: R1/Development
Keywords: Cc:
Blocked By: #12546 Blocking:
Has a Patch: yes Platform: All

Description

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

Attachments (1)

0001-Add-support-for-TLS-SNI.patch (5.8 KB) - added by markh 3 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 4 years ago by pulkomandy

Component: Applications/WebPositiveKits/Network Kit
Owner: changed from pulkomandy to axeld
Summary: Missing support for TLS SNIMissing support for TLS SNI (easy)

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.

comment:2 Changed 3 years ago by 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.

comment:3 Changed 3 years ago by 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.

comment:4 Changed 3 years ago by markh

Has a Patch: set

comment:5 Changed 3 years ago by 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.

Last edited 3 years ago by markh (previous) (diff)

comment:6 Changed 3 years ago by 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).

comment:7 Changed 3 years ago by 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.

comment:8 Changed 3 years ago by 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.

comment:9 Changed 3 years ago by 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.

comment:10 Changed 3 years ago by 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.

comment:11 Changed 3 years ago by 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.

comment:12 Changed 3 years ago by 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.

comment:13 Changed 3 years ago by waddlesplash

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

comment:14 Changed 3 years ago by 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...

Changed 3 years ago by markh

comment:15 Changed 3 years ago by 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).

comment:16 Changed 3 years ago by 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.

comment:17 Changed 3 years ago by waddlesplash

Resolution: fixed
Status: newclosed

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

comment:18 Changed 3 years ago by 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.

comment:19 Changed 3 years ago by 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

comment:20 Changed 3 years ago by pulkomandy

Blocked By: 12546 added
Note: See TracTickets for help on using tickets.