[PUP-3788] Puppet Agent does not support Chained CRLs Created: 2014/12/22 Updated: 2019/04/04 Resolved: 2018/05/21 |
|
Status: | Closed |
Project: | Puppet |
Component/s: | None |
Affects Version/s: | PUP 3.7.1 |
Fix Version/s: | None |
Type: | Bug | Priority: | Normal |
Reporter: | Stan Duffy | Assignee: | Unassigned |
Resolution: | Duplicate | Votes: | 2 |
Labels: | Server | ||
Remaining Estimate: | Not Specified | ||
Time Spent: | Not Specified | ||
Original Estimate: | Not Specified | ||
Environment: |
CentOS 6 x86_64 Monolithic Puppet Master Running PE 3.7.1 |
Attachments: |
![]() ![]() ![]() ![]() ![]() ![]() ![]() ![]() ![]() ![]() ![]() ![]() |
||||||||||||||||||||||||
Issue Links: |
|
||||||||||||||||||||||||
Template: | customfield_10700 62370 | ||||||||||||||||||||||||
Epic Link: | Future Work for Intermediate CA Improvements | ||||||||||||||||||||||||
Team: | Server | ||||||||||||||||||||||||
Story Points: | 3 | ||||||||||||||||||||||||
Sprint: | Server 2017-07-25, Platform Core 2017-08-08, Platform Core 2017-08-22 | ||||||||||||||||||||||||
CS Priority: | Major | ||||||||||||||||||||||||
CS Frequency: | 2 - 5-25% of Customers | ||||||||||||||||||||||||
CS Severity: | 3 - Serious | ||||||||||||||||||||||||
CS Business Value: | 5 - $$$$$$ | ||||||||||||||||||||||||
CS Impact: | By not supporting chained CRLS the agent would not know that it should not talk to a master with a revoke cert. | ||||||||||||||||||||||||
Release Notes: | Bug Fix | ||||||||||||||||||||||||
QA Contact: | Stan Duffy | ||||||||||||||||||||||||
QA Risk Assessment: | Automate |
Description |
While trying to test the following configuration: https://docs.puppetlabs.com/puppet/3/reference/config_ssl_external_ca.html#option-2-single-intermediate-ca I found that trying to use the CRL of the Root CA or the Intermediate CA OR a bundled version of these did not work. Puppet Agent runs failed with the SSL error "unable to get certificate crl " To work around this issue, I have had to disable CRL checking on the agent by using the following command 'puppet config set -section agent certificate_revocation false'. We need a way to either accept a bundled CRL or have separate settings for the Root CA CRL and 'Issuer CA' CRL. |
Comments |
Comment by Kylo Ginsberg [ 2015/01/09 ] | ||||||||||||||||||||
Tentatively assigning to server team since this is in the CA space that the server team is re-working. Chris Price or Jeremy Barlow does that make sense? | ||||||||||||||||||||
Comment by Josh Cooper [ 2015/01/09 ] | ||||||||||||||||||||
There is an agent bug where once it downloads a CRL once it will never update it, see https://tickets.puppetlabs.com/browse/PUP-2310. It may be possible that the behavior you are seeing is because the agent is not using the CRL you think it is. Especially since it doesn't seem to work when using "the CRL of the Root CA". - Verify that the CRL matches the one you expect via $ openssl crl -in $(bundle exec puppet agent --configprint hostcrl) -noout -text | ||||||||||||||||||||
Comment by Kylo Ginsberg [ 2015/01/09 ] | ||||||||||||||||||||
Adding some of my comments from HipChat:
| ||||||||||||||||||||
Comment by Jeremy Barlow [ 2015/01/09 ] | ||||||||||||||||||||
Where Stan Duffy is testing this in the context of an external CA, I was thinking he would have disabled the Puppet CA service and have had to manually copy the CRL into the agent's conf dir. I don't recall seeing a puppet.conf setting related to separating the CRL for a root CA from the CRLs for other authorities in the chain into different files. I'd been thinking then that the assumption was that the single CRL file could have multiple CRL PEMs inside that would be loaded up. From the Puppet Server / Jetty side of things, I had been expecting that we're already covering the ability to load multiple CRL PEMs out of a single file. Basically, Jetty would be doing this for us based on the file name we pass over to it at initialization time - https://github.com/eclipse/jetty.project/blob/master/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SslContextFactory.java#L284. Do we expect that Puppet agents should be able to properly pull multiple CRL PEMs out of a single 'hostcrl' file and use all of them for validating the server it is connecting to? | ||||||||||||||||||||
Comment by Jeremy Barlow [ 2015/01/09 ] | ||||||||||||||||||||
+1 on getting to the root cause and not suggesting certificate_revocation be disabled. Also +1 on making this ticket public – assuming we don't believe a vulnerability has been discovered here. | ||||||||||||||||||||
Comment by Jeremy Barlow [ 2015/01/09 ] | ||||||||||||||||||||
Although, I read the documentation for the "certificate_revocation" puppet.conf setting – https://docs.puppetlabs.com/references/latest/configuration.html#certificaterevocation – as pertaining to whether or not the agent should try to download the CRL from the server and not specifically whether or not the agent would try to use whatever CRL it has available locally to validate the server it is talking to. For the case of an external CA where the Puppet CA service is disabled and the external CA's CRL is manually put in place on the agent, it probably makes sense to disable the agent from polling the server for a CRL since the endpoint wouldn't be functional anyway, right? | ||||||||||||||||||||
Comment by Josh Cooper [ 2015/01/09 ] | ||||||||||||||||||||
Jeremy Barlow yeah that setting makes no sense... If you set it to false, then we don't do CRL verification at all. The setting is conflating two different things (whether to download the CRL and whether to check the peer certs against the CRL) | ||||||||||||||||||||
Comment by Nicholas Fagerlund [ 2015/01/12 ] | ||||||||||||||||||||
This is a good time to also remember that puppet agent never updates the crl if it already has one on disk; it doesn't try to find out if the CA's copy is newer, so if you want your agents to do real revocation checking, you need either:
| ||||||||||||||||||||
Comment by Reid Vandewiele [ 2015/01/12 ] | ||||||||||||||||||||
Yes. The Puppet agent can be configured to trust multiple, related or unrelated CAs today. Use cases range anywhere from 3rd party CAs to HA master systems. The most intuitive way to support this is multiple PEM-encoded CRLs in the hostcrl file, since concatenated PEM-encoded certs is how we support multiple entries for localcacert. | ||||||||||||||||||||
Comment by Jean Bond [ 2015/01/12 ] | ||||||||||||||||||||
Docs are still broken on this issue ( | ||||||||||||||||||||
Comment by Jeremy Barlow [ 2015/01/12 ] | ||||||||||||||||||||
I'd like to get to understanding whether there is a real problem here or not. From Reid Vandewiele's description, it sounds as though the agent should already support the CRL configuration that Stan Duffy tried. The error message encountered, however, sounds as though the agent is failing to process the CRL correctly for some reason. Stan Duffy, could you attach the certs, private keys, and CRLs you were trying to use for your testing so that others could attempt to reproduce the problem you saw? | ||||||||||||||||||||
Comment by Reid Vandewiele [ 2015/01/12 ] | ||||||||||||||||||||
Jeremy Barlow my description was intended to convey how things feel like they should be, not necessarily how they are. My understanding is that today, the hostcrl file in fact doesn't support concatenated pem-encoded crls. | ||||||||||||||||||||
Comment by Jeremy Barlow [ 2015/01/12 ] | ||||||||||||||||||||
Thanks for the clarification, Reid Vandewiele. I agree with your description with respect to how one would expect this to work for CRLs – even though we don't believe it currently does – given how the implementation works for CAs. Josh Cooper - do you agree that this is actually a bug and is in the lack of client implementation – Puppet agent – support for validating multiple CRL PEMs from a single CRL file? Kylo Ginsberg - if this is an issue on the client side, would this make most sense to a "client" team? | ||||||||||||||||||||
Comment by Josh Cooper [ 2015/01/13 ] | ||||||||||||||||||||
Kylo Ginsberg I don't believe this is a vulnerability since the puppet agent run fails based on I see no reason to keep it private. Jeremy Barlow I'm a bit surprised this doesn't "just work". Puppet just hands off the file to openssl, and I thought openssl could handle a multi-crl pem file. I think we need to dig into this and understand what's going on. It could be that due to | ||||||||||||||||||||
Comment by Jeremy Barlow [ 2015/01/13 ] | ||||||||||||||||||||
+1 on opening this up to the public - do we have a quorum?
Josh Cooper Like I'd mentioned earlier, I think Stan Duffy would have been constructing the multi-CRL file manually and dropping that in place on the agent for this test since this is in the context of an "external CA" setup. Unless I'm misunderstanding the conversation in Stan Duffy, I still think it would be helpful if you could attach the certs, private keys, and CRL files you were using for this testing so that others can try to duplicate the results you've been seeing. Could you do that when you get a chance? | ||||||||||||||||||||
Comment by Jean Bond [ 2015/01/23 ] | ||||||||||||||||||||
Any movement here? I would like to get our docs on this fixed ( | ||||||||||||||||||||
Comment by Kylo Ginsberg [ 2015/01/23 ] | ||||||||||||||||||||
Thanks for the ping Jean Bond. Stan Duffy two things: Thanks! | ||||||||||||||||||||
Comment by Stan Duffy [ 2015/01/27 ] | ||||||||||||||||||||
I don't have a problem with this being Public. I have attached the following: | ||||||||||||||||||||
Comment by Andreas Paul [ 2015/07/09 ] | ||||||||||||||||||||
Josh Cooper If that is indeed how the Puppet agent tries to validate a CRL file, then there is your root cause.
| ||||||||||||||||||||
Comment by Andreas Paul [ 2015/07/10 ] | ||||||||||||||||||||
I started digging through the Puppet agent CRL code, trying to find the point where the agent loads the CRL file and only found this function: https://github.com/puppetlabs/puppet/blob/master/lib/puppet/ssl/certificate_revocation_list.rb#L17 Is this the function that I was looking for or is there something else using OpenSSL::X509::CRL to create a CRL from the disk file? If it is the correct function then I don't know how the loading of multiple CRLs from one file did ever work in Puppet, because OpenSSL::X509::CRL.new(crl_bundle_string) only returns the first CRL just like the openssl binary:
| ||||||||||||||||||||
Comment by Josh Cooper [ 2015/12/16 ] | ||||||||||||||||||||
Andreas Paul my bad, I was conflating openssl and puppet's behavior. It's possible for puppet agents to accept multiple CA certificates, because we add the path to the CA cert to the X509 store, and that method accepts multiple CA certs in the same file. However, the same isn't true for CRLs. Puppet loads the CRL through the indirector, and we add the in-memory CRL to the X509 store. So summary, the agent doesn't support multiple CRLs. | ||||||||||||||||||||
Comment by Graham Leggett [ 2016/01/27 ] | ||||||||||||||||||||
This bug just affected us, any timeline for a fix? | ||||||||||||||||||||
Comment by Eric Sorenson [ 2016/05/06 ] | ||||||||||||||||||||
Note to those affected by the bug: there is a workaround available by setting certificate_revocation=false in your puppet.conf or --no-certificate_revocation on the command line; this would, as earlier commenters note, introduce a slight decrease in operational security because the agent will continue to connect to a master whose cert has been revoked. However, also up-thread, the practical decrease in security is minimal due to We are working on full chained-CA support and fixing this for-real will be part of that effort. (As a side note, it seems weird that while the underlying OpenSSL library's CRL loading code does not support chained CAs, the CRL verification requires it if there is a chain-of-trust in the CA certificate.) | ||||||||||||||||||||
Comment by Adrien Thebo [ 2017/08/14 ] | ||||||||||||||||||||
The assumption that Puppet will have a single CRL is wired deeply into the code and unwinding this assumption may require a number of nontrivial and potentially dangerous changes. First off, Puppet::SSL::CertificateRevocationList itself is hardcoded for a single CRL, and it hardcodes a number of things like the CRL name. For example:
This by itself isn't a difficult fix but it's a demonstration that the singleton CRL assumption is deeply wired. Further complicating things is that the logic for fetching and caching the CRL is tightly interwoven with the indirector. The indirector is used both for making the HTTP request to fetch the CRL, which assumes a singleton instance, and caching the CRL, which has the singleton logic even more deeply wired. The Puppet::Indirector::SslFile terminus is built such that if we have a resource that relies on a single file or bundle, the #search method won't work at all. Even if #search could work on bundle files, the indirector itself doesn't support caching the results of #search which means that we can't properly cache and retrieve fetched CRLs. In effect in order to support multiple CRLs we'll need to re-plumb every location that touches CRLs; if that's the case then there might be a compelling case to just write something new and bypass the current, indirector entangled code. In the short run we can sidestep this by supporting checking for the end entity certificate, like what Apache does with SslCARevocationCheck leaf. Enabling leaf checking means dropping the OpenSSL::X509::V_FLAG_CRL_CHECK_ALL flag when creating an SSL store, and then we're done. Since we've never supported multiple CRLs this doesn't decrease security, and by allowing people to perform any CRL checking at all we've provided a bit more security while we look at properly supporting CRL bundles. For the sake of compatibility we can change the certificate_revocation setting to accept chain and leaf settings, alias true to chain to support the current behavior, and allow people to use leaf checking when intermediate CAs are in place. | ||||||||||||||||||||
Comment by Moses Mendoza [ 2017/08/15 ] | ||||||||||||||||||||
Adrien Thebo that seems reasonable. Also at this point its probably worth it to just bypass the indirector altogether, as you suggest. | ||||||||||||||||||||
Comment by Moses Mendoza [ 2017/08/24 ] | ||||||||||||||||||||
Per discussion on team it was determined that the total effort to support chained CRLs through the system exceeds our short term capacity. Decision was that | ||||||||||||||||||||
Comment by Maggie Dreyer [ 2018/05/21 ] | ||||||||||||||||||||
Duplicated by | ||||||||||||||||||||
Comment by Maggie Dreyer [ 2018/08/01 ] | ||||||||||||||||||||
This is fixed in Puppet 6 by the above ticket, |