On Jul 3, 2009, at 2:23 PM, Brice Figureau wrote: > > Hi, > > I'm depiling some of the easy tickets in my redmine page. This one > was really > easier than I first thought when it was assigned to me. > So I think it can make it to 0.25, it's low risk and fixes a ssl bug.
Nice. > > Available in my github repository, branch tickets/master/2082 > > Brice > > Original commit msg: > This patch does two things: > * it enhance puppetca to list revoked certificates (prefixed by -) > * it fixes the ca crl verification which was broken > > Signed-off-by: Brice Figureau <[email protected]> > --- > lib/puppet/ssl/certificate_authority.rb | 1 + > lib/puppet/ssl/certificate_authority/interface.rb | 10 +++++++++- > sbin/puppetca | 3 ++- > spec/integration/ssl/certificate_authority.rb | 8 +++----- > spec/unit/ssl/certificate_authority.rb | 8 +++++++- > spec/unit/ssl/certificate_authority/interface.rb | 6 +++++- > 6 files changed, 27 insertions(+), 9 deletions(-) > > diff --git a/lib/puppet/ssl/certificate_authority.rb b/lib/puppet/ > ssl/certificate_authority.rb > index 4a7d461..06f1d00 100644 > --- a/lib/puppet/ssl/certificate_authority.rb > +++ b/lib/puppet/ssl/certificate_authority.rb > @@ -276,6 +276,7 @@ class Puppet::SSL::CertificateAuthority > store.add_file Puppet[:cacert] > store.add_crl crl.content if self.crl > store.purpose = OpenSSL::X509::PURPOSE_SSL_CLIENT > + store.flags = OpenSSL::X509::V_FLAG_CRL_CHECK_ALL| > OpenSSL::X509::V_FLAG_CRL_CHECK If you actually understand the CRL stuff and could document it somewhere, that would be just awesome. I have vague memories of late nights creating and testing and reading up on CRLs, and I came out with what I thought was functional code but no clear idea why it had to be built that way. Like most non-core parts of SSL, not many people seem to use it and even fewer people talk about it. > > unless store.verify(cert.content) > raise "Certificate for %s failed verification" % name > diff --git a/lib/puppet/ssl/certificate_authority/interface.rb b/lib/ > puppet/ssl/certificate_authority/interface.rb > index e455295..bf41d94 100644 > --- a/lib/puppet/ssl/certificate_authority/interface.rb > +++ b/lib/puppet/ssl/certificate_authority/interface.rb > @@ -60,8 +60,16 @@ class Puppet::SSL::CertificateAuthority::Interface > end > > hosts.uniq.sort.each do |host| > - if signed.include?(host) > + revoked = false > + begin > + ca.verify(host) unless requests.include?(host) > + rescue > + revoked = true > + end > + if not revoked and signed.include?(host) > puts "+ " + host > + elsif revoked > + puts "- " + host > else > puts host > end Mightn't there be other reasons for the cert not verifying? Does the exception provide the reason it didn't verify? If so, it'd be great to have that reason in parens after the hostname. E.g., - host.domain.com (Certificate is revoked) - other.domain.com (Certificate is expired) > > diff --git a/sbin/puppetca b/sbin/puppetca > index 572f72c..51826d9 100755 > --- a/sbin/puppetca > +++ b/sbin/puppetca > @@ -55,7 +55,8 @@ > # > # list:: > # List outstanding certificate requests. If '--all' is specified, > -# signed certificates are also listed, prefixed by '+'. > +# signed certificates are also listed, prefixed by '+', and revoked > +# certificates are prefixed by '-'. > # > # print:: > # Print the full-text version of a host's certificate. > diff --git a/spec/integration/ssl/certificate_authority.rb b/spec/ > integration/ssl/certificate_authority.rb > index 5f963f7..553c9b3 100755 > --- a/spec/integration/ssl/certificate_authority.rb > +++ b/spec/integration/ssl/certificate_authority.rb > @@ -50,13 +50,11 @@ describe Puppet::SSL::CertificateAuthority do > end > > it "should be able to revoke a host certificate" do > - pending("This test doesn't actually work yet") do > - @ca.generate("newhost") > + @ca.generate("newhost") > > - @ca.revoke("newhost") > + @ca.revoke("newhost") > > - lambda { @ca.verify("newhost") }.should raise_error > - end > + lambda { @ca.verify("newhost") }.should raise_error > end > > it "should have a CRL" do > diff --git a/spec/unit/ssl/certificate_authority.rb b/spec/unit/ssl/ > certificate_authority.rb > index 4c2466d..8011430 100755 > --- a/spec/unit/ssl/certificate_authority.rb > +++ b/spec/unit/ssl/certificate_authority.rb > @@ -585,7 +585,7 @@ describe Puppet::SSL::CertificateAuthority do > > describe "and verifying certificates" do > before do > - @store = stub 'store', :verify => true, :add_file > => nil, :purpose= => nil, :add_crl => true > + @store = stub 'store', :verify => true, :add_file > => nil, :purpose= => nil, :add_crl => true, :flags= => nil > > OpenSSL::X509::Store.stubs(:new).returns @store > > @@ -631,6 +631,12 @@ describe Puppet::SSL::CertificateAuthority do > @ca.verify("me") > end > > + it "should set the store flags to check the crl" do > + @store.expects(:flags=).with > OpenSSL::X509::V_FLAG_CRL_CHECK_ALL|OpenSSL::X509::V_FLAG_CRL_CHECK > + > + @ca.verify("me") > + end > + > it "should use the store to verify the certificate" do > @cert.expects(:content).returns "mycert" > > diff --git a/spec/unit/ssl/certificate_authority/interface.rb b/spec/ > unit/ssl/certificate_authority/interface.rb > index 784c6cf..c615544 100755 > --- a/spec/unit/ssl/certificate_authority/interface.rb > +++ b/spec/unit/ssl/certificate_authority/interface.rb > @@ -176,6 +176,7 @@ describe > Puppet::SSL::CertificateAuthority::Interface do > describe "and an empty array was provided" do > it "should print a string containing all certificate > requests" do > @ca.expects(:waiting?).returns %w{host1 host2} > + @ca.stubs(:verify) > > @applier = @class.new(:list, []) > > @@ -189,12 +190,14 @@ describe > Puppet::SSL::CertificateAuthority::Interface do > it "should print a string containing all certificate > requests and certificates" do > @ca.expects(:waiting?).returns %w{host1 host2} > @ca.expects(:list).returns %w{host3 host4} > + @ca.stubs(:verify) > + @ca.expects(:verify).with("host3").raises > > @applier = @class.new(:list, :all) > > @applier.expects(:puts).with "host1" > @applier.expects(:puts).with "host2" > - @applier.expects(:puts).with "+ host3" > + @applier.expects(:puts).with "- host3" > @applier.expects(:puts).with "+ host4" > > @applier.apply(@ca) > @@ -205,6 +208,7 @@ describe > Puppet::SSL::CertificateAuthority::Interface do > it "should print a string of all named hosts that > have a waiting request" do > @ca.expects(:waiting?).returns %w{host1 host2} > @ca.expects(:list).returns %w{host3 host4} > + @ca.stubs(:verify) > > @applier = @class.new(:list, %w{host1 host2 > host3 host4}) > > -- > 1.6.0.2 > > > > -- Westheimer's Discovery: A couple of months in the laboratory can frequently save a couple of hours in the library. --------------------------------------------------------------------- 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 -~----------~----~----~----~------~----~------~--~---
