Merge pull request #6 from Jupeyy/pr_certdownloader_fixes

Fix cert downloader not writing the current state to file.

Cargo.toml

@@ -11,7 +11,7 @@ members = [
 
 [package]
 name = "ddnet-accounts"
-version = "0.2.0"
+version = "0.2.1"
 edition = "2021"
 authors = ["Jupeyy"]
 license = "MIT OR Apache-2.0"
@@ -64,8 +64,8 @@ notify = { version = "7.0.0", default-features = false, features = ["macos_kqueu
 [dev-dependencies]
 ddnet-account-client = { version = "0.2.0", path = "lib/ddnet-account-client" }
 ddnet-account-game-server = { version = "0.3.0", path = "lib/ddnet-account-game-server" }
-ddnet-account-client-http-fs = { version = "0.2.0", path = "lib/ddnet-account-client-http-fs" }
-ddnet-account-client-reqwest = { version = "0.2.0", path = "lib/ddnet-account-client-reqwest" }
+ddnet-account-client-http-fs = { version = "0.3.0", path = "lib/ddnet-account-client-http-fs" }
+ddnet-account-client-reqwest = { version = "0.3.0", path = "lib/ddnet-account-client-reqwest" }
 
 regex = "1.11.1"
 tempfile = "3.14.0"

lib/ddnet-account-client-http-fs/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "ddnet-account-client-http-fs"
-version = "0.2.0"
+version = "0.3.0"
 edition = "2021"
 authors = ["Jupeyy"]
 license = "MIT OR Apache-2.0"

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: 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<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 = 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: 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,67 @@ 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) -> 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() = chrono::Utc::now();
 
-            *self.account_server_public_keys.write().unwrap() =
-                Arc::new(new_account_server_public_keys);
+        *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();
+        // Check last request is at least one week ago
+        let last_request_sys_time = <DateTime<chrono::Local>>::from(last_request);
+
+        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))
         }
     }
 
     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() = chrono::Utc::now()
+                        .checked_sub_signed(one_week_minus_one_day)
+                        // but in worst case wait one week
+                        .unwrap_or_else(chrono::Utc::now);
+                }
+            }
         }
     }
 

lib/ddnet-account-client-reqwest/Cargo.toml

@@ -1,14 +1,14 @@
 [package]
 name = "ddnet-account-client-reqwest"
-version = "0.2.0"
+version = "0.3.0"
 edition = "2021"
 authors = ["Jupeyy"]
 license = "MIT OR Apache-2.0"
 description = "The client implementation using reqwest as HTTP client."
 repository = "https://github.com/ddnet/ddnet-accounts"
 
 [dependencies]
-ddnet-account-client-http-fs = { version = "0.2.0", path = "../ddnet-account-client-http-fs" }
+ddnet-account-client-http-fs = { version = "0.3.0", path = "../ddnet-account-client-http-fs" }
 ddnet-account-client = { version = "0.2.0", path = "../ddnet-account-client" }
 
 async-trait = "0.1.83"

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));