This patch implements the two-part suggestion from the ticket; 1) a client that receives a certificate that doesn't match its current private key does not accept, store or use the certificate--instead it removes any locally cached copies and acts as if the certificate had never been found.
2) a puppetmaster that receives a csr from a client for whom it already has a signed certificate now honors the request and considers it to supercede any previously signed certificates. In order to make the cache expiration work as expected, I changed a few assumptions in the caching system: * The expiration of a cached certificate is the earlier of the envelope expiration and the certificate's expiration, as opposed to just overriding the cache value * Telling the cache to expire an item now removes it from the cache if possible, rather than just setting an expiration date in the past and hoping that somebody notices. Signed-off-by: Markus Roberts <[email protected]> --- lib/puppet/indirector/envelope.rb | 4 +- lib/puppet/indirector/indirection.rb | 29 ++++++------- lib/puppet/indirector/ssl_file.rb | 2 +- lib/puppet/ssl/certificate.rb | 5 +- lib/puppet/ssl/host.rb | 47 ++++++--------------- lib/puppet/sslcertificates/ca.rb | 11 +++-- spec/unit/indirector/indirection.rb | 47 +++++++++++++-------- spec/unit/ssl/host.rb | 76 ++++++++------------------------- 8 files changed, 86 insertions(+), 135 deletions(-) diff --git a/lib/puppet/indirector/envelope.rb b/lib/puppet/indirector/envelope.rb index ef7952e..5fdea70 100644 --- a/lib/puppet/indirector/envelope.rb +++ b/lib/puppet/indirector/envelope.rb @@ -6,8 +6,6 @@ module Puppet::Indirector::Envelope attr_accessor :expiration def expired? - return false unless expiration - return false if expiration >= Time.now - return true + expiration and expiration < Time.now end end diff --git a/lib/puppet/indirector/indirection.rb b/lib/puppet/indirector/indirection.rb index dc7e58f..d762701 100644 --- a/lib/puppet/indirector/indirection.rb +++ b/lib/puppet/indirector/indirection.rb @@ -161,22 +161,19 @@ class Puppet::Indirector::Indirection end end - # Expire a cached object, if one is cached. Note that we don't actually - # remove it, we expire it and write it back out to disk. This way people - # can still use the expired object if they want. + # Expire a cached object, if one is cached. Note that we now actually + # remove it if possible, and only mark it as expired if destroy isn't + # supported. def expire(key, *args) - request = request(:expire, key, *args) - - return nil unless cache? - - return nil unless instance = cache.find(request(:find, key, *args)) - - Puppet.info "Expiring the %s cache of %s" % [self.name, instance.name] - - # Set an expiration date in the past - instance.expiration = Time.now - 60 - - cache.save(request(:save, instance, *args)) + if cache? and instance = cache.find(request(:find, key, *args)) + Puppet.info "Expiring the #{name} cache of #{instance.name}" + if cache.respond_to? :destroy + cache.destroy(request(:destroy, instance, *args)) + else + instance.expiration = Time.now - 1 + cache.save(request(:save,instance,*args)) + end + end end # Search for an instance in the appropriate terminus, caching the @@ -216,7 +213,7 @@ class Puppet::Indirector::Indirection return nil end - Puppet.debug "Using cached %s for %s" % [self.name, request.key] + Puppet.debug "Using cached #{name} for #{request.key}, good until #{cached.expiration}" return cached end diff --git a/lib/puppet/indirector/ssl_file.rb b/lib/puppet/indirector/ssl_file.rb index 6720269..fc1e65d 100644 --- a/lib/puppet/indirector/ssl_file.rb +++ b/lib/puppet/indirector/ssl_file.rb @@ -91,7 +91,7 @@ class Puppet::Indirector::SslFile < Puppet::Indirector::Terminus def save(request) path = path(request.key) dir = File.dirname(path) - + raise Puppet::Error.new("Cannot save %s; parent directory %s does not exist" % [request.key, dir]) unless FileTest.directory?(dir) raise Puppet::Error.new("Cannot save %s; parent directory %s is not writable" % [request.key, dir]) unless FileTest.writable?(dir) diff --git a/lib/puppet/ssl/certificate.rb b/lib/puppet/ssl/certificate.rb index f9297f3..b6cba99 100644 --- a/lib/puppet/ssl/certificate.rb +++ b/lib/puppet/ssl/certificate.rb @@ -28,7 +28,8 @@ class Puppet::SSL::Certificate < Puppet::SSL::Base end def expiration - return nil unless content - return content.not_after + # Our expiration is either that of the cache or the content, whichever comes first + cache_expiration = @expiration + [(content and content.not_after), cache_expiration].compact.sort.first end end diff --git a/lib/puppet/ssl/host.rb b/lib/puppet/ssl/host.rb index 11e11c5..75e51e5 100644 --- a/lib/puppet/ssl/host.rb +++ b/lib/puppet/ssl/host.rb @@ -94,12 +94,7 @@ class Puppet::SSL::Host # Remove all traces of a given host def self.destroy(name) - [Key, Certificate, CertificateRequest].inject(false) do |result, klass| - if klass.destroy(name) - result = true - end - result - end + [Key, Certificate, CertificateRequest].collect { |part| part.destroy(name) }.any? { |x| x } end # Search for more than one host, optionally only specifying @@ -107,12 +102,7 @@ class Puppet::SSL::Host # This just allows our non-indirected class to have one of # indirection methods. def self.search(options = {}) - classes = [Key, CertificateRequest, Certificate] - if klass = options[:for] - classlist = [klass].flatten - else - classlist = [Key, CertificateRequest, Certificate] - end + classlist = [options[:for] || [Key, CertificateRequest, Certificate]].flatten # Collect the results from each class, flatten them, collect all of the names, make the name list unique, # then create a Host instance for each one. @@ -127,8 +117,7 @@ class Puppet::SSL::Host end def key - return nil unless @key ||= Key.find(name) - @key + @key ||= Key.find(name) end # This is the private key; we can create it from scratch @@ -146,8 +135,7 @@ class Puppet::SSL::Host end def certificate_request - return nil unless @certificate_request ||= CertificateRequest.find(name) - @certificate_request + @certificate_request ||= CertificateRequest.find(name) end # Our certificate request requires the key but that's all. @@ -166,26 +154,19 @@ class Puppet::SSL::Host end def certificate - unless @certificate - generate_key unless key - + @certificate ||= ( # get the CA cert first, since it's required for the normal cert # to be of any use. - return nil unless Certificate.find("ca") unless ca? - return nil unless @certificate = Certificate.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" + if not (key or generate_key) or not (ca? or Certificate.find("ca")) or not (cert = Certificate.find(name)) or cert.expired? + nil + elsif not cert.content.check_private_key(key.content) + Certificate.expire(name) + Puppet.warning "Retrieved certificate does not match private key" + nil + else + cert end - end - @certificate - end - - def certificate_matches_key? - return false unless key - return false unless certificate - - return certificate.content.check_private_key(key.content) + ) end # Generate all necessary parts of our ssl host. diff --git a/lib/puppet/sslcertificates/ca.rb b/lib/puppet/sslcertificates/ca.rb index f6bcbc1..f9efc02 100644 --- a/lib/puppet/sslcertificates/ca.rb +++ b/lib/puppet/sslcertificates/ca.rb @@ -278,12 +278,13 @@ class Puppet::SSLCertificates::CA host = thing2name(csr) csrfile = host2csrfile(host) - if File.exists?(csrfile) - raise Puppet::Error, "Certificate request for %s already exists" % host - end + raise Puppet::Error, "Certificate request for #{host} already exists" if File.exists?(csrfile) + Puppet.settings.writesub(:csrdir, csrfile) { |f| f.print csr.to_pem } - Puppet.settings.writesub(:csrdir, csrfile) do |f| - f.print csr.to_pem + certfile = host2certfile(host) + if File.exists?(certfile) + Puppet.notice "Removing previously signed certificate #{certfile} for #{host}" + Puppet::SSLCertificates::Inventory::rebuild end end diff --git a/spec/unit/indirector/indirection.rb b/spec/unit/indirector/indirection.rb index 220aa24..ca2a412 100755 --- a/spec/unit/indirector/indirection.rb +++ b/spec/unit/indirector/indirection.rb @@ -536,7 +536,7 @@ describe Puppet::Indirector::Indirection do @indirection.expire("/my/key") end - it "should log that it is expiring any found instance" do + it "should log when expiring a found instance" do @cache.expects(:find).returns @cached @cache.stubs(:save) @@ -545,33 +545,44 @@ describe Puppet::Indirector::Indirection do @indirection.expire("/my/key") end - it "should set the cached instance's expiration to a time in the past" do - @cache.expects(:find).returns @cached - @cache.stubs(:save) + describe "and the terminus supports removal of cache items with destroy" do + it "should destroy the cached instance" do + @cache.expects(:find).returns @cached + @cache.expects(:destroy).with { |r| r.method == :destroy and r.key == "/my/key" } + @cache.expects(:save).never + @indirection.expire("/my/key") + end + end - @cached.expects(:expiration=).with { |t| t < Time.now } + describe "and the terminus does not support removal of cache items with destroy" do + it "should set the cached instance's expiration to a time in the past" do + @cache.expects(:find).returns @cached + @cache.stubs(:save) - @indirection.expire("/my/key") - end + @cached.expects(:expiration=).with { |t| t < Time.now } - it "should save the now expired instance back into the cache" do - @cache.expects(:find).returns @cached + @indirection.expire("/my/key") + end - @cached.expects(:expiration=).with { |t| t < Time.now } + it "should save the now expired instance back into the cache" do + @cache.expects(:find).returns @cached - @cache.expects(:save) + @cached.expects(:expiration=).with { |t| t < Time.now } - @indirection.expire("/my/key") - end + @cache.expects(:save) - it "should use a request to save the expired resource to the cache" do - @cache.expects(:find).returns @cached + @indirection.expire("/my/key") + end - @cached.expects(:expiration=).with { |t| t < Time.now } + it "should use a request to save the expired resource to the cache" do + @cache.expects(:find).returns @cached - @cache.expects(:save).with { |r| r.is_a?(Puppet::Indirector::Request) and r.instance == @cached and r.method == :save }.returns(@cached) + @cached.expects(:expiration=).with { |t| t < Time.now } - @indirection.expire("/my/key") + @cache.expects(:save).with { |r| r.is_a?(Puppet::Indirector::Request) and r.instance == @cached and r.method == :save }.returns(@cached) + + @indirection.expire("/my/key") + end end end end diff --git a/spec/unit/ssl/host.rb b/spec/unit/ssl/host.rb index 38a1f3e..9174f2e 100755 --- a/spec/unit/ssl/host.rb +++ b/spec/unit/ssl/host.rb @@ -90,55 +90,6 @@ describe Puppet::SSL::Host do Puppet::SSL::Host.localhost.should equal(two) end - it "should be able to verify its certificate matches its key" do - Puppet::SSL::Host.new("foo").should respond_to(:certificate_matches_key?) - end - - it "should consider the certificate invalid if it cannot find a key" do - host = Puppet::SSL::Host.new("foo") - host.expects(:key).returns nil - - host.should_not be_certificate_matches_key - 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(:certificate).returns nil - - host.should_not be_certificate_matches_key - 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 - - 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 - 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 - end - describe "when specifying the CA location" do before do [Puppet::SSL::Key, Puppet::SSL::Certificate, Puppet::SSL::CertificateRequest, Puppet::SSL::CertificateRevocationList].each do |klass| @@ -408,10 +359,11 @@ describe Puppet::SSL::Host do describe "when managing its certificate" do before do @realcert = mock 'certificate' - @cert = stub 'cert', :content => @realcert + @realcert.stubs(:check_private_key).with('private key').returns true + + @cert = stub 'cert', :content => @realcert, :expired? => false - @host.stubs(:key).returns mock("key") - @host.stubs(:certificate_matches_key?).returns true + @host.stubs(:key).returns stub("key",:content => 'private key' ) end it "should find the CA certificate if it does not have a certificate" do @@ -459,12 +411,22 @@ describe Puppet::SSL::Host do @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 + it "should immediately expire the cached copy if the found certificate does not match the private key" do + @realcert.expects(:check_private_key).with('private key').returns false + + Puppet::SSL::Certificate.stubs(:find).returns @cert + Puppet::SSL::Certificate.expects(:expire).with("myname") + + @host.certificate + end + + it "should not return a certificate if it does not match the private key" do + @realcert.expects(:check_private_key).with('private key').returns false Puppet::SSL::Certificate.stubs(:find).returns @cert + Puppet::SSL::Certificate.stubs(:expire).with("myname") - lambda { @host.certificate }.should raise_error(Puppet::Error) + @host.certificate.should == nil end it "should return any previously found certificate" do @@ -654,14 +616,14 @@ describe Puppet::SSL::Host do it "should catch and log errors during CSR saving" do @host.expects(:certificate).times(2).returns(nil).then.returns "foo" - @host.expects(:generate).times(2).raises(RuntimeError).then.returns nil + @host.expects(:generate).raises(RuntimeError).then.returns nil @host.stubs(:sleep) @host.wait_for_cert(1) end it "should sleep and retry after failures saving the CSR if waitforcert is enabled" do @host.expects(:certificate).times(2).returns(nil).then.returns "foo" - @host.expects(:generate).times(2).raises(RuntimeError).then.returns nil + @host.expects(:generate).raises(RuntimeError).then.returns nil @host.expects(:sleep).with(1) @host.wait_for_cert(1) end -- 1.6.4 -- 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.
