Fixing oauth bugs and working on secret handling

Cargo.toml

@@ -62,12 +62,13 @@ ring = "0.17.3"
 rsa = "0.9.2"
 rustfmt-wrapper = "0.2.0"
 schemars = "0.8.15"
-sha2 = "0.10.8"
+secrecy = "0.8.0"
 semver = "1.0.20"
 serde = "1"
 serde_bytes = "0.11.12"
 serde_json = "1"
 serde_urlencoded = "0.7.1"
+sha2 = "0.10.8"
 similar = "2.3.0"
 slog = "2.7.0"
 slog-async = "2.8.0"

rfd-api/Cargo.toml

@@ -35,11 +35,12 @@ ring = { workspace = true }
 rfd-model = { path = "../rfd-model" }
 rsa = { workspace = true, features = ["sha2"] }
 schemars = { workspace = true, features = ["chrono"] }
-sha2 = { workspace = true }
+secrecy = { workspace = true, features = ["serde"] }
 serde = { workspace = true, features = ["derive"] }
 serde_bytes = { workspace = true }
 serde_json = { workspace = true }
 serde_urlencoded = { workspace = true }
+sha2 = { workspace = true }
 slog = { workspace = true }
 slog-async = { workspace = true }
 tap = { workspace = true }

rfd-api/src/authn/key.rs

@@ -1,13 +1,13 @@
 use hex::FromHexError;
 use rand::{rngs::OsRng, RngCore};
+use secrecy::{SecretString, ExposeSecret, SecretVec};
 use thiserror::Error;
 use uuid::Uuid;
 
 use super::{Signer, SigningKeyError};
 
-#[derive(Debug)]
 pub struct RawApiKey {
-    clear: Vec<u8>,
+    clear: SecretVec<u8>,
 }
 
 #[derive(Debug, Error)]
@@ -34,27 +34,27 @@ impl RawApiKey {
         let mut clear = id.as_bytes().to_vec();
         clear.append(&mut token_raw.to_vec());
 
-        Self { clear }
+        Self { clear: clear.into() }
     }
 
     pub fn id(&self) -> &[u8] {
-        &self.clear[0..16]
+        &self.clear.expose_secret()[0..16]
     }
 
     pub async fn sign(self, signer: &dyn Signer) -> Result<SignedApiKey, ApiKeyError> {
         let signature = hex::encode(
             signer
-                .sign(&self.clear)
+                .sign(&self.clear.expose_secret())
                 .await
                 .map_err(ApiKeyError::Signing)?,
         );
-        Ok(SignedApiKey::new(hex::encode(self.clear), signature))
+        Ok(SignedApiKey::new(hex::encode(self.clear.expose_secret()).into(), signature))
     }
 
     pub fn verify(&self, signer: &dyn Signer, signature: &[u8]) -> Result<(), ApiKeyError> {
         let signature = hex::decode(signature)?;
         Ok(signer
-            .verify(&self.clear, &signature)
+            .verify(&self.clear.expose_secret(), &signature)
             .map_err(ApiKeyError::Verify)?)
     }
 }
@@ -66,7 +66,22 @@ impl TryFrom<&str> for RawApiKey {
         let decoded = hex::decode(value)?;
 
         if decoded.len() > 16 {
-            Ok(RawApiKey { clear: decoded })
+            Ok(RawApiKey { clear: decoded.into() })
+        } else {
+            tracing::debug!(len = ?decoded.len(), "API key is too short");
+            Err(ApiKeyError::FailedToParse)
+        }
+    }
+}
+
+impl TryFrom<&SecretString> for RawApiKey {
+    type Error = ApiKeyError;
+
+    fn try_from(value: &SecretString) -> Result<Self, Self::Error> {
+        let decoded = hex::decode(value.expose_secret())?;
+
+        if decoded.len() > 16 {
+            Ok(RawApiKey { clear: decoded.into() })
         } else {
             tracing::debug!(len = ?decoded.len(), "API key is too short");
             Err(ApiKeyError::FailedToParse)
@@ -75,16 +90,16 @@ impl TryFrom<&str> for RawApiKey {
 }
 
 pub struct SignedApiKey {
-    key: String,
+    key: SecretString,
     signature: String,
 }
 
 impl SignedApiKey {
-    fn new(key: String, signature: String) -> Self {
+    fn new(key: SecretString, signature: String) -> Self {
         Self { key, signature }
     }
 
-    pub fn key(self) -> String {
+    pub fn key(self) -> SecretString {
         self.key
     }
 
@@ -95,6 +110,7 @@ impl SignedApiKey {
 
 #[cfg(test)]
 mod tests {
+    use secrecy::ExposeSecret;
     use uuid::Uuid;
 
     use super::RawApiKey;
@@ -106,12 +122,9 @@ mod tests {
         let signer = mock_key().as_signer().await.unwrap();
 
         let raw = RawApiKey::generate::<8>(&id);
-        println!("{:?}", raw);
-
         let signed = raw.sign(&*signer).await.unwrap();
 
-        let raw2 = RawApiKey::try_from(signed.key.as_str()).unwrap();
-        println!("{:?}", raw2);
+        let raw2 = RawApiKey::try_from(signed.key.expose_secret().as_str()).unwrap();
 
         assert_eq!(
             (),

rfd-api/src/config.rs

@@ -1,6 +1,7 @@
 use std::path::PathBuf;
 
 use config::{Config, ConfigError, Environment, File};
+use secrecy::SecretString;
 use serde::{
     de::{self, Visitor},
     Deserialize, Deserializer,
@@ -123,13 +124,13 @@ pub struct OAuthConfig {
 #[derive(Debug, Deserialize)]
 pub struct OAuthDeviceConfig {
     pub client_id: String,
-    pub client_secret: String,
+    pub client_secret: SecretString,
 }
 
 #[derive(Debug, Deserialize)]
 pub struct OAuthWebConfig {
     pub client_id: String,
-    pub client_secret: String,
+    pub client_secret: SecretString,
     pub redirect_uri: String,
 }
 

rfd-api/src/context.rs

@@ -44,7 +44,7 @@ use crate::{
     },
     config::{AsymmetricKey, JwtConfig, SearchConfig},
     endpoints::login::{
-        oauth::{OAuthProvider, OAuthProviderError, OAuthProviderFn, OAuthProviderName},
+        oauth::{OAuthProvider, OAuthProviderError, OAuthProviderFn, OAuthProviderName, ClientType},
         UserInfo,
     },
     error::{ApiError, AppError},
@@ -244,6 +244,14 @@ impl ApiContext {
         })
     }
 
+    pub fn device_client(&self) -> ClientType {
+        ClientType::Device
+    }
+
+    pub fn web_client(&self) -> ClientType {
+        ClientType::Web { prefix: self.public_url.to_string() }
+    }
+
     pub fn set_storage(&mut self, storage: Arc<dyn Storage>) {
         self.storage = storage;
     }
@@ -1382,9 +1390,9 @@ pub(crate) mod test_mocks {
             Box::new(move || {
                 Box::new(GoogleOAuthProvider::new(
                     "google_device_client_id".to_string(),
-                    "google_device_client_secret".to_string(),
+                    "google_device_client_secret".to_string().into(),
                     "google_web_client_id".to_string(),
-                    "google_web_client_secret".to_string(),
+                    "google_web_client_secret".to_string().into(),
                 ))
             }),
         );

rfd-api/src/endpoints/api_user.rs

@@ -25,7 +25,7 @@ use crate::{
     util::response::{
         bad_request, forbidden, internal_error, not_found, to_internal_error, unauthorized,
     },
-    ApiCaller, ApiPermissions, User,
+    ApiCaller, ApiPermissions, User, secrets::OpenApiSecretString,
 };
 
 #[derive(Debug, Deserialize, Serialize, JsonSchema)]
@@ -266,7 +266,7 @@ pub struct ApiKeyCreateParams {
 pub struct InitialApiKeyResponse {
     pub id: Uuid,
     #[partial(ApiKeyResponse(skip))]
-    pub key: String,
+    pub key: OpenApiSecretString,
     pub permissions: ApiPermissions,
     pub created_at: DateTime<Utc>,
 }
@@ -329,7 +329,7 @@ async fn create_api_user_token_op(
             // plaintext token as we do not store a copy
             Ok(HttpResponseCreated(InitialApiKeyResponse {
                 id: user_key.id,
-                key: key.key(),
+                key: key.key().into(),
                 permissions: user_key.permissions,
                 created_at: user_key.created_at,
             }))

rfd-api/src/endpoints/api_user_provider.rs

@@ -6,7 +6,7 @@ use tracing::instrument;
 use uuid::Uuid;
 
 use crate::{
-    context::ApiContext, error::ApiError, permissions::ApiPermission, util::response::forbidden,
+    context::ApiContext, error::ApiError, permissions::ApiPermission, util::response::forbidden, secrets::OpenApiSecretString,
 };
 
 #[derive(Debug, Deserialize, JsonSchema)]
@@ -21,7 +21,7 @@ pub struct ApiUserLinkRequestPayload {
 
 #[derive(Debug, Serialize, JsonSchema)]
 pub struct ApiUserLinkRequestResponse {
-    token: String,
+    token: OpenApiSecretString,
 }
 
 /// Create a new link token for linking this provider to a different api user
@@ -57,7 +57,7 @@ pub async fn create_link_token(
                 .map_err(ApiError::Storage)?;
 
             Ok(HttpResponseOk(ApiUserLinkRequestResponse {
-                token: token.key(),
+                token: token.key().into(),
             }))
         } else {
             tracing::info!(caller = ?caller.id, provider = ?provider.id, provider_user = ?provider.api_user_id, "User does not have permission to modify this provider");

rfd-api/src/endpoints/login/mod.rs

@@ -161,4 +161,4 @@ pub trait UserInfoProvider {
         client: &reqwest::Client,
         token: &str,
     ) -> Result<UserInfo, UserInfoError>;
-}
+}

rfd-api/src/endpoints/login/oauth/client.rs

@@ -14,7 +14,7 @@ use crate::{
     error::ApiError,
     permissions::ApiPermission,
     util::response::{client_error, to_internal_error},
-    ApiCaller,
+    ApiCaller, secrets::OpenApiSecretString,
 };
 
 /// List OAuth clients
@@ -139,7 +139,7 @@ pub struct AddOAuthClientSecretPath {
 #[derive(Debug, Serialize, JsonSchema)]
 pub struct InitialOAuthClientSecretResponse {
     pub id: Uuid,
-    pub key: String,
+    pub key: OpenApiSecretString,
     pub created_at: DateTime<Utc>,
 }
 
@@ -179,7 +179,7 @@ async fn create_oauth_client_secret_op(
 
         Ok(HttpResponseOk(InitialOAuthClientSecretResponse {
             id: client_secret.id,
-            key: secret.key(),
+            key: secret.key().into(),
             created_at: client_secret.created_at,
         }))
     } else {
@@ -424,7 +424,7 @@ mod tests {
             .secrets
             .push(last_stored_secret.lock().unwrap().clone().unwrap());
 
-        let key = RawApiKey::try_from(secret.key.as_str()).unwrap();
+        let key = RawApiKey::try_from(&secret.key.0).unwrap();
 
         assert!(client.is_secret_valid(&key, &*ctx.secrets.signer))
     }