markt-de / puppet-acme

Centralized SSL certificate management using acme.sh and the ACME protocol
https://forge.puppet.com/markt/acme
Apache License 2.0
9 stars 17 forks source link

[ Feature Request ] Allow the same certificate on multiple nodes #40

Closed oxc closed 1 year ago

oxc commented 2 years ago

I am in the process of migrating a service from one node to another. As part of this, I set up the same certificates on the second node, both with the respective hostname as a SAN:

The puppet run completes on the second node, but on the puppet master it now errors out with:

Error: Could not retrieve catalog from remote server: Error 500 on SERVER: Server Error: Evaluation Error: Error while evaluating a Resource Statement, Evaluation Error: Error while evaluating a Resource Statement, Duplicate declaration: Acme::Request[myservice.myhost.tld] is already declared; cannot redeclare (file: /etc/puppetlabs/code/environments/production/modules/acme/manifests/csr.pp, line: 192) (file: /etc/puppetlabs/code/environments/production/modules/acme/manifests/csr.pp, line: 192, column: 5) (file: /etc/puppetlabs/code/environments/production/modules/acme/manifests/certificate.pp, line: 59) on node puppetmaster.myhost.tld

The certificates are set up in hiera like this:

on node1 (which is also the puppetmaster, in case that matters):

acme::certificates:
 'myservice.myhost.tld node1.myhost.tld':
    use_profile: my_profile

on node2:

acme::certificates:
 'myservice.myhost.tld node2.myhost.tld':
    use_profile: my_profile

Perhaps the acme::request resources that are exported "to" the puppetmaster should be canonicalized with the hostname?

oxc commented 2 years ago

I probably could work around this by specifying the node's hostname as first domain, but that would only solve the issue for a single certificate. While we're at it, I could even contrive situations where I want two different certificates with the same hostname on a single node (for example. one with ocsp_must_staple, and one without). So perhaps it makes sense to make the resource title derived from a user-created resource, and not the (first) domain name.

fraenki commented 2 years ago

Perhaps the acme::request resources that are exported "to" the puppetmaster should be canonicalized with the hostname?

Yes, I think this would solve this issue. While at it, it's probably best to prefix all exported resources with the hostname, just for safekeeping.

c33s commented 2 years ago

shouldn't the exact same certificate be deploy on multiple nodes? what if i have a wildcard certificate "example.com *.example.com" where the same certificate would be used on many different nodes like "blog.example.com", "app.example.com", ...

oxc commented 2 years ago

shouldn't the exact same certificate be deploy on multiple nodes? what if i have a wildcard certificate "example.com *.example.com" where the same certificate would be used on many different nodes like "blog.example.com", "app.example.com", ...

They would not have the same key, so they could not all use the same certificate. This module does by design not distribute keys. They can use each use the same definition of a certificate of course, which is what this ticket is about.

However, in your example, each node would probably just define the hostname(s) it serves.

oxc commented 2 years ago

@fraenki, do you happen to have made any work on this?

Can you gauge what will happen if I just change the name of the exported resources? Will this cause problems with the resources that are already exported under the old names?

fraenki commented 2 years ago

@oxc Sorry for the slow response. I'll try to work on this next week, but can't promise anything.

oxc commented 2 years ago

So I've looked into this, and it's not as trivial as I thought. Changing the exported resources to have unique names is easy, but acme.sh on the puppet host will use the same directory for both certificates, which will not work because of different private keys.

To make this work, we would indeed have to implement it so that multiple certificates for the same domain are possible. They can then also be possible on the same host, as I've suggested above.

As far as I can see, this would work, but it would require setting a different CERT_HOME for every certificate (derived from $fqdn and its $name, not just $domain).

The docs of acme.sh are very vague regarding --cert-home, officially it is only supported while using the --install command, but according to the author overriding the config locations during --issue is possible for experts of acme.sh 😂

If I have read the source code right, it would indeed pick up the --cert-home flag on every command. If we set it to a stable value for each certificate, I think this would work.

However, of course this might break with a future version of acme.sh, since it's not officially supported. Is this a risk that you are willing to take? Perhaps we can hide this behavior behind an opt-in flag. If we do it per-certificate, it would also simplify/avoid backwards-compatibility with existing configs.

fraenki commented 2 years ago

However, of course this might break with a future version of acme.sh, since it's not officially supported. Is this a risk that you are willing to take? Perhaps we can hide this behavior behind an opt-in flag. If we do it per-certificate, it would also simplify/avoid backwards-compatibility with existing configs.

Well, that's a tough decision to make. Generally speaking it's not a good idea to base core functionality on undocumented/unsupported features. This is backed by the fact that we're talking about a rather complex change, so it would be non-trivial to revert back to the old behaviour if the undocumented feature will be removed from acme.sh in the future.

@oxc, you've suggested an opt-int flag... would you be willing to change your PR accordingly? I think this would be a good compromise and everyone could decide whether or not to take this risk.

oxc commented 2 years ago

It is currently opt-in as long as you use a resource name that's identical to the certificate name.

Would you expect the module to throw an error in other situations as long as the flag is not set?

fraenki commented 2 years ago

Would you expect the module to throw an error in other situations as long as the flag is not set?

Currently it is opt-in, but the user would not be aware of the possible drawback. And the features is promoted in README without any warning.

So yes, maybe an error would ensure that this feature is only used when strictly required – and that the user is aware that this feature may be removed in the future. A similar warning should be added to the new examples in the README.

Would you agree?

oxc commented 2 years ago

Yes, this sounds reasonable. I will try to update the PR in the next week or so.

fraenki commented 1 year ago

Generally speaking it's not a good idea to base core functionality on undocumented/unsupported features.

After thinking more about this, I think my previous statement is just wrong. The --config-home parameter is properly documented and even recommended for advanced use-cases.

So I'll just merge the PR, if @oxc does not stop me from doing that :D

oxc commented 1 year ago

Sorry, I thought about the PR once or twice, but could not find the time to refactor it. It has been working without problems for me, so I'm definitely not going to stop you, but I sure would like your input if it also works on your setup.

oxc commented 1 year ago

After thinking more about this, I think my previous statement is just wrong. The --config-home parameter is properly documented and even recommended for advanced use-cases.

I really don't want to shoot myself in the foot, but please be aware that while the --config-home is documented, the --cert-home is marked as "only valid for '--install' command", whereas this PR uses it also for the --signcsr and --issue commands.

fraenki commented 1 year ago

the --cert-home is marked as "only valid for '--install' command", whereas this PR uses it also for the --signcsr and --issue commands

I think that's a documentation bug in acme.sh, I'll probably submit a PR :)