[RC1 Sodium] Properly handle libcrypto library search on macOS.

[kaorihinata]

macOS has been steadily moving towards requiring programmers to directly
link against the library version they prefer and forbidding linking against
unversioned libraries. With Catalina this has moved from simply logging a
warning when a programmer links against an unversioned library (typically a
stub) to outright crashing with a pretty clear error.

In an effort to honor Apple's intentions but not completely reimplement
their dlopen methods, this change takes advantage of Apple's library
naming scheme to load the most recent versioned dylib while ignoring the
unversioned stub.

Fixes: #55084

Signed-off-by: Thomas J. Gallen kaori.hinata@gmail.com

What does this PR do?

This PR fixes issue #55084 caused by salt's use of an unversioned dylib stub on macOS which is no longer allowed.

What issues does this PR fix or reference?

Fixes: #55084

Previous Behavior

Any salt application which results in a call to _load_libcrypto would cause a hard crash of the python interpreter follow by a message from macOS indicating that python had attempted to load an unversioned dylib which is no longer supported.

New Behavior

Any salt application which results in a call to _load_libcrypto should now operate normally.

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

• Docs
• Changelog
• Tests written/updated

Regarding docs, besides no longer crashing, there should be no user facing changes that need to be documented.
Regarding the changelog, I didn't see anything about it in the contributing guide, please let me know how I should go about this, or if I should considering how small the change is.
Regarding tests, I don't believe any changes need to be made, as any test that would catch an exception from _load_libcrypto should still catch it.

Commits signed with GPG?

No

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

[kaorihinata]

As an addendum regarding testing, the only tests available for pycrypto are simple functionality tests at this time, and as this involves a hard crash, we may be better served by adding Catalina to CI some time in the near future.

[twangboy]

This needs a test to make sure it's getting the right lib location on OSX

[kaorihinata]

Hi, @twangboy. _load_libcrypto returns a wrapper class from ctypes. How should/can this be tested? Assuming it completes, I can check _name, but what is considered right in this context?

(Edit: Fixed crude wording that probably would make more sense had it not been text.)

[kaorihinata]

I ask because, if the wrong library was loaded, we would hard crash before I ever had the chance to test the result. Any library that loads successfully based on the given globs would essentially be the correct library (at least, correct enough to satisfy any of the other checks in that file.)

[kaorihinata]

I've tested a pytest run without the patch and, as pytest doesn't isolate the python instance it's using to run tests from the tests themselves, a hard crash interrupts the entire testing process. As the testing is currently being done, I don't think we can write a test that would adequately handle this situation.

[bayandin]

If it's possible, I would suggest running tests on macOS Catalina as well (or change ci/py3/macosxmojave to it)

[twangboy]

@kaorihinata You would probably have to split out that code into another function and test that function. Something like _find_libcrypto. Then you could test the return of that function for a valid path.

[kaorihinata]

Correct, but then the code for macOS would be more thoroughly tested than the surrounding code (which I would be reorganizing), and I would be splitting _load_libcrypto into _find_libcrypto and a one liner (ostensibly) _load_libcrypto or folding what's left of _load_libcrypto into the the beginning of _init_libcrypto, and CI does not contain AIX, SmartOS, or SunOS, nor do I possess systems to provide these tests. If you're comfortable with this, then I can split this out this evening.

[kaorihinata]

After exhausting every reasonable avenue to try to make the salt.utils.rsax931 module's _load_libcrypto function testable, it's just not reasonable with the way that salt's tests currently function. You can make the individual test case testable by:

• Splitting _load_libcrypto into it's search component and it's loading component.
• Defining the libcrypto global as None at module scope.
• Modifying _init_libcrypto to initialize libcrypto then store it at module scope (using global.)
• Having RSAX931Signer and RSAX931Verifier call _init_libcrypto in their constructors if libcrypto set to None which makes importing salt.utils.rsax931 no longer fatal if somehow the search component returns the un-versioned library.
• Making the salt.utils.rsax931 test case require checking the search component in its setUp and raising an AssertionError if it's found to have returned that string.
• Sabotaging my patch from commit 1 by forcing the conditional to always come up False, because the glob pattern "/usr/lib/libcrypto.*.dylib" will never match "/usr/lib/libcrypto.dylib".

If you do all of that, then you should now be able to run just the test cases for salt.utils.rsax931 and see how the tests act if the situation you want me to test for does occur. The string will match, it would abort and the rest of the tests in that test case wouldn't run.

The problem after that is that, once you reverse the last item on the list, it's not possible for the search component to ever return "/usr/lib/libcrypto.dylib" again on Darwin/macOS, and even if it could, this wouldn't allow a full test load to run because the first thing the tests do overall (and repeats multiple times later) is launch master and minion instances. This will make use of the salt.utils.rsax931 module before it's been tested, and if somehow you get the search component to return "/usr/lib/libcrypto.dylib", it would crash the entire testing process which would fail CI, but backing out my last 2 commits and just going with the initial patch would have the same effect.

How do we want to proceed?

[cmcmarrow]

@kaorihinata Salt does not expect you to write tests for functions you did not change. You just need to write tests for “_load_libcrypto” and “_find_libcrypto”. In reality you don’t need to mock anything. You can test for what your function returns. For example ”check if your on Windows and then see if “_find_libcrytpto” returns “"libeay32"”. When testing “_load_libcrypot” see if it returns a ctype and it has the members it needs for that platform. For unsupported platforms check that the OSError gets raised.

[kaorihinata]

Sure. The issue is that you cannot test on darwin/macOS for the bug I was fixing. If you're okay with me making tests that cannot test the issue I originally set out to fix then sure, I'll write some general tests.

[kaorihinata]

It would seem like there are some issues with CI, but tests should be completed. I'll await results from some of the non-errored jobs and fix anything that needs to be fixed.

[kaorihinata]

ci/py3/fedora32 and ci/py3/ubuntu2004 seem to be broken. Let me know if you'd like anything done differently.

(Edit: Codecov isn't moving anywhere without mocking to test all supported platforms as well though.)

[weswhet]

@kaorihinata can you pull master again?

[kaorihinata]

@weswhet All green, except for what looks like a CI issue for ci/lint.

[cmcmarrow]

@kaorihinata thanks for the PR it looks good! Should get merged here soon.

[weswhet]

Thanks all!!!!

[weswhet]

Welp this didn't age well. Currently broken on macOS Big Sur. with 3001

[kaorihinata]

Hah! You must be really eager to try it out! I just watched the keynote this morning. It's probably not too hard to fix, but I'd have to find a copy of Big Sur somehow. How is it currently failing? Have they changed the naming scheme for libcrypto finally?

[weswhet]

WWDC is Christmas in June :) yeah /usr/lib/libcrypto* is gone.

[kaorihinata]

Well, good time to test out the fallback then. Homebrew may just refuse to operate right now, but once you install openssl (or openssl 1.1) via Homebrew the patched section should actually pick it up just fine.

Edit: Does it correctly bomb by saying that it can't find libcrypto right now?

[weswhet]

yeah it fails as intended, might need to update this to support the /opt/salt/ paths that are included in the macOS pkg. As not everyone uses homebrew.

[kaorihinata]

Sure. My ability to test it will be limited to Catalina for the time being, but it's a simple 1 liner. Should the order of precedence be:

• Apple's libs (for older versions.)
• Salt's libs (in /opt.)
• Homebrew's libs.

...then?

[weswhet]

yeah, that sounds good, the only other thing I can think of is the pip salt install libs.

[weswhet]

I'm gonna see if I can get the rsax931.py file modified to work on Big Sur.

[kaorihinata]

Last I checked timelib is still broken and requires a manual fix if you want to install via pip (that I include in my venv builder script.) Has any decision been made on how to deal with that since the maintainer has evaporated?

You should be able to to just add the path to the beginning of the second block that checks for non-Apple paths. Should work fine.

[kaorihinata]

I've expanded the package and 3001 doesn't appear to include a dylib that ctypes can consume. Just an ar archive of object files, and I don't believe ctypes can use that. Is that consistent with what you're seeing?

[weswhet]

I was just looking at that as well. I'm seeing the same. I'm looking to see if we can compile opnessl in a manner to include the proper dylib

[weswhet]

ahh looks like my open PR here will include the necessary shared lib. :) #57607

[kaorihinata]

Oh. That's good. Should this do it then?

diff --git a/salt/utils/rsax931.py b/salt/utils/rsax931.py
index c60ef6ba0e..0a1e0d4e3a 100644
--- a/salt/utils/rsax931.py
+++ b/salt/utils/rsax931.py
@@ -61,8 +61,10 @@ def _find_libcrypto():
                 # Sort so as to prefer the newest version.
                 lib = list(reversed(sorted(lib)))
             else:
+                # Find library symlink in Salt locations.
+                lib = glob.glob("/opt/salt/lib/libcrypto.dylib")
                 # Find library symlinks in Homebrew locations.
-                lib = glob.glob("/usr/local/opt/openssl/lib/libcrypto.dylib")
+                lib = lib or glob.glob("/usr/local/opt/openssl/lib/libcrypto.dylib")
                 lib = lib or glob.glob("/usr/local/opt/openssl@*/lib/libcrypto.dylib")
             lib = lib[0] if lib else None
     if not lib:

[kaorihinata]

Here we go. Sorry about that.

Edit: Added changes to tests.
Edit 2: Added the check for Homebrew locations in both places.
Edit 3: Corrected a copy/paste mistake.

diff --git a/salt/utils/rsax931.py b/salt/utils/rsax931.py
index c60ef6ba0e..4a9a589ab9 100644
--- a/salt/utils/rsax931.py
+++ b/salt/utils/rsax931.py
@@ -53,6 +53,14 @@ def _find_libcrypto():
                 else:
                     lib = glob.glob("/opt/freeware/lib/libcrypto.so*")
                 lib = lib[0] if lib else None
+            elif salt.utils.platform.is_darwin():
+                # If no library (or library symlink) was found at all on
+                # Darwin (macOS), attempt to find one in Salt locations.
+                lib = glob.glob("/opt/salt/lib/libcrypto.dylib")
+                # Find library symlinks in Homebrew locations.
+                lib = lib or glob.glob("/usr/local/opt/openssl/lib/libcrypto.dylib")
+                lib = lib or glob.glob("/usr/local/opt/openssl@*/lib/libcrypto.dylib")
+                lib = lib[0] if lib else None
         elif salt.utils.platform.is_darwin():
             # Find versioned libraries in system locations, being careful
             # to avoid the unversioned stub which is no longer permitted.
diff --git a/tests/unit/utils/test_rsax931.py b/tests/unit/utils/test_rsax931.py
index 85d5e4e32f..1b6bf584a0 100644
--- a/tests/unit/utils/test_rsax931.py
+++ b/tests/unit/utils/test_rsax931.py
@@ -181,6 +181,7 @@ class RSAX931Test(TestCase):
         lib_path = _find_libcrypto()
         passed = False
         for i in (
+            "/opt/salt/lib/libcrypto.so",
             "/usr/lib/libcrypto.*.dylib",
             "/usr/local/opt/openssl/lib/libcrypto.dylib",
             "/usr/local/opt/openssl@*/lib/libcrypto.dylib",

[kaorihinata]

I've updated the patch to check Homebrew locations in both instances, and updated the applicable tests as well.

[weswhet]

@kaorihinata thanks! On the newer build /opt/salt/lib/libcrypto.so doesn't exist, so I'm just going to move it over to the new /opt/salt/lib/libcrypto.dylib Sound good? I should have a new PR up shortly

[kaorihinata]

Serves me right for lazily copy/pasting. I'll fix the above snippet just for correctness. Go for it.

[kaorihinata]

As a side note, very technically, if someone deleted the Apple libraries on purpose for some reason (and SIP didn't cover /usr/lib/libcrypto*, which it does), we would also want to check Salt locations in the section that covers macOS pre-Catalina. I added Homebrew there previously for posterity, but it's effectively a dead code path since you would need to disabled SIP to even touch those files, and that's not really a situation we are responsible for supporting, but it's not my project. If you want to cover that situation, then we should probably include the glob call there as well.