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

Server request URL with spring-webflux 6.0.5 is in resolved IP6 format #30033

Closed
Tomasz-Marciniak opened this issue Feb 25, 2023 · 24 comments
Closed
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: superseded An issue that has been superseded by another type: regression A bug that is also a regression
Milestone

Comments

@Tomasz-Marciniak
Copy link

Tomasz-Marciniak commented Feb 25, 2023

After upgrade to spring boot 3.0.3, Swagger authorization stopped working on http://localhost:8080.

image

The issue is caused by changed method:

private static URI resolveBaseUrl(HttpServerRequest request) throws URISyntaxException {
		String scheme = getScheme(request);

		InetSocketAddress hostAddress = request.hostAddress();
		...
}

From:

private static URI resolveBaseUrl(HttpServerRequest request) throws URISyntaxException {
		String scheme = getScheme(request);
		String header = request.requestHeaders().get(HttpHeaderNames.HOST);
		...
}

The root cause of issue is that instead of resolving host to http://localhost, it resolves http://[0:0:0:0:0:0:0:1].

The main functionality of swagger works with this IP address, but authorization fails because it redirects to:

image

To fix that issue I can set server.address, but then it breaks accessing swagger by external IP.

I found the issue after checking swagger, but rest of application works fine. Should openapi be fixed or resolveBaseUrl method?

Even enforcing IP4 by -Djava.net.preferIPv4Stack=true does not solve issue.

Downgrading to spring-web 6.0.4 solves the issue. Also using IP instead of DSN does not generate such issues.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Feb 25, 2023
@bclozel
Copy link
Member

bclozel commented Feb 25, 2023

I think this is linked to #28601.

Transferring this issue to Spring Framework.

@bclozel bclozel transferred this issue from spring-projects/spring-boot Feb 25, 2023
@bclozel bclozel added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Feb 25, 2023
@Tomasz-Marciniak
Copy link
Author

@bclozel - thanks for handling this.

@wilkinsona
Copy link
Member

We suspect that spring-projects/spring-boot#34395 is another symptom of the same underlying problem.

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Feb 27, 2023

Indeed, both this issue and spring-projects/spring-boot#34395 are linked to the change in #28601.

I tried the sample from spring-projects/spring-boot#34395, and the case there is an "X-Forwarded-Host" that contains both host and port (e.g. "localhost:3000"). When we parse the "Host" header ourselves, we handle this case. However, when relying on Reactor's request.hostAddress(), the hostString in it has the port and so the java.net.URI constructor fails with such a host. @violetagg, is there another Reactor request method we should use that takes care of this, or should we always parse the hostString we get from requst.hostAddress() just like we parse the "Host" header?

The case reported here is a little different. It looks like request.hostAddress() has a hostString that differs from what's in the "Host" header. I'm not clear on what to do with this one. It seems like the "Host" header has better information in this case, but if we use it first, then we're back to the issue with #28601. @violetagg any insight from your side would be appreciated. Ideally we would use a Reactor API, if available to obtain this information.

@bclozel
Copy link
Member

bclozel commented Feb 28, 2023

See #30047 for a possible fix involving Netty's ˋNetUtil`.

@rstoyanchev
Copy link
Contributor

#30047 looks more of an optimization that avoids an extra URI creation. I'm not sure it solves the issues here, but worth to consider at the same time.

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Feb 28, 2023

As expected, Netty's NetUtil doesn't do anything for a hostString that has a port, so #30047 doesn't help for spring-projects/spring-boot#34395, and I suspect it doesn't help for this issue either, but I have no way to confirm that.

@Intosoft if you can provide an isolated sample to debug or verify changes with, that would be very helpful. Probably no need to see the authorization failures, but just enough to get such a server URL that differs between 6.0.4 and 6.0.5.

@violetagg
Copy link
Member

"X-Forwarded-Host" that contains both host and port (e.g. "localhost:3000")

I think this is not a valid value

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-Host

[Syntax]
X-Forwarded-Host: <host>

[Directives]
<host>
The domain name of the forwarded server.

[Examples]
X-Forwarded-Host: id42.example-cdn.com

@msosa
Copy link

msosa commented Feb 28, 2023

But <host> according to them may optionally include a port

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Host

The Host request header specifies the host and port number of the server to which the request is being sent.

I would think that X-Forwarded-Host would be closely related to just the Host header

@violetagg
Copy link
Member

violetagg commented Feb 28, 2023

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Host

@msosa

Here I think the <host> corresponds to those in X-Forwarded-Host. It does not contain port, which is a separate directive.

[Syntax]
Host: <host>:<port>

[Directives]
<host>
the domain name of the server (for virtual hosting).

<port> Optional
TCP port number on which the server is listening.

[Examples]
Host: developer.mozilla.org

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Feb 28, 2023

Also the port is present in both "X-Forwarded-Port: 3000" and "X-Forwarded-Host: localhost:3000", and it's not clear why does it even needs to be in "X-Forwarded-Host" in the first place, creating ambiguity. Yes, they are the same in this case, but generally when both are present one would have to be preferred over the other. This is why it makes sense that "X-Forwarded-Host" is for the host (only) and "X-Forwarded-Port" is for the port.

@msosa
Copy link

msosa commented Feb 28, 2023

I definitely agree that it is odd to have the port in the X-Forwarded-Host, not sure why my frontend proxy sends the header in that way.

I did think that Host could include port and X-Forwarded-Host is just a forwarded header of that. But if that's not the case I'll try and find another proxy that sends the header correctly, or just stick with framework strategy until it changes similarly

@abelsromero
Copy link
Contributor

We found a related issue when running spring-cloud-gateway with oauth spring-security integration. The redirect_uris sent to the auth server now contain the port (even when using 80) and when running with localhost they are sent as 127.0.0.1. That means the admin needs to change the auth server configuration.
I narrowed it down to the actual commit related to #28601 issue. A custom build of 6.0.5 without that commit works fine.

@violetagg
Copy link
Member

violetagg commented Feb 28, 2023

We found a related issue when running spring-cloud-gateway with oauth spring-security integration. The redirect_uris sent to the auth server now contain the port (even when using 80) and when running with localhost they are sent as 127.0.0.1. That means the admin needs to change the auth server configuration.
I narrowed it down to the actual commit related to #28601 issue. A custom build of 6.0.5 without that commit works fine.

This is related to the first issue isn't it? Not the one with X-Forwarded-For?
Is it easy to reproduce it?

@msosa
Copy link

msosa commented Mar 1, 2023

Here is an example that reproduces the problem for the first issue(the non X-Forwarded-For issue). The Servers dropdown represents the url as

http://[0:0:0:0:0:0:0:1]:8080 - Generated server url

instead of

http://localhost:8080 - Generated server url

@abelsromero
Copy link
Contributor

This is related to the first issue isn't it? Not the one with X-Forwarded-For?

It seems to me the origin is the same. We see the same "localhost -> IP" change and as I said, I narrowed it down to this actual commit a2b7a90 and confirmed via debugging that we are entering into the new if condition.

Is it easy to reproduce it?

Sadly not, it's a complex setup, including an Okta service.

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Mar 1, 2023

Thanks for the sample @msosa. I can't load the index.html page, probably because I'm not running an API server, but the problem is easy to see anyway. The Reactor Netty request.hostAddress() resolves to an IPv6 address while the "Host" header, which we used before has a host name. That's one of the issues.

The other one is the "X-Forwarded-Host" with "host:port", and will be addressed in reactor/reactor-netty#2711.

@abelsromero yes we know it's linked to that change, but it isn't as simple as reverting it, and we need to understand the scenarios in order to fix the issues for each.

@violetagg
Copy link
Member

For the issue with X-Forwarded-Host, are you able to test Reactor Netty 1.0.29-SNAPSHOT/1.1.4-SNAPSHOT
(https://repo.spring.io/snapshot)

@rstoyanchev rstoyanchev added this to the 6.0.6 milestone Mar 1, 2023
@rstoyanchev rstoyanchev added type: regression A bug that is also a regression and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Mar 1, 2023
@rstoyanchev rstoyanchev changed the title spring-web 6.0.5 breaks org.springdoc:springdoc-openapi-starter-webflux-ui:2.0.2 authorization and URLs are in IP6 format Server request URL with spring-webflux 6.0.5 is in resolved IP6 format Mar 1, 2023
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Mar 1, 2023

I've scheduled this for 6.0.6, as we are expecting a change in Reactor Netty to provide a hostName and hostPort, and we are going to build on it.

The X-Forwarded-Host issue should already have a fix in the latest Reactor Netty 1.1.4 snapshot. @msosa, I've checked with your sample but if you can also confirm from your end, that would be great.

@msosa
Copy link

msosa commented Mar 2, 2023

Yes, the enhancement works on my project as well, thank you!

Something to note though, when I set my UI port to 80 and go to http://localhost the OAuth redirect URI is generated with http://localhost:80 instead of http://localhost. Not sure if this is something that needs to be looked at or not, but if you would like a repo for this I can post one.

@violetagg
Copy link
Member

violetagg commented Mar 2, 2023

Yes, the enhancement works on my project as well, thank you!

Something to note though, when I set my UI port to 80 and go to http://localhost the OAuth redirect URI is generated with http://localhost:80 instead of http://localhost. Not sure if this is something that needs to be looked at or not, but if you would like a repo for this I can post one.

This will be addressed with the changes that we are preparing for the other issue.

violetagg added a commit to violetagg/spring-framework that referenced this issue Mar 2, 2023
- Prefer request hostName and hostPort in ReactorServerHttpRequest#resolveBaseUrl
- The request hostName and hostPort are derived from the Host/X-Forwarded-*/Forwarded header associated with this request.
- Do not add the port when it is the default one

Closes spring-projectsgh-30033
@rstoyanchev rstoyanchev added the status: superseded An issue that has been superseded by another label Mar 2, 2023
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Mar 2, 2023

We now have reactor/reactor-netty#2711 and reactor/reactor-netty#2714 in place, and also #30062 to take advantage of the new hostName and hostPort request properties. So in effect this issue is now superceded by #30062.

@rstoyanchev
Copy link
Contributor

There is now a 6.0.6 snapshot available with the changes. If you have the option to test, please give it a try together with Reactor-Bom 2022.0.4 snapshot.

I'll close this as superseded, but please feel free to add more comments.

@rstoyanchev
Copy link
Contributor

Thanks @msosa and @abelsromero for confirming the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: superseded An issue that has been superseded by another type: regression A bug that is also a regression
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants