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

nixos/sshd: fixes validation for cross-compilation #62853

Merged
merged 1 commit into from Jun 15, 2019

Conversation

samueldr
Copy link
Member

@samueldr samueldr commented Jun 8, 2019

Motivation for this change

Make cross-compilation work for sd-image without modifications in Nixpkgs.

This is the only bit which cannot be tackled through options or overlays.

I do not like the solution. Do not merge before one of our cross expert chimed in with another solution.

The issue here is that, AFAIUI, cfgc.package will not resolve to the right nativeBuildInput, though I do not understand why exactly. This is not as simple as testing with any other sshd, ideally you want to test with the one that will run the configuration. Thus, it needs to be the one configured in the option, but its cross-compiled cousin.

Using @yegortimoshenko's suggestion.

This was tested by building sd_image through cross-compilation.

Things done
  • ☑️ Tested using sandboxing (nix.useSandbox on NixOS, or option 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 nix-review --run "nix-review wip"
  • 🔲 Tested execution of all binary files (usually in ./result/bin/)
  • 🔲 Determined the impact on package closure size (by running nix path-info -S before and after)
  • 🔲 Assured whether relevant documentation is up to date
  • ☑️ Fits CONTRIBUTING.md.

cc @matthewbauer @Ericson2314 as cross-compilation experts

@Ma27
Copy link
Member

Ma27 commented Jun 8, 2019

First of all sorry for the breakage I caused!

In fact @fpletz and I discussed about this in #58718.

The issue here is that, AFAIUI, cfgc.package will not resolve to the right nativeBuildInput, though I do not understand why exactly.

Same here, we assumed that nativeBuildInputs would solve the problem. I'd like to wait for the suggestions of somebody with more experience in cross-compilation first, but I'm happy to help finding an appropriate solution :)

On a side-note: there are some other modules like services.prometheus which use a similar approach to validate their configs, I guess they need to be fixed as well.

This is not as simple as testing with any other sshd, ideally you want to test with the one that will run the configuration

Naively asked: why? In other words: which sort of issues do you have in mind?

The current validation only ensures the syntactical correctness of the resulting sshd configuration, so we shouldn't experience too much behavioral differences in that case, right?

@samueldr
Copy link
Member Author

samueldr commented Jun 9, 2019

The current validation only ensures the syntactical correctness of the resulting sshd configuration, so we shouldn't experience too much behavioral differences in that case, right?

I assumed that it would validate that options are set in valid ways, and that options are valid. Meaning that those cases could be problematic:

  • testing a configuration with new settings on an old sshd
  • new sshd with backwards incompatible options
  • sshd fork with misc. incompatible changes

If it only checks syntax it might be less of an issue, assuming at that point that an incompatible syntax using Open SSH sshd fork would likely not the module anyway.


-t Test mode. Only check the validity of the configuration file and sanity of the keys.
This is useful for updating sshd reliably as configuration options may change.

This is not obvious whether it checks validity of options and values or only that it can parse the file.

@lukateras
Copy link
Member

lukateras commented Jun 11, 2019

In native mode, this patch causes:

building '/nix/store/d4mfzz0a60abcw8370fbm80nxmkr7b3r-etc.drv'...
ln: invalid option -- 'N'
Try 'ln --help' for more information.

Looking at it, I'm not sure why.

Copy link
Member

@lukateras lukateras left a comment

Choose a reason for hiding this comment

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

I would've went for cfgc.package -> pkgs.buildPackages.openssh in nativeBuildInputs.

@Ericson2314
Copy link
Member

Ericson2314 commented Jun 11, 2019

The splicing that makes nativeBuildInputs do the trick only works with callPackage, so it isn't available here.

I too would just unconditionally do pkgs.buildPackages.openssh----is that that important to customize the build-time dependency? Otherwise one can make a config.programs.ssh-for-build.

@lukateras
Copy link
Member

Middle ground would be something like:

if stdenv.buildPlatform == stdenv.hostPlatform
  then [ cfgc.package ]
  else [ pkgs.buildPackages.openssh ]

That would address both potential closure size concerns and fix cross-compilation (if only a bit verbose).

@samueldr samueldr force-pushed the fix/sshd-cross-compile-issue branch 2 times, most recently from 30cd4ac to 0bb0ff1 Compare June 15, 2019 02:54
@samueldr samueldr force-pushed the fix/sshd-cross-compile-issue branch from 0bb0ff1 to 317d67a Compare June 15, 2019 02:54
@samueldr samueldr changed the title WIP: nixos/sshd: disable validation for cross-compilation nixos/sshd: disable validation for cross-compilation Jun 15, 2019
@samueldr samueldr changed the title nixos/sshd: disable validation for cross-compilation nixos/sshd: fixes validation for cross-compilation Jun 15, 2019
@samueldr samueldr force-pushed the fix/sshd-cross-compile-issue branch from 317d67a to 861bbbc Compare June 15, 2019 04:56
@lukateras lukateras merged commit d089f23 into NixOS:master Jun 15, 2019
@lukateras
Copy link
Member

lukateras commented Jun 15, 2019

Thank you so much for working on this! Tested here too.

@samueldr samueldr deleted the fix/sshd-cross-compile-issue branch June 15, 2019 16:05
@aanderse aanderse mentioned this pull request Jan 16, 2021
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants