diff options
| -rw-r--r-- | lib/puppet/indirector/envelope.rb | 4 | ||||
| -rw-r--r-- | lib/puppet/indirector/indirection.rb | 29 | ||||
| -rw-r--r-- | lib/puppet/indirector/ssl_file.rb | 2 | ||||
| -rw-r--r-- | lib/puppet/ssl/certificate.rb | 5 | ||||
| -rw-r--r-- | lib/puppet/ssl/host.rb | 47 | ||||
| -rw-r--r-- | lib/puppet/sslcertificates/ca.rb | 11 | ||||
| -rwxr-xr-x | spec/unit/indirector/indirection.rb | 47 | ||||
| -rwxr-xr-x | 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 ef7952ef6..5fdea7081 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 dc7e58f36..d762701f5 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 67202699d..fc1e65d22 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 f9297f380..b6cba99a7 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 11e11c597..75e51e5c8 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 f6bcbc1f7..f9efc02f7 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 220aa24fe..ca2a412e3 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 4e4d3f570..9174f2e49 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") - lambda { @host.certificate }.should raise_error(Puppet::Error) + @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") + + @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).raises(RuntimeError) + @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).raises(RuntimeError) + @host.expects(:generate).raises(RuntimeError).then.returns nil @host.expects(:sleep).with(1) @host.wait_for_cert(1) end |