From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <pve-devel-bounces@lists.proxmox.com> Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 6C8831FF173 for <inbox@lore.proxmox.com>; Mon, 10 Feb 2025 11:43:26 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 6619C84DB; Mon, 10 Feb 2025 11:43:23 +0100 (CET) Date: Mon, 10 Feb 2025 11:43:16 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= <f.gruenbichler@proxmox.com> To: Thomas Skinner <thomas@atskinner.net> References: <20241224202429.3072813-1-thomas@atskinner.net> <20241224202429.3072813-4-thomas@atskinner.net> <1737710597.ns84scjebn.astroid@yuna.none> <CALn9RMdajoXYjMOpS_vrRZk=vkTDKNMj7goMZx-u+aL8e9XDNw@mail.gmail.com> In-Reply-To: <CALn9RMdajoXYjMOpS_vrRZk=vkTDKNMj7goMZx-u+aL8e9XDNw@mail.gmail.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1739183668.sqso1o7yg0.astroid@yuna.none> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.044 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: [pve-devel] [PATCH access-control v2 1/1] fix #4411: openid: add logic for openid groups support X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com> List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe> List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/> List-Post: <mailto:pve-devel@lists.proxmox.com> List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help> List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, <mailto:pve-devel-request@lists.proxmox.com?subject=subscribe> Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com> Cc: Proxmox VE development discussion <pve-devel@lists.proxmox.com> Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com> On February 8, 2025 7:40 am, Thomas Skinner wrote: >> do we want to mangle the group names to include the OIDC-realm name, >> like we do for LDAP/AD syncing? that way it is more clear that those >> groups originated from OIDC.. downside is that you can't use a group >> shared between OIDC and other realms.. > > More on this: it looks like in LDAP/AD sync, the group is suffixed > with `-$realm`, which does meet the requirements of the existing regex > character requirements for groups. We could do the same thing with the > OIDC groups. I will add that suffix in as the option to be consistent. > > ----- > > On a similar but separate note, I think that it may be more clear of > where groups come from to have them optionally suffixed with something > like `@$realm` like is already done for users. The verify function for > groups could be adjusted to validate on a regex that makes the suffix > optional and validates it as `group_regex@realm_regex`. The downside > of this change is that it would break any existing groups or the > module would need to be adjusted to continue to support or migrate the > groups with the old suffix. An upside of this change is eliminating > inadvertent group collisions. For example: groups are not really scoped to a realm though, we only did this "soft-namespace" for the sync feature to avoid accidents (similar to the one you describe below). > Consider we have 2 realms: AD/LDAP "ad-admins" and OIDC "oidc". Realm > "ad-admins" suffixes groups automatically and realm "oidc" is > configured to not suffix. Realm "ad-admins" contains a group > "privileged" and a realm "oidc" claim sends a group > "privileged-ad-admins". > > With the existing configuration, the group "privileged" from realm > "ad-admins" becomes "privileged-ad-admins" in PVE. The user with the > group "privileged-ad-admins" in the OIDC claim is then granted access > to this group on login. This could lead to potential erroneous group > membership because the group names could overlap. yes, this is why I think OIDC groups should also always add the suffix, just like LDAP/AD. that way, any group name collision must stem from a manually created group containing the suffix (which can still happen, but is a lot less likely than without). > > If the suffix was changed to include a character that would not exist > in the group name (e.g. "@"), then it would be impossible to have this > overlap. Additionally, this suffixing option could be extended to any > other realm type that could introduce groups into PVE. I can see > scenarios where having the option to enable/disable group suffixes for > all realms would be useful. If the admin controls all the > authentication services (or at least can ensure no inadvertent > collisions would occur), the suffix is not needed. Non-suffixed group > names could also simplify ACL delegation. If the admin cannot > guarantee inadvertent collisions in group names, then suffixes that do > not include a valid group character would be necessary to prevent > collisions. > > If PVE is interested in moving towards this, I'd be happy to start > authoring a patch to support it. If nothing else, I can file a bug and > we can continue discussion there. I think this would be a very hard transition given that user.cfg contents are at the very core of PVE - and it would break valid use cases like pre-configuring groups that are part of a AD/LDAP-sync or OIDC-autocreation scope. but if other people think this is worth exploring, I am of course not objecting to further discussing it :) _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel