Re: svn commit: r1756540 - in /httpd/httpd/trunk: docs/manual/mod/core.xml modules/http/http_filters.c server/core.c server/protocol.c server/vhost.c

[Yann Ylavic]

On Wed, Aug 17, 2016 at 12:24 AM, William A Rowe Jr  wrote:
> On Aug 16, 2016 4:39 PM, "Yann Ylavic"  wrote:
>> > -AP_INIT_ITERATE("HttpProtocol", set_http_protocol, NULL, RSRC_CONF,
>> > -  "'min=0.9' (default) or 'min=1.0' to allow/deny HTTP/0.9;
>> > "
>> > -  "'liberal', 'strict', 'strict,log-only'"),
>> > +AP_INIT_ITERATE("EnforceHttpProtocol", set_enforce_http_protocol, NULL,
>> > RSRC_CONF,
>> > +  "'Allow0.9' or 'Require1.0' (default) to allow or deny
>> > HTTP/0.9; "
>> > +  "'Unsafe' or 'Strict' (default) to process incorrect
>> > requests"),
>>
>> The original min= was naturally mutually exclusive, and the original
>> = looks more extensible (to me)...
>
> It appeared that both exclusivity tests were borked because the value was
> first set, ergo the old bit was unset, and then a meaningless test occurred
> (doh).
>
> Allow0.9 or Require1.0 is more readily apparent in perusing someone else's
> config files. We have too many uses of generic (and case sensitive? Ick)
> keywords that makes it harder to spot misconfiguration. Do you really want
> to grep for the token and the directive name, or an arbitrary search for
> min= which has a number of contextual meanings?

Fair enough.

>
>> Also, since this directive will mainly be used to relax HTTP protocol
>> (wrt RFC 7230) for compatibility reasons, EnforceHttpProtocol may not
>> be appropriate.
>>
>> How about HttpProtocolPolicy [Strict|Unsafe] min=[0.9|1.0|1.1] ?
>
> I'm open to suggestions, what about HTTPProtocolOptions (option list)?

Works for me too.


Thanks,
Yann.

Re: svn commit: r1756540 - in /httpd/httpd/trunk: docs/manual/mod/core.xml modules/http/http_filters.c server/core.c server/protocol.c server/vhost.c

[William A Rowe Jr]

On Aug 16, 2016 4:39 PM, "Yann Ylavic"  wrote:
>
> On Tue, Aug 16, 2016 at 8:11 PM,   wrote:
> > Author: wrowe
> > Date: Tue Aug 16 18:11:14 2016
> > New Revision: 1756540
> >
> > URL: http://svn.apache.org/viewvc?rev=1756540=rev
> > Log:
> > Rename the previously undocumented HTTPProtocol directive
> > to EnforceHTTPProtocol, and invert the default behavior
> > to strictly observe RFC 7230 unless otherwise configured.
> > And Document This.
> >
> > The relaxation option is renamed 'Unsafe'. 'Strict' is no
> > longer case sensitive. 'min=0.9|1.0' is now the verbose
> > 'Allow0.9' or 'Require1.0' case-insenstive grammer. The
> > exclusivity tests have been modified to detect conflicts.
> >
> > The 'strict,log' option failed to enforce strict conformance,
> > and has been removed. Unsafe, informational logging is possible
> > in any loadable module, after the request data is unsafely
> > accepted.
>
> This probably requires a MMN bump (possibly major w.r.t trunk).

Agreed, I'll note it. Was thinking this was an internal config but it is in
a published header, as you note.

> > Modified:
> > httpd/httpd/trunk/docs/manual/mod/core.xml
> > httpd/httpd/trunk/modules/http/http_filters.c
> > httpd/httpd/trunk/server/core.c
> > httpd/httpd/trunk/server/protocol.c
> > httpd/httpd/trunk/server/vhost.c
>
> Looks like changes to include/http_core.h are missing, at least
> AP_HTTP_CONFORMANCE_UNSAFE is undefined for now.

Will fix that along with MMN.

> > Modified: httpd/httpd/trunk/server/core.c
> > URL:
http://svn.apache.org/viewvc/httpd/httpd/trunk/server/core.c?rev=1756540=1756539=1756540=diff
> >
==
> > --- httpd/httpd/trunk/server/core.c (original)
> > +++ httpd/httpd/trunk/server/core.c Tue Aug 16 18:11:14 2016
> > @@ -4011,37 +4011,39 @@ static const char *set_protocols_honor_o
> []
> > +static const char *set_enforce_http_protocol(cmd_parms *cmd, void
*dummy,
> > + const char *arg)
> >  {
> []
> > +if ((conf->http09_enable & AP_HTTP09_ENABLE) &&
> > +(conf->http09_enable & AP_HTTP09_DISABLE)) {
> > +return "EnforceHttpProtocol 'Allow0.9' and 'Require1.0'"
> > +   " are mutually exclusive";
> > +}
>
> So why define two separate keywords for those?
> Wouldn't "Forbid0.9" be simpler?

IMO, no. I anticipated we might someday add Require1.1 as a third
alternative, as you alluded to below.

> > -AP_INIT_ITERATE("HttpProtocol", set_http_protocol, NULL, RSRC_CONF,
> > -  "'min=0.9' (default) or 'min=1.0' to allow/deny
HTTP/0.9; "
> > -  "'liberal', 'strict', 'strict,log-only'"),
> > +AP_INIT_ITERATE("EnforceHttpProtocol", set_enforce_http_protocol,
NULL, RSRC_CONF,
> > +  "'Allow0.9' or 'Require1.0' (default) to allow or deny
HTTP/0.9; "
> > +  "'Unsafe' or 'Strict' (default) to process incorrect
requests"),
>
> The original min= was naturally mutually exclusive, and the original
> = looks more extensible (to me)...

It appeared that both exclusivity tests were borked because the value was
first set, ergo the old bit was unset, and then a meaningless test occurred
(doh).

Allow0.9 or Require1.0 is more readily apparent in perusing someone else's
config files. We have too many uses of generic (and case sensitive? Ick)
keywords that makes it harder to spot misconfiguration. Do you really want
to grep for the token and the directive name, or an arbitrary search for
min= which has a number of contextual meanings?

> Also, since this directive will mainly be used to relax HTTP protocol
> (wrt RFC 7230) for compatibility reasons, EnforceHttpProtocol may not
> be appropriate.
>
> How about HttpProtocolPolicy [Strict|Unsafe] min=[0.9|1.0|1.1] ?

I'm open to suggestions, what about HTTPProtocolOptions (option list)?

> > Modified: httpd/httpd/trunk/server/protocol.c
> > URL:
http://svn.apache.org/viewvc/httpd/httpd/trunk/server/protocol.c?rev=1756540=1756539=1756540=diff
> >
==
> > --- httpd/httpd/trunk/server/protocol.c (original)
> > +++ httpd/httpd/trunk/server/protocol.c Tue Aug 16 18:11:14 2016
> > @@ -687,13 +686,11 @@ static int read_request_line(request_rec
> >  if (strict) {
> >  ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02418)
> >"Invalid protocol '%s'", r->protocol);
> > -if (enforce_strict) {
> > -r->proto_num = HTTP_VERSION(1,0);
> > -r->protocol  = "HTTP/1.0";
> > -r->connection->keepalive = AP_CONN_CLOSE;
> > -r->status = HTTP_BAD_REQUEST;
> > -return 0;
> > -}
> > +r->proto_num = HTTP_VERSION(1,0);
> > +r->protocol  = "HTTP/1.0";
> > +r->connection->keepalive = AP_CONN_CLOSE;
> > + 

Re: svn commit: r1756540 - in /httpd/httpd/trunk: docs/manual/mod/core.xml modules/http/http_filters.c server/core.c server/protocol.c server/vhost.c

[Yann Ylavic]

On Tue, Aug 16, 2016 at 8:11 PM,   wrote:
> Author: wrowe
> Date: Tue Aug 16 18:11:14 2016
> New Revision: 1756540
>
> URL: http://svn.apache.org/viewvc?rev=1756540=rev
> Log:
> Rename the previously undocumented HTTPProtocol directive
> to EnforceHTTPProtocol, and invert the default behavior
> to strictly observe RFC 7230 unless otherwise configured.
> And Document This.
>
> The relaxation option is renamed 'Unsafe'. 'Strict' is no
> longer case sensitive. 'min=0.9|1.0' is now the verbose
> 'Allow0.9' or 'Require1.0' case-insenstive grammer. The
> exclusivity tests have been modified to detect conflicts.
>
> The 'strict,log' option failed to enforce strict conformance,
> and has been removed. Unsafe, informational logging is possible
> in any loadable module, after the request data is unsafely
> accepted.

This probably requires a MMN bump (possibly major w.r.t trunk).

>
[]
>
> Modified:
> httpd/httpd/trunk/docs/manual/mod/core.xml
> httpd/httpd/trunk/modules/http/http_filters.c
> httpd/httpd/trunk/server/core.c
> httpd/httpd/trunk/server/protocol.c
> httpd/httpd/trunk/server/vhost.c

Looks like changes to include/http_core.h are missing, at least
AP_HTTP_CONFORMANCE_UNSAFE is undefined for now.

>
[]
>
> Modified: httpd/httpd/trunk/server/core.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/core.c?rev=1756540=1756539=1756540=diff
> ==
> --- httpd/httpd/trunk/server/core.c (original)
> +++ httpd/httpd/trunk/server/core.c Tue Aug 16 18:11:14 2016
> @@ -4011,37 +4011,39 @@ static const char *set_protocols_honor_o
[]
> +static const char *set_enforce_http_protocol(cmd_parms *cmd, void *dummy,
> + const char *arg)
>  {
[]
> +if ((conf->http09_enable & AP_HTTP09_ENABLE) &&
> +(conf->http09_enable & AP_HTTP09_DISABLE)) {
> +return "EnforceHttpProtocol 'Allow0.9' and 'Require1.0'"
> +   " are mutually exclusive";
> +}

So why define two separate keywords for those?
Wouldn't "Forbid0.9" be simpler?

>
[]
> -AP_INIT_ITERATE("HttpProtocol", set_http_protocol, NULL, RSRC_CONF,
> -  "'min=0.9' (default) or 'min=1.0' to allow/deny HTTP/0.9; "
> -  "'liberal', 'strict', 'strict,log-only'"),
> +AP_INIT_ITERATE("EnforceHttpProtocol", set_enforce_http_protocol, NULL, 
> RSRC_CONF,
> +  "'Allow0.9' or 'Require1.0' (default) to allow or deny 
> HTTP/0.9; "
> +  "'Unsafe' or 'Strict' (default) to process incorrect 
> requests"),

The original min= was naturally mutually exclusive, and the original
= looks more extensible (to me)...

Also, since this directive will mainly be used to relax HTTP protocol
(wrt RFC 7230) for compatibility reasons, EnforceHttpProtocol may not
be appropriate.

How about HttpProtocolPolicy [Strict|Unsafe] min=[0.9|1.0|1.1] ?

>
[]
> Modified: httpd/httpd/trunk/server/protocol.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/protocol.c?rev=1756540=1756539=1756540=diff
> ==
> --- httpd/httpd/trunk/server/protocol.c (original)
> +++ httpd/httpd/trunk/server/protocol.c Tue Aug 16 18:11:14 2016
> @@ -687,13 +686,11 @@ static int read_request_line(request_rec
>  if (strict) {
>  ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02418)
>"Invalid protocol '%s'", r->protocol);
> -if (enforce_strict) {
> -r->proto_num = HTTP_VERSION(1,0);
> -r->protocol  = "HTTP/1.0";
> -r->connection->keepalive = AP_CONN_CLOSE;
> -r->status = HTTP_BAD_REQUEST;
> -return 0;
> -}
> +r->proto_num = HTTP_VERSION(1,0);
> +r->protocol  = "HTTP/1.0";
> +r->connection->keepalive = AP_CONN_CLOSE;
> +r->status = HTTP_BAD_REQUEST;
> +return 0;
>  }
>  if (3 == sscanf(r->protocol, "%4s/%u.%u", http, , )
>  && (ap_cstr_casecmp("http", http) == 0)

While at it, "HTTP" (caps) looks a good strict candidate here :)


Regards,
Yann.