[pve-devel] Allow accessing novnc console without being logged in

[Dietmar Maurer]

Hi Henry,

first, thanks for the patch. 

Please note that you need to sign a CLA if you want that we 
include your code, see:

https://pve.proxmox.com/wiki/Developer_Documentation

further comments inline.

> When using an API it's impossible to use the novnc console.
> This patch introduces a new API endpoint
> (https://proxmoxurl.com:8006/api2/json/websocket) where we can connect
> without being logged in. Authentication is done by validating the vnc
> ticket.

Is this secure enough?

> Tested on Proxmox VE 4.
> 
> From 0ca59236a4cdcc6e7479b982e8baec1466ac809d Mon Sep 17 00:00:00 2001
> From: Henry Spanka <henry at myvirtualserver.de>
> Date: Mon, 2 Nov 2015 21:45:46 +0100
> Subject: [PATCH 1/1] Allow accessing novnc console when not logged in
> 
> ---
>  /PVE/HTTPServer.pm | 59 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
> 
> diff --git a/PVE/HTTPServer.pm b/PVE/HTTPServer.pm
> index 927abc0..f23d9e4 100755
> --- a/PVE/HTTPServer.pm
> +++ b/PVE/HTTPServer.pm
> @@ -1221,6 +1221,65 @@ sub unshift_read_header {
>  		    }
>  		    $self->handle_spice_proxy_request($reqstate,
> $connect_str, $vmid, $node, $port);
>  		    return;
> +        } elsif ($path =~ /^\/api2\/json\/websocket$/) {
> +            my $upgrade = $r->header('upgrade');
> +            $upgrade = lc($upgrade) if $upgrade;
> +
> +            my $vncticket = extract_params($r, $method)->{vncticket};
> +
> +            my $vmid = extract_params($r, $method)->{vmid};
> +
> +            my $user = extract_params($r, $method)->{user};

Please try to avoid multiple calls to extract_param(). Instead, call
once and store the result.

This is the first security risk. We try to read/parse parameters
before the call is authenticated - no good.
How can we improve that? Pass VNC ticket as header?

> +
> +            my $authpath = "/vms/$vmid";
> +
> +            if (!$upgrade || ($upgrade ne 'websocket')) {
> +                $self->error($reqstate, HTTP_INTERNAL_SERVER_ERROR, "unable
> to upgrade to protocol '$upgrade'\n");
> +                return;
> +            }
> +
> +            my $wsver = $r->header('sec-websocket-version');
> +            if (!$wsver || ($wsver ne '13')) {
> +                $self->error($reqstate, HTTP_INTERNAL_SERVER_ERROR,
> "unsupported websocket-version '$wsver'\n");
> +                return;
> +            }
> +
> +            my $wsproto_str = $r->header('sec-websocket-protocol');
> +            if (!$wsproto_str) {
> +                $self->error($reqstate, HTTP_INTERNAL_SERVER_ERROR,
> "missing websocket-protocol header");
> +                return;
> +            }
> +
> +            my $wsproto;
> +
> +            foreach my $p (PVE::Tools::split_list($wsproto_str)) {
> +                $wsproto = $p if !$wsproto && $p eq 'base64';
> +                $wsproto = $p if $p eq 'binary';
> +            }
> +
> +            if (!$wsproto) {
> +                $self->error($reqstate, HTTP_INTERNAL_SERVER_ERROR,
> "unsupported websocket-protocol protocol '$wsproto_str'\n");
> +                return;
> +            }
> +
> +            my $wskey = $r->header('sec-websocket-key');
> +
> +            if (!$wskey) {
> +                $self->error($reqstate, HTTP_INTERNAL_SERVER_ERROR,
> "missing websocket-key\n");
> +                return;
> +            }

That code looks somehow familiar - I see same code handle_api2_request().
Is it possible to reuse that somehow?