[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
CentOS 6 x86_64 Puppet Agent Running PE 3.7.1
CentOS 6 x86_64 Root & Intermediate CA


Attachments: File bc4day8u22sutut.delivery.puppetlabs.net.cert     File bc4day8u22sutut.delivery.puppetlabs.net.key     File ca_bundle.pem     File ca_crl.pem     File ca_crt.pem     File ca_key.pem     File crl_bundle.pem     File intermediate.cert.pem     File intermediate.key.pem     File root_ca_crl.pem     File to05sy0zz5zlsdj.delivery.puppetlabs.net.cert     File to05sy0zz5zlsdj.delivery.puppetlabs.net.key    
Issue Links:
Relates
relates to PUP-8654 Agents should save all CRLs downloade... Closed
relates to PUP-7845 Support leaf certificate CRL checking Closed
relates to SERVER-895 SSL configuration with intermediate CA Closed
relates to DOCUMENT-59 External CA - Support for CRL - Docum... Closed
relates to PUP-6697 Allow full downloaded CA bundle to be... Closed
Template:
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".

-Make sure to remove the agent's CRL, rerun the agent, and verify it downloads and uses the correct CRL-

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:

  • Offhand (and I'm not a security buff) I'd say the short-term docs fix is "We don't recommend this because you'd have to disable CRL checking". I'm probably missing some details/tradeoffs here though - thoughts?
  • Also since DOCUMENT-59 is public, do we need this one to be internal? It would make it easier for the docs change to have a public PUP ticket to reference.
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:

  • Out of band CRL distribution, for secure updates
  • Like, a cron job or something that periodically blows away the CRL, for... insecure? updates (it'll grab them from the CA server, but I can't remember how well it's validating the server certificate for that transaction.)
Comment by Reid Vandewiele [ 2015/01/12 ]

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?

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 (DOCUMENT-59). To figure out the doc fix, I need to know whether this ticket will be made public, so that I can link to it in the docs (or not). I also need to know what our recommendation is on this issue (They should disable CRL checking? They should not use options 2 and 3 until this issue is fixed? A different recommendation?)

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. PUP-3788 (this bug) exists to state that the lack of such support is a bug, and should be fixed.

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

Puppet Agent runs failed with the SSL error "unable to get certificate crl "

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 PUP-2310, the crl actually contained the single CA from the puppetmaster instead of the multi-crl file.

Comment by Jeremy Barlow [ 2015/01/13 ]

+1 on opening this up to the public - do we have a quorum?

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 PUP-2310, the crl actually contained the single CA from the puppetmaster instead of the multi-crl file.

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 PUP-2310, dropping the file in place under the agent's conf / SSL dir and restarting the agent should ensure that the multi-CRL file be the one used by the agent, right?

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 (DOCUMENT-59) so our users can know what they should do. Thanks!

Comment by Kylo Ginsberg [ 2015/01/23 ]

Thanks for the ping Jean Bond.

Stan Duffy two things:
1) do you have opinions on keeping this private? Comments above were that it could be open (which will make this easier on docs), but I wanted your opinion.
2) per Jeremy's question just above, can you attach the artifacts needed to duplicate this?

Thanks!

Comment by Stan Duffy [ 2015/01/27 ]

I don't have a problem with this being Public.

I have attached the following:
Puppet Master Private Key: to05sy0zz5zlsdj.delivery.puppetlabs.net.key
Puppet Master Cert: to05sy0zz5zlsdj.delivery.puppetlabs.net.cert
Puppet Agent Private Key: bc4day8u22sutut.delivery.puppetlabs.net.key
Puppet Agent Cert: bc4day8u22sutut.delivery.puppetlabs.net.cert
External Root CA Private Key: ca_key.pem
External Root CA Cert: ca_crt.pem
External Root CA CRL: root_ca_crl.pem
External Intermediate CA Private Key: intermediate.key.pem
External Intermediate CA Cert: intermediate.cert.pem
External Intermediate CA CRL: ca_crl.pem
Chained CA Bundle: ca_bundle.pem
Chained CRL Bundle: crl_bundle.pem

Comment by Andreas Paul [ 2015/07/09 ]

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.

Josh Cooper If that is indeed how the Puppet agent tries to validate a CRL file, then there is your root cause.
openssl can not handle multiple CRLs in one file.

$ curl -s https://tickets.puppetlabs.com/secure/attachment/17918/crl_bundle.pem | openssl crl -noout -text | grep Issuer
        Issuer: /C=US/ST=Oregon/O=Puppet Labs/OU=Engineering/CN=szxkjow4ae4a4zt.delivery.puppetlabs.net\x0A

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:

$ curl -s -o crl_bundle.pem https://tickets.puppetlabs.com/secure/attachment/17918/crl_bundle.pem && irb                                                                                                                  
irb(main):001:0> require 'openssl'
=> true
irb(main):002:0> crl_bundle_string = File.open('./crl_bundle.pem').read()
=> "-----BEGIN X509 CRL-----\nMIIB2jCBwzANBgkqhkiG9w0BAQUFADB9MQswCQYDVQQGEwJVUzEPMA0GA1UECBMG\nT3JlZ29uMRQwEgYDVQQKEwtQdXBwZXQgTGFiczEUMBIGA1UECxMLRW5naW5lZXJp\nbmcxMTAvBgNVBAMUKHN6eGtqb3c0YWU0YTR6dC5kZWxpdmVyeS5wdXBwZXRsYWJz\nLm5ldAoXDTE1MDEyNzIwNTk1MVoXDTE2MDEyNzIwNTk1MVowFTATAgIQBRcNMTUw\nMTI3MjA1OTUxWjANBgkqhkiG9w0BAQUFAAOCAQEAFs4G+3TsRN6ju5BrkUQJook8\nsLpCi237WU5vQZjVElEmRbDHtT7QgriCj2ftNB8z7R0RgPqdI9FSwJUrYIwuU/uO\nSW7FRPbBZQc+jzLBLyB/29ybKpgvyI84YGiberNSEQResU14oMIySZrQm+3nxm7t\noQf0l7STgbpsVUKRtyC/OsAfYoUhJW1HDvqQTsmda+fu5zVdalrGsmH4ufZGYPav\nLloontjU3QFnPSFwUSRccK/oBhX3e6SKaHKMetvAtyFhsDI03rNLmJFG6QiA2B5+\nKNk7RIPVqyAW5BM9xNGExfcAsG09J5DSgER3diJI5qaehzvgHl2mt2emCVsGIQ==\n-----END X509 CRL-----\n-----BEGIN X509 CRL-----\nMIIB7TCB1jANBgkqhkiG9w0BAQUFADCBkDELMAkGA1UEBhMCVVMxDzANBgNVBAgT\nBk9yZWdvbjERMA8GA1UEBxMIUG9ydGxhbmQxFDASBgNVBAoTC1B1cHBldCBMYWJz\nMTEwLwYDVQQDFChzenhram93NGFlNGE0enQuZGVsaXZlcnkucHVwcGV0bGFicy5u\nZXQKMRQwEgYDVQQLEwtFbmdpbmVlcmluZxcNMTUwMTI3MjA1OTQ5WhcNMTYwMTI3\nMjA1OTQ5WjAUMBICAQEXDTE1MDEyNzIwNTk0OVowDQYJKoZIhvcNAQEFBQADggEB\nACG8Wy0oyyyEAtEzTdfolxPWuf0uq0uxYxkZW3Z5H021KAx0IsrIyccTSRkwIPpv\nmjEJWjUx82gWXsgp7UJkjs8Ys3t4CcpHg2T6tqr7MxQDc2iaKdCNzZdP7ViHMiwy\nar9uUle6nm9RBKV9rQ2Mk+4tECs/85aL9dTNpatLab1D2kDS7WbqWx7ArYcbvdWI\nNEHnQ8rkGqLlRBcfpV5c4M3Oq3/yGKbe5RUAMfdwcwYLibccyYSLrCTRZvJWVhUX\nSurhaffqC7sYf11pNWwPs0Z9pF9JwIU3tx7IBYZ5sdShvsW6FR+D5/3xR9aoxDFM\nHimM1WcUrP47HrnFawLxOHs=\n-----END X509 CRL-----\n"
irb(main):003:0> OpenSSL::X509::CRL.new(crl_bundle_string)
=> -----BEGIN X509 CRL-----
MIIB2jCBwzANBgkqhkiG9w0BAQUFADB9MQswCQYDVQQGEwJVUzEPMA0GA1UECBMG
T3JlZ29uMRQwEgYDVQQKEwtQdXBwZXQgTGFiczEUMBIGA1UECxMLRW5naW5lZXJp
bmcxMTAvBgNVBAMUKHN6eGtqb3c0YWU0YTR6dC5kZWxpdmVyeS5wdXBwZXRsYWJz
Lm5ldAoXDTE1MDEyNzIwNTk1MVoXDTE2MDEyNzIwNTk1MVowFTATAgIQBRcNMTUw
MTI3MjA1OTUxWjANBgkqhkiG9w0BAQUFAAOCAQEAFs4G+3TsRN6ju5BrkUQJook8
sLpCi237WU5vQZjVElEmRbDHtT7QgriCj2ftNB8z7R0RgPqdI9FSwJUrYIwuU/uO
SW7FRPbBZQc+jzLBLyB/29ybKpgvyI84YGiberNSEQResU14oMIySZrQm+3nxm7t
oQf0l7STgbpsVUKRtyC/OsAfYoUhJW1HDvqQTsmda+fu5zVdalrGsmH4ufZGYPav
LloontjU3QFnPSFwUSRccK/oBhX3e6SKaHKMetvAtyFhsDI03rNLmJFG6QiA2B5+
KNk7RIPVqyAW5BM9xNGExfcAsG09J5DSgER3diJI5qaehzvgHl2mt2emCVsGIQ==
-----END X509 CRL-----
 
irb(main):004:0> 

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 PUP-2310.

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:

  # Convert a string into an instance.
  def self.from_s(string)
    super(string, 'foo') # The name doesn't matter
  end
 
# ...
 
  # The name doesn't actually matter; there's only one CRL.
  # We just need the name so our Indirector stuff all works more easily.
  def initialize(fakename)
    @name = _("crl")
  end

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 PUP-7845 (leaf CRL checking) moves us closer to the goal and is sufficient for short term requirements, so that is what we are proceeding with.

Comment by Maggie Dreyer [ 2018/05/21 ]

Duplicated by PUP-8652

Comment by Maggie Dreyer [ 2018/08/01 ]

This is fixed in Puppet 6 by the above ticket, PUP-8652.

Generated at Thu Oct 17 09:29:04 PDT 2019 using JIRA 7.7.1#77002-sha1:e75ca93d5574d9409c0630b81c894d9065296414.