Closes #22431: Replace random.choice with secrets.choice in Token.generate()

[bctiemann]

Closes: #22431

Summary

Replaces the non-CSPRNG random.choice() call in Token.generate() with secrets.choice() (SR-001 / VM-317, CWE-338).

Root cause

Token.generate() used random.choice(TOKEN_CHARSET) to build token strings. Python's random module uses the Mersenne Twister PRNG, which is not cryptographically secure. Observing approximately 624 32-bit outputs from the same worker process is sufficient to recover the full PRNG state and predict all subsequent outputs — including tokens minted for other users (potentially administrators) by the same worker.

Fix

Replace import random with import secrets and change random.choice to secrets.choice. secrets is backed by os.urandom() / getrandom() (OS-level CSPRNG) and is immune to state-recovery attacks. This is a one-line logic change with no behavioural impact beyond improved entropy.

The fix plan also requested an audit of other random.* usages in auth-adjacent code; none were found.

Tests

• test_generate_uses_csprng — patches secrets.choice with wraps= to verify it is called exactly TOKEN_DEFAULT_LENGTH times and that the output only contains TOKEN_CHARSET characters.
• test_generate_length_parameter — verifies the length= parameter is respected across several values.

Ref: SR-001 / VM-317 (internal security review, not an externally-reported disclosure)

🤖 Generated with Claude Code

[bctiemann]

@claude /review and limit response only to areas of potential concern.

[github-actions]

Claude finished @bctiemann's task in 1m 33s —— View job

---

Code Review: PR #22432

• Gathering context and reading changed files
• Analyzing changes
• Posting review

---

The fix itself is correct and minimal. Two areas worth noting:

1. test_generate_uses_csprng is bound to a specific implementation detail

tokens.py:263 — The test patches secrets.choice specifically. If the generation were ever changed to use another secrets.* function (e.g., secrets.token_bytes() or secrets.token_hex()), this test would fail even though the implementation would remain cryptographically secure. This is an intentional regression guard, but it also means the test enforces how randomness is generated rather than that it is cryptographically secure. A comment in the test noting this trade-off explicitly would help future maintainers understand why the patch target is intentional.

2. generate_key() is not covered by the new CSPRNG test

tokens.py:252-256 — generate_key() calls cls.generate(length=TOKEN_KEY_LENGTH). The TOKEN_KEY_LENGTH value is 12 characters, which yields ~71 bits of entropy — sufficient for a lookup key, but notably shorter than the full token. Neither of the two new tests exercises this path directly, nor does test_generate_length_parameter include TOKEN_KEY_LENGTH in its test values. Adding it to the length parameter test would close this gap:

# test_models.py – test_generate_length_parameter
for length in (8, 20, TOKEN_KEY_LENGTH, TOKEN_DEFAULT_LENGTH, 64):

3. Audit note: get_random_string() in testing utils

The PR description says the audit found no other auth-adjacent random.* usages. For completeness: utilities/testing/utils.py:193 also uses random.choice() via get_random_string(), but this is test-only scaffolding and not production auth code, so it is out of scope.

---

No blockers. The core fix (random.choice → secrets.choice) is correct. The concerns above are minor.