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

Support for proxy hosts black listing in ProxyOptions #2600

Closed
haithkris opened this issue Aug 23, 2018 · 31 comments
Closed

Support for proxy hosts black listing in ProxyOptions #2600

haithkris opened this issue Aug 23, 2018 · 31 comments
Assignees
Milestone

Comments

@haithkris
Copy link

haithkris commented Aug 23, 2018

Hi, we have this issue when we want to ignore the proxy and when it comes to call certain components of our internal network. Since vertx ignores JVM proxy arguments like http.nonProxyHosts there is no real workaround.

The proxy configuration is set at the WebClient level. ProxyOptions object should have an equivalent to http.nonProxyHosts.

Vertx web version: 3.5.1
Vertx core version: 3.5.1

@vietj
Copy link
Member

vietj commented Aug 23, 2018

is that something you can contribute ?

@haithkris
Copy link
Author

I would if had proper time and proper knowledge on how vertx works in the internal. But I was hoping you guys can pull it through.

@vietj
Copy link
Member

vietj commented Aug 24, 2018

ok, i'm keeping this as feature request

@vietj vietj changed the title Non proxy host is not supported Support for proxy hosts black listing in ProxyOptions Aug 24, 2018
@marcottedan
Copy link

marcottedan commented Aug 24, 2018

Actually I believe it's more "whitelisting" than "blacklisting" since the noProxy feature of the JVM (and docker network has the same feature) is that everything goes to the proxy except the noProxy hosts.

Edit for references:

KowalczykBartek added a commit to KowalczykBartek/vert.x that referenced this issue Sep 4, 2018
@KowalczykBartek
Copy link
Contributor

Hi, I crafted something today,
of course there is a lot of to do in this PR still (only HttpClientImpl covered), but I wanted to ask what do you think about API,
cc @haithkris

new ProxyOptions()
  .setType(ProxyType.HTTP)
  .exludeHost("localhost") //
  .exludeHost("localhost2") //
  .setHost("google.com")
  .setPort(proxy.getPort());

@haithkris
Copy link
Author

haithkris commented Sep 5, 2018

looks good for me! if we can make accept a list of string or something like that it could be better, but just having the behavior is already good! thanks

@vietj
Copy link
Member

vietj commented Sep 5, 2018

the setter should be setExcludedHosts(List<String>) and there should be a method addExcludedHost(String) that adds element to the list.

Look at how other options handle this.

KowalczykBartek added a commit to KowalczykBartek/vert.x that referenced this issue Sep 5, 2018
KowalczykBartek added a commit to KowalczykBartek/vert.x that referenced this issue Sep 5, 2018
KowalczykBartek added a commit to KowalczykBartek/vert.x that referenced this issue Sep 5, 2018
@KowalczykBartek
Copy link
Contributor

What with NetClient ? should ProxyOptions include option to exclude SocketAddress ?

NetClient connect(SocketAddress remoteAddress, Handler<AsyncResult<NetSocket>> connectHandler);

@vietj
Copy link
Member

vietj commented Sep 6, 2018

that's a concern I had indeed, but ProxyOptions don't support @VertxGen so I think the best would be to have the excluded host to begin with / instead and we should parse them accordingly when we build the proxy

@KowalczykBartek
Copy link
Contributor

so I think the best would be to have the excluded host to begin with / instead and we should parse them accordingly when we build the proxy

can you explain it a little bit ?

@vietj
Copy link
Member

vietj commented Sep 8, 2018

well actually I don't think we need to do that because one can still create 2 net clients if he wants to use domain sockets.

so just a plain list of excluded hosts is fine for me, we don't need to address the port.

KowalczykBartek added a commit to KowalczykBartek/vert.x that referenced this issue Sep 8, 2018
Signed-off-by: KowalczykBartek <[email protected]>

Signed-off-by: KowalczykBartek <[email protected]>
@KowalczykBartek
Copy link
Contributor

KowalczykBartek commented Sep 8, 2018

hmm, so, I guess that I can ask you for review,
If someone wants to test it "in action", I played as follow:

CountDownLatch a = new CountDownLatch(1);

Vertx v = Vertx.vertx();

ProxyOptions proxyOptions = new ProxyOptions();
proxyOptions.setHost("targethost");
proxyOptions.setPort(8080);
proxyOptions.addExcludedHost("exludedhost1");
proxyOptions.addExcludedHost("exludedhost2");

HttpClient client = v.createHttpClient(new HttpClientOptions()
        .setProxyOptions(proxyOptions));

client.getNow(8080,"exludedhost1","/", request -> {
    System.out.println(request);
    a.countDown();
});

a.await();

and /etc/hosts

127.0.0.1 targethost
127.0.0.1 exludedhost1
127.0.0.1 exludedhost2

P.S As I played with this proxy feature, I noticed that it doesn't work with HTTPS traffic, ProxyChannelProvider do not understand https, generates HTTP request, then this proxied http response is handled by handler(SslHandler) that expects to have SSL record.
this is request that fails

