Add hardcoded paths to macOS frameworks for Big Sur compatibility. #1905

sheagcraig · 2020-07-16T15:30:47Z

ctypes.util.find_library is checking for the presence of the framework
files, which will fail due to macOS Big Sur's "dynamic linker cache".
Since these files really can't be anywhere else, we just hardcode the
path and load them. (see #1904 )

See: https://developer.apple.com/documentation/macos-release-notes/macos-big-sur-11-beta-release-notes
(relevent excerpt provided below)

This update takes the approach of leaving the existing behavior as is
for macOS 10.8 (lowest supported version) through 10.15.

Allegedly Apple is going to number Big Sur as macOS 11, although right
now the beta returns a version of "10.16", so this code handles both as
being greater than or equal to "10.16".

| New in macOS Big Sur 11 beta, the system ships with a built-in dynamic
linker cache of all system-provided libraries. As part of this change,
copies of dynamic libraries are no longer present on the filesystem.
Code that attempts to check for dynamic library presence by looking for
a file at a path or enumerating a directory will fail. Instead, check
for library presence by attempting to dlopen() the path, which will
correctly check for the library in the cache. (62986286)

ctypes.util.find_library is checking for the presence of the framework files, which will fail due to macOS Big Sur's "dynamic linker cache". Since these files really can't be anywhere else, we just hardcode the path and load them. See: https://developer.apple.com/documentation/macos-release-notes/macos-big-sur-11-beta-release-notes (relevent excerpt provided below) This update takes the approach of leaving the existing behavior as is for macOS 10.8 (lowest supported version) through 10.15. Allegedly Apple is going to number Big Sur as macOS 11, although right now the beta returns a version of "10.16", so this code handles both as being greater than or equal to "10.16". | New in macOS Big Sur 11 beta, the system ships with a built-in dynamic linker cache of all system-provided libraries. As part of this change, copies of dynamic libraries are no longer present on the filesystem. Code that attempts to check for dynamic library presence by looking for a file at a path or enumerating a directory will fail. Instead, check for library presence by attempting to dlopen() the path, which will correctly check for the library in the cache. (62986286)

pquentin

Woah, thanks! How did you notice this issue? Are you running Big Sur and can confirm that this change works?

src/urllib3/contrib/_securetransport/bindings.py

sheagcraig · 2020-07-16T16:20:00Z

I do a lot with Saltstack's macOS working group, and I have a project for making requests Sessions that use the macOS keychain for mutual TLS and cert verification; there have been a lot of projects that use either PyObjC or ctypes directly to get at the macOS frameworks, and the failures are all pretty much due to checking for the existence of the files (which aren't there).

I have a Big Sur 10.16 beta2 machine on my desk that I tested this change out on.

I wanted to get this out there; obviously there's some time.

If anyone wants to direct me in any way, the version check and importing code could probably be cleaned up a bit. I mostly wanted to at least get a working POC out there.

sethmlarson · 2020-07-16T16:21:18Z

Just chiming in to thank you for raising this issue to our attention and for providing a fix. We'll be sure to backport to 1.25-series and get a release out there before Big Sur is released.

sheagcraig · 2020-07-16T16:22:33Z

👍🏻

I hate leaving it with a kind of speculative version check... May be 10.16, may be 11... wait until September!

pquentin · 2020-07-16T16:27:26Z

According to this answer by an Apple engineer, it will be 11. But it's good if it works on the beta too, so we can leave 10.16 until the release.

sethmlarson · 2020-07-16T16:28:32Z

@sheagcraig As long as the last released version we know of is 10.15 I'm fine with leaving the version check to be less restrictive. Sometimes with these things you have to cover all your bases. Another way we could go about this is only doing the version lookup after a failed find_library() call, I'm unsure what the overhead for that is.

sheagcraig · 2020-07-16T16:30:22Z

Cool @pquentin. And yes @sethmlarson, that sounds like a plan.

I'll see if I can cut out some of the repetition. I wasn't sure initially if the linter would be cranky with me for having the version check earlier, so I'll give it a little polish.

sheagcraig · 2020-07-16T16:33:08Z

I have a contact at Apple that I'll ask about this; maybe we could use the hardcoded paths back further due to SIP making it very unlikely that those paths are anything but what we expect.

This should never be anything but these paths. Instead of checking for `os.path.isfile` with `ctypes.util.find_library`, we just try to load with `ctypes.CDLL` and catch the `OSError` if it fails, add context, and reraise.

sheagcraig · 2020-07-16T17:01:29Z

Well try this on for size. So I looked through ctypes.util.find_library, which is really running macholib.dyld.dyld_find(). It follows a search path, performing an os.path.isfile() looking for a path that is actually a file. This obviously won't work on Big Sur.

But the paths have been the same for all of the versions supported by the existing code (10.8+), and shouldn't be anywhere else.

So I just cleaned out a lot of the version conditionals and went straight to trying to load the libraries. If it fails to load for some reason, we catch the OSError, add a more friendly message, and reraise.

