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

ssh: add crypto options #41798

Open
wants to merge 1 commit into
base: master
from

Conversation

@Izorkin
Copy link
Contributor

commented Jun 10, 2018

Motivation for this change

Add custom options - PreferredAuthentications, KexAlgorithms, Ciphers, MACs.

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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@grahamc

This comment has been minimized.

Copy link
Member

commented Jun 10, 2018

Is there a nice benefit to using the structured form here, compared to the extraConfig?

@Izorkin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2018

It seems to me that it looks better. Also, the user can see the description here https://nixos.org/nixos/options.html#programs.ssh. and apply it in your config

@oxij

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2018

@Izorkin Izorkin force-pushed the Izorkin:ssh-custom branch Jun 10, 2018

@Izorkin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2018

@oxij variant with kexAlgorithms set to null not worked, error -
error: The option value programs.ssh.kexAlgorithms' in /home/test/src_nix/nixpkgs/nixos/modules/programs/ssh.nix' is not of type list of strings'.`
I do not plan to add all the options )

@oxij

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2018

@Izorkin Izorkin force-pushed the Izorkin:ssh-custom branch 3 times, most recently Jun 10, 2018

@oxij oxij referenced this pull request Jun 14, 2018
2 of 8 tasks complete
@qolii

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2018

But what is your criteria for adding something

Honest question: what are ever the criteria? Is there a guideline? It seems like every module in the whole OS is subject to an arbitrary decision on how much of the configuration space to support "properly". Most modules of significant complexity seem not to support it all.

I've always seen deferring of module options to an extraConfig to be a necessary (due to manpower), but ultimately undesirable, practice, and thought that the more of the configuration space is supported, the better. Especially for something like sshd, where these things are quite stable; once the module code is in place, it should keep working and hopefully not need much maintenance.

What are the downsides? Worry about maintenance? Or does it bog down Hydra or something too?

@oxij

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2018

@Izorkin Izorkin force-pushed the Izorkin:ssh-custom branch 2 times, most recently to 8791dab Jul 21, 2018

@Izorkin Izorkin changed the title ssh: add custom options ssh: add crypto options Jul 21, 2018

@coretemp

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2018

If you write a script which translates sshd options into NixOS options, I think that would be something more people would welcome (at least, I would), but just adding code duplication (the ssh people already wrote that code) doesn't solve anything.

It's not as if NixOS would continue to support module options if the underlying tool would have removed it. As such for options for which NixOS cannot guarantee its continued support, manually created options don't make sense.

@aanderse

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

@Izorkin considering a few of these options have been implemented now and it seems unlikely the others will can we close this PR?

@Izorkin

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

@aanderse, which options have been implemented? I've checked the code, and I don't see any changes to implement the options.

@aanderse

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

@Izorkin

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

This optins in openssh server. Such options are available for the openssh client.
https://man.openbsd.org/sshd_config#KexAlgorithms
https://man.openbsd.org/ssh_config#KexAlgorithms

@aanderse

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

@Izorkin ah sorry I only skimmed over this PR and didn't notice it was for the client software. Do you have any interest to rework this to include only the crypto options as @oxij mentioned?

@Izorkin Izorkin force-pushed the Izorkin:ssh-custom branch from 8791dab to 8b0a607 May 22, 2019

@Izorkin

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

@aanderse this variant normall?
Or preferredAuthentications remove?

@aanderse

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

I'm not sure if @oxij just meant kexAlgorithms and ciphers but that's what I assume.

@Izorkin Izorkin force-pushed the Izorkin:ssh-custom branch from 8b0a607 to 7de9d0a May 22, 2019

@Izorkin

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

preferredAuthentications removed.

@nixos-discourse

This comment has been minimized.

Copy link

commented Jun 11, 2019

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/15

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.