[pve-devel] RFC for ACME DNS Challenge

[Fabian Grünbichler]

On October 21, 2019 12:11 pm, Wolfgang Link wrote:
> comment inline
> 
> On 10/18/19 11:28 AM, Fabian Grünbichler wrote:
>> so this got a bit longer than expected - just a high-level feedback, I
>> haven't actually tested anything yet since there are too many open
>> general design questions for that to make sense.
>>
>> On October 14, 2019 1:08 pm, Wolfgang Link wrote:
>>> This series also includes a new GIT repository on proxdev.
>>> The path is staff/wl/pve-acme.
>> please send those as patches as well for the next iteration - otherwise
>> how are we supposed to discuss them here? some quick notes anyway:
> I ask Thomas before, and he said it is ok for this new repository to go 
> this way.
>> - don't set permissions via patches (do we really need that? those files
>>    are either unused or just sourced, not directly executed?)
> 
> No from a technical point of view, but lintian was thrown a warning 
> about the permission.

that doesn't mean you should generate a patch to set the permission - 
just override the permissions in debian/rules, or add a lintian override 
if they don't actually need to be executable.

>> - use debian/patches, not manual patching via Makefile
> Will do.

I just noticed some more things to consider:
- think about how to run the external script with as little privilege as 
  possible (it just needs to do some HTTP requests after all)
- think about possible side-effects of the plugin config data and how to 
  mitigate those (AFAICT _read_conf does not directly source the file at 
  least ;))
- if possible, avoid _initHome, or even make it and the save*conf 
  functions no-ops.

if we have our config on pmxcfs, we don't want acme.sh to litter any 
home directory with copies of DNS API tokens or the acme.sh script or 
anything else.

>> do we really need to package all of acme.sh? wouldn't a short stub with
>> the following methods be enough (note: this is for ALL plugins, if we
>> just want to support a subset this list would probably get smaller):
>  From a technical point of the problem no.
> 
> But in the future, a new plugin uses a not stub new function,
> 
> we have to search what feature is missing and stub it.
> 
> I guess it is much easier to maintain it if we have these small four 
> patches on top of the acme.sh repository.
> Also, if we ship this package user could use it for alternative use like 
> appalling certs in containers.
> 
> I know this is not the main propose of this here, but it does not harm?

the problem with this is that we are then expected to support all those 
use cases as well. in general, we don't care about in-guest setup beyond 
the basics. certainly not application-level stuff like TLS certificate 
deployment.

>> [SNIP]
>>
>>> The acme_sh project is used as DNS API plugin systems.
>>> So we can reuse the already defiend plugins.
>>> I deside to install the complett acme_sh scrips so somwone could use it
>>> for alternative use.
>> but the whole reason this series exists is that we want to eliminate the
>> need for users to manually setup and use acme.sh instead of our
>> integration?
> 
> See the container case above.
> 
> I don't meant that the user should use it for Proxmox web server 
> certificates.

if the user wants to setup certificates in containers, they can already 
do so (either in the container, or via ansible/puppet/.., or with any 
ACME client on the host and pct push / qm guest exec).

the user gains nothing by using some arbitrary snapshot of acme.sh that 
we ship, compared to just using acme.sh directly as recommended by 
upstream. we lose a lot of time and energy trouble-shooting users that 
try to use our copy of acme.sh that we only ship for integration of the 
DNS plugins.

> 
>>> I'm not sure about where we save the information about the dns_plugin.
>>> I deside to load it dynamicly like we do with ceph key for the storage.
>>> Alternative we could save the information in the node config,
>>> as I already specify in patch manger 6 Add dns_api_config.
>>>
>>> The dns key file is not standiziert so ervery plugin expect other paramerts.
>>> So I would say the dns key file has to be created from the user manually.
>> see acme.sh's _read_conf()/_save_conf() - the format is a simple
>> key=value, with the latter optionally and transparently base64
>> encoded/decoded (although none of the current dnsapi plugins seem to use
>> this feature).
>>
>> see the long comment to pve-common #1
> It is indeed a key=value format, but the keys count and the key itself 
> aren't the same.

yes. see my long comment ;)

>>> If someone need a OVH key for testing please contact me.
>>>
>>> Steps to test.
>>> The api key file must exists on /etc/pve/priv/acme/dns_ovh.cred
>>>
>>> 1.) pvenode acme account register default<mail at example.invalid>
>>> 2.) pvenode config set --acme domains=test.linksystems.li,plugin=acme_sh
>>> 3.) pvenode cert order
>>>
>>> These patches are only tested against the OVH API because of missing alternative possibilities.
>>>
>>> There are two bugs in this Series.
>>> I send it anyway because they are not essential to the genrall functionality
>>> and this is anyway an RFC and not the final version.
>>> Known bugs:
>>> Alias does not work in acme_sh.
>>> Multiples domains will only use one domain in certivicate.
>> multiple domain support is pretty central to the rest of the series
>> though - see my comments regarding 'plugin' and 'alias' properties in
>> node config..
> 
> I guess the most common use case is the wildcard in the cluster what 
> lets-encrypt supports with DNS-Challenge.
> 
> https://letsencrypt.org/de/docs/challenge-types/

multi-domain support is relevant in general - systems might be reachable 
via multiple FQDNs, and the certificate should be valid for all of them? 

this is not really related to wildcard certificates at all, except that 
sometimes you might want to have 'example.com' and '*.example.com' in 
one certificate..