From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 159891FF0E5 for ; Wed, 29 Jul 2026 11:53:44 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id D993221332; Wed, 29 Jul 2026 11:53:43 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Wed, 29 Jul 2026 11:53:13 +0200 Message-Id: Subject: Re: [PATCH proxmox-backup 04/11] config/server/api: add certificate renewal logic including notifications To: "Lukas Wagner" , X-Mailer: aerc 0.20.0 References: <20260618115443.48618-1-s.sterz@proxmox.com> <20260618115443.48618-5-s.sterz@proxmox.com> In-Reply-To: From: "Shannon Sterz" X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1785318754594 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.152 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_LOW -0.7 Sender listed at https://www.dnswl.org/, low trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: XY3GU7EYIUP5MP2YKKEQGQPPHOCZNH7Q X-Message-ID-Hash: XY3GU7EYIUP5MP2YKKEQGQPPHOCZNH7Q X-MailFrom: s.sterz@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Datacenter Manager development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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<(), E= rror> { >> + let metadata =3D HashMap::from([ >> + ("hostname".into(), proxmox_sys::nodename().into()), >> + ("type".into(), "cert".into()), >> + ]); >> + >> + let notification =3D 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.c= om/ >> + "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 =3D 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 =3D match result { >> + Err(e) =3D> { >> + let template_data =3D 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, >> + ) >> + } >> + _ =3D> 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/temp= lates/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<--