Move log opening to appropriate execution phase

[TomasKorbar]

When piped logs are opened during parsing of configuration it results in unexpected situations in apache httpd and can cause hang of process which is trying to log into auditlog.

Closes #2822

[martinhsv]

Hello @TomasKorbar ,

There is at least one downside here.

Current functionality means that the attempt to open the audit log file occurs at startup. If it fails -- perhaps because the chosen location is unwriteable -- then Apache will fail to start and the user can see a message like:

ModSecurity: Failed to open the audit log file: /var/log/apache/modsec_audit.log

(As a side note, with these changes I could not see a failure message anywhere, including in Apache's error.log. Perhaps I didn't look carefully enough.)

In general, it's preferable to have configuration errors identified at startup whenever possible.

[TomasKorbar]

Hi @martinhsv
Log about not accessible file is now present and failure to open log prevents apache from starting.
Let me know if there are any more changes needed.

[TomasKorbar]

@martinhsv Hi, could we get this merged please? I can provide further help if needed.

[TomasKorbar]

@martinhsv should be good now.

[eflanagan0]

@marcstern I've verified this builds correctly on a NixOS system after cherry-picking the commit to v2.7.3. I can also start an Apache/2.4.59 server loading the compiled module.

[marcstern]

Sounds sensible, but I wonder why it was handled differently than error logs at first. Does anybody see a potential problem with this fix?

Otherwise, I think it's the right moment to de-duplicate the code between dcfg->auditlog_name & dcfg->auditlog_name2 file opening. Can you use a static function that centralizes the common code?

Additional question (linked to code duplication):
Why do we have pipe_name = dcfg->auditlog_name + 1; and pipe_name = ap_server_root_relative(p, dcfg->auditlog2_name + 1);? Is this normal?

[eflanagan0]

I realized this PR also will need rebased atop v2/master to incorporate the CI changes and kick off those checks.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[TomasKorbar]

@marcstern Hi,
I added static function to deduplicate the code. Also the secondary pipe is now not evaluated relatively to server root, as i think that when you want to enter executable path, it is more useful to just specify absolute path, as the executable does not have to be in the same directory as server.

Please tell me about additional changes if necessary.

[marcstern]

Using a hook looks indeed cleaner and more consistent with error log. Does anybody see a potential risk with this change?

[airween]

Using a hook looks indeed cleaner and more consistent with error log. Does anybody see a potential risk with this change?

Let me check this a bit later (probably in next few days).