This error message is grammatically incorrect and unhelpful, so we replace it
with a message that explains more correctly what went wrong and what was
expected. This message happens when making an authenticated connection to a
server where the certificate doesn't match its hostname. This happens in the
REST terminuses, so we wrap their HTTP methods with a helper that will catch
the appropriate SSLError and re-raise it with the better message stating the
hostname used, and the list of hostnames that we were expecting it to be a part
of.
Unfortunately, because the certificate in question isn't available at error
time, we have to use the Net::HTTP#verify_callback to capture it.
Paired-With: Jacob Helwig <ja...@puppetlabs.com>
Reviewed-By: Dominic Maraglia <domi...@puppetlabs.com>
Signed-off-by: Nick Lewis <n...@puppetlabs.com>
---
Local-branch: ticket/2.7.x/7224
...e_when_hostname_not_match_server_certificate.rb | 12 +++++
...ket_3360_allow_duplicate_csr_with_option_set.rb | 4 +-
lib/puppet/indirector/rest.rb | 47 +++++++++++++++++---
spec/unit/indirector/certificate/rest_spec.rb | 1 +
spec/unit/indirector/rest_spec.rb | 47 +++++++++++++++++--
5 files changed, 97 insertions(+), 14 deletions(-)
create mode 100644
acceptance/tests/helpful_error_message_when_hostname_not_match_server_certificate.rb
diff --git
a/acceptance/tests/helpful_error_message_when_hostname_not_match_server_certificate.rb
b/acceptance/tests/helpful_error_message_when_hostname_not_match_server_certificate.rb
new file mode 100644
index 0000000..c3b5b67
--- /dev/null
+++
b/acceptance/tests/helpful_error_message_when_hostname_not_match_server_certificate.rb
@@ -0,0 +1,12 @@
+test_name "generate a helpful error message when hostname doesn't match server
certificate"
+
+step "Clear any existing SSL directories"
+on(hosts, "rm -r #{config['puppetpath']}/ssl")
+
+# Start the master with a certname not matching its hostname
+with_master_running_on(master, "--certname foobar_not_my_hostname
--certdnsnames one_cert:two_cert:red_cert:blue_cert --autosign true") do
+ run_agent_on(agents, "--no-daemonize --verbose --onetime --server
#{master}", :acceptable_exit_codes => (1..255)) do
+ msg = "Server hostname '#{master}' did not match server certificate;
expected one of foobar_not_my_hostname, one_cert, two_cert, red_cert, blue_cert"
+ assert_match(msg, stdout)
+ end
+end
diff --git
a/acceptance/tests/ticket_3360_allow_duplicate_csr_with_option_set.rb
b/acceptance/tests/ticket_3360_allow_duplicate_csr_with_option_set.rb
index 9eaf4c2..a34a3e7 100644
--- a/acceptance/tests/ticket_3360_allow_duplicate_csr_with_option_set.rb
+++ b/acceptance/tests/ticket_3360_allow_duplicate_csr_with_option_set.rb
@@ -2,8 +2,8 @@ test_name "#3360: Allow duplicate CSR when
allow_duplicate_certs is on"
agent_hostnames = agents.map {|a| a.to_s}
-step "Remove existing SSL directory for agents"
-on agents, "rm -r #{config['puppetpath']}/ssl"
+step "Remove existing SSL directory for hosts"
+on hosts, "rm -r #{config['puppetpath']}/ssl"
with_master_running_on master, "--allow_duplicate_certs
--certdnsnames=\"puppet:$(hostname -s):$(hostname -f)\" --verbose --noop" do
step "Generate a certificate request for the agent"
diff --git a/lib/puppet/indirector/rest.rb b/lib/puppet/indirector/rest.rb
index 0d39972..8018fe8 100644
--- a/lib/puppet/indirector/rest.rb
+++ b/lib/puppet/indirector/rest.rb
@@ -71,16 +71,49 @@ class Puppet::Indirector::REST <
Puppet::Indirector::Terminus
Puppet::Network::HttpPool.http_instance(request.server ||
self.class.server, request.port || self.class.port)
end
+ [:get, :post, :head, :delete, :put].each do |method|
+ define_method "http_#{method}" do |request, *args|
+ http_request(method, request, *args)
+ end
+ end
+
+ def http_request(method, request, *args)
+ http_connection = network(request)
+ peer_certs = []
+
+ # We add the callback to collect the certificates for use in constructing
+ # the error message if the verification failed. This is necessary since we
+ # don't have direct access to the cert that we expected the connection to
+ # use otherwise.
+ #
+ http_connection.verify_callback = proc do |preverify_ok, ssl_context|
+ peer_certs <<
Puppet::SSL::Certificate.from_s(ssl_context.current_cert.to_pem)
+ preverify_ok
+ end
+
+ http_connection.send(method, *args)
+ rescue OpenSSL::SSL::SSLError => error
+ if error.message.include? "hostname was not match"
+ raise unless cert = peer_certs.find { |c| c.name !~ /^puppet ca/i }
+
+ valid_certnames = [cert.name, *cert.alternate_names].uniq
+ msg = valid_certnames.length > 1 ? "one of #{valid_certnames.join(',
')}" : valid_certnames.first
+
+ raise Puppet::Error, "Server hostname '#{http_connection.address}' did
not match server certificate; expected #{msg}"
+ else
+ raise
+ end
+ end
+
def find(request)
uri, body = request_to_uri_and_body(request)
uri_with_query_string = "#{uri}?#{body}"
- http_connection = network(request)
# WEBrick in Ruby 1.9.1 only supports up to 1024 character lines in an
HTTP request
# http://redmine.ruby-lang.org/issues/show/3991
response = if "GET #{uri_with_query_string} HTTP/1.1\r\n".length > 1024
- http_connection.post(uri, body, headers)
+ http_post(request, uri, body, headers)
else
- http_connection.get(uri_with_query_string, headers)
+ http_get(request, uri_with_query_string, headers)
end
result = deserialize response
result.name = request.key if result.respond_to?(:name=)
@@ -88,7 +121,7 @@ class Puppet::Indirector::REST < Puppet::Indirector::Terminus
end
def head(request)
- response = network(request).head(indirection2uri(request), headers)
+ response = http_head(request, indirection2uri(request), headers)
case response.code
when "404"
return false
@@ -101,7 +134,7 @@ class Puppet::Indirector::REST <
Puppet::Indirector::Terminus
end
def search(request)
- unless result = deserialize(network(request).get(indirection2uri(request),
headers), true)
+ unless result = deserialize(http_get(request, indirection2uri(request),
headers), true)
return []
end
result
@@ -109,12 +142,12 @@ class Puppet::Indirector::REST <
Puppet::Indirector::Terminus
def destroy(request)
raise ArgumentError, "DELETE does not accept options" unless
request.options.empty?
- deserialize network(request).delete(indirection2uri(request), headers)
+ deserialize http_delete(request, indirection2uri(request), headers)
end
def save(request)
raise ArgumentError, "PUT does not accept options" unless
request.options.empty?
- deserialize network(request).put(indirection2uri(request),
request.instance.render, headers.merge({ "Content-Type" =>
request.instance.mime }))
+ deserialize http_put(request, indirection2uri(request),
request.instance.render, headers.merge({ "Content-Type" =>
request.instance.mime }))
end
private
diff --git a/spec/unit/indirector/certificate/rest_spec.rb
b/spec/unit/indirector/certificate/rest_spec.rb
index 21e10e3..870d9e4 100755
--- a/spec/unit/indirector/certificate/rest_spec.rb
+++ b/spec/unit/indirector/certificate/rest_spec.rb
@@ -47,6 +47,7 @@ rn/G
response = stub 'response', :code => "200", :body => cert_string
response.stubs(:[]).with('content-type').returns "text/plain"
response.stubs(:[]).with('content-encoding')
+ network.stubs(:verify_callback=)
network.expects(:get).returns response
request = Puppet::Indirector::Request.new(:certificate, :find, "foo.com")
diff --git a/spec/unit/indirector/rest_spec.rb
b/spec/unit/indirector/rest_spec.rb
index 5637080..ee0111a 100755
--- a/spec/unit/indirector/rest_spec.rb
+++ b/spec/unit/indirector/rest_spec.rb
@@ -90,6 +90,43 @@ describe Puppet::Indirector::REST do
@rest_class.port.should == 543
end
+ describe "when making http requests" do
+ it "should provide a helpful error message when hostname was
not match with server certificate" do
+ Puppet[:certdnsnames] = 'foo:bar:baz'
+ csr = OpenSSL::X509::Request.new
+ csr.subject = OpenSSL::X509::Name.new([['CN',
'not_my_server']])
+ csr.public_key =
OpenSSL::PKey::RSA.generate(Puppet[:keylength]).public_key
+ cert = Puppet::SSL::CertificateFactory.new('server',
csr, csr, 14).result
+
+ connection = Net::HTTP.new('my_server', 8140)
+ @searcher.stubs(:network).returns(connection)
+ ssl_context = OpenSSL::SSL::SSLContext.new
+ ssl_context.stubs(:current_cert).returns(cert)
+ connection.stubs(:get).with do
+ connection.verify_callback.call(true,
ssl_context)
+ end.raises(OpenSSL::SSL::SSLError.new('hostname was not
match with server certificate'))
+
+ msg = /Server hostname 'my_server' did not match server
certificate; expected one of (.+)/
+ expect { @searcher.http_request(:get, stub('request'))
}.to(
+ raise_error(Puppet::Error, msg) do |error|
+ error.message =~ msg
+ $1.split(', ').should =~ ['foo', 'bar',
'baz', 'not_my_server']
+ end
+ )
+ end
+
+ it "should pass along the error message otherwise" do
+ connection = Net::HTTP.new('my_server', 8140)
+ @searcher.stubs(:network).returns(connection)
+
+
connection.stubs(:get).raises(OpenSSL::SSL::SSLError.new('certificate verify
failed'))
+
+ expect do
+ @searcher.http_request(:get, stub('request'))
+ end.to raise_error(/certificate verify failed/)
+ end
+ end
+
describe "when deserializing responses" do
it "should return nil if the response code is 404" do
response = mock 'response'
@@ -219,7 +256,7 @@ describe Puppet::Indirector::REST do
describe "when doing a find" do
before :each do
- @connection = stub('mock http connection', :get => @response)
+ @connection = stub('mock http connection', :get => @response,
:verify_callback= => nil)
@searcher.stubs(:network).returns(@connection) # neuter the network
connection
# Use a key with spaces, so we can test escaping
@@ -313,7 +350,7 @@ describe Puppet::Indirector::REST do
describe "when doing a head" do
before :each do
- @connection = stub('mock http connection', :head => @response)
+ @connection = stub('mock http connection', :head => @response,
:verify_callback= => nil)
@searcher.stubs(:network).returns(@connection)
# Use a key with spaces, so we can test escaping
@@ -349,7 +386,7 @@ describe Puppet::Indirector::REST do
describe "when doing a search" do
before :each do
- @connection = stub('mock http connection', :get => @response)
+ @connection = stub('mock http connection', :get => @response,
:verify_callback= => nil)
@searcher.stubs(:network).returns(@connection) # neuter the network
connection
@model.stubs(:convert_from_multiple)
@@ -397,7 +434,7 @@ describe Puppet::Indirector::REST do
describe "when doing a destroy" do
before :each do
- @connection = stub('mock http connection', :delete => @response)
+ @connection = stub('mock http connection', :delete => @response,
:verify_callback= => nil)
@searcher.stubs(:network).returns(@connection) # neuter the network
connection
@request = Puppet::Indirector::Request.new(:foo, :destroy, "foo bar")
@@ -453,7 +490,7 @@ describe Puppet::Indirector::REST do
describe "when doing a save" do
before :each do
- @connection = stub('mock http connection', :put => @response)
+ @connection = stub('mock http connection', :put => @response,
:verify_callback= => nil)
@searcher.stubs(:network).returns(@connection) # neuter the network
connection
@instance = stub 'instance', :render => "mydata", :mime => "mime"
--
1.7.5.1
--
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To post to this group, send email to puppet-dev@googlegroups.com.
To unsubscribe from this group, send email to
puppet-dev+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en.
