Puppet bugs #17295 and #18393

[Alex Harvey]

Hi all,

I've spent the holidays trying to make my upgrade to puppet v3 on my n+1 versions of Unix work and in the process found two more bugs, one a known issue, and the other a new issue, and these are stopping puppet from working on HP-UX.

I'm happy to send in patches for them but I'll need some advice/discussion.


Redmine #17295 - puppet not honouring --digest

This is a real showstopper on HP-UX unless you happen to like compiling OpenSSL.

I've investigated and hacked together a workaround that works for me -

# diff /usr/local/lib/ruby/gems/1.8/gems/puppet-3.0.1/lib/puppet/ssl/certificate_request.rb*
62c62,66
<     csr.sign(key, OpenSSL::Digest::SHA256.new)
---
>     if OpenSSL::Digest.const_defined?('SHA256')
>       csr.sign(key, OpenSSL::Digest::SHA256.new)
>     elsif OpenSSL::Digest.const_defined?('SHA1')
>       csr.sign(key, OpenSSL::Digest::SHA1.new)
>     end

# diff /usr/local/lib/ruby/gems/1.8/gems/puppet-3.0.1/lib/puppet/ssl/certificate_authority.rb.orig /usr/local/lib/ruby/gems/1.8/gems/puppet-3.0.1/lib/puppet/ssl/certificate_authority.rb
278c278,283
<     cert.content.sign(host.key.content, OpenSSL::Digest::SHA256.new)
---
>
>     if OpenSSL::Digest.const_defined?('SHA256')
>       cert.content.sign(host.key.content, OpenSSL::Digest::SHA256.new)
>     elsif OpenSSL::Digest.const_defined?('SHA1')
>       cert.content.sign(host.key.content, OpenSSL::Digest::SHA1.new)
>     end

This allows me to generate CSRs which is great, but doesn't seem to be the right solution.

From reading the help page for puppet agent I tend to agree with the Greg Boug who raised the issue that --digest ought to affect both the algorithm used to generate a fingerprint (which it apparently does) and also the algorithm used to generate the CSR. 

If people agree, I will fix it so that it does this.


Redmine #18393 - puppet assumes that all versions of diff support -u whereas the HP-UX version doesn't and neither did Solaris 8.

This one looks harder to fix because there is a global default set in diff_args in lib/puppet/defaults.rb.  The whole concept of having a global default doesn't seem sensible if we're passing an argument in that can't be relied upon to be globally available.  So some thoughts on what to do here would help greatly.


Happy new year to all!

Best regards,
Alex

[Andy Parker]

On Sat, Jan 5, 2013 at 8:28 AM, Alex Harvey <alexh...@gmail.com> wrote:

Hi all,

I've spent the holidays trying to make my upgrade to puppet v3 on my n+1 versions of Unix work and in the process found two more bugs, one a known issue, and the other a new issue, and these are stopping puppet from working on HP-UX.

Thanks for working on this!

 

I'm happy to send in patches for them but I'll need some advice/discussion.


Redmine #17295 - puppet not honouring --digest

This is a real showstopper on HP-UX unless you happen to like compiling OpenSSL.

I've investigated and hacked together a workaround that works for me -

# diff /usr/local/lib/ruby/gems/1.8/gems/puppet-3.0.1/lib/puppet/ssl/certificate_request.rb*
62c62,66
<     csr.sign(key, OpenSSL::Digest::SHA256.new)
---
>     if OpenSSL::Digest.const_defined?('SHA256')
>       csr.sign(key, OpenSSL::Digest::SHA256.new)
>     elsif OpenSSL::Digest.const_defined?('SHA1')
>       csr.sign(key, OpenSSL::Digest::SHA1.new)
>     end

# diff /usr/local/lib/ruby/gems/1.8/gems/puppet-3.0.1/lib/puppet/ssl/certificate_authority.rb.orig /usr/local/lib/ruby/gems/1.8/gems/puppet-3.0.1/lib/puppet/ssl/certificate_authority.rb
278c278,283
<     cert.content.sign(host.key.content, OpenSSL::Digest::SHA256.new)
---
>
>     if OpenSSL::Digest.const_defined?('SHA256')
>       cert.content.sign(host.key.content, OpenSSL::Digest::SHA256.new)
>     elsif OpenSSL::Digest.const_defined?('SHA1')
>       cert.content.sign(host.key.content, OpenSSL::Digest::SHA1.new)
>     end

I see what you are getting at. The fact that this is showing up in two places like this makes me think that there needs to be something pulled out to handle signing. Maybe something like:

  class Puppet::SSL::CertificateSigner

    def initialize

      @digest = OpenSSL::Digest.const_defined?('SHA256') ? OpenSSL::Digest::SHA256 : OpenSSL::Digest::SHA1

    end

    def sign(content, key)

      content.sign(key, @digest.new)

    end

  end

Then use that at those two points. I haven't looked at the code around what you show, so there might be more things can can be moved around at the same time.

 

This allows me to generate CSRs which is great, but doesn't seem to be the right solution.

From reading the help page for puppet agent I tend to agree with the Greg Boug who raised the issue that --digest ought to affect both the algorithm used to generate a fingerprint (which it apparently does) and also the algorithm used to generate the CSR. 

If people agree, I will fix it so that it does this.

I would be a little wary of conflating these things together. It isn't clear to me how much the --digest should affect.

 

Redmine #18393 - puppet assumes that all versions of diff support -u whereas the HP-UX version doesn't and neither did Solaris 8.

This one looks harder to fix because there is a global default set in diff_args in lib/puppet/defaults.rb.  The whole concept of having a global default doesn't seem sensible if we're passing an argument in that can't be relied upon to be globally available.  So some thoughts on what to do here would help greatly.

I've always thought it a little strange that we create these diffs by shelling out and running diff. There should be another way of doing this. Normally I would say that we should just rely on a library for generating diffs, but I know that we like to keep the core from requiring other libraries in order to work.

 

Happy new year to all!

Best regards,
Alex

--
You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
To view this discussion on the web visit https://groups.google.com/d/msg/puppet-dev/-/0uyUg-xObAEJ.
To post to this group, send email to puppe...@googlegroups.com.
To unsubscribe from this group, send email to puppet-dev+...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.

[Alex Harvey]

Hi Andy,

On Tuesday, January 8, 2013 6:19:40 AM UTC+11, Andy Parker wrote:

On Sat, Jan 5, 2013 at 8:28 AM, Alex Harvey <alexh...@gmail.com> wrote:

 

From reading the help page for puppet agent I tend to agree with the Greg Boug who raised the issue that --digest ought to affect both the algorithm used to generate a fingerprint (which it apparently does) and also the algorithm used to generate the CSR. 

I would be a little wary of conflating these things together. It isn't clear to me how much the --digest should affect.

Yes I am also wary. 

Here's what I can see --digest doing so far -

1)

# puppet cert list --all |grep myhost
+ "myhost.example.com" (SHA256) E7:96:FB:68:38:B5:6B:B7:98:AB:79:79:6C:A6:84:3F:97:F1:1E:D6:5B:3A:AB:F4:0F:5D:EF:ED:CB:DF:F9:75

# puppet cert list --all --digest MD5 |grep myhost
+ "myhost.example.com" (MD5) C0:46:14:5E:0E:73:63:00:5A:72:5A:61:84:A9:24:C5

From the help pages -

# puppet help cert
...
* --digest:
  Set the digest for fingerprinting (defaults to the the digest used when
  signing the cert). Valid values depends on your openssl and openssl ruby
  extension version.

2)

Second use is in

# puppet help agent
...
* --fingerprint:
  Display the current certificate or certificate signing request
  fingerprint and then exit. Use the '--digest' option to change the
  digest algorithm used.
...
* --digest:
  Change the certificate fingerprinting digest algorithm. The default is
  SHA256. Valid values depends on the version of OpenSSL installed, but
  will likely contain MD5, MD2, SHA1 and SHA256.

This allows me on my client to do

# puppet agent --fingerprint
(SHA256) E7:96:FB:68:38:B5:6B:B7:98:AB:79:79:6C:A6:84:3F:97:F1:1E:D6:5B:3A:AB:F4:0F:5D:EF:ED:CB:DF:F9:75

and then because I am really in love with MD5 hashes I can also do -

# puppet agent --fingerprint --digest MD5
(MD5) C0:46:14:5E:0E:73:63:00:5A:72:5A:61:84:A9:24:C5

3)

# puppet man ca

* `fingerprint` - :
  `SYNOPSIS`

  puppet ca fingerprint [--digest ALGORITHM]

  `DESCRIPTION`

  Undocumented action.

  `OPTIONS`
  <--digest ALGORITHM> -
  The hash algorithm to use when displaying the fingerprint


