Fix cert downloader not writing the current state to file.

Fix cert downloader not respecting the last request time.
Make the cert downloader more error resistant and add tests for the sleep time.

lib/ddnet-account-client-http-fs/src/cert_downloader.rs

@@ -4,12 +4,20 @@ use std::{
 };
 
 use anyhow::anyhow;
+use chrono::{DateTime, TimeDelta, Utc};
 use ddnet_account_client::certs::certs_to_pub_keys;
 use ddnet_accounts_shared::game_server::user_id::VerifyingKey;
+use serde::{Deserialize, Serialize};
 use tokio::time::Instant;
 use x509_cert::der::{Decode, Encode};
 
-use crate::client::ClientHttpTokioFs;
+use crate::{client::ClientHttpTokioFs, fs::Fs};
+
+#[derive(Debug, Clone, Serialize, Deserialize)]
+struct CertsDownloaderProps {
+    certs_der: Vec<Vec<u8>>,
+    last_request: Option<DateTime<Utc>>,
+}
 
 /// Helper to download the latest public certificates
 /// of the account server(s).
@@ -21,6 +29,7 @@ pub struct CertsDownloader {
     client: Arc<ClientHttpTokioFs>,
     account_server_public_keys: RwLock<Arc<Vec<VerifyingKey>>>,
     cur_certs: RwLock<Vec<x509_cert::Certificate>>,
+    last_request: RwLock<Option<DateTime<Utc>>>,
 }
 
 impl CertsDownloader {
@@ -32,52 +41,65 @@ impl CertsDownloader {
             .await
             .map_err(|err| anyhow!(err))
             .and_then(|cert_json| {
-                serde_json::from_slice::<Vec<Vec<u8>>>(&cert_json)
+                serde_json::from_slice::<CertsDownloaderProps>(&cert_json)
                     .map_err(|err| anyhow!(err))
-                    .and_then(|certs_der| {
-                        certs_der
+                    .and_then(|props| {
+                        props
+                            .certs_der
                             .into_iter()
                             .map(|cert_der| {
                                 x509_cert::Certificate::from_der(&cert_der)
                                     .map_err(|err| anyhow!(err))
                             })
                             .collect::<anyhow::Result<Vec<x509_cert::Certificate>>>()
+                            .map(|certs| (certs, props.last_request))
                     })
             });
 
         match certs_file {
-            Ok(certs_file) => Ok(Arc::new(Self {
+            Ok((certs_file, last_request)) => Ok(Arc::new(Self {
                 client,
                 account_server_public_keys: RwLock::new(Arc::new(certs_to_pub_keys(&certs_file))),
                 cur_certs: RwLock::new(certs_file),
+                last_request: RwLock::new(last_request),
             })),
             Err(_) => {
                 // try to download latest cert instead
                 let certs = ddnet_account_client::certs::download_certs(client.as_ref()).await?;
 
-                let _ = client
-                    .fs
-                    .write(
-                        "".as_ref(),
-                        "account_server_certs.json".as_ref(),
-                        serde_json::to_vec(
-                            &certs
-                                .iter()
-                                .map(|cert| cert.to_der().map_err(|err| anyhow!(err)))
-                                .collect::<anyhow::Result<Vec<_>>>()?,
-                        )?,
-                    )
-                    .await;
+                let now_utc = Some(Utc::now());
+                let _ = Self::write_file(&client.fs, &certs, now_utc).await;
 
                 Ok(Arc::new(Self {
                     account_server_public_keys: RwLock::new(Arc::new(certs_to_pub_keys(&certs))),
                     client,
                     cur_certs: RwLock::new(certs),
+                    last_request: RwLock::new(now_utc),
                 }))
             }
         }
     }
 
+    async fn write_file(
+        fs: &Fs,
+        certs: &[x509_cert::Certificate],
+        last_request: Option<DateTime<Utc>>,
+    ) -> anyhow::Result<()> {
+        Ok(fs
+            .write(
+                "".as_ref(),
+                "account_server_certs.json".as_ref(),
+                serde_json::to_vec(&CertsDownloaderProps {
+                    certs_der: certs
+                        .iter()
+                        .map(|cert| cert.to_der().map_err(|err| anyhow!(err)))
+                        .collect::<anyhow::Result<Vec<_>>>()?,
+                    last_request,
+                })?,
+            )
+            .await?)
+    }
+
     /// Returns the duration when the next certificate gets invalid,
     /// or `None` if no certificate exists.
     ///
@@ -101,28 +123,74 @@ impl CertsDownloader {
             .min()
     }
 
-    pub async fn download_certs(&self) {
-        if let Ok(certs) = ddnet_account_client::certs::download_certs(self.client.as_ref()).await {
-            let new_account_server_public_keys = certs_to_pub_keys(&certs);
-            *self.cur_certs.write().unwrap() = certs;
+    pub fn last_request(&self) -> Option<DateTime<Utc>> {
+        *self.last_request.read().unwrap()
+    }
+
+    pub async fn download_certs(&self) -> anyhow::Result<()> {
+        let certs = ddnet_account_client::certs::download_certs(self.client.as_ref()).await?;
+        let new_account_server_public_keys = certs_to_pub_keys(&certs);
+        *self.cur_certs.write().unwrap() = certs;
+        *self.last_request.write().unwrap() = Some(chrono::Utc::now());
+
+        *self.account_server_public_keys.write().unwrap() =
+            Arc::new(new_account_server_public_keys);
+
+        Ok(())
+    }
+
+    pub fn sleep_time(&self) -> Duration {
+        let invalid_in = self.invalid_in(SystemTime::now(), Duration::from_secs(7 * 24 * 60 * 60));
+
+        // either if first cert is about to invalidate or when one week passed
+        let one_week = Duration::from_secs(7 * 24 * 60 * 60);
+        let duration_offset = invalid_in.unwrap_or(one_week).min(one_week);
+
+        let last_request = self.last_request();
+        match last_request {
+            Some(last_request) => {
+                // Check last request is at least one week ago
+                let last_request_sys_time = <DateTime<chrono::Local>>::from(last_request);
 
-            *self.account_server_public_keys.write().unwrap() =
-                Arc::new(new_account_server_public_keys);
+                let time_diff = chrono::Local::now()
+                    .signed_duration_since(last_request_sys_time)
+                    .to_std()
+                    .unwrap_or(Duration::ZERO);
+                if time_diff > one_week {
+                    duration_offset
+                } else {
+                    // If one week didn't pass, wait at least the remaining time
+                    duration_offset.max(one_week.saturating_sub(time_diff))
+                }
+            }
+            None => duration_offset,
         }
     }
 
     pub async fn download_task(&self) -> ! {
         loop {
-            let invalid_in =
-                self.invalid_in(SystemTime::now(), Duration::from_secs(7 * 24 * 60 * 60));
-
-            // either if first cert is about to invalidate or when one week passed
-            let one_week = Duration::from_secs(7 * 24 * 60 * 60);
-            let duration_offset = invalid_in.unwrap_or(one_week).min(one_week);
-
+            let duration_offset = self.sleep_time();
             tokio::time::sleep_until(Instant::now() + duration_offset).await;
 
-            self.download_certs().await;
+            match self.download_certs().await {
+                Ok(_) => {
+                    let certs = self.cur_certs.read().unwrap().clone();
+                    let last_request = self.last_request();
+                    // write the server certs to file
+                    let _ = Self::write_file(&self.client.fs, &certs, last_request).await;
+                }
+                Err(_) => {
+                    // if the download task failed we still want to assure some sleep
+                    // of at least one day.
+                    let one_week_minus_one_day = TimeDelta::seconds(6 * 24 * 60 * 60);
+                    *self.last_request.write().unwrap() = Some(
+                        chrono::Utc::now()
+                            .checked_sub_signed(one_week_minus_one_day)
+                            // but in worst case wait one week
+                            .unwrap_or_else(chrono::Utc::now),
+                    );
+                }
+            }
         }
     }
 

src/tests/signing_certs.rs

@@ -49,8 +49,26 @@ async fn signing_certs() {
             ddnet_account_client::certs::download_certs(client.client.as_ref()).await?;
         assert!(downloaded_certs.contains(&cert));
 
+        // the sleep time is around one week, since no request was made before (the file didn't exist).
+        assert!(cert_downloader.sleep_time() > Duration::from_secs(60 * 60 * 24));
+        let last_request = cert_downloader.last_request();
+
         // now force download the newest certs
-        cert_downloader.download_certs().await;
+        cert_downloader.download_certs().await?;
+
+        // the sleep time is around one week, since a request was just made.
+        assert!(cert_downloader.sleep_time() > Duration::from_secs(60 * 60 * 24));
+        // also make sure the request is reset properly.
+        assert!(cert_downloader.last_request() >= last_request);
+
+        // this what the sleep time in the cert downloader would do
+        let invalid_in = cert_downloader.invalid_in(now, Duration::from_secs(7 * 24 * 60 * 60));
+        let one_week = Duration::from_secs(7 * 24 * 60 * 60);
+        let duration_offset = invalid_in.unwrap_or(one_week).min(one_week);
+        // since our cert is only valid for 5 seconds, it must have been invalid in one week.
+        assert!(duration_offset == Duration::ZERO);
+        // but the sleep time is still bigger due to the last request
+        assert!(cert_downloader.sleep_time() > Duration::from_secs(60 * 60 * 24));
 
         // now the cert should be invalid in the previously specified time (now)
         let invalid_in = cert_downloader.invalid_in(now, Duration::from_secs(0));