diff --git a/conformance/wire-contract.json b/conformance/wire-contract.json index 03d286a..a8adeca 100644 --- a/conformance/wire-contract.json +++ b/conformance/wire-contract.json @@ -21,7 +21,14 @@ "keyed with it. Three things are pinned at once: the ':' separator between label and", "secret, the SHA-256, and the fact that the HMAC is keyed with the hex TEXT of the digest", "rather than its raw bytes. Any of the three drifting produces different Redis keys for", - "every lockout, OTP, resend cooldown, MFA setup, and anti-replay record." + "every lockout, OTP, resend cooldown, MFA setup, and anti-replay record.", + "", + "The same vector continues into the OTP record. The stored value is never the code", + "itself — six digits is a keyspace of a million, reversible offline from a Redis dump —", + "but the hmac-sha256 of `{identifier}:{code}` under that derived key. Pinned here", + "because both implementations write and compare the SAME `otp:` record, so a drift is", + "not a parse error on the other side: it is every code minted on one backend failing to", + "verify on the other, in both directions." ], "label": "bymax-auth:hmac-key:v1", "vectors": [ @@ -29,7 +36,9 @@ "secret": "0123456789abcdef0123456789abcdef", "derivedKeyHex": "0dd66555bd2d89e0eb4ce050f1fef427bea6799bec27fb8e313f69ab965048c1", "identifierMessage": "tenant-a:user@example.com", - "identifierHex": "609a759522bd8b397748fad2dbde07957cea580fe4f4f1f0ce0f526485de2b6d" + "identifierHex": "609a759522bd8b397748fad2dbde07957cea580fe4f4f1f0ce0f526485de2b6d", + "otpCode": "123456", + "otpRecordCodeHex": "d3483e9ed6fe3d6fc54ab28ac5d69dc6a60399d3519fdc0de95dbe77b9065327" } ] }, @@ -353,6 +362,7 @@ "passwordHash": "PHC — see the passwordHashFormat section, which pins it by vector", "totpSecretAtRest": "aes-256-gcm over the BASE32 TEXT of the secret", "recoveryCodeDigest": "hex hmac-sha256 of the code under the derived identifier key", + "otpRecordCode": "hex hmac-sha256 of `{identifier}:{code}` under the derived identifier key", "wsTicket": "64 lowercase hex characters (32 CSPRNG bytes), single-use, 30 s lifetime" }, diff --git a/crates/bymax-auth-axum/tests/adapter.rs b/crates/bymax-auth-axum/tests/adapter.rs index 74e89dd..eb7ba54 100644 --- a/crates/bymax-auth-axum/tests/adapter.rs +++ b/crates/bymax-auth-axum/tests/adapter.rs @@ -2225,7 +2225,6 @@ async fn invitation_create_with_an_unknown_role_hits_the_error_arm() { async fn password_reset_otp_two_step_success_flow() { // forgot-password mints an OTP; verify-otp exchanges it for a verified token (success arm); // reset-password with that token succeeds (204). Uses the in-memory OTP peek. - use bymax_auth_core::traits::OtpPurpose; let Some(h) = build(EngineSpec::default()) else { return }; let app = router(&h); seed_user(&h, "pw@e.com", "glidingwalnut42", "USER").await; @@ -2236,12 +2235,20 @@ async fn password_reset_otp_two_step_success_flow() { .send(&app) .await; - // Recover the OTP from the in-memory store (the engine derives the identifier internally). - // Asserted rather than skipped: the OTP existing is what proves the route reached the + // Recovered from the mail the engine actually sent, which is where the code is. The stored + // record holds a keyed fingerprint, not the code, so reading the store back would hand the + // test the wrong string — and a leaked Redis dump would hand an attacker nothing, which is + // the point of storing it that way. + // + // Asserted rather than skipped: the code existing is what proves the route reached the // engine at all, and a skip here would pass just as happily against a handler that did // nothing but answer 200. - let otp = common::peek_otp(&h, OtpPurpose::PasswordReset, "pw@e.com").unwrap_or_default(); - assert!(!otp.is_empty(), "forgot-password minted no reset OTP"); + let otp = h + .emails + .await_password_reset_code() + .await + .unwrap_or_default(); + assert!(!otp.is_empty(), "forgot-password mailed no reset OTP"); let verify = Req::post("/auth/password/verify-otp") .json(serde_json::json!({ "email": "pw@e.com", "otp": otp, "tenantId": TENANT })) @@ -2650,7 +2657,6 @@ async fn oauth_callback_mfa_branch_without_redirect_returns_json() { #[tokio::test] async fn verify_email_success_with_a_live_otp() { // The verify-email happy path: a registered user with a real verification OTP verifies (204). - use bymax_auth_core::traits::OtpPurpose; let Some(h) = build(EngineSpec { verification_required: true, ..EngineSpec::default() @@ -2665,9 +2671,12 @@ async fn verify_email_success_with_a_live_otp() { })) .send(&app) .await; - let Some(otp) = common::peek_otp(&h, OtpPurpose::EmailVerification, "vfy@e.com") else { - return; - }; + // From the mail, not the store: the record holds a keyed fingerprint of the code. Awaited + // and asserted — the send is detached, and an `else { return }` on a lost race is a test + // that skips itself and reports success. + let mailed = h.emails.await_verification_code().await; + assert!(mailed.is_some(), "register mailed no verification code"); + let Some(otp) = mailed else { return }; let verify = Req::post("/auth/verify-email") .json(serde_json::json!({ "email": "vfy@e.com", "otp": otp, "tenantId": TENANT })) .send(&app) @@ -3633,3 +3642,71 @@ async fn mfa_management_error_arms_with_a_real_account() { assert_eq!(resp.json()["error"]["code"], code, "{path}"); } } + +#[tokio::test] +async fn the_capturing_mailer_records_the_codes_and_ignores_the_rest() { + // The harness's double keeps the two codes a flow has to submit back — the OTP record holds + // a keyed fingerprint, so the mailbox is the only place the plaintext exists — and no-ops + // the rest. Driven end to end so the object-safe impl is covered, and so the recording + // halves are asserted rather than assumed: a double that quietly kept nothing would make + // every test reading a code from it skip its own assertions and still pass. + use bymax_auth_core::traits::EmailProvider; + + let mailer = common::CapturingEmails::default(); + assert!(mailer.verification_code().is_none()); + assert!(mailer.password_reset_code().is_none()); + + let provider: &dyn EmailProvider = &mailer; + assert!( + provider + .send_email_verification_otp("t1", "u@e.com", "123456", None) + .await + .is_ok() + ); + assert!( + provider + .send_password_reset_otp("t1", "u@e.com", "654321", Some("pt-BR")) + .await + .is_ok() + ); + assert_eq!(mailer.verification_code().as_deref(), Some("123456")); + assert_eq!(mailer.password_reset_code().as_deref(), Some("654321")); + + // The rest carry nothing a test submits back, so they are no-ops. + assert!( + provider + .send_password_reset_token("t1", "u@e.com", "tok", None) + .await + .is_ok() + ); + assert!( + provider + .send_email_change_verification("t1", "new@e.com", "tok", None) + .await + .is_ok() + ); + assert!( + provider + .send_mfa_enabled("t1", "u@e.com", None) + .await + .is_ok() + ); + assert!( + provider + .send_mfa_disabled("t1", "u@e.com", None) + .await + .is_ok() + ); + let invite = bymax_auth_core::traits::InviteData { + inviter_name: "Owner".to_owned(), + tenant_name: "Acme".to_owned(), + invite_token: "0".repeat(64), + expires_at: time::OffsetDateTime::UNIX_EPOCH, + }; + assert!( + provider + .send_invitation("t1", "u@e.com", &invite, None) + .await + .is_ok() + ); +} diff --git a/crates/bymax-auth-axum/tests/common/mod.rs b/crates/bymax-auth-axum/tests/common/mod.rs index 3f2bb0d..0324754 100644 --- a/crates/bymax-auth-axum/tests/common/mod.rs +++ b/crates/bymax-auth-axum/tests/common/mod.rs @@ -124,6 +124,136 @@ pub fn hash_password(plain: &str) -> String { bymax_auth_crypto::password::hash(plain.as_bytes(), ¶ms).unwrap_or_default() } +/// An email provider that keeps the codes it was asked to send. +/// +/// The OTP record no longer holds the plaintext code — it holds a keyed fingerprint, so a test +/// cannot read the code back out of the store and never could once the store stopped being a +/// place a leak would hand an attacker the code. The recipient's mailbox is where the code +/// actually is, so that is where a test that drives the real flow has to get it. +#[derive(Default)] +pub struct CapturingEmails { + verification: Mutex>, + password_reset: Mutex>, +} + +impl CapturingEmails { + /// Wait for the detached send to deliver the verification code, up to a deadline. + /// + /// The engine mails fire-and-forget, so reading the mailbox the instant a route returns is + /// a race — and one a caller loses SILENTLY when it unwraps with `else { return }`. Polling + /// rather than a fixed sleep keeps it from becoming a flake on a slower runner. + pub async fn await_verification_code(&self) -> Option { + for _ in 0..40 { + if let Some(code) = self.verification_code() { + return Some(code); + } + tokio::time::sleep(std::time::Duration::from_millis(25)).await; + } + None + } + + /// The password-reset twin of [`Self::await_verification_code`]. + pub async fn await_password_reset_code(&self) -> Option { + for _ in 0..40 { + if let Some(code) = self.password_reset_code() { + return Some(code); + } + tokio::time::sleep(std::time::Duration::from_millis(25)).await; + } + None + } + + /// Take the last email-verification code sent, leaving the mailbox empty. + /// + /// Consuming, not peeking: a flow that mails twice would otherwise have the second read + /// answer instantly with the FIRST code, and the test would submit a stale one. + pub fn verification_code(&self) -> Option { + self.verification.lock().ok().and_then(|mut c| c.take()) + } + + /// Take the last password-reset code sent. See [`Self::verification_code`]. + pub fn password_reset_code(&self) -> Option { + self.password_reset.lock().ok().and_then(|mut c| c.take()) + } +} + +#[async_trait::async_trait] +impl bymax_auth_core::traits::EmailProvider for CapturingEmails { + async fn send_email_verification_otp( + &self, + _tenant_id: &str, + _email: &str, + otp: &str, + _locale: Option<&str>, + ) -> Result<(), bymax_auth_core::traits::EmailError> { + if let Ok(mut slot) = self.verification.lock() { + *slot = Some(otp.to_owned()); + } + Ok(()) + } + + async fn send_password_reset_otp( + &self, + _tenant_id: &str, + _email: &str, + otp: &str, + _locale: Option<&str>, + ) -> Result<(), bymax_auth_core::traits::EmailError> { + if let Ok(mut slot) = self.password_reset.lock() { + *slot = Some(otp.to_owned()); + } + Ok(()) + } + + async fn send_password_reset_token( + &self, + _tenant_id: &str, + _email: &str, + _token: &str, + _locale: Option<&str>, + ) -> Result<(), bymax_auth_core::traits::EmailError> { + Ok(()) + } + + async fn send_email_change_verification( + &self, + _tenant_id: &str, + _new_email: &str, + _token: &str, + _locale: Option<&str>, + ) -> Result<(), bymax_auth_core::traits::EmailError> { + Ok(()) + } + + async fn send_mfa_enabled( + &self, + _tenant_id: &str, + _email: &str, + _locale: Option<&str>, + ) -> Result<(), bymax_auth_core::traits::EmailError> { + Ok(()) + } + + async fn send_mfa_disabled( + &self, + _tenant_id: &str, + _email: &str, + _locale: Option<&str>, + ) -> Result<(), bymax_auth_core::traits::EmailError> { + Ok(()) + } + + async fn send_invitation( + &self, + _tenant_id: &str, + _email: &str, + _invite: &bymax_auth_core::traits::InviteData, + _locale: Option<&str>, + ) -> Result<(), bymax_auth_core::traits::EmailError> { + Ok(()) + } +} + /// The built engine plus the concrete in-memory collaborators a test seeds/inspects. pub struct Harness { pub engine: Arc, @@ -132,6 +262,8 @@ pub struct Harness { pub stores: Arc, /// The cookie-domain resolver, present only when `EngineSpec::cookie_domains` asked for one. pub domain_resolver: Option>, + /// The codes the engine actually mailed, for flows whose store holds only a fingerprint. + pub emails: Arc, } /// Build an engine + router over in-memory stores per `spec`. @@ -197,11 +329,13 @@ pub fn build(spec: EngineSpec) -> Option { ])); } + let emails = Arc::new(CapturingEmails::default()); let mut builder = AuthEngine::builder() .config(config) .environment(Environment::Test) .user_repository(users.clone()) .platform_user_repository(admins.clone()) + .email_provider(emails.clone()) .redis_stores(stores.clone()); if spec.oauth { @@ -220,6 +354,7 @@ pub fn build(spec: EngineSpec) -> Option { admins, stores, domain_resolver, + emails, }) } @@ -256,6 +391,7 @@ pub fn build_oauth_with_redirects() -> Option { admins, stores, domain_resolver: None, + emails: Arc::new(CapturingEmails::default()), }) } @@ -293,6 +429,7 @@ pub fn build_oauth_with_failing_state_store() -> Option { admins, stores: inert, domain_resolver: None, + emails: Arc::new(CapturingEmails::default()), }) } @@ -868,6 +1005,7 @@ pub fn build_failing() -> Option { admins, stores: inert, domain_resolver: None, + emails: Arc::new(CapturingEmails::default()), }) } @@ -903,20 +1041,10 @@ pub fn build_failing_blacklist() -> Option { admins, stores: inert, domain_resolver: None, + emails: Arc::new(CapturingEmails::default()), }) } -/// Peek the engine-generated OTP for a `(tenant, email)` pair under a purpose (the in-memory -/// store keeps the plaintext code so the reset/verify flows can be driven end to end). -pub fn peek_otp( - harness: &Harness, - purpose: bymax_auth_core::traits::OtpPurpose, - email: &str, -) -> Option { - let identifier = harness.engine.hashed_identifier_for(TENANT, email); - harness.stores.peek_otp(purpose, &identifier) -} - /// Seed an active platform admin with the given role; returns its id. pub async fn seed_admin(harness: &Harness, email: &str, role: &str) -> String { use bymax_auth_types::AuthPlatformUser; diff --git a/crates/bymax-auth-core/src/engine/builder.rs b/crates/bymax-auth-core/src/engine/builder.rs index 0bcca17..25ee0c3 100644 --- a/crates/bymax-auth-core/src/engine/builder.rs +++ b/crates/bymax-auth-core/src/engine/builder.rs @@ -462,7 +462,10 @@ impl AuthEngineBuilder { brute_max_attempts, brute_window_secs, )); - let otp = OtpService::new(otp_store.clone()); + let otp = OtpService::new( + otp_store.clone(), + zeroize::Zeroizing::new(*config.hmac_key()), + ); let sessions = Arc::new(SessionService::new( session_store.clone(), user_repository.clone(), diff --git a/crates/bymax-auth-core/src/services/auth/email_change.rs b/crates/bymax-auth-core/src/services/auth/email_change.rs index 7b8c028..453498a 100644 --- a/crates/bymax-auth-core/src/services/auth/email_change.rs +++ b/crates/bymax-auth-core/src/services/auth/email_change.rs @@ -385,6 +385,11 @@ mod tests { engine, users, stores, + // This harness deliberately wires a FAILING email provider, so nothing is + // captured; the field exists for the flows that read a mailed code back. + emails: std::sync::Arc::new( + crate::services::auth::test_support::CapturingEmails::default(), + ), }) } diff --git a/crates/bymax-auth-core/src/services/auth/email_verification.rs b/crates/bymax-auth-core/src/services/auth/email_verification.rs index 1d69a9c..3cc5b08 100644 --- a/crates/bymax-auth-core/src/services/auth/email_verification.rs +++ b/crates/bymax-auth-core/src/services/auth/email_verification.rs @@ -214,11 +214,20 @@ mod tests { .await .is_ok() ); - let identifier = h.engine.hashed_identifier("t1", "v@example.com"); - let stored = h - .stores - .peek_otp(OtpPurpose::EmailVerification, &identifier); - let Some(code) = stored else { return }; + // From the mail the engine sent, not from the store: the `otp:` record holds a keyed + // fingerprint of the code, which is what keeps a Redis dump from handing over a + // six-digit keyspace. The recipient's mailbox is where the code is. + // + // Awaited, and then ASSERTED before it is unwrapped. The send is detached, so reading + // the mailbox immediately is a race — and an `else { return }` on a lost race is a test + // that skips itself and reports success. That is exactly what happened here before the + // coverage gate caught the whole flow going unexercised. + let mailed = crate::services::auth::test_support::await_verification_code(&h).await; + assert!( + mailed.is_some(), + "the verification mail never carried a code" + ); + let Some(code) = mailed else { return }; // Captured so the event's fields are actually rendered: `log_safe` is the second lock on // a host-supplied tenant reaching a log line, and with no subscriber installed the call // never runs, which leaves the sanitization unfalsifiable from a test. @@ -270,6 +279,17 @@ mod tests { .await, Err(AuthError::OtpInvalid) )); + // Drain the first account's mail before the ghost's is sent. The read is consuming, but + // nothing has consumed this one yet — so without draining, the poll below answers + // instantly with the WRONG account's code, the submission fails the OTP check, and the + // assertion passes without ever reaching the vanished-account arm it exists for. + assert!( + crate::services::auth::test_support::await_verification_code(&h) + .await + .is_some(), + "the seeded account's verification mail never arrived" + ); + // An OTP stored for an email with no backing user collapses to OtpInvalid on success. assert!( h.engine @@ -277,11 +297,16 @@ mod tests { .await .is_ok() ); - let identifier = h.engine.hashed_identifier("t1", "ghost@example.com"); - let stored = h - .stores - .peek_otp(OtpPurpose::EmailVerification, &identifier); - let Some(code) = stored else { return }; + // From the mail: the record holds a keyed fingerprint, so submitting the stored value + // would fail the OTP check itself and this assertion would pass for the wrong reason — + // reporting "no such account" where the code simply did not match, and never reaching + // the vanished-account arm this test exists for. + let mailed = crate::services::auth::test_support::await_verification_code(&h).await; + assert!( + mailed.is_some(), + "no verification code was mailed for the ghost" + ); + let Some(code) = mailed else { return }; assert!(matches!( h.engine .verify_email(Some("t1"), "ghost@example.com", &code, &ctx()) diff --git a/crates/bymax-auth-core/src/services/auth/mod.rs b/crates/bymax-auth-core/src/services/auth/mod.rs index e9893e1..7303acc 100644 --- a/crates/bymax-auth-core/src/services/auth/mod.rs +++ b/crates/bymax-auth-core/src/services/auth/mod.rs @@ -426,6 +426,106 @@ pub(crate) mod test_support { pub engine: AuthEngine, pub users: Arc, pub stores: Arc, + /// The codes the engine actually mailed. The OTP record holds a keyed fingerprint, not + /// the code, so a flow that has to submit the code back reads it from here — which is + /// also where the recipient reads it. + pub emails: Arc, + } + + /// An email provider that keeps the codes it was asked to send. + #[derive(Default)] + pub(crate) struct CapturingEmails { + verification: std::sync::Mutex>, + password_reset: std::sync::Mutex>, + } + + impl CapturingEmails { + /// Take the last email-verification code sent, leaving the mailbox empty. + /// + /// Consuming, not peeking, and that is load-bearing: a flow that mails twice would + /// otherwise have the second read answer instantly with the FIRST code, because the + /// mailbox is already non-empty when the poll starts. The test then submits a stale code + /// and its assertion passes for the wrong reason. + pub(crate) fn verification_code(&self) -> Option { + self.verification.lock().ok().and_then(|mut c| c.take()) + } + + /// Take the last password-reset code sent, leaving the mailbox empty. See + /// [`Self::verification_code`] for why it consumes. + pub(crate) fn password_reset_code(&self) -> Option { + self.password_reset.lock().ok().and_then(|mut c| c.take()) + } + } + + #[async_trait::async_trait] + impl crate::traits::EmailProvider for CapturingEmails { + async fn send_email_verification_otp( + &self, + _tenant_id: &str, + _email: &str, + otp: &str, + _locale: Option<&str>, + ) -> Result<(), crate::traits::EmailError> { + if let Ok(mut slot) = self.verification.lock() { + *slot = Some(otp.to_owned()); + } + Ok(()) + } + async fn send_password_reset_otp( + &self, + _tenant_id: &str, + _email: &str, + otp: &str, + _locale: Option<&str>, + ) -> Result<(), crate::traits::EmailError> { + if let Ok(mut slot) = self.password_reset.lock() { + *slot = Some(otp.to_owned()); + } + Ok(()) + } + async fn send_password_reset_token( + &self, + _tenant_id: &str, + _email: &str, + _token: &str, + _locale: Option<&str>, + ) -> Result<(), crate::traits::EmailError> { + Ok(()) + } + async fn send_email_change_verification( + &self, + _tenant_id: &str, + _new_email: &str, + _token: &str, + _locale: Option<&str>, + ) -> Result<(), crate::traits::EmailError> { + Ok(()) + } + async fn send_mfa_enabled( + &self, + _tenant_id: &str, + _email: &str, + _locale: Option<&str>, + ) -> Result<(), crate::traits::EmailError> { + Ok(()) + } + async fn send_mfa_disabled( + &self, + _tenant_id: &str, + _email: &str, + _locale: Option<&str>, + ) -> Result<(), crate::traits::EmailError> { + Ok(()) + } + async fn send_invitation( + &self, + _tenant_id: &str, + _email: &str, + _invite: &crate::traits::InviteData, + _locale: Option<&str>, + ) -> Result<(), crate::traits::EmailError> { + Ok(()) + } } impl Harness { @@ -501,13 +601,60 @@ pub(crate) mod test_support { await_rehash_within(harness, user_id, previous, 40).await } + /// Wait for the detached email send to deliver the verification code, up to a deadline. + /// + /// The send is fire-and-forget (`spawn_guarded`), so reading the mailbox the instant the + /// flow returns is a race the test loses on a quiet machine — and loses SILENTLY, because + /// the caller's `let Some(code) = .. else { return }` turns a lost race into a pass. Polling + /// rather than sleeping a fixed span for the reason `await_rehash_within` does: a wait tuned + /// here becomes a flake on a slower runner. + /// Polls to a deadline of `attempts` × 25 ms. Callers pass a generous count; the give-up + /// path is reachable — and therefore testable — by passing a small one, exactly as + /// [`await_rehash_within`] is. + pub(crate) async fn await_verification_code_within( + harness: &Harness, + attempts: u32, + ) -> Option { + for _ in 0..attempts { + if let Some(code) = harness.emails.verification_code() { + return Some(code); + } + tokio::time::sleep(std::time::Duration::from_millis(25)).await; + } + None + } + + pub(crate) async fn await_verification_code(harness: &Harness) -> Option { + await_verification_code_within(harness, 40).await + } + + /// The password-reset twin of [`await_verification_code_within`]. + pub(crate) async fn await_password_reset_code_within( + harness: &Harness, + attempts: u32, + ) -> Option { + for _ in 0..attempts { + if let Some(code) = harness.emails.password_reset_code() { + return Some(code); + } + tokio::time::sleep(std::time::Duration::from_millis(25)).await; + } + None + } + + pub(crate) async fn await_password_reset_code(harness: &Harness) -> Option { + await_password_reset_code_within(harness, 40).await + } + pub(crate) fn harness(cfg: AuthConfig, hooks: Option>) -> Option { let users = Arc::new(InMemoryUserRepository::new()); let stores = Arc::new(InMemoryStores::new()); + let emails = Arc::new(CapturingEmails::default()); let mut builder = AuthEngine::builder() .config(cfg) .environment(Environment::Test) .user_repository(users.clone()) + .email_provider(emails.clone()) .redis_stores(stores.clone()); if let Some(hooks) = hooks { builder = builder.hooks(hooks); @@ -516,6 +663,7 @@ pub(crate) mod test_support { engine, users, stores, + emails, }) } } @@ -671,6 +819,92 @@ mod tests { )); } + #[tokio::test] + async fn the_mail_poll_gives_up_rather_than_hanging() { + // The deadline arm, driven with a tiny attempt count against a mailbox nothing ever + // wrote to. It exists so a caller that loses the race asserts and fails instead of + // blocking a CI run forever, and it is only reachable — and only testable — through the + // parameterized form, exactly as `await_rehash_within` is. + let cfg = test_support::base_config(); + let Some(h) = test_support::harness(cfg, None) else { return }; + assert!( + test_support::await_verification_code_within(&h, 1) + .await + .is_none() + ); + assert!( + test_support::await_password_reset_code_within(&h, 1) + .await + .is_none() + ); + } + + #[tokio::test] + async fn the_capturing_mailer_records_the_codes_and_ignores_the_rest() { + // The harness's double keeps the two codes a flow has to submit back, and no-ops the + // other sends. Exercised end to end here so the object-safe impl is covered and so the + // recording halves are asserted rather than assumed: a double that quietly kept nothing + // would make every test that reads a code from it skip its own assertions. + let mailer = test_support::CapturingEmails::default(); + assert!(mailer.verification_code().is_none()); + assert!(mailer.password_reset_code().is_none()); + + let provider: &dyn crate::traits::EmailProvider = &mailer; + assert!( + provider + .send_email_verification_otp("t1", "u@example.com", "123456", None) + .await + .is_ok() + ); + assert!( + provider + .send_password_reset_otp("t1", "u@example.com", "654321", Some("pt-BR")) + .await + .is_ok() + ); + assert_eq!(mailer.verification_code().as_deref(), Some("123456")); + assert_eq!(mailer.password_reset_code().as_deref(), Some("654321")); + + // The rest carry no code a test submits back, so they are no-ops — driven here so the + // impl is fully covered. + assert!( + provider + .send_password_reset_token("t1", "u@example.com", "tok", None) + .await + .is_ok() + ); + assert!( + provider + .send_email_change_verification("t1", "new@example.com", "tok", None) + .await + .is_ok() + ); + assert!( + provider + .send_mfa_enabled("t1", "u@example.com", None) + .await + .is_ok() + ); + assert!( + provider + .send_mfa_disabled("t1", "u@example.com", None) + .await + .is_ok() + ); + let invite = crate::traits::InviteData { + inviter_name: "Owner".to_owned(), + tenant_name: "Acme".to_owned(), + invite_token: "0".repeat(64), + expires_at: time::OffsetDateTime::UNIX_EPOCH, + }; + assert!( + provider + .send_invitation("t1", "u@example.com", &invite, None) + .await + .is_ok() + ); + } + #[tokio::test] async fn a_request_naming_no_tenant_with_no_resolver_is_refused_rather_than_defaulted() { // Without a resolver the caller's value is the ONLY thing that can scope the request, diff --git a/crates/bymax-auth-core/src/services/auth/password_reset.rs b/crates/bymax-auth-core/src/services/auth/password_reset.rs index 8de91fb..7513f8d 100644 --- a/crates/bymax-auth-core/src/services/auth/password_reset.rs +++ b/crates/bymax-auth-core/src/services/auth/password_reset.rs @@ -848,7 +848,7 @@ mod tests { use crate::services::auth::LoginInput; use crate::services::auth::test_support::{Harness, SeedUser, base_config, ctx, harness}; use crate::traits::{ - EmailProvider, OtpStore, PasswordResetStore, SessionKind, SessionStore, UserRepository, + EmailProvider, PasswordResetStore, SessionKind, SessionStore, UserRepository, }; use bymax_auth_types::{AuthResult, CreateUserData, LoginResult}; use std::time::Duration; @@ -1118,7 +1118,13 @@ mod tests { .await .is_ok() ); - let Some(code) = h.stores.peek_otp(OtpPurpose::PasswordReset, &identifier) else { return }; + // From the mail; the stored record is a keyed fingerprint, not the code. Awaited and + // asserted like the second issuance below: the send is detached, so an immediate read + // races it, and `else { return }` would turn a lost race into a test that exercises + // neither reset path and still reports success. + let mailed = crate::services::auth::test_support::await_password_reset_code(&h).await; + assert!(mailed.is_some(), "the reset mail never carried a code"); + let Some(code) = mailed else { return }; let reset = ResetPasswordInput { email: "otp@example.com".to_owned(), tenant_id: Some("t1".to_owned()), @@ -1143,7 +1149,11 @@ mod tests { .await .is_ok() ); - let Some(code2) = h.stores.peek_otp(OtpPurpose::PasswordReset, &identifier) else { return }; + // Submitted below, so it has to be the plaintext — from the mail, not the record. + // Awaited and asserted: the send is detached, and a lost race must fail rather than skip. + let mailed = crate::services::auth::test_support::await_password_reset_code(&h).await; + assert!(mailed.is_some(), "the reset mail never carried a code"); + let Some(code2) = mailed else { return }; let verified = h .engine .verify_reset_otp( @@ -1257,9 +1267,9 @@ mod tests { ); // The OTP is filed under the canonical spelling, whatever was typed. let identifier = h.engine.hashed_identifier("t1", "case@example.com"); - let code = h - .stores - .peek_otp(OtpPurpose::PasswordReset, &identifier) + // Submitted below, so it has to be the plaintext — from the mail, not the record. + let code = crate::services::auth::test_support::await_password_reset_code(&h) + .await .unwrap_or_default(); assert!( !code.is_empty(), @@ -1293,9 +1303,9 @@ mod tests { .await .is_ok() ); - let second = h - .stores - .peek_otp(OtpPurpose::PasswordReset, &identifier) + // Submitted below, so it has to be the plaintext — from the mail, not the record. + let second = crate::services::auth::test_support::await_password_reset_code(&h) + .await .unwrap_or_default(); assert!(!second.is_empty()); let verified = h @@ -1509,9 +1519,14 @@ mod tests { // A valid OTP stored for an email with no backing user collapses to the // invalid-token error (no verified token is issued for a vanished account). let ghost_id = h.engine.hashed_identifier("t1", "ghost@example.com"); + // Planted through the OTP service, not straight into the store: the service fingerprints + // the code before it writes, so a record written at the store level would hold a + // plaintext the verify step no longer looks for — the test would then pass for the wrong + // reason, reporting "no user" where the code simply did not match. assert!( - h.stores - .put(OtpPurpose::PasswordReset, &ghost_id, "111111", 600) + h.engine + .otp() + .store(OtpPurpose::PasswordReset, &ghost_id, "111111", 600) .await .is_ok() ); @@ -1891,16 +1906,15 @@ mod tests { cfg.password_reset.method = ResetMethod::Otp; let Some(h) = harness(cfg, Some(hooks)) else { return }; let id = h.seed(SeedUser::active("hooked@example.com", "old")).await; - let identifier = h.engine.hashed_identifier("t1", "hooked@example.com"); assert!( h.engine .initiate_reset(forgot("hooked@example.com"), &ctx()) .await .is_ok() ); - let code = h - .stores - .peek_otp(OtpPurpose::PasswordReset, &identifier) + // Submitted below, so it has to be the plaintext — from the mail, not the record. + let code = crate::services::auth::test_support::await_password_reset_code(&h) + .await .unwrap_or_default(); assert!(!code.is_empty()); let reset = ResetPasswordInput { diff --git a/crates/bymax-auth-core/src/services/otp.rs b/crates/bymax-auth-core/src/services/otp.rs index 04d0637..8e41e3c 100644 --- a/crates/bymax-auth-core/src/services/otp.rs +++ b/crates/bymax-auth-core/src/services/otp.rs @@ -9,9 +9,12 @@ use std::sync::Arc; use std::time::{Duration, Instant}; +use bymax_auth_crypto::mac::hmac_sha256; use bymax_auth_crypto::token::random_array; use bymax_auth_types::AuthError; +use zeroize::Zeroizing; +use crate::services::to_hex; use crate::traits::{OtpPurpose, OtpStore}; /// Maximum failed verify attempts before the record is consumed (§7.6 `MAX_ATTEMPTS`). @@ -24,12 +27,34 @@ const MIN_VERIFY_MS: u64 = 100; /// Generates, stores, and verifies numeric OTPs over the [`OtpStore`]. pub struct OtpService { store: Arc, + /// The identifier-hashing key, reused here to fingerprint the code before it is stored so + /// the record never holds the plaintext OTP. Held byte-for-byte with nest-auth's `hmacKey`, + /// which is what keeps the two implementations reading the same `otp:` records. + identifier_key: Zeroizing<[u8; 64]>, } impl OtpService { - /// Assemble the service over an OTP store. - pub(crate) fn new(store: Arc) -> Self { - Self { store } + /// Assemble the service over an OTP store and the identifier-hashing key. + pub(crate) fn new(store: Arc, identifier_key: Zeroizing<[u8; 64]>) -> Self { + Self { + store, + identifier_key, + } + } + + /// Keyed one-way transform under which the OTP is stored and compared. + /// + /// A six-digit code is a keyspace of a million, so a plain digest lets anyone who reads + /// Redis reverse it instantly; the transform is therefore HMAC-SHA256 under the server-only + /// identifier key, bound to the identifier so the same code under two accounts does not + /// collapse to one value. `store` and `verify` transform the same way, and the byte-identical + /// verify script keeps comparing two opaque strings — so nest-auth stays in step by hashing + /// the code the same way before it reads or writes the shared record. + fn fingerprint(&self, identifier: &str, code: &str) -> String { + to_hex(&hmac_sha256( + &self.identifier_key[..], + format!("{identifier}:{code}").as_bytes(), + )) } /// Generate a `length`-digit numeric OTP from the CSPRNG, zero-padded. Each digit is @@ -56,7 +81,10 @@ impl OtpService { code: &str, ttl_secs: u64, ) -> Result<(), AuthError> { - self.store.put(purpose, identifier, code, ttl_secs).await + let fingerprint = self.fingerprint(identifier, code); + self.store + .put(purpose, identifier, &fingerprint, ttl_secs) + .await } /// Verify a submitted `code` atomically (match + attempt bump + single-use consume), @@ -74,9 +102,10 @@ impl OtpService { code: &str, ) -> Result<(), AuthError> { let started = Instant::now(); + let fingerprint = self.fingerprint(identifier, code); let result = self .store - .verify(purpose, identifier, code, MAX_ATTEMPTS) + .verify(purpose, identifier, &fingerprint, MAX_ATTEMPTS) .await; normalize_timing(started).await; result @@ -142,8 +171,28 @@ mod tests { use super::*; use crate::testing::InMemoryStores; + /// A fixed identifier-hashing key for the tests; any 64 bytes serve. + const TEST_KEY: [u8; 64] = [7u8; 64]; + fn service(store: Arc) -> OtpService { - OtpService::new(store) + OtpService::new(store, Zeroizing::new(TEST_KEY)) + } + + #[tokio::test] + async fn store_writes_the_keyed_fingerprint_not_the_plaintext_code() { + // The record holds the keyed fingerprint of the code, never the plaintext OTP: a reader + // of Redis must not be able to reverse the six-digit keyspace. Held identical to + // nest-auth, whose OTP records carry the same HMAC so the two share one keyspace. + let store = Arc::new(InMemoryStores::new()); + let svc = service(store.clone()); + let purpose = OtpPurpose::PasswordReset; + + assert!(svc.store(purpose, "id", "123456", 600).await.is_ok()); + + let stored = store.peek_otp(purpose, "id"); + let expected = to_hex(&hmac_sha256(&TEST_KEY[..], b"id:123456")); + assert_eq!(stored, Some(expected)); + assert_ne!(stored, Some("123456".to_owned())); } #[test] @@ -270,4 +319,64 @@ mod tests { Ok(false) )); } + + /// Read a field of the shared wire contract's HMAC-derivation vector. + /// + /// `conformance/wire-contract.json` is held byte-identical by nest-auth, so the value read + /// here is the value that side computes. Reading it rather than restating it is the point: + /// a constant copied into this file would be re-derived from the same code it is meant to + /// check, and would follow any drift instead of catching it. + fn contract_vector_field(field: &str) -> String { + let path = concat!( + env!("CARGO_MANIFEST_DIR"), + "/../../conformance/wire-contract.json" + ); + let raw = std::fs::read_to_string(path).unwrap_or_default(); + let root: serde_json::Value = serde_json::from_str(&raw).unwrap_or(serde_json::Value::Null); + let value = root + .get("hmacKeyDerivation") + .and_then(|d| d.get("vectors")) + .and_then(|v| v.get(0)) + .and_then(|v| v.get(field)) + .and_then(serde_json::Value::as_str) + .unwrap_or_default() + .to_owned(); + assert!( + !value.is_empty(), + "the wire contract declared no `hmacKeyDerivation.vectors[0].{field}` — it did not load" + ); + value + } + + #[test] + fn the_stored_otp_matches_the_shared_wire_contract_vector() { + // Both backends write and compare the SAME `otp:` record, so this transform is a wire + // contract rather than an internal detail: a drift is not a parse error on the other + // side, it is every code minted on one failing to verify on the other, both ways. nest-auth + // computes `hmacSha256(`${identifier}:${code}`, hmacKey)`; this asserts the Rust side lands + // on the byte-identical value for the vector the contract carries. + let key_hex = contract_vector_field("derivedKeyHex"); + let identifier = contract_vector_field("identifierHex"); + let code = contract_vector_field("otpCode"); + let expected = contract_vector_field("otpRecordCodeHex"); + + // The HMAC key is the hex TEXT of the digest, not its raw bytes — the distinction the + // derivation section exists to pin, and the one that would silently split the keyspace. + assert_eq!( + key_hex.len(), + 64, + "the contract's derivedKeyHex is not the 64 hex characters the key is built from" + ); + let mut key_bytes = [0u8; 64]; + key_bytes.copy_from_slice(key_hex.as_bytes()); + + let store = Arc::new(InMemoryStores::new()); + let svc = OtpService::new(store, Zeroizing::new(key_bytes)); + assert_eq!( + svc.fingerprint(&identifier, &code), + expected, + "the stored OTP transform drifted from the shared contract — nest-auth would refuse \ + every code this backend mints, and vice versa" + ); + } }