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

HTTP Proxy TODOs #7287

Merged
merged 2 commits into from Aug 9, 2023
Merged

HTTP Proxy TODOs #7287

merged 2 commits into from Aug 9, 2023

Conversation

jbescos
Copy link
Member

@jbescos jbescos commented Aug 1, 2023

#7143

  • Support it in HTTP 2 (Tomas already did it, but I added tests)
  • Support Proxy HTTP authentication
  • Proxy keep alive headers
  • Some proxies respond with HTTP 1.0 protocol and Http1StatusParser will fail
  • HTTP CONNECT should specify the host:port of the remote (not the proxy)
    In my opinion, it makes no sense because the header HOST is mandatory and it contains the remote host:port, but you can try it by yourself configuring a proxy in your browser. For example when you visit youtube you will see the next:
CONNECT www.youtube.com:443 HTTP/1.1
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:108.0) Gecko/20100101 Firefox/108.0
Proxy-Connection: keep-alive
Connection: keep-alive
Host: www.youtube.com:443

Some proxies could not work properly if the CONNECT is not set correctly.

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Aug 1, 2023
@jbescos jbescos marked this pull request as draft August 1, 2023 13:24
@jbescos jbescos mentioned this pull request Aug 1, 2023
5 tasks
@jbescos jbescos force-pushed the issue7143_todos branch 2 times, most recently from 2218c03 to 2749bc2 Compare August 1, 2023 14:01
@jbescos jbescos changed the title HTTP 1 parser and CONNECT host:port fix HTTP Proxy TODOs Aug 1, 2023
@jbescos jbescos force-pushed the issue7143_todos branch 4 times, most recently from e6bce35 to 1a54714 Compare August 2, 2023 09:34
@jbescos
Copy link
Member Author

jbescos commented Aug 2, 2023

I am sniffing how does it work the authentication using Spotify as a client, and it is unsecure.

The CONNECT is always HTTP so it is possible to see exactly what is sent in any of the network nodes. The authentication is sent as basic authentication in this way:

CONNECT ap-gew1.spotify.com:4070 HTTP/1.1
Host: ap-gew1.spotify.com:4070
Proxy-Authorization: Basic amJlc2Nvczp0ZXN0

Anybody sniffing the network will see that user:password is jbescos:test

@jbescos jbescos force-pushed the issue7143_todos branch 3 times, most recently from ef79a4f to 7cbba07 Compare August 2, 2023 12:50
@jbescos jbescos marked this pull request as ready for review August 2, 2023 13:27
@jbescos jbescos force-pushed the issue7143_todos branch 2 times, most recently from 63d093c to 0b972bb Compare August 7, 2023 08:11
@tomas-langer
Copy link
Member

Basic authentication is terrible, but if required, we must support it.

httpProxy.stop();
}

AuthHttpProxyTest(WebServer server) {
Copy link
Member

Choose a reason for hiding this comment

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

You can just inject the clients into constructor parameters:

AuthHttpProxyTest(Http1Client http1Client, Http2Client http2Client)

and they will be initialized to the host and port of the server

Copy link
Member Author

Choose a reason for hiding this comment

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

I am getting:

[ERROR] io.helidon.nima.tests.integration.webclient.AuthHttpProxyTest.testUserPasswordNotCorrect1  Time elapsed: 0 s  <<< ERROR!
org.junit.jupiter.api.extension.ParameterResolutionException: No ParameterResolver registered for parameter [io.helidon.nima.http2.webclient.Http2Client arg1] in constructor [io.helidon.nima.tests.integration.webclient.AuthHttpProxyTest(io.helidon.nima.webclient.http1.Http1Client,io.helidon.nima.http2.webclient.Http2Client)].
	at org.junit.jupiter.engine.execution.ParameterResolutionUtils.resolveParameter(ParameterResolutionUtils.java:120)
	at org.junit.jupiter.engine.execution.ParameterResolutionUtils.resolveParameters(ParameterResolutionUtils.java:103)
	at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.invoke(InterceptingExecutableInvoker.java:59)
	at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.invokeTestClassConstructor(ClassBasedTestDescriptor.java:363)

Copy link
Member

Choose a reason for hiding this comment

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

please add the http2 testing support as well

Copy link
Member

Choose a reason for hiding this comment

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

        <dependency>
            <groupId>io.helidon.nima.testing.junit5</groupId>
            <artifactId>helidon-nima-testing-junit5-http2</artifactId>
            <scope>test</scope>
        </dependency>

Signed-off-by: Jorge Bescos Gascon <jorge.bescos.gascon@oracle.com>
Signed-off-by: Jorge Bescos Gascon <jorge.bescos.gascon@oracle.com>
@jbescos jbescos merged commit b3b533b into helidon-io:main Aug 9, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants