Use authenticator as 2FA to unlock fs#320
Conversation
Signed-off-by: Cristian Andrei <cristian.andrei1423@gmail.com>
Signed-off-by: Cristian Andrei <cristian.andrei1423@gmail.com>
Signed-off-by: Cristian Andrei <cristian.andrei1423@gmail.com>
Signed-off-by: Cristian Andrei <cristian.andrei1423@gmail.com>
radumarias
left a comment
There was a problem hiding this comment.
Comprehensive PR Review Summary
PR: TOTP 2FA Implementation
Branch: totp → main
Task: #226 - Use 2FA to unlock fs
Overview
This PR implements Time-based One-Time Password (TOTP) authentication as a second factor for mounting encrypted filesystems. The implementation adds:
- New
--init-2faCLI flag for initial setup - TOTP secret encrypted storage in
identity.enc - QR code generation for authenticator app setup
- TOTP verification on mount when 2FA is enabled
Critical Issues (Must fix before merge)
1. TOTP Secret Not Memory-Protected
File: src/run.rs:338-339
The TOTP secret is stored in a regular String rather than SecretString. Per CLAUDE.md: "Keys are mlock'd, mprotect'd, and zeroized when not in use." The TOTP secret is equivalent to a cryptographic key.
let secret = Secret::generate_secret();
let secret_str = secret.to_encoded().to_string(); // Plain String!Also exposed at: lines 386 (HTML), 406 (stdout), 415 (function call)
2. TOTP Secret Written to Plaintext HTML File
File: src/run.rs:355-394
The secret is written to rencfs_2fa_setup.html in the current directory in plaintext. Security concerns:
- Can be recovered forensically even after deletion
- Browser may cache the content
- If program crashes before
TempFileGuarddrops, file persists
Recommendation: Use terminal-based QR display or create in secure temp location with secure deletion.
3. Dangerous unwrap() Calls Can Panic
Files: src/run.rs:346-349, src/run.rs:432-440
secret.to_bytes().unwrap(), // Can panic on malformed secret
TOTP::new(...).unwrap(); // Can panic on config errorUsers will see panic backtraces instead of helpful error messages.
4. Silent Failure on TOTP Verification (Masks Time Sync Issues)
Files: src/run.rs:413, src/run.rs:447
if totp.check_current(code.trim()).unwrap_or(false) {unwrap_or(false) converts ALL errors to "invalid code" - including system clock desync, which is a common TOTP issue. Users with correct codes but wrong system time will be locked out with misleading errors.
5. Code Formatting Violations
Files: src/encryptedfs.rs, src/run.rs
Multiple formatting issues detected by cargo fmt --check. Must run cargo fmt --all before merge.
Important Issues (Should fix)
6. Error Context Discarded
File: src/run.rs:425-429
.map_err(|_| { // Original error discarded
error!("Failed to unlock 2FA. Password might be incorrect.");All errors collapsed to "password might be incorrect" even when the real issue is file corruption or permissions.
7. SHA1 Algorithm Not Documented
File: src/run.rs:342
SHA1 is used for TOTP compatibility, but this conflicts with project security principles. Should document why SHA1 is acceptable here (RFC 6238 standard, authenticator compatibility).
8. Hardcoded Account Name "MyVault"
File: src/run.rs:347
Users with multiple rencfs vaults cannot distinguish them in their authenticator app.
9. Wrong Comment Style for Public API
File: src/encryptedfs.rs:644-766
New functions use // comments instead of /// doc comments, inconsistent with rest of codebase.
10. Dead Code: verify_or_bind_identity and get_bound_identity
File: src/encryptedfs.rs:695-766
These functions are defined but never called. They reference "Google Account ID" but the implementation only does TOTP. Remove or complete the OAuth implementation.
11. Missing File Existence Check
File: src/encryptedfs.rs:674-693
get_totp_secret will throw IO error if identity.enc doesn't exist. Should check existence first like get_bound_identity does.
Test Coverage Analysis
Current State: ZERO tests for the new functionality.
Critical tests needed:
| Function | Priority |
|---|---|
bind_totp_secret + get_totp_secret roundtrip |
Critical |
| Wrong password returns error | Critical |
verify_or_bind_identity identity mismatch denied |
Critical |
is_identity_bound returns correct state |
High |
| Corrupted identity file handling | High |
Task Requirements Compliance
Per issue #226:
| Requirement | Status |
|---|---|
| Use Authenticator to unlock | Implemented |
| Use SMS/Email to unlock | Not implemented |
| Work with TPM chip | Not implemented |
| 2FA in addition to master pass | Implemented |
| "Remember pass" option with only 2FA | Partially (uses existing keyring but requires both on each mount) |
Summary of Required Actions
Before Merge (Critical)
- Memory-protect TOTP secret using
SecretString - Avoid writing secret to disk or use secure temp with overwrite-deletion
- Replace all
.unwrap()calls with proper error handling - Handle TOTP verification errors explicitly (don't use
unwrap_or(false)) - Run
cargo fmt --all
Should Address
- Preserve error context in
map_errclosures - Use
///doc comments for public functions - Remove or document dead code (
verify_or_bind_identity,get_bound_identity) - Make vault name configurable (not hardcoded "MyVault")
- Add existence check in
get_totp_secret
Add Tests
- Unit tests for all 5 new
EncryptedFsfunctions - Test wrong password scenarios
- Test identity mismatch denial
Documentation
- Document why SHA1 is used for TOTP
- Document security model (secret protected by filesystem password)
- Add recovery documentation (what if user loses authenticator?)
Strengths of This PR
- Correctly reuses existing crypto infrastructure (
atomic_serialize_encrypt_into) - Proper key derivation through existing
read_or_create_key - RAII cleanup pattern with
TempFileGuard - Clear user-facing setup instructions
- Security-critical operations logged appropriately
Signed-off-by: Cristian Andrei <cristian.andrei1423@gmail.com>
Signed-off-by: Cristian Andrei <cristian.andrei1423@gmail.com>
Signed-off-by: Cristian Andrei <cristian.andrei1423@gmail.com>
Signed-off-by: Cristian Andrei <cristian.andrei1423@gmail.com>
Signed-off-by: Cristian Andrei <cristian.andrei1423@gmail.com>
|
Hey @radumarias, thanks again for the detailed review! I've pushed some updates that tackle the critical memory protection for the TOTP secret, added the missing unit tests, and updated the recovery warnings if you could please review them. |
Title
Use authenticator TOTP as 2FA to unlock fs #319
Description
Implemented feature user with parameter --init-2fa to enable 2FA for any rencfs file system. It automatically opens the browser with a QR code and shows the additional code if that doesn't work. Once the 2fa is made for a system, it will always be requested.
Fixes # (issue)
#228
Type of change
Please delete options that are not relevant.
Checklist:
Screenshots (if applicable)
Add screenshots to illustrate changes if necessary.
Additional Context
Add any additional context or information here.