Dave Voutila writes:
> Claudio Jeker writes:
>
>> On Wed, Apr 28, 2021 at 09:55:16AM -0400, Dave Voutila wrote:
>>>
>>> Claudio Jeker writes:
>>>
>>> > On Wed, Apr 28, 2021 at 08:18:47AM -0400, Dave Voutila wrote:
>>> >>
>>> >> Claudio Jeker writes:
>>> >>
>>> >> > Another thing to consider is that X-Forwarded headers should only be
>>> >> > accepted from trusted sources. I don't think this particular usage of
>>> >> > X-Forwarded-Proto is probelmatic. In the end for this particular case
>>> >> > of
>>> >> > redirect using a relative URL seems to be a better choice since it will
>>> >> > preserve the host, port and proto from the original requests without
>>> >> > any
>>> >> > configuration magic.
>>> >>
>>> >> Here's a quick effort to test out the idea. Not sure I like it yet...but
>>> >> figured this could help move discussion forward. (Not looking for an OK
>>> >> yet as I doubt this config style is good yet.)
>>> >>
>>> >> Couple points:
>>> >> * the srvflag bitmap is sort of at the semantic breaking point as a
>>> >> uint32_t...hesitant to bump it to uint64_t just to allow better flag
>>> >> groupings but that's a thought
>>> >>
>>> >> * not sure i like my config style...maybe (yet another) optional token
>>> >> or two in the "auto index" pattern instead?
>>> >>
>>> >> Quick example config folks can test with. First `# cp -R /var/www /tmp`
>>> >> and then stage some files there for your enjoyment.
>>> >>
>>> >> chroot "/tmp/www"
>>> >> server "default" {
>>> >> listen on * port 80
>>> >> directory {
>>> >> auto index
>>> >> relative redirects
>>> >> }
>>> >> }
>>> >>
>>> >> Example without "relative redirects":
>>> >>
>>> >> ~ $ curl -i localhost/bgplg
>>> >> HTTP/1.0 301 Moved Permanently
>>> >> Server: OpenBSD httpd
>>> >> Connection: close
>>> >> Content-Type: text/html
>>> >> Content-Length: 510
>>> >> Location: http://localhost/bgplg/
>>> >>
>>> >> With "relative redirects":
>>> >>
>>> >> ~ $ curl -i localhost/bgplg
>>> >> HTTP/1.0 301 Moved Permanently
>>> >> Server: OpenBSD httpd
>>> >> Connection: close
>>> >> Content-Type: text/html
>>> >> Content-Length: 510
>>> >> Location: /bgplg/
>>> >>
>>> >
>>> > To be honest I feel this is total overkill. This is an automatic redirect
>>> > from /foo/bar to /foo/bar/ when accessing directories that have an index
>>> > file or have auto index on. It is one very specific case of redirect that
>>> > never changes the host, port or protocol. I see no reason why this can't
>>> > default to a relative redirect (but using an absolute path).
>>> >
>>>
>>> My first concern is the behavior of user-agent caching. Since the
>>> request was a full url, but the redirect is relative, it's not clear if
>>> there might be issues or not.
>>
>> Unless the client is broken it should not matter I think.
>>
>>> > Are there clients in the wild that fail to handle such a redirect?
>>>
>>> I'm doing some research, but I found this interesting commit [1] from long
>>> ago (2006) which allude to this caching issue I'm describing above.
>>>
>>> Moreover, that commit had me look into RFC 2616 (the HTTP/1.1 spec),
>>> which states we technically violate spec if we move to "always relative"
>>> redirects:
>>>
>>> 14.30 Location
>>>
>>> The Location response-header field is used to redirect the recipient
>>> to a location other than the Request-URI for completion of the
>>> request or identification of a new resource. For 201 (Created)
>>> responses, the Location is that of the new resource which was created
>>> by the request. For 3xx responses, the location SHOULD indicate the
>>> server's preferred URI for automatic redirection to the resource. The
>>> field value consists of a single absolute URI.
>>>
>>> Location = "Location" ":" absoluteURI
>>>
>>> In practice, it seems user-agents handle relative Location values...so
>>> the question is do we want to follow the RFC by default or a "de facto"
>>> behavior? (Not sure how other decisions have been made in httpd(8)
>>> previously.)
>>
>> But then there is RFC7231 (update of 2616) that defines Location a bit
>> different.
>>
>> 7.1.2. Location
>>
>> The "Location" header field is used in some responses to refer to a
>> specific resource in relation to the response. The type of
>> relationship is defined by the combination of request method and
>> status code semantics.
>>
>> Location = URI-reference
>>
>> The field value consists of a single URI-reference. When it has the
>> form of a relative reference ([RFC3986], Section 4.2), the final
>> value is computed by resolving it against the effective request URI
>> ([RFC3986], Section 5).
>>
>> ...
>>
>> With RFC3986 defining the various relative URI. As usual with anything web
>> related there are a lot of cargo cults and this feels like one of those
>> cases.
>
> Ha, I should have known it was superseded! In that case, I like the
> default relative redirect idea. It's as simple as the below diff.
>
> Any objections?
I wasn't aware relative redirects were a thing now! In that case,
I think this is a better solution than reading X-Forwarded-Proto.
Thanks for the discussion!
>
>
> Index: server_file.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/httpd/server_file.c,v
> retrieving revision 1.69
> diff -u -p -r1.69 server_file.c
> --- server_file.c 16 Mar 2021 06:44:14 -0000 1.69
> +++ server_file.c 28 Apr 2021 14:25:06 -0000
> @@ -85,9 +85,7 @@ server_file_access(struct httpd *env, st
> if (path[strlen(path) - 1] != '/') {
> if ((encodedpath = url_encode(desc->http_path)) == NULL)
> return (500);
> - if (asprintf(&newpath, "http%s://%s%s/",
> - srv_conf->flags & SRVFLAG_TLS ? "s" : "",
> - desc->http_host, encodedpath) == -1) {
> + if (asprintf(&newpath, "%s/", encodedpath) == -1) {
> free(encodedpath);
> return (500);
> }
