Repository: relayrides/pushy
Pull request #691 (2019-04-11 11:05:22)

Implement factory methods to return HttpProxyHandlerFactory instances…
… or not based on system default proxies

The code has been structured to allow doing the same for SOCKS proxies in the same way, hence the new package-protected helper class.

This recreates https://github.com/relayrides/pushy/pull/690 after I force-pushed the fixes.

This has now been tested successfully from OSGi container to APNs. I was held up resubmitting this due to having to debug unrelated SSL issues.

I will be available today to answer any questions and then will be unavailable until after Easter.

arunan-interlink: I have pushed a further commit with fixups from the review, will repush with it fixed up into the previous if you are happy with them (and want me to).
arunan-interlink: @jchambers I have rebased this against master and merged in the fixups I made from the previous discussion - this was everything that you requested and that I had no response to. It leaves the open questions though.
arunan-interlink: Rebase again to lastest master.

I would be happy to write the SOCKS equivalent, although as mentioned it would be written blind and untested. It being trivial, it would hopefully work.
jchambers: And, of course, thank you very much for addressing all of the feedback to date!
arunan-interlink: I don't see an option to reply to the comment on SOCKS4 vs SOCKS5 proxies https://github.com/relayrides/pushy/pull/691#discussion_r380799545 . I don't pretend to know anything about SOCKS proxies, but from https://docs.oracle.com/javase/8/docs/technotes/guides/net/proxies.html and https://docs.oracle.com/javase/8/docs/api/java/net/Proxy.Type.html#SOCKS it looks like both versions are handled by the same proxy type in Java.
jchambers: > it looks like both versions are handled by the same proxy type in Java.

I'm also pretty new to the whole SOCKS thing, but I'm also coming to the same conclusion. I can think of three ways forward:

1. We don't worry about SOCKS proxies for now and consider this an enhancement to the HTTP proxy handler factory. We can expand later if we want to, but consider it a non-goal for the time being.
2. We add a nearly-identical `fromSystemProxies` method to both the SOCKS4 and SOCKS5 handler factories and leave it to callers to decide which version they want. This risks getting a a SOCKS5 proxy configuration for a SOCKS4 handler, which isn't great (though my understanding is that going in the other direction is basically fine).
3. We add `fromSystemProxies` to the SOCKS4 handler factory only because SOCKS5 is (I'm pretty sure!) backward-compatible with SOCKS4, so it should always be safe to use a SOCKS4 client regardless of what the server is doing.

I'm really suspicious that I'm missing something obvious/important about SOCKS4/5 interoperability/negotiation/compatibility or something, though. It _seems_ like more people should have this problem, but I can't find anybody talking about it in any meaningful way yet. I'm going to continue to dig and read some RFCs and source code and see what I can figure out.

In the meantime, what's your preference? I'm thinking the safest course of action is probably the first option, but am certainly open to other arguments!
jchambers: …also, I'm surprised that anybody still uses SOCKS4 since SOCKS5 became A Thing™ in… 1996? Hard to get a clear census, but it seems like it's still a going concern.
jchambers: Well, I've since learned that SOCKS4-only proxies are certainly still A Thing. I've also learned that figuring out [SOCKS5 authentication from system properties](https://stackoverflow.com/questions/1540424/java-proxy-client-class-that-supports-authentication) is possible, but non-trivial.

I poked a bit at making a generic `SocksProxyHandler` for Netty that first tries SOCKS5, then falls back to SOCKS4 if that doesn't work out, but it looks like that will also be a significant effort.

I'm still trying to figure out how (if?) the JDK itself handles the SOCKS4/SOCKS5 situation internally.
arunan-interlink: If I was doing this for something internal where I could control/update the clients, I think I would write a best guess for now and debug it if problems arose later. Given Pushy is used by a number of downstream users (congratulation :) ) I would be more hesitant to add anything.

On the other hand, this only uses the system configuration as set in deployment; if the config is wrong, that seems to be more a deployment issue (fundamentally due to Java's conflating the two versions admittedly). that would justify going with b).

a) is definitely safest. Nobody has yet asked for this and can submit a PR if they need it. Might be worth writing up your findings and linking them from the SOCKS proxy classes so this question can pick up where you have left off.
jchambers: Sounds like we're on the same page. Let's keep this HTTP-only for now. My only remaining technical concerns are:

1. Let's inline everything in `HttpProxyHandlerFactory` rather than having a separate `ProxyLocator` for now.
2. Do we need to do anything about finding a username/password for the HTTP proxy from system settings? I'm perfectly happy to accept "no" as an answer, but it seems like it might be a good thing to do in the interest of completeness.

In any case, I'm pretty much ready to merge this even as-is, though do think there's a little room to improve before we ship if you still have the patience.

Thanks again for your patience and for joining me on this voyage of proxy discovery!
arunan-interlink: 1. Makes sense. I don't have a test system anymore, but it's just moving a method so should be fine. Would you like me to force push an edited commit?
2. Given that the authenticator for the proxy gets sets system-wide, I think Pushy should not be trying to do anything with it. Libraries affecting global state will end up causing someone somewhere trouble one day.
jchambers: > Would you like me to force push an edited commit?

If you'd be so kind, that'd be great!

> Given that the authenticator for the proxy gets sets system-wide, I think Pushy should not be trying to do anything with it.

I'm not quite sure I'm following; could you please elaborate or rephrase? My understanding—which could be wrong!—is that the `ProxySelector` is drawing proxy information from a "pool" of system properties, and users might specify a username/password in the same place. Why would it be problematic to draw credentials from that source, but not the address of the proxy itself?
arunan-interlink: Getting the credentials from the system properties is fine, but my understanding is that to use them, a call to Authenticator.setDefault() is used, which would be a globally visible change, which could also overwrite any other set Authenticator. I have not done this, so could be wrong. https://docs.oracle.com/javase/8/docs/api/java/net/Authenticator.html#setDefault-java.net.Authenticator-
jchambers: > my understanding is that to use them, a call to Authenticator.setDefault() is used

Ah—I see! Yeah, I read that, too. I THINK that's what you need to do if you want the JDK-level stuff (e.g. `URLConnection`) to use those credentials. My understanding is that the Netty proxy handler doesn't use `Authenticator` at all, and we'd just need to get the properties and pass them in to the constructor. Let me read the source a bit to confirm that, though.
jchambers: > Let me read the source a bit to confirm that, though.

Yes, I'm confident that's the case. We definitely don't need to mess with `Authenticator`; dealing with credentials in this case is just a matter of reading the properties and passing them to the constructor.
arunan-interlink: Ah sorry, my head was out of the game :) I no longer have a test system to verify any new code on unfortunately though.
jchambers: > I no longer have a test system to verify any new code on unfortunately though.

