Make handling of proxies more consistent

* Always use `*_proxy` environment variables.
* Make a `ChefConfig::Config.proxy_uri` method that gets used by
  `Chef::Provider::RemoteFile::FTP` and `Chef::HTTP::BasicClient`.
* Remove `env` method from `Chef::HTTP::BasicClient` (using
  `stub_const("ENV", ...)` in specs instead.)
* Remove `http_proxy_user` and `http_proxy_pass` methods from
  `Chef::HTTP::BasicClient` (replaced by functionality in `ChefConfig`.)

7 files changed, 209 insertions, 205 deletions

diff --git a/chef-config/lib/chef-config/config.rb b/chef-config/lib/chef-config/config.rb
index ec41ccdf8c..c3d330ef77 100644
--- a/chef-config/lib/chef-config/config.rb
+++ b/chef-config/lib/chef-config/config.rb
@@ -846,6 +846,28 @@ module ChefConfig
       ENV["NO_PROXY"] = value unless ENV["NO_PROXY"]
     end
 
+    # Given a scheme, host, and port, return the correct proxy URI based on the
+    # set environment variables, unless exluded by no_proxy, in which case nil
+    # is returned
+    def self.proxy_uri(scheme, host, port)
+      proxy_env_var = ENV["#{scheme}_proxy"].to_s.strip
+
+      # Check if the proxy string contains a scheme. If not, add the url's scheme to the
+      # proxy before parsing. The regex /^.*:\/\// matches, for example, http://. Reusing proxy
+      # here since we are really just trying to get the string built correctly.
+      proxy = if !proxy_env_var.empty?
+        if proxy_env_var.match(/^.*:\/\//)
+          URI.parse(proxy_env_var)
+        else
+          URI.parse("#{scheme}://#{proxy_env_var}")
+        end
+      end
+
+      excludes = ENV['no_proxy'].to_s.split(/\s*,\s*/).compact
+      excludes = excludes.map { |exclude| exclude =~ /:\d+$/ ? exclude : "#{exclude}:*" }
+      return proxy unless excludes.any? { |exclude| File.fnmatch(exclude, "#{host}:#{port}") }
+    end
+
     # Chef requires an English-language UTF-8 locale to function properly.  We attempt
     # to use the 'locale -a' command and search through a list of preferences until we
     # find one that we can use.  On Ubuntu systems we should find 'C.UTF-8' and be
diff --git a/chef-config/spec/unit/config_spec.rb b/chef-config/spec/unit/config_spec.rb
index 797fe3a233..1e28f5b3e4 100644
--- a/chef-config/spec/unit/config_spec.rb
+++ b/chef-config/spec/unit/config_spec.rb
@@ -802,6 +802,116 @@ RSpec.describe ChefConfig::Config do
     end
   end
 
+  describe "proxy_uri" do
+    subject(:proxy_uri) { described_class.proxy_uri(scheme, host, port) }
+    let(:env) { {} }
+    let(:scheme) { "http" }
+    let(:host) { "test.example.com" }
+    let(:port) { 8080 }
+    let(:proxy) { "#{proxy_prefix}#{proxy_host}:#{proxy_port}" }
+    let(:proxy_prefix) { "http://" }
+    let(:proxy_host) { "proxy.mycorp.com" }
+    let(:proxy_port) { 8080 }
+
+    before do
+      stub_const("ENV", env)
+    end
+
+    shared_examples_for "a proxy uri" do
+      it "contains the host" do
+        expect(proxy_uri.host).to eq(proxy_host)
+      end
+
+      it "contains the port" do
+        expect(proxy_uri.port).to eq(proxy_port)
+      end
+    end
+
+    context "when the config setting is normalized (does not contain the scheme)" do
+      include_examples "a proxy uri" do
+
+        let(:proxy_prefix) { "" }
+
+        let(:env) do
+          {
+            "#{scheme}_proxy" => proxy,
+            "no_proxy" => nil,
+          }
+        end
+      end
+    end
+
+    context "when the proxy is set by the environment" do
+      include_examples "a proxy uri" do
+        let(:scheme) { "https" }
+        let(:env) do
+          {
+            "https_proxy" => "https://jane_username:opensesame@proxy.mycorp.com:8080",
+          }
+        end
+      end
+    end
+
+    context "when an empty proxy is set by the environment" do
+      let(:env) do
+        {
+          "https_proxy" => ""
+        }
+      end
+
+      it "does not fail with URI parse exception" do
+        expect { proxy_uri }.to_not raise_error
+      end
+    end
+
+    context "when no_proxy is set" do
+      context "when no_proxy is the exact host" do
+        let(:env) do
+          {
+            "http_proxy" => proxy,
+            "no_proxy" => host,
+          }
+        end
+
+        it { is_expected.to eq nil }
+      end
+
+      context "when no_proxy includes the same domain with a wildcard" do
+        let(:env) do
+          {
+            "http_proxy" => proxy,
+            "no_proxy" => "*.example.com",
+          }
+        end
+
+        it { is_expected.to eq nil }
+      end
+
+
+      context "when no_proxy is included on a list" do
+        let(:env) do
+          {
+            "http_proxy" => proxy,
+            "no_proxy" => "chef.io,getchef.com,opscode.com,test.example.com",
+          }
+        end
+
+        it { is_expected.to eq nil }
+      end
+
+      context "when no_proxy is included on a list with wildcards" do
+        let(:env) do
+          {
+            "http_proxy" => proxy,
+            "no_proxy" => "10.*,*.example.com",
+          }
+        end
+
+        it { is_expected.to eq nil }
+      end
+    end
+  end
+
   describe "allowing chefdk configuration outside of chefdk" do
 
     it "allows arbitrary settings in the chefdk config context" do
diff --git a/lib/chef/http/basic_client.rb b/lib/chef/http/basic_client.rb
index 58ae496418..b6ae5d9a6f 100644
--- a/lib/chef/http/basic_client.rb
+++ b/lib/chef/http/basic_client.rb
@@ -95,26 +95,8 @@ class Chef
         raise
       end
 
-      #adapted from buildr/lib/buildr/core/transports.rb
       def proxy_uri
-        proxy = Chef::Config["#{url.scheme}_proxy"] ||
-          env["#{url.scheme.upcase}_PROXY"] || env["#{url.scheme}_proxy"]
-
-        # Check if the proxy string contains a scheme. If not, add the url's scheme to the
-        # proxy before parsing. The regex /^.*:\/\// matches, for example, http://. Reusing proxy
-        # here since we are really just trying to get the string built correctly.
-        if String === proxy && !proxy.strip.empty?
-          if proxy =~ /^.*:\/\//
-            proxy = URI.parse(proxy.strip)
-          else
-            proxy = URI.parse("#{url.scheme}://#{proxy.strip}")
-          end
-        end
-
-        no_proxy = Chef::Config[:no_proxy] || env["NO_PROXY"] || env["no_proxy"]
-        excludes = no_proxy.to_s.split(/\s*,\s*/).compact
-        excludes = excludes.map { |exclude| exclude =~ /:\d+$/ ? exclude : "#{exclude}:*" }
-        return proxy unless excludes.any? { |exclude| File.fnmatch(exclude, "#{host}:#{port}") }
+        @proxy_uri ||= ChefConfig::Config.proxy_uri(url.scheme, host, port)
       end
 
       def build_http_client
@@ -133,32 +115,16 @@ class Chef
         Chef::Config
       end
 
-      def env
-        ENV
-      end
-
       def http_client_builder
-        http_proxy = proxy_uri
-        if http_proxy.nil?
+        if proxy_uri.nil?
           Net::HTTP
         else
-          Chef::Log.debug("Using #{http_proxy.host}:#{http_proxy.port} for proxy")
-          user = http_proxy_user(http_proxy)
-          pass = http_proxy_pass(http_proxy)
-          Net::HTTP.Proxy(http_proxy.host, http_proxy.port, user, pass)
+          Chef::Log.debug("Using #{proxy_uri.host}:#{proxy_uri.port} for proxy")
+          Net::HTTP.Proxy(proxy_uri.host, proxy_uri.port, proxy_uri.user,
+                          proxy_uri.password)
         end
       end
 
-      def http_proxy_user(http_proxy)
-        http_proxy.user || Chef::Config["#{url.scheme}_proxy_user"] ||
-          env["#{url.scheme.upcase}_PROXY_USER"] || env["#{url.scheme}_proxy_user"]
-      end
-
-      def http_proxy_pass(http_proxy)
-        http_proxy.password || Chef::Config["#{url.scheme}_proxy_pass"] ||
-          env["#{url.scheme.upcase}_PROXY_PASS"] || env["#{url.scheme}_proxy_pass"]
-      end
-
       def configure_ssl(http_client)
         http_client.use_ssl = true
         ssl_policy.apply_to(http_client)
diff --git a/lib/chef/provider/remote_file/ftp.rb b/lib/chef/provider/remote_file/ftp.rb
index b49e84fa69..f63acdc06c 100644
--- a/lib/chef/provider/remote_file/ftp.rb
+++ b/lib/chef/provider/remote_file/ftp.rb
@@ -146,19 +146,8 @@ class Chef
           tempfile
         end
 
-        #adapted from buildr/lib/buildr/core/transports.rb via chef/rest/rest_client.rb
         def proxy_uri(uri)
-          proxy = Chef::Config["ftp_proxy"]
-          proxy = URI.parse(proxy) if String === proxy
-          if Chef::Config["ftp_proxy_user"]
-            proxy.user = Chef::Config["ftp_proxy_user"]
-          end
-          if Chef::Config["ftp_proxy_pass"]
-            proxy.password = Chef::Config["ftp_proxy_pass"]
-          end
-          excludes = Chef::Config[:no_proxy].to_s.split(/\s*,\s*/).compact
-          excludes = excludes.map { |exclude| exclude =~ /:\d+$/ ? exclude : "#{exclude}:*" }
-          return proxy unless excludes.any? { |exclude| File.fnmatch(exclude, "#{host}:#{port}") }
+          ChefConfig::Config.proxy_uri("ftp", hostname, port)
         end
 
         def parse_path
diff --git a/spec/unit/http/basic_client_spec.rb b/spec/unit/http/basic_client_spec.rb
index 97356b4887..8cf63d4441 100644
--- a/spec/unit/http/basic_client_spec.rb
+++ b/spec/unit/http/basic_client_spec.rb
@@ -40,90 +40,13 @@ describe "HTTP Connection" do
   end
 
   describe "#proxy_uri" do
-    shared_examples_for "a proxy uri" do
-      let(:proxy_host) { "proxy.mycorp.com" }
-      let(:proxy_port) { 8080 }
-      let(:proxy) { "#{proxy_prefix}#{proxy_host}:#{proxy_port}" }
+    subject(:proxy_uri) { basic_client.proxy_uri }
 
-      it "should contain the host" do
-        proxy_uri = subject.proxy_uri
-        expect(proxy_uri.host).to eq(proxy_host)
-      end
-
-      it "should contain the port" do
-        proxy_uri = subject.proxy_uri
-        expect(proxy_uri.port).to eq(proxy_port)
-      end
-    end
-
-    context "when the config setting is normalized (does not contain the scheme)" do
-      include_examples "a proxy uri" do
-
-        let(:proxy_prefix) { "" }
-
-        before do
-          Chef::Config["#{uri.scheme}_proxy"] = proxy
-          Chef::Config[:no_proxy] = nil
-        end
-
-      end
-    end
-
-    context "when the config setting is not normalized (contains the scheme)" do
-      include_examples "a proxy uri" do
-        let(:proxy_prefix) { "#{uri.scheme}://" }
-
-        before do
-          Chef::Config["#{uri.scheme}_proxy"] = proxy
-          Chef::Config[:no_proxy] = nil
-        end
-
-      end
-    end
-
-    context "when the proxy is set by the environment" do
-
-      include_examples "a proxy uri" do
-
-        let(:env) do
-          {
-            "https_proxy" => "https://proxy.mycorp.com:8080",
-            "https_proxy_user" => "jane_username",
-            "https_proxy_pass" => "opensesame",
-          }
-        end
-
-        let(:proxy_uri) { URI.parse(env["https_proxy"]) }
-
-        before do
-          allow(basic_client).to receive(:env).and_return(env)
-        end
-
-        it "sets the proxy user" do
-          expect(basic_client.http_proxy_user(proxy_uri)).to eq("jane_username")
-        end
-
-        it "sets the proxy pass" do
-          expect(basic_client.http_proxy_pass(proxy_uri)).to eq("opensesame")
-        end
-      end
-
-    end
-
-    context "when an empty proxy is set by the environment" do
-      let(:env) do
-        {
-          "https_proxy" => "",
-        }
-      end
-
-      before do
-        allow(subject).to receive(:env).and_return(env)
-      end
-
-      it "to not fail with URI parse exception" do
-        expect { subject.proxy_uri }.to_not raise_error
-      end
+    it "uses ChefConfig's proxy_uri method" do
+      expect(ChefConfig::Config).to receive(:proxy_uri).at_least(:once).with(
+        uri.scheme, uri.host, uri.port
+      )
+      proxy_uri
     end
   end
 end
diff --git a/spec/unit/provider/remote_file/ftp_spec.rb b/spec/unit/provider/remote_file/ftp_spec.rb
index c044621ed4..9963c401d2 100644
--- a/spec/unit/provider/remote_file/ftp_spec.rb
+++ b/spec/unit/provider/remote_file/ftp_spec.rb
@@ -200,9 +200,7 @@ describe Chef::Provider::RemoteFile::FTP do
 
     context "and proxying is enabled" do
       before do
-        Chef::Config[:ftp_proxy] = "socks5://socks.example.com:5000"
-        Chef::Config[:ftp_proxy_user] = "bill"
-        Chef::Config[:ftp_proxy_pass] = "ted"
+        stub_const("ENV", "ftp_proxy" => "socks5://bill:ted@socks.example.com:5000")
       end
 
       it "fetches the file via the proxy" do
diff --git a/spec/unit/rest/auth_credentials_spec.rb b/spec/unit/rest/auth_credentials_spec.rb
index 164c0eb235..af6fc01aeb 100644
--- a/spec/unit/rest/auth_credentials_spec.rb
+++ b/spec/unit/rest/auth_credentials_spec.rb
@@ -94,24 +94,25 @@ describe Chef::REST::AuthCredentials do
 end
 
 describe Chef::REST::RESTRequest do
-  def new_request(method = nil)
+  let(:url) { URI.parse("http://chef.example.com:4000/?q=chef_is_awesome") }
+
+  def new_request(method=nil)
     method ||= :POST
-    Chef::REST::RESTRequest.new(method, @url, @req_body, @headers)
+    Chef::REST::RESTRequest.new(method, url, @req_body, @headers)
   end
 
   before do
-    @auth_credentials = Chef::REST::AuthCredentials.new("client-name", CHEF_SPEC_DATA + "/ssl/private_key.pem")
-    @url = URI.parse("http://chef.example.com:4000/?q=chef_is_awesome")
+    @auth_credentials = Chef::REST::AuthCredentials.new("client-name", CHEF_SPEC_DATA + '/ssl/private_key.pem')
     @req_body = '{"json_data":"as_a_string"}'
     @headers = { "Content-type" => "application/json",
                  "Accept" => "application/json",
                  "Accept-Encoding" => Chef::REST::RESTRequest::ENCODING_GZIP_DEFLATE,
                  "Host" => "chef.example.com:4000" }
-    @request = Chef::REST::RESTRequest.new(:POST, @url, @req_body, @headers)
+    @request = Chef::REST::RESTRequest.new(:POST, url, @req_body, @headers)
   end
 
   it "stores the url it was created with" do
-    expect(@request.url).to eq(@url)
+    expect(@request.url).to eq(url)
   end
 
   it "stores the HTTP method" do
@@ -123,6 +124,10 @@ describe Chef::REST::RESTRequest do
   end
 
   describe "configuring the HTTP request" do
+    let(:url) do
+      URI.parse("http://homie:theclown@chef.example.com:4000/?q=chef_is_awesome")
+    end
+
     it "configures GET requests" do
       @req_body = nil
       rest_req = new_request(:GET)
@@ -152,7 +157,6 @@ describe Chef::REST::RESTRequest do
     end
 
     it "configures HTTP basic auth" do
-      @url = URI.parse("http://homie:theclown@chef.example.com:4000/?q=chef_is_awesome")
       rest_req = new_request(:GET)
       expect(rest_req.http_request.to_hash["authorization"]).to eq(["Basic aG9taWU6dGhlY2xvd24="])
     end
@@ -172,23 +176,14 @@ describe Chef::REST::RESTRequest do
 
     describe "for proxy" do
       before do
-        Chef::Config[:http_proxy]  = "http://proxy.example.com:3128"
-        Chef::Config[:https_proxy] = "http://sproxy.example.com:3129"
-        Chef::Config[:http_proxy_user] = nil
-        Chef::Config[:http_proxy_pass] = nil
-        Chef::Config[:https_proxy_user] = nil
-        Chef::Config[:https_proxy_pass] = nil
-        Chef::Config[:no_proxy] = nil
-      end
-
-      after do
-        Chef::Config[:http_proxy]  = nil
-        Chef::Config[:https_proxy] = nil
-        Chef::Config[:http_proxy_user] = nil
-        Chef::Config[:http_proxy_pass] = nil
-        Chef::Config[:https_proxy_user] = nil
-        Chef::Config[:https_proxy_pass] = nil
-        Chef::Config[:no_proxy] = nil
+        stub_const("ENV", "http_proxy" => "http://proxy.example.com:3128",
+                          "https_proxy" => "http://sproxy.example.com:3129",
+                          "http_proxy_user" => nil,
+                          "http_proxy_pass" => nil,
+                          "https_proxy_user" => nil,
+                          "https_proxy_pass" => nil,
+                          "no_proxy" => nil,
+                  )
       end
 
       describe "with :no_proxy nil" do
@@ -201,20 +196,23 @@ describe Chef::REST::RESTRequest do
           expect(http_client.proxy_pass).to be_nil
         end
 
-        it "configures the proxy address and port when using https scheme" do
-          @url.scheme = "https"
-          http_client = new_request.http_client
-          expect(http_client.proxy?).to eq(true)
-          expect(http_client.proxy_address).to eq("sproxy.example.com")
-          expect(http_client.proxy_port).to eq(3129)
-          expect(http_client.proxy_user).to be_nil
-          expect(http_client.proxy_pass).to be_nil
+        context "when the url has an https scheme" do
+          let(:url) { URI.parse("https://chef.example.com:4000/?q=chef_is_awesome") }
+
+          it "configures the proxy address and port when using https scheme" do
+            http_client = new_request.http_client
+            expect(http_client.proxy?).to eq(true)
+            expect(http_client.proxy_address).to eq("sproxy.example.com")
+            expect(http_client.proxy_port).to eq(3129)
+            expect(http_client.proxy_user).to be_nil
+            expect(http_client.proxy_pass).to be_nil
+          end
         end
       end
 
       describe "with :no_proxy set" do
         before do
-          Chef::Config[:no_proxy] = "10.*,*.example.com"
+          stub_const("ENV", "no_proxy" => "10.*,*.example.com")
         end
 
         it "does not configure the proxy address and port when using http scheme" do
@@ -226,26 +224,23 @@ describe Chef::REST::RESTRequest do
           expect(http_client.proxy_pass).to be_nil
         end
 
-        it "does not configure the proxy address and port when using https scheme" do
-          @url.scheme = "https"
-          http_client = new_request.http_client
-          expect(http_client.proxy?).to eq(false)
-          expect(http_client.proxy_address).to be_nil
-          expect(http_client.proxy_port).to be_nil
-          expect(http_client.proxy_user).to be_nil
-          expect(http_client.proxy_pass).to be_nil
+        context "when the url has an https scheme" do
+          let(:url) { URI.parse("https://chef.example.com:4000/?q=chef_is_awesome") }
+
+          it "does not configure the proxy address and port when using https scheme" do
+            http_client = new_request.http_client
+            expect(http_client.proxy?).to eq(false)
+            expect(http_client.proxy_address).to be_nil
+            expect(http_client.proxy_port).to be_nil
+            expect(http_client.proxy_user).to be_nil
+            expect(http_client.proxy_pass).to be_nil
+          end
         end
       end
 
       describe "with :http_proxy_user and :http_proxy_pass set" do
         before do
-          Chef::Config[:http_proxy_user] = "homie"
-          Chef::Config[:http_proxy_pass] = "theclown"
-        end
-
-        after do
-          Chef::Config[:http_proxy_user] = nil
-          Chef::Config[:http_proxy_pass] = nil
+          stub_const("ENV", "http_proxy" => "http://homie:theclown@proxy.example.com:3128")
         end
 
         it "configures the proxy user and pass when using http scheme" do
@@ -255,24 +250,23 @@ describe Chef::REST::RESTRequest do
           expect(http_client.proxy_pass).to eq("theclown")
         end
 
-        it "does not configure the proxy user and pass when using https scheme" do
-          @url.scheme = "https"
-          http_client = new_request.http_client
-          expect(http_client.proxy?).to eq(true)
-          expect(http_client.proxy_user).to be_nil
-          expect(http_client.proxy_pass).to be_nil
+        context "when the url has an https scheme" do
+          let(:url) { URI.parse("https://chef.example.com:4000/?q=chef_is_awesome") }
+
+          it "does not configure the proxy user and pass when using https scheme" do
+            http_client = new_request.http_client
+            expect(http_client.proxy?).to eq(true)
+            expect(http_client.proxy_user).to be_nil
+            expect(http_client.proxy_pass).to be_nil
+          end
         end
       end
 
       describe "with :https_proxy_user and :https_proxy_pass set" do
         before do
-          Chef::Config[:https_proxy_user] = "homie"
-          Chef::Config[:https_proxy_pass] = "theclown"
-        end
-
-        after do
-          Chef::Config[:https_proxy_user] = nil
-          Chef::Config[:https_proxy_pass] = nil
+          stub_const("ENV", "http_proxy" => "http://proxy.example.com:3128",
+                            "https_proxy" => "https://homie:theclown@sproxy.example.com:3129",
+                    )
         end
 
         it "does not configure the proxy user and pass when using http scheme" do
@@ -282,15 +276,17 @@ describe Chef::REST::RESTRequest do
           expect(http_client.proxy_pass).to be_nil
         end
 
-        it "configures the proxy user and pass when using https scheme" do
-          @url.scheme = "https"
-          http_client = new_request.http_client
-          expect(http_client.proxy?).to eq(true)
-          expect(http_client.proxy_user).to eq("homie")
-          expect(http_client.proxy_pass).to eq("theclown")
+        context "when the url has an https scheme" do
+          let(:url) { URI.parse("https://chef.example.com:4000/?q=chef_is_awesome") }
+
+          it "configures the proxy user and pass when using https scheme" do
+            http_client = new_request.http_client
+            expect(http_client.proxy?).to eq(true)
+            expect(http_client.proxy_user).to eq("homie")
+            expect(http_client.proxy_pass).to eq("theclown")
+          end
         end
       end
     end
   end
-
 end