[pbs-devel] [PATCH proxmox-backup] fix #3867: server/api: send emails on certificate renewal failure

[pbs-devel] [PATCH proxmox-backup] fix #3867: server/api: send emails on certificate renewal failure

[Stefan Sterz]

the superuser's email will be used to notify them that certificate
renewal has failed. also adds support for a notification setting to
`node.cfg` and the api so that emails are sent either always, on
error or never(similar to datastore notifications).

Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
not sure if adding this setting to the gui is usefull and if so, where
should we put it? if we put it next to the acme account setting, users
might be confused as to which email will be used for notifications
(acme vs. superuser's).

 src/api2/node/certificates.rs     | 25 +++++++++---
 src/api2/node/config.rs           |  4 ++
 src/config/node.rs                | 12 +++++-
 src/server/email_notifications.rs | 66 +++++++++++++++++++++++++++++++
 4 files changed, 100 insertions(+), 7 deletions(-)

diff --git a/src/api2/node/certificates.rs b/src/api2/node/certificates.rs
index 9508ea38..12db992c 100644
--- a/src/api2/node/certificates.rs
+++ b/src/api2/node/certificates.rs
@@ -13,13 +13,14 @@ use proxmox_router::list_subdirs_api_method;
 use proxmox_schema::api;
 use proxmox_sys::{task_log, task_warn};
 
-use pbs_api_types::{NODE_SCHEMA, PRIV_SYS_MODIFY};
+use pbs_api_types::{Notify, NODE_SCHEMA, PRIV_SYS_MODIFY};
 use pbs_buildcfg::configdir;
 use pbs_tools::cert;
 
 use crate::acme::AcmeClient;
 use crate::api2::types::AcmeDomain;
 use crate::config::node::NodeConfig;
+use crate::server::send_certificate_renewal_mail;
 use proxmox_rest_server::WorkerTask;
 
 pub const ROUTER: Router = Router::new()
@@ -536,11 +537,23 @@ fn spawn_certificate_worker(
     let auth_id = rpcenv.get_auth_id().unwrap();
 
     WorkerTask::spawn(name, None, auth_id, true, move |worker| async move {
-        if let Some(cert) = order_certificate(worker, &node_config).await? {
-            crate::config::set_proxy_certificate(&cert.certificate, &cert.private_key_pem)?;
-            crate::server::reload_proxy_certificate().await?;
-        }
-        Ok(())
+        let work = || async {
+            if let Some(cert) = order_certificate(worker, &node_config).await? {
+                crate::config::set_proxy_certificate(&cert.certificate, &cert.private_key_pem)?;
+                crate::server::reload_proxy_certificate().await?;
+            }
+
+            Ok(())
+        };
+
+        let res = work().await;
+
+        send_certificate_renewal_mail(
+            node_config.renewal_notification.unwrap_or(Notify::Error),
+            &res,
+        )?;
+
+        res
     })
 }
 
diff --git a/src/api2/node/config.rs b/src/api2/node/config.rs
index 0a119354..c96c6dcf 100644
--- a/src/api2/node/config.rs
+++ b/src/api2/node/config.rs
@@ -64,6 +64,8 @@ pub enum DeletableProperty {
     ciphers_tls_1_2,
     /// Delete the default-lang property.
     default_lang,
+    /// Delete renewal-notification settings.
+    renewal_notification,
 }
 
 #[api(
@@ -124,6 +126,7 @@ pub fn update_node_config(
                 DeletableProperty::ciphers_tls_1_3 => { config.ciphers_tls_1_3 = None; },
                 DeletableProperty::ciphers_tls_1_2 => { config.ciphers_tls_1_2 = None; },
                 DeletableProperty::default_lang => { config.default_lang = None; },
+                DeletableProperty::renewal_notification => { config.renewal_notification = None; },
             }
         }
     }
