Re: [pve-devel] [PATCH pve-access-control] fix #5076: Added the "aud" field to the configuration and API schemas

[Dominik Csapak <d.csapak@proxmox.com>]

On 2/6/25 13:20, Alexander Abraham wrote:
> Added a field for OpenID audiences to the JSON schema
> and retrieved the data for the audiences the user
> configured on the frontend.

Hi,

same comment as for the last patch:

please don't write the 'what'/'how' here, but rather the 'why'
e.g. 'We want to let users specify the audiences to verify [...]'

some comments inline

> 
> Signed-off-by: Alexander Abraham <a.abraham@proxmox.com>
> ---
>   src/PVE/API2/OpenId.pm | 5 ++++-
>   src/PVE/Auth/OpenId.pm | 6 ++++++
>   2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/API2/OpenId.pm b/src/PVE/API2/OpenId.pm
> index 77410e6..ad92fcb 100644
> --- a/src/PVE/API2/OpenId.pm
> +++ b/src/PVE/API2/OpenId.pm
> @@ -45,6 +45,10 @@ my $lookup_openid_auth = sub {
>   	$openid_config->{acr_values} = [ PVE::Tools::split_list($acr) ];
>       }
>   
> +	if (defined(my $aud = $config->{'aud'})) {
> +	$openid_config->{aud} = [ PVE::Tools::split_list($aud) ];
> +    }
> +

you do a split list here

>       my $openid = PVE::RS::OpenId->discover($openid_config, $redirect_url);
>       return ($config, $openid);
>   };
> @@ -169,7 +173,6 @@ __PACKAGE__->register_method ({
>   	    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'};
>   
> diff --git a/src/PVE/Auth/OpenId.pm b/src/PVE/Auth/OpenId.pm
> index c8e4db9..a7af1bc 100755
> --- a/src/PVE/Auth/OpenId.pm
> +++ b/src/PVE/Auth/OpenId.pm
> @@ -63,6 +63,11 @@ sub properties {
>   	    pattern => '^[^\x00-\x1F\x7F <>#"]*$', # Prohibit characters not allowed in URI RFC 2396.
>   	    optional => 1,
>   	},
> +	'aud' => {
> +	    description => "Specifies the Authentication claims neccessary for checking the privileges the requesting user has.",
> +	    type => 'string',
> +	    optional => 1,
> +	},

but here you don't mention that it's actually a list

also there is no limit on what users can enter, normally we are rather strict with that
(e.g. just allow [a-zA-Z0-9_+.] etc.)

it's better to be strict first since we can always loosen the requirements later.
when we accept everything it's much harder to get more strict later

also, i don't think 'aud' is the best way to name this field. what about 'audiences' ?

>      };
>   }
>   
> @@ -76,6 +81,7 @@ sub options {
>   	prompt => { optional => 1 },
>   	scopes => { optional => 1 },
>   	"acr-values" => { optional => 1 },
> +	"aud" => { optional => 1 },
>   	default => { optional => 1 },
>   	comment => { optional => 1 },
>       };



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel