The Puppet Labs Issue Tracker has Moved: https://tickets.puppetlabs.com
https://tickets.puppetlabs.com. See the following page for information on filing tickets with JIRA:
CSRs should be signed with SHA1, not MD5
|Assignee:||Patrick Carlisle||% Done:|
|Affected Puppet version:||2.6.12||Branch:||https://github.com/puppetlabs/puppet/pull/1172|
Ticket tracking is now hosted in JIRA: https://tickets.puppetlabs.com
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.
#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:
- PE 2.0.1
- Signing code unchanged
- 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
#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.
#9 Updated by Patrick Carlisle over 3 years ago
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.
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
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