@@ -139,6 +142,7 @@ pub fn update_node_config(
     if update.ciphers_tls_1_3.is_some() { config.ciphers_tls_1_3 = update.ciphers_tls_1_3; }
     if update.ciphers_tls_1_2.is_some() { config.ciphers_tls_1_2 = update.ciphers_tls_1_2; }
     if update.default_lang.is_some() { config.default_lang = update.default_lang; }
+    if update.renewal_notification.is_some() { config.renewal_notification = update.renewal_notification; }
 
     crate::config::node::save_config(&config)?;
 
diff --git a/src/config/node.rs b/src/config/node.rs
index 0ba87450..87046ad4 100644
--- a/src/config/node.rs
+++ b/src/config/node.rs
@@ -8,7 +8,9 @@ use proxmox_schema::{api, ApiStringFormat, ApiType, Updater};
 
 use proxmox_http::ProxyConfig;
 
-use pbs_api_types::{EMAIL_SCHEMA, OPENSSL_CIPHERS_TLS_1_2_SCHEMA, OPENSSL_CIPHERS_TLS_1_3_SCHEMA};
+use pbs_api_types::{
+    Notify, EMAIL_SCHEMA, OPENSSL_CIPHERS_TLS_1_2_SCHEMA, OPENSSL_CIPHERS_TLS_1_3_SCHEMA,
+};
 use pbs_buildcfg::configdir;
 use pbs_config::{open_backup_lockfile, BackupLockGuard};
 
@@ -167,6 +169,10 @@ pub enum Translation {
         "default-lang" : {
             schema: Translation::API_SCHEMA,
             optional: true,
+        },
+        "renewal-notification" : {
+            type: Notify,
+            optional: true,
         }
     },
 )]
@@ -210,6 +216,10 @@ pub struct NodeConfig {
     /// Default language used in the GUI
     #[serde(skip_serializing_if = "Option::is_none")]
     pub default_lang: Option<String>,
+
+    /// Whether to report certificate renewale on failure, always, never
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub renewal_notification: Option<Notify>,
 }
 
 impl NodeConfig {
diff --git a/src/server/email_notifications.rs b/src/server/email_notifications.rs
index 4d734368..347bc25d 100644
--- a/src/server/email_notifications.rs
+++ b/src/server/email_notifications.rs
@@ -183,6 +183,28 @@ Please visit the web interface for further details:
 
 "###;
 
+const ACME_CERTIFICATE_OK_RENEWAL: &str = r###"
+
+Proxmox Backup Server just renewed a TLS certificate successfully.
+
+Please visit the web interface for further details:
+
+<https://{{fqdn}}:{{port}}/#pbsCertificateConfiguration>
+
+"###;
+
+const ACME_CERTIFICATE_ERR_RENEWAL: &str = r###"
+
+Proxmox Backup Server was not able to renew a TLS certificate.
+
+Encountered an error: {{error}}
+
+Please visit the web interface for further details:
+
+<https://{{fqdn}}:{{port}}/#pbsCertificateConfiguration>
+
+"###;
+
 lazy_static::lazy_static!{
 
     static ref HANDLEBARS: Handlebars<'static> = {
@@ -209,6 +231,9 @@ lazy_static::lazy_static!{
 
             hb.register_template_string("package_update_template", PACKAGE_UPDATES_TEMPLATE)?;
 
+            hb.register_template_string("certificate_renewal_ok_template", ACME_CERTIFICATE_OK_RENEWAL)?;
+            hb.register_template_string("certificate_renewal_err_template", ACME_CERTIFICATE_ERR_RENEWAL)?;
+
             Ok(())
         });
 
@@ -543,6 +568,44 @@ pub fn send_updates_available(
     Ok(())
 }
 
+/// send email on certificate renewal failure
+pub fn send_certificate_renewal_mail(
+    notify: Notify,
+    result: &Result<(), Error>,
+) -> Result<(), Error> {
+    match notify {
+        Notify::Never => return Ok(()),
+        Notify::Error if result.is_ok() => return Ok(()),
+        _ => (),
+    }
+
+    if let Some(email) = lookup_user_email(Userid::root_userid()) {
+        let (fqdn, port) = get_server_url();
+
+        let mut data = json!({
+            "fqdn": fqdn,
+            "port": port,
+        });
+
+        let text = match result {
+            Ok(_) => HANDLEBARS.render("certificate_renewal_ok_template", &data)?,
+            Err(err) => {
+                data["error"] = err.to_string().into();
+                HANDLEBARS.render("certificate_renewal_err_template", &data)?
+            }
+        };
+
+        let subject = match result {
+            Ok(_) => "Certificate renewed successfully",
+            Err(_) => "Could not renew certificate",
+        };
+
+        send_job_status_mail(&email, subject, &text)?;
+    }
+
+    Ok(())
+}
+
 /// Lookup users email address
 pub fn lookup_user_email(userid: &Userid) -> Option<String> {
 
@@ -648,4 +711,7 @@ fn test_template_register() {
     assert!(HANDLEBARS.has_template("tape_backup_err_template"));
 
     assert!(HANDLEBARS.has_template("package_update_template"));
+
+    assert!(HANDLEBARS.has_template("certificate_renewal_ok_template"));
+    assert!(HANDLEBARS.has_template("certificate_renewal_err_template"));
 }
-- 
2.30.2

Re: [pbs-devel] [PATCH proxmox-backup] fix #3867: server/api: send emails on certificate renewal failure

[Thomas Lamprecht]

On 08.03.22 13:02, Stefan Sterz wrote:
> the superuser's email will be used to notify them that certificate
> renewal has failed. also adds support for a notification setting to
> `node.cfg` and the api so that emails are sent either always, on
> error or never(similar to datastore notifications).
> 
> Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
> ---
> not sure if adding this setting to the gui is usefull and if so, where
> should we put it? if we put it next to the acme account setting, users
> might be confused as to which email will be used for notifications
> (acme vs. superuser's).

hmm, on a second thought I'd really prefer to start out as fixed, non-configurable,
on-error setting internally, we can always add the config option transparently later
on, if really requested.

To answer the original question: If we'd add it the gui entry would be useful, most
settings without gui are hidden for ~ over 90% of our users.

> @@ -536,11 +537,23 @@ fn spawn_certificate_worker(
>      let auth_id = rpcenv.get_auth_id().unwrap();
>  
>      WorkerTask::spawn(name, None, auth_id, true, move |worker| async move {
> -        if let Some(cert) = order_certificate(worker, &node_config).await? {
> -            crate::config::set_proxy_certificate(&cert.certificate, &cert.private_key_pem)?;
> -            crate::server::reload_proxy_certificate().await?;
> -        }
> -        Ok(())
> +        let work = || async {
> +            if let Some(cert) = order_certificate(worker, &node_config).await? {
> +                crate::config::set_proxy_certificate(&cert.certificate, &cert.private_key_pem)?;
> +                crate::server::reload_proxy_certificate().await?;
> +            }
> +
> +            Ok(())
> +        };
> +
> +        let res = work().await;
> +
> +        send_certificate_renewal_mail(
> +            node_config.renewal_notification.unwrap_or(Notify::Error),
> +            &res,
> +        )?;
> +
> +        res
>      })
>  }
>  

> diff --git a/src/config/node.rs b/src/config/node.rs
> index 0ba87450..87046ad4 100644
> --- a/src/config/node.rs
> +++ b/src/config/node.rs

> @@ -210,6 +216,10 @@ pub struct NodeConfig {
>      /// Default language used in the GUI
>      #[serde(skip_serializing_if = "Option::is_none")]
>      pub default_lang: Option<String>,
> +
> +    /// Whether to report certificate renewale on failure, always, never

typo: s/renewale/renewal/

> +    #[serde(skip_serializing_if = "Option::is_none")]
> +    pub renewal_notification: Option<Notify>,

IMO an ambiguous option name for the node.cfg, i.e., if I read this in the config 
I cannot know "what" gets renewed, so iff we'd have such a config it should probably
be named "acme_renewal_notify", but as written above, I'd avoid it for now and set
on-error

>  }
>  
>  impl NodeConfig {
> diff --git a/src/server/email_notifications.rs b/src/server/email_notifications.rs
> index 4d734368..347bc25d 100644
> --- a/src/server/email_notifications.rs
> +++ b/src/server/email_notifications.rs

> +
> +const ACME_CERTIFICATE_ERR_RENEWAL: &str = r###"
> +
> +Proxmox Backup Server was not able to renew a TLS certificate.

maybe: s/a/its/ 

> +
> +Encountered an error: {{error}}

Just "Error: {{error}}"

or

"... renew its TLS certificate:

{{error}}


Please..."

but not too hard feelings on that one.

> +
> +Please visit the web interface for further details:
> +
> +<https://{{fqdn}}:{{port}}/#pbsCertificateConfiguration>
> +
> +"###;
> +
>  lazy_static::lazy_static!{
>  
>      static ref HANDLEBARS: Handlebars<'static> = {