No problem! Let's get this merged as-is. I'll do the follow-up stuff to handle the credentials.

One small complication: it looks like this isn't up-to-date with `master`. Would you mind giving this one final rebase, and then I'll get it merged?

Cheers!
jchambers: @arunan-interlink I am The Worst. You rebased this, I didn't see it, and then I merged some stuff because I thought I was still waiting for you to rebase. I'm sincerely sorry, but could you please give this one more rebase? And could you post a comment when you do so I don't inadvertently make a mess again?

Thank you, and sorry for the trouble!
arunan-interlink: Apologies for not making a comment before. Rebased and repushed.

Patch:
diff --git a/pushy/src/main/java/com/eatthepath/pushy/apns/proxy/HttpProxyHandlerFactory.java b/pushy/src/main/java/com/eatthepath/pushy/apns/proxy/HttpProxyHandlerFactory.java
index fb68dbd2..1ae69e29 100644
--- a/pushy/src/main/java/com/eatthepath/pushy/apns/proxy/HttpProxyHandlerFactory.java
+++ b/pushy/src/main/java/com/eatthepath/pushy/apns/proxy/HttpProxyHandlerFactory.java
@@ -22,7 +22,16 @@
 
 package com.eatthepath.pushy.apns.proxy;
 
+import java.net.InetSocketAddress;
+import java.net.Proxy;
+import java.net.ProxySelector;
 import java.net.SocketAddress;
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.util.List;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import io.netty.handler.proxy.HttpProxyHandler;
 import io.netty.handler.proxy.ProxyHandler;
@@ -36,11 +45,59 @@ import io.netty.handler.proxy.ProxyHandler;
  */
 public class HttpProxyHandlerFactory implements ProxyHandlerFactory {
 
+    private static final Logger log = LoggerFactory.getLogger(HttpProxyHandlerFactory.class);
+
     private final SocketAddress proxyAddress;
 
     private final String username;
     private final String password;
 
+    /**
+     * Checks the system default proxies to determine if an {@link HttpProxyHandlerFactory}
+     * is required for the given host or not, returning an instance or null for each case.
+     *
+     * @param apnsHost the APNs host to check e.g. "api.push.apple.com".
+     * @return an {@link HttpProxyHandlerFactory} if one is required, or {@code null} if not.
+     * @throws URISyntaxException if <code>apnsHost</code> is malformed in some way.
+     *
+     * @see <a href="https://docs.oracle.com/javase/7/docs/api/java/net/doc-files/net-properties.html#Proxies">The Java documentation on proxies.</a>
+     * @since 0.14
+     */
+    public static HttpProxyHandlerFactory fromSystemProxies(final String apnsHost)
+            throws URISyntaxException {
+
+        final URI hostUri = new URI("https", apnsHost, null, null);
+        final SocketAddress proxyAddress = getProxyAddressForUri(hostUri, Proxy.Type.HTTP);
+
+        return (proxyAddress != null) ? new HttpProxyHandlerFactory(proxyAddress) : null;
+    }
+
+    /**
+     * Returns the {@link SocketAddress} for any system-wide {@link Proxy} of a given {@link Proxy.Type} for a given {@link URI}.
+     *
+     * @param uri the URI to find any system proxy for.
+     * @param proxyType the types of proxy to find for the URI.
+     * @return the {@link SocketAddress} of any <code>Proxy</code> found, or <code>null</code> if there were none.
+     * @since 0.14
+     */
+    private static SocketAddress getProxyAddressForUri(final URI uri, final Proxy.Type proxyType) {
+        final ProxySelector defaultProxySelector = ProxySelector.getDefault();
+        final List<Proxy> proxiesForUri = defaultProxySelector.select(uri);
+        log.debug("Proxies for URI \"{}\" were {}", uri, proxiesForUri);
+
+        for (java.net.Proxy proxy : proxiesForUri) {
+            if (proxy.type() == proxyType) {
+                InetSocketAddress proxyAddress = (InetSocketAddress) proxy.address();
+                if (proxyAddress.isUnresolved()) {
+                    proxyAddress = new InetSocketAddress(proxyAddress.getHostString(), proxyAddress.getPort());
+                }
+                return proxyAddress;
+            }
+        }
+
+        return null;
+    }
+
     /**
      * Creates a new proxy handler factory that will create HTTP proxy handlers that use the proxy at the given
      * address and that will not perform authentication.
@@ -89,4 +146,16 @@ public class HttpProxyHandlerFactory implements ProxyHandlerFactory {
 
         return handler;
     }
+
+    /* (non-Javadoc)
+     * @see java.lang.Object#toString()
+     */
+    @Override
+    public String toString() {
+        return "HttpProxyHandlerFactory {" +
+                "proxyAddress=" + proxyAddress +
+                ", username=" + username +
+                ", password=*****"
+                + "}";
+    }
 }


