Merge pull request #4727 from chef/salam/fix-proxy

Client option no_proxy does not work as advertised

10 files changed, 101 insertions, 11 deletions

diff --git a/chef-config/chef-config.gemspec b/chef-config/chef-config.gemspec
index afbd69f188..307112126e 100644
--- a/chef-config/chef-config.gemspec
+++ b/chef-config/chef-config.gemspec
@@ -17,6 +17,7 @@ Gem::Specification.new do |spec|
 
   spec.add_dependency "mixlib-shellout", "~> 2.0"
   spec.add_dependency "mixlib-config", "~> 2.0"
+  spec.add_dependency "fuzzyurl", "~> 0.8.0"
 
   spec.add_development_dependency "rake", "~> 10.0"
 
diff --git a/chef-config/lib/chef-config/config.rb b/chef-config/lib/chef-config/config.rb
index 9da69a1867..bea357dad6 100644
--- a/chef-config/lib/chef-config/config.rb
+++ b/chef-config/lib/chef-config/config.rb
@@ -25,6 +25,8 @@ require "pathname"
 require "chef-config/logger"
 require "chef-config/windows"
 require "chef-config/path_helper"
+require "chef-config/mixin/fuzzy_hostname_matcher"
+
 require "mixlib/shellout"
 require "uri"
 require "openssl"
@@ -34,6 +36,7 @@ module ChefConfig
   class Config
 
     extend Mixlib::Config
+    extend ChefConfig::Mixin::FuzzyHostnameMatcher
 
     # Evaluates the given string as config.
     #
@@ -863,9 +866,7 @@ module ChefConfig
                 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}") }
+      return proxy unless fuzzy_hostname_match_any?(host, ENV["no_proxy"])
     end
 
     # Chef requires an English-language UTF-8 locale to function properly.  We attempt
diff --git a/chef-config/lib/chef-config/mixin/fuzzy_hostname_matcher.rb b/chef-config/lib/chef-config/mixin/fuzzy_hostname_matcher.rb
new file mode 100644
index 0000000000..c4d9185d81
--- /dev/null
+++ b/chef-config/lib/chef-config/mixin/fuzzy_hostname_matcher.rb
@@ -0,0 +1,39 @@
+#
+# Copyright:: Copyright 2016, Chef Software Inc.
+# License:: Apache License, Version 2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+require "fuzzyurl"
+
+module ChefConfig
+  module Mixin
+    module FuzzyHostnameMatcher
+
+      def fuzzy_hostname_match_any?(hostname, matches)
+        return matches.to_s.split(/\s*,\s*/).compact.any? {
+          |m| fuzzy_hostname_match?(hostname, m)
+        } if (hostname != nil) && (matches != nil)
+
+        false
+      end
+
+      def fuzzy_hostname_match?(hostname, match)
+        # Do greedy matching by adding wildcard if it is not specified
+        match = "*" + match if !match.start_with?("*")
+        Fuzzyurl.matches?(Fuzzyurl.mask(hostname: match), hostname)
+      end
+
+    end
+  end
+end
diff --git a/chef-config/lib/chef-config/package_task.rb b/chef-config/lib/chef-config/package_task.rb
index d6f7e55415..43d01f53ef 100644
--- a/chef-config/lib/chef-config/package_task.rb
+++ b/chef-config/lib/chef-config/package_task.rb
@@ -209,7 +209,7 @@ end
         end
       end
 
-      task :version => 'version:update'
+      task :version => "version:update"
 
       Dir[File.expand_path("*gemspec", root_path)].reverse_each do |gemspec_path|
         gemspec = eval(IO.read(gemspec_path))
diff --git a/chef-config/spec/unit/config_spec.rb b/chef-config/spec/unit/config_spec.rb
index 3bada755de..72c0981eca 100644
--- a/chef-config/spec/unit/config_spec.rb
+++ b/chef-config/spec/unit/config_spec.rb
@@ -684,6 +684,19 @@ RSpec.describe ChefConfig::Config do
   end
 
   describe "export_proxies" do
+    before(:all) do
+      @original_env = ENV.to_hash
+      ENV["http_proxy"] = nil
+      ENV["https_proxy"] = nil
+      ENV["ftp_proxy"] = nil
+      ENV["no_proxy"] = nil
+    end
+
+    after(:all) do
+      ENV.clear
+      ENV.update(@original_env)
+    end
+
     let(:http_proxy) { "http://localhost:7979" }
     let(:https_proxy) { "https://localhost:7979" }
     let(:ftp_proxy) { "ftp://localhost:7979" }
@@ -908,6 +921,28 @@ RSpec.describe ChefConfig::Config do
 
         it { is_expected.to eq nil }
       end
+
+      context "when no_proxy is a domain with a dot prefix" do
+        let(:env) do
+          {
+            "http_proxy" => proxy,
+            "no_proxy" => ".example.com",
+          }
+        end
+
+        it { is_expected.to eq nil }
+      end
+
+      context "when no_proxy is a domain with no wildcard" do
+        let(:env) do
+          {
+            "http_proxy" => proxy,
+            "no_proxy" => "example.com",
+          }
+        end
+
+        it { is_expected.to eq nil }
+      end
     end
   end
 
diff --git a/lib/chef/http/basic_client.rb b/lib/chef/http/basic_client.rb
index 190635f798..3a87fe85e4 100644
--- a/lib/chef/http/basic_client.rb
+++ b/lib/chef/http/basic_client.rb
@@ -100,7 +100,13 @@ class Chef
       end
 
       def build_http_client
-        http_client = http_client_builder.new(host, port)
+        # Note: the last nil in the new below forces Net::HTTP to ignore the
+        # no_proxy environment variable. This is a workaround for limitations
+        # in Net::HTTP use of the no_proxy environment variable. We internally
+        # match no_proxy with a fuzzy matcher, rather than letting Net::HTTP
+        # do it.
+        http_client = http_client_builder.new(host, port, nil)
+        http_client.proxy_port = nil if http_client.proxy_address == nil
 
         if url.scheme == HTTPS
           configure_ssl(http_client)
diff --git a/lib/chef/knife/ssl_check.rb b/lib/chef/knife/ssl_check.rb
index 0995fc8a54..0c672f322e 100644
--- a/lib/chef/knife/ssl_check.rb
+++ b/lib/chef/knife/ssl_check.rb
@@ -245,6 +245,7 @@ ADVICE
 
       def run
         validate_uri
+
         if verify_X509 && verify_cert && verify_cert_host
           ui.msg "Successfully verified certificates from `#{host}'"
         else
diff --git a/lib/chef/mixin/proxified_socket.rb b/lib/chef/mixin/proxified_socket.rb
index c3b0f7688c..5c9bc3c7d0 100644
--- a/lib/chef/mixin/proxified_socket.rb
+++ b/lib/chef/mixin/proxified_socket.rb
@@ -16,18 +16,22 @@
 #
 
 require "proxifier"
+require "chef-config/mixin/fuzzy_hostname_matcher"
 
 class Chef
   module Mixin
     module ProxifiedSocket
 
+      include ChefConfig::Mixin::FuzzyHostnameMatcher
+
       # This looks at the environment variables and leverages Proxifier to
       # make the TCPSocket respect ENV['https_proxy'] or ENV['http_proxy'] if
       # they are present
       def proxified_socket(host, port)
         proxy = ENV["https_proxy"] || ENV["http_proxy"] || false
-        if proxy
-          Proxifier.Proxy(proxy, no_proxy: ENV["no_proxy"]).open(host, port)
+
+        if proxy && !fuzzy_hostname_match_any?(host, ENV["no_proxy"])
+          Proxifier.Proxy(proxy).open(host, port)
         else
           TCPSocket.new(host, port)
         end
diff --git a/spec/unit/mixin/proxified_socket_spec.rb b/spec/unit/mixin/proxified_socket_spec.rb
index 1d752bb600..d3ba54f618 100644
--- a/spec/unit/mixin/proxified_socket_spec.rb
+++ b/spec/unit/mixin/proxified_socket_spec.rb
@@ -26,11 +26,11 @@ end
 
 describe Chef::Mixin::ProxifiedSocket do
 
-  before do
+  before(:all) do
     @original_env = ENV.to_hash
   end
 
-  after do
+  after(:all) do
     ENV.clear
     ENV.update(@original_env)
   end
@@ -46,7 +46,7 @@ describe Chef::Mixin::ProxifiedSocket do
 
   shared_examples "proxified socket" do
     it "wraps the Socket in a Proxifier::Proxy" do
-      expect(Proxifier).to receive(:Proxy).with(proxy_uri, no_proxy: no_proxy_spec).and_return(proxifier_double)
+      expect(Proxifier).to receive(:Proxy).with(proxy_uri).and_return(proxifier_double)
       expect(proxifier_double).to receive(:open).with(host, port).and_return(socket_double)
       expect(test_instance.proxified_socket(host, port)).to eq(socket_double)
     end
@@ -54,6 +54,8 @@ describe Chef::Mixin::ProxifiedSocket do
 
   context "when no proxy is set" do
     it "returns a plain TCPSocket" do
+      ENV["http_proxy"] = nil
+      ENV["https_proxy"] = nil
       expect(TCPSocket).to receive(:new).with(host, port).and_return(socket_double)
       expect(test_instance.proxified_socket(host, port)).to eq(socket_double)
     end
@@ -84,6 +86,7 @@ describe Chef::Mixin::ProxifiedSocket do
 
   context "when http_proxy is set" do
     before do
+      ENV["https_proxy"] = nil
       ENV["http_proxy"] = http_uri
     end
 
diff --git a/spec/unit/rest/auth_credentials_spec.rb b/spec/unit/rest/auth_credentials_spec.rb
index dcc0f923b4..2728463c81 100644
--- a/spec/unit/rest/auth_credentials_spec.rb
+++ b/spec/unit/rest/auth_credentials_spec.rb
@@ -255,7 +255,7 @@ describe Chef::REST::RESTRequest do
 
           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?).to eq(false)
             expect(http_client.proxy_user).to be_nil
             expect(http_client.proxy_pass).to be_nil
           end