Merge pull request #123 from rmarscher/lucia-totp-per-auth-method

Lucia separate TOTP secret per auth method

.env.example

@@ -29,8 +29,3 @@ DISCORD_CLIENT_SECRET=
 
 # Optional for password reset emails
 RESEND_API_KEY=
-
-# Optional for paassword reset codes / OTP
-# Use the password reset feature locally to generate a secret in the API terminal console
-# Then copy and paste it here in .env.local
-TOTP_SECRET=

packages/api/migrations/0004_famous_ultimates.sql

@@ -4,6 +4,10 @@ CREATE TABLE `AuthMethod` (
 	`hashed_password` text,
 	`hash_method` text,
 	`created_at` text DEFAULT CURRENT_TIMESTAMP,
+	`totp_secret` text,
+	`totp_expires` integer,
+	`timeout_until` integer,
+	`timeout_seconds` integer,
 	FOREIGN KEY (`user_id`) REFERENCES `User`(`id`) ON UPDATE no action ON DELETE no action
 );
 --> statement-breakpoint
@@ -14,17 +18,5 @@ CREATE TABLE `Session` (
 	FOREIGN KEY (`user_id`) REFERENCES `User`(`id`) ON UPDATE no action ON DELETE no action
 );
 --> statement-breakpoint
-CREATE TABLE `VerificationCode` (
-	`id` text PRIMARY KEY NOT NULL,
-	`user_id` text NOT NULL,
-	`code` text NOT NULL,
-	`expires` integer NOT NULL,
-	`timeout_until` integer,
-	`timeout_seconds` integer DEFAULT 0 NOT NULL,
-	FOREIGN KEY (`user_id`) REFERENCES `User`(`id`) ON UPDATE no action ON DELETE no action
-);
---> statement-breakpoint
 CREATE INDEX `idx_userKey_userId` ON `AuthMethod` (`user_id`);--> statement-breakpoint
-CREATE INDEX `idx_session_userId` ON `Session` (`user_id`);--> statement-breakpoint
-CREATE UNIQUE INDEX `VerificationCode_user_id_unique` ON `VerificationCode` (`user_id`);--> statement-breakpoint
-CREATE INDEX `idx_verificationCode_userId` ON `VerificationCode` (`user_id`);
+CREATE INDEX `idx_session_userId` ON `Session` (`user_id`);

packages/api/src/auth/user.ts

@@ -1,13 +1,13 @@
 import { ApiContextProps } from '../context'
-import { AuthMethodTable, SessionTable, User, UserTable, VerificationCodeTable } from '../db/schema'
+import { AuthMethodTable, SessionTable, User, UserTable } from '../db/schema'
 import { createId } from '../utils/id'
 import { hashPassword, verifyPassword } from '../utils/password'
 import { DEFAULT_HASH_METHOD } from '../utils/password/hash-methods'
 import { TRPCError } from '@trpc/server'
 import { and, eq, lt } from 'drizzle-orm'
 import type { RegisteredLucia, Session } from 'lucia'
 import { isWithinExpirationDate } from 'oslo'
-import { createCode, verifyCode } from '../utils/crypto'
+import { createCode, createTotpSecret, verifyCode } from '../utils/crypto'
 import { AuthProviderName } from './providers'
 import { OAuth2RequestError } from 'arctic'
 import { getOAuthUser } from './shared'
@@ -175,28 +175,32 @@ export const signInWithEmail = async (
   return { session }
 }
 
