Re: [pve-devel] [PATCH pve-access-control 3/4] api: implement openid API

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On June 24, 2021 10:18 am, Dietmar Maurer wrote:
> This moves compute_api_permission() into RPCEnvironment.pm.
> ---
>  src/PVE/API2/AccessControl.pm |  60 ++--------
>  src/PVE/API2/Makefile         |   3 +-
>  src/PVE/API2/OpenId.pm        | 214 ++++++++++++++++++++++++++++++++++
>  src/PVE/RPCEnvironment.pm     |  49 ++++++++
>  4 files changed, 273 insertions(+), 53 deletions(-)
>  create mode 100644 src/PVE/API2/OpenId.pm
> 
> diff --git a/src/PVE/API2/AccessControl.pm b/src/PVE/API2/AccessControl.pm
> index a77694b..6dec66c 100644
> --- a/src/PVE/API2/AccessControl.pm
> +++ b/src/PVE/API2/AccessControl.pm
> @@ -19,9 +19,9 @@ use PVE::API2::User;
>  use PVE::API2::Group;
>  use PVE::API2::Role;
>  use PVE::API2::ACL;
> +use PVE::API2::OpenId;
>  use PVE::Auth::Plugin;
>  use PVE::OTP;
> -use PVE::Tools;
>  
>  my $u2f_available = 0;
>  eval {
> @@ -56,6 +56,11 @@ __PACKAGE__->register_method ({
>      path => 'domains',
>  });
>  
> +__PACKAGE__->register_method ({
> +    subclass => "PVE::API2::OpenId",
> +    path => 'openid',
> +});
> +
>  __PACKAGE__->register_method ({
>      name => 'index',
>      path => '',
> @@ -165,55 +170,6 @@ my $create_ticket = sub {
>      };
>  };
>  
> -my $compute_api_permission = sub {
> -    my ($rpcenv, $authuser) = @_;
> -
> -    my $usercfg = $rpcenv->{user_cfg};
> -
> -    my $res = {};
> -    my $priv_re_map = {
> -	vms => qr/VM\.|Permissions\.Modify/,
> -	access => qr/(User|Group)\.|Permissions\.Modify/,
> -	storage => qr/Datastore\.|Permissions\.Modify/,
> -	nodes => qr/Sys\.|Permissions\.Modify/,
> -	sdn => qr/SDN\.|Permissions\.Modify/,
> -	dc => qr/Sys\.Audit|SDN\./,
> -    };
> -    map { $res->{$_} = {} } keys %$priv_re_map;
> -
> -    my $required_paths = ['/', '/nodes', '/access/groups', '/vms', '/storage', '/sdn'];
> -
> -    my $checked_paths = {};
> -    foreach my $path (@$required_paths, keys %{$usercfg->{acl}}) {
> -	next if $checked_paths->{$path};
> -	$checked_paths->{$path} = 1;
> -
> -	my $path_perm = $rpcenv->permissions($authuser, $path);
> -
> -	my $toplevel = ($path =~ /^\/(\w+)/) ? $1 : 'dc';
> -	if ($toplevel eq 'pool') {
> -	    foreach my $priv (keys %$path_perm) {
> -		if ($priv =~ m/^VM\./) {
> -		    $res->{vms}->{$priv} = 1;
> -		} elsif ($priv =~ m/^Datastore\./) {
> -		    $res->{storage}->{$priv} = 1;
> -		} elsif ($priv eq 'Permissions.Modify') {
> -		    $res->{storage}->{$priv} = 1;
> -		    $res->{vms}->{$priv} = 1;
> -		}
> -	    }
> -	} else {
> -	    my $priv_regex = $priv_re_map->{$toplevel} // next;
> -	    foreach my $priv (keys %$path_perm) {
> -		next if $priv !~ m/^($priv_regex)/;
> -		$res->{$toplevel}->{$priv} = 1;
> -	    }
> -	}
> -    }
> -
> -    return $res;
> -};
> -
>  __PACKAGE__->register_method ({
>      name => 'get_ticket',
>      path => 'ticket',
> @@ -314,7 +270,7 @@ __PACKAGE__->register_method ({
>  	    die PVE::Exception->new("authentication failure\n", code => 401);
>  	}
>  
> -	$res->{cap} = &$compute_api_permission($rpcenv, $username)
> +	$res->{cap} = $rpcenv->compute_api_permission($username)
>  	    if !defined($res->{NeedTFA});
>  
>  	my $clinfo = PVE::Cluster::get_clinfo();
> @@ -659,7 +615,7 @@ __PACKAGE__->register_method({
>  
>  	return {
>  	    ticket => PVE::AccessControl::assemble_ticket($authuser),
> -	    cap => &$compute_api_permission($rpcenv, $authuser),
> +	    cap => $rpcenv->compute_api_permission($authuser),
>  	}
>      }});
>  
> diff --git a/src/PVE/API2/Makefile b/src/PVE/API2/Makefile
> index 1bf8c05..4e49037 100644
> --- a/src/PVE/API2/Makefile
> +++ b/src/PVE/API2/Makefile
> @@ -5,7 +5,8 @@ API2_SOURCES= 		 	\
>  	ACL.pm		 	\
>  	Role.pm		 	\
>  	Group.pm	 	\
> -	User.pm
> +	User.pm			\
> +	OpenId.pm
>  
>  .PHONY: install
>  install:
> diff --git a/src/PVE/API2/OpenId.pm b/src/PVE/API2/OpenId.pm
> new file mode 100644
> index 0000000..db9f9eb
> --- /dev/null
> +++ b/src/PVE/API2/OpenId.pm
> @@ -0,0 +1,214 @@
> +package PVE::API2::OpenId;
> +
> +use strict;
> +use warnings;
> +
> +use PVE::Tools qw(extract_param);
> +use PVE::RS::OpenId;
> +
> +use PVE::Exception qw(raise raise_perm_exc raise_param_exc);
> +use PVE::SafeSyslog;
> +use PVE::RPCEnvironment;
> +use PVE::Cluster qw(cfs_read_file);
> +use PVE::AccessControl;
> +use PVE::JSONSchema qw(get_standard_option);
> +
> +use PVE::RESTHandler;
> +
> +use base qw(PVE::RESTHandler);
> +
> +my $openid_state_path = "/var/lib/pve-manager";
> +
> +my $lookup_openid_auth = sub {
> +    my ($realm, $redirect_url) = @_;
> +
> +    my $cfg = cfs_read_file('domains.cfg');
> +    my $ids = $cfg->{ids};
> +
> +    die "authentication domain '$realm' does not exist\n" if !$ids->{$realm};
> +
> +    my $config = $ids->{$realm};
> +    die "wrong realm type ($config->{type} != openid)" if $config->{type} ne "openid";

missing \n

> +
> +    my $openid_config = {
> +	issuer_url => $config->{'issuer-url'},
> +	client_id => $config->{'client-id'},
> +	client_key => $config->{'client-key'},
> +    };
> +
> +    my $openid = PVE::RS::OpenId->discover($openid_config, $redirect_url);

this also potentially dies and does not have the newline appended, maybe
wrap in an eval? (there are probably quite a few of those I missed in 
the endpoints below)

also, most likely an error here happens when either of the three 
parameters is wrong / not matching what the provider expects.  when 
testing I got 404 here because I entered the full issuer-url instead of 
just the https://FQDN part, maybe we could improve the error handling 
story in proxmox-openid-connect-rs by being more explicit which steps 
fail, or even detecting common pitfalls like that..

> +    return ($config, $openid);
> +};
> +
> +__PACKAGE__->register_method ({
> +    name => 'index',
> +    path => '',
> +    method => 'GET',
> +    description => "Directory index.",
> +    permissions => {
> +	user => 'all',
> +    },
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {},
> +    },
> +    returns => {
> +	type => 'array',
> +	items => {
> +	    type => "object",
> +	    properties => {
> +		subdir => { type => 'string' },
> +	    },
> +	},
> +	links => [ { rel => 'child', href => "{subdir}" } ],
> +    },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	return [
> +	    { subdir => 'auth-url' },
> +	    { subdir => 'login' },
> +	];
> +    }});
> +
> +__PACKAGE__->register_method ({
> +    name => 'auth_url',
> +    path => 'auth-url',
> +    method => 'POST',
> +    protected => 1,
> +    description => "Get the OpenId Authorization Url for the specified realm.",
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    realm => get_standard_option('realm'),
> +	    'redirect-url' => {
> +		description => "Redirection Url. The client should set this to the used server url (location.origin).",
> +		type => 'string',
> +		maxLength => 255,
> +	    },
> +	},
> +    },
> +    returns => {
> +	type => "string",
> +	description => "Redirection URL.",
> +    },
> +    permissions => { user => 'world' },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $realm = extract_param($param, 'realm');
> +	my $redirect_url = extract_param($param, 'redirect-url');
> +
> +	my ($config, $openid) = $lookup_openid_auth->($realm, $redirect_url);
> +	my $url = $openid->authorize_url($openid_state_path , $realm);
> +
> +	return $url;
> +    }});
> +
> +__PACKAGE__->register_method ({
> +    name => 'login',
> +    path => 'login',
> +    method => 'POST',
> +    protected => 1,
> +    description => " Verify OpenID authorization code and create a ticket.",
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    'state' => {
> +		description => "OpenId state.",
> +		type => 'string',
> +		maxLength => 1024,
> +            },
> +	    code => {
> +		description => "OpenId authorization code.",
> +		type => 'string',
> +		maxLength => 1024,
> +            },
> +	    'redirect-url' => {
> +		description => "Redirection Url. The client should set this to the used server url (location.origin).",
> +		type => 'string',
> +		maxLength => 255,
> +	    },
> +	},
> +    },
> +    returns => {
> +	properties => {
> +	    username => { type => 'string' },
> +	    ticket => { type => 'string' },
> +	    CSRFPreventionToken => { type => 'string' },
> +	    cap => { type => 'object' },  # computed api permissions
> +	    clustername => { type => 'string', optional => 1 },
> +	},
> +    },
> +    permissions => { user => 'world' },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $rpcenv = PVE::RPCEnvironment::get();
> +
> +	my $res;
> +	eval {
> +	    use Data::Dumper;
> +
> +	    # fixme /tmp

leftover fixme? this is now in /var/lib/pve-manager AFAICT..

> +	    my ($realm, $private_auth_state) = PVE::RS::OpenId::verify_public_auth_state(
> +		$openid_state_path, $param->{'state'});
> +
> +	    my $redirect_url = extract_param($param, 'redirect-url');
> +
> +	    my ($config, $openid) = $lookup_openid_auth->($realm, $redirect_url);
> +
> +	    my $info = $openid->verify_authorization_code($param->{code}, $private_auth_state);
> +	    my $subject = $info->{'sub'};
> +
> +	    die "missing openid claim 'sub'" if !defined($subject);

missing \n

> +
> +	    my $unique_name = $subject; # default
> +	    if (defined(my $user_attr = $config->{'user-attr'})) {
> +		if ($user_attr eq 'subject') {
> +		    $unique_name = $subject;
> +		} elsif ($user_attr eq 'username') {
> +		    my $username = $info->{'preferred_username'};
> +		    die "missing claim 'preferred_username'" if !defined($username);

same

> +		    $unique_name =  $username;
> +		} elsif ($user_attr eq 'email') {
> +		    my $email = $info->{'email'};
> +		    die "missing claim 'email'" if !defined($email);

same

> +		    $unique_name = $email;
> +		} else {
> +		    die "got unexpected value for user_attr '${user_attr}'";

same

> +		}
> +	    }
> +
> +	    my $username = "${unique_name}\@${realm}";
> +
> +	    # test if user exists and is enabled
> +	    $rpcenv->check_user_enabled($username);

missing check for user being expired

> +
> +	    my $ticket = PVE::AccessControl::assemble_ticket($username);
> +	    my $csrftoken = PVE::AccessControl::assemble_csrf_prevention_token($username);
> +	    my $cap = $rpcenv->compute_api_permission($username);
> +
> +	    $res = {
> +		ticket => $ticket,
> +		username => $username,
> +		CSRFPreventionToken => $csrftoken,
> +		cap => $cap,
> +	    };
> +
> +	    my $clinfo = PVE::Cluster::get_clinfo();
> +	    if ($clinfo->{cluster}->{name} && $rpcenv->check($username, '/', ['Sys.Audit'], 1)) {
> +		$res->{clustername} = $clinfo->{cluster}->{name};
> +	    }
> +	};
> +	if (my $err = $@) {
> +	    my $clientip = $rpcenv->get_client_ip() || '';
> +	    syslog('err', "openid authentication failure; rhost=$clientip msg=$err");
> +	    # do not return any info to prevent user enumeration attacks
> +	    die PVE::Exception->new("authentication failure\n", code => 401);
> +	}
> +
> +	PVE::Cluster::log_msg('info', 'root@pam', "successful openid auth for user '$res->{username}'");
> +
> +	return $res;
> +    }});
> diff --git a/src/PVE/RPCEnvironment.pm b/src/PVE/RPCEnvironment.pm
> index 2e5a33b..8aae094 100644
> --- a/src/PVE/RPCEnvironment.pm
> +++ b/src/PVE/RPCEnvironment.pm
> @@ -126,6 +126,55 @@ sub permissions {
>      return &$compile_acl_path($self, $user, $path);
>  }
>  
> +sub compute_api_permission {
> +    my ($self, $authuser) = @_;
> +
> +    my $usercfg = $self->{user_cfg};
> +
> +    my $res = {};
> +    my $priv_re_map = {
> +	vms => qr/VM\.|Permissions\.Modify/,
> +	access => qr/(User|Group)\.|Permissions\.Modify/,
> +	storage => qr/Datastore\.|Permissions\.Modify/,
> +	nodes => qr/Sys\.|Permissions\.Modify/,
> +	sdn => qr/SDN\.|Permissions\.Modify/,
> +	dc => qr/Sys\.Audit|SDN\./,
> +    };
> +    map { $res->{$_} = {} } keys %$priv_re_map;
> +
> +    my $required_paths = ['/', '/nodes', '/access/groups', '/vms', '/storage', '/sdn'];
> +
> +    my $checked_paths = {};
> +    foreach my $path (@$required_paths, keys %{$usercfg->{acl}}) {
> +	next if $checked_paths->{$path};
> +	$checked_paths->{$path} = 1;
> +
> +	my $path_perm = $self->permissions($authuser, $path);
> +
> +	my $toplevel = ($path =~ /^\/(\w+)/) ? $1 : 'dc';
> +	if ($toplevel eq 'pool') {
> +	    foreach my $priv (keys %$path_perm) {
> +		if ($priv =~ m/^VM\./) {
> +		    $res->{vms}->{$priv} = 1;
> +		} elsif ($priv =~ m/^Datastore\./) {
> +		    $res->{storage}->{$priv} = 1;
> +		} elsif ($priv eq 'Permissions.Modify') {
> +		    $res->{storage}->{$priv} = 1;
> +		    $res->{vms}->{$priv} = 1;
> +		}
> +	    }
> +	} else {
> +	    my $priv_regex = $priv_re_map->{$toplevel} // next;
> +	    foreach my $priv (keys %$path_perm) {
> +		next if $priv !~ m/^($priv_regex)/;
> +		$res->{$toplevel}->{$priv} = 1;
> +	    }
> +	}
> +    }
> +
> +    return $res;
> +}
> +
>  sub get_effective_permissions {
>      my ($self, $user) = @_;
>  
> -- 
> 2.30.2
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>