Translate ONC ProxySettings <-> Shill ProxyConfig

chromeos/network/onc/onc_utils.cc

Index: chromeos/network/onc/onc_utils.cc
diff --git a/chromeos/network/onc/onc_utils.cc b/chromeos/network/onc/onc_utils.cc
index 2e4d384c4985fdde5ef2b0fb49f789fa373654f8..831c5d1567b238323666bcdb0b705a9ca5d63ef1 100644
--- a/chromeos/network/onc/onc_utils.cc
+++ b/chromeos/network/onc/onc_utils.cc
@@ -17,11 +17,16 @@
 #include "chromeos/network/onc/onc_utils.h"
 #include "chromeos/network/onc/onc_validator.h"
 #include "components/device_event_log/device_event_log.h"
+#include "components/proxy_config/proxy_config_dictionary.h"
 #include "crypto/encryptor.h"
 #include "crypto/hmac.h"
 #include "crypto/symmetric_key.h"
+#include "net/base/host_port_pair.h"
 #include "net/cert/pem_tokenizer.h"
 #include "net/cert/x509_certificate.h"
+#include "net/proxy/proxy_bypass_rules.h"
+#include "net/proxy/proxy_config.h"
+#include "net/proxy/proxy_server.h"
 
 using namespace ::onc;
 
@@ -738,5 +743,216 @@ bool IsRecommendedValue(const base::DictionaryValue* onc,
           recommended_keys->end());
 }
 
+namespace {
+
+const char kSchemeFtp[] = "ftp";
+const char kSchemeHttp[] = "http";
+const char kSchemeHttps[] = "https";
+const char kSchemeSocks[] = "socks";

[jochen (gone - plz use gerrit), 2015/07/08 13:20:15]

please don't redefine but use the proper constants

[stevenjb, 2015/07/08 16:26:14]

Done.

+
+net::ProxyServer ConvertOncProxyLocationToHostPort(
+    net::ProxyServer::Scheme default_proxy_scheme,
+    const base::DictionaryValue& onc_proxy_location) {
+  std::string host;
+  onc_proxy_location.GetStringWithoutPathExpansion(::onc::proxy::kHost, &host);
+  // Parse |host| according to the format [<scheme>"://"]<server>[":"<port>].
+  net::ProxyServer proxy_server =
+      net::ProxyServer::FromURI(host, default_proxy_scheme);
+  int port = 0;
+  onc_proxy_location.GetIntegerWithoutPathExpansion(::onc::proxy::kPort, &port);
+
+  // Replace the port parsed from |host| by the provided |port|.
+  return net::ProxyServer(
+      proxy_server.scheme(),
+      net::HostPortPair(proxy_server.host_port_pair().host(),
+                        static_cast<uint16>(port)));
+}
+
+void AppendProxyServerForScheme(const base::DictionaryValue& onc_manual,
+                                const std::string& onc_scheme,
+                                std::string* spec) {
+  const base::DictionaryValue* onc_proxy_location = nullptr;
+  if (!onc_manual.GetDictionaryWithoutPathExpansion(onc_scheme,
+                                                    &onc_proxy_location)) {
+    return;
+  }
+
+  net::ProxyServer::Scheme default_proxy_scheme = net::ProxyServer::SCHEME_HTTP;
+  std::string url_scheme;
+  if (onc_scheme == ::onc::proxy::kFtp) {
+    url_scheme = kSchemeFtp;
+  } else if (onc_scheme == ::onc::proxy::kHttp) {
+    url_scheme = kSchemeHttp;
+  } else if (onc_scheme == ::onc::proxy::kHttps) {
+    url_scheme = kSchemeHttps;
+  } else if (onc_scheme == ::onc::proxy::kSocks) {
+    default_proxy_scheme = net::ProxyServer::SCHEME_SOCKS4;
+    url_scheme = kSchemeSocks;
+  } else {
+    NOTREACHED();
+  }
+
+  net::ProxyServer proxy_server = ConvertOncProxyLocationToHostPort(
+      default_proxy_scheme, *onc_proxy_location);
+
+  ProxyConfigDictionary::EncodeAndAppendProxyServer(url_scheme, proxy_server,
+                                                    spec);
+}
+
+net::ProxyBypassRules ConvertOncExcludeDomainsToBypassRules(
+    const base::ListValue& onc_exclude_domains) {
+  net::ProxyBypassRules rules;
+  for (base::ListValue::const_iterator it = onc_exclude_domains.begin();
+       it != onc_exclude_domains.end(); ++it) {
+    std::string rule;
+    (*it)->GetAsString(&rule);
+    rules.AddRuleFromString(rule);
+  }
+  return rules;
+}
+
+void SetProxyForScheme(const net::ProxyConfig::ProxyRules& proxy_rules,
+                       const std::string& scheme,
+                       const std::string& onc_scheme,
+                       base::DictionaryValue* dict) {
+  const net::ProxyList* proxy_list = nullptr;
+  if (proxy_rules.type == net::ProxyConfig::ProxyRules::TYPE_SINGLE_PROXY) {
+    proxy_list = &proxy_rules.single_proxies;
+  } else {
+    proxy_list = proxy_rules.MapUrlSchemeToProxyList(scheme);

[pneubeck (no reviews), 2015/07/08 06:52:48]

according to the documentation, this should not be called if type ==
TYPE_NO_RULES .
this is ensured by the caller of this function, so probably drop a comment in
front of this function.

[stevenjb, 2015/07/08 16:26:14]

I added an explicit test here.

+  }
+  if (!proxy_list || proxy_list->IsEmpty())
+    return;
+  const net::ProxyServer& server = proxy_list->Get();
+  scoped_ptr<base::DictionaryValue> url_dict(new base::DictionaryValue);
+  std::string host = server.host_port_pair().host();
+  // Special case: Include the scheme for socks5 only.
+  if (server.scheme() == net::ProxyServer::SCHEME_SOCKS5)

[pneubeck (no reviews), 2015/07/08 06:52:48]

i think you should handle all non-default schemes here. as the default for
non-socks is HTTP, this could look like (analogous to lines 780, 789 and 760):

// For all proxy types except SOCKS, the default scheme of the proxy host is
HTTP.
net::ProxyServer::Scheme default_scheme = net::ProxyServer::SCHEME_HTTP;
if (onc_scheme == ::onc::proxy::kSocks) {
  default_scheme = net::ProxyServer::SCHEME_SOCKS4;
}
// Only prefix the host with a non-default scheme.
if (server.scheme() != default_scheme)
  host = SchemeToString(server.scheme()) + "://" + host;

[stevenjb, 2015/07/08 16:26:14]

So, there is no "SchemeToString" function, and effectively if I wrote one here
it would be "if (scheme == net::ProxyServer::SCHEME_SOCKS5) return "socks5",
unless you are suggesting we handle SCHEME_DIRECT and SCEME_QUIC?

I went ahead and updated the comment.

[pneubeck (no reviews), 2015/07/09 07:16:12]

sadly... the net/proxy code hides this conversion in the .cc .
no, that's not right. you're adding the prefix for every _non_-default scheme.
at least SCHEME_DIRECT should not be necessary, i guess.
The current code will not be able to handle:

http=socks4://example.com;https=...

which must be translated to

Manual: {
  "HTTP": {
     host: "socks4://example.com",
     port: 123
  },
  "HTTPS":
   ....
}

instead i think you're currently dropping the scheme 'socks4'.


e.g. a similar configuration is mentioned here:
https://www.chromium.org/developers/design-documents/network-stack/socks-proxy

[stevenjb, 2015/07/09 18:24:28]

My head is starting to hurt. I am looking at the code in ProxyConfig/ProxyServer
and am now wondering if we should even be using it here. Sigh.

This example seems to contradict your previous suggestion. If we set
default_scheme == SCHEME_SOCKS4 for ::onc::proxy::kSocks, then both socks:// and
socks4:// will skip the scheme since they both translate to SCHEME_SOCKS4 in
ProxyConfig.

I went ahead and added a SchemeToString() function to make this explicit and to
cover QUIC

[pneubeck (no reviews), 2015/07/10 07:20:42]

I'm not sure where you see a contradiction.
As socks:// == socks4://, skipping these two in a host url seems the intended
behavior for socks proxies.

Thinking more about it, the whole skipping the default scheme is only an
optimization and might not be worth it. Neither skipping the default nor always
adding the scheme will make the round trip (ONC -> Shill -> ONC) an identity
operation. There are always cases where we get back a different result than what
was originally set.
e.g. set ONC to
   HTTPProxy: {
     Host: "http://1.2.3.4"
     Port: 1234
   }
 => "http://" will be dropped if we skip the default scheme

 or set ONC to
   HTTPProxy: {
     Host: "1.2.3.4"
     Port: 1234
   }
 =>  Scheme prefix "http://" will be added if we always add the prefix

[eroman, 2015/07/13 20:08:38]

There are a few factors at play here with regards to the SOCKS default
protocol...

(1) In PAC scripts, one indicates use of a SOCKS (v4) proxy by using the the
string format "SOCKS proxy:port" rather than "PROXY proxy:port". When SOCKS v5
was added, browsers required specifying it instead as "SOCKS5 proxy:port" to
disambiguate. So in this domain, "SOCKS" refers to v4.

(2) The serialization format you are using by calling
ProxyRules::ParseFromString() is actually the Windows WinInet serialization
format (with more allowances). I guess you decided to use it because there was
already code for it, but it has its own idiosyncrasies. In this format,
"socks=host:port" indicates the use of a socks v4 proxy, because that is how
Windows apps interpreted it. In fact, strictly speaking there is no support for
SOCKS v5 in this windows format, however Chrome hacks it in by interpret leading
protocol designators, such as "socks=socks5://host:port"

(3) Command line flags. This is where it gets interesting. In Chrome you can
specify proxy servers using
--proxy-server="<proxy-protocol>:://<proxy-host>:<proxy-port>". This 3 tuple
expressing the proxy protocol, host, port is represented by ProxyServer class.
Unlike the other representations this is unambiguous about he role of the proxy.
This representation looks like a URL, but isn't, it is just a convenient
serialization. It allows expressing things like "HTTPS" proxies which were a
Chrome invention (not to be confused with an HTTP proxy for https://* URLs,
which is what you are configuring with https=...)

OK, so in this format, "socks://" actually means "socks5://". We invented this
format, so for consistency we could have made socks:// mean socks4://. And in
fact is originally what we did. But because so many people were filing bugs due
to incorrectly specifying a socks 4 proxy when they mean to specify socks 5, we
switched the default here. Socks v4 is practically never what you want to be
using in Chrome. Socks v5 dates back to the 1996 and is widely supported

So to sum it up, if you want to be using ProxyRules::ParseFromString() using the
Windows semantics, then:

   socks=foobar     --> means socks v4 proxy
   socks=socks://foobar --> means socks v5 proxy
   socks=socks4://foobar --> means socks v4 proxy
   socks=socks5://foobar --> means socks v5 proxy

Where the addition of a scheme in front of the host:port is a Chrome invention
and not something that Windows would actually interpret (If you loaded these in
Internet Explorer would fail).

Hope that helps clarify the confusion.

Unfortunately the "standard" way of expressing proxy settings as a series of
host:port per URL scheme, and then SOCKS, is really quite terrible. SOCKS is not
a URL scheme like the others, but rather a proxy protocol (with implicit version
depending on who sets it). And this socks proxy is generally used as a fallback
for something not matched by the URL specific ones (even though you could
interpret it as a proxy for the transport layer in addition to the other proxy
usages).

Also include plenty of tests on how your serialized settings get interpreted,
since if ProxyRules::ParseFromString() were ever changed to match some Windows
specific behavior (which it occasionally does) you wouldn't want your
serializations to break.

</rant>

+    host = "socks5://" + host;
+  url_dict->SetStringWithoutPathExpansion(::onc::proxy::kHost, host);
+  url_dict->SetIntegerWithoutPathExpansion(::onc::proxy::kPort,
+                                           server.host_port_pair().port());
+  dict->SetWithoutPathExpansion(onc_scheme, url_dict.release());
+}
+
+}  // namespace
+
+scoped_ptr<base::DictionaryValue> ConvertOncProxySettingsToProxyConfig(
+    const base::DictionaryValue& onc_proxy_settings) {
+  std::string type;
+  onc_proxy_settings.GetStringWithoutPathExpansion(::onc::proxy::kType, &type);
+  scoped_ptr<base::DictionaryValue> proxy_dict;
+
+  if (type == ::onc::proxy::kDirect) {
+    proxy_dict.reset(ProxyConfigDictionary::CreateDirect());
+  } else if (type == ::onc::proxy::kWPAD) {
+    proxy_dict.reset(ProxyConfigDictionary::CreateAutoDetect());
+  } else if (type == ::onc::proxy::kPAC) {
+    std::string pac_url;
+    onc_proxy_settings.GetStringWithoutPathExpansion(::onc::proxy::kPAC,
+                                                     &pac_url);
+    GURL url(pac_url);
+    DCHECK(url.is_valid()) << "Invalid URL in ProxySettings.PAC";
+    proxy_dict.reset(ProxyConfigDictionary::CreatePacScript(url.spec(), false));
+  } else if (type == ::onc::proxy::kManual) {
+    const base::DictionaryValue* manual_dict = nullptr;
+    onc_proxy_settings.GetDictionaryWithoutPathExpansion(::onc::proxy::kManual,
+                                                         &manual_dict);
+    std::string manual_spec;
+    AppendProxyServerForScheme(*manual_dict, ::onc::proxy::kFtp, &manual_spec);
+    AppendProxyServerForScheme(*manual_dict, ::onc::proxy::kHttp, &manual_spec);
+    AppendProxyServerForScheme(*manual_dict, ::onc::proxy::kSocks,
+                               &manual_spec);
+    AppendProxyServerForScheme(*manual_dict, ::onc::proxy::kHttps,
+                               &manual_spec);
+
+    const base::ListValue* exclude_domains = nullptr;
+    net::ProxyBypassRules bypass_rules;
+    if (onc_proxy_settings.GetListWithoutPathExpansion(
+            ::onc::proxy::kExcludeDomains, &exclude_domains)) {
+      bypass_rules.AssignFrom(
+          ConvertOncExcludeDomainsToBypassRules(*exclude_domains));
+    }
+    proxy_dict.reset(ProxyConfigDictionary::CreateFixedServers(
+        manual_spec, bypass_rules.ToString()));
+  } else {
+    NOTREACHED();
+  }
+  return proxy_dict.Pass();
+}
+
+scoped_ptr<base::DictionaryValue> ConvertProxyConfigToOncProxySettings(
+    const base::DictionaryValue& proxy_config_value) {
+  // Create a ProxyConfigDictionary from the DictionaryValue.
+  scoped_ptr<ProxyConfigDictionary> proxy_config(
+      new ProxyConfigDictionary(&proxy_config_value));
+
+  // Create the result DictionaryValue and populate it.
+  scoped_ptr<base::DictionaryValue> proxy_settings(new base::DictionaryValue);
+  ProxyPrefs::ProxyMode mode;
+  if (!proxy_config->GetMode(&mode))
+    return nullptr;
+  switch (mode) {
+    case ProxyPrefs::MODE_DIRECT: {
+      proxy_settings->SetStringWithoutPathExpansion(::onc::proxy::kType,
+                                                    ::onc::proxy::kDirect);
+      break;
+    }
+    case ProxyPrefs::MODE_AUTO_DETECT: {
+      proxy_settings->SetStringWithoutPathExpansion(::onc::proxy::kType,
+                                                    ::onc::proxy::kWPAD);
+      break;
+    }
+    case ProxyPrefs::MODE_PAC_SCRIPT: {
+      proxy_settings->SetStringWithoutPathExpansion(::onc::proxy::kType,
+                                                    ::onc::proxy::kPAC);
+      std::string pac_url;
+      proxy_config->GetPacUrl(&pac_url);
+      proxy_settings->SetStringWithoutPathExpansion(::onc::proxy::kPAC,
+                                                    pac_url);
+      break;
+    }
+    case ProxyPrefs::MODE_FIXED_SERVERS: {
+      proxy_settings->SetString(::onc::proxy::kType, ::onc::proxy::kManual);
+      scoped_ptr<base::DictionaryValue> manual(new base::DictionaryValue);
+      std::string proxy_rules_string;
+      if (proxy_config->GetProxyServer(&proxy_rules_string)) {
+        net::ProxyConfig::ProxyRules proxy_rules;
+        proxy_rules.ParseFromString(proxy_rules_string);
+        SetProxyForScheme(proxy_rules, kSchemeFtp, ::onc::proxy::kFtp,
+                          manual.get());
+        SetProxyForScheme(proxy_rules, kSchemeHttp, ::onc::proxy::kHttp,
+                          manual.get());
+        SetProxyForScheme(proxy_rules, kSchemeHttps, ::onc::proxy::kHttps,
+                          manual.get());
+        SetProxyForScheme(proxy_rules, kSchemeSocks, ::onc::proxy::kSocks,
+                          manual.get());
+      }
+      proxy_settings->SetWithoutPathExpansion(::onc::proxy::kManual,
+                                              manual.release());
+
+      // Convert the 'bypass_list' string into dictionary entries.
+      std::string bypass_rules_string;
+      if (proxy_config->GetBypassList(&bypass_rules_string)) {
+        net::ProxyBypassRules bypass_rules;
+        bypass_rules.ParseFromString(bypass_rules_string);
+        scoped_ptr<base::ListValue> exclude_domains(new base::ListValue);
+        for (const net::ProxyBypassRules::Rule* rule : bypass_rules.rules())
+          exclude_domains->AppendString(rule->ToString());
+        if (!exclude_domains->empty()) {
+          proxy_settings->SetWithoutPathExpansion(::onc::proxy::kExcludeDomains,
+                                                  exclude_domains.release());
+        }
+      }
+      break;
+    }
+    default: {
+      LOG(ERROR) << "Unexpected proxy mode in Shill config: " << mode;
+      return nullptr;
+    }
+  }
+  return proxy_settings.Pass();
+}
+
 }  // namespace onc
 }  // namespace chromeos