From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pmg-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id 3BFCD1FF16F
for <inbox@lore.proxmox.com>; Thu, 27 Feb 2025 11:01:53 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 24A5A2EA54;
Thu, 27 Feb 2025 11:01:52 +0100 (CET)
Date: Thu, 27 Feb 2025 11:01:17 +0100
From: Stoiko Ivanov <s.ivanov@proxmox.com>
To: Thomas Lamprecht <t.lamprecht@proxmox.com>
Message-ID: <20250227110117.41c6b97b@rosa.proxmox.com>
In-Reply-To: <20250227095152.281531-1-t.lamprecht@proxmox.com>
References: <20250227095152.281531-1-t.lamprecht@proxmox.com>
X-Mailer: Claws Mail 4.1.1 (GTK 3.24.38; x86_64-pc-linux-gnu)
MIME-Version: 1.0
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.064 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
Subject: Re: [pmg-devel] applied: [PATCH api] oidc realm: add more flexiple
access-role assignment
X-BeenThere: pmg-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Mail Gateway development discussion
<pmg-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pmg-devel>,
<mailto:pmg-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pmg-devel/>
List-Post: <mailto:pmg-devel@lists.proxmox.com>
List-Help: <mailto:pmg-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel>,
<mailto:pmg-devel-request@lists.proxmox.com?subject=subscribe>
Cc: pmg-devel@lists.proxmox.com
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pmg-devel-bounces@lists.proxmox.com
Sender: "pmg-devel" <pmg-devel-bounces@lists.proxmox.com>
Thanks! from a quick look - it seems sensible, and the naming is in order.
one typo and 2 minor nits inline:
On Thu, 27 Feb 2025 10:51:52 +0100
Thomas Lamprecht <t.lamprecht@proxmox.com> wrote:
> deprecate the fixed role assignment property and replace it with one
> that allows to select the source of the role, for now allow static,
> which is equivalent with what we had, and also to query the role from
> a role claim, so that the OIDC provider can return the correct role
> inside the returned user information.
>
> Tested the dynamic role assignment with keycload by setting up an
> 'User Attribute' Mapper for a client and added added the respective
> user attribute to some test user.
>
> Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
> ---
>
> It's not much code and most is schema and parsing, the implementation
> itself is pretty straigt forward, basically the only questionable thing
> is naming of properties.
>
> src/PMG/API2/OIDC.pm | 26 ++++++++++++++++++++-
> src/PMG/Auth/OIDC.pm | 54 +++++++++++++++++++++++++++++++++++++++++---
> 2 files changed, 76 insertions(+), 4 deletions(-)
>
> diff --git a/src/PMG/API2/OIDC.pm b/src/PMG/API2/OIDC.pm
> index 92ff88d..20acc41 100644
> --- a/src/PMG/API2/OIDC.pm
> +++ b/src/PMG/API2/OIDC.pm
> @@ -13,6 +13,7 @@ use PVE::JSONSchema qw(get_standard_option);
> use PVE::RESTHandler;
>
> use PMG::AccessControl;
> +use PMG::Auth::OIDC;
> use PMG::Auth::Plugin;
> use PMG::RESTEnvironment;
>
> @@ -204,7 +205,30 @@ __PACKAGE__->register_method ({
> if (defined(my $family_name = $info->{'family_name'})) {
> $entry->{lastname} = $family_name;
> }
> - $entry->{role} = $config->{'autocreate-role'} // 'audit';
> +
> + # NOTE: 'autocreate-role' is deprecated and has less priority than the more
> + # flexible 'autocreate-role-assignment'
> + $entry->{role} = $config->{'autocreate-role'} // 'audit'; # default
> + if (my $role_assignment_raw = $config->{'autocreate-role-assignment'}) {
> + my $role_assignment =
> + PMG::Auth::OIDC::parse_autocreate_role_assignment($role_assignment_raw);
> +
> + if ($role_assignment->{source} eq 'fixed') {
> + $entry->{role} = $role_assignment->{'fixed-role'};
> + } elsif ($role_assignment->{source} eq 'from-claim') {
> + my $role_attr = $role_assignment->{'role-claim'};
> + if (my $role = $info->{$role_attr}) {
> + $role = lc($role); # normalize to lower-case
> + die "required '$role_attr' role-claim attribute not found, cannot autocreate user\n"
> + if $role !~ /^(?:admin|qmanager|audit|helpdesk)$/;
> + $entry->{role} = $role;
> + } else {
> + die "required '$role_attr' role-claim attribute not found, cannot autocreate user\n";
> + }
> + } else {
> + die "unkown role assignment source '$role_assignment->{source}' - implement me";
> + }
> + }
> $entry->{userid} = $username;
> $entry->{username} = $unique_name;
> $entry->{realm} = $realm;
> diff --git a/src/PMG/Auth/OIDC.pm b/src/PMG/Auth/OIDC.pm
> index 4129d47..9082c75 100755
> --- a/src/PMG/Auth/OIDC.pm
> +++ b/src/PMG/Auth/OIDC.pm
> @@ -4,6 +4,8 @@ use strict;
> use warnings;
>
> use PVE::Tools;
> +use PVE::JSONSchema qw(parse_property_string);
> +
> use PMG::Auth::Plugin;
>
> use base qw(PMG::Auth::Plugin);
> @@ -12,6 +14,44 @@ sub type {
> return 'oidc';
> }
>
> +my $autocreate_role_assignment_format = {
> + source => {
> + type => 'string',
> + enum => ['fixed', 'from-claim'],
> + default => 'fixed',
> + description => "How the access role for a newly auto-created user should be selected.",
> + },
> + 'fixed-role' => {
> + type => 'string',
> + enum => ['admin', 'qmanager', 'audit', 'helpdesk'],
> + default => 'audit',
> + optional => 1,
> + description => "The fixed role that should be assigned to auto-created users.",
> + },
> + 'role-claim' => {
> + description => "OIDC claim used to assign the unique username.",
> + type => 'string',
> + default => 'role',
> + optional => 1,
> + pattern => qr/^[a-zA-Z0-9._:-]+$/,
> + },
> +};
> +
> +
> +sub parse_autocreate_role_assignment {
> + my ($raw) = @_;
> + return undef if !$raw or !length($raw);
> +
> + my $role_assignment = parse_property_string($autocreate_role_assignment_format, $raw);
> + $role_assignment->{'fixed-role'} = 'audit'
> + if $role_assignment->{'source'} eq 'fixed' && !defined($role_assignment->{'fixed-role'});
> +
> + $role_assignment->{'role-claim'} = 'role'
> + if $role_assignment->{'source'} eq 'from-clain' && !defined($role_assignment->{'role-claim'});
`s/from-clain/from-claim/`
> +
> + return $role_assignment;
> +}
> +
> sub properties {
> return {
> 'issuer-url' => {
> @@ -39,13 +79,20 @@ sub properties {
> type => 'boolean',
> default => 0,
> },
> - 'autocreate-role' => {
> - description => "Automatically create users with a specific role.",
> + 'autocreate-role' => { # NOTE: depreacated since the beginning, just here for compat
nit: `s/depreacated/deprecated/` (had to google myself, as I always get
that wrong)
> + description => "Automatically create users with a specific role."
> + ." NOTE: Depreacated, favor 'autocreate-role-assignment'",
nit: deprecated
> type => 'string',
> enum => ['admin', 'qmanager', 'audit', 'helpdesk'],
> default => 'audit',
> optional => 1,
> },
> + 'autocreate-role-assignment' => {
> + description => "Defines which role should be assigned to auto-created users.",
> + type => 'string', format => $autocreate_role_assignment_format,
> + default => 'source=fixed,fixed-role=auditor',
> + optional => 1,
> + },
> 'username-claim' => {
> description => "OpenID Connect claim used to generate the unique username.",
> type => 'string',
> @@ -84,7 +131,8 @@ sub options {
> 'client-id' => {},
> 'client-key' => { optional => 1 },
> autocreate => { optional => 1 },
> - 'autocreate-role' => { optional => 1 },
> + 'autocreate-role' => { optional => 1 }, # NOTE: depreacated in favor of 'autocreate-role-assignment'
nit: deprecated
> + 'autocreate-role-assignment' => { optional => 1 },
> 'username-claim' => { optional => 1, fixed => 1 },
> prompt => { optional => 1 },
> scopes => { optional => 1 },
_______________________________________________
pmg-devel mailing list
pmg-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel