diff --git a/src/auth.rs b/src/auth.rs index ac68c74..61c4370 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -213,6 +213,10 @@ impl AuthHandler { /// /// This is called when creating a new backup with fresh factors or when adding a new `Sync` or `Main` factor to an existing backup. /// + /// `consume_oidc_nonce` should normally be `true`. Pass `false` when the same OIDC ID token / + /// session keypair already had its nonce marked used earlier in this request (same-account + /// add-factor upgrade using one sign-in). + /// /// # Errors /// Returns error if the factor is not valid, or is improperly authenticated (following each factor type's specific rules). pub async fn validate_factor_registration( @@ -222,6 +226,7 @@ impl AuthHandler { expected_challenge_context: ChallengeContext, turnkey_provider_id: Option, is_sync_factor: bool, + consume_oidc_nonce: bool, ) -> Result { // Step 1: Verify that the authorization type is valid for the factor scope // Sync factors must be EC keypairs - passkeys and OIDC accounts are not allowed as sync factors @@ -267,6 +272,7 @@ impl AuthHandler { signature, &challenge_token_payload, turnkey_provider_id.ok_or_else(|| AuthError::MissingTurnkeyProviderId)?, + consume_oidc_nonce, ) .await? } @@ -439,10 +445,11 @@ impl AuthHandler { signature: &str, challenge_token_payload: &[u8], turnkey_provider_id: String, + consume_oidc_nonce: bool, ) -> Result<(Factor, FactorToLookup), AuthError> { let claims = self .oidc_token_verifier - .verify_token(oidc_token, public_key.to_string()) + .verify_token(oidc_token, public_key.to_string(), consume_oidc_nonce) .await?; verify_signature(public_key, signature, challenge_token_payload)?; @@ -489,7 +496,7 @@ impl AuthHandler { ) -> Result<(String, BackupMetadata), AuthError> { let claims = self .oidc_token_verifier - .verify_token(oidc_token, public_key.to_string()) + .verify_token(oidc_token, public_key.to_string(), true) .await?; verify_signature(public_key, signature, challenge_token_payload)?; diff --git a/src/oidc_token_verifier.rs b/src/oidc_token_verifier.rs index f442e4a..5bd6829 100644 --- a/src/oidc_token_verifier.rs +++ b/src/oidc_token_verifier.rs @@ -99,10 +99,15 @@ impl OidcTokenVerifier { /// /// # Errors /// - `OidcTokenVerifierError`s will be raised if the token is not valid or the nonce has been used before. + /// + /// Set `consume_nonce` to `false` when the nonce was already marked used earlier in the same + /// request (e.g. same OIDC session authorizing both existing and new factor sides of add-factor). + /// Cryptographic nonce↔session-key binding is still verified. pub async fn verify_token( &self, token: &OidcToken, expected_public_key_sec1_base64: String, + consume_nonce: bool, ) -> Result, OidcTokenVerifierError> { // Step 1: Extract the token and other parameters based on the OIDC provider let (oidc_token, jwk_set_url, client_id, issuer_url) = match token { @@ -149,15 +154,19 @@ impl OidcTokenVerifier { } })?; - // Step 6: Track the nonce to prevent replays - let nonce = claims - .nonce() - .ok_or(OidcTokenVerifierError::MissingNonce)? - .secret(); - - self.redis_cache_manager - .use_oidc_nonce(nonce, &token.into()) - .await?; + // Step 6: Track the nonce to prevent replays (unless already consumed in this request). + if consume_nonce { + let nonce = claims + .nonce() + .ok_or(OidcTokenVerifierError::MissingNonce)? + .secret(); + + self.redis_cache_manager + .use_oidc_nonce(nonce, &token.into()) + .await?; + } else if claims.nonce().is_none() { + return Err(OidcTokenVerifierError::MissingNonce); + } Ok(claims.clone()) } @@ -259,13 +268,13 @@ mod tests { match provider { OidcProvider::Google => { verifier - .verify_token(&OidcToken::Google { token }, public_key) + .verify_token(&OidcToken::Google { token }, public_key, true) .await } OidcProvider::Apple => { verifier // Use the default - .verify_token(&OidcToken::Apple { token, aud }, public_key) + .verify_token(&OidcToken::Apple { token, aud }, public_key, true) .await } } @@ -354,7 +363,7 @@ mod tests { let oidc_token: OidcToken = serde_json::from_str(&json).unwrap(); assert!(matches!(oidc_token, OidcToken::Apple { aud: None, .. })); - let result = verifier.verify_token(&oidc_token, public_key).await; + let result = verifier.verify_token(&oidc_token, public_key, true).await; assert!(result.is_ok()); } diff --git a/src/routes/add_factor.rs b/src/routes/add_factor.rs index 966b7df..5a0c220 100644 --- a/src/routes/add_factor.rs +++ b/src/routes/add_factor.rs @@ -8,7 +8,7 @@ use crate::redis_cache::RedisCacheManager; use crate::turnkey_activity::{ verify_turnkey_activity_parameters, verify_turnkey_activity_webauthn_stamp, }; -use crate::types::backup_metadata::{ExportedBackupMetadata, FactorKind}; +use crate::types::backup_metadata::{BackupMetadata, ExportedBackupMetadata, FactorKind}; use crate::types::encryption_key::BackupEncryptionKey; use crate::types::{Authorization, ErrorResponse}; use crate::webauthn::TryFromValue; @@ -314,7 +314,40 @@ pub async fn handler( } } - // Step 2A.2: Use AuthHandler to validate the new factor + // Step 2A.2: Use AuthHandler to validate the new factor. + // When the same OIDC ID token + session keypair authorize both sides (same-account + // metadata-only upgrade), the existing-factor verify already consumed the nonce — skip a + // second Redis mark so registration can still verify the new-factor challenge signature. + // Compare raw JWT (+ session key), not full `OidcToken`, so Apple `aud: None` vs explicit + // default does not block reuse of the already-consumed nonce. + let reuse_same_oidc_session = match ( + &request.existing_factor_authorization, + &request.new_factor_authorization, + ) { + ( + Authorization::OidcAccount { + oidc_token: existing_token, + public_key: existing_pk, + .. + }, + Authorization::OidcAccount { + oidc_token: new_token, + public_key: new_pk, + .. + }, + ) => { + let existing_raw = match existing_token { + crate::types::OidcToken::Google { token } + | crate::types::OidcToken::Apple { token, .. } => token.as_str(), + }; + let new_raw = match new_token { + crate::types::OidcToken::Google { token } + | crate::types::OidcToken::Apple { token, .. } => token.as_str(), + }; + existing_raw == new_raw && existing_pk == new_pk + } + _ => false, + }; let validation_result = auth_handler .validate_factor_registration( &request.new_factor_authorization, @@ -322,6 +355,7 @@ pub async fn handler( ChallengeContext::AddFactorByNewFactor {}, request.turnkey_provider_id.clone(), false, // not a sync factor + !reuse_same_oidc_session, ) .await?; @@ -398,12 +432,9 @@ pub async fn handler( else { return Err(BackupManagerError::BackupNotFound.into()); }; - let Some(factor_id) = metadata - .factors - .iter() - .find(|f| f.kind == new_factor_kind) - .map(|f| f.id.clone()) - else { + // Require the stored factor still exist. A concurrent delete can remove it between the + // AlreadyExists race and this read — do not invent an ID or restore a stale lookup. + let Some(factor_id) = stored_main_factor_id(&metadata, &new_factor_kind) else { tracing::warn!( message = "FactorAlreadyExists reconcile found no matching factor in metadata", factor_pk = factor_to_lookup.primary_key(), @@ -422,7 +453,9 @@ pub async fn handler( } return Err(BackupManagerError::FactorNotFound.into()); }; - // Do not roll back lookup: factor is present in metadata; keeping lookup can heal inconsistency. + // Factor is in metadata — ensure lookup still maps here (a concurrent inserter may have + // rolled back the shared row after we adopted it). + ensure_main_factor_lookup(&factor_lookup, &factor_to_lookup, &backup_id).await?; return Ok(Json(AddFactorResponse { factor_id, backup_metadata: metadata.exported(), @@ -432,10 +465,9 @@ pub async fn handler( // Step 3.3: Roll back FactorLookup only when we inserted this request's row and the metadata // write definitely did not land (`NotInserted`). // - // Another concurrent request may have adopted this lookup row (same-backup - // ConditionalCheckFailed) and successfully written the factor while our write failed - // (e.g. encryption-key conflict). Re-check metadata before delete so we do not orphan - // that other request's factor. + // Another concurrent request may adopt this lookup row (same-backup ConditionalCheckFailed) + // and write the factor around our rollback. Skip delete if the factor is already present; + // after delete, re-check and re-insert (heal) if it appeared in the race window. if lookup_insert_succeeded && write.should_rollback_lookup() { let factor_still_absent = match backup_storage.get_metadata_by_backup_id(&backup_id).await { Ok(Some((metadata, _))) => !metadata.factors.iter().any(|f| f.kind == new_factor_kind), @@ -455,8 +487,22 @@ pub async fn handler( .delete(FactorScope::Main, &factor_to_lookup) .await { + // Delete may still have applied despite a timeout/dispatch error — continue to + // heal so we do not leave a concurrent successful writer untraceable. tracing::error!(message = "Failed to delete factor from lookup table after failed factor addition.", error = ?delete_err, factor_pk = factor_to_lookup.primary_key()); } + + // Heal whether delete returned Ok or Err: a concurrent writer may have committed the + // factor around this rollback, and an ambiguous delete response may have removed a + // lookup that writer had just restored. + heal_main_factor_lookup_if_present( + &backup_storage, + &factor_lookup, + &factor_to_lookup, + &backup_id, + &new_factor_kind, + ) + .await; } else { tracing::info!( message = @@ -468,9 +514,234 @@ pub async fn handler( let updated_metadata = write.into_result()?; + // Successful writer always verifies lookup: a concurrent request that inserted the row may + // still roll it back around our metadata commit. + ensure_main_factor_lookup(&factor_lookup, &factor_to_lookup, &backup_id).await?; + // Step 4: Return the new factor ID and the updated backup metadata Ok(Json(AddFactorResponse { factor_id: new_factor.id, backup_metadata: updated_metadata.exported(), })) } + +/// After a lookup rollback delete (or ambiguous delete error), restore the row if metadata now +/// contains the factor — covering concurrent successful writers and lost delete ACKs. +async fn heal_main_factor_lookup_if_present( + backup_storage: &BackupStorage, + factor_lookup: &FactorLookup, + factor_to_lookup: &FactorToLookup, + backup_id: &str, + new_factor_kind: &FactorKind, +) { + let needs_heal = match backup_storage.get_metadata_by_backup_id(backup_id).await { + Ok(Some((metadata, _))) => metadata.factors.iter().any(|f| f.kind == *new_factor_kind), + Ok(None) => false, + Err(err) => { + tracing::error!( + message = "Failed to re-read metadata after lookup rollback; cannot heal", + error = ?err, + factor_pk = factor_to_lookup.primary_key(), + ); + false + } + }; + + if !needs_heal { + return; + } + + match factor_lookup + .insert(FactorScope::Main, factor_to_lookup, backup_id.to_string()) + .await + { + Ok(()) => { + tracing::info!( + message = "Healed FactorLookup after concurrent factor write during rollback", + factor_pk = factor_to_lookup.primary_key(), + ); + } + Err(FactorLookupError::DynamoDbPutError(ref sdk_err)) + if matches!( + sdk_err, + aws_sdk_dynamodb::error::SdkError::ServiceError(inner) + if inner.err().is_conditional_check_failed_exception() + ) => + { + // Confirm the existing row maps to this backup — ConditionalCheckFailed alone can mean + // another backup now owns the factor identity. + match factor_lookup + .lookup_consistent(FactorScope::Main, factor_to_lookup) + .await + { + Ok(Some(existing)) if existing == backup_id => {} + Ok(Some(other_backup_id)) => { + tracing::error!( + message = "Heal skipped: FactorLookup maps factor to a different backup", + factor_pk = factor_to_lookup.primary_key(), + expected_backup_id = backup_id, + actual_backup_id = other_backup_id.as_str(), + ); + } + Ok(None) => { + // Row vanished between ConditionalCheckFailed and read — retry once. + if let Err(err) = factor_lookup + .insert(FactorScope::Main, factor_to_lookup, backup_id.to_string()) + .await + { + tracing::error!( + message = "Failed to heal FactorLookup after row disappeared during reconcile", + error = ?err, + factor_pk = factor_to_lookup.primary_key(), + ); + } + } + Err(err) => { + tracing::error!( + message = "Failed consistent FactorLookup read during heal reconcile", + error = ?err, + factor_pk = factor_to_lookup.primary_key(), + ); + } + } + } + Err(err) => { + tracing::error!( + message = "Failed to heal FactorLookup after concurrent factor write during rollback", + error = ?err, + factor_pk = factor_to_lookup.primary_key(), + ); + } + } +} + +/// Ensures `FactorLookup` maps this factor to `backup_id` after metadata was written successfully. +/// +/// Closes the race where another request inserted the lookup, we adopted it, wrote metadata, and +/// that other request then deleted the row during its `NotInserted` rollback. +async fn ensure_main_factor_lookup( + factor_lookup: &FactorLookup, + factor_to_lookup: &FactorToLookup, + backup_id: &str, +) -> Result<(), ErrorResponse> { + match factor_lookup + .insert(FactorScope::Main, factor_to_lookup, backup_id.to_string()) + .await + { + Ok(()) => { + tracing::info!( + message = "Restored FactorLookup after successful factor write", + factor_pk = factor_to_lookup.primary_key(), + ); + Ok(()) + } + Err(FactorLookupError::DynamoDbPutError(ref sdk_err)) + if matches!( + sdk_err, + aws_sdk_dynamodb::error::SdkError::ServiceError(inner) + if inner.err().is_conditional_check_failed_exception() + ) => + { + match factor_lookup + .lookup_consistent(FactorScope::Main, factor_to_lookup) + .await? + { + Some(existing) if existing == backup_id => Ok(()), + Some(_) => Err(ErrorResponse::bad_request( + "factor_already_exists", + "This factor already exists.", + )), + None => { + // Row disappeared between ConditionalCheckFailed and read (rollback race). + match factor_lookup + .insert(FactorScope::Main, factor_to_lookup, backup_id.to_string()) + .await + { + Ok(()) => Ok(()), + Err(FactorLookupError::DynamoDbPutError(ref sdk_err)) + if matches!( + sdk_err, + aws_sdk_dynamodb::error::SdkError::ServiceError(inner) + if inner.err().is_conditional_check_failed_exception() + ) => + { + match factor_lookup + .lookup_consistent(FactorScope::Main, factor_to_lookup) + .await? + { + Some(existing) if existing == backup_id => Ok(()), + _ => { + tracing::error!( + message = "Failed to ensure FactorLookup after successful factor write", + factor_pk = factor_to_lookup.primary_key(), + ); + Err(ErrorResponse::internal_server_error()) + } + } + } + Err(err) => Err(err.into()), + } + } + } + } + Err(err) => Err(err.into()), + } +} + +/// Returns the stored main-factor id for `kind`, if present. +fn stored_main_factor_id(metadata: &BackupMetadata, kind: &FactorKind) -> Option { + metadata + .factors + .iter() + .find(|f| f.kind == *kind) + .map(|f| f.id.clone()) +} + +#[cfg(test)] +mod tests { + use super::stored_main_factor_id; + use crate::types::backup_metadata::{BackupMetadata, Factor, FactorKind, OidcAccountKind}; + + #[test] + fn stored_main_factor_id_returns_none_when_factor_absent() { + let metadata = BackupMetadata { + id: "backup".to_string(), + factors: vec![], + sync_factors: vec![], + keys: vec![], + manifest_hash: hex::encode([1u8; 32]), + }; + let kind = FactorKind::OidcAccount { + account: OidcAccountKind::Google { + sub: "sub".to_string(), + masked_email: "a****@b.com".to_string(), + }, + turnkey_provider_id: "tp".to_string(), + }; + assert!(stored_main_factor_id(&metadata, &kind).is_none()); + } + + #[test] + fn stored_main_factor_id_returns_persisted_id_when_present() { + let factor = Factor::new_oidc_account( + OidcAccountKind::Google { + sub: "sub".to_string(), + masked_email: "a****@b.com".to_string(), + }, + "tp".to_string(), + ); + let expected_id = factor.id.clone(); + let kind = factor.kind.clone(); + let metadata = BackupMetadata { + id: "backup".to_string(), + factors: vec![factor], + sync_factors: vec![], + keys: vec![], + manifest_hash: hex::encode([1u8; 32]), + }; + assert_eq!( + stored_main_factor_id(&metadata, &kind).as_deref(), + Some(expected_id.as_str()) + ); + } +} diff --git a/src/routes/add_sync_factor.rs b/src/routes/add_sync_factor.rs index d127714..27030bb 100644 --- a/src/routes/add_sync_factor.rs +++ b/src/routes/add_sync_factor.rs @@ -45,6 +45,7 @@ pub async fn handler( ChallengeContext::AddSyncFactor {}, None, true, // is_sync_factor + true, // consume OIDC nonce (N/A for EC sync factors) ) .await?; diff --git a/src/routes/create_backup.rs b/src/routes/create_backup.rs index 74105af..76bb7f7 100644 --- a/src/routes/create_backup.rs +++ b/src/routes/create_backup.rs @@ -106,6 +106,7 @@ pub async fn handler( ChallengeContext::Create {}, request.turnkey_provider_id.clone(), false, // not a sync factor + true, // consume OIDC nonce ) .await?; @@ -121,6 +122,7 @@ pub async fn handler( ChallengeContext::Create {}, None, true, // is a sync factor + true, // consume OIDC nonce (N/A for EC) ) .await?; diff --git a/tests/add_factor_happy_paths.rs b/tests/add_factor_happy_paths.rs index f15e135..da6154c 100644 --- a/tests/add_factor_happy_paths.rs +++ b/tests/add_factor_happy_paths.rs @@ -387,6 +387,91 @@ async fn test_add_factor_same_oidc_metadata_only_turnkey_upgrade() { assert_eq!(oidc_count, 1); } +// Same-account Turnkey upgrade with one OIDC ID token + session keypair for both sides. +#[tokio::test] +#[serial] +async fn test_add_factor_same_oidc_single_session_metadata_only_upgrade() { + let subject = format!("same-oidc-one-session-{}", Uuid::new_v4()); + let test = create_test_backup_with_oidc_account(&subject, b"BACKUP DATA").await; + assert_eq!(test.response.status(), StatusCode::OK); + let body = test + .response + .into_body() + .collect() + .await + .unwrap() + .to_bytes(); + let create_json: serde_json::Value = serde_json::from_slice(&body).unwrap(); + let backup_id = create_json["backupId"].as_str().unwrap(); + + let (session_public_key, session_secret_key) = crate::common::generate_keypair(); + let oidc_token = test.oidc_server.generate_token( + &backup_service_test_utils::MockOidcProvider::Google, + Some(openidconnect::SubjectIdentifier::new(subject)), + &session_public_key, + ); + + let challenges = get_add_factor_challenges_generic( + json!({ "kind": "OIDC_ACCOUNT", "oidcToken": oidc_token }), + Some("OIDC_ACCOUNT"), + ) + .await; + + let existing_sig = crate::common::sign_keypair_challenge( + &session_secret_key, + challenges["existingFactorChallenge"].as_str().unwrap(), + ); + let new_sig = crate::common::sign_keypair_challenge( + &session_secret_key, + challenges["newFactorChallenge"].as_str().unwrap(), + ); + + let response = send_post_request_with_environment( + "/v1/add-factor", + json!({ + "existingFactorAuthorization": { + "kind": "OIDC_ACCOUNT", + "oidcToken": { "kind": "GOOGLE", "token": oidc_token }, + "publicKey": session_public_key, + "signature": existing_sig, + }, + "existingFactorChallengeToken": challenges["existingFactorToken"], + "newFactorAuthorization": { + "kind": "OIDC_ACCOUNT", + "oidcToken": { "kind": "GOOGLE", "token": oidc_token }, + "publicKey": session_public_key, + "signature": new_sig, + }, + "newFactorChallengeToken": challenges["newFactorToken"], + "turnkeyProviderId": "turnkey_provider_id", + "encryptedBackupKey": { + "kind": "TURNKEY", + "encryptedKey": "ENCRYPTED_KEY", + "turnkeyAccountId": "org123", + "turnkeyUserId": "TURNKEY_USER_ID", + "turnkeyPrivateKeyId": "TURNKEY_PRIVATE_KEY_ID" + } + }), + Some(test.environment), + ) + .await; + + assert_eq!(response.status(), StatusCode::OK); + let metadata = verify_s3_metadata_exists(backup_id).await; + assert!(metadata["keys"] + .as_array() + .unwrap() + .iter() + .any(|k| k["kind"] == "TURNKEY")); + let oidc_count = metadata["factors"] + .as_array() + .unwrap() + .iter() + .filter(|f| f["kind"]["kind"] == "OIDC_ACCOUNT") + .count(); + assert_eq!(oidc_count, 1); +} + // Same OIDC account with a different Turnkey provider id must not create a duplicate factor. #[tokio::test] #[serial]