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

salt: 2016.11.5 -> 2017.7.1, patch fix #29020

Merged
merged 1 commit into from Sep 24, 2017
Merged

Conversation

danbst
Copy link
Contributor

@danbst danbst commented Sep 5, 2017

Motivation for this change

Apart from regular update, I've changed the libcrypto patch. It didn't work well with salt-ssh (that code failed on remote machines)

saltstack/salt#43350

ping @aneeshusa for review

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

The libcrypto patch didn't work well with `salt-ssh` (that code failed on
remote machines), so let's make Nix-based library lookup as fallback.

saltstack/salt#43350
@mention-bot
Copy link

@danbst, thanks for your PR! By analyzing the history of the files in this pull request, we identified @aneeshusa, @globin and @dezgeg to be potential reviewers.

@danbst
Copy link
Contributor Author

danbst commented Sep 6, 2017

I do realise now, that patch will work not as intended. It may break salt installs on non-NixOS machines (because it will find the libcrypto impurely, potentially wrong version)

Unfortunately, original Salt doesn't distinguish host and target when running libcrypto code, so the best solution would be to patch it to copy-over libcrypto.so to target machines. That is a tricky way, and I don't think it should be done on NixOS side. On other hand, Salt devs would say Meh for such a change...

if lib:
return cdll.LoadLibrary(lib)
+ else:
+ return cdll.LoadLibrary('@libcrypto@')
Copy link
Member

Choose a reason for hiding this comment

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

What if libcrypto would be in LD_PRELOAD of salt executable on nixos?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, python dll loading library doesn't support LD_PRELOAD and LD_LIBRARY_PATH. @domenkozar has found it's implementation when investigated #7307. We should patch python then...

@aneeshusa
Copy link
Contributor

@danbst Thanks for the patch! I haven't yet gotten a chance to take it for a spin but will try to do so soon.
In the interest of getting the 2017.7.1 update in, can you split the libcrypto patch change to a separate PR? It looks like it is unrelated to the update and might take some bikeshedding.

@aneeshusa
Copy link
Contributor

Also I haven't tried this at all, but maybe something like this this is an easier approach than fixing Python dll loading:

try:
   return cdll.LoadLibrary('@libcrypto@')
except OSError:
    if not lib:
        raise OSError('Cannot locate OpenSSL libcrypto')
    return cdll.LoadLibrary('@libcrypto@')

@aneeshusa
Copy link
Contributor

Or we could add a check for if __file__.startswith('/nix/store'), module the storeDir Nix argument which would need to be propagated through for correctness. I don't think we can reference the final output path (which is safer) without causing a recursion/self-reference error.

@danbst
Copy link
Contributor Author

danbst commented Sep 13, 2017

I'll try the exception catch way, thanks for the pointer. Python except: is almost same as else:, I should never forget that.

But still, I see this as a defect in Salt itself.

  • on master node, rsax931.py uses libcrypto with API version, say, X
  • salt-ssh copies that file to target node, which may run, for example, Ubuntu 12
  • and runs that file. But rsax931.py finds libcrypto with API version, say Y, which doesn't have some features. And fails with some random error

And another problem, now in setup with NixOS on both master and target:

  • on master node, rsax931.py uses /nix/store/XXXX-libcrypto.so
  • salt-ssh copies that file to target node, which runs NixOS
  • and runs that file. But >50% chance that /nix/store/XXXX-libcrypto.so file would be absent there, and real name is /nix/store/YYYY-libcrypto.so.

I don't know whether I should report this upstream. First example is very minor ("update libcrypto on target, what's the problem?"), and second is very Nix specific ("why should we solve your bugs?" and "why the hell would you saltify NixOS?")...

@aneeshusa
Copy link
Contributor

Turns out I already had patched Salt to 2017.7.1 locally and have been using it for a while and forgot, so the Salt update itself LGTM.

In general it's reasonable to run Salt between machines of differing OSes and/or libcrypto versions, so I think the fix here will need to be on the NixOS side. There is already an currently-buggy upstream patch to make find_library return absolute paths (https://bugs.python.org/issue21042) - IMO the best path forward would be to fix that patch up enough to make it work for NixOS. Whatever we do is going to be involved, so please split that to a separate issue or PR so we can get the Salt update merged.

@aneeshusa aneeshusa mentioned this pull request Sep 24, 2017
8 tasks
@joachifm joachifm merged commit 92513b8 into NixOS:master Sep 24, 2017
danbst added a commit to danbst/nixpkgs that referenced this pull request Sep 24, 2017
Fixes NixOS#29020

Patch introduced in PR NixOS#29020 was OK when installing salt on NixOS, but on Linux
behavior was switched to less pure variant. Here I'm returning back original
behavior (use hardcoded path to `libcrypto` by default), but don't disallow dynamic
library lookups (which was essential for `salt-ssh`).

"Easier to Ask Forgiveness than Permission" principle at work.
@danbst danbst deleted the salt-update branch August 2, 2018 09:12
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.

None yet

5 participants