Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add BearSSL vtls implementation #4597

Closed
wants to merge 1 commit into from

Conversation

@michaelforney
Copy link
Contributor

michaelforney commented Nov 14, 2019

I saw in the curl user survey 2017 that you were open to adding support for BearSSL:

Support for BearSSL backend
It’s not that hard to add support for a new SSL backend. Go ahead!

So, here it is :)

Right now it is just the basics and doesn't support any optional vtls features. I've done some basic tests with a few different websites, but am looking for feedback for how to verify behavior in corner cases (EOF, close_notify, TCP RST, etc).

I've used the sread and swrite functions for reading and writing to the socket, so I believe there is no reason this shouldn't work on Windows, but I don't have access to a Windows machine to test.

@bagder bagder added the SSL/TLS label Nov 14, 2019
configure.ac Outdated Show resolved Hide resolved
@michaelforney michaelforney force-pushed the michaelforney:bearssl branch from 95f0024 to ad6252d Nov 15, 2019
@bagder bagder dismissed their stale review Nov 16, 2019

updated

@bagder

This comment has been minimized.

Copy link
Member

bagder commented Nov 16, 2019

Thanks!

Now I can build and I've tested it out a little bit. I think a major feature (I think it needs to be considered mandatory) that doesn't work is curl's -k option that disables the server certificate verification.

The other larger problem I found, that might be related to the above issue, is that it can't connect with TLS to a site specified with IP address only - so all FTPS tests fail. Run test 400 or just invoke curl https://151.101.86.49 -vk

A more minor thing I found that is lacking in the curl adaptation is ALPN support, so curl will not use HTTP/2 with BearSSL at this point - it seems there is ALPN support in the library so its only a matter of using it correctly.

Finally, BearSSL lacks for example TLS 1.3 support which is pretty significant these days, so I can't see how we can recommend this backend to any new users...

@michaelforney michaelforney force-pushed the michaelforney:bearssl branch 3 times, most recently from 6aee72f to 84b966b Nov 16, 2019
@michaelforney

This comment has been minimized.

Copy link
Contributor Author

michaelforney commented Nov 17, 2019

Thanks!

Now I can build and I've tested it out a little bit.

Thanks for testing it out!

I think a major feature (I think it needs to be considered mandatory) that doesn't work is curl's -k option that disables the server certificate verification.

BearSSL makes you go a bit out of your way to skip certificate verification, but this is done now.

The other larger problem I found, that might be related to the above issue, is that it can't connect with TLS to a site specified with IP address only - so all FTPS tests fail. Run test 400 or just invoke curl https://151.101.86.49 -vk

Should be fixed now, I needed to pass a NULL hostname to br_ssl_client_reset when it could be parsed as an IP address to disable hostname verification.

A more minor thing I found that is lacking in the curl adaptation is ALPN support, so curl will not use HTTP/2 with BearSSL at this point - it seems there is ALPN support in the library so its only a matter of using it correctly.

I added ALPN support, and it looks like HTTP/2 is working now.

Finally, BearSSL lacks for example TLS 1.3 support which is pretty significant these days, so I can't see how we can recommend this backend to any new users...

It is definitely on the roadmap (see https://bearssl.org/tls13.html), which I think is slightly out of date (it looks like RSA/PSS is already implemented in the latest git version).

@bagder

This comment has been minimized.

Copy link
Member

bagder commented Nov 18, 2019

Thanks! But it seems something is still off though with the cert checks.

curl https://151.101.86.49/

This is meant to fail (since it can't verify the cert) but should work and provide contents with -k. My BearSSL build gets content without -k. (The IP is one of Fastly's CDN servers that host the curl site and many others.)

curl https://ns1.haxx.se/ -k

This should work since the host name resolves to an IP address and that host serves HTTPS content there, but my BearSSL build fails to deliver anything and says:
curl: (35) SSL connect error (without -k it correctly complains about the cert)

@michaelforney michaelforney force-pushed the michaelforney:bearssl branch 2 times, most recently from 158847b to 70ce89a Nov 18, 2019
@michaelforney

This comment has been minimized.

Copy link
Contributor Author

michaelforney commented Nov 18, 2019

Both of these issues are related to hostname handling. I've made a few changes, outlined below.

In BearSSL, when you initialize a client with br_ssl_client_reset, you can pass a hostname, which is used for SNI, and is also passed to the X.509 context for hostname verification. If it is NULL, both SNI and hostname verification are disabled. However, if you implement your own X.509 context (by wrapping the minimal one), you can separately control SNI and hostname verification (i.e. for verifyhost=0). It looks like BearSSL does not support verifying a subjectAltName against an iPAddress, so I think if we ever have verifyhost==1 and hostname is an IP address, it should just fail early.

Right now, things work as follows:

  • If the hostname from conn->host.name is an IP address, then if verifyhost==0, set hostname to NULL, otherwise fail early in step 1.
  • The hostname (possibly NULL) is then passed to br_ssl_client_reset, enabling (or disabling) SNI, and also passing through to the X.509 context.
  • The X.509 context will pass the hostname to start_chain if verifyhost!=0 enabling hostname verification. Otherwise, it will pass NULL, disabling hostname verification.
  • If verifypeer==0, any errors from the X.509 engine are ignored.

Does this seem like correct behavior of verifyhost and verifypeer to you?

@bagder
bagder approved these changes Nov 21, 2019
Copy link
Member

bagder left a comment

Looks good! Will merge within soon.

@michaelforney michaelforney force-pushed the michaelforney:bearssl branch from 70ce89a to ed2f9e8 Nov 21, 2019
@michaelforney

This comment has been minimized.

Copy link
Contributor Author

michaelforney commented Nov 21, 2019

Thanks for reviewing. I made a couple last tweaks related to closing the context. I added a boolean active flag to the ssl_backend_data indicating that br_ssl_client_reset has been called. The previous check for connssl->connecting_state == ssl_connect_done was wrong, since connecting_state gets reset when the connection is established. connssl->state == ssl_connection_complete would've been better, but I think we should still send close_notify after a failed handshake (is this right?).

Also, the loaded certificates are now freed in all cases, even if we didn't make it very far when connecting.

@bagder bagder closed this in 9b87916 Nov 26, 2019
@bagder

This comment has been minimized.

Copy link
Member

bagder commented Nov 26, 2019

Thanks a lot for your hard work on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.