Please review pull request #249: (#7110) Better SSL error message when certificate doesn't match key opened by (lifton)
Description:
Previously, any error with the certificate retrieved from the master
matching the agent's private key would give the same static error
message, which wasn't particularly helpful. This commit differentiates
three different error cases: missing certificate, missing private key,
and certificate doesn't match private key. In the last case, the error
message includes the fingerprint of the certificate in question and
explicit command line instructions on how to fix the problem.
In addition to all tests passing, I tested the error messaging and
included instructions in a virtual machine setup running PE 2.0.0.
I removed a redundant test.
Thanks to Jeff McCune, Jacob Helwig, and Nick Lewis for answering questions and giving suggestions.
- Opened: Wed Dec 07 20:38:26 UTC 2011
- Based on: puppetlabs:master (051cb74fc71fef086cf345fefe1718fbeeb79f55)
- Requested merge: lifton:master (b466c1801844606031580d06ea2a98bf7b8667c6)
Diff follows:
diff --git a/lib/puppet/ssl/host.rb b/lib/puppet/ssl/host.rb
index d70fe32..a4baf5b 100644
--- a/lib/puppet/ssl/host.rb
+++ b/lib/puppet/ssl/host.rb
@@ -199,18 +199,26 @@ def certificate
return nil unless Certificate.indirection.find("ca") unless ca?
return nil unless @certificate = Certificate.indirection.find(name)
- unless certificate_matches_key?
- raise Puppet::Error, "Retrieved certificate does not match private key; please remove certificate from server and regenerate it with the current key"
- end
+ validate_certificate_with_key
end
@certificate
end
- def certificate_matches_key?
- return false unless key
- return false unless certificate
-
- certificate.content.check_private_key(key.content)
+ def validate_certificate_with_key
+ raise Puppet::Error, "No certificate to validate." unless certificate
+ raise Puppet::Error, "No private key with which to validate certificate with fingerprint: #{certificate.fingerprint}" unless key
+ unless certificate.content.check_private_key(key.content)
+ raise Puppet::Error, <<ERROR_STRING
+The certificate retrieved from the master does not match the agent's private key.
+Certificate fingerprint: #{certificate.fingerprint}
+To fix this, remove the certificate from both the master and the agent and then start a puppet run, which will automatically regenerate a certficate.
+On the master:
+ puppet cert clean #{Puppet[:certname]}
+On the agent:
+ rm -f #{Puppet[:hostcert]}
+ puppet agent -t
+ERROR_STRING
+ end
end
# Generate all necessary parts of our ssl host.
diff --git a/spec/unit/ssl/host_spec.rb b/spec/unit/ssl/host_spec.rb
index dd64343..3f94407 100755
--- a/spec/unit/ssl/host_spec.rb
+++ b/spec/unit/ssl/host_spec.rb
@@ -182,57 +182,48 @@
it "should cache the localhost instance" do
host = stub 'host', :certificate => "eh", :key => 'foo'
Puppet::SSL::Host.expects(:new).once.returns host
-
Puppet::SSL::Host.localhost.should == Puppet::SSL::Host.localhost
end
it "should be able to verify its certificate matches its key" do
- Puppet::SSL::Host.new("foo").should respond_to(:certificate_matches_key?)
+ Puppet::SSL::Host.new("foo").should respond_to(:validate_certificate_with_key)
end
it "should consider the certificate invalid if it cannot find a key" do
host = Puppet::SSL::Host.new("foo")
+ certificate = mock('cert', :fingerprint => 'DEADBEEF')
+ host.expects(:certificate).twice.returns certificate
host.expects(:key).returns nil
-
- host.should_not be_certificate_matches_key
+ lambda { host.validate_certificate_with_key }.should raise_error(Puppet::Error, "No private key with which to validate certificate with fingerprint: DEADBEEF")
end
it "should consider the certificate invalid if it cannot find a certificate" do
host = Puppet::SSL::Host.new("foo")
- host.expects(:key).returns mock("key")
+ host.expects(:key).never
host.expects(:certificate).returns nil
-
- host.should_not be_certificate_matches_key
+ lambda { host.validate_certificate_with_key }.should raise_error(Puppet::Error, "No certificate to validate.")
end
it "should consider the certificate invalid if the SSL certificate's key verification fails" do
host = Puppet::SSL::Host.new("foo")
-
key = mock 'key', :content => "private_key"
sslcert = mock 'sslcert'
- certificate = mock 'cert', :content => sslcert
-
+ certificate = mock 'cert', {:content => sslcert, :fingerprint => 'DEADBEEF'}
host.stubs(:key).returns key
host.stubs(:certificate).returns certificate
-
sslcert.expects(:check_private_key).with("private_key").returns false
-
- host.should_not be_certificate_matches_key
+ lambda { host.validate_certificate_with_key }.should raise_error(Puppet::Error, /DEADBEEF/)
end
it "should consider the certificate valid if the SSL certificate's key verification succeeds" do
host = Puppet::SSL::Host.new("foo")
-
key = mock 'key', :content => "private_key"
sslcert = mock 'sslcert'
certificate = mock 'cert', :content => sslcert
-
host.stubs(:key).returns key
host.stubs(:certificate).returns certificate
-
sslcert.expects(:check_private_key).with("private_key").returns true
-
- host.should be_certificate_matches_key
+ lambda{ host.validate_certificate_with_key }.should_not raise_error
end
describe "when specifying the CA location" do
@@ -511,15 +502,13 @@
before do
@realcert = mock 'certificate'
@cert = stub 'cert', :content => @realcert
-
@host.stubs(:key).returns mock("key")
- @host.stubs(:certificate_matches_key?).returns true
+ @host.stubs(:validate_certificate_with_key)
end
it "should find the CA certificate if it does not have a certificate" do
Puppet::SSL::Certificate.indirection.expects(:find).with(Puppet::SSL::CA_NAME).returns mock("cacert")
Puppet::SSL::Certificate.indirection.stubs(:find).with("myname").returns @cert
-
@host.certificate
end
@@ -541,34 +530,22 @@
it "should find the key if it does not have one" do
Puppet::SSL::Certificate.indirection.stubs(:find)
@host.expects(:key).returns mock("key")
-
@host.certificate
end
it "should generate the key if one cannot be found" do
Puppet::SSL::Certificate.indirection.stubs(:find)
-
@host.expects(:key).returns nil
@host.expects(:generate_key)
-
@host.certificate
end
it "should find the certificate in the Certificate class and return the Puppet certificate instance" do
Puppet::SSL::Certificate.indirection.expects(:find).with(Puppet::SSL::CA_NAME).returns mock("cacert")
Puppet::SSL::Certificate.indirection.expects(:find).with("myname").returns @cert
-
@host.certificate.should equal(@cert)
end
- it "should fail if the found certificate does not match the private key" do
- @host.expects(:certificate_matches_key?).returns false
-
- Puppet::SSL::Certificate.indirection.stubs(:find).returns @cert
-
- lambda { @host.certificate }.should raise_error(Puppet::Error)
- end
-
it "should return any previously found certificate" do
Puppet::SSL::Certificate.indirection.expects(:find).with(Puppet::SSL::CA_NAME).returns mock("cacert")
Puppet::SSL::Certificate.indirection.expects(:find).with("myname").returns(@cert).once
-- You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to [email protected].
For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
