Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Host and :authority must agree #968

Merged
merged 7 commits into from
Sep 24, 2021
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions draft-ietf-httpbis-http2bis.xml
Original file line number Diff line number Diff line change
Expand Up @@ -2934,16 +2934,29 @@ cookie: e=f
pseudo-header field to convey authority information, unless there is no authority
information to convey (in which case it MUST NOT generate :authority).
</t>
<t>
Clients MUST NOT generate a request with a <tt>Host</tt> header field that differs
from the <tt>:authority</tt> pseudo-header field. A server MAY treat a request as
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I get your reasoning for MAY here instead of something stronger. It seems that this is a security issue, so we should have stronger reasoning for optionality here. Personally I think this should be at least SHOULD, if not MUST.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reasoning is that old implementations won't apply this rule (so it can't be MUST) and some might choose not to (as it requires an extra comparison). I'm OK with "SHOULD", but "MAY" seemed safer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO if there's significant security impact, it can be a MUST - especially since this is about recipient behaviour, not sender behaviour.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @mnot. I also think that for senders we can safely say that the fields MUST be byte-for-byte identical.

malformed if it contains a <tt>Host</tt> header field that is different from the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is 'different' here? If one is present and one isn't, is that different? What if they have case differences? Etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't specify that deliberately, allowing someone to treat "example.com" and "example.com:443" and "Example.com" each as different to the other.

Would it be best to point that out?

Servers are not required to normalize these fields before comparing them, which means that clients that produce both fields need to make the values byte-for-byte identical.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or just add 'when their values are compared, byte-for-byte'

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They should not be compared byte-for-byte, we did this mistake recently and broke a few clients :-)
RFC3986#6.2.3 explains the scheme-based normalization, that works particularly well for H2 (in short, drop ":80" on "http" and ":443" on "https").

I think I'm fine with a SHOULD though. As Stefan (I think) mentioned, most implementations currently do not perform the comparison and a MUST would instantly make them non-compliant.

HTTP/1.1 messaging proceeds differently (in 3.2). It enumerates what a client must send, then says that a recipient MUST reject as badreq any request with invalid or missing Host. This leaves a bit of gray area about what you consider as invalid but does mandate that some checks are performed. I'm not sure I like this approach better, but I wanted to note it if that can fuel the discussion.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely think we should leave a breadcrumb to, say, scheme-based normalization, as well as to the relevant bit of the -semantics draft (§ 7.2 Host and Authority).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with citing RFC 3986 is that it doesn't actually specify anything in Section 6.2. It's all examples.

Section 7.2 of -semantics is similarly unhelpful.

However, as the server is authoritative, perhaps we can rely on its own definitions of authority and say if the fields identify different authorities - by its own definition - it can reject the request. Would that work?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we'd apply the same guidance to intermediaries? I think that's probably acceptable from where I'm sitting.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I've worked something out. It's pretty gross though. Let me know what you think.

value of the <tt>:authority</tt> pseudo-header field.
</t>
<t>
An intermediary that forwards a request over HTTP/2 MUST construct an
<tt>:authority</tt> pseudo-header field using the authority information from the
control data of the original request, unless the the original request's target URI
does not contain authority information (in which case it MUST NOT generate
<tt>:authority</tt>). Note that the Host header field is not the sole source of this
information; see <xref target="HTTP" section="7.2"/>.
<tt>:authority</tt>). Note that the <tt>Host</tt> header field is not the sole
source of this information; see <xref target="HTTP" section="7.2"/>.
</t>
<t>
An intermediary that forwards a request received in HTTP/2 via HTTP/1.1 MUST set the
Copy link
Contributor

@wtarreau wtarreau Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still embarrassed by the normative language involving H1 here. What about this variant:

An intermediary that needs to produce a Host header field (e.g. to translate
an HTTP/2 request to HTTP/1.1) MUST set the ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that's a good suggestion. I've taken it.

<tt>Host</tt> field in the forwarded request to the value from the
<tt>:authority</tt> pseudo-header field, unless it changes the request target. This
martinthomson marked this conversation as resolved.
Show resolved Hide resolved
replaces any existing <tt>Host</tt> field to avoid potential vulnerabilities in HTTP
routing.
</t>
<t>
An intermediary that forwards a request over HTTP/2 MUST retain any <tt>Host</tt>
An intermediary that forwards a request over HTTP/2 MAY retain any <tt>Host</tt>
header field.
</t>
<t>
Expand Down