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

[Demo] [Incomplete] Allow compilation and linking against BoringSSL #116399

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

gpshead
Copy link
Member

@gpshead gpshead commented Mar 6, 2024

NOTE: This is focused solely on the C code changes required. It is not usable as is. Missing items if you were to attempt to use this yourself: build system changes necessary for this to work have not been completed -- we don't use CPython's build system when building the runtime at Google.

The purpose of opening this as a Draft PR is to show what kinds of changes are currently needed. I'm not aiming to see this merged as is - though we may choose pull bits and pieces out into their own small PRs that could make sense on their own to minimize the changes necessary for those using other TLS libraries where easily maintainable.

It may be of potential interest to some ongoing discussions (I expect to link to it) and issue #116333 authors and its draft PR #116334.


Playing around with hackish quick attempts (mostly not yet included in the PR branch) at using CPython's existing build system:

  • BoringSSL libssl.a requires C++ linkage. The configure check for _ssl module being available fails otherwise.
  • The wrong OpenSSL headers appear to get included when compiling Modules/_hashopenssl.c
  • You must statically link BoringSSL and thus the _ssl and _hashlib modules as that is its intent (this requires Modules/Setup* changes)

Proper configure.ac, Setup, & Makefile.pre.in plumbing would be needed to alleivate all of this.


📚 Documentation preview 📚: https://cpython-previews--116399.org.readthedocs.build/

Does not include configure.ac or Makefile.ac changes.  The _ssl_data
changes are hand created rather than properly updating the code
generator.  A future upstream API is anticipated to deal with those
better anyways.
BoringSSL, for now, does not provide these APIs because of design flaws
in the API.  When we've disentangled this mess, and implemented the
functions in BoringSSL, this patch can be removed.
It adds an ill-advised feature that BoringSSL can't support.
This concurrency fix needs reworking for use with BoringSSL.
Don't test for hash functions that are entirely optional for a TLS
implementation.  This configure check should be reworked anyways, even
md5 and sha1 could possibly not be present (FIPS?).
@gpshead gpshead self-assigned this Mar 6, 2024
@gpshead
Copy link
Member Author

gpshead commented Mar 6, 2024

FYI for @twouters & @davidben - This serves as a public example of most of the changes needed to link CPython against BoringSSL.

@davidben
Copy link
Contributor

davidben commented Mar 6, 2024

For some context, some of the changes can probably be reduced. In particular, I plan to implement the "ex" versions of SSL_read and SSL_write. We'd skipped them originally because OpenSSL messed them up. (They forgot EOF was different from error.) But between a fix I landed in CPython a bit ago and some ideas we had on our end, I think we can implement them without picking up the problems from OpenSSL's version.

Likewise I think the way to resolve errors is to add new API to return the symbol name of things. OpenSSL hasn't been responsive to this feature request but I plan to just go ahead and add them on our end.

some manual fixups, more testing and post-fixing will be required.
Python needs to map OpenSSL error codes like ERR_R_INTERNAL_ERROR into strings
like "INTERNAL_ERROR". OpenSSL lacks an API for this, so CPython instead
maintains its own table.

This table is necessarily sensitive to the OpenSSL version and causes issues for
BoringSSL. Rather than maintain our own copy of this table, BoringSSL has APIs
to do the thing CPython actually wants. This patch switches CPython to use them.
To keep the patch small, it doesn't ifdef the err_codes_to_names, etc., fields,
but they are no longer necessary.

See openssl/openssl#19848 and
https://discuss.python.org/t/error-tables-in-the-ssl-module/25431 for context.

BoringSSL API addition:
https://boringssl.googlesource.com/boringssl/+/dbad745811195c00b729efd0ee0a09b7d9fce1d2
BoringSSL was originally missing BIO_FP_TEXT preventing the keylog
callback API from working.  That was added to BoringSSL.
@@ -4542,50 +4582,6 @@ set_sni_callback(PySSLContext *self, PyObject *arg, void *c)
return 0;
}

#if OPENSSL_VERSION_NUMBER < 0x30300000L
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh BoringSSL supports get1_objects. My thinking was we'd just update this ifdef to skip the polyfill in BoringSSL and then leave the rest as-is. get1_objects avoids a race condition in the old CPython code. (get0_objects was added to OpenSSL for CPython, but it turned out to have been the wrong API the whole time. get1_objects was to undo that mistake)

@gpshead gpshead removed their assignment Sep 24, 2024
@awilfox
Copy link

awilfox commented Oct 14, 2024

From the BoringSSL README:

Although BoringSSL is an open source project, it is not intended for general use, as OpenSSL is. We don't recommend that third parties depend upon it. Doing so is likely to be frustrating because there are no guarantees of API or ABI stability.

It seems odd to then add support for building Python against BoringSSL, unless there will be a guarantee of API/ABI stability. If that is indeed the case, which version will be considered stable? That would be the one that CPython should target. If this lands for 3.14, such a theoretical BoringSSL API/ABI would need to be supported until October 2030; what would the maintenance cycle look like? If there is a security issue requiring an API change that is relevant to already-released CPython versions, what would the disclosure process look like?

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

Successfully merging this pull request may close these issues.

3 participants