validate_x509 does not work if passing certificate and/or key by content

[philomory]

The call to validate_x509_rsa_key_pair on line 269 of site.pp assumes that that certificates are being passed in as paths rather than by content. If the certificate and content is being retrieved from hiera-eyaml and does not exist on disk on the puppet server, then the call to validate_x509_rsa_key_pair is guaranteed to fail.

Actually, looking at the code for validate_x509_rsa_key_pair in Stdlib, and subsequently looking at the Ruby documentation for OpenSSL::X509::Certificate and OpenSSL::PKey::RSA, it looks like you need to pass in the certificate and key content regardless, not the path to them.

[rcalixte]

@philomory You're correct about this. I've been mulling over how best to proceed. I'd love to utilize the function in Stdlib, but it might be a little tight to implement effectively. Definitely open to suggestions! :smile:

[philomory]

My suggestion would be the following: if the key pair is passed by content, pass the content verbatim to validate_x509_rsa_key_pair. If they're passed by source url, you might optionally want to use the file function to load the key pair content into string variables and validate those, but considering how many weird edge-cases source parameters can lead to in Puppet, you might just make validate_x509 only compatible with certs passed by content.

[greatflyingsteve]

Hi, Broad Institute folks! I came across this issue thread while I was trying to figure out why I couldn't get this function to work, either. Since we use (and love) your qualys_agent Puppet module, I figured karma demanded I share the love from my own research.

TL;DR: issues with validate_x509_rsa_key_pair() are not a "you" problem, the function is just incredibly broken.

Our use case for this would have been as a safety check for SSL key, cert, and CA chain contents retrieved from Hiera and encrypted with eYAML, which it seems pretty perfect for. Using pdk console, I grabbed a real, actual cert/key/CA chain set used in one of our development environments, and pasted their raw contents into a few quick variable assignments so I could work with them. I set to work seeing if I could grab the screwdriver by the correct end, and quickly discovered that validate_x509_rsa_key_pair() would always raise an exception. Not sometimes, always. There was no permutation of options, string escaping, argument order, or anything that would get it to succeed.

I dropped by the nearest handy shell server and performed an equivalent test using a couple of quick test files and the relevant openssl x509 commands; the cert and key definitely matched. Was it something I was doing wrong? Was there some gotcha in the implementation? I had no idea. The docs were uselessly sparse, so I went and read the implementation directly. And then I pulled out the Ruby docs, and the OpenSSL gem docs, and oh boy, did things get exciting from there.

It appears that the original author actually may not have understood the assignment. The function purports to "validate a PEM-formatted X.509 certificate and RSA private key using OpenSSL." The CORE of the implementation is:

unless cert.verify(key)
 raise <exception goes here>
end  

The key to this puzzle is in Ruby's OpenSSL gem docs. While the docs on the verify() method are sparse, they do specifically say the method "verifies the signature of the certificate, with the [given] public key" (emphasis mine). The method is meant to take a certificate and an RSA public key, not a private key, and ensure that the given certificate has been signed with that public key. The public key is part of a PEM-format x509 cert, but not an easy part to grab onto without some assistance. Meanwhile, the same class also provides a check_private_key() method. Would you like to guess what THAT does?

Okay anyway, ACTUAL TESTING TIME. Armed with the info from the docs, and with the key, cert, and chain cert stuck in files for easier loading, the following Ruby code actually worked as expected:

irb(main):002:0> require 'openssl'
=> true

# Load the chain cert text from file as a new OpenSSL::X509::Certificate object
irb(main):003:0> chain_cert = OpenSSL::X509::Certificate.new(File.read('test_chain_cert'))
=> <Ruby prints long Chain Certificate object summary>

# Load the issued cert text from file as a new OpenSSL::X509::Certificate object
irb(main):006:0> cert = OpenSSL::X509::Certificate.new(File.read('test_dev_cert'))
=> <Ruby prints long Certificate object summary>

# Load the private key text from file as a new OpenSSL::PKey::RSA object
irb(main):008:0> key = OpenSSL::PKey::RSA.new(File.read('test_dev_key'))
=> #<OpenSSL::PKey::RSA:0x0000000096fa78> 

irb(main):007:0> cert.verify(chain_cert.public_key) 
=> true 

irb(main):009:0> cert.check_private_key(key)
=> true 

...so what the stdlib implementation CLAIMS to do is in fact provided by the check_private_key() method that it doesn't use, not by the verify() method that it does use. The verify() method does actually work for what the Ruby docs say it's meant for, but there is no easy way in Puppet code to extract the public key from a CA's x509 chain cert (though it's easy enough in Ruby). I have no idea if or when this would ever have been useful to anyone. The most charitable explanation I can think of is that possibly the OpenSSL gem changed out from under the Stdlib function implementation, but I didn't have the energy left after all this digging to go back and check for that, and it wouldn't fix anything now anyways.

I would love to go toss together a quick 20-line Ruby implementation each of check_private_key() and public_key()/verify() for our use, and hopefully also assemble them in a way that returns a useful error rather than forcing Puppet to bail out without useful output. Whether I'll be allowed to publish them to the Forge or commit them to stdlib, I'm not sure, but I'll certainly try for that. The place I'd wanted to use them was inside a Puppet DSL function that gets called from all over our codebase by like ten different engineering teams in their respective profile classes and modules. Having it just bail like it does would be agonizingly unclear about where the error happened; Puppet only goes one or (in the case of templates) two layers up its own call stack when there are Ruby exceptions, so we wouldn't even have been able to see who was calling our DSL function, only that it encountered an error in some way. Arguably still better than downing a production server with a mismatched cert-and-key or cert-and-chain, but only just.

Hopefully all this is at least a little bit useful to you guys. Good luck!

[coreone]

@greatflyingsteve Thanks for all the work on this. As an FYI, we have moved this to a new name broadinstitute/puppet-certificates to get around some naming conflicts with the certs class name. If you ever get any traction on this, please open up an issue in that new repo if we need to do anything to get this working.

Thanks again!