allow_absolute_url causes misleading logs

[snowp]

The allow_aboslute_url option on the http1 codec options on the HCM imply that leaving at the default of false will mean that absolute URLs are not allowed. Instead, what happens is that the url is passed unchanged as the :path header to the HCM, which results in the full URI being used for route matching. As a result, the common idiom of using prefix: / as "match any" doesn't work because it doesn't match the scheme-prefixed URL.

I'm not sure if this warrants any behavioral changes, but at the very least the docs should be clearer about what the implication of this option is. Maybe even renaming it to handle_absolute_url with V3?

Relevant code: https://github.com/envoyproxy/envoy/blob/master/source/common/http/http1/codec_impl.cc#L544-L572

[alyssawilk]

I think this may be a bug. I wouldn't expect "http://foo.com" to be a valid path. I think that'd imply we could have a url http://http://foo.com which I'm pretty sure isn't actually valid. I suspect what's happening is that http_parser (which handles absolute urls) is treating it as a url and we don't do path validation because we trust http parser, so I think I should just go ahead and fix this.

@PiotrSikora for sanity checking

[alyssawilk]

Actually @snowp can you talk more to this problem?
https://github.com/envoyproxy/envoy/blob/master/source/common/http/conn_manager_impl.cc#L734
should be rejecting everything that isn't relative.
If allow_absolute_url is on, the h1 codec should be breaking absolute urls into the relative components. If it's off, we should fail checks in decodeHeaders and sendLocalReply.

[PiotrSikora]

http://foo.com (i.e. GET http://foo.com HTTP/1.1) is a valid form (see: RFC7230, sec. 5.3.2).

However, it should be split into :scheme, :authority and :path before it's passed to HCM.

I didn't look at the code, but from what @snowp is saying, it seems that's happening with allow_aboslute_url: true, but with allow_aboslute_url: false (default) everything is passed inside :path instead of being rejected, which is definitely an incorrect behavior.

[alyssawilk]

Right, everything is passed through, and then immediately rejected by the HCM early on in decodeHeaders, as far as I can tell

TEST_P(IntegrationTest, BadPath)
verifies that the link above is blocking
[C1][S11522070672761765734] Sending local reply with details absolute_path_rejected

Snow: is it possible you read the 404 as your matchers not matching rather than the URL being rejected as absolute?

[snowp]

We sere seeing the REQ(:path) access log segment display "http://...", which lead to the conclusion that it was the full URL being used as :path when computing the route, so it ended up not matching any of our routes (including our fallback match_prefix: /).

[alyssawilk]

gotcha.
this might lead to confusing access logs, but treating the full url as an invalid path is how absolute url lack-of-support has always been implemented. Even if you match * it's going to 404 due to the linked code.

we could flat out reject it in the codec instead, which AFIK would mean you had no logs data as the stream won't have gotten headers.

[PiotrSikora]

Is there any reason that we don't allow absolute-form by default?

[alyssawilk]

Orthogonal from how to make this behavior less confusing when we disallow:

I'm failing to find where it was added but as I recall it was a high-ish risk PR (we didn't fully trust the url splitting utility) and we didn't yet have runtime overrides, so we made it a config option and defaulted to the old, safe, behavior. There was at least one bug with url parsing (I think it was that query parameters were dropped accidentally?) so it wasn't a terrible call.

Today I would have been inclined to protect it rather than make it configurable. Annoyingly I don't think we can remove the option now that it's there but given it's a BoolValue I don't think it'd be unreasonable to switch the default where it's not configured and add a release note about the switch so folks who want to disallow can explicitly configure it off. Given this is totally legit HTTP/1 I think allowing it by default is the right thing to do. @mattklein123 WDYT? If you agree I'm happy to take that on.

[mattklein123]

Given this is totally legit HTTP/1 I think allowing it by default is the right thing to do. @mattklein123 WDYT? If you agree I'm happy to take that on.

Sure makes sense to me to default to on at this point.

[alyssawilk]

I'm going to re-title.
allow_absolute_url absolutely controls if an absolute url is allowed.
The logs are arguably confusing, though if we'd split it up it could be equally confusing why the url which "should match" didn't match (when it wasn't obvious it was absolute). Less sure what to do about that.

[PiotrSikora]

REQ(:path) and others should log the same value regardless of what form was used, i.e.:

GET http://foo.com HTTP/1.1

GET / HTTP/1.1
Host: foo.com

should evaluate REQ(:path) to / in both cases.

[htuch]

Is this still a v3 API renaming ask or should we remove the label?

[alyssawilk]

I think it's a code fix not an API fix. label updated.

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

[github-actions]

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions.