ssh: add generic Match support for matchBlocks

[jficz]

Description

Allow creating actual match block in ssh config using Match instead of Host.

This is a preliminary Match support which doesn't try to exhaustively define the Match rules but rather takes a more conservative approach and lets users define their own verbatim Match rules as a string.

It's better than nothing but it's not an ideal solution (yet). It works well for the author though.

Fixes #2769

Checklist

• [x] Change is backwards compatible.

• [x] Code formatted with ./format.

• [x] Code tested through nix-shell --pure tests -A run.all.

• [x] Test cases updated/added. See example.

• [x] Commit messages are formatted like

  {component}: {description}
  
  {long description}
  

  See CONTRIBUTING for more information and recent commit messages for examples.

[esclear]

I tried / am trying to do the same in #2370 (though I started with a narrower case there). #567 was along those lines as well. I'll look at this later and may have some input.

[jficz]

I took inspiration in the two but took a different approach so I could "ship a fully functional prototype" so to speak.

[esclear]

As the match condition overwrites the host condition, I think it should cause an error or at least a warning, if both, host and match, are defined.

As I don't have a strong preference of specifying the match condition manually over specifying the parts in a set and this would solve my problems / applications, I'd like to see this land. However, I don't know how others feel about this and have not received feedback about this in #2370 for a long time :/

[SuperSandro2000]

As the match condition overwrites the host condition, I think it should cause an error or at least a warning, if both, host and match, are defined.

No, there shouldn't be any warnings when that is done. ssh_config does not consider this and it is totally fine to mix and match multiple Host and Match. The ssh_config parser is very simple and goes from top to bottom. If a setting is set it can no longer be changed later but no errors are emitted. In a complex ssh_config you could have some defaults at the end of the file which are overwritten by host specific settings earlier.

This means we don't need complex logic around this and worst case it breaks configs which are working like intended.

[cptMikky]

I think he meant that both Match and Host are set for a single entry in nix home configuration.

That would make sense but I couldn't find a way to make that work without breaking other logic - namely default Host by entry name (https://github.com/nix-community/home-manager/pull/2992/files#diff-a58732e56068a41ed3b90c4069d4c4934a01dd8f1372633f8aae82995ee14495R289).

That's why I went with CSS-like priorities - the more specific the settings the higher priority it gets.

[esclear]

@jficz is correct about my concern being what happens when both of the host and match options are set for one match block.

I don't have much experience with defining options myself, but a fallback to the dagName to use as the Host, if neither host nor match options are set, seems very doable to me. I may even try to build that, though it almost certainly won't be very idiomatic.

[esclear]

I'd like to propose this:

diff --git a/modules/programs/ssh.nix b/modules/programs/ssh.nix
index 00616d5a..0d089927 100644
--- a/modules/programs/ssh.nix
+++ b/modules/programs/ssh.nix
@@ -285,15 +285,14 @@ let
         description = "Extra configuration options for the host.";
       };
     };
-
-    config.host = mkDefault dagName;
   });

-  matchBlockStr = cf: concatStringsSep "\n" (
+  matchBlockStr = key: cf: concatStringsSep "\n" (
     let
-      matchHead = if cf.match != null then
-        "Match ${cf.match}"
-      else "Host ${cf.host}";
+      hostOrDagName = if cf.host != null then cf.host else key;
+      matchHead = if cf.match != null
+        then  "Match ${cf.match}"
+        else "Host ${hostOrDagName}";
     in [ "${matchHead}" ]
     ++ optional (cf.port != null)            "  Port ${toString cf.port}"
     ++ optional (cf.forwardAgent != null)    "  ForwardAgent ${lib.hm.booleans.yesNo cf.forwardAgent}"
@@ -506,7 +505,7 @@ in
         ++ (optional (cfg.includes != [ ]) ''
           Include ${concatStringsSep " " cfg.includes}
         '')
-        ++ (map (block: matchBlockStr block.data) matchBlocks)
+        ++ (map (block: matchBlockStr block.name block.data) matchBlocks)
       )}

       Host *
@@ -522,5 +521,9 @@ in

         ${replaceStrings ["\n"] ["\n  "] cfg.extraConfig}
     '';
+
+    warnings = mapAttrsToList
+        (n: v: "The SSH config match block `programs.ssh.matchBlocks.${n}` sets both of the host and match options.\nThe match option takes precedence.")
+        (filterAttrs (n: v: v.data.host != null && v.data.match != null) cfg.matchBlocks);
   };
 }

This would maintain the fallback to the dagName, if there is neither a match nor host option set and also warns for every match block where both, match and option are set.

But yeah, it might not be the most idiomatic solution.

[jficz]

thanks all for the patches, I'll try to review and apply them as soon as I have some free cycles, hopefully some time this week

[jficz]

suggested news entry: programs.ssh.matchBlocks.* now supports literal Match blocks via programs.ssh.matchBlocks.*.match option as an alternative to plain Host blocks.

@rycee @berbiche I believe the PR is now ready to be merged

[jficz]

@sumnerevans could you re-review the change pls so it doesn't block the PR? also @rycee , @berbiche anything else I can do to help with pushing the PR?

[jficz]

maybe add the proposed news entry to the PR?

@esclear was that on me? wouldn't it be easier for whoever merges this to create the entry to avoid conflicts? They can use this snippet, possibly updating the timestamp:

      {
        time = "2022-11-02T13:40:04+00:00";
        #condition = config.programs.ssh.enable; # no condition preferred, see below
        message = ''
          'programs.ssh.matchBlocks.*' now supports literal 'Match' blocks via
          'programs.ssh.matchBlocks.*.match' option as an alternative to plain
          'Host' blocks
        '';
      }

I would, however, prefer to not use the conditional in the entry as I happen to know at least one person who does not use the ssh module exactly because it does not support Match - with this condition he wouldn't receive the news.

[teto]

@jficz if you update the news entry, I can try to merge this tomorrow

[jficz]

@teto added