[pve-devel] [common 1/9] Add dynamic plugin lookup for ACME

[Fabian Grünbichler]

On October 21, 2019 12:12 pm, Wolfgang Link wrote:
> comment inline
> 
> On 10/18/19 11:22 AM, Fabian Grünbichler wrote:
>> note: the comment here is not just for this patch, but also references
>> stuff that comes in later patches..
>>
>> On October 14, 2019 1:08 pm, Wolfgang Link wrote:
>>> The dynamic approach reduces failure if new plugins will included.
>>> ---
>>>   src/PVE/ACME.pm            | 4 ++++
>>>   src/PVE/ACME/Challenge.pm  | 8 ++++++++
>>>   src/PVE/ACME/StandAlone.pm | 4 ++++
>>>   3 files changed, 16 insertions(+)
>>>
>>> diff --git a/src/PVE/ACME.pm b/src/PVE/ACME.pm
>>> index 38a14a5..da4cbcc 100644
>>> --- a/src/PVE/ACME.pm
>>> +++ b/src/PVE/ACME.pm
>>> @@ -17,6 +17,9 @@ use LWP::UserAgent;
>>>   
>>>   use Crypt::OpenSSL::RSA;
>>>   
>>> +use PVE::ACME::Challenge;
>>> +use PVE::ACME::StandAlone;
>>> +
>>>   use PVE::Certificate;
>>>   use PVE::Tools qw(
>>>   file_set_contents
>>> @@ -24,6 +27,7 @@ file_get_contents
>>>   );
>>>   
>>>   Crypt::OpenSSL::RSA->import_random_seed();
>>> +PVE::ACME::StandAlone->register();
>>>   
>>>   my $LETSENCRYPT_STAGING = 'https://acme-staging-v02.api.letsencrypt.org/directory';
>>>   
>>> diff --git a/src/PVE/ACME/Challenge.pm b/src/PVE/ACME/Challenge.pm
>>> index 40d32b6..786666c 100644
>>> --- a/src/PVE/ACME/Challenge.pm
>>> +++ b/src/PVE/ACME/Challenge.pm
>>> @@ -3,6 +3,14 @@ package PVE::ACME::Challenge;
>>>   use strict;
>>>   use warnings;
>>>   
>>> +use base qw(PVE::SectionConfig);
>> this would be the only SectionConfig that is not actually a config..
>>
>> after some off-list discussion with Thomas, how about the following:
>>
>> move PVE::ACME::Challenge and PVE::ACME::StandAlone to pve-manager, or a
>> new pve-acme package (the latter could be combined with PVE::ACME, and
>> some parts of what is currently in pve-manager, and your new
>> package/repo)
>>
>> make the challenge plugins a real SectionConfig:
>>
>> standalone: http80
>>    port 80
>>
>> dns: ovh1
>>    api ovh
>>    data encode_text("KEY1=foobar\nKEY2=foobaz")
>>    nodes node1
>>
>> dns: ovh2
>>    api ovh
>>    data encode_text("KEY1=barfoo\nKEY2=foobaz")
>>    nodes node2
>>
>> for PVE, store that section config file in
>>
>> /etc/pve/priv/acme/plugins.cfg
> 
> I agree.
> 
> It the ovh1/ovh2 the name of the definition like in storage.cfg?

yes. the string before the colon is the section type/plugin, the string 
after is the ID of that specific section/plugin instance, just like in 
storage.cfg

> Nodes should be a node list or '*' to indicate to use wildcards certificate.

I'd skip wildcard certificates for now (unless you explicitly want to 
include them in your v1?) and just use the same definition like for 
storages: either a list of nodes, or all nodes if absent.

for wild-card certificates we'd need more changes anyway:
- not stored under /etc/pve/local
- which nodes should use it (-> flag in node config?)
- which node(s) should renew it
- the code to order/renew needs to support it
- ... ?

>> (moving the definition together with the rest of general ACME code to a
>> new package would allow PMG to re-use most of it with just a different
>> high-level client and file location)
>>
>> the GUI could have a mapping of the most important plugins and their
>> config KEYs, with a fallback to just "edit whole data as textfield" or
>> "edit whole data as key-value pairs". this would still allow adding your
>> own acme.sh dnsapi compatible plugins, and would avoid having to import
>> the full key-value schema into our section config.
> We could get the plugins dynamically on call.
> The keys are not extractable because some plugins do not notate them.

my (or actually mostly Thomas' ;)) suggestion would be the following:

for the most important plugins, ship their known keys + description + 
validation in pve-manager (just for the GUI), so that we can display 
proper fields for those keys in addition to a general free-form key -> 
value entry with arbitrary values. this is just a usability improvement.

the API just takes a single multi-line string and does not care about 
the format at all. for storing/retrieving the keys and their values, we 
don't need to know anything about their count, names, schema, ... since 
we just pass them along to the acme.sh plugin anyway..

>> the node config's acme key would then just reference the concrete plugin
>> instance (but see later patches for another issue regarding this), which
>> would be available cluster-wide, but also allow different plugin
>> instances for each node..
>>
>>> +
>>> +my $defaultData = {};
>>> +
>>> +sub private {
>>> +    return $defaultData;
>>> +}
>>> +
>>>   sub supported_challenge_types {
>>>       return {};
>>>   }
>>> diff --git a/src/PVE/ACME/StandAlone.pm b/src/PVE/ACME/StandAlone.pm
>>> index f48d638..3766862 100644
>>> --- a/src/PVE/ACME/StandAlone.pm
>>> +++ b/src/PVE/ACME/StandAlone.pm
>>> @@ -8,6 +8,10 @@ use HTTP::Response;
>>>   
>>>   use base qw(PVE::ACME::Challenge);
>>>   
>>> +sub type {
>>> +    return 'standalone';
>>> +}
>>> +
>>>   sub supported_challenge_types {
>>>       return { 'http-01' => 1 };
>>>   }
>>> -- 
>>> 2.20.1
>>>
>>>
>>> _______________________________________________
>>> pve-devel mailing list
>>> pve-devel at pve.proxmox.com
>>> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>>
>>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel at pve.proxmox.com
>> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>