[pve-devel] [PATCH common] fix #2023: try out all available yubico api endpoints

[Thomas Lamprecht]

On 4/9/19 9:21 AM, Dominik Csapak wrote:
> one comment inline
> 
> On 4/9/19 9:11 AM, Thomas Lamprecht wrote:
>> While we moved to the new v2 API a bit ago we still only ever tried
>> a single hard coded endpoint, but yubico say:
>>
>>> To ensure high-availability, configure your client to
>>> simultaneously issue requests to all five addresses and accept the
>>> first successful reply.
>> -- yubico server status site [0]
>>
>> For now I ommitted the initial parallel requesting, it's a best
>> effort try, serialized.
>> That could be added on top of this later one, if desired, either by
>> just forking and starting connections in parallel, or some anyevent
>> like async request.
>>
>> But lets hope we can save this complexity completely and wait out
>> until yubico sets up a single highly-available API endpoint. Until
>> then this is a cheap "one can only win" approach.
>>
>> [0]: https://status.yubico.com/2018/11/26/deprecating-yubicloud-v1-protocol-plain-text-requests-and-old-tls-versions/
>>
>> Signed-off-by: Thomas Lamprecht <t.lamprecht at proxmox.com>
>> ---
>>
>> best (re)viewed with git's ignore all space option (-w) to ignore indentation
>> only changes
>>
>>   src/PVE/OTP.pm | 38 +++++++++++++++++++++++++++++---------
>>   1 file changed, 29 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/PVE/OTP.pm b/src/PVE/OTP.pm
>> index 019076b..f9b6654 100644
>> --- a/src/PVE/OTP.pm
>> +++ b/src/PVE/OTP.pm
>> @@ -46,7 +46,7 @@ sub yubico_compute_param_sig {
>>   }
>>     sub yubico_verify_otp {
>> -    my ($otp, $keys, $url, $api_id, $api_key, $proxy) = @_;
>> +    my ($otp, $keys, $api_url, $api_id, $api_key, $proxy) = @_;
>>         die "yubico: missing password\n" if !defined($otp);
>>       die "yubico: missing API ID\n" if !defined($api_id);
>> @@ -55,7 +55,17 @@ sub yubico_verify_otp {
>>         die "yubico: wrong OTP length\n" if (length($otp) < 32) || (length($otp) > 48);
>>   -    $url = 'https://api2.yubico.com/wsapi/2.0/verify' if !defined($url);
>> +    my $urls;
>> +    if (!defined($api_url)) {
>> +    # see https://status.yubico.com/2018/11/26/deprecating-yubicloud-v1-protocol-plain-text-requests-and-old-tls-versions/
>> +    $urls = [
>> +        map { "https://api${_}.yubico.com/wsapi/2.0/verify" } (''), (2..5)
>> +    ];
>> +    } elsif (!ref($api_url)) {
>> +    $urls = [ $api_url ];
>> +    } else {
>> +    $urls = $api_url;
>> +    }
>>         my $params = {
>>       nonce =>  Digest::SHA::hmac_sha1_hex(time(), rand()),
>> @@ -68,8 +78,6 @@ sub yubico_verify_otp {
>>         $paramstr .= "&h=$sig" if $api_key;
>>   -    my $req = HTTP::Request->new('GET' => "$url?$paramstr");
>> -
>>       my $ua = LWP::UserAgent->new(protocols_allowed => ['http', 'https'], timeout => 30);
>>         if ($proxy) {
>> @@ -78,13 +86,25 @@ sub yubico_verify_otp {
>>       $ua->env_proxy;
>>       }
>>   -    my $response = $ua->request($req);
>> -    my $code = $response->code;
>> +    my ($response, $lasterr);
>> +    for my $url (@$urls) {
>> +    my $req = HTTP::Request->new('GET' => "$url?$paramstr");
>>   -    if ($code != 200) {
>> -    my $msg = $response->message || 'unknown';
>> -    die "Invalid response from server: $code $msg\n";
>> +    $response = $ua->request($req);
>> +    my $code = $response->code;
>> +
>> +    if ($code != 200) {
>> +        $lasterr = $response->message || 'unknown';
>> +        warn "Invalid response from '$url': $code $lasterr\n";
>> +        next;
>> +    } else {
>> +        $lasterr = undef;
> i guess there is a 'last' missing here?
> else we always try all even if we already had a successful attempt ? or am i missing something?

no, you're right, it's missing..

> 
>> +    }
>>       }
>> +    # just report last error to user, don't throw > 5, possible identical, errors
>> +    # at him, they're logged above for an admin already..
>> +    die "Could not connect to any yubico OTP API server specified: $lasterr\n"
>> +    if defined($lasterr);
>>         my $raw = $response->decoded_content;
>>  
> 
>