From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 55B7D1FF16B for ; Thu, 29 Aug 2024 13:22:30 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 56BB21AB43; Thu, 29 Aug 2024 13:22:58 +0200 (CEST) Date: Thu, 29 Aug 2024 13:22:23 +0200 From: Wolfgang Bumiller To: Gabriel Goller Message-ID: References: <20240823091215.124453-1-g.goller@proxmox.com> <20240829103122.ainqdg2k3ewqfd5y@luna.proxmox.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240829103122.ainqdg2k3ewqfd5y@luna.proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.087 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 T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pbs-devel] [PATCH proxmox-backup] proxy: check permissions on proxy.key and proxy.pem files 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: , Reply-To: Proxmox Backup Server development discussion Cc: pbs-devel@lists.proxmox.com Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" On Thu, Aug 29, 2024 at 12:31:22PM GMT, Gabriel Goller wrote: > On 27.08.2024 11:37, Wolfgang Bumiller wrote: > > NAK > > > > On Fri, Aug 23, 2024 at 11:12:15AM GMT, Gabriel Goller wrote: > > > Check the owner and permission of the proxy.key and proxy.pem files. > > > This avoids openssl's unhelpful error message and prints a nicer one. > > > > > > Motivation: https://forum.proxmox.com/threads/proxmox-backup-tailscale-proxmox-backup-proxy-service-wont-boot.153204 > > > > > > Signed-off-by: Gabriel Goller > > > --- > > > > > > Note: not sure about the correct permissions, we currently default to > > > 640, but maybe a minimum of 400 is enough? > > > > > > src/bin/proxmox-backup-proxy.rs | 27 +++++++++++++++++++++++++++ > > > 1 file changed, 27 insertions(+) > > > > > > diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs > > > index 041f3aff999c..544196b8bc5d 100644 > > > --- a/src/bin/proxmox-backup-proxy.rs > > > +++ b/src/bin/proxmox-backup-proxy.rs > > > @@ -367,6 +367,30 @@ async fn run() -> Result<(), Error> { > > > Ok(()) > > > } > > > > > > +/// Check permissions and owner of passed path. > > > +fn check_permissions>(path: T, file_mode: u32) -> Result<(), Error> { > > > + match nix::sys::stat::stat(path.as_ref()) { > > > + Ok(stat) => { > > > + if stat.st_uid != u32::from(pbs_config::backup_user()?.uid) > > > + || stat.st_gid != u32::from(pbs_config::backup_group()?.gid) > > > + || stat.st_mode & 0o770 < file_mode > > > > If you want to test whether you can open a file, you should either just > > `open(2)` it, or, if you really want to avoid it, use `access(2)`. > > You do not ever want to attempt to try to perform the kernel's > > permission checks yourself. There could be ACLs, AppArmor profiles, ... > > and while we can say that, for now, this is not supposed to be the case, > > it's bad practice in general. > > > > Also note that this only covers the case at a point in time where the > > certificate isn't actually loaded, and won't help with changes to the > > permissions while a daemon is already running. > > > > A better approach to handle this specific case would be to adapt > > `proxmox-rest-server`'s handling of `Tls::PemFiles` so that instead of > > using `openssl`'s ".set_private_key_file()` convenience methods, it > > loads the files, and handles `EPERM`/`ENOENT`/... with useful error > > messags, and then uses > > `acceptor.set_private_key(PKey::private_key_from_pem(data)?)` > > I agree. > We can use the function you mentioned for the private key (and it > works!), but not for the certificate. AFAICT (and I know nothing about > openssl) there is no use_certificate_chain() function, so the > certificate chain can only be loaded using a path [0]. This seems kinda > weird, as all the other functions have path and file-content flavors, > but this one hasn't. > > Nevertheless we could just forget about the certificate or we could do > a open() and close() to check the permissions? That's unfortunate. However, I think it's usually only the key that gets strict permissions, not the certificate, since it's meant to be public anyway. In theory, I think `.set_certificate()` + `.add_extra_chain_cert()` should work iterating through the certificates from `X509::stack_from_pem()`, but this would need careful testing to make sure the full chain is properly visible. Simply doing an `open()+close()` before calling the `set_certificate_chain_file()` is probably okay for now. _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel