bugzilla-daemon at bugzilla.mindrot.org
2020-Feb-18 10:46 UTC
[Bug 3122] New: New Include functionality does not work as documented
https://bugzilla.mindrot.org/show_bug.cgi?id=3122 Bug ID: 3122 Summary: New Include functionality does not work as documented Product: Portable OpenSSH Version: 8.2p1 Hardware: amd64 OS: Linux Status: NEW Severity: normal Priority: P5 Component: sshd Assignee: unassigned-bugs at mindrot.org Reporter: grazzolini at archlinux.org 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. -- You are receiving this mail because: You are watching the assignee of the bug.
bugzilla-daemon at mindrot.org
2020-Apr-17 05:24 UTC
[Bug 3122] New Include functionality does not work as documented
https://bugzilla.mindrot.org/show_bug.cgi?id=3122 Damien Miller <djm at mindrot.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |djm at mindrot.org, | |jjelen at redhat.com --- Comment #1 from Damien Miller <djm at mindrot.org> --- 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. -- You are receiving this mail because: You are watching the assignee of the bug. You are watching someone on the CC list of the bug.
bugzilla-daemon at mindrot.org
2020-Apr-17 11:52 UTC
[Bug 3122] New Include functionality does not work as documented
https://bugzilla.mindrot.org/show_bug.cgi?id=3122 --- Comment #2 from Jakub Jelen <jjelen at redhat.com> --- Created attachment 3384 --> https://bugzilla.mindrot.org/attachment.cgi?id=3384&action=edit 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 directiveAll 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 moreIt 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. -- You are receiving this mail because: You are watching someone on the CC list of the bug. You are watching the assignee of the bug.
bugzilla-daemon at mindrot.org
2020-Apr-17 14:59 UTC
[Bug 3122] New Include functionality does not work as documented
https://bugzilla.mindrot.org/show_bug.cgi?id=3122 --- Comment #3 from Giancarlo Razzolini <grazzolini at archlinux.org> --- @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. -- You are receiving this mail because: You are watching the assignee of the bug. You are watching someone on the CC list of the bug.
bugzilla-daemon at mindrot.org
2020-May-13 19:57 UTC
[Bug 3122] New Include functionality does not work as documented
https://bugzilla.mindrot.org/show_bug.cgi?id=3122 --- Comment #4 from Giancarlo Razzolini <grazzolini at archlinux.org> --- The patch is working just fine, just to let you guys know. -- You are receiving this mail because: You are watching someone on the CC list of the bug. You are watching the assignee of the bug.
bugzilla-daemon at mindrot.org
2020-May-15 03:52 UTC
[Bug 3122] New Include functionality does not work as documented
https://bugzilla.mindrot.org/show_bug.cgi?id=3122 --- Comment #5 from Damien Miller <djm at mindrot.org> --- 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? -- You are receiving this mail because: You are watching someone on the CC list of the bug. You are watching the assignee of the bug.
bugzilla-daemon at mindrot.org
2020-May-15 11:03 UTC
[Bug 3122] New Include functionality does not work as documented
https://bugzilla.mindrot.org/show_bug.cgi?id=3122 --- Comment #6 from Jakub Jelen <jjelen at redhat.com> --- (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 -- You are receiving this mail because: You are watching someone on the CC list of the bug. You are watching the assignee of the bug.
bugzilla-daemon at mindrot.org
2020-May-26 14:38 UTC
[Bug 3122] New Include functionality does not work as documented
https://bugzilla.mindrot.org/show_bug.cgi?id=3122 Jakub Jelen <jjelen at redhat.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #3384|0 |1 is obsolete| | Attachment #3399| |ok?(djm at mindrot.org) Flags| | --- Comment #7 from Jakub Jelen <jjelen at redhat.com> --- Created attachment 3399 --> https://bugzilla.mindrot.org/attachment.cgi?id=3399&action=edit 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? -- You are receiving this mail because: You are watching the assignee of the bug. You are watching someone on the CC list of the bug.
bugzilla-daemon at mindrot.org
2020-May-27 22:43 UTC
[Bug 3122] New Include functionality does not work as documented
https://bugzilla.mindrot.org/show_bug.cgi?id=3122 Damien Miller <djm at mindrot.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |RESOLVED Resolution|--- |FIXED Blocks| |3162 --- Comment #8 from Damien Miller <djm at mindrot.org> --- Thanks - this patch has been applied and will be in OpenSSH 8.4, due in ~3 months. Referenced Bugs: https://bugzilla.mindrot.org/show_bug.cgi?id=3162 [Bug 3162] Tracking bug for 8.4 release -- You are receiving this mail because: You are watching the assignee of the bug. You are watching someone on the CC list of the bug.
bugzilla-daemon at mindrot.org
2020-Sep-01 01:59 UTC
[Bug 3122] New Include functionality does not work as documented
https://bugzilla.mindrot.org/show_bug.cgi?id=3122 Damien Miller <djm at mindrot.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |devel at sapphirepaw.org --- Comment #9 from Damien Miller <djm at mindrot.org> --- *** Bug 3207 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching someone on the CC list of the bug. You are watching the assignee of the bug.
bugzilla-daemon at mindrot.org
2020-Oct-02 04:55 UTC
[Bug 3122] New Include functionality does not work as documented
https://bugzilla.mindrot.org/show_bug.cgi?id=3122 Darren Tucker <dtucker at dtucker.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED --- Comment #10 from Darren Tucker <dtucker at dtucker.net> --- Mass close of all bugs fixed in 8.4 release. -- You are receiving this mail because: You are watching someone on the CC list of the bug. You are watching the assignee of the bug.
bugzilla-daemon at mindrot.org
2023-Jan-13 02:26 UTC
[Bug 3122] New Include functionality does not work as documented
https://bugzilla.mindrot.org/show_bug.cgi?id=3122 Damien Miller <djm at mindrot.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #3399|ok?(djm at mindrot.org) | Flags| | -- You are receiving this mail because: You are watching someone on the CC list of the bug. You are watching the assignee of the bug.
Maybe Matching Threads
- [Bug 2496] New: sshd hangs when using AuthorizedKeysCommand
- [Bug 3169] New: sshd listens to port 22 AND whatever port is specified in or after Include(s)
- [Bug 2288] New: documentation of options defaulting to "none"
- Feature request/EOI: Match interactive config?
- [Bug 2755] New: [PATCH] sshd_config: allow directories in AuthorizedKeysFile=