The Puppet Labs Issue Tracker has Moved: https://tickets.puppetlabs.com

This issue tracker is now in read-only archive mode and automatic ticket export has been disabled. Redmine users will need to create a new JIRA account to file tickets using https://tickets.puppetlabs.com. See the following page for information on filing tickets with JIRA:

Feature #13435

CSRs should be signed with SHA1, not MD5

Added by Michael Smith about 4 years ago. Updated over 3 years ago.

Status:ClosedStart date:03/27/2012
Priority:NormalDue date:
Assignee:Patrick Carlisle% Done:

0%

Category:SSL
Target version:3.0.0
Affected Puppet version:2.6.12 Branch:https://github.com/puppetlabs/puppet/pull/1172
Keywords:

We've Moved!

Ticket tracking is now hosted in JIRA: https://tickets.puppetlabs.com


Description

The code in certificate_authority.rb uses SHA1 to issue certs, but the CSR generation code in certificate_request.rb signs the CSR using “csr.sign(key, OpenSSL::Digest::MD5.new)”.

I might be naive, but I figure this could be changed to SHA1 and get Puppet one step closer to working in FIPS mode (#8120).

I couldn’t find the spot in the CA code where the CSR signature is actually verified. I suppose the CA should probably check the CSR is signed using a recommended algorithm, but FIPS mode would take care of disabling other algorithms anyway so I’m not particularly worried.


Related issues

Related to Puppet - Feature #8120: Let user change hashing algorithm, to avoid crashing on F... Code Insufficient 06/28/2011
Related to Puppet - Bug #13563: Puppet CA doesn't verify CSR signature Closed 03/31/2012

History

#1 Updated by Chris Price about 4 years ago

  • Status changed from Unreviewed to Accepted
  • Assignee set to Jeff Weiss

#2 Updated by Jeff Weiss about 4 years ago

From what I can tell in the signing code, there’s no explicit verification of the fingerprint of the CSR that’s generated on the agent to what appears on the master.

I believe that what is going on is that the agent generates a csr, fingerprints it, and displays it, so that when you run puppet cert list on the master you can say, “Yep. that fingerprint matches.” As it stands you can currently run

puppet cert list --digest sha1

Changing lib/puppet/ssl/certificate_authority/interface.rb from :MD5 to :SHA1 will alter the default digest for puppet cert list.

I ran this scenario:

Master

  • PE 2.0.1
  • Signing code unchanged

Agent

  • PE 2.0.1
  • Signing code changed from MD5 to SHA1

[Agent]# puppet agent --test
  ... fingerprint (sha1) ...
[Master]# puppet cert sign agent1.localdomain
[Agent]# puppet agent --test

Result: successful agent communication

#3 Updated by Jeff Weiss about 4 years ago

  • Status changed from Accepted to In Topic Branch Pending Review
  • Branch set to https://github.com/puppetlabs/puppet/pull/616

#4 Updated by Michael Smith about 4 years ago

Thanks, it’s really nice to see the switch from MD5/SHA1 to SHA256.

By the way, I was concerned with the CSR signature. The CSR signature is used for proof-of-possession of the private key. The fingerprint/digest is just a hash of the whole CSR including signature.

I couldn’t find the place where the server verifies the CSR signature. I think that’s because it doesn’t check. I can open a separate bug for that; it’s not really critical when the cert is just used for Puppet authentication, but it becomes important if people are using the certs for other purposes.

#5 Updated by Anonymous about 4 years ago

  • Status changed from In Topic Branch Pending Review to Merged - Pending Release
  • Target version set to 3.x

Thanks for this. The change in algorithm should be solid, and is in place now. Additional signature checking sounds reasonable, but should be a separate ticket.

#6 Updated by Anonymous almost 4 years ago

  • Target version changed from 3.x to 3.0.0

#7 Updated by Patrick Carlisle almost 4 years ago

  • Status changed from Merged - Pending Release to Code Insufficient

This causes a problem with 2.7 agent and 3.0 master, where fingerprint verification doesn’t work since they are calculated with a different algorithm on each side.

#8 Updated by Patrick Carlisle over 3 years ago

  • Assignee changed from Jeff Weiss to Patrick Carlisle

#9 Updated by Patrick Carlisle over 3 years ago

To clarify:

A puppet agent running 2.7 will still sign a cert with md5, and on that agent will display an md5 fingerprint. The master running 3.0 will display the sha256 fingerprint without any hint that it’s using a different algorithm, and it will appear not to match.

Some options:

1) default to using the algorithm that the csr was signed with (md5 on 2.7 agents, sha256 on newer ones), possibly using different algorithms for different certs in the output of cert list

2) default to showing sha256 fingerprints, but print a warning if a cert was signed with md5

3) nothing

I think in all cases the output should at least mention the digest being used.

#10 Updated by eric sorenson over 3 years ago

Seems like if we’re headed towards option 2 and have gone through all the work to discern whether, and emit a warning if, the CSR is md5-signed, we might as well print it out correctly too as in option 1 — and it’s not really an error to have old clients, it’s supposed to be supported. So IMO option 1 is a pretty clear winner.

#11 Updated by eric sorenson over 3 years ago

  • Status changed from Code Insufficient to Merged - Pending Release
  • Branch changed from https://github.com/puppetlabs/puppet/pull/616 to https://github.com/puppetlabs/puppet/pull/1172

Branch reworked and, updated pull request.

Merged in https://github.com/puppetlabs/puppet/compare/3443586c85b9…940c3e4dc5da

#13 Updated by Gary Wilson (@earthgecko) over 3 years ago

Well done, one step closer, the puppet team rocks. Even with all the scope creep. You are appreciated and do a damn fine job!

#14 Updated by Moses Mendoza over 3 years ago

  • Status changed from Merged - Pending Release to Closed

released in 3.0.0-rc7

Also available in: Atom PDF