[pbs-devel] [PATCH v2] rest-server: check permissions on proxy.key and proxy.pem files

[Wolfgang Bumiller]

On Thu, Aug 29, 2024 at 02:10:47PM GMT, Gabriel Goller wrote:
> To avoid openssl's unhelpful error messages when the proxy.key or
> proxy.pem files have the wrong permissions, we open the files. To load
> the private key, we can simply read from the file and pass it to the
> `set_private_key` openssl function. Sadly such a function does not exist
> for loading certificate chains, so we have to open and close the file
> before calling the `set_certificate_chain_file` fn.
> 
> 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>
> ---
> 
> v2, thanks @Wolfgang:
>  - move check from proxmox-backup-server to proxmox-rest-server
>  - check permissions by opening files
> 
>  proxmox-rest-server/src/connection.rs | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/proxmox-rest-server/src/connection.rs b/proxmox-rest-server/src/connection.rs
> index fbdfe96cdfdb..f985d25f7e63 100644
> --- a/proxmox-rest-server/src/connection.rs
> +++ b/proxmox-rest-server/src/connection.rs
> @@ -12,19 +12,21 @@ use std::pin::{pin, Pin};
>  use std::sync::{Arc, Mutex};
>  use std::time::Duration;
>  
> -use anyhow::{format_err, Context as _, Error};
> +use anyhow::{format_err, Context, Error};
>  use futures::FutureExt;
>  use hyper::server::accept;
>  use openssl::ec::{EcGroup, EcKey};
>  use openssl::nid::Nid;
>  use openssl::pkey::{PKey, Private};
> -use openssl::ssl::{SslAcceptor, SslFiletype, SslMethod};
> +use openssl::ssl::{SslAcceptor, SslMethod};
>  use openssl::x509::X509;
>  use tokio::net::{TcpListener, TcpStream};
>  use tokio::sync::mpsc;
>  use tokio_openssl::SslStream;
>  use tokio_stream::wrappers::ReceiverStream;
>  
> +use proxmox_sys::fs::file_read_string;
> +
>  #[cfg(feature = "rate-limited-stream")]
>  use proxmox_http::{RateLimitedStream, ShareableRateLimit};
>  
> @@ -88,9 +90,17 @@ impl TlsAcceptorBuilder {
>                      .context("failed to set tls acceptor certificate")?;
>              }
>              Some(Tls::FilesPem(key, cert)) => {
> +                let key_content =
> +                    file_read_string(key).context("Failed to read from private key file")?;

Why not `std::fs::read()`? openssl expects bytes below, the utf-8 check
is unnecessary.
(just include the path via `.with_context(|| format!(...))`)

>                  acceptor
> -                    .set_private_key_file(key, SslFiletype::PEM)
> +                    .set_private_key(PKey::private_key_from_pem(key_content.as_bytes())?.as_ref())
>                      .context("failed to set tls acceptor private key file")?;
> +
> +                {
> +                    // Check the permissions by opening the file
> +                    let _cert_fd = std::fs::File::open(&cert)
> +                        .context(format!("Failed to open certificate at {:?}", cert))?;

use `.with_context` to avoid formatting the string when no error
happens.

> +                }
>                  acceptor
>                      .set_certificate_chain_file(cert)
>                      .context("failed to set tls acceptor certificate chain file")?;
> -- 
> 2.39.2