Puppet

Low because there’s a workaround, but accepted because it shouldn’t happen in the first place, or should at least be handled more gracefully.

A more specific workaround is to remove ssldir. BTW: when you remove ssldir you loss your CA and signed certs, so it is not suitable for a production environment.

I’ve managed to have a working ssldir and one with this issue. These are the differences:

$ diff -rq  ssl-with-issue/ ssl-ok/
Files ssl-with-issue/ca/ca_crl.pem and ssl-ok/ca/ca_crl.pem differ
Files ssl-with-issue/ca/ca_crt.pem and ssl-ok/ca/ca_crt.pem differ
Files ssl-with-issue/ca/ca_key.pem and ssl-ok/ca/ca_key.pem differ
Only in ssl-ok/ca: ca_pub.pem
Files ssl-with-issue/ca/inventory.txt and ssl-ok/ca/inventory.txt differ
Files ssl-with-issue/ca/private/ca.pass and ssl-ok/ca/private/ca.pass differ
Files ssl-with-issue/ca/serial and ssl-ok/ca/serial differ
Only in ssl-with-issue/ca/signed:

... all our signed certs ...

Files ssl-with-issue/certs/ca.pem and ssl-ok/certs/ca.pem differ
Files ssl-with-issue/certs/one.ingent.net.pem and ssl-ok/certs/one.ingent.net.pem differ
Files ssl-with-issue/private_keys/one.ingent.net.pem and ssl-ok/private_keys/one.ingent.net.pem differ
Files ssl-with-issue/public_keys/one.ingent.net.pem and ssl-ok/public_keys/one.ingent.net.pem differ

So the key may be this ca_pub.pem that only gets created by 0.25.0 for a new CA … maybe we should migrate old ones?

The workaround is too disruptive.

David Escala wrote:

A more specific workaround is to remove ssldir. BTW: when you remove ssldir you loss your CA and signed certs, so it is not suitable for a production environment.

I’ve managed to have a working ssldir and one with this issue. These are the differences:

[…]

So the key may be this ca_pub.pem that only gets created by 0.25.0 for a new CA … maybe we should migrate old ones?

I think you might have nailed down the issue.

I was able to somewhat reproduce #2619, where I found that the client is missing the ca.pem file, thus it never even tries to SSL authenticate to the server which does its job (ie denying access). What happens when a 0.25 new client connects for the first time to a newly upgraded 0.25 master, it should normally store the ca cert and generate a csr that should be sent to the server. It seems to me the ca cert is never created, and then later on (see Puppet::Network::HttpPool.cert_setup) it never adds the SSL info to the http client code.

Brice Figureau wrote:

David Escala wrote:

A more specific workaround is to remove ssldir. BTW: when you remove ssldir you loss your CA and signed certs, so it is not suitable for a production environment.

I’ve managed to have a working ssldir and one with this issue. These are the differences:

[…]

So the key may be this ca_pub.pem that only gets created by 0.25.0 for a new CA … maybe we should migrate old ones?

I think you might have nailed down the issue.

I was able to somewhat reproduce #2619, where I found that the client is missing the ca.pem file, thus it never even tries to SSL authenticate to the server which does its job (ie denying access). What happens when a 0.25 new client connects for the first time to a newly upgraded 0.25 master, it should normally store the ca cert and generate a csr that should be sent to the server. It seems to me the ca cert is never created, and then later on (see Puppet::Network::HttpPool.cert_setup) it never adds the SSL info to the http client code.

OK, I found the issue (see #2619 for how to reproduce it).

In 0.24.x, the $ssldir/ca/ca_crt.pem file has the following Subject DN: CN=$fqdn In 0.25 the same file has the subject DN: CN=ca

All of the 0.25 SSL::Host code assume that the ca certificate has the subject DN: CN=ca. So basically everytime we deal with a certificate we know it is a ca either because its CN is ca or by its filename (which matches the CN).

When we register a new 0.25.x client to an upgraded master, it first ‘finds’ the CA through rest to the master and locally cache it as an SslFile. But since this ca cert is not named exactly ‘ca’, it is not recognized as a ca later when the client wants to connect back to the master, hence the issue.

Now, I really don’t know how to fix this…

It appears to me that we either need to change the assumption or document it (and how to deal with it).

Luke?

This is a bug, but it’s a bit of a doozie.

We need to fix the CA cert so its subject is the fqdn again, but in doing so we have to fix the cert recognition so that we can tell if a cert is a CA cert. This should just be a question of actually asking the stupid cert if it’s capable of signing certs.

Luke Kanies wrote:

This is a bug, but it’s a bit of a doozie.

We need to fix the CA cert so its subject is the fqdn again, but in doing so we have to fix the cert recognition so that we can tell if a cert is a CA cert. This should just be a question of actually asking the stupid cert if it’s capable of signing certs.

Yes good idea. I focused on the name of the not on the cert itself :–) Testing the cert has the critical extension basicConstraint containing CA:true should be enough:

def ca?(cert)
    cert.extensions.each do |ext|
        return true if ext.oid == "basicConstraints" and ext.value == 'CA:TRUE' and ext.critical?
    end
    false
end

This doesn’t prove the cert is the right CA, but in our context, it should work…

Brice Figureau wrote:

[…] This doesn’t prove the cert is the right CA, but in our context, it should work…

Yep, that should essentially work.

Looks like this needs a bit more thinking.

As discussed in the list, I’m going to produce two patches: One that has a minimal change and is for 0.25.1, and one that has a larger change and is for 0.26. The basic fix is this:

diff --git a/lib/puppet/indirector/rest.rb b/lib/puppet/indirector/rest.rb
index e1ee89f..ec3f70f 100644
--- a/lib/puppet/indirector/rest.rb
+++ b/lib/puppet/indirector/rest.rb
@@ -66,7 +66,11 @@ class Puppet::Indirector::REST < Puppet::Indirector::Terminus
     end
 
     def find(request)
-        deserialize network(request).get(indirection2uri(request), headers)
+        return nil unless result = deserialize(network(request).get(indirection2uri(request), headers))
+        unless result.name == request.key
+            result.name = request.key
+        end
+        result
     end
 
     def search(request)

I’m publishing a limited fix for 0.25.1 at tickets/0.25.x/2617 and a more complete fix at tickets/master/2617.

0.25.1 fix pushed in commit:“a1d3b04296babc42b6a00956508c86c18e2b39bc” in branch 0.25.x. Ticket re-assigned to 0.26.0 for larger fix.

Applied in changeset commit:“a1d3b04296babc42b6a00956508c86c18e2b39bc”.

Can you rebase the master branch please.

Pushed full fix in commit:“089ac3e37dd1418751bc4dfe152e09fbacbc5122” in branch master.