Skip to content

Add TOTP 2FA#60

Merged
DJAndries merged 1 commit into
masterfrom
2fa-totp
Jun 5, 2025
Merged

Add TOTP 2FA#60
DJAndries merged 1 commit into
masterfrom
2fa-totp

Conversation

@DJAndries

@DJAndries DJAndries commented Apr 26, 2025

Copy link
Copy Markdown
Collaborator

Adds support for time-based one time passwords and recovery/scratch codes.

Security review: https://github.com/brave/reviews/issues/1957

@DJAndries DJAndries marked this pull request as ready for review May 7, 2025 04:39
@DJAndries DJAndries requested review from claucece and fmarier and removed request for fmarier May 7, 2025 04:39
Comment thread misc/test-client-rust/src/util.rs
Comment thread misc/test-client-rust/src/util.rs
Comment thread misc/test-client-rust/src/util.rs
Comment thread misc/test-client-rust/src/util.rs
@github-actions

github-actions Bot commented May 7, 2025

Copy link
Copy Markdown

The security team is monitoring all repositories for certain keywords. This PR includes the word(s) "password" and so security team members have been added as reviewers to take a look.

No need to request a full security review at this stage, the security team will take a look shortly and either clear the label or request more information/changes.

Notifications have already been sent, but if this is blocking your merge feel free to reach out directly to the security team on Slack so that we can expedite this check.

Comment thread services/twofa.go Outdated
Comment thread services/twofa.go
Comment thread services/twofa.go
Comment thread migrations/11_cron.up.sql
Comment thread controllers/accounts.go Outdated
Comment thread controllers/accounts.go Outdated
Comment thread controllers/accounts.go Outdated
Comment thread controllers/accounts.go
Comment thread controllers/accounts.go Outdated
Comment thread services/twofa.go
Comment thread controllers/accounts_test.go
Comment thread controllers/accounts_test.go
Comment thread controllers/accounts_test.go Outdated
Comment thread controllers/accounts_test.go
Comment thread controllers/accounts_test.go
Comment thread controllers/accounts_test.go Outdated
Comment thread controllers/accounts_test.go
Comment thread controllers/auth.go
Comment thread services/twofa.go
@DJAndries DJAndries force-pushed the 2fa-totp branch 2 times, most recently from 4dc7961 to e098773 Compare May 26, 2025 21:25
@fmarier

fmarier commented May 26, 2025 via email

Copy link
Copy Markdown
Member

@DJAndries

Copy link
Copy Markdown
Collaborator Author

Sure. Will it be noted in the Swagger docs somehow?

I'll have to look into that. I'm not sure if the swagger lib allows us to generate docs from our error types. Worst case scenario, we can just provide a link to error.go on Github.

Comment thread controllers/accounts_test.go
Comment thread controllers/accounts_test.go
Comment thread controllers/auth_test.go Outdated
Comment thread controllers/auth_test.go
Comment thread controllers/server_keys.go Outdated
Comment thread util/util.go
KeyServiceSecretHeader = "key-service-secret"
KeyServiceURLEnv = "KEY_SERVICE_URL"

recoveryKeyArgonTime = 1

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@claucece to double-check these parameters

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes! it is see here: https://github.com/golang/crypto/blob/master/argon2/argon2.go#L76 and also: https://datatracker.ietf.org/doc/html/draft-irtf-cfrg-argon2-03#section-9.4

In 'recoveryKeyArgonThreads' as 4, I assume we have 4CPUs available, right @DJAndries ? If we are using more threads, we can do:

// 4 threads x 64 MiB = 256 MiB
key := argon2.IDKey(password, salt, 1, 256*1024, 4, 32)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

each server pod runs on 0.5 vCPU. do you think it would be safe to reduce the thread count to 1? if not, no worries. i'm not too concerned about performance issues given the expected infrequency of recovery code usage.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes! we can ;)

Comment thread services/twofa.go Outdated
Comment thread services/twofa.go Outdated
Comment thread services/twofa.go Outdated
Comment thread docs/swagger.yaml Outdated
Comment thread services/twofa.go Outdated
Comment thread .gitignore Outdated
Comment thread .gitignore Outdated
Comment thread datastore/password_states.go
Comment thread datastore/password_states.go
Comment thread datastore/password_states.go
Comment thread datastore/twofa.go
Comment thread datastore/twofa.go
Comment thread misc/test-client-rust/src/util.rs
@fmarier fmarier requested a review from Copilot May 28, 2025 22:05

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for TOTP 2FA and recovery/scratch codes. It updates migrations, middleware, various datastore operations, controllers for both server keys and account authentication flows, and extends the test suites to cover the new TOTP functionality.

  • Introduced new TOTP key management in the datastore and added recovery key support.
  • Updated authentication and account controllers with new endpoints for TOTP generation, validation, and disabling.
  • Adjusted build tooling and workflow configurations to generate updated Swagger documentation.

Reviewed Changes

Copilot reviewed 38 out of 38 changed files in this pull request and generated no comments.

Show a summary per file
File Description
migrations/11_cron.down.sql Removed obsolete cron unschedules and added new ones for interim password & TOTP codes
middleware/auth.go Updated KeyServiceMiddleware signature to accept an environment flag
main.go Modified server startup to use the new twoFAService and updated middleware usage
go.mod Added dependencies for TOTP and barcode generation
datastore/twofa.go Added new datastore models and functions for TOTP key handling
datastore/password_states.go Renamed and refactored interim password state handling to support 2FA workflows
datastore/accounts.go Extended account fields and functions to support TOTP settings and recovery keys
controllers/server_keys.go Introduced new endpoints for TOTP key operations in the server keys API
controllers/auth.go Updated authentication flow to use login tokens and support 2FA finalization
controllers/accounts.go Added multiple new endpoints for 2FA setup, finalization, and recovery key management
Various test files Updated tests to integrate the new 2FA flows and endpoints
Makefile, Dockerfile, GitHub workflows Added swag init commands to ensure Swagger docs are generated with new API changes
.dockerignore Updated ignore patterns

Comment thread util/util.go Outdated
Comment thread datastore/password_states.go Outdated

@fmarier fmarier left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Great work @DJAndries !

PR can be merged after @claucece 's review of these two items:

@claucece claucece left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except for a very small discussion and the addition of the constant time check, it is looking awesome! Thank you @DJAndries !

@claucece claucece self-requested a review June 5, 2025 05:47
@github-actions

github-actions Bot commented Jun 5, 2025

Copy link
Copy Markdown

[puLL-Merge] - brave/accounts@60

Description

This is a comprehensive pull request that adds Two-Factor Authentication (2FA) support to the Brave Accounts service. The PR introduces TOTP-based 2FA with recovery keys, integrates it into the login and registration flows, and includes extensive API endpoints for managing 2FA settings. The implementation supports both standalone operation and integration with a separate key service for enhanced security.

Possible Issues

  1. Token Field Naming Inconsistency: The login flow changes the response field name from akeToken to loginToken which could break existing clients that expect the old field name.

  2. Recovery Key Exposure: Recovery keys are returned in API responses as plain text, which could be exposed in logs or other monitoring systems.

  3. Database Migration Complexity: The migration renames tables and adds multiple new columns, which could be risky in production environments with large datasets.

  4. Error Handling Gaps: Some error paths in 2FA processing don't properly clean up interim password states, potentially leaving orphaned records.

  5. QR Code Memory Usage: QR codes are generated as base64-encoded images in memory, which could consume significant resources for concurrent requests.

Security Hotspots

  1. TOTP Secret Storage (services/twofa.go:149, datastore/twofa.go:40): TOTP secrets are stored as plain text in the database. Consider encrypting these secrets at rest.

  2. Recovery Key Generation (util/util.go:81-98): Recovery keys use a relatively weak entropy source (20 bytes for 32-character base32). Consider increasing entropy or using a cryptographically secure random number generator with additional entropy sources.

  3. Time-based Attack Vulnerability (services/twofa.go:202): TOTP validation allows for time skew but doesn't implement rate limiting, potentially allowing brute force attacks within the time window.

  4. Recovery Key Hash Validation (util/util.go:91-103): While using Argon2 is good, the parameters (1 iteration, 64KB memory) may be too weak for production use against modern attack hardware.

  5. Key Service Authentication (util/util.go:245-247): The key service authentication relies on a shared secret in headers, which could be logged or exposed in network traffic if not properly secured.

  6. Session Management (controllers/accounts.go:268-288): The password setup flow deletes all existing sessions, which could be exploited for account takeover if an attacker can trigger password reset.

Changes

Changes

.dockerignore

  • Added basic Docker ignore patterns for misc and .git directories

.github/workflows/checks.yaml

  • Added swagger generation step to CI pipeline

.gitignore

  • Added /docs and /.vscode to ignore patterns

Dockerfile

  • Added swagger installation and initialization steps

Makefile

  • Added swagger generation to run and test targets
  • Added new dev-key-service target for running key service in development

controllers/accounts.go

  • Major Changes: Complete restructuring to support 2FA integration
  • Added 2FA service dependency injection
  • Added endpoints for TOTP setup, management, and recovery key operations
  • Modified password finalization to support 2FA challenges
  • Added session management helpers
  • New endpoints: /2fa, /2fa/totp/init, /2fa/totp/finalize, /2fa/totp (DELETE), /2fa/recovery

