From f588069b278fa068359b06a1368e95c09d677b9e Mon Sep 17 00:00:00 2001 From: Claire McQuin Date: Thu, 29 May 2014 12:44:29 -0700 Subject: add error handling, set *_proxy instead of *_PROXY --- CHANGELOG.md | 2 +- DOC_CHANGES.md | 4 +- lib/chef/application.rb | 55 +++++++------ lib/chef/exceptions.rb | 2 + spec/functional/application_spec.rb | 4 +- spec/unit/application_spec.rb | 152 +++++++++++++++++++++++++----------- 6 files changed, 143 insertions(+), 76 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a91f9fb2f7..c81d935590 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,7 +44,7 @@ * Add config options for attribute whitelisting in node.save. (CHEF-3811) * Use user's .chef as a fallback cache path if /var/chef is not accessible. (CHEF-5259) * Fixed Ruby 2.0 Windows compatibility issues around ruby-wmi gem by replacing it with wmi-lite gem. -* Set PROXY environment variables if preset in config. (CHEF-4712) +* Set proxy environment variables if preset in config. (CHEF-4712) ## Release: 11.12.4 (04/30/2014) http://www.getchef.com/blog/2014/04/30/release-chef-client-11-12-4-ohai-7-0-4/ diff --git a/DOC_CHANGES.md b/DOC_CHANGES.md index cf3fdc5bd3..901d79d090 100644 --- a/DOC_CHANGES.md +++ b/DOC_CHANGES.md @@ -82,7 +82,7 @@ The default behavior is for the node to save all the attribute data. This can be We recommend only using `automatic_attribute_whitelist` to reduce the size of the system data being stored for nodes, and discourage the use of the other attribute whitelists except by advanced users. -### Set PROXY environment variables if present in your config file. +### Set proxy environment variables if present in your config file. If `:http_proxy`, `:https_proxy`, `:ftp_proxy`, or `:no_proxy` is found in your config file, we will configure your environment variables according to the variable form and configuration info given. If your config file looks like @@ -92,4 +92,4 @@ http_proxy_user "myself" http_proxy_pass "Password1" ```` -then Chef will set `ENV[:HTTP_PROXY] = "http://myself:Password1@proxy.example.org:8080"` +then Chef will set `ENV['http_proxy'] = "http://myself:Password1@proxy.example.org:8080"` diff --git a/lib/chef/application.rb b/lib/chef/application.rb index 24305cfcc4..da06438f8e 100644 --- a/lib/chef/application.rb +++ b/lib/chef/application.rb @@ -251,33 +251,36 @@ class Chef::Application Chef::Application.fatal!("Aborting due to error in '#{config_file_path}'", 2) end - # Set ENV['HTTP_PROXY'] + # Set ENV['http_proxy'] def configure_http_proxy if http_proxy = Chef::Config[:http_proxy] - env['HTTP_PROXY'] = configure_proxy("http", http_proxy, Chef::Config[:http_proxy_user], Chef::Config[:http_proxy_pass]) + env['http_proxy'] = configure_proxy("http", http_proxy, + Chef::Config[:http_proxy_user], Chef::Config[:http_proxy_pass]) end end - # Set ENV['HTTPS_PROXY'] + # Set ENV['https_proxy'] def configure_https_proxy if https_proxy = Chef::Config[:https_proxy] - env['HTTPS_PROXY'] = configure_proxy("https", https_proxy, Chef::Config[:https_proxy_user], Chef::Config[:https_proxy_pass]) + env['https_proxy'] = configure_proxy("https", https_proxy, + Chef::Config[:https_proxy_user], Chef::Config[:https_proxy_pass]) end end - # Set ENV['FTP_PROXY'] + # Set ENV['ftp_proxy'] def configure_ftp_proxy if ftp_proxy = Chef::Config[:ftp_proxy] - env['FTP_PROXY'] = configure_proxy("ftp", ftp_proxy, Chef::Config[:ftp_proxy_user], Chef::Config[:ftp_proxy_pass]) + env['ftp_proxy'] = configure_proxy("ftp", ftp_proxy, + Chef::Config[:ftp_proxy_user], Chef::Config[:ftp_proxy_pass]) end end - # Set ENV['NO_PROXY'] + # Set ENV['no_proxy'] def configure_no_proxy - env['NO_PROXY'] = Chef::Config[:no_proxy] if Chef::Config[:no_proxy] + env['no_proxy'] = Chef::Config[:no_proxy] if Chef::Config[:no_proxy] end - #Builds a proxy uri. Examples: + # Builds a proxy uri. Examples: # http://username:password@hostname:port # https://username@hostname:port # ftp://hostname:port @@ -286,23 +289,27 @@ class Chef::Application # hostport = hostname:port # user = username # pass = password - def configure_proxy(scheme, hostport, user, pass) - # URI.split returns the following parts: - # [scheme, userinfo, host, port, registry, path, opaque, query, fragment] - parts = URI.split(URI.encode(hostport)) - parts[0] = scheme if parts[0].nil? - # URI::Generic.build requires an integer for the port, but URI::split gives - # returns a string for the port. - parts[3] = parts[3].to_i if parts[3] - if user - userinfo = URI.encode(URI.encode(user), '@:') - if pass - userinfo << ":#{URI.encode(URI.encode(pass), '@:')}" + def configure_proxy(scheme, path, user, pass) + begin + path = "#{scheme}://#{path}" unless path.start_with?(scheme) + # URI.split returns the following parts: + # [scheme, userinfo, host, port, registry, path, opaque, query, fragment] + parts = URI.split(URI.encode(path)) + # URI::Generic.build requires an integer for the port, but URI::split gives + # returns a string for the port. + parts[3] = parts[3].to_i if parts[3] + if user + userinfo = URI.encode(URI.encode(user), '@:') + if pass + userinfo << ":#{URI.encode(URI.encode(pass), '@:')}" + end + parts[1] = userinfo end - parts[1] = userinfo - end - URI::Generic.build(parts).to_s + return URI::Generic.build(parts).to_s + rescue URI::Error => e + raise Chef::Exceptions::BadProxyURI, e.message + end end # This is a hook for testing diff --git a/lib/chef/exceptions.rb b/lib/chef/exceptions.rb index 782ecc3fd8..438055acd1 100644 --- a/lib/chef/exceptions.rb +++ b/lib/chef/exceptions.rb @@ -329,5 +329,7 @@ class Chef super "Unable to acquire lock. Waited #{duration} seconds for #{blocking_pid} to release." end end + + class BadProxyURI < RuntimeError; end end end diff --git a/spec/functional/application_spec.rb b/spec/functional/application_spec.rb index bed293a80d..ae7bc5aa29 100644 --- a/spec/functional/application_spec.rb +++ b/spec/functional/application_spec.rb @@ -43,9 +43,9 @@ describe Chef::Application do it "saves built proxy to ENV which shell_out can use" do so = if windows? - shell_out("echo $env:HTTP_PROXY") + shell_out("echo $env:http_proxy") else - shell_out("echo $HTTP_PROXY") + shell_out("echo $http_proxy") end so.stdout.should == "http://proxy.example.org:8080\n" diff --git a/spec/unit/application_spec.rb b/spec/unit/application_spec.rb index 5054a59b68..3b1eae0b55 100644 --- a/spec/unit/application_spec.rb +++ b/spec/unit/application_spec.rb @@ -121,8 +121,8 @@ describe Chef::Application do config_location_pathname Pathname.stub(:new).with(config_location).and_return(config_location_pathname) File.should_receive(:read). - with(config_location). - and_return(config_content) + with(config_location). + and_return(config_content) end it "should configure chef::config from a file" do @@ -248,7 +248,74 @@ describe Chef::Application do @app.stub(:configure_no_proxy).and_return(true) end - describe "when configuring ENV['HTTP_PROXY']" do + shared_examples_for "setting ENV['http_proxy']" do + before do + Chef::Config[:http_proxy] = http_proxy + end + + it "should set ENV['http_proxy']" do + @app.configure_proxy_environment_variables + @env['http_proxy'].should == "http://#{address}:#{port}" + end + + describe "when Chef::Config[:http_proxy_user] is set" do + before do + Chef::Config[:http_proxy_user] = "username" + end + + it "should set ENV['http_proxy'] with the username" do + @app.configure_proxy_environment_variables + @env['http_proxy'].should == "http://username@#{address}:#{port}" + end + + context "when :http_proxy_user contains '@' and/or ':'" do + before do + Chef::Config[:http_proxy_user] = "my:usern@me" + end + + it "should set ENV['http_proxy'] with the escaped username" do + @app.configure_proxy_environment_variables + @env['http_proxy'].should == "http://my%3Ausern%40me@#{address}:#{port}" + end + end + + describe "when Chef::Config[:http_proxy_pass] is set" do + before do + Chef::Config[:http_proxy_pass] = "password" + end + + it "should set ENV['http_proxy'] with the password" do + @app.configure_proxy_environment_variables + @env['http_proxy'].should == "http://username:password@#{address}:#{port}" + end + + context "when :http_proxy_pass contains '@' and/or ':'" do + before do + Chef::Config[:http_proxy_pass] = ":P@ssword101" + end + + it "should set ENV['http_proxy'] with the escaped password" do + @app.configure_proxy_environment_variables + @env['http_proxy'].should == "http://username:%3AP%40ssword101@#{address}:#{port}" + end + end + end + end + + describe "when Chef::Config[:http_proxy_pass] is set (but not Chef::Config[:http_proxy_user])" do + before do + Chef::Config[:http_proxy_user] = nil + Chef::Config[:http_proxy_pass] = "password" + end + + it "should set ENV['http_proxy']" do + @app.configure_proxy_environment_variables + @env['http_proxy'].should == "http://#{address}:#{port}" + end + end + end + + describe "when configuring ENV['http_proxy']" do before do @env = {} @app.stub(:env).and_return(@env) @@ -263,71 +330,62 @@ describe Chef::Application do Chef::Config[:http_proxy] = nil end - it "should not set ENV['HTTP_PROXY']" do + it "should not set ENV['http_proxy']" do @app.configure_proxy_environment_variables @env.should == {} end end describe "when Chef::Config[:http_proxy] is set" do - before do - Chef::Config[:http_proxy] = "http://proxy.example.org:8080" - end - - it "should set ENV['HTTP_PROXY'] to http://proxy.example.org:8080" do - @app.configure_proxy_environment_variables - @env['HTTP_PROXY'].should == "http://proxy.example.org:8080" - end + context "when given an FQDN" do + let(:address) { "proxy.example.org" } + let(:port) { 8080 } + let(:http_proxy) { "http://#{address}:#{port}" } - it "should percent encode the proxy, if necessary" do - Chef::Config[:http_proxy] = "http://needs\\some escaping:1234" - @app.configure_proxy_environment_variables - @env['HTTP_PROXY'].should == "http://needs%5Csome%20escaping:1234" + it_should_behave_like "setting ENV['http_proxy']" end - describe "when Chef::Config[:http_proxy_user] is set" do - before do - Chef::Config[:http_proxy_user] = "username" - end + context "when given an IP" do + let(:address) { "127.0.0.1" } + let(:port) { 22 } + let(:http_proxy) { "http://#{address}:#{port}" } - it "should set ENV['HTTP_PROXY'] to http://username@proxy.example.org:8080" do - @app.configure_proxy_environment_variables - @env['HTTP_PROXY'].should == "http://username@proxy.example.org:8080" - end + it_should_behave_like "setting ENV['http_proxy']" + end - it "should percent encode the username, including @ and : characters" do - Chef::Config[:http_proxy_user] = "K:tty C@t" - @app.configure_proxy_environment_variables - @env['HTTP_PROXY'].should == "http://K%3Atty%20C%40t@proxy.example.org:8080" - end + context "when given an IPv6" do + let(:address) { "[2001:db8::1]" } + let(:port) { 80 } + let(:http_proxy) { "http://#{address}:#{port}" } - describe "when Chef::Config[:http_proxy_pass] is set" do - before do - Chef::Config[:http_proxy_pass] = "password" - end + it_should_behave_like "setting ENV['http_proxy']" + end - it "should set ENV['HTTP_PROXY'] to http://username:password@proxy.example.org:8080" do - @app.configure_proxy_environment_variables - @env['HTTP_PROXY'].should == "http://username:password@proxy.example.org:8080" - end + context "when given without including http://" do + let(:address) { "proxy.example.org" } + let(:port) { 8181 } + let(:http_proxy) { "#{address}:#{port}" } - it "should fully percent escape the password, including @ and : characters" do - Chef::Config[:http_proxy_pass] = ":P@ssword101" - @app.configure_proxy_environment_variables - @env['HTTP_PROXY'].should == "http://username:%3AP%40ssword101@proxy.example.org:8080" - end - end + it_should_behave_like "setting ENV['http_proxy']" end - describe "when Chef::Config[:http_proxy_pass] is set (but not Chef::Config[:http_proxy_user])" do + context "when given the full proxy in :http_proxy only" do before do + Chef::Config[:http_proxy] = "http://username:password@proxy.example.org:2222" Chef::Config[:http_proxy_user] = nil - Chef::Config[:http_proxy_pass] = "password" + Chef::Config[:http_proxy_pass] = nil end - it "should set ENV['HTTP_PROXY'] to http://proxy.example.org:8080" do + it "should set ENV['http_proxy']" do @app.configure_proxy_environment_variables - @env['HTTP_PROXY'].should == "http://proxy.example.org:8080" + @env['http_proxy'].should == Chef::Config[:http_proxy] + end + end + + context "when the config options aren't URI compliant" do + it "raises Chef::Exceptions::BadProxyURI" do + Chef::Config[:http_proxy] = "http://proxy.bad_example.org/:8080" + expect { @app.configure_proxy_environment_variables }.to raise_error(Chef::Exceptions::BadProxyURI) end end end -- cgit v1.2.1