I also added a check to make sure we're even on macOS before going any further through the module.

I'm super green on urllib3 proper; I've poked around in the secure transport code a fair bit... But I guess I don't know why this was causing tests to fail on non-macOS, as the new code should have had the same behavior as the old? Regardless, this last couple of commits sidesteps that issue.

sheagcraig · 2020-07-16T17:06:32Z

If this looks good, I'll yank that new test file too, since it basically does nothing now.

sethmlarson

One thought from me, let me know what you think and if you could give it a test locally :)

sethmlarson · 2020-07-16T17:58:48Z

test/contrib/test_securetransport_big_sur.py

@@ -0,0 +1,26 @@
+# -*- coding: utf-8 -*-
+import mock


You can remove this test script now :)

WIth the changes to retain the pre-Big Sur behavior, do it still make sense to take the test out?

src/urllib3/contrib/_securetransport/bindings.py

sheagcraig · 2020-07-16T19:37:14Z

Hah; well I just put your suggested changes in. It's fine like that!

src/urllib3/contrib/_securetransport/bindings.py

sethmlarson · 2020-07-19T00:27:40Z

I went ahead and made some changes myself on the branch:

Fixed the imports so ImportError is raised on non-macOS as before.
Deleted the test script, it's not necessary since CI doesn't support macOS 10.16 yet anyways so we can't test the alternate load_cdll() code path either way. Knowing it works on your system is enough for me to be confident it'll work for most others since the functionality is fairly simple. :)

codecov · 2020-07-19T01:05:40Z

Codecov Report

Merging #1905 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##            master     #1905   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           23        23           
  Lines         2049      2055    +6     
=========================================
+ Hits          2049      2055    +6

