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

[Gabriel Goller]

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?

[0]: https://github.com/openssl/openssl/blob/14c45338e986d5827f1e944d0cffe54a7f4697ea/ssl/ssl_rsa.c#L437