Test patch:
diff --git a/pushy/src/test/java/com/eatthepath/pushy/apns/proxy/HttpProxyHandlerFactoryTest.java b/pushy/src/test/java/com/eatthepath/pushy/apns/proxy/HttpProxyHandlerFactoryTest.java
new file mode 100644
index 00000000..94ee8a3d
--- /dev/null
+++ b/pushy/src/test/java/com/eatthepath/pushy/apns/proxy/HttpProxyHandlerFactoryTest.java
@@ -0,0 +1,139 @@
+package com.eatthepath.pushy.apns.proxy;
+
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.net.Proxy;
+import java.net.ProxySelector;
+import java.net.SocketAddress;
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.util.Arrays;
+import java.util.List;
+
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import com.eatthepath.pushy.apns.ApnsClientBuilder;
+
+
+public class HttpProxyHandlerFactoryTest {
+
+    private static class FixedProxiesSelector extends ProxySelector {
+
+        private final List<Proxy> availableProxies;
+
+        public FixedProxiesSelector(final Proxy... availableProxies) {
+            this.availableProxies = Arrays.asList(availableProxies);
+        }
+
+        @Override
+        public List<Proxy> select(URI uri) {
+            return availableProxies;
+        }
+
+        @Override
+        public void connectFailed(URI uri, SocketAddress sa, IOException ioe) {
+            // Not needed for this test.
+        }
+    }
+
+    private static class SingleHostHttpProxySelector extends ProxySelector {
+
+        private final String proxiedHost;
+
+        public SingleHostHttpProxySelector(final String proxiedHost) {
+            this.proxiedHost = proxiedHost;
+        }
+
+        @Override
+        public List<Proxy> select(URI uri) {
+            Proxy proxy;
+
+            if (uri.getHost().equals(proxiedHost)) {
+                proxy = DUMMY_HTTP_PROXY;
+            } else {
+                proxy = Proxy.NO_PROXY;
+            }
+
+            return Arrays.asList(proxy);
+        }
+
+        @Override
+        public void connectFailed(URI uri, SocketAddress sa, IOException ioe) {
+            // Not needed for this test.
+        }
+    }
+
+    private static final Proxy DUMMY_HTTP_PROXY = new Proxy(Proxy.Type.HTTP, new InetSocketAddress("httpproxy", 123));
+    private static final Proxy DUMMY_SOCKS_PROXY = new Proxy(Proxy.Type.SOCKS, new InetSocketAddress("socksproxy", 456));
+
+    private static ProxySelector defaultProxySelector;
+
+    @BeforeClass
+    public static void setUpClass() {
+        defaultProxySelector = ProxySelector.getDefault();
+    }
+
+    @AfterClass
+    public static void tearDownClass() {
+        ProxySelector.setDefault(defaultProxySelector);
+    }
+
+    @Test
+    public void testNoProxy() throws URISyntaxException {
+
+        ProxySelector.setDefault(new FixedProxiesSelector(Proxy.NO_PROXY));
+
+        assertNull(HttpProxyHandlerFactory.fromSystemProxies(ApnsClientBuilder.PRODUCTION_APNS_HOST));
+        assertNull(HttpProxyHandlerFactory.fromSystemProxies(ApnsClientBuilder.DEVELOPMENT_APNS_HOST));
+    }
+
+    @Test
+    public void testHasHttpProxy() throws URISyntaxException {
+
+        ProxySelector.setDefault(new FixedProxiesSelector(DUMMY_HTTP_PROXY));
+
+        assertNotNull(HttpProxyHandlerFactory.fromSystemProxies(ApnsClientBuilder.PRODUCTION_APNS_HOST));
+        assertNotNull(HttpProxyHandlerFactory.fromSystemProxies(ApnsClientBuilder.DEVELOPMENT_APNS_HOST));
+    }
+
+    @Test
+    public void testHasSocksProxy() throws URISyntaxException {
+
+        ProxySelector.setDefault(new FixedProxiesSelector(DUMMY_SOCKS_PROXY));
+
+        assertNull(HttpProxyHandlerFactory.fromSystemProxies(ApnsClientBuilder.PRODUCTION_APNS_HOST));
+        assertNull(HttpProxyHandlerFactory.fromSystemProxies(ApnsClientBuilder.DEVELOPMENT_APNS_HOST));
+    }
+
+    @Test
+    public void testHasSocksAndHttpProxies() throws URISyntaxException {
+
+        ProxySelector.setDefault(new FixedProxiesSelector(DUMMY_HTTP_PROXY, DUMMY_SOCKS_PROXY));
+
+        assertNotNull(HttpProxyHandlerFactory.fromSystemProxies(ApnsClientBuilder.PRODUCTION_APNS_HOST));
+        assertNotNull(HttpProxyHandlerFactory.fromSystemProxies(ApnsClientBuilder.DEVELOPMENT_APNS_HOST));
+    }
+
+    @Test
+    public void testHasHttpProxyForApnsProductionOnly() throws URISyntaxException {
+
+        ProxySelector.setDefault(new SingleHostHttpProxySelector(ApnsClientBuilder.PRODUCTION_APNS_HOST));
+
+        assertNotNull(HttpProxyHandlerFactory.fromSystemProxies(ApnsClientBuilder.PRODUCTION_APNS_HOST));
+        assertNull(HttpProxyHandlerFactory.fromSystemProxies(ApnsClientBuilder.DEVELOPMENT_APNS_HOST));
+    }
+
+    @Test
+    public void testHasHttpProxyForApnsDevelopmentOnly() throws URISyntaxException {
+
+        ProxySelector.setDefault(new SingleHostHttpProxySelector(ApnsClientBuilder.DEVELOPMENT_APNS_HOST));
+
+        assertNull(HttpProxyHandlerFactory.fromSystemProxies(ApnsClientBuilder.PRODUCTION_APNS_HOST));
+        assertNotNull(HttpProxyHandlerFactory.fromSystemProxies(ApnsClientBuilder.DEVELOPMENT_APNS_HOST));
+    }
+}