Skip to content

BoringSSL compatibility fixes #1892

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

Merged
merged 1 commit into from
Aug 8, 2024
Merged

Conversation

markmentovai
Copy link
Contributor

I’m not sure if BoringSSL compatibility is still desired, but there’s an explicit callout to BoringSSL support at httplib.h:242, added in 4a7a81e. I don’t know if eba9808, estabilshing the OpenSSL 3 minimum, changes anything. While it’s true that OpenSSL 1 has reached end-of-life, BoringSSL is still supported and largely implements the OpenSSL 1 API. It’s worth trying to upstream this patch, so that I don’t need to maintain it downstream.

This patch is necessary to build cpp-httplib in Crashpad, itself in Chromium, using BoringSSL. Details here.

The fixes include:

  • Library version check: tolerate BoringSSL as an alternative to OpenSSL 3.
  • Don’t call OPENSSL_thread_stop, which is not in BoringSSL.
  • Use SSL_get_peer_certificate (deprecated in OpenSSL 3), the old name for SSL_get1_peer_certificate, because the new name is not in BoringSSL.
  • Call SSL_set_tlsext_host_name directly instead of making an SSL_ctrl call that BoringSSL does not support. The feared -Wold-style-cast warning that occurs when buidling with OpenSSL is not triggered in BoringSSL.

@yhirose
Copy link
Owner

yhirose commented Aug 8, 2024

@markmentovai thanks for the pull request. Since I am not planning to use BoringSSL, I won't be able to maintain this code changes. So if you can accept a responsibility to maintain this BoringSSL related code, I don't mind merging the pull request. (I actually do the same to the CMake build code. Some contributors maintain it for me.)

As for your changes, I just wonder if you can disallow BoringSSL versions which are no longer supported. (In the case of OpenSSL, I disallow all the "end of life" versions for safety by checking with OPENSSL_VERSION_NUMBER.)

This patch is necessary to build cpp-httplib in Crashpad, itself in
Chromium, using BoringSSL. Details at [1].

The fixes include:
 - Library version check: tolerate BoringSSL as an alternative to
   OpenSSL 3.
 - Don’t call `OPENSSL_thread_stop`, which is not in BoringSSL.
 - Use `SSL_get_peer_certificate` (deprecated in OpenSSL 3), the old
   name for `SSL_get1_peer_certificate`, because the new name is not in
   BoringSSL.
 - Call `SSL_set_tlsext_host_name` directly instead of making an
   `SSL_ctrl` call that BoringSSL does not support. The feared
   -Wold-style-cast warning that occurs when buidling with OpenSSL is
   not triggered in BoringSSL.

[1] https://chromium.googlesource.com/crashpad/crashpad/+/1a62a0182557c89494676c06611f1ca731bcb2db
@markmentovai
Copy link
Contributor Author

#1892 (comment):

@markmentovai thanks for the pull request. Since I am not planning to use BoringSSL, I won't be able to maintain this code changes. So if you can accept a responsibility to maintain this BoringSSL related code, I don't mind merging the pull request. (I actually do the same to the CMake build code. Some contributors maintain it for me.)

I completely understand.

My team and I can certainly contribute patches to maintain BoringSSL support in cpp-httplib as necessary for our use. We wouldn’t be able to commit to continuing ongoing support outside of when we sync to a new version of cpp-httplib. But certainly as we pull new versions of cpp-httplib, we’d need to write the patches to make it work with BoringSSL anyway, and I’m happy to (and in fact, prefer) send those patches back here.

As for your changes, I just wonder if you can disallow BoringSSL versions which are no longer supported. (In the case of OpenSSL, I disallow all the "end of life" versions for safety by checking with OPENSSL_VERSION_NUMBER.)

BoringSSL doesn’t really version itself in the same way that OpenSSL does. In fact, API stability is explicitly listed as not a goal (see its README.md). The current version does identify as OpenSSL 1.1.1, although this is more for API purposes than anything else—and it doesn’t really help with much, because it’s been at that version for 4 years.

I’ve pinned it to that at your request, I’m just mentioning it so that you know what you’re getting.

@yhirose yhirose merged commit 69c84c9 into yhirose:master Aug 8, 2024
4 checks passed
@yhirose
Copy link
Owner

yhirose commented Aug 8, 2024

@markmentovai thanks for the clear explanation. I have just merged this pr and bumped cpp-httplib version to v0.16.2. Thanks for your fine contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants