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

feat: Update reverse proxy example to support both http and https #2696

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

afifurrohman-id
Copy link
Contributor

Motivation

Example for reverse proxy currently only using http, but sometimes we want to proxy https server as well.

Solution

Update existing example and added it.

Comment on lines 59 to 60
//? Remove incorrect header host, hyper will add automatically for you.
req.headers_mut().remove(HOST).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Why the question mark in front here? Personally I don't know if this is the right behavior here, @yanns can you comment on it since you approved (in addition to @afifurrohman-id)?

Even if we keep it I don't think there's any reason to .unwrap() there though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

When proxing a HTTP request, the initial HTTP request contains as HOST the address of the proxy (locahost:4000). If we use that value against another server, that server will refuse the HTTP request as it expects localhost:3000 instead.

Copy link
Member

Choose a reason for hiding this comment

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

So this was previously broken? Also, should the header value be added as some (de-facto) standard proxied-host header?

Copy link
Collaborator

@yanns yanns May 2, 2024

Choose a reason for hiding this comment

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

So this was previously broken?

I guess yes. I have not checked. I've built several proxy and this is one of the usual issues.

Also, should the header value be added as some (de-facto) standard proxied-host header?

If we want to go in the direction of "good behaving" proxy, yes they are several headers that should be set:

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just tried the original example and it works.

The host header would be an issue if the two servers were running externally visible on the same port as virtual servers with nginx or something else in front of them that would forward the requests based on the host header.

axum, hyper, or anything else in the stack does not check the header, because it can't even know what to check it for. The request just arrives either on localhost:4000 or localhost:3000 and then it is processed.

Personally, I think that handling of this and the addition of an external dependency on example.com is a bit sad and it would be better to just add an axum tls server (based on any of the other examples) and call that instead.

But even with example.com, the header change is not really needed as curl -v https://example.com -H 'host: localhost:3000' returns a 404 -- it's not the same page you get if you let curl use the correct host (and it might be returned by a reverse proxy and not the actual server), but it is a response to an https request so it does show that this works.

Generally, I think the examples should be as simple as possible to showcase the thing they mean to (here that's relaying a request, potentially with a change to the protocol), but details such as extension headers can and should be omitted for brevity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why the question mark in front here?

I just for clarify why we remove HOST header, as @yanns say.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we want to go in the direction of "good behaving" proxy, yes they are several headers that should be set:

I also think like so, we need add it as well, like some other reverse proxy, it will also make more great example.

Copy link
Contributor Author

@afifurrohman-id afifurrohman-id May 2, 2024

Choose a reason for hiding this comment

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

Personally, I think that handling of this and the addition of an external dependency on example.com is a bit sad and it would be better to just add an axum tls server (based on any of the other examples) and call that instead.

Yes, first because it think to simple as possible use an example.com can add more precisely how we can interact with external service, but will add some external network request as well.

However, using other examples like tls-rustls add another complexity because will required to handle self-signed certificate

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.

4 participants