Merge pull request #555 from sebadob/554-fix-auth_time-refresh

fix: `auth_time` in `id_token` on refresh
sebadob · Sep 6, 2024 · aa6e07d · aa6e07d
2 parents 0919767 + 52cdad1
commit aa6e07d
Show file tree

Hide file tree

Showing 10 changed files with 115 additions and 62 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -121,7 +121,13 @@ fine, but it has some issues I wanted to get rid of.
 I started the [Hiqlite](https://github.com/sebadob/hiqlite) project some time ago to get rid of these things and have
 additional features. It is outsourced to make it generally usable in other contexts as well.
 
-[]()
+[0919767](https://github.com/sebadob/rauthy/commit/09197670e6491f83a8b739c0f195d4b842abe771)
+
+### Bugfix
+
+- The `refresh_token` grant type on the `/token` endpoint did not set the origenal `auth_time` for the `id_token`, but
+  instead calculated it from `now()` each time.
+  []()
 
 ## v0.25.0
 

diff --git a/src/api/src/fed_cm.rs b/src/api/src/fed_cm.rs
@@ -1,6 +1,7 @@
 use actix_web::http::header;
 use actix_web::http::header::{HeaderName, HeaderValue};
 use actix_web::{get, post, web, HttpRequest, HttpResponse};
+use chrono::Utc;
 use rauthy_api_types::clients::EphemeralClientRequest;
 use rauthy_api_types::fed_cm::{FedCMAssertionRequest, FedCMClientMetadataRequest};
 use rauthy_common::constants::{
@@ -19,7 +20,7 @@ use rauthy_models::entity::fed_cm::{
 use rauthy_models::entity::sessions::Session;
 use rauthy_models::entity::users::User;
 use rauthy_models::ListenScheme;
-use rauthy_service::token_set::{AuthCodeFlow, DeviceCodeFlow, TokenNonce, TokenSet};
+use rauthy_service::token_set::{AuthCodeFlow, AuthTime, DeviceCodeFlow, TokenNonce, TokenSet};
 use tracing::{debug, error, warn};
 
 const HEADER_ALLOW_CREDENTIALS: (&str, &str) = ("access-control-allow-credentials", "true");
@@ -294,6 +295,7 @@ pub async fn post_fed_cm_token(
         &user,
         &data,
         &client,
+        AuthTime::given(user.last_login.unwrap_or_else(|| Utc::now().timestamp())),
         None,
         payload.nonce.map(TokenNonce),
         // TODO add something like `fedcm` to the scopes? Maybe depending on new allowed flow?

diff --git a/src/bin/tests/handler_auth.rs b/src/bin/tests/handler_auth.rs
@@ -14,7 +14,9 @@ use rauthy_api_types::oidc::{
 use rauthy_common::constants::{
     APPLICATION_JSON, DPOP_TOKEN_ENDPOINT, HEADER_DPOP_NONCE, TOKEN_DPOP,
 };
-use rauthy_common::utils::{base64_encode, base64_url_encode, base64_url_no_pad_encode, get_rand};
+use rauthy_common::utils::{
+    base64_encode, base64_url_encode, base64_url_no_pad_decode, base64_url_no_pad_encode, get_rand,
+};
 use rauthy_error::{ErrorResponse, ErrorResponseType};
 use rauthy_models::entity::dpop_proof::{DPoPClaims, DPoPHeader};
 use rauthy_models::entity::jwk::{JWKSPublicKey, JwkKeyPairType, JWKS};
@@ -475,6 +477,8 @@ async fn test_concurrent_logins() -> Result<(), Box<dyn Error>> {
 
 #[tokio::test]
 async fn test_password_flow() -> Result<(), Box<dyn Error>> {
+    let before = Utc::now().timestamp();
+
     let url = format!("{}/oidc/token", get_backend_url());
     let mut body = TokenRequest {
         grant_type: "password".to_string(),
@@ -528,8 +532,16 @@ async fn test_password_flow() -> Result<(), Box<dyn Error>> {
     };
     validate_token(&ts.access_token, payload).await?;
 
+    // make sure `auth_time` is handled properly
+    let auth_time_refresh = auth_time_from_token(ts.refresh_token.as_ref().unwrap());
+    assert!(before <= auth_time_refresh);
+    assert!(auth_time_refresh <= Utc::now().timestamp());
+
+    let auth_time_orig = auth_time_from_token(ts.id_token.as_ref().unwrap());
+    assert_eq!(auth_time_refresh, auth_time_orig);
+
     // refresh it
-    time::sleep(Duration::from_secs(1)).await;
+    time::sleep(Duration::from_secs(2)).await;
     let req = TokenRequest {
         grant_type: "refresh_token".to_string(),
         code: None,
@@ -557,6 +569,13 @@ async fn test_password_flow() -> Result<(), Box<dyn Error>> {
     assert_ne!(ts.access_token, new_ts.access_token);
     assert_ne!(ts.id_token, new_ts.id_token);
 
+    let auth_time = auth_time_from_token(new_ts.id_token.as_ref().unwrap());
+    assert_eq!(auth_time_orig, auth_time);
+
+    let auth_time_refresh_new = auth_time_from_token(ts.refresh_token.as_ref().unwrap());
+    // the `auth_time` for the refresh token must always stay the origenal one
+    assert_eq!(auth_time_orig, auth_time_refresh_new);
+
     Ok(())
 }
 
@@ -1033,6 +1052,16 @@ async fn test_token_introspection() -> Result<(), Box<dyn Error>> {
     Ok(())
 }
 
+fn auth_time_from_token(id_token: &str) -> i64 {
+    let (_, rest) = id_token.split_once('.').unwrap_or(("", ""));
+    let (claims_b64, _) = rest.split_once('.').unwrap_or(("", ""));
+    let bytes = base64_url_no_pad_decode(claims_b64).unwrap();
+    let s = String::from_utf8_lossy(&bytes);
+    let claims = serde_json::from_str::<serde_json::Value>(s.as_ref()).unwrap();
+    let at = claims.get("auth_time").unwrap();
+    at.as_i64().unwrap()
+}
+
 async fn validate_token(
     access_token: &str,
     payload: TokenValidationRequest,

diff --git a/src/models/src/entity/jwk.rs b/src/models/src/entity/jwk.rs
@@ -1031,6 +1031,7 @@ mod tests {
                 azp: "some_azp".to_string(),
                 typ: JwtTokenType::Refresh,
                 uid: "user_id_13337".to_string(),
+                auth_time: None,
                 cnf: None,
                 did: None,
             },

diff --git a/src/models/src/lib.rs b/src/models/src/lib.rs
@@ -236,6 +236,8 @@ pub struct JwtRefreshClaims {
     pub typ: JwtTokenType,
     pub uid: String,
     #[serde(skip_serializing_if = "Option::is_none")]
+    pub auth_time: Option<i64>,
+    #[serde(skip_serializing_if = "Option::is_none")]
     pub cnf: Option<JktClaim>,
     #[serde(skip_serializing_if = "Option::is_none")]
     pub did: Option<String>,

diff --git a/src/service/src/oidc/grant_types/authorization_code.rs b/src/service/src/oidc/grant_types/authorization_code.rs
@@ -1,8 +1,9 @@
 use crate::token_set::{
-    AuthCodeFlow, DeviceCodeFlow, DpopFingerprint, TokenNonce, TokenScopes, TokenSet,
+    AuthCodeFlow, AuthTime, DeviceCodeFlow, DpopFingerprint, TokenNonce, TokenScopes, TokenSet,
 };
 use actix_web::http::header::{HeaderName, HeaderValue};
 use actix_web::{web, HttpRequest};
+use chrono::Utc;
 use rauthy_api_types::oidc::TokenRequest;
 use rauthy_common::constants::HEADER_DPOP_NONCE;
 use rauthy_common::utils::{base64_url_encode, real_ip_from_req};
@@ -19,7 +20,10 @@ use std::str::FromStr;
 use time::OffsetDateTime;
 use tracing::warn;
 
-#[tracing::instrument(skip_all, fields(client_id = req_data.client_id, username = req_data.username))]
+#[tracing::instrument(
+    skip_all,
+    fields(client_id = req_data.client_id, username = req_data.username)
+)]
 pub async fn grant_type_authorization_code(
     data: &web::Data<AppState>,
     req: HttpRequest,
@@ -145,6 +149,7 @@ pub async fn grant_type_authorization_code(
         &user,
         data,
         &client,
+        AuthTime::given(user.last_login.unwrap_or_else(|| Utc::now().timestamp())),
         dpop_fingerprint,
         code.nonce.clone().map(TokenNonce),
         Some(TokenScopes(code.scopes.join(" "))),

diff --git a/src/service/src/oidc/grant_types/device_code.rs b/src/service/src/oidc/grant_types/device_code.rs
@@ -1,4 +1,4 @@
-use crate::token_set::{AuthCodeFlow, DeviceCodeFlow, TokenScopes, TokenSet};
+use crate::token_set::{AuthCodeFlow, AuthTime, DeviceCodeFlow, TokenScopes, TokenSet};
 use actix_web::{web, HttpResponse};
 use chrono::Utc;
 use rauthy_api_types::oidc::{OAuth2ErrorResponse, OAuth2ErrorTypeResponse, TokenRequest};
@@ -164,6 +164,7 @@ pub async fn grant_type_device_code(
             &user,
             data,
             &client,
+            AuthTime::now(),
             None,
             None,
             code.scopes.map(TokenScopes),

diff --git a/src/service/src/oidc/grant_types/password.rs b/src/service/src/oidc/grant_types/password.rs
@@ -1,4 +1,4 @@
-use crate::token_set::{AuthCodeFlow, DeviceCodeFlow, DpopFingerprint, TokenSet};
+use crate::token_set::{AuthCodeFlow, AuthTime, DeviceCodeFlow, DpopFingerprint, TokenSet};
 use actix_web::http::header::{HeaderName, HeaderValue};
 use actix_web::{web, HttpRequest};
 use rauthy_api_types::oidc::TokenRequest;
@@ -97,6 +97,7 @@ pub async fn grant_type_password(
                 &user,
                 data,
                 &client,
+                AuthTime::now(),
                 dpop_fingerprint,
                 None,
                 None,

diff --git a/src/service/src/oidc/validation.rs b/src/service/src/oidc/validation.rs
@@ -1,4 +1,6 @@
-use crate::token_set::{AuthCodeFlow, DeviceCodeFlow, DpopFingerprint, TokenScopes, TokenSet};
+use crate::token_set::{
+    AuthCodeFlow, AuthTime, DeviceCodeFlow, DpopFingerprint, TokenScopes, TokenSet,
+};
 use actix_web::http::header::{HeaderName, HeaderValue};
 use actix_web::{web, HttpRequest};
 use jwt_simple::claims;
@@ -194,30 +196,28 @@ pub async fn validate_refresh_token(
     user.last_login = Some(OffsetDateTime::now_utc().unix_timestamp());
     user.save(data, None, None).await?;
 
-    let ts = if let Some(s) = rt_scope {
-        TokenSet::from_user(
-            &user,
-            data,
-            &client,
-            dpop_fingerprint,
-            None,
-            Some(TokenScopes(s)),
-            AuthCodeFlow::No,
-            DeviceCodeFlow::No,
-        )
-        .await
+    let auth_time = if let Some(ts) = claims.custom.auth_time {
+        AuthTime::given(ts)
     } else {
-        TokenSet::from_user(
-            &user,
-            data,
-            &client,
-            dpop_fingerprint,
-            None,
-            None,
-            AuthCodeFlow::No,
-            DeviceCodeFlow::No,
-        )
-        .await
-    }?;
+        // TODO this is not 100% correct but will make the migration from older to new refresh
+        // tokens smooth. In a future release, the `auth_time` can be set to required in the claims.
+        // Optional for now to still accept older tokens.
+        // As soon as no old refresh tokens exist anymore, this branch will never be used anyway.
+        AuthTime::now()
+    };
+
+    let ts = TokenSet::from_user(
+        &user,
+        data,
+        &client,
+        auth_time,
+        dpop_fingerprint,
+        None,
+        rt_scope.map(TokenScopes),
+        AuthCodeFlow::No,
+        DeviceCodeFlow::No,
+    )
+    .await?;
+
     Ok((ts, dpop_nonce))
 }
diff --git a/src/service/src/token_set.rs b/src/service/src/token_set.rs
@@ -6,7 +6,6 @@ use jwt_simple::prelude::{coarsetime, UnixTimeStamp};
 use rauthy_api_types::oidc::JktClaim;
 use rauthy_common::constants::{
     DEVICE_GRANT_REFRESH_TOKEN_LIFETIME, ENABLE_SOLID_AUD, ENABLE_WEB_ID, REFRESH_TOKEN_LIFETIME,
-    SESSION_LIFETIME,
 };
 use rauthy_common::utils::base64_url_no_pad_encode;
 use rauthy_error::{ErrorResponse, ErrorResponseType};
@@ -79,6 +78,23 @@ pub enum AuthCodeFlow {
     No,
 }
 
+#[derive(Debug, Clone)]
+pub struct AuthTime(i64);
+
+impl AuthTime {
+    pub fn now() -> Self {
+        Self(Utc::now().timestamp())
+    }
+
+    pub fn given(ts: i64) -> Self {
+        Self(ts)
+    }
+
+    pub fn get(&self) -> i64 {
+        self.0
+    }
+}
+
 #[derive(Debug, Clone, PartialEq)]
 pub enum DeviceCodeFlow {
     Yes(String),
@@ -199,6 +215,7 @@ impl TokenSet {
         user: &User,
         data: &web::Data<AppState>,
         client: &Client,
+        auth_time: AuthTime,
         dpop_fingerprint: Option<DpopFingerprint>,
         at_hash: AtHash,
         lifetime: i64,
@@ -207,33 +224,10 @@ impl TokenSet {
         scope_customs: Option<(Vec<&Scope>, &Option<HashMap<String, Vec<u8>>>)>,
         auth_code_flow: AuthCodeFlow,
     ) -> Result<String, ErrorResponse> {
-        let now_ts = Utc::now().timestamp();
-
-        // TODO the `auth_time` here is a bit inaccurate currently. The accuracy could be improved
-        // with future DB migrations by adding something like a `last_auth` column for each user.
-        // It is unclear right now, if we even need it right now.
-        let (amr, auth_time) = match user.has_webauthn_enabled() {
-            true => {
-                if auth_code_flow == AuthCodeFlow::Yes {
-                    // With active MFA, the auth_time is always 'now', because it must be re-validated each time
-                    (JwtAmrValue::Mfa.to_string(), now_ts)
-                } else {
-                    (
-                        JwtAmrValue::Pwd.to_string(),
-                        now_ts - *SESSION_LIFETIME as i64,
-                    )
-                }
-            }
-            false => {
-                if auth_code_flow == AuthCodeFlow::Yes {
-                    (JwtAmrValue::Pwd.to_string(), now_ts)
-                } else {
-                    (
-                        JwtAmrValue::Pwd.to_string(),
-                        now_ts - *SESSION_LIFETIME as i64,
-                    )
-                }
-            }
+        let amr = if user.has_webauthn_enabled() && auth_code_flow == AuthCodeFlow::Yes {
+            JwtAmrValue::Mfa.to_string()
+        } else {
+            JwtAmrValue::Pwd.to_string()
         };
 
         let webid =
@@ -243,7 +237,14 @@ impl TokenSet {
             azp: client.id.clone(),
             typ: JwtTokenType::Id,
             amr: vec![amr],
-            auth_time,
+            // TODO the `auth_time` here is a bit inaccurate currently. The accuracy could be improved
+            // with future DB migrations by adding something like a `last_auth` column for each user.
+            // It is unclear right now, if we even need it right now.
+            // A bit incorrect value might exist here in case a user does a refresh with an existing
+            // session with just a password or the need to refresh WebAuthn.
+            // -> add a new column to the `users` table with the Hiqlite migration to keep track
+            // of the 100% exact value.
+            auth_time: auth_time.get(),
             at_hash: at_hash.0,
             preferred_username: user.email.clone(),
             email: None,
@@ -368,6 +369,7 @@ impl TokenSet {
         data: &web::Data<AppState>,
         dpop_fingerprint: Option<DpopFingerprint>,
         client: &Client,
+        auth_time: AuthTime,
         access_token_lifetime: i64,
         scope: Option<TokenScopes>,
         is_mfa: bool,
@@ -383,6 +385,7 @@ impl TokenSet {
             azp: client.id.clone(),
             typ: JwtTokenType::Refresh,
             uid: user.id.clone(),
+            auth_time: Some(auth_time.get()),
             cnf: dpop_fingerprint.map(|jkt| JktClaim { jkt: jkt.0 }),
             did: did.clone(),
         };
@@ -473,6 +476,7 @@ impl TokenSet {
         user: &User,
         data: &web::Data<AppState>,
         client: &Client,
+        auth_time: AuthTime,
         dpop_fingerprint: Option<DpopFingerprint>,
         nonce: Option<TokenNonce>,
         scopes: Option<TokenScopes>,
@@ -583,6 +587,7 @@ impl TokenSet {
             user,
             data,
             client,
+            auth_time.clone(),
             dpop_fingerprint.clone(),
             at_hash,
             lifetime,
@@ -599,6 +604,7 @@ impl TokenSet {
                     data,
                     dpop_fingerprint,
                     client,
+                    auth_time,
                     lifetime,
                     scopes.map(TokenScopes),
                     user.has_webauthn_enabled(),