public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pbs-devel] [PATCH v2] rest-server: check permissions on proxy.key and proxy.pem files
@ 2024-08-29 12:10 Gabriel Goller
  2024-08-29 12:21 ` Wolfgang Bumiller
  0 siblings, 1 reply; 3+ messages in thread
From: Gabriel Goller @ 2024-08-29 12:10 UTC (permalink / raw)
  To: pbs-devel

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@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")?;
                 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))?;
+                }
                 acceptor
                     .set_certificate_chain_file(cert)
                     .context("failed to set tls acceptor certificate chain file")?;
-- 
2.39.2



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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [pbs-devel] [PATCH v2] rest-server: check permissions on proxy.key and proxy.pem files
  2024-08-29 12:10 [pbs-devel] [PATCH v2] rest-server: check permissions on proxy.key and proxy.pem files Gabriel Goller
@ 2024-08-29 12:21 ` Wolfgang Bumiller
  2024-08-29 13:07   ` Gabriel Goller
  0 siblings, 1 reply; 3+ messages in thread
From: Wolfgang Bumiller @ 2024-08-29 12:21 UTC (permalink / raw)
  To: Gabriel Goller; +Cc: pbs-devel

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@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


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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [pbs-devel] [PATCH v2] rest-server: check permissions on proxy.key and proxy.pem files
  2024-08-29 12:21 ` Wolfgang Bumiller
@ 2024-08-29 13:07   ` Gabriel Goller
  0 siblings, 0 replies; 3+ messages in thread
From: Gabriel Goller @ 2024-08-29 13:07 UTC (permalink / raw)
  To: Wolfgang Bumiller; +Cc: pbs-devel

On 29.08.2024 14:21, Wolfgang Bumiller wrote:
>On Thu, Aug 29, 2024 at 02:10:47PM GMT, Gabriel Goller wrote:
>> [skip]
>> @@ -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!(...))`)

Good point!

>>                  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.

Done as well, thanks for the review!
Submitted a v3.


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


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2024-08-29 13:07 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-08-29 12:10 [pbs-devel] [PATCH v2] rest-server: check permissions on proxy.key and proxy.pem files Gabriel Goller
2024-08-29 12:21 ` Wolfgang Bumiller
2024-08-29 13:07   ` Gabriel Goller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal