You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by William A Rowe Jr <wr...@rowe-clan.net> on 2017/10/11 15:15:37 UTC

Re: svn commit: r1740928 - in /httpd/httpd/trunk: ./ include/ modules/http2/ modules/proxy/ modules/ssl/ server/

On Mon, Apr 25, 2016 at 7:04 PM, <yl...@apache.org> wrote:

> Author: ylavic
> Date: Tue Apr 26 00:04:57 2016
> New Revision: 1740928
>
> URL: http://svn.apache.org/viewvc?rev=1740928&view=rev
> Log:
> mod_proxy, mod_ssl: Handle SSLProxy* directives in <Proxy> sections,
> allowing different TLS configurations per backend.
>
>
> Modified: httpd/httpd/trunk/CHANGES
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=
> 1740928&r1=1740927&r2=1740928&view=diff
> ============================================================
> ==================
> --- httpd/httpd/trunk/CHANGES [utf-8] (original)
> +++ httpd/httpd/trunk/CHANGES [utf-8] Tue Apr 26 00:04:57 2016
> @@ -1,6 +1,9 @@
>                                                           -*- coding:
> utf-8 -*-
>  Changes with Apache 2.5.0
>
> +  *) mod_proxy, mod_ssl: Handle SSLProxy* directives in <Proxy> sections,
> +     allowing different TLS configurations per backend.  [Yann Ylavic]
> +
>    *) mod_http2: eliminating h2_io instances for streams, reducing memory
>       pools and footprint. [Stefan Eissing]
>
>
> Modified: httpd/httpd/trunk/include/http_config.h
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/include/
> http_config.h?rev=1740928&r1=1740927&r2=1740928&view=diff
> ============================================================
> ==================
> --- httpd/httpd/trunk/include/http_config.h (original)
> +++ httpd/httpd/trunk/include/http_config.h Tue Apr 26 00:04:57 2016
> @@ -249,6 +249,8 @@ struct command_struct {
>  #define NONFATAL_UNKNOWN 1024    /* Unrecognised directive */
>  #define NONFATAL_ALL (NONFATAL_OVERRIDE|NONFATAL_UNKNOWN)
>
> +#define PROXY_CONF 2048      /**< *.conf inside &lt;Proxy&gt; only */
> +
>  /** this directive can be placed anywhere */
>  #define OR_ALL (OR_LIMIT|OR_OPTIONS|OR_FILEINFO|OR_AUTHCFG|OR_INDEXES)
>
> @@ -923,9 +925,10 @@ AP_DECLARE(const char *) ap_check_cmd_co
>  #define  NOT_IN_LOCATION        0x08 /**< Forbidden in &lt;Location&gt; */
>  #define  NOT_IN_FILES           0x10 /**< Forbidden in &lt;Files&gt; or
> &lt;If&gt;*/
>  #define  NOT_IN_HTACCESS        0x20 /**< Forbidden in .htaccess files */
> -/** Forbidden in &lt;Directory&gt;/&lt;Location&gt;/&lt;Files&gt;&lt;
> If&gt;*/
> -#define  NOT_IN_DIR_LOC_FILE    (NOT_IN_DIRECTORY|NOT_IN_
> LOCATION|NOT_IN_FILES)
> -/** Forbidden in &lt;VirtualHost&gt;/&lt;Limit&gt;/&lt;Directory&gt;/&lt;
> Location&gt;/&lt;Files&gt;/&lt;If&gt; */
> +#define  NOT_IN_PROXY           0x40 /**< Forbidden in &lt;Proxy&gt; */
> +/** Forbidden in &lt;Directory&gt;/&lt;Location&gt;/&lt;Files&gt;&lt;
> If&gt;&lt;Proxy&gt;*/
> +#define  NOT_IN_DIR_LOC_FILE    (NOT_IN_DIRECTORY|NOT_IN_
> LOCATION|NOT_IN_FILES|NOT_IN_PROXY)
> +/** Forbidden in &lt;VirtualHost&gt;/&lt;Limit&gt;/&lt;Directory&gt;/&lt;
> Location&gt;/&lt;Files&gt;/&lt;If&gt;&lt;Proxy&gt;*/
>  #define  GLOBAL_ONLY            (NOT_IN_VIRTUALHOST|NOT_IN_
> LIMIT|NOT_IN_DIR_LOC_FILE)
>
>  /** @} */
>

Reviewing this in much more depth yesterday ... this doesn't seem
to be a minor MMN bump, so would not be backportable without some
extra heavy lifting on 2.4 interop.

A module previously compiled against 2.4 would not carry the newly
correct definition of NOT_IN_DIR_LOC_FILE because all previously
compiled modules are missing the 0x40 bit, right?

That would be the definition of an MMN major bump, previously
compiled modules require recompilation.

Re: svn commit: r1740928 - in /httpd/httpd/trunk: ./ include/ modules/http2/ modules/proxy/ modules/ssl/ server/

Posted by Yann Ylavic <yl...@gmail.com>.
Committed something with explanations (hopefully) in r1812193. Does it
sound good?

On Thu, Oct 12, 2017 at 3:54 AM, William A Rowe Jr <wr...@rowe-clan.net> wrote:
> I will review again tomorrow.
>
> My jump-around idea was to check against all of the bits in not dir loc
> file, and if the module's MMN minor is too early, treat the <Proxy > section
> as if that bit is set.
>
>
>
> On Oct 11, 2017 16:13, "Yann Ylavic" <yl...@gmail.com> wrote:
>
> On Wed, Oct 11, 2017 at 11:02 PM, Yann Ylavic <yl...@gmail.com> wrote:
>> On Wed, Oct 11, 2017 at 10:49 PM, Yann Ylavic <yl...@gmail.com>
>> wrote:
>>> On Wed, Oct 11, 2017 at 10:38 PM, Yann Ylavic <yl...@gmail.com>
>>> wrote:
>>>>
>>>> Thus, how about if there, for 2.4.x only (i.e. backport patch), we'd
>>>> instead check for:
>>>>> +        || (((forbidden & NOT_IN_PROXY)
>>>>> +             || (forbidden & NOT_IN_DIR_LOC_FILE) ==
>>>>> NOT_IN_DIR_LOC_FILE
>>>>> +             || (forbidden & GLOBAL_ONLY) == GLOBAL_ONLY)
>>>>> +            && ((found = ...)
>>>>> +                || ...)))
>>>> ?
>>>
>>> Looks like there are other usages of NOT_IN_DIR_LOC_FILE we should
>>> hack in ap_check_cmd_context() too, but you probably see the idea...
>>
>> Actually, I think the correct fix, even for 2.5/trunk, is something
>> for the attached patch.
>> WDYT?
>
> Sorry, spoke^R patched too soon, this v2 is more correct I guess.
>
>

Re: svn commit: r1740928 - in /httpd/httpd/trunk: ./ include/ modules/http2/ modules/proxy/ modules/ssl/ server/

Posted by William A Rowe Jr <wr...@rowe-clan.net>.
I will review again tomorrow.

My jump-around idea was to check against all of the bits in not dir loc
file, and if the module's MMN minor is too early, treat the <Proxy >
section as if that bit is set.



On Oct 11, 2017 16:13, "Yann Ylavic" <yl...@gmail.com> wrote:

On Wed, Oct 11, 2017 at 11:02 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Wed, Oct 11, 2017 at 10:49 PM, Yann Ylavic <yl...@gmail.com>
wrote:
>> On Wed, Oct 11, 2017 at 10:38 PM, Yann Ylavic <yl...@gmail.com>
wrote:
>>>
>>> Thus, how about if there, for 2.4.x only (i.e. backport patch), we'd
>>> instead check for:
>>>> +        || (((forbidden & NOT_IN_PROXY)
>>>> +             || (forbidden & NOT_IN_DIR_LOC_FILE) ==
NOT_IN_DIR_LOC_FILE
>>>> +             || (forbidden & GLOBAL_ONLY) == GLOBAL_ONLY)
>>>> +            && ((found = ...)
>>>> +                || ...)))
>>> ?
>>
>> Looks like there are other usages of NOT_IN_DIR_LOC_FILE we should
>> hack in ap_check_cmd_context() too, but you probably see the idea...
>
> Actually, I think the correct fix, even for 2.5/trunk, is something
> for the attached patch.
> WDYT?

Sorry, spoke^R patched too soon, this v2 is more correct I guess.

Re: svn commit: r1740928 - in /httpd/httpd/trunk: ./ include/ modules/http2/ modules/proxy/ modules/ssl/ server/

Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Oct 11, 2017 at 11:02 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Wed, Oct 11, 2017 at 10:49 PM, Yann Ylavic <yl...@gmail.com> wrote:
>> On Wed, Oct 11, 2017 at 10:38 PM, Yann Ylavic <yl...@gmail.com> wrote:
>>>
>>> Thus, how about if there, for 2.4.x only (i.e. backport patch), we'd
>>> instead check for:
>>>> +        || (((forbidden & NOT_IN_PROXY)
>>>> +             || (forbidden & NOT_IN_DIR_LOC_FILE) == NOT_IN_DIR_LOC_FILE
>>>> +             || (forbidden & GLOBAL_ONLY) == GLOBAL_ONLY)
>>>> +            && ((found = ...)
>>>> +                || ...)))
>>> ?
>>
>> Looks like there are other usages of NOT_IN_DIR_LOC_FILE we should
>> hack in ap_check_cmd_context() too, but you probably see the idea...
>
> Actually, I think the correct fix, even for 2.5/trunk, is something
> for the attached patch.
> WDYT?

Sorry, spoke^R patched too soon, this v2 is more correct I guess.

