Skip to content

Conversation

@JSCU-CNI
Copy link
Contributor

@JSCU-CNI JSCU-CNI commented Jan 8, 2025

This PR improves the log file discovery of the ApachePlugin on targets.

  • We now parse Include and IncludeOptional directives in all apache .conf files (mods, sites and confs) and extract ErrorLog and CustomLog from those configuration files.
  • We now substitute Apache envvars from those log file location directives (e.g. ${APACHE_LOG_DIR}).

Fixes #1013.

@JSCU-CNI JSCU-CNI force-pushed the improvement/apache-log-discovery branch from 9a74e80 to 8191b9b Compare February 25, 2025 13:43
@JSCU-CNI
Copy link
Contributor Author

Could this issue be assigned a reviewer @EinatFox? This PR should improve log file detection for the Apache plugin significantly.

@Schamper Schamper requested a review from Copilot February 26, 2025 21:14
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Overview

This pull request improves ApachePlugin log file discovery by enhancing configuration file parsing, including support for processing Include/IncludeOptional directives and substituting Apache environment variables within log file paths.

  • Parses Include and IncludeOptional directives in Apache configuration files
  • Substitutes Apache envvars in log file locations
  • Updates test cases and refactors plugin initialization and log discovery routines

Reviewed Changes

File Description
tests/plugins/apps/webserver/test_apache.py Adjusted tests to use the new plugin registration and log discovery
dissect/target/plugins/apps/webserver/apache.py Refactored log discovery logic and added environment variable support
tests/plugins/apps/webserver/test_citrix.py Updated test to verify unsupported plugin registration for Citrix

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

dissect/target/plugins/apps/webserver/apache.py:409

  • The word 'occured' is misspelled; consider using 'occurred'.
self.target.log.warning("An error occured parsing Apache log file %s: %s", path, str(e))

@JSCU-CNI
Copy link
Contributor Author

JSCU-CNI commented Mar 5, 2025

The tests test_custom_config, test_config_commented_logs, test_config_vhosts_httpd and test_config_vhosts_apache2 seem to interfere with each other. Disabling the first two makes the last two pass. Do you have a clue how we can fix this @Schamper?

@JSCU-CNI JSCU-CNI requested a review from Schamper March 5, 2025 13:58
@JSCU-CNI
Copy link
Contributor Author

JSCU-CNI commented Mar 11, 2025

It seems like some form of plugin caching is interfering with the tests here. We have ruled out object bleeding by switching to a factory in 8e127b8.

If we make the ApachePlugin.find_logs() method log the seen argument, we can see that fresh target_unix, fs_unix and ApachePlugin objects are populated with values from previous tests (test_custom_config and test_config_commented_logs).

We can refactor the Apache plugin to prevent this situation from happening, however this seems like a dissect caching bug to us.

    def _process_conf_file(self, path: Path, seen: set[Path] | None = set()) -> None:
        """Process an Apache ``.conf`` file for ``ServerRoot``, ``CustomLog``, ``Include``
        and ``OptionalInclude`` directives. Populates ``self.access_paths`` and ``self.error_paths``.
        """
        self.target.log.warning("Parsing %s", path)
        self.target.log.warning("Current seen %s", seen)
$ pytest tests/plugins/apps/webserver/test_apache.py -vv
...
tests/plugins/apps/webserver/test_apache.py::test_custom_config PASSED                                                                                                                                                                                                                                                                   [ 70%]
tests/plugins/apps/webserver/test_apache.py::test_config_commented_logs PASSED                                                                                                                                                                                                                                                           [ 76%]
tests/plugins/apps/webserver/test_apache.py::test_config_vhosts_httpd FAILED                                                                                                                                                                                                                                                             [ 82%]
tests/plugins/apps/webserver/test_apache.py::test_config_vhosts_apache2 FAILED 
...
test_config_vhosts_httpd
...
WARNING  dissect.target.target:apache.py:274 <Target /tmp/pytest-of-user/pytest-22/test_config_vhosts_apache20/MockTarget-19g2hmxv>: Parsing /etc/apache2/apache2.conf
WARNING  dissect.target.target:apache.py:275 <Target /tmp/pytest-of-user/pytest-22/test_config_vhosts_apache20/MockTarget-19g2hmxv>: Current seen {TargetPath('/etc/apache2/apache2.conf'), TargetPath('/etc/httpd/conf/httpd.conf')}
WARNING  dissect.target.target:apache.py:278 <Target /tmp/pytest-of-user/pytest-22/test_config_vhosts_apache20/MockTarget-19g2hmxv>: Detected recursion in Apache configuration, file already parsed: /etc/apache2/apache2.conf
...
test_config_vhosts_apache2
...
WARNING  dissect.target.target:apache.py:274 <Target /tmp/pytest-of-user/pytest-23/test_config_vhosts_apache20/MockTarget-3s13z1rn>: Parsing /etc/apache2/apache2.conf
WARNING  dissect.target.target:apache.py:275 <Target /tmp/pytest-of-user/pytest-23/test_config_vhosts_apache20/MockTarget-3s13z1rn>: Current seen {TargetPath('/etc/httpd/conf/httpd.conf'), TargetPath('/etc/apache2/apache2.conf')}
WARNING  dissect.target.target:apache.py:278 <Target /tmp/pytest-of-user/pytest-23/test_config_vhosts_apache20/MockTarget-3s13z1rn>: Detected recursion in Apache configuration, file already parsed: /etc/apache2/apache2.conf

Explicitly setting the seen argument to None is a dirty workaround for the caching bug: d86bd33.

Copy link
Member

@Schamper Schamper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the comment, I think this makes the change to a factory redundant.

@JSCU-CNI JSCU-CNI requested a review from Schamper March 17, 2025 10:18
@codecov
Copy link

codecov bot commented Mar 17, 2025

Codecov Report

Attention: Patch coverage is 84.37500% with 15 lines in your changes missing coverage. Please review.

Project coverage is 79.11%. Comparing base (67742a2) to head (c60b21e).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
dissect/target/plugins/apps/webserver/apache.py 84.37% 15 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #980   +/-   ##
=======================================
  Coverage   79.10%   79.11%           
=======================================
  Files         341      341           
  Lines       30091    30155   +64     
=======================================
+ Hits        23804    23856   +52     
- Misses       6287     6299   +12     
Flag Coverage Δ
unittests 79.11% <84.37%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@JSCU-CNI JSCU-CNI requested a review from Schamper March 17, 2025 10:50
@JSCU-CNI JSCU-CNI requested a review from Schamper March 17, 2025 16:52
@JSCU-CNI JSCU-CNI requested a review from Schamper March 19, 2025 12:52
@JSCU-CNI
Copy link
Contributor Author

Is this PR good to go? We have another PR ready that depends on this branch :)

@JSCU-CNI JSCU-CNI requested a review from Schamper March 24, 2025 13:14
@Schamper Schamper merged commit 12306a3 into fox-it:main Mar 25, 2025
20 of 22 checks passed
@JSCU-CNI JSCU-CNI deleted the improvement/apache-log-discovery branch March 25, 2025 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

webserver.apache : Support Include/IncludeOptionnal directive in apache configurations files

3 participants