Puppet

Seems reasonable – unless I am misunderstanding something.

Looks like this is a regression – we used to do this correctly, but with the new caching system, we cache the cert before we verify that it works for us.

We need to move the validation further up the chain, apparently.

We should make two fixes here:

1) When the client finds out it has a bad cert, it should remove the cached cert.

2) When the server receives a new CSR, it should remove any existing cert for the client. I think this one step will fix 99% of the problems people have with SSL.

Luke Kanies wrote:

We should make two fixes here:

1) When the client finds out it has a bad cert, it should remove the cached cert.

+1

2) When the server receives a new CSR, it should remove any existing cert for the client. I think this one step will fix 99% of the problems people have with SSL.

yes it would, but wouldn’t it then be possible that a malicious client could knock off all the existing clients with a bunch of CSR? Or am I just wrong in thinking that a client with the old cert (which was never thought to be removed) can’t connect anymore?

Peter Meier wrote:

yes it would, but wouldn’t it then be possible that a malicious client could knock off all the existing clients with a bunch of CSR? Or am I just wrong in thinking that a client with the old cert (which was never thought to be removed) can’t connect anymore?

The server doesn’t need a copy of the client’s certificate for the client to authenticate – this is one of the big differences between SSH-style auth and SSL auth.

For any malicious client to have any access at all, someone would have to decide to actually sign the CSR from the malicious client.

This is definitely more open and destructive than I would prefer to be, but it’s not at all a security hole and it’s a huge usability issue.

Luke Kanies wrote:

The server doesn’t need a copy of the client’s certificate for the client to authenticate – this is one of the big differences between SSH-style auth and SSL auth.

right, this was the part where I was unsure. Thanks for the clarification.

For any malicious client to have any access at all, someone would have to decide to actually sign the CSR from the malicious client.

obvious.

This is definitely more open and destructive than I would prefer to be, but it’s not at all a security hole and it’s a huge usability issue.

Definitely. Maybe we could add an option which would enable people to disable this behavior? It should be enabled by default, but for somebody who might have a well established CA-Setup and knows how everything works would like to have a stricter and less destructive setup. Do I see a “Hardening Puppet” chapter here? ;) Anyway I think it’s fine to open by default, but I would appreciate it if it would be possible to keep somehow the old behavior as well.

Patch up.

It doesn’t include Peter’s “keep the old behaviour” option, in part because I’m not seeing how that would play out in practice. If desired, it could be as simple as a one line change to lib/puppet/sslcertificates/ca.rb (making the removal conditional on the flag) and the usual setup for flags in defaults, etc.

It doesn’t include Peter’s “keep the old behaviour” option, in part because I’m not seeing how that would play out in practice. If desired, it could be as simple as a one line change to lib/puppet/sslcertificates/ca.rb (making the removal conditional on the flag) and the usual setup for flags in defaults, etc.

hmm yeah maybe it isn’t worth to do it, as there isn’t really any possible harm nor could any nasty things come up with the destructive way, couldn’t it?

Peter Meier wrote:

It doesn’t include Peter’s “keep the old behaviour” option, in part because I’m not seeing how that would play out in practice. If desired, it could be as simple as a one line change to lib/puppet/sslcertificates/ca.rb (making the removal conditional on the flag) and the usual setup for flags in defaults, etc.

hmm yeah maybe it isn’t worth to do it, as there isn’t really any possible harm nor could any nasty things come up with the destructive way, couldn’t it?

Correct, no harm can come.

I say this is a YAGNI feature, and we shouldn’t implement it until someone specifically asks for it.

I tested this and it seems to work – Dan welcome a second opinion. Also fixed a rebase issue.

Pushed in commit:“0dc2dbafe65b59bfbb3ab66e26f595260bdde356” in branch 0.25.x

hmm yeah maybe it isn’t worth to do it, as there isn’t really any possible harm nor could any nasty things come up with the destructive way, couldn’t it?

Correct, no harm can come.

Actually, there is harm done in connection with autosigning: When autosigning is active, any client can pretend to be another node as long as the desired node name matches the autosigning pattern(s), because the CA doesn’t care about pre-existing certs anymore and just overwrites them. In consequence, autosigning can make your puppetmaster accept any client with any requested node name. If there are node specific secrets distributed with puppet, an attacker can simply pretend to be another node with the “puppetd —fqdn” option to get the other node’s secrets.

The default behaviour should be to reject new CSRs for a node that already has a signed cert.

Closing in lieu of #3360.