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

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?

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

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.

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?

+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.

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?

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)

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.)

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.

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?)

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?

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.

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?

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 "

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.

+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?

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!

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!

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

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

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> 

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.

This bug just affected us, any timeline for a fix?

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.)

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.

Adrien Thebo that seems reasonable. Also at this point its probably worth it to just bypass the indirector altogether, as you suggest.

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.

Duplicated by PUP-8652

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