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

Add Host extractor #827

Merged
merged 7 commits into from
Mar 6, 2022
Merged

Add Host extractor #827

merged 7 commits into from
Mar 6, 2022

Conversation

tbillington
Copy link
Contributor

Motivation

This pr closes #826, implementing the host extractor.

Solution

  1. Check the host portion of the uri to see if it's already been provided, returning if it has
  2. Read from the "host" header value and return if it was present
  3. Return an empty string if neither strategy yielded a value

axum/src/extract/host.rs Outdated Show resolved Hide resolved
Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

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

Good stuff! Thanks for working on this 😃

axum/src/extract/host.rs Outdated Show resolved Hide resolved
axum/src/extract/host.rs Outdated Show resolved Hide resolved
axum/src/extract/host.rs Show resolved Hide resolved
axum/src/extract/mod.rs Outdated Show resolved Hide resolved
axum/src/extract/host.rs Outdated Show resolved Hide resolved
axum/src/extract/host.rs Outdated Show resolved Hide resolved
Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

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

The code looks good!

Wanna add an entry to the changelog as well? Then we can merge this.

Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

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

Thanks!

@davidpdrsn davidpdrsn enabled auto-merge (squash) March 6, 2022 15:56
auto-merge was automatically disabled March 6, 2022 16:04

Head branch was pushed to by a user without write access

@davidpdrsn davidpdrsn enabled auto-merge (squash) March 6, 2022 16:24
@davidpdrsn davidpdrsn merged commit 843437b into tokio-rs:main Mar 6, 2022
@bluetech
Copy link

bluetech commented Mar 6, 2022

I don't think axum should trust X-Forwarded-Host blindly. The X-Forwarded-* headers are meant to be set by proxies, but a client is free to set them itself, so they should only be trusted when axum is running behind a proxy which sets or santitizes them. But this is circumstantial, especially X-Forwarded-Host is not set often. This will be a security issue if someone ends up using the Host extractor for something security-sensitive.

Here's a recent article about X-Forwarded-For, which is more complex/sensitive, but the gist is also relevant for X-Forwarded-Host.

@jwangnz
Copy link

jwangnz commented Mar 6, 2022

@bluetech I guess it also applies to the Host header as the a client is also free to set the Host header given the Axum is not running behind a proxy.

@bluetech
Copy link

bluetech commented Mar 6, 2022

The client is supposed to set the Host header - it is expected. But the client is not supposed to set X-Forwarded-Host - if it does, that's unexpected. But it's still possible, so it must be considered.

@bluetech
Copy link

bluetech commented Mar 6, 2022

My suggestion is to make it configurable, like Django does.

@tbillington
Copy link
Contributor Author

I'm struggling to see the practical implication.

  • The application developer doesn't see which header the host came from
  • The proxy will set x-forwarded-host if it is set up to, clobbering the user host in the process
  • Fallsback to host header, which the user is supposed to set
  • Optionally it could have been pulled from x-forwarded-host which the user set, but if we're already relying on them to inform us which host then does it matter?

@tbillington
Copy link
Contributor Author

Unless the user is somehow able to override the proxys set header, I don't know enough about proxy servers to know what's common/possible.

@davidpdrsn
Copy link
Member

@bluetech clients can also set the Host header to whatever they want.

curl localhost:3000 -H "Host: foobar"

[examples/hello-world/src/main.rs:25] &headers = {
    "host": "foobar",
    "user-agent": "curl/7.64.1",
    "accept": "*/*",
}

What makes x-forwarded-host different? Seems to me like you can never trust data from clients and have to be careful regardless which header you look at.

@jplatte
Copy link
Member

jplatte commented Mar 7, 2022

Hm, can JavaScript fetch set x-forwarded-host from the browser? Since it's checked first, that would allow to "override" the host set by the browser itself.

Edit: Not forbidden (would've been surprising to see an x-something forbidden, honestly)

@bluetech
Copy link

bluetech commented Mar 7, 2022

I started writing a longer explanation, but I found an article which does so already: https://portswigger.net/web-security/host-header/exploiting. Also https://portswigger.net/web-security/host-header.

@davidpdrsn
Copy link
Member

Still seems to me like devs are gonna have to validate the host header regardless.

@bluetech what do you think about this:

What makes x-forwarded-host different? Seems to me like you can never trust data from clients and have to be careful regardless which header you look at.

@davidpdrsn
Copy link
Member

I've spoken to the lead maintainer of actix-web and think we should keep things as they are now, but add a note to the docs about the security risks. I'll do that.

@bluetech
Copy link

bluetech commented Mar 7, 2022

A warning is good, thanks.

I would still recommend following what Django does, namely ALLOWED_HOSTS (to validate Host - must be specified) and USE_X_FORWARDED_HOST (to opt into using X-Forwarded-Host when it's safe - default false). Because a lot of people won't read the docs and won't bother to validate the host which will expose them to attacks.

I also looked at what Ruby on Rails does -- they once trusted X-Forwarded-Host but made some changes (I'm not sure what exactly... I'm less familiar with rails) - see rails/rails#29893.

I look to Django and Ruby on Rails because they're the most popular web frameworks and have had to deal with every security issue under the sun, so "just do what they do" is a decent heuristic.

@davidpdrsn
Copy link
Member

Axum doesn't have something similar to Rails' url_for. Our closest thing is Redirect but still requires using piecing things together themselves. Axum doesn't implicitly use the host behind the scenes. To me this makes it seem like a different thing. That seems like the crux of Rails' issues.

@bluetech
Copy link

bluetech commented Mar 7, 2022

Right, I guess it depends on what is the usecase for the Host extractor in the first place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add host extractor
5 participants