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 Scheme extractor #2507

Merged
merged 4 commits into from
Oct 19, 2024
Merged

Add Scheme extractor #2507

merged 4 commits into from
Oct 19, 2024

Conversation

bengsparks
Copy link
Contributor

Motivation

Closes #2504.

Solution

Detect the scheme in the following order:

  1. From proto within the Forwarded header
  2. Reading from X-Forwarded-Proto
  3. Extracting from the URI

@bengsparks
Copy link
Contributor Author

bengsparks commented Jan 11, 2024

The test for extracting the scheme from the URI is missing as I haven't groked how to pass a URI with a scheme to the test server.
I would be grateful for a pointer in the right direction; and in the same commit I'll also fix the formatting.

@bengsparks
Copy link
Contributor Author

bengsparks commented Jan 22, 2024

@davidpdrsn Is this ready for merge, or do I need to add more tests?

@Wiezzel
Copy link

Wiezzel commented Feb 21, 2024

May I suggest additionally supporting X-Forwarded-Scheme header? (It's mentioned in a comment, but only X-Forwarded-Proto is supported).

@Wiezzel
Copy link

Wiezzel commented Feb 21, 2024

It also seems to me that the extraction from parts is not really working. I've added a debug print statement there and the parts.uri seems to contain only URI path.

@bengsparks
Copy link
Contributor Author

bengsparks commented Feb 27, 2024

Thanks for your comments @Wiezzel.
I will follow-up sometime during the next weekend.

@bengsparks
Copy link
Contributor Author

@Wiezzel Thanks for taking the time to review the PR.

May I suggest additionally supporting X-Forwarded-Scheme header? (It's mentioned in a comment, but only X-Forwarded-Proto is supported).

Turns out this was a typo; X-Forwarded-Scheme does not exist, or at least I cannot find any reference to it. I have updated the PR to reflect this.

It also seems to me that the extraction from parts is not really working. I've added a debug print statement there and the parts.uri seems to contain only URI path.

I have added a manual test that calls from_request directly upon a manually constructed request, and the extraction seems to work. It would be surprising if it did not, as this is the only part of the implementation that depends solely on existing methods on http's Parts type.

@Wiezzel
Copy link

Wiezzel commented Mar 4, 2024

@bengsparks Please check out this minimal example:

Cargo.toml

[package]
name = "scheme-extractor"
version = "0.1.0"
edition = "2021"

[dependencies]
axum = "0.7.3"
tokio = { version = "1.36.0", features = ["full"] }

[patch.crates-io]
axum = { git = "https://github.com/bengsparks/axum.git" }

main.rs

async fn test(scheme: axum::extract::Scheme) -> String {
    scheme.0
}

#[tokio::main]
async fn main() {
    let app = axum::Router::new()
        .route("/", axum::routing::get(test));

    let listener = tokio::net::TcpListener::bind("0.0.0.0:8000").await.unwrap();
    axum::serve(listener, app).await.unwrap();
}

Then run:

$ cargo run &
$ curl -i  http://localhost:8000
HTTP/1.1 400 Bad Request
content-type: text/plain; charset=utf-8
content-length: 26
date: Mon, 04 Mar 2024 12:52:01 GMT

No scheme found in request

@jplatte
Copy link
Member

jplatte commented Mar 5, 2024

I think we've made a point to not handle any conventional, or even standard headers for "original" request data from proxies in ConnectInfo, because it can be spoofed at least in some cases. I think the same reasoning applies here.

@bengsparks
Copy link
Contributor Author

bengsparks commented Mar 13, 2024

@jplatte Could you concretise what this means for the PR? Does this mean X-Forwarded-Proto should be ignored? In this case, the Host extractor would also have to be updated, as it reads from X-Forwarded-Host.

@Wiezzel
Copy link

Wiezzel commented Mar 13, 2024

I could suggest a fallback to HTTP in case the scheme cannot be extract neither from headers, nor from path. This sounds like a reasonable default to me, but it could be optional in case some application needs stricter check.

@jplatte
Copy link
Member

jplatte commented Mar 13, 2024

@jplatte Could you concretise what this means for the PR? Does this mean X-Forwarded-Proto should be ignored? In this case, the Host extractor would also have to be updated, as it reads from X-Forwarded-Host.

Yes, that is what I meant. I didn't know we even had a Host extractor 😅
I'll try to find some time to figure out the background on how that extractor was added, and also to find those discussions I remember about ConnectInfo.

@bengsparks
Copy link
Contributor Author

@jplatte I will put this PR on pause then until the matter is resolved; Having a Host extractor was half the reason I considered writing this PR to go along with it. There is also the question of what to do if the scheme cannot be found in the URI, despite being provided, which is the case in @Wiezzel's example.

@sclu1034
Copy link

sclu1034 commented Mar 15, 2024

In my tests, the Request never contained a scheme value. Regardless of whether I used the Request or OriginalUri extractors.

I think we've made a point to not handle any conventional, or even standard headers for "original" request data from proxies in ConnectInfo, because it can be spoofed at least in some cases. I think the same reasoning applies here.

If you wanted to walk that path to the end, you'd have to remove the Host extractor entirely, and prevent stuff like req.uri().host(), as that's from the Host header. The protocol would at least be possible, as the server should know whether it currently speaks with or without TLS.

But as a user, the extractors came across to me as an implementation of common practices. Which is why I would expect them to check those extra headers. Because if they didn't, they'd boil down to a one-liner like req.uri().host(), and I don't need an extractor for that.

I would not expect them to be subject to security-based mitigations.
Instead, the way I'd expect a security mitigation like this would be either some kind of middleware that I can deliberately add to have "spoofable" headers be stripped, or the reverse, where they are stripped/ignored by default unless some kind of .reverse_proxy(true) is set on a Router or for axum as a whole.

@bengsparks
Copy link
Contributor Author

bengsparks commented Sep 9, 2024

To reiterate:
The scheme is resolved through the following, in order:

  • Forwarded header
  • X-Forwarded-Proto header
  • Request URI

The final bullet point requires that axum is served with the http2 feature flag.
I've tested this manually with cURL and xh, but the TestClient that is used in the automated tests doesn't have the API to force HTTP/2 to be used.
How can / should such a test be included as part of this PR?

Should the naming of Scheme reflect this? Perhaps a rename into H2Scheme?

@sclu1034
Copy link

requires that axum is served with the http2 feature flag

Why is that? Regardless of the HTTP version, something in the stack has to initiate the TLS connection, and therefore knows what scheme is in use.
If it's a limitation in libraries not exposing this, it might be worthwhile to create an upstream feature request for that.

@bengsparks
Copy link
Contributor Author

bengsparks commented Sep 10, 2024

Ah, I think I've mixed something up.
I've managed to send requests through to an axum instance served with TLS (using axum_server), without enabling any feature flags.
I'm still open to testing this properly :)

@bengsparks
Copy link
Contributor Author

I accidentally closed this Pull Request when updating my fork of axum.
However, I'd like further instruction / advice on what should be done to make this mergeable or if this is even wanted.
If not, I'll simply reclose the PR, no hard feelings :).

@mladedav
Copy link
Collaborator

It seems you've accidentally pushed the already merged branch removing the allows instead of the scheme extractor one.

We've moved the Host extractor to axum-extra and the last consensus was to put this there too. I think (but can't really check) that your changes were already adding it there so I don't know if there's anything blocking it.

Sorry for the delays.

@bengsparks
Copy link
Contributor Author

@mladedav thanks for the quick reply. I've restored the state of the branch

@yanns
Copy link
Collaborator

yanns commented Oct 19, 2024

I think we've made a point to not handle any conventional, or even standard headers for "original" request data from proxies in ConnectInfo, because it can be spoofed at least in some cases. I think the same reasoning applies here.

I also share this concern.
One possible way to deal with that would be to expose the information as a method with a naming to indicate that this information can be "dangerous" to use.
Maybe something like scheme.spoofable_data().

@bengsparks
Copy link
Contributor Author

bengsparks commented Oct 19, 2024

The Host extractor, since opening of this PR, has been moved into axum-extra, and to my understanding, it was precisely because of this issue.
I have also since adjusted this PR to match this, and added an identical doc comment to refer to this.

Additionally, adding a separate method to indicate "danger" goes against the spirit of extractors, whose intended usage is documented as "reading via destructuring" i.e. Path(user_id): Path<u32>, Json(payload): Json<Value>, Extension(state): Extension<State>, etc.

@yanns
Copy link
Collaborator

yanns commented Oct 19, 2024

That's a good point. I'd like that we make users aware of the risks of using those extractors somehow, but I don't have better idea for now. Having them in axum-extra does not feel good enough. It's still very easy to import them and use them without being conscious of the possible security implications.

I'll wait for the point of view of @jplatte here.

@jplatte
Copy link
Member

jplatte commented Oct 19, 2024

@yanns yeah I agree, it would be nice to do more than just exclude these from the main crate. But also I think it wouldn't be great to hold up this PR again because of that.

Let's merge it but make an issue to revisit both Host and Scheme before v0.8.0?

@yanns
Copy link
Collaborator

yanns commented Oct 19, 2024

@yanns yeah I agree, it would be nice to do more than just exclude these from the main crate. But also I think it wouldn't be great to hold up this PR again because of that.

Let's merge it but make an issue to revisit both Host and Scheme before v0.8.0?

Agree with this plan!

@jplatte
Copy link
Member

jplatte commented Oct 19, 2024

Oh, CI failing is my fault.. I'll look into it 🫠

@yanns
Copy link
Collaborator

yanns commented Oct 19, 2024

I've added an issue about spoofable extractors: #2998

@jplatte jplatte merged commit ffeb4f9 into tokio-rs:main Oct 19, 2024
18 checks passed
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 Scheme extractor
6 participants