I have been trying to use the new Include functionality to expand a sshd configuration in order to add a snippet of config that matches to a user and use a custom AuthorizedKeysCommand to validate the ssh keys. If I use the include functionality like this: Include /etc/ssh/ssh.d/* And on the /etc/ssh/ssh.d directory I have a config file like this: Match User <user> PasswordAuthentication no AuthorizedKeysCommand <command> "%t" "%k" AuthorizedKeysCommandUser <user> AcceptEnv <some var> It doesn't work. sshd -t tells me the syntax is valid and, when I run sshd with -ddd I see the file getting parsed and loaded, but, when trying to login it operates as if the AuthorizedKeysCommand isn't there. On the other hand, if I do something like this: Match User <user> Include /etc/ssh/ssh.d/* And on the /etc/ssh/ssh.d directory I have a config file like this: PasswordAuthentication no AuthorizedKeysCommand <command> "%t" "%k" AuthorizedKeysCommandUser <user> AcceptEnv <some var> It does work. It also works if I do something like dropping the Match from the main config file: Include /etc/ssh/ssh.d/* Which leads me to conclude that the usage of Match on a included configuration file does not work.
This is the stanza that is causing this: > /* consult cache of include files */ > TAILQ_FOREACH(item, includes, entry) { > if (strcmp(item->selector, arg) != 0) > continue; > if (item->filename != NULL) { > parse_server_config_depth(options, > item->filename, item->contents, > includes, connectinfo, > (oactive ? 0 : SSHCFG_NEVERMATCH), > activep, depth + 1); > } > found = 1; > *activep = oactive; > } I'm not sure what the intention around NEVERMATCH is. There are a few cases to consider: 1) Include in sshd_config before Match 2) Include in sshd_config after Match directive and for each of those: a) included file contains non-match directives b) included file contains at least one Match directive From this I think we get case (b) wrong wrt processing of the Match - as NEVERMATCH gets set and the match never gets considered. I need to think about it a little more Adding Jakub, the author of the Include patch (well, before I mangled it anyway) in case he has something to add.
Created attachment 3384 [details] servconf: Unbreak match blocks in included files (In reply to Damien Miller from comment #1) > [...] > > I'm not sure what the intention around NEVERMATCH is. I was assuming it works the same as in the client configuration parsing. NEVERMATCH prevents using results from Match blocks in included files, that are already in not matching match blocks. > There are a > few cases to consider: > > 1) Include in sshd_config before Match > 2) Include in sshd_config after Match directive > > and for each of those: > > a) included file contains non-match directives > b) included file contains at least one Match directive All of these should be handled by the code as described in the manual page. > From this I think we get case (b) wrong wrt processing of the Match > - as NEVERMATCH gets set and the match never gets considered. I need > to think about it a little more It should not. If the initial activep is set to valid value, it should properly match in the included files. > Adding Jakub, the author of the Include patch (well, before I > mangled it anyway) in case he has something to add. Reading through the code a bit more, it is my bad that I did not notice the difference from client configuration parsing in the activep. The server configuration parsing is a bit different, moreover the comments describing how it works were not up to date. The first issue is that the activep is set to 0 when reprocessing configuration file. This works fine without includes since we want to skip global options as the part of the configuration outside of match block was already processed, but it breaks in this particular use case if we have some more match blocks inside of the includes. I got inspired by the outdated comment in the code: > The second time is after a connection has been established but before authentication. activep is initialized to 2 and global config directives are ignored since they have already been processed. If I follow this comment and adjust the checks for activep to match only when the value is 1 and update the include code to set nevermach only if we have activep == 0, we are able to achieve expected behavior. This way, the global values are skipped with the activep==2, this value is propagated to the included files, but first Match will revert it back to either 0 or 1 and from there everything follows as before. The attached patch worked for me and I was not able to figure out more elegant way to do this while preserving corner cases of current behavior.
@Jakub Thanks for taking the time to look into this. I will give this patch a try and perhaps backport it to the Arch Linux package, if it works. One of the reasons is that we plan to use this include functionality for our infrastructure ansible repository, it makes templating things easier.
The patch is working just fine, just to let you guys know.
Just so I understand what's going on in the patch, is *activep==2 supposed to mean "only allow match/include directives"? If so, rather than touch every *activep test but those, I think it might be better to add a new inc_flags value, say SSHCFG_IN_MATCH or perhaps SSHCFG_MATCH_ONLY drive the logic from that. What do you think?
(In reply to Damien Miller from comment #5) > Just so I understand what's going on in the patch, is *activep==2 > supposed to mean "only allow match/include directives"? Only options in match blocks are used. Regardless they are in the main file or in included file (after first match block). The includes are processed the same way as in normally, but if directive comes before any match block, it is ignored. > If so, rather than touch every *activep test but those, I think it > might be better to add a new inc_flags value, say SSHCFG_IN_MATCH or > perhaps SSHCFG_MATCH_ONLY drive the logic from that. What do you > think? Sure, if you would be able to plug it somehow together. I was not able to figure out correct conditions to make the flags working towards this goal. The main issue is that I need this flag to be active up to the first match block, but I do not have simple way to get this information out of process_server_config_line_depth() function to its caller, which is the only place I can for sure say "here was a Match block". I can probably introduce new parameters, modify return values or use global variables, but I was not satisfied with either direction so far. But what I put together and which is missing in my patch above is the regression test: @@ -150,5 +150,19 @@ ${SUDO} ${REAL_SSHD} -f $OBJ/sshd_config.i.x \ -C "host=x,user=test,addr=127.0.0.1" 2>/dev/null && \ fail "sshd allowed Include with no argument" +# Ensure the Include before any Match block works as expected (bug #3122) +cat > $OBJ/sshd_config.i << _EOF +Banner /xx +HostKey $OBJ/host.ssh-ed25519 +Include $OBJ/sshd_config.i.2 +_EOF +cat > $OBJ/sshd_config.i.2 << _EOF +Match host a + Banner /aa +_EOF + +trace "Include before match blocks" +trial a /aa "included file before match blocks is properly evaluated" + # cleanup rm -f $OBJ/sshd_config.i $OBJ/sshd_config.i.* $OBJ/sshd_config.out
Created attachment 3399 [details] proposed patch for this & #3169 With a bit of tweaking it looks like it is finally working as expected. I changed the flags that they are passed back to the caller so we can clean this flag after first match block. Also regression tests for this and #3169 are attached. Damien, would the proposed change work for you?
Thanks - this patch has been applied and will be in OpenSSH 8.4, due in ~3 months.
*** Bug 3207 has been marked as a duplicate of this bug. ***
Mass close of all bugs fixed in 8.4 release.