Flag	Coverage Δ
#unittests	`99.51% <ø> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/urllib3/util/ssl_.py	`100.00% <0.00%> (ø)`
src/urllib3/connection.py	`100.00% <0.00%> (ø)`
src/urllib3/util/__init__.py	`100.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe1f73b...d57db89. Read the comment docs.

pquentin · 2020-07-19T15:26:06Z

@sethmlarson can't merge myself until you dismiss your review. Thanks for finishing this!

sethmlarson · 2020-07-19T15:28:04Z

Thanks for the review @pquentin :)

…ity. (urllib3#1905) ctypes.util.find_library is checking for the presence of the framework files, which will fail due to macOS Big Sur's "dynamic linker cache". Since these files really can't be anywhere else, we just hardcode the path and load them. See: https://developer.apple.com/documentation/macos-release-notes/macos-big-sur-11-beta-release-notes (relevent excerpt provided below) This update takes the approach of leaving the existing behavior as is for macOS 10.8 (lowest supported version) through 10.15. Allegedly Apple is going to number Big Sur as macOS 11, although right now the beta returns a version of "10.16", so this code handles both as being greater than or equal to "10.16". Co-authored-by: Seth Michael Larson <sethmichaellarson@gmail.com>

pquentin · 2020-07-21T22:16:20Z

Big Sur will be both 10.16 and 11.0, depending on context: https://eclecticlight.co/2020/07/21/big-sur-is-both-10-16-and-11-0-its-official/

…ity. (#1905) ctypes.util.find_library is checking for the presence of the framework files, which will fail due to macOS Big Sur's "dynamic linker cache". Since these files really can't be anywhere else, we just hardcode the path and load them. See: https://developer.apple.com/documentation/macos-release-notes/macos-big-sur-11-beta-release-notes (relevent excerpt provided below) This update takes the approach of leaving the existing behavior as is for macOS 10.8 (lowest supported version) through 10.15. Allegedly Apple is going to number Big Sur as macOS 11, although right now the beta returns a version of "10.16", so this code handles both as being greater than or equal to "10.16". Co-authored-by: Seth Michael Larson <sethmichaellarson@gmail.com>

…rllib3#1905) * Add hardcoded paths to macOS frameworks for Big Sur compatibility. ctypes.util.find_library is checking for the presence of the framework files, which will fail due to macOS Big Sur's "dynamic linker cache". Since these files really can't be anywhere else, we just hardcode the path and load them. See: https://developer.apple.com/documentation/macos-release-notes/macos-big-sur-11-beta-release-notes (relevent excerpt provided below) This update takes the approach of leaving the existing behavior as is for macOS 10.8 (lowest supported version) through 10.15. Allegedly Apple is going to number Big Sur as macOS 11, although right now the beta returns a version of "10.16", so this code handles both as being greater than or equal to "10.16". | New in macOS Big Sur 11 beta, the system ships with a built-in dynamic linker cache of all system-provided libraries. As part of this change, copies of dynamic libraries are no longer present on the filesystem. Code that attempts to check for dynamic library presence by looking for a file at a path or enumerating a directory will fail. Instead, check for library presence by attempting to dlopen() the path, which will correctly check for the library in the cache. (62986286) * Raise an OSError if not on macOS and trying to import securetransport * Just use hardcoded paths for Security and CoreFoundation libs. This should never be anything but these paths. Instead of checking for `os.path.isfile` with `ctypes.util.find_library`, we just try to load with `ctypes.CDLL` and catch the `OSError` if it fails, add context, and reraise. * Updated to preserve pre-10.16 behavior. * Bail early on non-macOS. * Restore find_library import, OSError -> ImportError * ImportError * OSError * Delete test_securetransport_big_sur.py Co-authored-by: Seth Michael Larson <sethmichaellarson@gmail.com>

sheagcraig changed the title ~~Add hardcoded paths to macOS frameworks for Big Sur compatibility. (#1904)~~ Add hardcoded paths to macOS frameworks for Big Sur compatibility. Jul 16, 2020

sheagcraig mentioned this pull request Jul 16, 2020

macOS Big Sur dynamic linker cache causes contrib/securetransport to raise an ImportError #1904

Closed

pquentin requested changes Jul 16, 2020

View reviewed changes

src/urllib3/contrib/_securetransport/bindings.py Show resolved Hide resolved

sheagcraig added 2 commits July 16, 2020 12:55

Raise an OSError if not on macOS and trying to import securetransport

c3d5e56

Just use hardcoded paths for Security and CoreFoundation libs.

895f11f

This should never be anything but these paths. Instead of checking for `os.path.isfile` with `ctypes.util.find_library`, we just try to load with `ctypes.CDLL` and catch the `OSError` if it fails, add context, and reraise.

sethmlarson requested changes Jul 16, 2020

View reviewed changes

Updated to preserve pre-10.16 behavior.

6508362

pquentin reviewed Jul 17, 2020

View reviewed changes

src/urllib3/contrib/_securetransport/bindings.py Outdated Show resolved Hide resolved

sheagcraig and others added 5 commits July 17, 2020 14:41

Bail early on non-macOS.

0f8d2fa

Restore find_library import, OSError -> ImportError

f63062b

ImportError

5efec6a

OSError

43416bc

Delete test_securetransport_big_sur.py

d57db89

pquentin approved these changes Jul 19, 2020

View reviewed changes

sethmlarson merged commit 6bd33f6 into urllib3:master Jul 19, 2020

klfaustbe mentioned this pull request Nov 21, 2020

MRN Mac Z-H-Sun/CS_CCME_Posts#3

Closed

mateuszr mentioned this pull request Jan 21, 2021

No recorder/browser window drawn on MacOS BigSur 11.1 Intel webrecorder/webrecorder-desktop#63

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add hardcoded paths to macOS frameworks for Big Sur compatibility. #1905

Add hardcoded paths to macOS frameworks for Big Sur compatibility. #1905

sheagcraig commented Jul 16, 2020 •

edited

pquentin left a comment

sheagcraig commented Jul 16, 2020

sethmlarson commented Jul 16, 2020

sheagcraig commented Jul 16, 2020

pquentin commented Jul 16, 2020

sethmlarson commented Jul 16, 2020

sheagcraig commented Jul 16, 2020

sheagcraig commented Jul 16, 2020

sheagcraig commented Jul 16, 2020

sheagcraig commented Jul 16, 2020

sethmlarson left a comment

sethmlarson Jul 16, 2020

sheagcraig Jul 17, 2020

sheagcraig commented Jul 16, 2020

sethmlarson commented Jul 19, 2020

codecov bot commented Jul 19, 2020 •

edited

pquentin commented Jul 19, 2020

sethmlarson commented Jul 19, 2020

pquentin commented Jul 21, 2020

Add hardcoded paths to macOS frameworks for Big Sur compatibility. #1905

Add hardcoded paths to macOS frameworks for Big Sur compatibility. #1905

Conversation

sheagcraig commented Jul 16, 2020 • edited

pquentin left a comment

Choose a reason for hiding this comment

sheagcraig commented Jul 16, 2020

sethmlarson commented Jul 16, 2020

sheagcraig commented Jul 16, 2020

pquentin commented Jul 16, 2020

sethmlarson commented Jul 16, 2020

sheagcraig commented Jul 16, 2020

sheagcraig commented Jul 16, 2020

sheagcraig commented Jul 16, 2020

sheagcraig commented Jul 16, 2020

sethmlarson left a comment

Choose a reason for hiding this comment

sethmlarson Jul 16, 2020

Choose a reason for hiding this comment

sheagcraig Jul 17, 2020

Choose a reason for hiding this comment

sheagcraig commented Jul 16, 2020

sethmlarson commented Jul 19, 2020

codecov bot commented Jul 19, 2020 • edited

Codecov Report

pquentin commented Jul 19, 2020

sethmlarson commented Jul 19, 2020

pquentin commented Jul 21, 2020

sheagcraig commented Jul 16, 2020 •

edited

codecov bot commented Jul 19, 2020 •

edited