implement better errors for cert upload

crates/defguard_core/src/cert_settings.rs

@@ -16,8 +16,18 @@ use utoipa::ToSchema;
 /// Errors arising from certificate settings operations.
 #[derive(Debug, Error)]
 pub enum CertSettingsError {
-    #[error("Invalid certificate: {0}")]
-    InvalidCert(String),
+    #[error("cert_pem is required for own_cert")]
+    MissingCertPem,
+    #[error("key_pem is required for own_cert")]
+    MissingKeyPem,
+    #[error("Invalid certificate or private key PEM")]
+    InvalidCertOrKey,
+    #[error("Certificate validity period is invalid")]
+    InvalidValidityPeriod,
+    #[error("Certificate has expired")]
+    CertExpired,
+    #[error("Certificate is not valid yet")]
+    CertNotYetValid,
     #[error("Certificate error: {0}")]
     Cert(#[from] CertificateError),
     #[error("URL parse error: {0}")]
@@ -36,9 +46,7 @@ async fn parse_cert(cert_pem: &str, key_pem: &str) -> Result<CertificateInfo, Ce
 
     RustlsConfig::from_pem(cert_pem.as_bytes().to_vec(), key_pem.as_bytes().to_vec())
         .await
-        .map_err(|_| {
-            CertSettingsError::InvalidCert("Invalid certificate or private key PEM".to_owned())
-        })?;
+        .map_err(|_| CertSettingsError::InvalidCertOrKey)?;
 
     let cert_der = parse_pem_certificate(cert_pem)?;
     let info = CertificateInfo::from_der(cert_der.as_ref())?;
@@ -47,21 +55,15 @@ async fn parse_cert(cert_pem: &str, key_pem: &str) -> Result<CertificateInfo, Ce
     let now = Utc::now().naive_utc();
 
     if info.not_after <= info.not_before {
-        return Err(CertSettingsError::InvalidCert(
-            "Certificate validity period is invalid".to_owned(),
-        ));
+        return Err(CertSettingsError::InvalidValidityPeriod);
     }
 
     if info.not_after <= now {
-        return Err(CertSettingsError::InvalidCert(
-            "Certificate has expired".to_owned(),
-        ));
+        return Err(CertSettingsError::CertExpired);
     }
 
     if info.not_before > now {
-        return Err(CertSettingsError::InvalidCert(
-            "Certificate is not valid yet".to_owned(),
-        ));
+        return Err(CertSettingsError::CertNotYetValid);
     }
 
     Ok(info)
@@ -206,12 +208,8 @@ pub async fn apply_internal_url_settings(
             })
         }
         InternalSslType::OwnCert => {
-            let cert_pem_str = config.cert_pem.ok_or_else(|| {
-                CertSettingsError::InvalidCert("cert_pem is required for own_cert".to_owned())
-            })?;
-            let key_pem_str = config.key_pem.ok_or_else(|| {
-                CertSettingsError::InvalidCert("key_pem is required for own_cert".to_owned())
-            })?;
+            let cert_pem_str = config.cert_pem.ok_or(CertSettingsError::MissingCertPem)?;
+            let key_pem_str = config.key_pem.ok_or(CertSettingsError::MissingKeyPem)?;
 
             let info = parse_cert(&cert_pem_str, &key_pem_str).await?;
             let valid_for_days = (info.not_after.and_utc() - chrono::Utc::now()).num_days();
@@ -332,12 +330,8 @@ pub async fn apply_external_url_settings(
             })
         }
         ExternalSslType::OwnCert => {
-            let cert_pem_str = config.cert_pem.ok_or_else(|| {
-                CertSettingsError::InvalidCert("cert_pem is required for own_cert".to_owned())
-            })?;
-            let key_pem_str = config.key_pem.ok_or_else(|| {
-                CertSettingsError::InvalidCert("key_pem is required for own_cert".to_owned())
-            })?;
+            let cert_pem_str = config.cert_pem.ok_or(CertSettingsError::MissingCertPem)?;
+            let key_pem_str = config.key_pem.ok_or(CertSettingsError::MissingKeyPem)?;
 
             let info = parse_cert(&cert_pem_str, &key_pem_str).await?;
             let valid_for_days = (info.not_after.and_utc() - chrono::Utc::now()).num_days();
@@ -438,7 +432,10 @@ pub(crate) async fn refresh_proxy_self_signed_cert(
 #[cfg(test)]
 mod tests {
     use chrono::Utc;
-    use defguard_certs::CertificateAuthority;
+    use defguard_certs::{
+        CertificateAuthority, Csr, DnType, ExtendedKeyUsagePurpose, PemLabel, der_to_pem,
+        generate_key_pair,
+    };
     use defguard_common::db::{
         models::{
             Certificates, CoreCertSource, ProxyCertSource, Settings,
@@ -449,7 +446,7 @@ mod tests {
     use sqlx::postgres::{PgConnectOptions, PgPoolOptions};
 
     use super::{
-        CertSettingsError, extract_hostname, refresh_core_self_signed_cert,
+        CertSettingsError, extract_hostname, parse_cert, refresh_core_self_signed_cert,
         refresh_proxy_self_signed_cert,
     };
 
@@ -525,6 +522,68 @@ mod tests {
         );
     }
 
+    #[tokio::test]
+    async fn parse_cert_invalid_pem() {
+        let err = parse_cert("not a certificate", "not a key")
+            .await
+            .err()
+            .expect("expected an error");
+        assert!(matches!(err, CertSettingsError::InvalidCertOrKey));
+    }
+
+    #[tokio::test]
+    async fn parse_cert_key_mismatch() {
+        let ca = make_ca();
+
+        let key_pair = generate_key_pair().expect("failed to generate key pair");
+        let san = vec!["example.com".to_owned()];
+        let dn = vec![(DnType::CommonName, "example.com")];
+        let csr = Csr::new(&key_pair, &san, dn).expect("failed to build CSR");
+        let cert = ca
+            .sign_web_server_cert(&csr)
+            .expect("failed to sign certificate");
+        let cert_pem =
+            der_to_pem(cert.der(), PemLabel::Certificate).expect("failed to encode cert");
+
+        let other_key_pair = generate_key_pair().expect("failed to generate key pair");
+        let mismatched_key_pem = der_to_pem(
+            other_key_pair.serialize_der().as_slice(),
+            PemLabel::PrivateKey,
+        )
+        .expect("failed to encode key");
+
+        let err = parse_cert(&cert_pem, &mismatched_key_pem)
+            .await
+            .err()
+            .expect("expected an error");
+        assert!(matches!(err, CertSettingsError::InvalidCertOrKey));
+    }
+
+    #[tokio::test]
+    async fn parse_cert_expired() {
+        let ca = make_ca();
+
+        let key_pair = generate_key_pair().expect("failed to generate key pair");
+        let san = vec!["example.com".to_owned()];
+        let dn = vec![(DnType::CommonName, "example.com")];
+        let csr = Csr::new(&key_pair, &san, dn).expect("failed to build CSR");
+        let cert = ca
+            .sign_csr_with_validity(&csr, 0, &[ExtendedKeyUsagePurpose::ServerAuth])
+            .expect("failed to sign certificate");
+        let cert_pem =
+            der_to_pem(cert.der(), PemLabel::Certificate).expect("failed to encode cert");
+        let key_pem = der_to_pem(key_pair.serialize_der().as_slice(), PemLabel::PrivateKey)
+            .expect("failed to encode key");
+
+        tokio::time::sleep(std::time::Duration::from_millis(50)).await;
+
+        let err = parse_cert(&cert_pem, &key_pem)
+            .await
+            .err()
+            .expect("expected an error");
+        assert!(matches!(err, CertSettingsError::CertExpired));
+    }
+
     #[sqlx::test]
     async fn refresh_core_self_signed_cert_generates_new_cert(
         _: PgPoolOptions,

crates/defguard_core/src/enterprise/directory_sync/tests.rs

@@ -897,7 +897,7 @@ mod test {
             true,
         )
         .await;
-        provider.directory_sync_user_groups = Some(vec!["group1".to_string()]);
+        provider.directory_sync_user_groups = Some(vec!["group1".to_owned()]);
         provider.save(&pool).await.unwrap();
 
         // no users in Defguard before sync
@@ -938,7 +938,7 @@ mod test {
             true,
         )
         .await;
-        provider.directory_sync_user_groups = Some(vec!["nonexistent-group".to_string()]);
+        provider.directory_sync_user_groups = Some(vec!["nonexistent-group".to_owned()]);
         provider.save(&pool).await.unwrap();
 
         let (ldap_tx, _ldap_rx) = ldap_test_channel();
@@ -979,7 +979,7 @@ mod test {
         assert!(defguard_users.is_empty());
 
         // only allow one of the directory users to be imported
-        let allowed_emails = HashSet::from(["testuser@email.com".to_string()]);
+        let allowed_emails = HashSet::from(["testuser@email.com".to_owned()]);
         let all_users = client.get_all_users().await.unwrap();
         let (ldap_tx, _ldap_rx) = ldap_test_channel();
         sync_all_users_state(
@@ -1018,12 +1018,12 @@ mod test {
 
         // the test provider returns group1 as the only group of any user
         assert!(
-            user_in_directory_groups(&pool, "testuser@email.com", &["group1".to_string()])
+            user_in_directory_groups(&pool, "testuser@email.com", &["group1".to_owned()])
                 .await
                 .unwrap()
         );
         assert!(
-            !user_in_directory_groups(&pool, "testuser@email.com", &["group2".to_string()])
+            !user_in_directory_groups(&pool, "testuser@email.com", &["group2".to_owned()])
                 .await
                 .unwrap()
         );

crates/defguard_core/src/enterprise/handlers/openid_providers.rs

@@ -165,7 +165,7 @@ pub(crate) async fn add_openid_provider(
             Some(
                 user_groups
                     .split(',')
-                    .map(|s| s.trim().to_string())
+                    .map(|s| s.trim().to_owned())
                     .collect(),
             )
         }
@@ -410,7 +410,7 @@ pub(crate) async fn modify_openid_provider(
                 Some(
                     user_groups
                         .split(',')
-                        .map(|s| s.trim().to_string())
+                        .map(|s| s.trim().to_owned())
                         .collect(),
                 )
             }

crates/defguard_core/src/enterprise/oauth2/mod.rs

@@ -95,7 +95,7 @@ async fn microsoft_access_token(smtp_settings: &mut SmtpSettings) -> Result<Stri
 
     let oauth2 = MicrosoftOAuth2::new(
         client_id.clone(),
-        client_secret.expose_secret().to_string(),
+        client_secret.expose_secret().to_owned(),
         tenant_id.clone(),
         OUTLOOK_DEFAULT_SCOPE.into(),
     );

crates/defguard_core/src/error.rs

@@ -100,6 +100,20 @@ pub enum WebError {
     #[error(transparent)]
     #[schema(value_type=Object)]
     IpNetwork(#[from] ipnetwork::IpNetworkError),
+    #[error("cert_pem is required for own_cert")]
+    CertMissingCertPem,
+    #[error("key_pem is required for own_cert")]
+    CertMissingKeyPem,
+    #[error("Invalid certificate or private key PEM")]
+    CertInvalidCertOrKey,
+    #[error("Certificate validity period is invalid")]
+    CertInvalidValidityPeriod,
+    #[error("Certificate has expired")]
+    CertExpired,
+    #[error("Certificate is not valid yet")]
+    CertNotYetValid,
+    #[error("Certificate error: {0}")]
+    CertParseError(String),
 }
 
 impl From<tonic::Status> for WebError {
@@ -259,8 +273,13 @@ impl From<CertSettingsError> for WebError {
     fn from(err: CertSettingsError) -> Self {
         error!("{err}");
         match err {
-            CertSettingsError::InvalidCert(msg) => Self::BadRequest(msg),
-            CertSettingsError::Cert(e) => Self::CertificateError(e),
+            CertSettingsError::MissingCertPem => Self::CertMissingCertPem,
+            CertSettingsError::MissingKeyPem => Self::CertMissingKeyPem,
+            CertSettingsError::InvalidCertOrKey => Self::CertInvalidCertOrKey,
+            CertSettingsError::InvalidValidityPeriod => Self::CertInvalidValidityPeriod,
+            CertSettingsError::CertExpired => Self::CertExpired,
+            CertSettingsError::CertNotYetValid => Self::CertNotYetValid,
+            CertSettingsError::Cert(e) => Self::CertParseError(e.to_string()),
             CertSettingsError::Url(e) => Self::BadRequest(e),
             CertSettingsError::Settings(e) => Self::BadRequest(e.to_string()),
             CertSettingsError::Db(e) => Self::DbError(e.to_string()),

crates/defguard_core/src/handlers/mod.rs

@@ -67,6 +67,13 @@ pub(crate) mod yubikey;
 pub enum WebErrorCode {
     NetworkFull,
     UserGroupsNotSynced,
+    CertMissingCertPem,
+    CertMissingKeyPem,
+    CertInvalidCertOrKey,
+    CertInvalidValidityPeriod,
+    CertExpired,
+    CertNotYetValid,
+    CertParseError,
 }
 
 pub static SESSION_COOKIE_NAME: &str = "defguard_session";
@@ -296,6 +303,37 @@ impl From<WebError> for ApiResponse {
                     StatusCode::UNAUTHORIZED,
                 )
             }
+            WebError::CertMissingCertPem => Self::new(
+                json!({"msg": web_error.to_string(), "code": WebErrorCode::CertMissingCertPem}),
+                StatusCode::BAD_REQUEST,
+            ),
+            WebError::CertMissingKeyPem => Self::new(
+                json!({"msg": web_error.to_string(), "code": WebErrorCode::CertMissingKeyPem}),
+                StatusCode::BAD_REQUEST,
+            ),
+            WebError::CertInvalidCertOrKey => Self::new(
+                json!({"msg": web_error.to_string(), "code": WebErrorCode::CertInvalidCertOrKey}),
+                StatusCode::BAD_REQUEST,
+            ),
+            WebError::CertInvalidValidityPeriod => Self::new(
+                json!({"msg": web_error.to_string(), "code": WebErrorCode::CertInvalidValidityPeriod}),
+                StatusCode::BAD_REQUEST,
+            ),
+            WebError::CertExpired => Self::new(
+                json!({"msg": web_error.to_string(), "code": WebErrorCode::CertExpired}),
+                StatusCode::BAD_REQUEST,
+            ),
+            WebError::CertNotYetValid => Self::new(
+                json!({"msg": web_error.to_string(), "code": WebErrorCode::CertNotYetValid}),
+                StatusCode::BAD_REQUEST,
+            ),
+            WebError::CertParseError(msg) => {
+                warn!(msg);
+                Self::new(
+                    json!({"msg": msg, "code": WebErrorCode::CertParseError}),
+                    StatusCode::BAD_REQUEST,
+                )
+            }
             WebError::TemplateError(err) => {
                 error!("Template error: {err}");
                 Self::new(

crates/defguard_core/src/mail/mod.rs

@@ -114,7 +114,7 @@ pub struct Mail {
 impl Mail {
     /// Create new [`Mail`].
     #[must_use]
-    pub fn new<T>(to: T, subject: String, html: String, text: String) -> Mail
+    pub fn new<T>(to: T, subject: String, html: String, text: String) -> Self
     where
         T: Into<String>,
     {
@@ -355,31 +355,31 @@ impl MailMessage {
             }
         }
         match self {
-            Self::Test => "Defguard: Test message".to_string(),
-            Self::Welcome => WELCOME_EMAIL_SUBJECT.to_string(),
-            Self::SupportData => "Defguard: Support data".to_string(),
-            Self::DesktopStart => "Defguard: Desktop client configuration".to_string(),
-            Self::NewAccount => "Defguard: User enrollment".to_string(),
-            Self::NewDevice => "Defguard: new device added to your account".to_string(),
-            Self::NewDeviceLogin => "Defguard: New device logged in to your account".to_string(),
-            Self::NewDeviceOIDCLogin => "New login to OIDC application".to_string(),
-            Self::GatewayDisconnect => "Defguard: Gateway disconnected".to_string(),
-            Self::GatewayReconnect => "Defguard: Gateway reconnected".to_string(),
-            Self::MFAActivation => "Multi-Factor Authentication activation".to_string(),
+            Self::Test => "Defguard: Test message".to_owned(),
+            Self::Welcome => WELCOME_EMAIL_SUBJECT.to_owned(),
+            Self::SupportData => "Defguard: Support data".to_owned(),
+            Self::DesktopStart => "Defguard: Desktop client configuration".to_owned(),
+            Self::NewAccount => "Defguard: User enrollment".to_owned(),
+            Self::NewDevice => "Defguard: new device added to your account".to_owned(),
+            Self::NewDeviceLogin => "Defguard: New device logged in to your account".to_owned(),
+            Self::NewDeviceOIDCLogin => "New login to OIDC application".to_owned(),
+            Self::GatewayDisconnect => "Defguard: Gateway disconnected".to_owned(),
+            Self::GatewayReconnect => "Defguard: Gateway reconnected".to_owned(),
+            Self::MFAActivation => "Multi-Factor Authentication activation".to_owned(),
             Self::MFAConfigured { method } => {
                 format!("Multi-Factor Authentication {method} has been activated")
             }
-            Self::MFACode => "Defguard: Multi-Factor Authentication code for login".to_string(),
-            Self::PasswordReset => "Defguard: Password reset".to_string(),
-            Self::PasswordResetDone => "Defguard: Password reset success".to_string(),
-            Self::PasswordResetDisabled => "Defguard: Password reset disabled".to_string(),
-            Self::UserImportBlocked => "User import blocked".to_string(),
-            Self::EnrollmentNotification => "Defguard: User enrollment completed".to_string(),
+            Self::MFACode => "Defguard: Multi-Factor Authentication code for login".to_owned(),
+            Self::PasswordReset => "Defguard: Password reset".to_owned(),
+            Self::PasswordResetDone => "Defguard: Password reset success".to_owned(),
+            Self::PasswordResetDisabled => "Defguard: Password reset disabled".to_owned(),
+            Self::UserImportBlocked => "User import blocked".to_owned(),
+            Self::EnrollmentNotification => "Defguard: User enrollment completed".to_owned(),
             Self::LetsencryptCertRefreshFailed => {
-                "Defguard: automatic Let's Encrypt certificate refresh failed".to_string()
+                "Defguard: automatic Let's Encrypt certificate refresh failed".to_owned()
             }
-            Self::CertificateExpiration => "Defguard: Certificate expiration".to_string(),
-            Self::CertificateExpired => "Defguard: Certificate has expired".to_string(),
+            Self::CertificateExpiration => "Defguard: Certificate expiration".to_owned(),
+            Self::CertificateExpired => "Defguard: Certificate has expired".to_owned(),
         }
     }