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 A761B726FF for ; Wed, 26 May 2021 08:22:08 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 9ECF9BD7B for ; Wed, 26 May 2021 08:22:08 +0200 (CEST) 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) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id B4122BD6D for ; Wed, 26 May 2021 08:22:07 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 8798E46655; Wed, 26 May 2021 08:22:07 +0200 (CEST) Message-ID: Date: Wed, 26 May 2021 08:22:06 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:89.0) Gecko/20100101 Thunderbird/89.0 Content-Language: en-US To: wb , Proxmox VE development discussion References: <02c8e0ce3079939415c742edf16c0966@mwinf5d78.me-wanadoo.net> From: Thomas Lamprecht In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.661 Adjusted score from AWL reputation of From: address CTE_8BIT_MISMATCH 0.837 Header says 7bits but body disagrees KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment KAM_NUMSUBJECT 0.5 Subject ends in numbers excluding current years NICE_REPLY_A -0.001 Looks like a legit reply (A) 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] =?utf-8?b?UkXCoDogIFJFwqA6IHB2ZS1kZXZlbCBEaWdlc3Qs?= =?utf-8?q?_Vol_132=2C_Issue_53?= X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 26 May 2021 06:22:08 -0000 Hi, On 25.05.21 22:50, wb wrote: > Sorry, the first mail went a little too fast > >> running does not mean working... > Ok, but starting from an installation, I doubt :). > But you already started to patch around, so we just cannot know for sure - was just mentioning that to cover more possibilities :-) > systemctl status pve-cluster > > > > touch /etc/pve/foo > findmnt /etc/pve > Look OK, but please paste such things inline the next time, I and most others here on this list find pictures of plain text rather needlessly hard to deal with. > > I finally found the problem that comes from the read permission on the /etc/pve/priv/authkey.key file, probably because www-data is reading instead of root. > I mean yes, it still is a filesystem so read/write need to adhere to the standard POSIX file permissions. Quoting from our documentation: > All files and directories are owned by user root and have group www-data. > Only root has write permissions, but group www-data can read most files. > Files below the following paths:> > /etc/pve/priv/ > /etc/pve/nodes/${NAME}/priv/ > > are only accessible by root. -- https://pve.proxmox.com/pve-docs/chapter-pmxcfs.html#_file_access_rights If a API method needs to access the `priv` path it needs to set the `protected => 1` "flag" in the API-method schema definition, that way it will proxied to the local-only API daemon running as root. > By moving it to /etc/pve/authkey.key, I finally manage to create the ticket and thus, to do SSO. > An analysis of the problem in my code would be welcome. > > There will still be 2 points to be clean (not counting the anomaly due to the right in reading) : > ➢ First point, create links in the login page instead of putting saml authentication realm in the list. > ➢ Check the saml request id properly in the authentication part of the SAML class, (this is really not the case) > > Thank you in advance, > > Attached is the patch. This is a combined patch over multiple repositories, further having it as attachment makes replying (reviewing) hard to do on the list. Please check out the following resources: * Developer docs, especially the CLA, and preparing/sending patches section: https://pve.proxmox.com/wiki/Developer_Documentation * Our coding style guides, didn't looked to close to actually see violations so rather mentioning it for completeness sake: - https://pve.proxmox.com/wiki/Perl_Style_Guide - https://pve.proxmox.com/wiki/Javascript_Style_Guide Note, we're well aware that not all of the existing code holds up to those standards (yet), that's still no real excuse for new code to adapt to it, as much as the surrounding code style allows. Just two short comments regarding the patch, as I did not checked that out in full: 1. I'm not sure why you need to duplicate API calls, especially the `post_saml` one. 2. It also seems you missed including the diff for the new `PVE::Auth::SAML` module. If you want a real review I'd suggest to send out what you have now as real patche series via `git send-email`, ideally adding some reasoning for why things are done that way - making review much easier - thanks! cheers, Thomas