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

[Gabriel Goller]

On 29.08.2024 13:22, Wolfgang Bumiller wrote:
>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 <g.goller at 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)?)`
>>
>> 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.

Yes, this would be quite tricky to get right.

>Simply doing an `open()+close()` before calling the
>`set_certificate_chain_file()` is probably okay for now.

Will do!
Posted a v2!