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

Introduce HttpCompliance.MISMATCHED_AUTHORITY #9312

Closed
wants to merge 6 commits into from

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Feb 3, 2023

  • Checks if provided Host authority matches an absolute target-uri authority
  • Default is to reject with 400 Bad Request
  • Optional HttpCompliance to disable this check.

Signed-off-by: Joakim Erdfelt joakim.erdfelt@gmail.com

+ Checks if provided Host authority matches
  an absolute target-uri authority
+ Default is to reject with 400 Bad Request
+ Optional HttpCompliance to disable this
  check.

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime joakime added Bug For general bugs on Jetty side Sponsored This issue affects a user with a commercial support agreement Specification For all industry Specifications (IETF / Servlet / etc) labels Feb 3, 2023
@joakime joakime added this to the 10.0.x milestone Feb 3, 2023
@joakime joakime requested a review from gregw February 3, 2023 15:17
@joakime joakime self-assigned this Feb 3, 2023
+ use example.org (instead of example.net)
+ fix tests that are now failing due
  to enforcement of absolute target-uri
  authority and provided Host header

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
Copy link
Contributor Author

joakime commented Feb 3, 2023

@gregw this needs more attention.

I added an HttpCompliance.MISMATCHED_AUTHORITY, but in reality the Host header rules in combination with an absolute target-uri authority is different.

Some of the flavors of rules around this ...

  1. The Host header should be present.
  2. The Host header must be empty.
  3. The Host header must be the same as the target-uri authority.
  4. The Host header must be ignored.

The wording has changed over the years here.

RFC9112 - Section 3.2 - Request Target

https://www.rfc-editor.org/rfc/rfc9112#section-3.2

A client MUST send a Host header field (Section 7.2 of [HTTP]) in all HTTP/1.1 request messages. If the target URI includes an authority component, then a client MUST send a field value for Host that is identical to that authority component, excluding any userinfo subcomponent and its "@" delimiter (Section 4.2 of [HTTP]). If the authority component is missing or undefined for the target URI, then a client MUST send a Host header field with an empty field value.

A server MUST respond with a 400 (Bad Request) status code to any HTTP/1.1 request message that lacks a Host header field and to any request message that contains more than one Host header field line or a Host header field with an invalid field value.

RFC7230 - Section 5.4 - Host

https://www.rfc-editor.org/rfc/rfc7230#section-5.4

It says ...

   A client MUST send a Host header field in all HTTP/1.1 request
   messages.  If the target URI includes an authority component, then a
   client MUST send a field-value for Host that is identical to that
   authority component, excluding any userinfo subcomponent and its "@"
   delimiter (Section 2.7.1).  If the authority component is missing or
   undefined for the target URI, then a client MUST send a Host header
   field with an empty field-value.

and ...

   When a proxy receives a request with an absolute-form of
   request-target, the proxy MUST ignore the received Host header field
   (if any) and instead replace it with the host information of the
   request-target.  A proxy that forwards such a request MUST generate a
   new Host field-value based on the received request-target rather than
   forward the received Host field-value.

Which is kind of in line with RFC 2616

RFC 2616 - Section 5.2 - The Resource Identified by a Request

https://www.rfc-editor.org/rfc/rfc2616#section-5.2

   1. If Request-URI is an absoluteURI, the host is part of the
     Request-URI. Any Host header field value in the request MUST be
     ignored.

@@ -1037,22 +1038,32 @@ else if (_endOfContent == EndOfContent.CHUNKED_CONTENT)
LOG.warn("Encountered multiple `Host` headers. Previous `Host` header already seen as `{}`, new `Host` header has appeared as `{}`", _parsedHost, _valueString);
checkViolation(DUPLICATE_HOST_HEADERS);
}
if (!MISMATCHED_AUTHORITY.isAllowedBy(_complianceMode))
{
HttpURI httpURI = HttpURI.build().uri(_method == null ? null : _method.toString(), _uri.toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

We really don't want to go to the effort here build a HttpURI here, only to throw it away and then in fact we have already started building the exact same HttpURI in org.eclipse.jetty.server.HttpChannelOverHttp#startRequest.

Option A) is to change the RequestHandler signature to

    public interface RequestHandler extends HttpHandler
    {
        /**
         * This is the method called by parser when the HTTP request line is parsed
         *
         * @param method The method
         * @param uri The raw bytes of the URI.
         * @param version the http version in use
         * @deprecated use {@link #startRequest(String, HttpURI, HttpVersion)}
         */
        @Deprecated
        default void startRequest(String method, String uri, HttpVersion version)
        {}
        
        /**
         * This is the method called by parser when the HTTP request line is parsed
         *
         * @param method The method
         * @param uri The raw bytes of the URI.
         * @param version the http version in use
         */
        default void startRequest(String method, HttpURI uri, HttpVersion version)
        {
            startRequest(method, uri.toString(), version);
        }
    }

We can then have a recycled HttpURI.Mutable in the parser and build the URI there, so it is available.

Option B) is to move this check to HttpChannelOverHttp

I think option A is OK. Let me do some experiments and I may have a diff for you....

Copy link
Contributor

Choose a reason for hiding this comment

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

@joakime I have created branch jetty-10-9312-HttpParser-HttpURI that has this updated signature.
The only issue is that the field is a Mutable, and we take it as an Immutable as we call startRequest... and that immutable is not available when you need to do this check. So perhaps we need yet another field to remember the immutable?

@gregw
Copy link
Contributor

gregw commented Feb 12, 2023

Currently, I'm thinking we don't want big changes in 9, 10 or 11. Thus perhaps we just add a check in the Request.setMetaData method.

In 12, we can have a think about changing the parser API so that it creates the HttpURI once and only once, rather than doing the mix in that is currently done in setMetaData to combine the URI and host authority.

@sbordet
Copy link
Contributor

sbordet commented Feb 13, 2023

@gregw I agree that doing it in Request.setMetaData() is better because it will work for any protocol, not only HTTP/1.1.
So even in 12, we should keep it there, rather than thinking about changing (only) the HTTP/1.1 parser.

@gregw
Copy link
Contributor

gregw commented Feb 13, 2023

closed in favour of #9343

@gregw gregw closed this Feb 13, 2023
@joakime joakime deleted the fix/jetty-10.0.x/uri-host-mismatch branch March 29, 2023 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side Specification For all industry Specifications (IETF / Servlet / etc) Sponsored This issue affects a user with a commercial support agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants