salt: update libcrypto patch

[bayandin]

• Have you followed the guidelines for contributing?
• Have you checked that there aren't other open pull requests for the same formula update/change?
• Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
• Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
• Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

---

This PR updates patch for unversioned libcrypto.
Originally the patch was added as a workaround for saltstack/salt#55084, but it turned out it fixes one installation but breaks another (see saltstack/salt#55084 (comment) and saltstack/salt#55084 (comment)).

[jhass]

How about instead of replacing the line

        lib = find_library('crypto')

with

        lib = '#{Formula["openssl@1.1"].opt_lib}/libcrypto.dylib'

to instead replace

      else:
        lib = find_library('crypto')

with

      elif sys.platform == 'darwin':
        return cdll.LoadLibrary( '#{Formula["openssl@1.1"].opt_lib}/libcrypto.dylib')
      else:
        lib = find_library('crypto')

[bayandin]

Thanks @jhass,

I don't think this will work if a remote machine is Mac and doesn't have homebrew's openssl@1.1.

Anyway, it looks a bit too much for the patch. I would suggest to fix it in salt itself instead of patching homebrew installation.

[jhass]

Probably it won't fix remote macos using salt-ssh without homebrew openssl, true. But it will fix remote linux using salt-ssh, so it makes salt-ssh usable on macos at all, which to me seems quite worthwhile for two added lines.

[bayandin]

@Homebrew/core what do you think about it?
We have several options:

