[pve-devel] [common 4/9] refactor extract_callenge for code reuse.

[Fabian Grünbichler]

On October 21, 2019 12:12 pm, Wolfgang Link wrote:
> comment inline
> 
> On 10/18/19 11:25 AM, Fabian Grünbichler wrote:
>> On October 14, 2019 1:08 pm, Wolfgang Link wrote:
>>> ---
>>>   src/PVE/ACME.pm            | 16 ++++++++++++++++
>>>   src/PVE/ACME/StandAlone.pm |  9 +--------
>>>   2 files changed, 17 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/src/PVE/ACME.pm b/src/PVE/ACME.pm
>>> index d6b6e99..173af69 100644
>>> --- a/src/PVE/ACME.pm
>>> +++ b/src/PVE/ACME.pm
>>> @@ -76,6 +76,22 @@ sub fromjs($) {
>>>       return from_json($_[0]);
>>>   }
>>>   
>>> +sub extract_challenge ($$) {
>>> +    my ($challenges, $c_type) = @_;
>>> +
>>> +    die "no challenges defined\n" if !$challenges;
>>> +    die "no challenge type is defined \n" if !$c_type;
>>> +
>>> +    my $tmp_challenges = [ grep {$_->{type} eq $c_type} @$challenges ];
>>> +    die "no $c_type challenge defined in authorization\n"
>>> +	if ! scalar $tmp_challenges;
>>> +
>>> +    my $challenge = $tmp_challenges->[0];
>>> +
>>> +    die "no token found in $c_type challenge\n" if !$challenge->{token};
>> strictly speaking, not all challenges must require a token. http-01 and
>> dns-01 do though ;)
> Would it be better to do an extra check if http-01 and dns-01 the token 
> required?

I think it's okay for now, but maybe add a comment like

# only dns-01 and http-01 supported for now
# both require a challenge token

so that we can re-visit this if we add new challenge types or open this 
up for third-party plugins in the future.

if you want you can also extend the condition to check for one of those 
two types, but currently that should always evaluate to true ;)

>>> +    return $challenge;
>>> +}
>>> +
>>>   sub validating_url($$$$) {
>>>       my ($acme, $auth, $auth_url, $node_config) = @_;
>>>   
>>> diff --git a/src/PVE/ACME/StandAlone.pm b/src/PVE/ACME/StandAlone.pm
>>> index 965fb32..7910bfd 100644
>>> --- a/src/PVE/ACME/StandAlone.pm
>>> +++ b/src/PVE/ACME/StandAlone.pm
>>> @@ -49,14 +49,7 @@ sub validating_url {
>>>   sub setup {
>>>       my ($class, $acme, $authorization) = @_;
>>>   
>>> -    my $challenges = $authorization->{challenges};
>>> -    die "no challenges defined in authorization\n" if !$challenges;
>>> -
>>> -    my $http_challenges = [ grep {$_->{type} eq 'http-01'} @$challenges ];
>>> -    die "no http-01 challenge defined in authorization\n"
>>> -	if ! scalar $http_challenges;
>>> -
>>> -    my $http_challenge = $http_challenges->[0];
>>> +    my $http_challenge = extract_challenge($authorization->{challenges}, "http-01");
>>>   
>>>       die "no token found in http-01 challenge\n" if !$http_challenge->{token};
>> this line should no longer be needed?
>>
>>>   
>>> -- 
>>> 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
>>
>