Bug 31494 – Add unauthenticated proxy support to SocketStreamHandleCFNet

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Alexey Proskuryakov]

Patch forthcoming.

[Alexey Proskuryakov]

Created attachment 43205 [details]
proposed patch

[Darin Adler]

Comment on attachment 43205 [details]
proposed patch

>  void SocketStreamHandle::chooseProxy()
>  {
> -    // FIXME: Retrieve proxy information.
> +#if !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD)
> +    RetainPtr<CFDictionaryRef> proxyDictionary(AdoptCF, CFNetworkCopySystemProxySettings());
> +#else
> +    // We don't need proxy information often, so there is no need to set up a permanent dynamic store session.
> +    RetainPtr<CFDictionaryRef> proxyDictionary(AdoptCF, SCDynamicStoreCopyProxies(0));
> +#endif
> +
> +    // SOCKS or HTTPS (AKA CONNECT) proxies are supported.
> +    // WebSocket protocol relies on handshake being transferred unchanged, so we need a proxy that will not modify headers.
> +    // Since HTTP proxies must add Via headers, they are highly unlikely to work.
> +    // Many CONNECT proxies limit connectivity to port 443, so we prefer SOCKS, if configured.
> +
> +    if (!proxyDictionary) {
> +        m_connectionType = Direct;
> +        return;
> +    }
> +
> +#ifndef BUILDING_ON_TIGER
> +    // CFNetworkCopyProxiesForURL doesn't know about WebSocket schemes, so pretend to use http.
> +    // Always use "https" to get HTTPS proxies in result - we'll try to use those for ws:. even though many are configured to reject connections to ports other than 443.
> +    KURL httpsURL(KURL(), "https://" + m_url.host());
> +    RetainPtr<CFURLRef> httpsURLCF(AdoptCF, httpsURL.createCFURL());
> +
> +    RetainPtr<CFArrayRef> proxyArray(AdoptCF, CFNetworkCopyProxiesForURL(httpsURLCF.get(), proxyDictionary.get()));
> +    CFIndex proxyArrayCount = CFArrayGetCount(proxyArray.get());
> +
> +    // FIXME: Support PAC files (always the preferred entry).
> +
> +    CFDictionaryRef chosenProxy = 0;
> +    for (CFIndex i = 0; i < proxyArrayCount; ++i) {
> +        CFDictionaryRef proxyInfo = static_cast<CFDictionaryRef>(CFArrayGetValueAtIndex(proxyArray.get(), i));
> +        CFTypeRef proxyType = CFDictionaryGetValue(proxyInfo, kCFProxyTypeKey);
> +        if (proxyType && CFGetTypeID(proxyType) == CFStringGetTypeID()) {
> +            if (CFEqual(proxyType, kCFProxyTypeSOCKS)) {
> +                m_connectionType = SOCKSProxy;
> +                chosenProxy = proxyInfo;
> +                break;
> +            }
> +            if (CFEqual(proxyType, kCFProxyTypeHTTPS)) {
> +                m_connectionType = CONNECTProxy;
> +                chosenProxy = proxyInfo;
> +                // Keep looking for proxies, as a SOCKS one is preferable.
> +            }
> +        }
> +    }
> +
> +    if (chosenProxy) {
> +        ASSERT(m_connectionType != Unknown);
> +        ASSERT(m_connectionType != Direct);
> +
> +        CFTypeRef proxyHost = CFDictionaryGetValue(chosenProxy, kCFProxyHostNameKey);
> +        CFTypeRef proxyPort = CFDictionaryGetValue(chosenProxy, kCFProxyPortNumberKey);
> +
> +        if (proxyHost && CFGetTypeID(proxyHost) == CFStringGetTypeID() && proxyPort && CFGetTypeID(proxyPort) == CFNumberGetTypeID()) {
> +            m_proxyHost = static_cast<CFStringRef>(proxyHost);
> +            m_proxyPort = static_cast<CFNumberRef>(proxyPort);
> +            return;
> +        }
> +    }
> +#else // BUILDING_ON_TIGER
> +    // FIXME: check proxy bypass list and ExcludeSimpleHostnames.
> +    // FIXME: Support PAC files.
> +
> +    CFTypeRef socksEnableCF = CFDictionaryGetValue(proxyDictionary.get(), kSCPropNetProxiesSOCKSEnable);
> +    int socksEnable;
> +    if (socksEnableCF && CFGetTypeID(socksEnableCF) == CFNumberGetTypeID() && CFNumberGetValue(static_cast<CFNumberRef>(socksEnableCF), kCFNumberIntType, &socksEnable) && socksEnable) {
> +        CFTypeRef proxyHost = CFDictionaryGetValue(proxyDictionary.get(), kSCPropNetProxiesSOCKSProxy);
> +        CFTypeRef proxyPort = CFDictionaryGetValue(proxyDictionary.get(), kSCPropNetProxiesSOCKSPort);
> +        if (proxyHost && CFGetTypeID(proxyHost) == CFStringGetTypeID() && proxyPort && CFGetTypeID(proxyPort) == CFNumberGetTypeID()) {
> +            m_proxyHost = static_cast<CFStringRef>(proxyHost);
> +            m_proxyPort = static_cast<CFNumberRef>(proxyPort);
> +            m_connectionType = SOCKSProxy;
> +            return;
> +        }
> +    }
> +
> +    CFTypeRef httpsEnableCF = CFDictionaryGetValue(proxyDictionary.get(), kSCPropNetProxiesHTTPSEnable);
> +    int httpsEnable;
> +    if (httpsEnableCF && CFGetTypeID(httpsEnableCF) == CFNumberGetTypeID() && CFNumberGetValue(static_cast<CFNumberRef>(httpsEnableCF), kCFNumberIntType, &httpsEnable) && httpsEnable) {
> +        CFTypeRef proxyHost = CFDictionaryGetValue(proxyDictionary.get(), kSCPropNetProxiesHTTPSProxy);
> +        CFTypeRef proxyPort = CFDictionaryGetValue(proxyDictionary.get(), kSCPropNetProxiesHTTPSPort);
> +
> +        if (proxyHost && CFGetTypeID(proxyHost) == CFStringGetTypeID() && proxyPort && CFGetTypeID(proxyPort) == CFNumberGetTypeID()) {
> +            m_proxyHost = static_cast<CFStringRef>(proxyHost);
> +            m_proxyPort = static_cast<CFNumberRef>(proxyPort);
> +            m_connectionType = CONNECTProxy;
> +            return;
> +        }
> +    }
> +#endif
> +
>      m_connectionType = Direct;
>  }

So much of this is #ifdef'd that it seems maybe we should have an entirely separate copy of the function for Tiger.

> +    case SOCKSProxy: {
> +        // FIXME: SOCKS5 doesn't do challenge-response, should we try to apply credentials from Keychain right away?
> +        // But SOCKS5 credentials don't work at the time of this writing anyway, see <rdar://6776698>.
> +        const void* proxyKeys[] = { kCFStreamPropertySOCKSProxyHost, kCFStreamPropertySOCKSProxyPort };
> +        const void* proxyValues[] = { m_proxyHost.get(), m_proxyPort.get() };
> +        RetainPtr<CFDictionaryRef> connectDictionary(AdoptCF, CFDictionaryCreate(0, proxyKeys, proxyValues, sizeof(proxyKeys) / sizeof(*proxyKeys), &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));
> +        CFReadStreamSetProperty(m_readStream.get(), kCFStreamPropertySOCKSProxy, connectDictionary.get());
> +        break;
> +        }
> +    case CONNECTProxy: {
> +        const void* proxyKeys[] = { kCFStreamPropertyCONNECTProxyHost, kCFStreamPropertyCONNECTProxyPort };
> +        const void* proxyValues[] = { m_proxyHost.get(), m_proxyPort.get() };
> +        RetainPtr<CFDictionaryRef> connectDictionary(AdoptCF, CFDictionaryCreate(0, proxyKeys, proxyValues, sizeof(proxyKeys) / sizeof(*proxyKeys), &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));
> +        CFReadStreamSetProperty(m_readStream.get(), kCFStreamPropertyCONNECTProxy, connectDictionary.get());
> +        break;
> +        }
> +    }

I think the braces are not right here. Either we need to indent the code another level, or move the ending brace one level to the left.

I think this shows why some were pushing to indent cases one level inside a switch statement. I think I was one of those people. Too bad we didn't discuss this aspect of it. We should probably briefly discuss on webkit-dev, although I am sick of coding style discussions!

r=me

[Alexey Proskuryakov]

> I think this shows why some were pushing to indent cases one level inside a
> switch statement.

That's certainly why I prefer that style!

With the current guidelines, I think that what I have here is better than having two braces right underneath each other.

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/changeset/51040>.

> So much of this is #ifdef'd that it seems maybe we should have an entirely
> separate copy of the function for Tiger.

Sorry, missed this comment at first. I did consider this when making the patch, and have somewhat mixed feelings about it. I'll keep your comment in mind when working with this code next time (soon) to add PAC file support etc.