From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 70CE4B9D51 for ; Tue, 12 Dec 2023 13:19:44 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 5A43F3476C for ; Tue, 12 Dec 2023 13:19:44 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Tue, 12 Dec 2023 13:19:43 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 99CD7469BE for ; Tue, 12 Dec 2023 13:19:43 +0100 (CET) Date: Tue, 12 Dec 2023 13:19:42 +0100 From: Christoph Heiss To: Fabian =?utf-8?Q?Gr=C3=BCnbichler?= Cc: Proxmox Backup Server development discussion Message-ID: References: <20230816144746.1265108-1-c.heiss@proxmox.com> <20230816144746.1265108-8-c.heiss@proxmox.com> <1701158734.9lqyuqtc22.astroid@yuna.none> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1701158734.9lqyuqtc22.astroid@yuna.none> X-SPAM-LEVEL: Spam detection results: 0 AWL -0.147 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 POISEN_SPAM_PILL 0.1 Meta: its spam POISEN_SPAM_PILL_1 0.1 random spam to be learned in bayes POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pbs-devel] [PATCH proxmox-backup v2 07/15] api: access: add routes for managing AD realms X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 12 Dec 2023 12:19:44 -0000 On Tue, Nov 28, 2023 at 09:23:51AM +0100, Fabian Grünbichler wrote: > [..] > On August 16, 2023 4:47 pm, Christoph Heiss wrote: > > +#[api( > > + input: { > > + properties: {}, > > + }, > > + returns: { > > + description: "List of configured AD realms.", > > + type: Array, > > + items: { type: AdRealmConfig }, > > + }, > > + access: { > > + permission: &Permission::Privilege(&["access", "domains"], PRIV_REALM_ALLOCATE, false), > > this one here > > [..] > > + > > +#[api( > > + input: { > > + properties: { > > + realm: { > > + schema: REALM_ID_SCHEMA, > > + }, > > + }, > > + }, > > + returns: { type: AdRealmConfig }, > > + access: { > > + permission: &Permission::Privilege(&["access", "domains"], PRIV_SYS_AUDIT, false), > > and this one here don't really agree - copied over from LDAP ;) Well, the OpenID realm also uses PRIV_SYS_AUDIT for reading. So for the sake of consistency, AD should use the same. > > also, maybe this one here should check on /access/domains/{realm} > (although that might be postponed to do it in sync with the other > endpoint(s), but it would be more in line with how we handle entity ACLs > in general). Good catch, seems sensible. For the existing LDAP/OpenID endpoints changing it would probably constitute a breaking change? Changing both things above for the existing LDAP/OpenID endpoints would probably constitute a breaking change? > [..] > > +const ITEM_ROUTER: Router = Router::new() > > + .get(&API_METHOD_READ_AD_REALM) > > + .put(&API_METHOD_UPDATE_AD_REALM) > > + .delete(&super::ldap::API_METHOD_DELETE_LDAP_REALM); > > this seems a bit weird - as in - why doesn't that endpoint check that > it's actually being passed an LDAP realm? The LDAP endpoint for this is actually generic enough that it works for AD too. IOW, it just deletes the realm with that particular name from the config (since names are unique across all realm types anyway) and the LDAP bind password, if one exists. The infrastructure for the latter is also reused by the AD realm. So in favor of not unnecessarily duplicating code, I chose to simply reuse it as-is. At the end of the day, AD is just LDAP with some Microsoft idiosyncrasies, thus I tried to reuse as much code/infrastructure as possible.