I also note a comment by Jeff Weiss in lib/puppet/ssl/host.rb -

  def to_pson(*args)
    my_cert = Puppet::SSL::Certificate.indirection.find(name)
    pson_hash = { :name  => name }

    my_state = state

    pson_hash[:state] = my_state
    pson_hash[:desired_state] = desired_state if desired_state

    thing_to_use = (my_state == 'requested') ? certificate_request : my_cert

    # this is for backwards-compatibility
    # we should deprecate it and transition people to using
    # pson[:fingerprints][:default]
    # It appears that we have no internal consumers of this api
    # --jeffweiss 30 aug 2012
    pson_hash[:fingerprint] = thing_to_use.fingerprint

    # The above fingerprint doesn't tell us what message digest algorithm was used
    # No problem, except that the default is changing between 2.7 and 3.0. Also, as
    # we move to FIPS 140-2 compliance, MD5 is no longer allowed (and, gasp, will
    # segfault in rubies older than 1.9.3)
    # So, when we add the newer fingerprints, we're explicit about the hashing
    # algorithm used.
    # --jeffweiss 31 july 2012
    pson_hash[:fingerprints] = {}
    pson_hash[:fingerprints][:default] = thing_to_use.fingerprint

    suitable_message_digest_algorithms.each do |md|
      pson_hash[:fingerprints][md] = thing_to_use.fingerprint md
    end
    pson_hash[:dns_alt_names] = thing_to_use.subject_alt_names

    pson_hash.to_pson(*args)
  end

  # eventually we'll probably want to move this somewhere else or make it
  # configurable
  # --jeffweiss 29 aug 2012
  def suitable_message_digest_algorithms
    [:SHA1, :SHA256, :SHA512]
  end

I am yet to fully get my head around what to_pson is used for.  However, I am wondering if this move to FIPS 140-2 compliance and Jeff's comment about ruby segfaulting when using MD5 means I should handle a case where neither SHA1 nor SHA256 is available differently. 

So perhaps a new option is needed to choose (1) the algorithm used to generate a CSR (2) the algorithm used to create a certificate using puppet cert generate, (3) option used via puppet ca generate.

Or maybe all this is too ambitious and I should just refactor to create a class that takes care of signing a certificate and have it gracefully handle the situation where SHA256 isn't available.

[Andy Parker]

On Mon, Jan 7, 2013 at 10:03 PM, Alex Harvey <alexh...@gmail.com> wrote:

Hi Andy,

On Tuesday, January 8, 2013 6:19:40 AM UTC+11, Andy Parker wrote:

On Sat, Jan 5, 2013 at 8:28 AM, Alex Harvey <alexh...@gmail.com> wrote:

 

From reading the help page for puppet agent I tend to agree with the Greg Boug who raised the issue that --digest ought to affect both the algorithm used to generate a fingerprint (which it apparently does) and also the algorithm used to generate the CSR. 

I would be a little wary of conflating these things together. It isn't clear to me how much the --digest should affect.

Yes I am also wary. 

Here's what I can see --digest doing so far -

[snip]

They all look fairly consistent in dealing with the fingerprint of something.

 

I also note a comment by Jeff Weiss in lib/puppet/ssl/host.rb -

[snip]

I am yet to fully get my head around what to_pson is used for.  However, I am wondering if this move to FIPS 140-2 compliance and Jeff's comment about ruby segfaulting when using MD5 means I should handle a case where neither SHA1 nor SHA256 is available differently. 

to_pson is what generates the JSON (PSON is because of a collision that happened with ActiveSupport and so we hand to rename a module and the name started leaking out) that we send as a response in web requests, or anywhere that we need to show it as JSON.

 

So perhaps a new option is needed to choose (1) the algorithm used to generate a CSR (2) the algorithm used to create a certificate using puppet cert generate, (3) option used via puppet ca generate.

Or maybe all this is too ambitious and I should just refactor to create a class that takes care of signing a certificate and have it gracefully handle the situation where SHA256 isn't available.

It is as ambitious as you want it to be :) I've found all of the certification handling stuff pretty hard to follow, so I would be all ears on what we could do to make it work better/be more consistent/be easier to use. 

--
You received this message because you are subscribed to the Google Groups "Puppet Developers" group.

To view this discussion on the web visit https://groups.google.com/d/msg/puppet-dev/-/Bql3ya0CPX8J.

[Alex Harvey]

Hi Andy,

On Wednesday, January 9, 2013 11:15:09 AM UTC+11, Andy Parker wrote:

It is as ambitious as you want it to be :) I've found all of the certification handling stuff pretty hard to follow, so I would be all ears on what we could do to make it work better/be more consistent/be easier to use.

I think we should break it into a bugfix and a refactor/feature add.

Bug #17295 is a big problem that makes puppet v3 essentially unusable on HP-UX unless you compile your own OpenSSL.  This is because the HP Porting Archive version of OpenSSL doesn't seem to support SHA256 (at least doesn't on my 11.23 boxes).  But we can fix #17295 fairly easily and quickly in line with the proposal above in this thread.

A separate redmine could track a refactor/feature add.  It seems to me that the default hashing algorithm should be in defaults.rb.  Indeed, I see there are some defaults there that aren't used - e.g.

    :ca_md => {
      :default    => "md5",
      :desc       => "The type of hash used in certificates.",
    },

-bash-3.2$ grep -r :ca_md lib/
lib/puppet/defaults.rb:    :ca_md => {

Then, I still think, after seeking guidance from the design of the openssl command itself, that to avoid violating the Principle of Least Surprise we ought to reuse --digest -- unless someone can think of a weird situation where you'd want to use one algorithm for fingerprinting and another for signing a certificate and somehow do these two things simultaneously.  I can't think how that would happen.

Thus from man 1 dgst  we get the usage of openssl dgst -

 SYNOPSIS
      openssl dgst [-md5|-md4|-md2|-sha1|-sha|-mdc2|-ripemd160|-dss1] [-c]
      [-d] [-hex] [-binary] [-out filename] [-sign filename] [-passin arg]
      [-verify filename] [-prverify filename] [-signature filename] [file...]

E.g. I want a fingerprint and am happy with the default hashing algorithm -

# openssl dgst /var/lib/puppet/ssl/public_keys/myhost.example.com.pem
MD5(/var/lib/puppet/ssl/public_keys/myhost.example.com.pem)= ca4ad42cbc8c0f31618a9e316509df13

or I want a SHA1 fingerprint -

# openssl dgst -sha1 /var/lib/puppet/ssl/public_keys/myhost.example.com.pem
SHA1(/var/lib/puppet/ssl/public_keys/myhost.example.com.pem)= 663ea11d9f66d5705b67e393a02226b0d883fde2

or I want to sign my cert using SHA1 -

# openssl dgst -sha1 -sign somefile.csr -out somefile.pem

So how does all this sound?

[Alex Harvey]

On Thursday, January 10, 2013 12:47:52 AM UTC+11, Alex Harvey wrote:

So how does all this sound?

Oh - and if the --digest is no good what string should I use for that option on CSRs instead?

[Daniel Pittman]

On Mon, Jan 7, 2013 at 10:03 PM, Alex Harvey <alexh...@gmail.com> wrote:

So, the deal with FIPS 140-2 is that they made it a failing grade for
your crypto library to support MD5, full stop. So, if OpenSSL is
compiled in FIPS 140-2 compliant mode, MD5 is unavailable.

Ruby blindly assumes that it exists. It also assumes that SHA1 and
friends exist based on the date version of OpenSSL, without a check on
the FIPS 140-2 status.

They might fix their bug and stop segfaulting, but you absolutely need
to be concerned that the SHA1 algorithm may not exist for long; it has
shown some weakness, and the US government are slowly moving away from
it to other algorithms. The SHA3 process was part of that.

I have no strong opinion here, just that information. :)

--
Daniel Pittman
⎋ Puppet Labs Developer – http://puppetlabs.com
♲ Made with 100 percent post-consumer electrons

[Andy Parker]

On Wed, Jan 9, 2013 at 5:47 AM, Alex Harvey <alexh...@gmail.com> wrote:

Hi Andy,

On Wednesday, January 9, 2013 11:15:09 AM UTC+11, Andy Parker wrote:

It is as ambitious as you want it to be :) I've found all of the certification handling stuff pretty hard to follow, so I would be all ears on what we could do to make it work better/be more consistent/be easier to use.

I think we should break it into a bugfix and a refactor/feature add.

Bug #17295 is a big problem that makes puppet v3 essentially unusable on HP-UX unless you compile your own OpenSSL.  This is because the HP Porting Archive version of OpenSSL doesn't seem to support SHA256 (at least doesn't on my 11.23 boxes).  But we can fix #17295 fairly easily and quickly in line with the proposal above in this thread.

I think that split sounds reasonable. Fix the immediate problem, and then work toward something better.

 

A separate redmine could track a refactor/feature add.  It seems to me that the default hashing algorithm should be in defaults.rb.  Indeed, I see there are some defaults there that aren't used - e.g.

    :ca_md => {
      :default    => "md5",
      :desc       => "The type of hash used in certificates.",
    },

-bash-3.2$ grep -r :ca_md lib/
lib/puppet/defaults.rb:    :ca_md => {

Yeah, we've been finding these things. There are quite a few settings that are defined in there, but don't actually get used. Josh and I were looking into some of those, and I just had a conversation with him about signing and such with SSL. There are intricacies in there that I'm not fully familiar with, so I'll let him chime in on some of that.

However, I think what you have said makes sense. Can you convert the examples that you give below to the equivalent puppet commands so that we can all be clear on what you are proposing for changes to the puppet commands?

 

Then, I still think, after seeking guidance from the design of the openssl command itself, that to avoid violating the Principle of Least Surprise we ought to reuse --digest -- unless someone can think of a weird situation where you'd want to use one algorithm for fingerprinting and another for signing a certificate and somehow do these two things simultaneously.  I can't think how that would happen.

Thus from man 1 dgst  we get the usage of openssl dgst -

 SYNOPSIS
      openssl dgst [-md5|-md4|-md2|-sha1|-sha|-mdc2|-ripemd160|-dss1] [-c]
      [-d] [-hex] [-binary] [-out filename] [-sign filename] [-passin arg]
      [-verify filename] [-prverify filename] [-signature filename] [file...]

E.g. I want a fingerprint and am happy with the default hashing algorithm -

# openssl dgst /var/lib/puppet/ssl/public_keys/myhost.example.com.pem
MD5(/var/lib/puppet/ssl/public_keys/myhost.example.com.pem)= ca4ad42cbc8c0f31618a9e316509df13

or I want a SHA1 fingerprint -

# openssl dgst -sha1 /var/lib/puppet/ssl/public_keys/myhost.example.com.pem
SHA1(/var/lib/puppet/ssl/public_keys/myhost.example.com.pem)= 663ea11d9f66d5705b67e393a02226b0d883fde2

or I want to sign my cert using SHA1 -

# openssl dgst -sha1 -sign somefile.csr -out somefile.pem

So how does all this sound?

--

You received this message because you are subscribed to the Google Groups "Puppet Developers" group.

To view this discussion on the web visit https://groups.google.com/d/msg/puppet-dev/-/bYh_cg3tDGYJ.

[Alex Harvey]

On Thursday, January 10, 2013 5:58:02 AM UTC+11, Andy Parker wrote:

Yeah, we've been finding these things. There are quite a few settings that are defined in there, but don't actually get used. Josh and I were looking into some of those, and I just had a conversation with him about signing and such with SSL. There are intricacies in there that I'm not fully familiar with, so I'll let him chime in on some of that.

However, I think what you have said makes sense. Can you convert the examples that you give below to the equivalent puppet commands so that we can all be clear on what you are proposing for changes to the puppet commands?

The only change to puppet's functionality would be to connect --digest wherever applicable to override the default algorithm of SHA256.  The help pages and online documentation will need to be updated too and I'm happy to do that.

Thus the following are the proposed new uses for --digest -

1)   puppet agent -t [ --digest DIGEST ]

An initial certificate signing request from a client will allow digest to be optionally specified.  Although the option 'puppet agent --digest DIGEST' already exists it currently doesn't do anything unless used in conjunction with --fingerprint.  So this new usage is in addition to the existing usage via 'puppet agent -t --fingerprint [ --digest DIGEST ]'.  Note that currently when SHA256 doesn't exist we get the following -

# puppet agent -t --fingerprint
Error: Could not run: Unsupported digest algorithm (SHA256).

For overall consistency I would propose to fix this behaviour too (which seems to be again an error coming from OpenSSL itself and not Puppet) such that puppet always tries SHA1 after trying SHA256 even when just computing a fingerprint.  It would seem rather ugly to have it otherwise.

2)   puppet ca generate [ --digest DIGEST ]    # same as the above except done via puppet ca.

3)   puppet cert generate [ --digest DIGEST ]    # same as the above.

4)   puppet cert --sign CERTIFICATE [ --digest DIGEST ]   # digest algorithm to use when signing a certificate.

Meanwhile I think 'puppet certificate_request' and 'puppet certificate_revocation_list' subcommands are broken.  At least they don't seem to do what the man pages say they will do -

# puppet cert list
  "myhost.example.com" (SHA1) 97:68:B9:0B:B1:E7:05:A9:03:12:3D:C6:1B:38:8C:6A:6C:B1:D1:F2
#

# puppet certificate_request search myhost.example.com

#

# puppet certificate_revocation_list search myhost.example.com
Error: Could not call 'search' on 'certificate_revocation_list': can't convert nil into String
Error: Could not call 'search' on 'certificate_revocation_list': can't convert nil into String
Error: Try 'puppet help certificate_revocation_list search' for usage
#

Likewise the 'puppet certificate' subcommand doesn't seem to be fully implemented either. 

Thus, (1) to (4) above seem to be extent of the proposed changes as well as extending the internal default behaviour to be the highest supported digest algorith where applicable.

How does all this sound?

[Josh Cooper]

Hi Alex,

On Sun, Jan 13, 2013 at 11:29 PM, Alex Harvey <alexh...@gmail.com> wrote:
> On Thursday, January 10, 2013 5:58:02 AM UTC+11, Andy Parker wrote:
>>
>>
>> Yeah, we've been finding these things. There are quite a few settings that
>> are defined in there, but don't actually get used. Josh and I were looking
>> into some of those, and I just had a conversation with him about signing and
>> such with SSL. There are intricacies in there that I'm not fully familiar
>> with, so I'll let him chime in on some of that.
>>

I would recommend not reusing the ca_md setting, because the name of
the setting doesn't really make sense when talking about the signature
algorithm that the client would use to generate a certificate signing
request (CSR).

>> However, I think what you have said makes sense. Can you convert the
>> examples that you give below to the equivalent puppet commands so that we
>> can all be clear on what you are proposing for changes to the puppet
>> commands?
>
>
> The only change to puppet's functionality would be to connect --digest
> wherever applicable to override the default algorithm of SHA256. The help
> pages and online documentation will need to be updated too and I'm happy to
> do that.
>
> Thus the following are the proposed new uses for --digest -
>
> 1) puppet agent -t [ --digest DIGEST ]
>
> An initial certificate signing request from a client will allow digest to be
> optionally specified. Although the option 'puppet agent --digest DIGEST'
> already exists it currently doesn't do anything unless used in conjunction
> with --fingerprint. So this new usage is in addition to the existing usage
> via 'puppet agent -t --fingerprint [ --digest DIGEST ]'. Note that
> currently when SHA256 doesn't exist we get the following -
>
> # puppet agent -t --fingerprint
> Error: Could not run: Unsupported digest algorithm (SHA256).
>

I'm concerned about overloading the same flag to specify different
types of algorithms. To generate a CSR, we need to know what signature
algorithm to use, e.g. sha256WithRSAEncryption. To generate a
fingerprint, we only need to know what hash algorithm to apply to the
public key.

If we only allow the user to specify the hash portion of the signature
algorithm, then the code has to make hard-coded assumptions about the
type of encryption algorithm to use, and therefore, the types of keys
in use (RSA vs DSA). Sadly, puppet already does this, but I'd prefer
to not make it worse.

I would recommend leaving the `--digest` flag specific to specifying
the hash algorithm when generating a fingerprint.

Then add a different flag for specifying the signature algorithm.
Since this infrequently changes, I'd recommend making a global Puppet
setting (but not ca_md).

Ideally all of our ssl and certificate handling code should be
agnostic with respect to the underlying algorithms. This would make it
possible to easily change the algorithms in the future, similar to how
openssl provides EVP functions:
http://www.openssl.org/docs/crypto/evp.html. For example,
Puppet::Host::SSL#digest_algorithm is an example of what not to do,
because it makes assumptions about the syntax of the signature
algorithm.

> For overall consistency I would propose to fix this behaviour too (which
> seems to be again an error coming from OpenSSL itself and not Puppet) such
> that puppet always tries SHA1 after trying SHA256 even when just computing a
> fingerprint. It would seem rather ugly to have it otherwise.
>
> 2) puppet ca generate [ --digest DIGEST ] # same as the above except
> done via puppet ca.
>
> 3) puppet cert generate [ --digest DIGEST ] # same as the above.
>
> 4) puppet cert --sign CERTIFICATE [ --digest DIGEST ] # digest algorithm
> to use when signing a certificate.
>

These would all use the signature algorithm option.

> Meanwhile I think 'puppet certificate_request' and 'puppet
> certificate_revocation_list' subcommands are broken.
>
> At least they don't
> seem to do what the man pages say they will do -
>
> # puppet cert list
> "myhost.example.com" (SHA1)
> 97:68:B9:0B:B1:E7:05:A9:03:12:3D:C6:1B:38:8C:6A:6C:B1:D1:F2
> #
>
> # puppet certificate_request search myhost.example.com
>

Yes, I get the same thing. I am able to output the cert if I specify
the terminus to use, but I don't understand how anyone else would know
that:

# puppet certificate_request --terminus ca search 'agent1'
-----BEGIN CERTIFICATE REQUEST-----
MIIEVjCCAj4CAQAwETEPMA0GA1UEAwwGYWdlbnQxMIICIjANBgkqhkiG9w0BAQEF
...
-----END CERTIFICATE REQUEST-----

> #
>
> # puppet certificate_revocation_list search myhost.example.com
> Error: Could not call 'search' on 'certificate_revocation_list': can't
> convert nil into String
> Error: Could not call 'search' on 'certificate_revocation_list': can't
> convert nil into String
> Error: Try 'puppet help certificate_revocation_list search' for usage
> #
>

So search is not a supported action, why I have no idea. The
application does mention this in its help, but again the UX here is
terrible. Instead you have to say `find` and give it a dummy
argument.:

$ puppet certificate_revocation_list find whatever
-----BEGIN X509 CRL-----
MIICejBkAgEBMA0GCSqGSIb3DQEBBQUAMCIxIDAeBgNVBAMMF1B1cHBldCBDQTog
...
-----END X509 CRL-----

> Likewise the 'puppet certificate' subcommand doesn't seem to be fully
> implemented either.
>

It appears you need to specify the --ca-location (or --ca_location)
option, though `puppet cert` needs no such thing.

All of the face-based certificate applications are a mess...

> Thus, (1) to (4) above seem to be extent of the proposed changes as well as
> extending the internal default behaviour to be the highest supported digest
> algorith where applicable.
>
>
> How does all this sound?

Something else to keep in mind, and something puppet has historically
gotten wrong, is that we can't assume all agents and masters are using
the same set of algorithms, key lengths, etc. So an agent may
generate a CSR using md5WithRSAEncryption, but the master should be
able to use SHA256 when outputting fingerprints.

And one thing that puppet does wrong currently, is that it always uses
SHA256 on the master when signing certificates. Instead, it should,
depending on the CA policy, sign the CSR using the same signature
algorithm that was used to generate the CSR, or the one that the
master is configured to use. As it is now, if you have one agent that
doesn't support SHA256, then you have to globally lower the security
on your master when signing certificates for all other agents.

Finally, when testing your changes, I recommend setting different
default algorithms on your agent and master to ensure they can still
interoperate.

Josh

--
Josh Cooper
Developer, Puppet Labs

[Alex Harvey]

Hi Josh,

On Tuesday, January 15, 2013 8:50:53 AM UTC+11, Josh Cooper wrote:

I'm concerned about overloading the same flag to specify different
types of algorithms. To generate a CSR, we need to know what signature
algorithm to use, e.g. sha256WithRSAEncryption. To generate a
fingerprint, we only need to know what hash algorithm to apply to the
public key.

If we only allow the user to specify the hash portion of the signature
algorithm, then the code has to make hard-coded assumptions about the
type of encryption algorithm to use, and therefore, the types of keys
in use (RSA vs DSA). Sadly, puppet already does this, but I'd prefer
to not make it worse.

Obviously it's almost the same from a coding point of view whether I extend --digest or create a new option, e.g.

# puppet agent -t [ --signature AGORITHM ]

That said I am still not understanding why extending --digest would not be the least surprising solution, i.e. allow --digest to affect the 'digest algorithm' a.k.a. hashing algorithm in any situation where a hashing algorithm is required.  This seems more consistent with familiar system commands like openssl where passing -sha1 in any context would select the hash algorithm used - whether in the context of generating a fingerprint (openssl dgst), or a CSR (openssl req).

Okay - perhaps after I have digested the code it will become clearer. 

I would recommend leaving the `--digest` flag specific to specifying
the hash algorithm when generating a fingerprint.

Then add a different flag for specifying the signature algorithm.
Since this infrequently changes, I'd recommend making a global Puppet
setting (but not ca_md).

