[Apache] Traverse the include path to the root where relevant

[joohoi]

This PR allows Certbot to utilize full include tree for searches that need to know about the ancestors. Search for mod_macro block is included in this PR. The functionality also takes the possibility of multiple includes into account.

Because of the limitations of the current parsing engine, we have to search includes from the root up. This also introduces quite big performance penalty, especially if done dynamically. This is why a dictionary of Include, IncludeOptions and their values are saved to the parser object, and refreshed when new includes are added.

[ohemorange]

Before getting into the code, I'd love some high-level clarifications about this PR, probably mostly since it's been a while since before the holidays.

Basically, I'm wondering how much this is worth it. As I understand it, this code is a) pretty specific to the augeas parser implementation, which makes it relatively temporary and b) it involves caching results, which is a prime breeding ground for bugs. This combination might be worth it, but it seems like we're doing this to get a more correct implementation than we previously had (searching down include paths, because the augeas path won't have the full logical path in it). And on top of that, are there searches other than the macro search that's changed in this PR?

Were there other factors I'm missing here? Because based on just these, I'm surprised to see this PR.

[joohoi]

The main need for this PR is to allow runtime assertions to be utilized without the need of re-implementing a minor bug to the apacheparser implementation that has existed in the code for a long time.

The bug exists because we're simply reading the Augeas path of VirtualHost block to determine if it's wrapped in a <Macro> block. While this catches the majority of use cases, there may be configurations that it's not able to parse correctly because of how Augeas paths work.

This would not trigger the bug:

<Macro VHost $domain>
  <VirtualHost *:80>
    ServerName $domain
    ...
  </VirtualHost>
</Macro>
Use VHost example.com
Use VHost example.org

But this would:

# vhost.conf
<VirtualHost *:80>
  ServerName $domain
  ...
</VirtualHost>
# end of vhost.conf

# main.conf
<Macro VHost $domain>
  IncludeOptional vhost.conf
</Macro>
Use VHost example.com
Use VHost example.org
# end of main.conf

In the bugged example, we would be checking the /path/to/vhost.conf/VirtualHost to determine if it's in a mod_macro block.

[ohemorange]

While the overall logic seems to be implemented correctly, where possible we should attempt to simplify this, because as it is now it's a bit tricky to follow, which makes it more error-prone than I'm comfortable with.

[ohemorange]

it's probably not a problem, but I'd feel more comfortable if the normpath call came after the join, just in case the root isn't already norm.

[ohemorange]

While I see where the name is coming from, it's a bit opaque when reading the calling code. Especially if it's only being called in one place, leaving it as returning a boolean could make it more clear. Something like block_type_present_in_ancestors or something to that effect?

[joohoi]

The context here is that we will end up using this for reverse searching ancestor blocks in the Augeas implementation of ParseNode intefraces later, so the current functionality is needed for that purpose.

[ohemorange]

Suggested change

	queue = list()
	queue = []

to match block_paths?

[ohemorange]

actually, we seem to use both types of initialization in the code. I'd prefer that new code picks one form though.

[ohemorange]

Do we have a use case for actually returning the path itself, rather than just an inclusion test? If not, this could could be much smaller.

[joohoi]

Yeah, we do. Not within this PR but upcoming ones that will depend on this functionality. See previous comment

[ohemorange]

if we're going to have this here anyway, we can probably take it out of the augeas init, and simply none-out instead of calling _find_all_includes in add_dir_to_ifmodssl, saving some compute time with a lazy pattern.

[ohemorange]

this seems like the sort of preprocessing step that most correctly belongs in _find_all_includes, unless I'm missing something that relies on the specific check here.

[joohoi]

For consistency and reusability, I'd prefer keeping the exact include directive contents intact (which is handled by _find_all_includes) and do the path normalization when comparing against that data. This also lets us avoid the re-searching through the whole tree from root to leaves when subsequently looking if a file might be included by the existing Include and IncludeOptional directives. One of such cases would be when adding a new HTTPS virtualhost file, where we need to figure out if the created files filepath is already included in a wildcard include.

[ohemorange]

While the code is correct here, the semantics of this are a bit obfuscated -- paths aren't included, files are, and while in practice we're technically taking the file part of the path and checking for that, it still seems weird to have a function that finds includes for a path rather than a file. Basically having a too-smart helper function feels weird. How would you feel about taking the filename as an argument to this function? And maybe separating out the processing of path to file into a separate function?

[joohoi]

I was thinking of filepaths, and while the actual files are included in the configuration tree, they are included by their paths. These paths might also be wildcard ones, so a subsequently created file that wasn't included in the configuration tree might be automatically included by its path. This function is trying to look up Include and IncludeOptional directives that would include the filepath that we provide as an argument.

This was just explaining the naming, and I'm open to discuss the semantics further, but would prefer keeping it as it is based on the above.

[joohoi]

We agreed that it'd be good to write an additional test, testing and demonstrating the use of paths returned from the function find_blocks_from_include_tree() and use apache-parser-v2 tests to do so. Unfortunately the apache-parser-v2 tree didn't have specific tests doing reverse lookups for specific conditional directives so I wrote a new one.

The new test here demonstrates the functionality needed for doing reverse searches for ancestors with specific parameters, IfModule used as an example.

[ohemorange]

To summarize plans discussed elsewhere:

Step 1. Merge apache-parser-v2 into master (done)
Step 2. Rewrite find_ancestors() to call find_blocks_from_include_tree()

for ancestor in self.parser.find_blocks_from_include_tree(name)
    anc = self._create_blocknode(ancestor)
    ancestors.append(anc)
return ancestors

Step 3. Review code all working together properly to be more confident that find_blocks_from_include_tree() is implemented correctly

[bmw]

What's the status of this PR? Should we try to get it merged in the short term or should we save the work for later?

If the latter, I'd prefer this PR was closed to clean out the PR queue a bit. If we do this, the way I usually do it is to link the branch containing this work from the issue it addresses.