controllers/auth.go

  • Major Changes: Restructured login flow to support 2FA
  • Changed akeToken to loginToken in responses
  • Added 2FA challenge endpoint /login/finalize_2fa
  • Modified login finalization to handle 2FA requirements
  • Added helper methods for session creation

controllers/server_keys.go

  • Added TOTP key management endpoints for key service
  • New endpoints: /totp, /totp/validate, /totp/{accountId} (DELETE)
  • Added 2FA service dependency

datastore/accounts.go

  • Added 2FA-related fields to Account model: TOTPEnabled, TOTPEnabledAt, RecoveryKeyHash, RecoveryKeyCreatedAt
  • Added TwoFADetails struct for API responses
  • Added methods for managing 2FA settings: SetTOTPSetting, SetRecoveryKey, CheckRecoveryKey, etc.

datastore/password_states.go (renamed from datastore/ake_states.go)

  • Major Restructuring: Combined AKE and registration states into unified interim password states
  • Added 2FA-related fields: AwaitingTwoFA, RequiresTwoFA, IsRegistration
  • Added separate expiration handling for 2FA flows (5 minutes vs 30 seconds)

datastore/twofa.go

  • New File: Complete TOTP key and usage tracking implementation
  • Added TOTPKey and TOTPUsedCode models
  • Methods for storing, retrieving, and validating TOTP keys

main.go

  • Added 2FA service initialization
  • Updated controller dependency injection
  • Enhanced key service startup with environment parameter

services/jwt.go

  • Renamed AKE-related methods to login state methods
  • Updated token validation logic
  • Enhanced key service integration

services/opaque.go

  • Major Changes: Updated to support 2FA integration in login/registration flows
  • Modified login/registration flows to handle interim password states
  • Added 2FA requirement detection
  • Updated state management for 2FA challenges

services/twofa.go

  • New File: Comprehensive 2FA service implementation
  • TOTP key generation and validation
  • QR code generation for authenticator apps
  • Recovery key management
  • Integration with key service for distributed deployments

util/util.go

  • Added recovery key hashing/verification using Argon2
  • Added KeyServiceClient for improved key service communication
  • Enhanced error handling for key service responses

Database Migrations

  • Migration 20250425220000: Major schema changes
    • Added TOTP and recovery key fields to accounts
    • Renamed ake_states to interim_password_states with additional fields
    • Added totp_keys and totp_used_codes tables
    • Updated cron jobs for cleanup

Test Files

  • Comprehensive test coverage for all new 2FA functionality
  • Integration tests for complete 2FA flows
  • Mock implementations for key service testing
sequenceDiagram
    participant C as Client
    participant A as Accounts Service
    participant K as Key Service
    participant D as Database

    Note over C,D: 2FA Registration Flow
    C->>A: POST /accounts/password/init
    A->>D: Create registration state with 2FA flag
    A-->>C: Registration response
    
    C->>A: POST /accounts/password/finalize
    A->>D: Check 2FA requirement
    alt 2FA Required
        A->>D: Mark state as awaiting 2FA
        A-->>C: {requiresTwoFA: true}
        C->>A: POST /accounts/password/finalize_2fa
        A->>A: Validate TOTP/recovery key
        A->>D: Complete registration
    else No 2FA
        A->>D: Complete registration immediately
    end
    A-->>C: Auth token

    Note over C,D: 2FA Login Flow  
    C->>A: POST /auth/login/init
    A->>K: Get OPRF seed
    K-->>A: Seed response
    A->>D: Create login state
    A-->>C: Login token

    C->>A: POST /auth/login/finalize
    A->>D: Validate credentials & check 2FA
    alt 2FA Required
        A-->>C: {requiresTwoFA: true}
        C->>A: POST /auth/login/finalize_2fa
        A->>K: Validate TOTP (if using key service)
        K-->>A: Validation result
        A->>D: Check code not reused
        A->>D: Create session
    else No 2FA
        A->>D: Create session immediately  
    end
    A-->>C: Auth token

    Note over C,D: TOTP Setup
    C->>A: POST /2fa/totp/init
    A->>K: Generate TOTP key (if using key service)
    K-->>A: TOTP URI
    A-->>C: {uri, qrCode}
    
    C->>A: POST /2fa/totp/finalize
    A->>K: Validate setup code
    K-->>A: Validation success
    A->>D: Enable 2FA & generate recovery key
    A-->>C: {recoveryKey}
Loading

@DJAndries DJAndries merged commit a2b8676 into master Jun 5, 2025
5 checks passed
@DJAndries DJAndries deleted the 2fa-totp branch June 5, 2025 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants