On Dec 16, 2009, at 4:58 PM, Markus Roberts wrote: > 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
Have you tested this in the real world? That is, default expiration is usually something like 30 minutes; is this value stored in the cert somehow such that a cert is considered expired even though it's valid for five years? > * 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. I'm confused as to why these are relevant. It sounds like you're triggering expiration; couldn't you just as easily trigger removal via 'destroy'? Once again, this is also at least two significantly different changes - you've done a significant refactor and changed behaviour, and I can't at all see which is which. > 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 > . > > -- The remarkable thing about Shakespeare is that he really is very good, in spite of all the people who say he is very good. -- Robert Graves --------------------------------------------------------------------- Luke Kanies | http://reductivelabs.com | http://madstop.com -- 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.
