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

[Dominik Csapak]

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?

> +	}
>       }
> +    # 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;
>   
>