• Remove a patch (which makes salt completely unusable)
• Change the patch as @jhass suggested (which doesn't fix some cases, but should improve the situation)
• Do nothing

Lack of visible actions from salt maintainers regarding saltstack/salt#55084 makes me sad

[MikeMcQuaid]

• Remove a patch (which makes salt completely unusable)

Can't really do this without removing the formula entirely.

• Change the patch as @jhass suggested (which doesn't fix some cases, but should improve the situation)

I'm not sure I understand what this does differently and fixes/doesn't (having read the above)? Can you summarise?

• Do nothing

Similarly, I'm not sure I understand what the status quo bug(s) is? Can you elaborate?

Lack of visible actions from salt maintainers regarding saltstack/salt#55084 makes me sad

My take: we also get issues what we don't fix because we direct people towards making PRs. I'd suggest we (or someone with a stake in this working) submit a PR to fix this rather than an issue.

Alternatively, we remove the (broken) formula and make a merged PR a prerequisite to re-adding.

[jhass]

@MikeMcQuaid

I'm not sure I understand what this does differently and fixes/doesn't (having read the above)? Can you summarise?
Similarly, I'm not sure I understand what the status quo bug(s) is? Can you elaborate?

Salt is a infrastructure automation tool. Normally it works in a master/node kind of way but it also ships a tool called salt-ssh which packages up a minimal node environment and scps that to the target node you want to run operations on. This package is created from just the same files/libraries that are also used by your local installation, in other words patching the local files affects the minimal node environment salt-ssh creates on the target node. Generally such a target node would be a Linux server, not another macos box.

For some operations Salt requires access to libcrypto from OpenSSL. Salt chooses to dynamically load the shared library, for that it needs to find the physical location of it. Given a recent libcrypto is not found in the standard lookup path on macos, the current patch just very naively patches the lookup code, replacing the call to standard python library search with a hardcoded path to the Homebrew installed OpenSSL installation. This so patched file with the hardcoded path then ends up being packaged by salt-ssh and deployed on for example a Linux node, where it fails.

My suggested patch instead of replacing the lookup call, leaves that intact. The surrounding code already makes a special case for sunos environments, my patch simply adds another special case for macos enviroments.

[bayandin]

• Change the patch as @jhass suggested (which doesn't fix some cases, but should improve the situation)

I'm not sure I understand what this does differently and fixes/doesn't (having read the above)? Can you summarise?

There is a salt which loads unversioned libcrypto.dylib (found by find_library('crypto')) and it makes Python crash straight away (on Catalina).
Currently, we have a patch in the formula that changes "unversioned libcrypto.dylib" to /usr/local/opt/openssl@1.1/lib/libcrypto.dylib
There is a salt-ssh part of salt which connects to a remote machine and executes salt remotely. Since a remote machine could be a Linux one, it doesn't have /usr/local/opt/openssl@1.1/lib/libcrypto.dylib and fails.

Changes suggested by @jhass forces salt to use /usr/local/opt/openssl@1.1/lib/libcrypto.dylib only on macOS (and allows to use find_library('crypto') on Linux machines which works fine). It doesn't fix a case when a remote machine is Mac and doesn't have homebrews openssl. The patch it seems made salt work in more amount of cases than now.

• Do nothing

Similarly, I'm not sure I understand what the status quo bug(s) is? Can you elaborate?

I'm not familiar with salt code and salt use cases, but there could be some cases which will be broken by suggested changes. I don't see any so far.

Taking into account all of these, I think that changes suggested by @jhass a worth to apply. Will prepare a PR if no one is against it

[MikeMcQuaid]

Thanks for explanations @jhass and @bayandin!

Taking into account all of these, I think that changes suggested by @jhass a worth to apply. Will prepare a PR if no one is against it

I'm somewhat against it. I think this situation we've ended up in is due to our inreplace not being upstreamed. Could we instead submit a PR with @jhass's patch, use that in the formula and monitor the status of that PR?

[SMillerDev]

I'd say upstreaming the improved patch in #51843 (comment) and using the PR's patch is the way to go.

[jhass]

The patch is specific to homebrew. Unfortunately I don't know enough either what the proper way to do this with Homebrew and Python is for an upstream quality patch. Despite IME upstream isn't super responsive to third party contributors, so it would delay stuff further.

[MikeMcQuaid]

@jhass Sure but it's a starting point (and, as far as I can tell, wouldn't actually do any harm on non-Homebrew setups).

[bayandin]

I had a glance at salt code.

The crash caused by libcrypto = _init_libcrypto() and libcrypto is getting used only in class RSAX931Signer(object): and class RSAX931Verifier(object):.
These classes are used only in salt/crypt.py only if M2Crypto is not installed.

And libcrypto = _init_libcrypto() is getting executed always (during the file importing).

Since Homebrew installs M2Crypto as a resource dependency, this libcrypto is not used anyway.

Will try to fix this unconditional library loading later this week; hope will have time for it.

[SMillerDev]

The patch is specific to homebrew. Unfortunately I don't know enough either what the proper way to do this with Homebrew and Python is for an upstream quality patch.

I'd say homebrew on macOS isn't a very niche setup

Unfortunately I'm familiar with the saltstack speed of accepting PRs so I don't expect the PR to be merged before we pull in the patch, just filed with upstream.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[SMillerDev]

We could use this: saltstack/salt#56958

[bayandin]

We could use this: saltstack/salt#56958

I've updated the patch.
Let's wait for a couple of days for a reply to saltstack/salt#56958 (in case it should be updated) and merge it

[jhass]

Maybe a bit brittle in case the PR is updated? How about https://github.com/saltstack/salt/commit/3dea0e31759b6c2a2c7b46647827a72f7a20dafd.patch

[bayandin]

Maybe a bit brittle in case the PR is updated? How about https://github.com/saltstack/salt/commit/3dea0e31759b6c2a2c7b46647827a72f7a20dafd.patch

If PR will be rebased this commit probably disappear as well.
When we have bottles for these, changes in PR won't affect major amount of installations.

Generally we're not in favour of using patches from unmerged PR (since a lot of things could go wrong). But taking into account this bug exists for a half a year, we could make a small exception I believe

[SMillerDev]

I prefer to point to the PR. If that one is rebased it'll still contain a reference to the work, while the commit will just lose all context

[MikeMcQuaid]

If PR will be rebased this commit probably disappear as well.

It won't.

I think it would be good to link to the PR but don't use it as the URL.

[jhass]

Yes, IME Github keeps the commit objects around, at least more than long enough.

My concern is given the patch is verified with a hash, any updates to the PR easily make this formula not build anymore.

[kaorihinata]

Good morning. I'm the PR author of saltstack/salt#56958. As the contribution itself was directly to salt, I wrote it to favor Apple's dylibs, and as Apple probably isn't going to get rid of them any time soon, this essentially means that Homebrew's dylibs will never be selected. If this is acceptable, then feel free to wait on it, but if you'd like to give users the option of using Homebrew's dylibs (as some formulas do) you may wish to chop up that commit for your own uses.

[bayandin]

Thanks @kaorihinata, hope your patch gets merged soon, meanwhile, I've added a patch to Homebrew from it.

[bayandin]

Thanks everyone!

[BrewTestBot]

🤖 A scheduled task has triggered a merge.