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

[Thomas Lamprecht]

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