Skip to content

Commit

Permalink
Explain why #2356 "works". (#2358)
Browse files Browse the repository at this point in the history
The explanation was hard won and a bit embarrasing in outcome. The
original behavior was correct afaict and PBS use on a RedHat system with
custom RedHat OpenSSL config keys should fail, obviously with a much
better error message, and prompt OpenSSL configuration evaluation on the
machine. That said, the behavior is released now and Pex stands behind
it.

It may make sense to add a `--strict-ssl` option or something similar to
restore the old behavior and let the (confusing) error bubble, perhaps
with a pointer to what may be wrong.

This should also serve to close
indygreg/python-build-standalone#207 or at
least give Gregory enough information to decide what to do over in PBS.

Closes the loose ends in #2355.
  • Loading branch information
jsirois authored Feb 10, 2024
1 parent 27c2db2 commit e70d5d6
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 1 deletion.
35 changes: 35 additions & 0 deletions pex/fetcher.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,41 @@ def create(cls, network_configuration=None):

def create_ssl_context(self):
# type: () -> SSLContext

# These shenanigans deserve some explanation, since, in OpenSSL 3.0 anyhow, it is perfectly
# fine to create an SSL Context (`SSL_CTX_new`) in any thread:
#
# It turns out that, in typical use, the CPython ssl module hides OpenSSL configuration
# issues through no real fault of its own. This is due to the fact that an import of the
# `ssl` module, which generally happens in the main thread, triggers, through instantiation
# of the `ssl.Purpose` enum type [^1] a call to OpenSSL's `OBJ_obj2nid` [^2][^3] which
# loads OpenSSL config but throws away the return value; thus hiding errors in config. Since
# the OpenSSL config scheme is to load it at most once per thread, this means subsequent
# OpenSSL call paths in the same thread that imported `ssl` (like the one generated via
# `ssl.create_default_context`) that _do_ check the return value of config loading [^4][^5],
# will not have to load config (since it's been done once already in the thread) and thus
# will not get the chance to check the config load function return value and will thus not
# error. This default behavior is almost certainly bad, since it allows invalid OpenSSL
# configs to go partially read at best. That said, this is the default Python
# single-threaded behavior and, right or wrong, we preserve that here by forcing our SSL
# initialization to happen in the main thread, keeping any OpenSSL misconfiguration silently
# ignored.
#
# The only solace here is that the use cases where OpenSSL config can be bad on a machine
# and the machine still function are narrow. The case we know of, that triggered creation of
# this machinery, involves the combination of a modern PBS Python [^6] (which has a vanilla
# OpenSSL statically linked into the Python binary) running on a RedHat OS that expresses
# custom RedHat configuration keys [^7] in its OpenSSL config. These custom keys are only
# supported by RedHat patches to OpenSSL and cause vanilla versions of OpenSSL to error when
# loading config due to unknown configuration options.
#
# [^1]: https://github.com/python/cpython/blob/5a173efa693a053bf4a059c82c1c06c82a9fa8fb/Lib/ssl.py#L394-L419
# [^2]: https://github.com/python/cpython/blob/5a173efa693a053bf4a059c82c1c06c82a9fa8fb/Modules/_ssl.c#L5534-L5552
# [^3]: https://github.com/openssl/openssl/blob/c3cc0f1386b0544383a61244a4beeb762b67498f/crypto/objects/obj_dat.c#L326-L340
# [^4]: https://github.com/openssl/openssl/blob/c3cc0f1386b0544383a61244a4beeb762b67498f/ssl/ssl_lib.c#L3194-L3212
# [^5]: https://github.com/openssl/openssl/blob/c3cc0f1386b0544383a61244a4beeb762b67498f/ssl/ssl_init.c#L86-L116
# [^6]: https://github.com/indygreg/python-build-standalone/releases/tag/20240107
# [^7]: https://gitlab.com/redhat-crypto/fedora-crypto-policies/-/merge_requests/110/diffs#269a48e71ac25ad1d07ff00db2390834c8ba7596_11_16
asserts.production_assert(
in_main_thread(),
msg=(
Expand Down
3 changes: 2 additions & 1 deletion tests/integration/test_issue_2355.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,8 @@ def test_ssl_context(
"docker",
"run",
"--rm",
"-v" "{pex_project_dir}:/code".format(pex_project_dir=pex_project_dir),
"-v",
"{pex_project_dir}:/code".format(pex_project_dir=pex_project_dir),
"-v",
"{work_dir}:/work".format(work_dir=work_dir),
"-w",
Expand Down

0 comments on commit e70d5d6

Please sign in to comment.