[Bug 3122] New Include functionality does not work as documented

[bugzilla-daemon at mindrot.org]

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 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.

-- 
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.