ssh: custom config key types #40686

Izorkin · 2018-05-17T15:50:49Z

Motivation for this change

A possibility is necessary to configure active key types

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.

dotlambda · 2018-05-18T10:44:07Z

nixos/modules/programs/ssh.nix

@@ -190,8 +212,8 @@ in
        ForwardX11 ${if cfg.forwardX11 then "yes" else "no"}

        # Allow DSA keys for now. (These were deprecated in OpenSSH 7.0.)


This comment needs to be moved. But we should think about dropping DSA support by default.

Izorkin · 2018-05-18T10:50:41Z

That's normal?

dotlambda · 2018-05-18T10:52:21Z

nixos/modules/programs/ssh.nix

@@ -62,13 +62,37 @@ in
        '';
      };

+      pubkeyAcceptedKeyTypes  = mkOption {
+        type = types.str;
+        default = "+ssh-dss";


I'd put the comment above this line.

dotlambda · 2018-05-18T10:52:30Z

nixos/modules/programs/ssh.nix

+
+      hostKeyAlgorithms  = mkOption {
+        type = types.str;
+        default = "+ssh-dss";


and above this one

dotlambda · 2018-05-18T10:52:46Z

What's normal?

Izorkin · 2018-05-18T11:01:29Z

Moved comment

antifob · 2018-05-24T16:06:13Z

DSA keys have been deprecated upstream. Why does this commit defaults to re-supporting them?

edit: missed context. They should be disabled, though.

dotlambda · 2018-05-24T16:13:15Z

I'm also in favor of removing support for DSA keys by default. We can use the examples to show how to re-add support.
cc @edolstra who re-added support in a7b7ac8 and 3fb1708.

See also 401782c.

dotlambda

Let's continue the discussion in the appropriate issue/PR: #33381, #36937

This PR doesn't change anything if the new options are not used, so it should be ready to merge.

dotlambda · 2018-05-24T16:40:28Z

cc @aneeshusa

aneeshusa

Instead of taking a raw string, let's take a list of key types, and have the module take care of concatenting them with commas. I also think it would be nice to have a separate option to handle the potenial +/- potential prefixes.

Izorkin · 2018-05-25T08:54:30Z

Updated commit.

grahamc · 2018-06-07T13:49:28Z

nixos/modules/programs/ssh.nix

+      pubkeyAcceptedKeyTypes = mkOption {
+        type = types.listOf types.str;
+        # Allow DSA keys for now. (These were deprecated in OpenSSH 7.0.)
+        default = [


I think it may be better to not list all the types, but stay with the +ssh-dss instead. This way we get the default recommended options from the ssh package and don't have to remember to update this list when the recommendations change. Does that seem sensible?

To me it does. I don't see any advantage in having to list the default types if you just want to add some.

I also strongly prefer the OpenSSL defaults. I doubt we will keep track of this tightly.

I would also not want to introduce something that upstream has been deprecated as that could mean we can not take it out again. See the issue about deprecating the dsa support.

I would argue for removing some of the doubtful nistp algorithms but we probably can not be sure nobody relies on them.. That is a very sad story.

Changed to next variant

Izorkin · 2018-06-09T07:55:14Z

сс @dotlambda @grahamc

LnL7 · 2018-07-21T09:03:50Z

nixos/modules/programs/ssh.nix

-        # Allow DSA keys for now. (These were deprecated in OpenSSH 7.0.)
-        PubkeyAcceptedKeyTypes +ssh-dss
-        HostKeyAlgorithms +ssh-dss
+        ${optionalString (cfg.pubkeyAcceptedKeyTypes != null) ''PubkeyAcceptedKeyTypes ${concatStringsSep "," cfg.pubkeyAcceptedKeyTypes}''}


These options can't be null.

infinisil · 2018-07-21T09:06:30Z

nixos/modules/programs/ssh.nix

+      };
+
+      hostKeyAlgorithms = mkOption {
+        type = types.nullOr (types.listOf types.str);


The type of those can be just types.listOf types.str, then an empty list replaces the null case.

GrahamcOfBorg added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels May 17, 2018

Izorkin force-pushed the ssh branch from 771b774 to c2c60f8 Compare May 17, 2018 17:15

dotlambda reviewed May 18, 2018

View reviewed changes

Izorkin force-pushed the ssh branch from c2c60f8 to d4492e1 Compare May 18, 2018 10:49

dotlambda reviewed May 18, 2018

View reviewed changes

Izorkin force-pushed the ssh branch 2 times, most recently from 93117bf to 22d409c Compare May 18, 2018 17:06

dotlambda mentioned this pull request May 24, 2018

ssh: remove DSA key support by default #41033

Closed

dotlambda approved these changes May 24, 2018

View reviewed changes

aneeshusa reviewed May 25, 2018

View reviewed changes

Izorkin force-pushed the ssh branch 2 times, most recently from 39a71a7 to be7b414 Compare May 25, 2018 08:53

Izorkin force-pushed the ssh branch 2 times, most recently from f56d2ec to 9a26de8 Compare May 25, 2018 09:24

Izorkin changed the title ~~ssh: custom config key types~~ ssh: custom config key types and authentications method May 25, 2018

Izorkin force-pushed the ssh branch from 2a0d520 to dcb85fc Compare May 25, 2018 09:46

grahamc reviewed Jun 7, 2018

View reviewed changes

Izorkin force-pushed the ssh branch 2 times, most recently from 57401ef to a34407d Compare June 9, 2018 07:45

Izorkin force-pushed the ssh branch from a34407d to 73d3adc Compare June 10, 2018 17:45

Izorkin changed the title ~~ssh: custom config key types and authentications method~~ ssh: custom config key types Jun 10, 2018

aneeshusa approved these changes Jun 12, 2018

View reviewed changes

LnL7 reviewed Jul 21, 2018

View reviewed changes

infinisil requested changes Jul 21, 2018

View reviewed changes

ssh: custom config key types

05bc5fe

Izorkin force-pushed the ssh branch from 73d3adc to 05bc5fe Compare July 21, 2018 09:28

LnL7 approved these changes Jul 21, 2018

View reviewed changes

LnL7 merged commit e2444a4 into NixOS:master Jul 21, 2018

Izorkin deleted the ssh branch July 21, 2018 10:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ssh: custom config key types #40686

ssh: custom config key types #40686

Izorkin commented May 17, 2018 •

edited

dotlambda May 18, 2018

Izorkin commented May 18, 2018

dotlambda May 18, 2018

dotlambda May 18, 2018

dotlambda commented May 18, 2018

Izorkin commented May 18, 2018 •

edited

antifob commented May 24, 2018 •

edited

dotlambda commented May 24, 2018 •

edited

dotlambda left a comment •

edited

dotlambda commented May 24, 2018

aneeshusa left a comment

Izorkin commented May 25, 2018

grahamc Jun 7, 2018

dotlambda Jun 7, 2018

andir Jun 7, 2018

andir Jun 7, 2018

Izorkin Jun 8, 2018

Izorkin commented Jun 9, 2018

LnL7 Jul 21, 2018

infinisil Jul 21, 2018

		@@ -190,8 +212,8 @@ in
		ForwardX11 ${if cfg.forwardX11 then "yes" else "no"}

		# Allow DSA keys for now. (These were deprecated in OpenSSH 7.0.)

ssh: custom config key types #40686

ssh: custom config key types #40686

Conversation

Izorkin commented May 17, 2018 • edited

Motivation for this change

Things done

Choose a reason for hiding this comment

Izorkin commented May 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dotlambda commented May 18, 2018

Izorkin commented May 18, 2018 • edited

antifob commented May 24, 2018 • edited

dotlambda commented May 24, 2018 • edited

dotlambda left a comment • edited

Choose a reason for hiding this comment

dotlambda commented May 24, 2018

aneeshusa left a comment

Choose a reason for hiding this comment

Izorkin commented May 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Izorkin commented Jun 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Izorkin commented May 17, 2018 •

edited

Izorkin commented May 18, 2018 •

edited

antifob commented May 24, 2018 •

edited

dotlambda commented May 24, 2018 •

edited

dotlambda left a comment •

edited