ProxyOptions proxyOptions = new ProxyOptions();
proxyOptions.setHost("targethost");
proxyOptions.setPort(8080);
proxyOptions.addExcludedHost("exludedhost1");
proxyOptions.addExcludedHost("exludedhost2");

HttpClient client = v.createHttpClient(new HttpClientOptions()
        .setSsl(true)
        .setTrustAll(true)
        .setVerifyHost(false)
        .setProxyOptions(proxyOptions));

did I do something wrong ? maybe I can fix this in this branch as well ?

@vietj
Copy link
Member

vietj commented Sep 9, 2018

@KowalczykBartek it is possible to test it using configuration of the Vert.x AddressResolver, look at HttpTLSTest which uses it

@vietj
Copy link
Member

vietj commented Sep 9, 2018

@KowalczykBartek it's not clear what you mean with https. The HttpClient supports HTTPS proxy using the connect method, and the traffic will be encrypted after the proxy issues an CONNECT request to the server. This is explained here https://vertx.io/docs/vertx-core/java/#_using_a_proxy_for_http_https_connections

@vietj
Copy link
Member

vietj commented Sep 9, 2018

I think what you are missing is the ChannelConnect#doConnect method that here will make the decision to use or not a proxy:

    if (options.getProxyOptions() == null || !ssl && options.getProxyOptions().getType()== ProxyType.HTTP ) {
      channelProvider = ChannelProvider.INSTANCE;
    } else {
      channelProvider = ProxyChannelProvider.INSTANCE;
    }

@vietj
Copy link
Member

vietj commented Sep 9, 2018

the boolean usesProxy is only used for non https proxies that will rewrite the URL for the proxy as a full uri

@KowalczykBartek
Copy link
Contributor

KowalczykBartek commented Sep 9, 2018

ok, so it was my fault, I didn't know about this CONNECT request to create tunnel in case of HTTPS request :/ sorry :)
I need to read about that, and understand those tests, are a little bit tricky

NetSocket clientSocket = result.result();
serverSocket.write("HTTP/1.0 200 Connection established\n\n");
serverSocket.closeHandler(v -> clientSocket.close());
clientSocket.closeHandler(v -> serverSocket.close());
Pump.pump(serverSocket, clientSocket).start();
Pump.pump(clientSocket, serverSocket).start();

@vietj
Copy link
Member

vietj commented Sep 10, 2018

I'm wondering if we really need this feature, i.e if you don't need proxying then just create another HttpClient / NetClient that does not use a proxy and it should work in a simpler manner

@KowalczykBartek
Copy link
Contributor

That is question to @haithkris , i just wanted to make some code :)

@vietj
Copy link
Member

vietj commented Sep 10, 2018

oh right, if you are looking for contributions, we can guide you and assign you tasks

@marcottedan
Copy link

The goal of the feature seems to handle the Vertx Proxy settings like the JVM does: https://docs.oracle.com/javase/8/docs/technotes/guides/net/proxies.html

@vietj
Copy link
Member

vietj commented Sep 10, 2018

@marcottedan the JVM has a global handling (static), in vertx you can create (lightweight) many different clients with different configurations, so it's fine to create one HttpClient with proxy settings and one HttpClient without proxies and let the application chose the right client

@KowalczykBartek
Copy link
Contributor

@vietj do you mean some stuff not present/listed in Issues vert.x tab ?

@vietj
Copy link
Member

vietj commented Sep 14, 2018

@KowalczykBartek I got your mail and will reply to you soon, sorry for the delay

@laurentvaills
Copy link

Hi guys.
Any news on this feature ? It seems unfortunately inactive since Sept 2018.
Cheers.

@vietj
Copy link
Member

vietj commented Jul 17, 2019

we should review it and merge it I think

@laurentvaills
Copy link

Thanks @vietj .

@cescoffier
Copy link
Contributor

@vietj any update on this one?

@vietj
Copy link
Member

vietj commented Apr 14, 2021

it is not yet scheduled to be implemented by the team

@vietj vietj added this to the 4.1.0 milestone Apr 19, 2021
@vietj vietj self-assigned this May 11, 2021
vietj added a commit that referenced this issue May 12, 2021
Clients have been modified to filter proxy options based on a list of hosts support. Host declaration accept wildcard match like JVM nonProxyHosts list.

HTTP requests declares now a ProxyOptions property that will set the proxy options per request and override the client configuration.

fixes #2600
fixes #3795
vietj added a commit that referenced this issue May 12, 2021
Clients have been modified to filter proxy options based on a list of hosts support. Host declaration accept wildcard match like JVM nonProxyHosts list.

HTTP requests declares now a ProxyOptions property that will set the proxy options per request and override the client configuration.

fixes #2600
fixes #3795
@vietj vietj closed this as completed in 1e95256 May 17, 2021
@gurusreekanth
Copy link

Hi Julien,

Is there any plans to port this fix to 3.9.x releases?

Thank you.

@vietj
Copy link
Member

vietj commented Feb 8, 2022 via email

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

No branches or pull requests

7 participants