On Jul 5, 2009, at 7:19 AM, Brice Figureau wrote:
>
> 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 | 11 ++++++++++-
> lib/puppet/ssl/certificate_authority/interface.rb | 10 +++++++++-
> sbin/puppetca | 4 +++-
> 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, 37 insertions(+), 10 deletions(-)
>
> diff --git a/lib/puppet/ssl/certificate_authority.rb b/lib/puppet/
> ssl/certificate_authority.rb
> index 4a7d461..10d13c2 100644
> --- a/lib/puppet/ssl/certificate_authority.rb
> +++ b/lib/puppet/ssl/certificate_authority.rb
> @@ -17,6 +17,14 @@ class Puppet::SSL::CertificateAuthority
>
> require 'puppet/ssl/certificate_authority/interface'
>
> + class CertificateVerificationError < RuntimeError
> + attr_accessor :error_code
> +
> + def initialize(code)
> + @error_code = code
> + end
> + end
> +
> class << self
> include Puppet::Util::Cacher
>
> @@ -276,9 +284,10 @@ 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
>
> unless store.verify(cert.content)
> - raise "Certificate for %s failed verification" % name
> + raise CertificateVerificationError.new(store.error),
> store.error_string
> end
> end
Very nice.
>
> diff --git a/lib/puppet/ssl/certificate_authority/interface.rb b/lib/
> puppet/ssl/certificate_authority/interface.rb
> index e455295..689ab10 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
> Puppet::SSL::CertificateAuthority::CertificateVerificationError =>
> details
> + revoked = details.to_s
> + end
> + if not revoked and signed.include?(host)
> puts "+ " + host
> + elsif revoked
> + puts "- " + host + " (" + revoked + ")"
The code looks fine, but the use of 'revoked' here again implies that
the only failure you'd get would be a cert revocation, but (e.g.) I
see time-based cert invalidity all the time.
>
> else
> puts host
> end
> diff --git a/sbin/puppetca b/sbin/puppetca
> index 572f72c..27ba916 100755
> --- a/sbin/puppetca
> +++ b/sbin/puppetca
> @@ -55,7 +55,9 @@
> #
> # 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
> +# or invalid certificates are prefixed by '-' (the verification
> outcome
> +# is printed in parenthesis).
> #
> # 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..d741ec4 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
> (Puppet
> ::SSL::CertificateAuthority::CertificateVerificationError.new(23),
> "certificate revoked")
>
> @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
> (certificate revoked)"
> @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
>
>
> >
--
I don't deserve this award, but I have arthritis and I don't deserve
that either. -- Jack Benny
---------------------------------------------------------------------
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
-~----------~----~----~----~------~----~------~--~---
