Re: [PATCH proxmox-backup 04/11] config/server/api: add certificate renewal logic including notifications

["Shannon Sterz" <s.sterz@proxmox.com>]

On Fri Jul 10, 2026 at 1:17 PM CEST, Lukas Wagner wrote:
> Hi Shannon,
>
> some notes inline.
>
> On Thu Jun 18, 2026 at 1:54 PM CEST, Shannon Sterz wrote:

-->8 snip 8<--

>>
>> +/// Send email for upcoming self signed renewal.
>> +pub fn send_upcoming_self_signed_renewal_notification() -> Result<(), Error> {
>> +    let metadata = HashMap::from([
>> +        ("hostname".into(), proxmox_sys::nodename().into()),
>> +        ("type".into(), "cert".into()),
>> +    ]);
>> +
>> +    let notification = Notification::from_template(
>> +        Severity::Info,
>
> Maybe this should use Severity::Notice? Since it might require some
> attention from the user?

imo no, at least not yet. this series does not stage certificates yet
(like my series for pve would [1]), so there is nothing a user can do at
this stage. however, the notification further down that a renewal has
happened needs more attention as fingerprints would need to be
exchanged. so i'll adjust the severity there.

once we have a staging mechanism in place, we can include the new
fingerprint in the notification and increase the severity here to make
users aware this will happen. we can then tell the user to make sure to
check that any clients are either updated automatically (through a
mechanism similar to the one in that series) or that they should add the
fingerprint manually.

[1]: https://lore.proxmox.com/all/20260611120327.257523-1-s.sterz@proxmox.com/

>> +        "cert-upcoming-refresh",
>> +        serde_json::to_value(CommonData::new())?,
>> +        metadata,
>> +    );
>> +
>> +    send_notification(notification)?;
>> +    Ok(())
>> +}
>> +
>> +/// Send email renewed self-signed certificate
>> +pub fn send_self_signed_renewal_notification(result: &Result<(), Error>) -> Result<(), Error> {
>> +    let metadata = HashMap::from([
>> +        ("hostname".into(), proxmox_sys::nodename().into()),
>> +        ("type".into(), "acme".into()),
>
> I guess the type should be "cert"?
>
> Also, maybe a longer 'cert-renewal' might be clearer. Maybe we could
> consolidate the ACME and self-signed notifications at some point
> (probably during a major upgrade, since notification matchers might rely
> on the type being 'acme')

ack, i went with cert-renewal for now.

>
>> +    ]);
>> +
>> +    let notification = match result {
>> +        Err(e) => {
>> +            let template_data = AcmeErrTemplateData {
>> +                common: CommonData::new(),
>> +                error: format!("{e:#}"),
>> +            };
>> +
>> +            Notification::from_template(
>> +                Severity::Info,
>
> This should be Severity::Error. Seems like in the pre-existing
> ACME certificate notification it also only uses Info, which seems like a
> bug to me. Feel free to fix it there as well.

done.

>> +                "acme-err",
>
> Considering that templates can be overridden by users, we might want use
> a dedicated template for the self-signed ones. Or, as mentioned above,
> consolidate the notification types, which *could* (but does not have to)
> mean that we also use the same templates for both, ACME and self-signed.

ack, added a second cert-err template for now.

>> +                serde_json::to_value(template_data)?,
>> +                metadata,
>> +            )
>> +        }
>> +        _ => Notification::from_template(
>> +            Severity::Info,
>> +            "cert-refresh",
>> +            serde_json::to_value(CommonData::new())?,
>> +            metadata,
>> +        ),
>
> I've just realized that we don't really send notifications for
> successful ACME cert refreshes, so maybe we should do this in the future
> as well. This would of course be a separate patch series, just thinking
> out aloud here.

we could, but the point of using acme usually is that you use
certificates that are trusted by other hosts in you environment.
meaning, you either use a public acme provider that is generally trusted
or a private one and add your root cert to the trust stores of the
systems in your environment.

this means, most users using acme (usually) don't need to do anything
when successfully refreshing. whereas, a refreshed self-signed
certificate requires exchanging fingerprints again. otherwise, backups
will start failing.

so imo, if we want to add a notification for that, it should be lower
severity than the self-signed certificate one.

>> +    };
>> +
>> +    send_notification(notification)
>> +}

-->8 snip 8<--

>> diff --git a/templates/default/cert-upcoming-refresh-body.txt.hbs b/templates/default/cert-upcoming-refresh-body.txt.hbs
>> new file mode 100644
>> index 000000000..8d199c9fd
>> --- /dev/null
>> +++ b/templates/default/cert-upcoming-refresh-body.txt.hbs
>> @@ -0,0 +1,9 @@
>> +Proxmox Backup Server will refresh its TLS certificate within the next 30 days.
>
> I wonder, would it be possible to include a concrete date here? If I
> understood it correctly, the first time the user receives this
> notification, the renewal would be first attempted in 15 days at the
> earliest, so maybe this should be mentioned.

i can add a line that the earliest a certificate will updated is in X
days.

adding a date is imo a bit trickier due to localization. as far as i can
tell, handlebars [1] currently has no date formatting helpers. so either
we implement our own, which seems orthogonal to this series and probably
not worth it if we do decide to switch templating engines in the near
future. or we don't care about localization and simply decide on one
format (e.g. YYYY-MM-DD).

what do you think?

[1]: https://github.com/sunng87/handlebars-rust/issues/614

-->8 snip 8<--