Bug 57585

Summary: httpd fails to start if there is an IncludeOptional directive to a inexistent directory
Product: Apache httpd-2 Reporter: Alberto Murillo Silva <powerbsd>
Component: CoreAssignee: Apache HTTPD Bugs Mailing List <bugs>
Status: RESOLVED FIXED    
Severity: normal CC: JJJJust, powerbsd, toscano.luca
Priority: P2 Keywords: PatchAvailable
Version: 2.4.10   
Target Milestone: ---   
Hardware: All   
OS: Linux   
Attachments: Dont error when IncludeOptional refers to a non-existent directory
httpd-trunk-core_include_optional_fix.patch

Description Alberto Murillo Silva 2015-02-13 20:37:46 UTC
There must be cases were we want to Optionally include configuration files.

IncludeOptional check for the folder to exists and then parses the configuration files found in it (if any) but return an error if the
directory does not exists yet.

After apr_dir_open() fails in process_resource_config_fnmatch and process_resource_config_nofnmatch, we can check for the "optional" arg
and return error only if not optional.
Comment 1 Alberto Murillo Silva 2015-02-13 20:46:25 UTC
Created attachment 32472 [details]
Dont error when IncludeOptional refers to a non-existent directory
Comment 2 Luca Toscano 2017-10-26 11:10:20 UTC
Created attachment 35467 [details]
httpd-trunk-core_include_optional_fix.patch

Hi Alberto,

I have another version of the patch since I've tested it and stuff like the following seems not to be skipped:

IncludeOptional notexisstent

Still need to test it carefully but your review would surely help!

Luca
Comment 3 Luca Toscano 2017-10-26 13:03:43 UTC
So I just noticed that https://httpd.apache.org/docs/2.4/mod/core.html#includeoptional says:

"This directive allows inclusion of other configuration files from within the server configuration files. It works identically to the Include directive, with the exception that if wildcards do not match any file or directory, the IncludeOptional directive will be silently ignored instead of causing an error."

I was brought to this task since there is an outstanding bug in Debian for mod_security in stretch:

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=878920

Basically the maintainer of libapache2-mod_security added the following:

# Include OWASP ModSecurity CRS rules if installed
IncludeOptional /usr/share/modsecurity-crs/owasp-crs.load

This causes an error but something like the following does not:

# Include OWASP ModSecurity CRS rules if installed
IncludeOptional /usr/share/modsecurity-crs/owasp-crs*.load

I can see the value of this difference but is there any strong motivation to error out on the former and not on the latter?
Comment 4 Alberto Murillo Silva 2017-10-26 14:20:18 UTC
My patch has been working for almost 3 years in clearlinux
https://github.com/clearlinux-pkgs/httpd/blob/master/0002-do-not-crash-when-IncludeOptional-dir-is-not-existent.patch

I was new at the project so I just wrote and forget about it (my bad) I guess is time to chase the apache developers to review and include it in master.

I currently do not know how to contact them (mailing list I guess) if you find the way CC me :)
Comment 5 Luca Toscano 2017-10-26 14:28:00 UTC
(In reply to Alberto Murillo Silva from comment #4)
> My patch has been working for almost 3 years in clearlinux
> https://github.com/clearlinux-pkgs/httpd/blob/master/0002-do-not-crash-when-
> IncludeOptional-dir-is-not-existent.patch
> 
> I was new at the project so I just wrote and forget about it (my bad) I
> guess is time to chase the apache developers to review and include it in
> master.
> 
> I currently do not know how to contact them (mailing list I guess) if you
> find the way CC me :)

What are the use cases that you tested to make sure that it was working? I am trying to understand an example that I can test and verify, not trying to discard your work :)

You can surely contact the dev using the https://httpd.apache.org/lists.html#http-dev mailing list, but I guess that bugzilla is fine since some of them are reading it daily (like me :).

The patch that I proposed work even if you include a non existent file, but it might be a big change in behavior that is not worth it.

Anyhow, thanks for the work done!

Luca
Comment 6 Alberto Murillo Silva 2017-10-26 15:47:35 UTC
Luca you are right.

In ClearLinux:
IncludeOptional /not/existing/dir/*.conf WORKS
IncludeOptional /not/existing/file.conf  FAILS

I guess both options should work.

Our use case is:
# Virtual hosts
IncludeOptional /usr/share/defaults/httpd/conf.d/*.conf
IncludeOptional /usr/share/defaults/httpd/conf.modules.d/*.conf
IncludeOptional /etc/httpd/conf.d/*.conf
IncludeOptional /etc/httpd/conf.modules.d/*.conf

where /etc/httpd sometimes doesn't exist but the service successfully starts with the default page.
Comment 7 Luca Toscano 2017-11-04 12:17:37 UTC
Hi Alberto,

I just sent an email to the dev@ mailing list to see what the community thinks about this task. Let's see what are the answers :)

If you want to follow you can either register to dev@ (https://httpd.apache.org/lists.html#http-dev) or check https://lists.apache.org/list.html?dev@httpd.apache.org.

Luca
Comment 8 Luca Toscano 2017-11-11 19:21:48 UTC
Committed svn.apache.org/r1814968 to trunk, if the patch is accepted I'll also propose a backport to 2.4.x soon!
Comment 9 Luca Toscano 2017-12-21 19:37:17 UTC
Backported to 2.4.x with https://svn.apache.org/r1818964, it will be part of the next httpd release. Thanks Alberto for the help, please re-open if anything is missing!