Enforce PIN requirement for storage encryption

[wksantiago]

Summary

• Require PIN for storage_crypto_init(), eliminating device-only key derivation
• Add rate limiting: 3 attempts per 60s, 5-minute lockout after 5 failures
• Update boot sequence to defer crypto init until PIN provided
• Mark storage_crypto self-test as non-critical (skips when not initialized)

Test plan

• All 95+ native tests pass
• Rate limiting functions tested
• Self-test behavior verified

Summary by CodeRabbit

• New Features

  • PIN rate-limiting and lockout to protect PIN-protected share operations from brute-force attempts.

• Improvements

  • Boot flow simplified to avoid restarts related to storage migration; share operations now require PIN with an informational notice.
  • Failed decryption attempts are recorded to feed rate-limit tracking.

• Tests

  • Test suite updated to cover skipped self-test when crypto is uninitialized and rate-limit scenarios.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Warning

Rate limit exceeded

@kwsantiago has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 22 minutes and 50 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

Walkthrough

Removed boot-time storage_crypto initialization and migration; added PIN rate-limiting/lockout and APIs; made storage_crypto self-test skippable when uninitialized; record failed decrypt attempts; updated mocks and tests to exercise new behaviors.

Changes

Cohort / File(s)	Summary
Boot Initialization Cleanup main/main.c	Removed storage_crypto_init and migration/error-restart logic at boot; replaced with an informational log about PIN-protected storage.
Storage Crypto Rate‑Limiting main/storage_crypto.c, main/storage_crypto.h	Added timing utilities, rate-limit state and constants, lockout logic, and APIs: storage_crypto_check_rate_limit(), storage_crypto_record_attempt(bool), storage_crypto_reset_rate_limit(). Refactored storage_crypto_init() to enforce rate limits, validate PIN length, record attempts, and return new ERR_* codes.
Decryption Attempt Recording main/storage.c	On decrypt failure in storage_load_share, now calls storage_crypto_record_attempt(false) before clearing the slot and returning STORAGE_ERR_DECRYPT.
Self-Test Changes main/self_test.c	Marked SELF_TEST_STORAGE_CRYPTO non-required; self_test_storage_crypto() treats uninitialized crypto as a non-fatal skip.
Test Mocks & Test Updates test/native/mocks/storage_crypto.h, test/native/test_self_test.c	Added mock mock_rate_limit_result and mock implementations for the three new rate-limit APIs; storage_crypto_init() mock now checks rate limit and PIN length. Renamed/updated tests to expect skip behavior and to exercise failure paths via secp256k1 context mocks.

Sequence Diagram(s)

sequenceDiagram
    participant Caller as Caller
    participant StorageCrypto as storage_crypto
    participant RateLimit as RateLimit
    participant KeyDerive as KeyDerivation
    participant AttemptTrack as AttemptTracker

    Caller->>StorageCrypto: storage_crypto_init(pin)
    StorageCrypto->>RateLimit: storage_crypto_check_rate_limit()
    alt Locked / Must wait
        RateLimit-->>StorageCrypto: ERR_PIN_LOCKED / ERR_PIN_MUST_WAIT
        StorageCrypto-->>Caller: return error
    else Allowed
        StorageCrypto->>StorageCrypto: validate PIN length
        alt Invalid PIN
            StorageCrypto->>AttemptTrack: storage_crypto_record_attempt(false)
            AttemptTrack-->>StorageCrypto: record (maybe lockout)
            StorageCrypto-->>Caller: ERR_PIN_INVALID
        else Valid PIN
            StorageCrypto->>KeyDerive: derive key from device id + pin
            alt Derive success
                StorageCrypto->>AttemptTrack: storage_crypto_record_attempt(true)
                StorageCrypto-->>Caller: success
            else Derive fail
                StorageCrypto->>AttemptTrack: storage_crypto_record_attempt(false)
                StorageCrypto-->>Caller: error
            end
        end
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• Add storage versioning with AAD and migration support #113: Removes/changes boot migration and startup behavior that this PR also eliminates from boot flow.
• Encrypt shares at rest with AES-256-GCM #59: Prior storage_crypto implementation that this change extends/refactors with rate-limiting APIs and behavior.
• Add comprehensive boot-time self-test framework #118: Self-test integration changes that relate to the altered self-test behavior and test updates here.

Suggested reviewers

• kwsantiago

Poem

🐇 Hopping through logs with whiskers bright,
PINs paced gently, guarding the night.
Boot wakes lighter, tests politely skip,
Attempts are counted, lockouts take grip.
A rabbit nods, and munches a byte 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Enforce PIN requirement for storage encryption' directly and accurately summarizes the main change—requiring a PIN for storage crypto initialization and adding rate-limiting logic.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@main/storage_crypto.c`:
- Around line 159-183: The code currently only records failed attempts for
invalid PIN format and successful key init; update storage_crypto_init so that
when derive_key(device_id, ..., (const uint8_t *)pin, pin_len, storage_key)
returns non-zero you call storage_crypto_record_attempt(false) before returning
the error, and ensure secure_memzero/device cleanup still occurs; likewise, in
storage_crypto_decrypt (the decryption routine in storage.c) detect the
wrong-PIN decryption failure path and call storage_crypto_record_attempt(false)
when decryption fails due to an incorrect PIN so those failures count toward the
lockout limit.

🧹 Nitpick comments (1)

test/native/mocks/storage_crypto.h (1)

45-52: Prefer bounded length to mirror production.
strlen can read past non‑terminated buffers; using strnlen(..., STORAGE_CRYPTO_MAX_PIN_LEN + 1) keeps the mock aligned with real behavior.

♻️ Suggested change

-    size_t pin_len = pin ? strlen(pin) : 0;
+    size_t pin_len = pin ? strnlen(pin, STORAGE_CRYPTO_MAX_PIN_LEN + 1) : 0;

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@main/storage_crypto.c`:
- Around line 121-129: The signed cast of the time delta causes stale timestamps
to appear "recent" after ~24.8 days; fix the check in the loop that counts
recent attempts by using unsigned arithmetic: compute the difference between now
and pin_attempt_times[i] as an unsigned type (e.g., uint32_t or uint64_t) and
compare that to PIN_RATE_LIMIT_WINDOW_MS instead of casting to int32_t, ensuring
the condition becomes something like (now - pin_attempt_times[i]) <
PIN_RATE_LIMIT_WINDOW_MS so that stale (older) timestamps are not treated as
recent; update the loop that uses pin_attempt_count, pin_attempt_times,
PIN_RATE_LIMIT_MAX, PIN_RATE_LIMIT_WINDOW_MS and keep the ERR_PIN_MUST_WAIT
return logic unchanged.