Re: svn commit: r1740928 - in /httpd/httpd/trunk: ./ include/ modules/http2/ modules/proxy/ modules/ssl/ server/

Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Oct 11, 2017 at 10:49 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Wed, Oct 11, 2017 at 10:38 PM, Yann Ylavic <yl...@gmail.com> wrote:
>>
>> Thus, how about if there, for 2.4.x only (i.e. backport patch), we'd
>> instead check for:
>>> +        || (((forbidden & NOT_IN_PROXY)
>>> +             || (forbidden & NOT_IN_DIR_LOC_FILE) == NOT_IN_DIR_LOC_FILE
>>> +             || (forbidden & GLOBAL_ONLY) == GLOBAL_ONLY)
>>> +            && ((found = ...)
>>> +                || ...)))
>> ?
>
> Looks like there are other usages of NOT_IN_DIR_LOC_FILE we should
> hack in ap_check_cmd_context() too, but you probably see the idea...

Actually, I think the correct fix, even for 2.5/trunk, is something
for the attached patch.
WDYT?

Re: svn commit: r1740928 - in /httpd/httpd/trunk: ./ include/ modules/http2/ modules/proxy/ modules/ssl/ server/

Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Oct 11, 2017 at 10:38 PM, Yann Ylavic <yl...@gmail.com> wrote:
>
> Thus, how about if there, for 2.4.x only (i.e. backport patch), we'd
> instead check for:
>> +        || (((forbidden & NOT_IN_PROXY)
>> +             || (forbidden & NOT_IN_DIR_LOC_FILE) == NOT_IN_DIR_LOC_FILE
>> +             || (forbidden & GLOBAL_ONLY) == GLOBAL_ONLY)
>> +            && ((found = ...)
>> +                || ...)))
> ?

Looks like there are other usages of NOT_IN_DIR_LOC_FILE we should
hack in ap_check_cmd_context() too, but you probably see the idea...

Re: svn commit: r1740928 - in /httpd/httpd/trunk: ./ include/ modules/http2/ modules/proxy/ modules/ssl/ server/

Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Oct 11, 2017 at 5:15 PM, William A Rowe Jr <wr...@rowe-clan.net> wrote:
>>
>> +#define  NOT_IN_PROXY           0x40 /**< Forbidden in &lt;Proxy&gt; */
>> +/** Forbidden in
>> &lt;Directory&gt;/&lt;Location&gt;/&lt;Files&gt;&lt;If&gt;&lt;Proxy&gt;*/
>> +#define  NOT_IN_DIR_LOC_FILE
>> (NOT_IN_DIRECTORY|NOT_IN_LOCATION|NOT_IN_FILES|NOT_IN_PROXY)
>> +/** Forbidden in
>> &lt;VirtualHost&gt;/&lt;Limit&gt;/&lt;Directory&gt;/&lt;Location&gt;/&lt;Files&gt;/&lt;If&gt;&lt;Proxy&gt;*/
>>  #define  GLOBAL_ONLY
>> (NOT_IN_VIRTUALHOST|NOT_IN_LIMIT|NOT_IN_DIR_LOC_FILE)
>
> Reviewing this in much more depth yesterday ... this doesn't seem
> to be a minor MMN bump, so would not be backportable without some
> extra heavy lifting on 2.4 interop.
>
> A module previously compiled against 2.4 would not carry the newly
> correct definition of NOT_IN_DIR_LOC_FILE because all previously
> compiled modules are missing the 0x40 bit, right?

Hmm, right I suppose.

So provided a user updates core to 2.4.thispatch but does not
recompile some modules using NOT_IN_DIR_LOC_FILE or GLOBAL_ONLY
against it (which is valid), then the new core would allow such
defined directives in <Proxy context (looks like the only
consequence).

That's because in the new ap_check_cmd_context(), this:
> +        || ((forbidden & NOT_IN_PROXY)
> +            && ((found = find_parent(..., "<Proxy"))
> +                || (found = find_parent(..., "<ProxyMatch"))))) {
>          return apr_pstrcat(cmd->pool, cmd->cmd->name, gt,
>                             " cannot occur within ", found->directive,
>                             "> section", NULL);
wouldn't match.

Thus, how about if there, for 2.4.x only (i.e. backport patch), we'd
instead check for:
> +        || (((forbidden & NOT_IN_PROXY)
> +             || (forbidden & NOT_IN_DIR_LOC_FILE) == NOT_IN_DIR_LOC_FILE
> +             || (forbidden & GLOBAL_ONLY) == GLOBAL_ONLY)
> +            && ((found = ...)
> +                || ...)))
?

ISTM that it's strictly equivalent, without the compatibility caveat.

Does it work for you?

>
> That would be the definition of an MMN major bump, previously
> compiled modules require recompilation.

Yeah, we don't want that in 2.4.x, but SSLProxy* directives per
<Proxy> sections yes :)
At least I am, and will try ;)