-const passwordResetTimeoutSeconds = 300
+const passwordResetTimeoutSeconds = 300 // 5 minutes
 
 export const sendEmailSignIn = async (ctx: ApiContextProps, email: string) => {
   const authMethod = await getAuthMethod(ctx, createAuthMethodId('email', email))
   if (!authMethod) {
     throw new TRPCError({ message: 'No user found with that email', code: 'UNAUTHORIZED' })
   }
-  const props = {
-    code: await createCode({ seconds: passwordResetTimeoutSeconds, secret: ctx.env.TOTP_SECRET }),
-    expires: new Date(Date.now() + 1000 * passwordResetTimeoutSeconds), // 5 minutes
-  }
-  await ctx.db
-    .insert(VerificationCodeTable)
-    .values({
-      id: createId(),
-      userId: authMethod.userId,
-      ...props,
-    })
-    .onConflictDoUpdate({ target: VerificationCodeTable.userId, set: props })
+  // Generate a TOTP secret to verify codes
+  // While it could be possible to not generate a new secret every time,
+  // that opens some security risks if the database is compromised
+  // and the secret is leaked. D1 has encryption in rest and in transit,
+  // but it could be wise to encrypt the secret with a server-side RSA key
+  // so there is a second layer of security.
+  // The totpExpires field is only used to determine if validation fails
+  // due to an expired code or invalid code.
+  const secret = await createTotpSecret()
+  await ctx.db.update(AuthMethodTable).set({
+    totpExpires: new Date(Date.now() + 1000 * passwordResetTimeoutSeconds),
+    totpSecret: secret,
+    timeoutUntil: null,
+    timeoutSeconds: null,
+  })
+  const code = await createCode({ seconds: passwordResetTimeoutSeconds, secret })
   const resetLink =
     `${ctx.env.APP_URL}/password-reset/update-password` +
-    `?code=${encodeURIComponent(props.code)}&email=${encodeURIComponent(email)}`
+    `?code=${encodeURIComponent(code)}&email=${encodeURIComponent(email)}`
 
   if (ctx.env.RESEND_API_KEY) {
     // send email
@@ -212,7 +216,7 @@ export const sendEmailSignIn = async (ctx: ApiContextProps, email: string) => {
         subject: 'T4 App verification code',
         html: `<p>This email was used to sign in to ${ctx.env.APP_URL}.</p>
           <p><a href="${resetLink.replaceAll('&', '&amp;')}">Click here to continue to sign in.</a></p>
-          <p>Code: ${props.code}</p>
+          <p>Code: ${code}</p>
           <p>If you did not request this code, you can ignore this email or reply to let us know.</p>
         `,
       }),
@@ -221,7 +225,7 @@ export const sendEmailSignIn = async (ctx: ApiContextProps, email: string) => {
     return { codeSent: true }
   }
   throw new TRPCError({
-    message: `Sending the access code ${props.code} to ${email} has not been implemented. It should email a link to ${resetLink}.`,
+    message: `Sending the access code ${code} to ${email} has not been implemented. It should email a link to ${resetLink}.`,
     code: 'METHOD_NOT_SUPPORTED',
   })
 }
@@ -239,47 +243,53 @@ export const signInWithCode = async (
     // Note... you might want to create a user here
     // or throw a more generic error if you don't want
     // to expose that the user doesn't exist
-    throw new Error(`Unable to find account where ${providerId} is ${providerUserId}`)
+    throw new TRPCError({
+      message: `Unable to find account where ${providerId} is ${providerUserId}`,
+      code: 'NOT_FOUND',
+    })
   }
-  const verificationCode = (
-    await ctx.db
-      .select()
-      .from(VerificationCodeTable)
-      .where(and(eq(VerificationCodeTable.userId, authMethod.userId)))
-  )?.[0]
-
-  if (!verificationCode || !isWithinExpirationDate(verificationCode?.expires)) {
-    // optionally send a new code instead of an error
-    throw new TRPCError({ message: 'Expired verification code', code: 'UNAUTHORIZED' })
+  if (!authMethod.totpSecret) {
+    throw new TRPCError({
+      message: `There are no active verification codes where ${providerId} is ${providerUserId}`,
+      code: 'TIMEOUT',
+    })
   }
-
-  if (verificationCode.timeoutUntil && !isWithinExpirationDate(verificationCode.timeoutUntil)) {
-    throw new TRPCError({ message: 'Too many requests', code: 'TOO_MANY_REQUESTS' })
+  if (authMethod.timeoutUntil && isWithinExpirationDate(authMethod.timeoutUntil)) {
+    throw new TRPCError({
+      message:
+        'Sorry, that was too many attempts to enter the verification code. Please wait a moment and try again.',
+      code: 'TOO_MANY_REQUESTS',
+    })
   }
 
-  if (verificationCode.code !== code) {
+  if (
+    !(await verifyCode(code, {
+      seconds: passwordResetTimeoutSeconds,
+      secret: authMethod.totpSecret,
+    }))
+  ) {
+    if (authMethod.totpExpires && !isWithinExpirationDate(authMethod.totpExpires)) {
+      throw new TRPCError({ message: 'This verification code has expired.', code: 'TIMEOUT' })
+    }
     // exponential backoff to prevent brute force
-    const timeoutSeconds = verificationCode?.timeoutSeconds
-      ? verificationCode.timeoutSeconds * 2
-      : 1
+    const timeoutSeconds = authMethod?.timeoutSeconds ? authMethod.timeoutSeconds * 2 : 1
     const timeoutUntil = new Date(Date.now() + timeoutSeconds * 1000)
     await ctx.db
-      .update(VerificationCodeTable)
+      .update(AuthMethodTable)
       .set({ timeoutUntil, timeoutSeconds })
-      .where(eq(VerificationCodeTable.id, verificationCode.id))
-    throw new TRPCError({ message: 'Expired verification code', code: 'UNAUTHORIZED' })
-  }
-
-  // Extra cryptographic verification
-  if (
-    !(await verifyCode(code, { seconds: passwordResetTimeoutSeconds, secret: ctx.env.TOTP_SECRET }))
-  ) {
-    throw new TRPCError({ message: 'Expired verification code', code: 'UNAUTHORIZED' })
+      .where(eq(AuthMethodTable.id, authMethod.id))
+    throw new TRPCError({ message: 'Invalid verification code', code: 'UNAUTHORIZED' })
   }
 
   await ctx.db
-    .delete(VerificationCodeTable)
-    .where(eq(VerificationCodeTable.id, verificationCode.id))
+    .update(AuthMethodTable)
+    .set({
+      totpSecret: null,
+      totpExpires: null,
+      timeoutUntil: null,
+      timeoutSeconds: null,
+    })
+    .where(eq(AuthMethodTable.id, authMethod.id))
 
   // You may want to sign the user out of other sessions here,
   // but for now, we only do it if the user is changing their password

packages/api/src/db/schema.ts

@@ -1,7 +1,6 @@
 import { InferSelectModel, InferInsertModel, relations, sql } from 'drizzle-orm'
 import { sqliteTable, text, integer, real, index } from 'drizzle-orm/sqlite-core'
 import { createInsertSchema, createSelectSchema } from 'drizzle-valibot'
-import { createId } from '../utils/id'
 import { HASH_METHODS } from '../utils/password/hash-methods'
 
 // User
@@ -32,6 +31,12 @@ export const AuthMethodTable = sqliteTable(
     hashedPassword: text('hashed_password'),
     hashMethod: text('hash_method', { enum: HASH_METHODS }),
     createdAt: text('created_at').default(sql`CURRENT_TIMESTAMP`),
+    // The following could be used for password resets, one-time passwords or 2FA
+    totpSecret: text('totp_secret'),
+    totpExpires: integer('totp_expires', { mode: 'timestamp' }),
+    // This is used to prevent brute force attacks and rate limit invalid login attempts
+    timeoutUntil: integer('timeout_until', { mode: 'timestamp' }),
+    timeoutSeconds: integer('timeout_seconds'),
     // Depending on which providers you connect... you may want to store more data, i.e. username, profile pic, etc
     // Instead of creating separate fields for each, you could add a single field to store any additional data
     // data: text('data', { mode: 'json' })
@@ -73,29 +78,6 @@ export type Session = InferSelectModel<typeof SessionTable>
 export type InsertSession = InferInsertModel<typeof SessionTable>
 export const SessionSchema = createInsertSchema(SessionTable)
 
-export const VerificationCodeTable = sqliteTable(
-  'VerificationCode',
-  {
-    id: text('id')
-      .primaryKey()
-      .$defaultFn(() => createId()),
-    userId: text('user_id')
-      .notNull()
-      .unique()
-      .references(() => UserTable.id),
-    code: text('code').notNull(),
-    expires: integer('expires', { mode: 'timestamp_ms' }).notNull(),
-    timeoutUntil: integer('timeout_until', { mode: 'timestamp_ms' }),
-    timeoutSeconds: integer('timeout_seconds').notNull().default(0),
-  },
-  (t) => ({
-    userIdIdx: index('idx_verificationCode_userId').on(t.userId),
-  })
-)
-export type VerificationCode = InferSelectModel<typeof VerificationCodeTable>
-export type InsertVerificationCode = InferInsertModel<typeof VerificationCodeTable>
-export const VerificationCodeSchema = createInsertSchema(VerificationCodeTable)
-
 // Car
 export const CarTable = sqliteTable('Car', {
   id: text('id').primaryKey(),

packages/api/src/utils/crypto.ts

@@ -3,10 +3,9 @@ import { TOTPController } from 'oslo/otp'
 import { decodeBase64, encodeBase64, encodeHex } from 'oslo/encoding'
 import { type JWT, parseJWT, validateJWT } from './jwt'
 import { match, P } from 'ts-pattern'
-import { sha256 as sha256AB } from 'oslo/crypto'
+import { HMAC, sha256 as sha256AB } from 'oslo/crypto'
 
 const totpControllers: Record<number, TOTPController> = {}
-let totpSecret: ArrayBuffer | undefined = undefined
 
 function getTotpController({ seconds = 30 }: { seconds?: number } = {}) {
   if (!(seconds in totpControllers)) {
@@ -20,36 +19,27 @@ function getTotpController({ seconds = 30 }: { seconds?: number } = {}) {
   return totpControllers[seconds] as TOTPController
 }
 
-async function getTotpSecret(secret?: string) {
-  if (secret) {
-    totpSecret = decodeBase64(secret).buffer as ArrayBuffer
-  }
-  if (!totpSecret) {
-    // @ts-ignore TS1323 (fix needed for top-level tsc that runs without esnext module target)
-    const { HMAC } = await import('oslo/crypto')
-    totpSecret = await new HMAC('SHA-1').generateKey()
-    console.log(
-      `TOTP secret is being generated. Update TOTP_SECRET= in .env.local to TOTP_SECRET=${encodeBase64(
-        totpSecret
-      )}`
-    )
-  }
-  return totpSecret
+export async function createTotpSecret() {
+  return encodeBase64(await new HMAC('SHA-1').generateKey())
+}
+
+export function decodeTotpSecret(secret: string) {
+  return decodeBase64(secret).buffer as ArrayBuffer
 }
 
 export interface TotpOptions {
   seconds?: number
-  secret?: string
+  secret: string
 }
 
-export async function createCode({ seconds = 30, secret }: TotpOptions = {}) {
+export async function createCode({ seconds = 30, secret }: TotpOptions) {
   const totpController = getTotpController({ seconds })
-  return await totpController.generate(await getTotpSecret(secret))
+  return await totpController.generate(decodeTotpSecret(secret))
 }
 
-export async function verifyCode(code: string, totpOptions?: TotpOptions) {
+export async function verifyCode(code: string, totpOptions: TotpOptions) {
   const totpController = getTotpController(totpOptions)
-  return totpController.verify(code, await getTotpSecret(totpOptions?.secret))
+  return totpController.verify(code, decodeTotpSecret(totpOptions.secret))
 }
 
 /**

packages/api/src/worker.ts

@@ -22,7 +22,6 @@ export type Bindings = Env & {
   PUBLIC_SUPPORT_EMAIL: string
   PUBLIC_API_URL: string
   PUBLIC_NATIVE_SCHEME: string
-  TOTP_SECRET: string
   [k: string]: unknown
 }
 

packages/api/wrangler.toml

@@ -31,7 +31,6 @@ migrations_dir = "migrations"
 # - PUBLIC_SUPPORT_EMAIL
 # - PUBLIC_API_URL
 # - PUBLIC_NATIVE_SCHEME
-# - TOTP_SECRET
 #
 # These can be set in the top level .env.local file followed by running `bun i`
 # which will then generate a .dev.vars file used for local cloudflare development.