[pve-devel] applied: [PATCH v2 common] ticket: raise UNAUTHORIZED not FORBIDDEN in verify subs

[Wolfgang Bumiller]

applied

On Fri, Dec 15, 2017 at 06:41:49AM +0100, Thomas Lamprecht wrote:
> In the ticket and CSRF prevention token verification methods we used
> a raise_perm exception to tell our caller about a failure of such a
> verification. raise_perm uses HTTP_FORBIDDEN (403) as code.
> 
> Earlier, all such exceptions or die's where caught when the anyevent
> http server called the auth_handler method and transformed to
> HTTP_UNAUTHORIZED (401).
> 
> With commit d8327719e353198a1dffad88c246fee065054a6b from
> pve-http-server we gained the ability to tell a client about a server
> internal 5XX error, so that clients do not get wrongly logged out if
> we have a internal error.
> This resulted also in the effect that the exceptions of the
> verify_rsa_ticket and verify_csrf_prevention_token sub methods where
> passed to the client.
> 
> If an old, now invalid, ticket was sent to the server a client got
> 403 (FORBIDDEN) instead of the 401 (UNAUTHORIZED) - which he was used
> to, and thus meant that he did some wrong doing, instead of knowing
> that he just needs to login.
> 
> As we are not yet logged in here, and thus cannot possibly know if
> the call is forbidden or not, HTTP_FORBIDDEN seems the wrong code.
> Change it to HTTP_UNAUTHORIZED, which restores it to the code we told
> API clients since ever and is the correct one here.
> 
> Also RFC 2068 section 10.4.4 [1] defines that for the afformentioned
> verify methods FORBIDDEN was not really correct:
> 
>  > 403 Forbidden
>  >
>  >    The server understood the request, but is refusing to fulfill it.
>  >    Authorization will not help and the request SHOULD NOT be
>  >    repeated. [...]
> 
> With a invalid ticket or CSRF prevention token we have a
> authorization problem for the current call, not a permission problem
> (we may have, but we can't tell yet).
> 
> [1] https://tools.ietf.org/html/rfc2068#section-10.4.4
> 
> Signed-off-by: Thomas Lamprecht <t.lamprecht at proxmox.com>
> ---
> 
> changes v1 -> v2:
> * fix forgotten change to import raise not raise_perm_exc now, was
>   unseen as it was imported by our users (AnyEvent) and thus I did
>   not saw an error...
> 
>  src/PVE/Ticket.pm | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/src/PVE/Ticket.pm b/src/PVE/Ticket.pm
> index 68ea285..e9f8e3f 100644
> --- a/src/PVE/Ticket.pm
> +++ b/src/PVE/Ticket.pm
> @@ -9,10 +9,12 @@ use MIME::Base64;
>  use Digest::SHA;
>  use Time::HiRes qw(gettimeofday);
>  
> -use PVE::Exception qw(raise_perm_exc);
> +use PVE::Exception qw(raise);
>  
>  Crypt::OpenSSL::RSA->import_random_seed();
>  
> +use constant HTTP_UNAUTHORIZED => 401;
> +
>  sub assemble_csrf_prevention_token {
>      my ($secret, $username) = @_;
>  
> @@ -38,7 +40,8 @@ sub verify_csrf_prevention_token {
>  	    ($age < $max_age);
>      }
>  
> -    raise_perm_exc("Permission denied - invalid csrf token") if !$noerr;
> +    raise("Permission denied - invalid csrf token\n", code => HTTP_UNAUTHORIZED)
> +	if !$noerr;
>  
>      return undef;
>  }
> @@ -90,7 +93,8 @@ sub verify_rsa_ticket {
>  	}
>      }
>  
> -    raise_perm_exc("permission denied - invalid $prefix ticket") if !$noerr;
> +    raise("permission denied - invalid $prefix ticket\n", code => HTTP_UNAUTHORIZED)
> +	if !$noerr;
>  
>      return undef;
>  }
> -- 
> 2.11.0