Ideally all of our ssl and certificate handling code should be
agnostic with respect to the underlying algorithms. This would make it
possible to easily change the algorithms in the future, similar to how
openssl provides EVP functions:
http://www.openssl.org/docs/crypto/evp.html. For example,
Puppet::Host::SSL#digest_algorithm is an example of what not to do,
because it makes assumptions about the syntax of the signature
algorithm.

Is that method or the surrounding code actually being used though?

We've got in
lib/puppet/ssl/base.rb

 89   def fingerprint(md = :SHA256)
 90     mds = md.to_s.upcase
 91     digest(mds).to_hex
 92   end
 93
 94   def digest(algorithm=nil)
 95     unless algorithm
 96       algorithm = digest_algorithm
 97     end
 98
 99     Puppet::SSL::Digest.new(algorithm, content.to_der)
100   end
101
102   def digest_algorithm
103     # The signature_algorithm on the X509 cert is a combination of the digest
104     # algorithm and the encryption algorithm
105     # e.g. md5WithRSAEncryption, sha256WithRSAEncryption
106     # Unfortunately there isn't a consistent pattern
107     # See RFCs 3279, 5758
108     digest_re = Regexp.union(
109       /ripemd160/i,
110       /md[245]/i,
111       /sha\d*/i
112     )
113     ln = content.signature_algorithm
114     if match = digest_re.match(ln)
115       match[0].downcase
116     else
117       raise Puppet::Error, "Unknown signature algorithm '#{ln}'"
118     end
119   end
120

Maybe I'll go through the whole of lib/puppet/ssl and stick print statements everywhere to figure out what's actually be used and when.  But at the moment I don't think this code is used.

[Josh Cooper]

Hi Alex,

On Tue, Jan 15, 2013 at 5:31 PM, Alex Harvey <alexh...@gmail.com> wrote:
> Hi Josh,
>
>
> On Tuesday, January 15, 2013 8:50:53 AM UTC+11, Josh Cooper wrote:
>>
>>
>> I'm concerned about overloading the same flag to specify different
>> types of algorithms. To generate a CSR, we need to know what signature
>> algorithm to use, e.g. sha256WithRSAEncryption. To generate a
>> fingerprint, we only need to know what hash algorithm to apply to the
>> public key.
>>
>> If we only allow the user to specify the hash portion of the signature
>> algorithm, then the code has to make hard-coded assumptions about the
>> type of encryption algorithm to use, and therefore, the types of keys
>> in use (RSA vs DSA). Sadly, puppet already does this, but I'd prefer
>> to not make it worse.
>
>
> Obviously it's almost the same from a coding point of view whether I extend
> --digest or create a new option, e.g.
>
> # puppet agent -t [ --signature AGORITHM ]
>
> That said I am still not understanding why extending --digest would not be
> the least surprising solution, i.e. allow --digest to affect the 'digest
> algorithm' a.k.a. hashing algorithm in any situation where a hashing
> algorithm is required.

My concern was increasing the number of places where we hard code RSA
as the encryption algorithm. As in what happens when someone wants to
use a third-party CA that uses DSA? We'd have to add another parameter
to specify which encryption algorithm to use.

With that said, puppet has other issues preventing us from supporting
third-party CAs, trust chains, etc.

> This seems more consistent with familiar system
> commands like openssl where passing -sha1 in any context would select the
> hash algorithm used - whether in the context of generating a fingerprint
> (openssl dgst), or a CSR (openssl req).
>

Yeah, that makes sense, and I imagine that's what most puppet users
would expect. As long as we don't couple 'ca' and 'digest', that
sounds good to me.

> Okay - perhaps after I have digested the code it will become clearer.
>
>> I would recommend leaving the `--digest` flag specific to specifying
>> the hash algorithm when generating a fingerprint.
>>
>> Then add a different flag for specifying the signature algorithm.
>> Since this infrequently changes, I'd recommend making a global Puppet
>> setting (but not ca_md).
>>
>> Ideally all of our ssl and certificate handling code should be
>> agnostic with respect to the underlying algorithms. This would make it
>> possible to easily change the algorithms in the future, similar to how
>> openssl provides EVP functions:
>> http://www.openssl.org/docs/crypto/evp.html. For example,
>> Puppet::Host::SSL#digest_algorithm is an example of what not to do,
>> because it makes assumptions about the syntax of the signature
>> algorithm.
>
>
> Is that method or the surrounding code actually being used though?
>

Yep, when generating a new certificate request we log the fingerprint
of the cert:

Puppet.info "Certificate Request fingerprint (#{digest.name}):
#{digest.to_hex}"

Fun times.