You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by ic...@apache.org on 2022/02/10 10:59:08 UTC
svn commit: r1897940 - in /httpd/httpd/trunk: changes-entries/http2_request_scheme.txt modules/http2/h2_c2.c modules/http2/h2_request.c modules/http2/h2_stream.c test/modules/http2/test_003_get.py
Author: icing
Date: Thu Feb 10 10:59:08 2022
New Revision: 1897940
URL: http://svn.apache.org/viewvc?rev=1897940&view=rev
Log:
*) mod_http2: :scheme pseudo-header values, not matching the
connection scheme, are forwarded via absolute uris to the
http protocol processing to preserve semantics of the request.
Checks on combinations of pseudo-headers values/absence
have been added as described in RFC 7540.
Fixes <https://github.com/icing/mod_h2/issues/230>.
Modified:
httpd/httpd/trunk/changes-entries/http2_request_scheme.txt
httpd/httpd/trunk/modules/http2/h2_c2.c
httpd/httpd/trunk/modules/http2/h2_request.c
httpd/httpd/trunk/modules/http2/h2_stream.c
httpd/httpd/trunk/test/modules/http2/test_003_get.py
Modified: httpd/httpd/trunk/changes-entries/http2_request_scheme.txt
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/changes-entries/http2_request_scheme.txt?rev=1897940&r1=1897939&r2=1897940&view=diff
==============================================================================
--- httpd/httpd/trunk/changes-entries/http2_request_scheme.txt (original)
+++ httpd/httpd/trunk/changes-entries/http2_request_scheme.txt Thu Feb 10 10:59:08 2022
@@ -1,3 +1,7 @@
- *) mod_http2: when a h2 request carries a ':scheme' pseudoheader,
- it gives a 400 response if the scheme does not match the
- connection. Fixes <https://github.com/icing/mod_h2/issues/230>.
+ *) mod_http2: :scheme pseudo-header values, not matching the
+ connection scheme, are forwarded via absolute uris to the
+ http protocol processing to preserve semantics of the request.
+ Checks on combinations of pseudo-headers values/absence
+ have been added as described in RFC 7540.
+ Fixes <https://github.com/icing/mod_h2/issues/230>.
+ [Stefan Eissing]
Modified: httpd/httpd/trunk/modules/http2/h2_c2.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_c2.c?rev=1897940&r1=1897939&r2=1897940&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/http2/h2_c2.c (original)
+++ httpd/httpd/trunk/modules/http2/h2_c2.c Thu Feb 10 10:59:08 2022
@@ -574,8 +574,8 @@ static apr_status_t c2_process(h2_conn_c
}
ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, c,
- "h2_c2(%s-%d): created request_rec",
- conn_ctx->id, conn_ctx->stream_id);
+ "h2_c2(%s-%d): created request_rec for %s",
+ conn_ctx->id, conn_ctx->stream_id, r->the_request);
conn_ctx->server = r->server;
/* the request_rec->server carries the timeout value that applies */
Modified: httpd/httpd/trunk/modules/http2/h2_request.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_request.c?rev=1897940&r1=1897939&r2=1897940&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/http2/h2_request.c (original)
+++ httpd/httpd/trunk/modules/http2/h2_request.c Thu Feb 10 10:59:08 2022
@@ -16,7 +16,11 @@
#include <assert.h>
-#include <apr_strings.h>
+#include "apr.h"
+#include "apr_strings.h"
+#include "apr_lib.h"
+#include "apr_strmatch.h"
+
#include <ap_mmn.h>
#include <httpd.h>
@@ -25,6 +29,7 @@
#include <http_protocol.h>
#include <http_request.h>
#include <http_log.h>
+#include <http_ssl.h>
#include <http_vhost.h>
#include <util_filter.h>
#include <ap_mpm.h>
@@ -170,7 +175,7 @@ apr_status_t h2_request_add_header(h2_re
apr_status_t h2_request_end_headers(h2_request *req, apr_pool_t *pool, int eos, size_t raw_bytes)
{
const char *s;
-
+
/* rfc7540, ch. 8.1.2.3:
* - if we have :authority, it overrides any Host header
* - :authority MUST be omitted when converting h1->h2, so we
@@ -295,9 +300,30 @@ request_rec *h2_create_request_rec(const
/* Time to populate r with the data we have. */
r->request_time = req->request_time;
- r->the_request = apr_psprintf(r->pool, "%s %s HTTP/2.0",
- req->method, req->path ? req->path : "");
- r->headers_in = apr_table_clone(r->pool, req->headers);
+ AP_DEBUG_ASSERT(req->authority);
+ if (req->scheme && (ap_cstr_casecmp(req->scheme,
+ ap_ssl_conn_is_ssl(c->master? c->master : c)? "https" : "http")
+ || !ap_cstr_casecmp("CONNECT", req->method))) {
+ /* Client sent a non-matching ':scheme' pseudo header. Forward this
+ * via an absolute URI in the request line.
+ */
+ r->the_request = apr_psprintf(r->pool, "%s %s://%s%s HTTP/2.0",
+ req->method, req->scheme, req->authority,
+ req->path ? req->path : "");
+ }
+ else if (req->path) {
+ r->the_request = apr_psprintf(r->pool, "%s %s HTTP/2.0",
+ req->method, req->path);
+ }
+ else {
+ /* We should only come here on a request that is errored already.
+ * create a request line that passes parsing, we'll die anyway.
+ */
+ AP_DEBUG_ASSERT(req->http_status != H2_HTTP_STATUS_UNSET);
+ r->the_request = apr_psprintf(r->pool, "%s / HTTP/2.0", req->method);
+ }
+
+ r->headers_in = apr_table_copy(r->pool, req->headers);
/* Start with r->hostname = NULL, ap_check_request_header() will get it
* form Host: header, otherwise we get complains about port numbers.
@@ -399,6 +425,8 @@ request_rec *h2_create_request_rec(const
return r;
die:
+ ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, c,
+ "ap_die(%d) for %s", access_status, r->the_request);
ap_die(access_status, r);
/* ap_die() sent the response through the output filters, we must now
Modified: httpd/httpd/trunk/modules/http2/h2_stream.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_stream.c?rev=1897940&r1=1897939&r2=1897940&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/http2/h2_stream.c (original)
+++ httpd/httpd/trunk/modules/http2/h2_stream.c Thu Feb 10 10:59:08 2022
@@ -17,7 +17,10 @@
#include <assert.h>
#include <stddef.h>
-#include <apr_strings.h>
+#include "apr.h"
+#include "apr_strings.h"
+#include "apr_lib.h"
+#include "apr_strmatch.h"
#include <httpd.h>
#include <http_core.h>
@@ -770,33 +773,97 @@ apr_status_t h2_stream_end_headers(h2_st
{
apr_status_t status;
val_len_check_ctx ctx;
-
- status = h2_request_end_headers(stream->rtmp, stream->pool, eos, raw_bytes);
- if (APR_SUCCESS == status) {
- set_policy_for(stream, stream->rtmp);
+ int is_http_or_https;
+ h2_request *req = stream->rtmp;
- ctx.maxlen = stream->session->s->limit_req_fieldsize;
- ctx.failed_key = NULL;
- apr_table_do(table_check_val_len, &ctx, stream->rtmp->headers, NULL);
- if (ctx.failed_key) {
- if (!h2_stream_is_ready(stream)) {
- ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, stream->session->c1,
- H2_STRM_LOG(APLOGNO(10230), stream,"Request header exceeds "
- "LimitRequestFieldSize: %.*s"),
- (int)H2MIN(strlen(ctx.failed_key), 80), ctx.failed_key);
- }
- set_error_response(stream, HTTP_REQUEST_HEADER_FIELDS_TOO_LARGE);
- /* keep on returning APR_SUCCESS, so that we send a HTTP response and
- * do not RST the stream. */
- }
- if (stream->rtmp->scheme && strcasecmp(stream->rtmp->scheme,
- ap_ssl_conn_is_ssl(stream->session->c1)? "https" : "http")) {
- ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, stream->session->c1,
- H2_STRM_LOG(APLOGNO(10379), stream,"Request :scheme '%s' and "
- "connection do not match."), stream->rtmp->scheme);
+ status = h2_request_end_headers(req, stream->pool, eos, raw_bytes);
+ if (APR_SUCCESS != status || req->http_status != H2_HTTP_STATUS_UNSET) goto cleanup;
+
+ /* keep on returning APR_SUCCESS for error responses, so that we
+ * send it and do not RST the stream.
+ */
+ set_policy_for(stream, req);
+
+ ctx.maxlen = stream->session->s->limit_req_fieldsize;
+ ctx.failed_key = NULL;
+ apr_table_do(table_check_val_len, &ctx, req->headers, NULL);
+ if (ctx.failed_key) {
+ if (!h2_stream_is_ready(stream)) {
+ ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, stream->session->c1,
+ H2_STRM_LOG(APLOGNO(10230), stream,"Request header exceeds "
+ "LimitRequestFieldSize: %.*s"),
+ (int)H2MIN(strlen(ctx.failed_key), 80), ctx.failed_key);
+ }
+ set_error_response(stream, HTTP_REQUEST_HEADER_FIELDS_TOO_LARGE);
+ goto cleanup;
+ }
+
+ /* http(s) scheme. rfc7540, ch. 8.1.2.3:
+ * This [:path] pseudo-header field MUST NOT be empty for "http" or "https"
+ * URIs; "http" or "https" URIs that do not contain a path component
+ * MUST include a value of '/'. The exception to this rule is an
+ * OPTIONS request for an "http" or "https" URI that does not include
+ * a path component; these MUST include a ":path" pseudo-header field
+ * with a value of '*'
+ *
+ * All HTTP/2 requests MUST include exactly one valid value for the
+ * ":method", ":scheme", and ":path" pseudo-header fields, unless it is
+ * a CONNECT request.
+ */
+ is_http_or_https = (!req->scheme
+ || !(ap_cstr_casecmpn(req->scheme, "http", 4) != 0
+ || (req->scheme[4] != '\0'
+ && (apr_tolower(req->scheme[4]) != 's'
+ || req->scheme[5] != '\0'))));
+
+ /* CONNECT. rfc7540, ch. 8.3:
+ * In HTTP/2, the CONNECT method is used to establish a tunnel over a
+ * single HTTP/2 stream to a remote host for similar purposes. The HTTP
+ * header field mapping works as defined in Section 8.1.2.3 ("Request
+ * Pseudo-Header Fields"), with a few differences. Specifically:
+ * o The ":method" pseudo-header field is set to "CONNECT".
+ * o The ":scheme" and ":path" pseudo-header fields MUST be omitted.
+ * o The ":authority" pseudo-header field contains the host and port to
+ * connect to (equivalent to the authority-form of the request-target
+ * of CONNECT requests (see [RFC7230], Section 5.3)).
+ */
+ if (!ap_cstr_casecmp(req->method, "CONNECT")) {
+ if (req->scheme || req->path) {
+ ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, stream->session->c1,
+ H2_STRM_LOG(APLOGNO(), stream, "Request to CONNECT "
+ "with :scheme or :path specified, sending 400 answer"));
set_error_response(stream, HTTP_BAD_REQUEST);
+ goto cleanup;
+ }
+ }
+ else if (is_http_or_https) {
+ if (!req->path) {
+ ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, stream->session->c1,
+ H2_STRM_LOG(APLOGNO(), stream, "Request for http(s) "
+ "resource without :path, sending 400 answer"));
+ set_error_response(stream, HTTP_BAD_REQUEST);
+ goto cleanup;
+ }
+ if (!req->scheme) {
+ req->scheme = ap_ssl_conn_is_ssl(stream->session->c1)? "https" : "http";
}
- stream->request = stream->rtmp;
+ }
+
+ if (req->scheme && (req->path && req->path[0] != '/')) {
+ /* We still have a scheme, which means we need to pass an absolute URI into
+ * our HTTP protocol handling and the missing '/' at the start will prevent
+ * us from doing so (as it then confuses path and authority). */
+ ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, stream->session->c1,
+ H2_STRM_LOG(APLOGNO(10379), stream, "Request :scheme '%s' and "
+ "path '%s' do not allow creating an absolute URL. Failing "
+ "request with 400."), req->scheme, req->path);
+ set_error_response(stream, HTTP_BAD_REQUEST);
+ goto cleanup;
+ }
+
+cleanup:
+ if (APR_SUCCESS == status) {
+ stream->request = req;
stream->rtmp = NULL;
}
return status;
@@ -1108,6 +1175,7 @@ const h2_priority *h2_stream_get_priorit
int h2_stream_is_ready(h2_stream *stream)
{
+ /* Have we sent a response or do we have the response in our buffer? */
if (stream->response) {
return 1;
}
Modified: httpd/httpd/trunk/test/modules/http2/test_003_get.py
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/test/modules/http2/test_003_get.py?rev=1897940&r1=1897939&r2=1897940&view=diff
==============================================================================
--- httpd/httpd/trunk/test/modules/http2/test_003_get.py (original)
+++ httpd/httpd/trunk/test/modules/http2/test_003_get.py Thu Feb 10 10:59:08 2022
@@ -215,7 +215,15 @@ content-type: text/html
# use an invalid scheme
def test_h2_003_51(self, env):
url = env.mkurl("https", "cgi", "/")
- opt = ["-H:scheme: http"]
+ opt = ["-H:scheme: invalid"]
r = env.nghttp().get(url, options=opt)
assert r.exit_code == 0, r
assert r.response['status'] == 400
+
+ # use an differing scheme, but one that is acceptable
+ def test_h2_003_52(self, env):
+ url = env.mkurl("https", "cgi", "/")
+ opt = ["-H:scheme: http"]
+ r = env.nghttp().get(url, options=opt)
+ assert r.exit_code == 0, r
+ assert r.response['status'] == 200
Re: svn commit: r1897940 - in /httpd/httpd/trunk: changes-entries/http2_request_scheme.txt modules/http2/h2_c2.c modules/http2/h2_request.c modules/http2/h2_stream.c test/modules/http2/test_003_get.py
Posted by Stefan Eissing <st...@eissing.org>.
> Am 10.02.2022 um 15:26 schrieb Ruediger Pluem <rp...@apache.org>:
>
>
>
> On 2/10/22 2:18 PM, Stefan Eissing wrote:
>>
>>
>>> Am 10.02.2022 um 14:00 schrieb Ruediger Pluem <rp...@apache.org>:
>>>
>>>
>>>
>>> On 2/10/22 11:59 AM, icing@apache.org wrote:
>>>> Author: icing
>>>> Date: Thu Feb 10 10:59:08 2022
>>>> New Revision: 1897940
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=1897940&view=rev
>>>> Log:
>>>> *) mod_http2: :scheme pseudo-header values, not matching the
>>>> connection scheme, are forwarded via absolute uris to the
>>>> http protocol processing to preserve semantics of the request.
>>>> Checks on combinations of pseudo-headers values/absence
>>>> have been added as described in RFC 7540.
>>>> Fixes <https://github.com/icing/mod_h2/issues/230>.
>>>>
>>>>
>>>> Modified:
>>>> httpd/httpd/trunk/changes-entries/http2_request_scheme.txt
>>>> httpd/httpd/trunk/modules/http2/h2_c2.c
>>>> httpd/httpd/trunk/modules/http2/h2_request.c
>>>> httpd/httpd/trunk/modules/http2/h2_stream.c
>>>> httpd/httpd/trunk/test/modules/http2/test_003_get.py
>>>>
>>>
>>>> Modified: httpd/httpd/trunk/modules/http2/h2_request.c
>>>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_request.c?rev=1897940&r1=1897939&r2=1897940&view=diff
>>>> ==============================================================================
>>>> --- httpd/httpd/trunk/modules/http2/h2_request.c (original)
>>>> +++ httpd/httpd/trunk/modules/http2/h2_request.c Thu Feb 10 10:59:08 2022
>>>
>>>> @@ -295,9 +300,30 @@ request_rec *h2_create_request_rec(const
>>>>
>>>> /* Time to populate r with the data we have. */
>>>> r->request_time = req->request_time;
>>>> - r->the_request = apr_psprintf(r->pool, "%s %s HTTP/2.0",
>>>> - req->method, req->path ? req->path : "");
>>>> - r->headers_in = apr_table_clone(r->pool, req->headers);
>>>> + AP_DEBUG_ASSERT(req->authority);
>>>> + if (req->scheme && (ap_cstr_casecmp(req->scheme,
>>>> + ap_ssl_conn_is_ssl(c->master? c->master : c)? "https" : "http")
>>>> + || !ap_cstr_casecmp("CONNECT", req->method))) {
>>>> + /* Client sent a non-matching ':scheme' pseudo header. Forward this
>>>> + * via an absolute URI in the request line.
>>>> + */
>>>> + r->the_request = apr_psprintf(r->pool, "%s %s://%s%s HTTP/2.0",
>>>> + req->method, req->scheme, req->authority,
>>>> + req->path ? req->path : "");
>>>> + }
>>>> + else if (req->path) {
>>>> + r->the_request = apr_psprintf(r->pool, "%s %s HTTP/2.0",
>>>> + req->method, req->path);
>>>> + }
>>>> + else {
>>>> + /* We should only come here on a request that is errored already.
>>>> + * create a request line that passes parsing, we'll die anyway.
>>>> + */
>>>> + AP_DEBUG_ASSERT(req->http_status != H2_HTTP_STATUS_UNSET);
>>>> + r->the_request = apr_psprintf(r->pool, "%s / HTTP/2.0", req->method);
>>>> + }
>>>> +
>>>> + r->headers_in = apr_table_copy(r->pool, req->headers);
>>>
>>> Why no longer cloning? Are all req->headers elements allocated from r->pool anyway and are possible modifications of the headers
>>> on one side no issue for the other side?
>>
>> The table itself can be modified during processing. The existing values and keys live in stream->pool and that one lives as long as the request. So, a copy of the table should suffice, IMO.
>
> Agree on the lifecycle topic. What if header (values) would get changed in place on either side? Do you see an issue for the other
> side or are we sure that these changes never happen and if values to headers need to be changed the pointer to the value will be
> replaced in the respective table with a new pointer to the new value.
If my understanding of apr_table is correct, apr_table_copy() copies all apr_table_entry_t by value. So only the key and value strings are not dup()ed in comparison to apr_table_clone().
Replacing value pointers to a new string would then not be a problem. Modifying a value 'in place' would show also in the original table. I don't think we do that, but I do not think that it would pose a problem in this case. mod_http2 makes no use of the source table afterwards, really.
I think. But it is good to have your keen eyes on this.
Kind Regards,
Stefan
> Regards
>
> Rüdiger
Re: svn commit: r1897940 - in /httpd/httpd/trunk: changes-entries/http2_request_scheme.txt modules/http2/h2_c2.c modules/http2/h2_request.c modules/http2/h2_stream.c test/modules/http2/test_003_get.py
Posted by Ruediger Pluem <rp...@apache.org>.
On 2/10/22 2:18 PM, Stefan Eissing wrote:
>
>
>> Am 10.02.2022 um 14:00 schrieb Ruediger Pluem <rp...@apache.org>:
>>
>>
>>
>> On 2/10/22 11:59 AM, icing@apache.org wrote:
>>> Author: icing
>>> Date: Thu Feb 10 10:59:08 2022
>>> New Revision: 1897940
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1897940&view=rev
>>> Log:
>>> *) mod_http2: :scheme pseudo-header values, not matching the
>>> connection scheme, are forwarded via absolute uris to the
>>> http protocol processing to preserve semantics of the request.
>>> Checks on combinations of pseudo-headers values/absence
>>> have been added as described in RFC 7540.
>>> Fixes <https://github.com/icing/mod_h2/issues/230>.
>>>
>>>
>>> Modified:
>>> httpd/httpd/trunk/changes-entries/http2_request_scheme.txt
>>> httpd/httpd/trunk/modules/http2/h2_c2.c
>>> httpd/httpd/trunk/modules/http2/h2_request.c
>>> httpd/httpd/trunk/modules/http2/h2_stream.c
>>> httpd/httpd/trunk/test/modules/http2/test_003_get.py
>>>
>>
>>> Modified: httpd/httpd/trunk/modules/http2/h2_request.c
>>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_request.c?rev=1897940&r1=1897939&r2=1897940&view=diff
>>> ==============================================================================
>>> --- httpd/httpd/trunk/modules/http2/h2_request.c (original)
>>> +++ httpd/httpd/trunk/modules/http2/h2_request.c Thu Feb 10 10:59:08 2022
>>
>>> @@ -295,9 +300,30 @@ request_rec *h2_create_request_rec(const
>>>
>>> /* Time to populate r with the data we have. */
>>> r->request_time = req->request_time;
>>> - r->the_request = apr_psprintf(r->pool, "%s %s HTTP/2.0",
>>> - req->method, req->path ? req->path : "");
>>> - r->headers_in = apr_table_clone(r->pool, req->headers);
>>> + AP_DEBUG_ASSERT(req->authority);
>>> + if (req->scheme && (ap_cstr_casecmp(req->scheme,
>>> + ap_ssl_conn_is_ssl(c->master? c->master : c)? "https" : "http")
>>> + || !ap_cstr_casecmp("CONNECT", req->method))) {
>>> + /* Client sent a non-matching ':scheme' pseudo header. Forward this
>>> + * via an absolute URI in the request line.
>>> + */
>>> + r->the_request = apr_psprintf(r->pool, "%s %s://%s%s HTTP/2.0",
>>> + req->method, req->scheme, req->authority,
>>> + req->path ? req->path : "");
>>> + }
>>> + else if (req->path) {
>>> + r->the_request = apr_psprintf(r->pool, "%s %s HTTP/2.0",
>>> + req->method, req->path);
>>> + }
>>> + else {
>>> + /* We should only come here on a request that is errored already.
>>> + * create a request line that passes parsing, we'll die anyway.
>>> + */
>>> + AP_DEBUG_ASSERT(req->http_status != H2_HTTP_STATUS_UNSET);
>>> + r->the_request = apr_psprintf(r->pool, "%s / HTTP/2.0", req->method);
>>> + }
>>> +
>>> + r->headers_in = apr_table_copy(r->pool, req->headers);
>>
>> Why no longer cloning? Are all req->headers elements allocated from r->pool anyway and are possible modifications of the headers
>> on one side no issue for the other side?
>
> The table itself can be modified during processing. The existing values and keys live in stream->pool and that one lives as long as the request. So, a copy of the table should suffice, IMO.
Agree on the lifecycle topic. What if header (values) would get changed in place on either side? Do you see an issue for the other
side or are we sure that these changes never happen and if values to headers need to be changed the pointer to the value will be
replaced in the respective table with a new pointer to the new value.
Regards
Rüdiger
Re: svn commit: r1897940 - in /httpd/httpd/trunk: changes-entries/http2_request_scheme.txt modules/http2/h2_c2.c modules/http2/h2_request.c modules/http2/h2_stream.c test/modules/http2/test_003_get.py
Posted by Stefan Eissing <st...@eissing.org>.
> Am 10.02.2022 um 14:00 schrieb Ruediger Pluem <rp...@apache.org>:
>
>
>
> On 2/10/22 11:59 AM, icing@apache.org wrote:
>> Author: icing
>> Date: Thu Feb 10 10:59:08 2022
>> New Revision: 1897940
>>
>> URL: http://svn.apache.org/viewvc?rev=1897940&view=rev
>> Log:
>> *) mod_http2: :scheme pseudo-header values, not matching the
>> connection scheme, are forwarded via absolute uris to the
>> http protocol processing to preserve semantics of the request.
>> Checks on combinations of pseudo-headers values/absence
>> have been added as described in RFC 7540.
>> Fixes <https://github.com/icing/mod_h2/issues/230>.
>>
>>
>> Modified:
>> httpd/httpd/trunk/changes-entries/http2_request_scheme.txt
>> httpd/httpd/trunk/modules/http2/h2_c2.c
>> httpd/httpd/trunk/modules/http2/h2_request.c
>> httpd/httpd/trunk/modules/http2/h2_stream.c
>> httpd/httpd/trunk/test/modules/http2/test_003_get.py
>>
>
>> Modified: httpd/httpd/trunk/modules/http2/h2_request.c
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_request.c?rev=1897940&r1=1897939&r2=1897940&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/modules/http2/h2_request.c (original)
>> +++ httpd/httpd/trunk/modules/http2/h2_request.c Thu Feb 10 10:59:08 2022
>
>> @@ -295,9 +300,30 @@ request_rec *h2_create_request_rec(const
>>
>> /* Time to populate r with the data we have. */
>> r->request_time = req->request_time;
>> - r->the_request = apr_psprintf(r->pool, "%s %s HTTP/2.0",
>> - req->method, req->path ? req->path : "");
>> - r->headers_in = apr_table_clone(r->pool, req->headers);
>> + AP_DEBUG_ASSERT(req->authority);
>> + if (req->scheme && (ap_cstr_casecmp(req->scheme,
>> + ap_ssl_conn_is_ssl(c->master? c->master : c)? "https" : "http")
>> + || !ap_cstr_casecmp("CONNECT", req->method))) {
>> + /* Client sent a non-matching ':scheme' pseudo header. Forward this
>> + * via an absolute URI in the request line.
>> + */
>> + r->the_request = apr_psprintf(r->pool, "%s %s://%s%s HTTP/2.0",
>> + req->method, req->scheme, req->authority,
>> + req->path ? req->path : "");
>> + }
>> + else if (req->path) {
>> + r->the_request = apr_psprintf(r->pool, "%s %s HTTP/2.0",
>> + req->method, req->path);
>> + }
>> + else {
>> + /* We should only come here on a request that is errored already.
>> + * create a request line that passes parsing, we'll die anyway.
>> + */
>> + AP_DEBUG_ASSERT(req->http_status != H2_HTTP_STATUS_UNSET);
>> + r->the_request = apr_psprintf(r->pool, "%s / HTTP/2.0", req->method);
>> + }
>> +
>> + r->headers_in = apr_table_copy(r->pool, req->headers);
>
> Why no longer cloning? Are all req->headers elements allocated from r->pool anyway and are possible modifications of the headers
> on one side no issue for the other side?
The table itself can be modified during processing. The existing values and keys live in stream->pool and that one lives as long as the request. So, a copy of the table should suffice, IMO.
>
>>
>> /* Start with r->hostname = NULL, ap_check_request_header() will get it
>> * form Host: header, otherwise we get complains about port numbers.
Re: svn commit: r1897940 - in /httpd/httpd/trunk: changes-entries/http2_request_scheme.txt modules/http2/h2_c2.c modules/http2/h2_request.c modules/http2/h2_stream.c test/modules/http2/test_003_get.py
Posted by Ruediger Pluem <rp...@apache.org>.
On 2/10/22 11:59 AM, icing@apache.org wrote:
> Author: icing
> Date: Thu Feb 10 10:59:08 2022
> New Revision: 1897940
>
> URL: http://svn.apache.org/viewvc?rev=1897940&view=rev
> Log:
> *) mod_http2: :scheme pseudo-header values, not matching the
> connection scheme, are forwarded via absolute uris to the
> http protocol processing to preserve semantics of the request.
> Checks on combinations of pseudo-headers values/absence
> have been added as described in RFC 7540.
> Fixes <https://github.com/icing/mod_h2/issues/230>.
>
>
> Modified:
> httpd/httpd/trunk/changes-entries/http2_request_scheme.txt
> httpd/httpd/trunk/modules/http2/h2_c2.c
> httpd/httpd/trunk/modules/http2/h2_request.c
> httpd/httpd/trunk/modules/http2/h2_stream.c
> httpd/httpd/trunk/test/modules/http2/test_003_get.py
>
> Modified: httpd/httpd/trunk/modules/http2/h2_request.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_request.c?rev=1897940&r1=1897939&r2=1897940&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/http2/h2_request.c (original)
> +++ httpd/httpd/trunk/modules/http2/h2_request.c Thu Feb 10 10:59:08 2022
> @@ -295,9 +300,30 @@ request_rec *h2_create_request_rec(const
>
> /* Time to populate r with the data we have. */
> r->request_time = req->request_time;
> - r->the_request = apr_psprintf(r->pool, "%s %s HTTP/2.0",
> - req->method, req->path ? req->path : "");
> - r->headers_in = apr_table_clone(r->pool, req->headers);
> + AP_DEBUG_ASSERT(req->authority);
> + if (req->scheme && (ap_cstr_casecmp(req->scheme,
> + ap_ssl_conn_is_ssl(c->master? c->master : c)? "https" : "http")
> + || !ap_cstr_casecmp("CONNECT", req->method))) {
> + /* Client sent a non-matching ':scheme' pseudo header. Forward this
> + * via an absolute URI in the request line.
> + */
> + r->the_request = apr_psprintf(r->pool, "%s %s://%s%s HTTP/2.0",
> + req->method, req->scheme, req->authority,
> + req->path ? req->path : "");
> + }
> + else if (req->path) {
> + r->the_request = apr_psprintf(r->pool, "%s %s HTTP/2.0",
> + req->method, req->path);
> + }
> + else {
> + /* We should only come here on a request that is errored already.
> + * create a request line that passes parsing, we'll die anyway.
> + */
> + AP_DEBUG_ASSERT(req->http_status != H2_HTTP_STATUS_UNSET);
> + r->the_request = apr_psprintf(r->pool, "%s / HTTP/2.0", req->method);
> + }
> +
> + r->headers_in = apr_table_copy(r->pool, req->headers);
Why no longer cloning? Are all req->headers elements allocated from r->pool anyway and are possible modifications of the headers
on one side no issue for the other side?
>
> /* Start with r->hostname = NULL, ap_check_request_header() will get it
> * form Host: header, otherwise we get complains about port numbers.