Skip to content

openssh: fix linkOpenSSL=false by linking libxcrypt#307464

Merged
LeSuisse merged 1 commit intoNixOS:masterfrom
tomfitzhenry:ssh-minimal
May 1, 2024
Merged

openssh: fix linkOpenSSL=false by linking libxcrypt#307464
LeSuisse merged 1 commit intoNixOS:masterfrom
tomfitzhenry:ssh-minimal

Conversation

@tomfitzhenry
Copy link
Contributor

@tomfitzhenry tomfitzhenry commented Apr 28, 2024

Description of changes

Fixes openssh to build when linkOpenssl=false. This needed libxcrypt. Possibly this broke after #181764 but wasn't caught by Hydra, since Hydra wasn't building openssh with linkOpenssl=false. I've added a test to cover this functionality.

For more context on glibc/libxcrypt, see https://sourceware.org/legacy-ml/libc-alpha/2017-08/msg01257.html .

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@tomfitzhenry tomfitzhenry requested a review from aszlig April 28, 2024 13:12
@github-actions github-actions bot added the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Apr 28, 2024
@tomfitzhenry
Copy link
Contributor Author

Seeking review from @mweinelt re the introduction of libxcrypt, for when linkOpenssl = false. Does this sound reasonable? Prior to this PR, setting linkOpenssl = false would fail at link time looking for the crypt symbol.

@mweinelt
Copy link
Member

I won't speak to the idea of creating opensshMinimal, but apparently what you found checks out. Either OpenSSL provides the crypto primitives or it falls back to lib(x)crypt.

@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Apr 28, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 101-500 This PR causes between 101 and 500 packages to rebuild on Linux. labels Apr 28, 2024
@tomfitzhenry
Copy link
Contributor Author

tomfitzhenry commented Apr 28, 2024

In hindsight I'm skeptical of introducing opensshMinimal, since:

  • it would have few users, and those users are free/able to define their own openssh package
  • offering a package introduce maintenance/compatibility burden. We can always add opensshMinimal if there's more demand. When in doubt, leave it out.

I'll remove this package from the PR, but keep the test, covering linkOpenssl=false behavior.

@tomfitzhenry tomfitzhenry marked this pull request as draft April 28, 2024 14:00
@tomfitzhenry tomfitzhenry changed the title Introduce opensshMinimal and fix when built with linkOpenssl=false openssh: fix and test linkOpenssl, and introduce withZlib Apr 28, 2024
@tomfitzhenry
Copy link
Contributor Author

tomfitzhenry commented Apr 28, 2024

And I'm skeptical of the value of withZlib too. I had added that parameter so it could be set to false for opensshMinimal (so it was minimal), but since I'm not going to add opensshMinimal, there's less of a need for withZlib. zlib functionality likely occurs post-auth so doesn't have as large an attack surface as, say, OpenSSL/PAM.

I'll remove the zlib parameter too.

@tomfitzhenry tomfitzhenry changed the title openssh: fix and test linkOpenssl, and introduce withZlib openssh: fix and test linkOpenssl parameter Apr 28, 2024
@tomfitzhenry tomfitzhenry marked this pull request as ready for review April 28, 2024 14:16
@tomfitzhenry
Copy link
Contributor Author

PR updated to just fix linkOpenssl=false, and add a test covering that. Much simpler now.

@tomfitzhenry tomfitzhenry changed the title openssh: fix and test linkOpenssl parameter openssh: fix linkOpenSSL=false by linking libxcrypt Apr 28, 2024
Copy link
Member

@LeSuisse LeSuisse left a comment

Choose a reason for hiding this comment

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

Changes look good. Built and tested on Linux x86_64 and aarch64, all tests are passing and nixpkgs-review does not show new failures.

@LeSuisse LeSuisse merged commit e83dd85 into NixOS:master May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 101-500 This PR causes between 101 and 500 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants