Re: [pbs-devel] [PATCH proxmox-backup] proxy: check permissions on proxy.key and proxy.pem files

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

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 <g.goller@proxmox.com>
> ---
> 
> 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<T: AsRef<Path>>(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)?)`


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel