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.

Sign up for GitHub

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

Jump to bottom

salt: Prefer to load Nix libcrypto #33971

Closed
wants to merge 1 commit into from
Closed

salt: Prefer to load Nix libcrypto #33971

wants to merge 1 commit into from

Conversation

aneeshusa
Copy link
Contributor

Prefer to load the static Nix version,
but still fallback to the normal loading pathway if needed.

Being more aggressive about using the Nix dependency
improves stability and means Salt works more often,
namely in cases where find_library/LoadLibrary don't work
even though we have the Nix libcrypto available.

We still need to keep the fallback for usage with salt-ssh.

Motivation for this change

Better fix than what was in #29020.
See also #29723 cc @danbst.

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
    • other Linux distributions
  • 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.

@aneeshusa
salt: Prefer to load Nix libcrypto
df8d2ff 
Prefer to load the static Nix version,
but still fallback to the normal loading pathway if needed.

Being more aggressive about using the Nix dependency
improves stability and means Salt works more often,
namely in cases where find_library/LoadLibrary don't work
even though we have the Nix libcrypto available.

We still need to keep the fallback for usage with salt-ssh.
@aneeshusa aneeshusa mentioned this pull request Jan 17, 2018
Closed
8 tasks
@danbst danbst mentioned this pull request Jan 17, 2018
Closed
@danbst
Copy link
Contributor

danbst commented Jan 17, 2018

While I'm all for this patch, I still don't like that master Salt update, even without version bump, (Nix Salt) can cause minion side breakages (when using Nix minion Salt). This probably doesn't happen often because of delayed garbage collection on minions, but if minions remove garbage ASAP, the breakage threat is real.

Also, now I think this isn't Salt issue (Salt can compare library versions for integrity check), and it isn't Nix issue (it does it's job - perfect dependency specification).

The patch in #33972 looks interesting, but which libcrypto version does it return?

@FRidh
Copy link
Member

FRidh commented Jan 17, 2018 •
edited

Why are you not hardcoding the path to the library? Nevermind

@danbst
Copy link
Contributor

danbst commented Jan 17, 2018

@FRidh there is hardcode + return cdll.LoadLibrary('@libcrypto@')

@aneeshusa
Copy link
Contributor Author

I don't see a good way to have Salt copy over the whole libcrypto closure, since the target machine may have a different architecture, so I thnk it's reasonable to fallback to impure libcrypto on the target.

@danbst agreed that this patch isn't perfect, but maybe we should just say it doesn't make sense to use salt-ssh to minions with Nix Salt already installed since it reduces purity?

Or maybe extend upstream Salt to learn how to re-use an existing Salt installation,
instead of always copying over modules. They do some magic bits with wrapping certain functions that normally rely on the fileserver since during SSH they instead need to preload files with scp, but that shouldn't affect this crypto module, so maybe a combination of use preinstalled-version + have wrappers would work best.

I'll need to double-check which version of libcrypto #33972 returns (build closure or runtime libcrypto), but I can say it's one that seems to work for me :)
For most cases (not salt-ssh to a Nix minion that GC'd or doesn't have the client's hardcoded libcrypto) IIRC either patch should be sufficient

@danbst
Copy link
Contributor

danbst commented Jan 17, 2018

I'm talking about setup when Nix Salt master makes commands via salt-ssh to NixOS minion host. Without #33972 it would be pain to do this (but probably better tool here is NixOps).

But I'm all for this patch, it is simpler than my previous one.

@FRidh
Copy link
Member

FRidh commented Jan 18, 2018

Thanks for the explanation. I don't know what Salt is, but considering this is quite specific/special behavior, I think this should be documented in the expression.

matthewbauer
matthewbauer requested changes Apr 21, 2018
View reviewed changes
Copy link
Member

@matthewbauer matthewbauer left a comment

Choose a reason for hiding this comment

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

See @FRidh's comments

@mmahut
Copy link
Member

mmahut commented Aug 3, 2019

What is the status of this pull request?

@mmahut
Copy link
Member

mmahut commented Oct 5, 2019

Closing due to lack of activity, feel free to re-open this if needed.

@mmahut mmahut closed this Oct 5, 2019
@aneeshusa aneeshusa deleted the salt-prefer-nix-libcrypto branch January 7, 2022 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewbauer matthewbauer matthewbauer requested changes

@danbst danbst danbst approved these changes

Assignees
No one assigned
Labels
6.topic: python 9.needs: documentation 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@aneeshusa @danbst @FRidh @mmahut @matthewbauer @GrahamcOfBorg