Add PiOS wallet security architecture - #2
Open
Sidzeppelin95 wants to merge 1 commit into
Open
Conversation
Reviewer's GuideIntroduces a PiOS-compatible configuration class, in-memory wallet and security models, and a trust-based security engine so that revoked passphrases are denied but still drive risk analysis, recovery workflows, and analyst review. Sequence diagram for revoked passphrase authentication and security enginesequenceDiagram
actor User
participant PiWalletManager
participant wallet_db
participant PiWallet
participant PiSecurityEngine
participant PiTrustAnalyzer
participant security_events_db
User->>PiWalletManager: authenticate(wallet_id, entered_passphrase, ip_address, device_id)
PiWalletManager->>wallet_db: get(wallet_id)
wallet_db-->>PiWalletManager: PiWallet
PiWalletManager->>PiWalletManager: hash_passphrase(entered_passphrase)
alt [entered_hash == active_passphrase_hash]
PiWalletManager-->>User: True
else [entered_hash in revoked_passphrase_hashes]
PiWalletManager->>PiSecurityEngine: handle_old_passphrase_attempt(wallet, ip_address, device_id)
PiSecurityEngine->>PiTrustAnalyzer: is_trusted_device(wallet, device_id)
PiTrustAnalyzer-->>PiSecurityEngine: trusted_device
PiSecurityEngine->>PiTrustAnalyzer: is_trusted_ip(wallet, ip_address)
PiTrustAnalyzer-->>PiSecurityEngine: trusted_ip
PiSecurityEngine->>PiTrustAnalyzer: calculate_risk_score(wallet, ip_address, device_id)
PiTrustAnalyzer-->>PiSecurityEngine: risk_score
PiSecurityEngine->>PiTrustAnalyzer: classify_attempt(risk_score, trusted_device, trusted_ip)
PiTrustAnalyzer-->>PiSecurityEngine: classification
PiSecurityEngine->>PiSecurityEngine: trigger_response(wallet, event)
PiSecurityEngine->>security_events_db: store SecurityEvent
PiWalletManager-->>User: False
else [unknown passphrase]
PiWalletManager-->>User: False
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The mutual dependency between
wallet_managerandsecurity_engine(top-level import ofSecurityUtilsfromwallet_managerand runtime import ofPiSecurityEngineinsideauthenticate) is brittle; consider extracting shared utilities/types into a separate module to avoid circular-coupling and simplify imports. - Using
datetime.utcnow()and storing naive datetimes forcreated_atandrecovery_locked_untilcan lead to subtle bugs once you start mixing with timezone-aware values or external systems; consider standardizing on timezone-aware UTC datetimes. - The risk scoring in
PiTrustAnalyzer.calculate_risk_scorebased on substrings like'vpn'or'tor'in thedevice_idis quite ad hoc and tightly couples semantics to an identifier string; it may be more robust to pass explicit network/connection metadata into the analyzer instead of inferring it fromdevice_id.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The mutual dependency between `wallet_manager` and `security_engine` (top-level import of `SecurityUtils` from `wallet_manager` and runtime import of `PiSecurityEngine` inside `authenticate`) is brittle; consider extracting shared utilities/types into a separate module to avoid circular-coupling and simplify imports.
- Using `datetime.utcnow()` and storing naive datetimes for `created_at` and `recovery_locked_until` can lead to subtle bugs once you start mixing with timezone-aware values or external systems; consider standardizing on timezone-aware UTC datetimes.
- The risk scoring in `PiTrustAnalyzer.calculate_risk_score` based on substrings like `'vpn'` or `'tor'` in the `device_id` is quite ad hoc and tightly couples semantics to an identifier string; it may be more robust to pass explicit network/connection metadata into the analyzer instead of inferring it from `device_id`.
## Individual Comments
### Comment 1
<location path="pishield/backend/wallet_manager.py" line_range="111-114" />
<code_context>
+ raise ValueError("Wallet not found")
+
+ entered_hash = SecurityUtils.hash_passphrase(entered_passphrase)
+ if entered_hash == wallet.active_passphrase_hash:
+ return True
+
+ if entered_hash in wallet.revoked_passphrase_hashes:
+ from security_engine import PiSecurityEngine
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Recovery lock state is never enforced during authentication.
`PiSecurityEngine.trigger_response` sets `wallet.recovery_locked_until`, but `authenticate` never checks it, so locked wallets still accept attempts (including revoked passphrases). Add a check that short-circuits when `recovery_locked_until` is in the future (optionally with a distinct return/status) so the lock is actually enforced.
</issue_to_address>
### Comment 2
<location path="pishield/backend/security_engine.py" line_range="83" />
<code_context>
+ if "tor" in lowered_device_id:
+ score += 35
+
+ if ip_address.startswith("192.168"):
+ score -= 10
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Private-network heuristic only covers 192.168/16 and may misclassify other private ranges.
This logic only treats `192.168.0.0/16` as lower risk and skips other RFC1918 ranges like `10.0.0.0/8` and `172.16.0.0/12`, which could inflate risk scores for those users. Consider broadening the check (e.g., using `ipaddress` to test `is_private`) if the goal is to treat all private-network addresses more favorably.
Suggested implementation:
```python
from dataclasses import dataclass
from datetime import datetime, timedelta
from typing import TYPE_CHECKING, Dict, Optional
import ipaddress
```
```python
lowered_device_id = device_id.lower()
if "vpn" in lowered_device_id:
score += 20
if "tor" in lowered_device_id:
score += 35
# Treat private-network addresses (RFC1918, etc.) as slightly lower risk.
try:
ip_obj = ipaddress.ip_address(ip_address)
except ValueError:
# If the IP is malformed or missing, leave the score unchanged.
pass
else:
if ip_obj.is_private:
score -= 10
```
</issue_to_address>
### Comment 3
<location path="pishield/backend/security_engine.py" line_range="43-52" />
<code_context>
+class PiSecurityReviewSystem:
+ """Analyst review workflow for flagged security events."""
+
+ @staticmethod
+ def review_event(event_id: str, suspicious: bool, notes: str) -> SecurityEvent:
+ event = security_events_db.get(event_id)
+ if not event:
+ raise ValueError("Security event not found")
+
+ event.analyst_notes = notes
+ if suspicious:
+ event.status = "CONFIRMED_PHISHING_ACTIVITY"
+ PiSecurityReviewSystem.take_action(event)
+ else:
</code_context>
<issue_to_address>
**nitpick:** Status assignment in `take_action` duplicates state already set in `review_event`.
`review_event` sets `event.status = "CONFIRMED_PHISHING_ACTIVITY"` before calling `take_action`, which then sets the same status again. Consider choosing a single place to own this side effect—either keep the status change in `take_action` and remove it from `review_event`, or vice versa—to avoid duplication and clarify responsibility.
</issue_to_address>
### Comment 4
<location path="pishield/backend/config.py" line_range="4" />
<code_context>
-PI_APP_NAME = "PiShield"
-PI_API_KEY = "YOUR_PI_API_KEY"
+class PiOSConfig:
+ """Configuration values for Pi Browser and wallet-security workflows."""
</code_context>
<issue_to_address>
**issue (complexity):** Consider making PiOSConfig the single source of truth for URLs and using clearly named legacy aliases so configuration stays centralized and unambiguous.
You can reduce the added complexity by making `PiOSConfig` the single source of truth and turning the module-level constants into *trivial* passthroughs with clear semantics.
Concrete suggestions:
1. **Disambiguate PROD vs DEV URLs in `PiOSConfig`**
Right now `APP_URL` vs `DEV_URL` plus `PI_APP_URL = PiOSConfig.DEV_URL` is semantically muddy. Make the meaning explicit in the class:
```python
class PiOSConfig:
"""Configuration values for Pi Browser and wallet-security workflows."""
APP_NAME = "PiShield"
APP_VERSION = "1.0.0"
PIOS_COMPATIBLE = True
PROD_APP_URL = "https://pishield.pinet.com"
DEV_APP_URL = "http://localhost:31415"
API_URL = "https://api.pishield.pinet.com"
PRIVACY_POLICY_URL = "https://pishield.pinet.com/privacy-policy"
TERMS_OF_SERVICE_URL = "https://pishield.pinet.com/terms"
SANDBOX_URL = "https://sandbox.minepi.com"
PI_BROWSER_REQUIRED = True
PI_SDK_ENABLED = True
PI_MAINNET_ENABLED = True
ROTATION_DELAY_HOURS = 48
RECOVERY_LOCK_HOURS = 24
THREAT_SCORE_THRESHOLD = 70
HIGH_RISK_THRESHOLD = 90
MAX_RECOVERY_ATTEMPTS = 3
```
2. **Make aliases trivial and clearly “legacy”**
Keep behavior identical (Flask still uses dev URL) but make the mapping obvious and fully driven by `PiOSConfig`:
```python
# Backwards-compatible aliases used by the existing Flask app/config imports.
# TODO: migrate callers to PiOSConfig and remove these aliases.
PI_SANDBOX = True
PI_APP_NAME = PiOSConfig.APP_NAME
PI_API_KEY = "YOUR_PI_API_KEY"
PI_NETWORK = "Pi Testnet"
# Legacy Flask uses dev URL for the app:
PI_APP_URL = PiOSConfig.DEV_APP_URL
PI_SANDBOX_URL = PiOSConfig.SANDBOX_URL
PRIVACY_POLICY_URL = PiOSConfig.PRIVACY_POLICY_URL
TERMS_OF_SERVICE_URL = PiOSConfig.TERMS_OF_SERVICE_URL
```
This keeps all current functionality but:
- Eliminates the “which URL is which?” ambiguity (`DEV_APP_URL` vs `PROD_APP_URL`).
- Makes it clear that module-level constants are legacy shims, not a second config surface.
- Ensures all URLs live in one place (`PiOSConfig`), so future changes don’t have to reconcile literals vs class attributes.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+111
to
+114
| if entered_hash == wallet.active_passphrase_hash: | ||
| return True | ||
|
|
||
| if entered_hash in wallet.revoked_passphrase_hashes: |
There was a problem hiding this comment.
issue (bug_risk): Recovery lock state is never enforced during authentication.
PiSecurityEngine.trigger_response sets wallet.recovery_locked_until, but authenticate never checks it, so locked wallets still accept attempts (including revoked passphrases). Add a check that short-circuits when recovery_locked_until is in the future (optionally with a distinct return/status) so the lock is actually enforced.
| if "tor" in lowered_device_id: | ||
| score += 35 | ||
|
|
||
| if ip_address.startswith("192.168"): |
There was a problem hiding this comment.
suggestion (bug_risk): Private-network heuristic only covers 192.168/16 and may misclassify other private ranges.
This logic only treats 192.168.0.0/16 as lower risk and skips other RFC1918 ranges like 10.0.0.0/8 and 172.16.0.0/12, which could inflate risk scores for those users. Consider broadening the check (e.g., using ipaddress to test is_private) if the goal is to treat all private-network addresses more favorably.
Suggested implementation:
from dataclasses import dataclass
from datetime import datetime, timedelta
from typing import TYPE_CHECKING, Dict, Optional
import ipaddress lowered_device_id = device_id.lower()
if "vpn" in lowered_device_id:
score += 20
if "tor" in lowered_device_id:
score += 35
# Treat private-network addresses (RFC1918, etc.) as slightly lower risk.
try:
ip_obj = ipaddress.ip_address(ip_address)
except ValueError:
# If the IP is malformed or missing, leave the score unchanged.
pass
else:
if ip_obj.is_private:
score -= 10
Comment on lines
+43
to
+52
| @staticmethod | ||
| def is_trusted_device(wallet: "PiWallet", device_id: str) -> bool: | ||
| return any(device.device_id == device_id for device in wallet.trusted_devices) | ||
|
|
||
| @staticmethod | ||
| def is_trusted_ip(wallet: "PiWallet", ip_address: str) -> bool: | ||
| return ip_address in wallet.trusted_ips | ||
|
|
||
| @staticmethod | ||
| def get_device(wallet: "PiWallet", device_id: str): |
There was a problem hiding this comment.
nitpick: Status assignment in take_action duplicates state already set in review_event.
review_event sets event.status = "CONFIRMED_PHISHING_ACTIVITY" before calling take_action, which then sets the same status again. Consider choosing a single place to own this side effect—either keep the status change in take_action and remove it from review_event, or vice versa—to avoid duplication and clarify responsibility.
| PI_APP_NAME = "PiShield" | ||
|
|
||
| PI_API_KEY = "YOUR_PI_API_KEY" | ||
| class PiOSConfig: |
There was a problem hiding this comment.
issue (complexity): Consider making PiOSConfig the single source of truth for URLs and using clearly named legacy aliases so configuration stays centralized and unambiguous.
You can reduce the added complexity by making PiOSConfig the single source of truth and turning the module-level constants into trivial passthroughs with clear semantics.
Concrete suggestions:
- Disambiguate PROD vs DEV URLs in
PiOSConfig
Right now APP_URL vs DEV_URL plus PI_APP_URL = PiOSConfig.DEV_URL is semantically muddy. Make the meaning explicit in the class:
class PiOSConfig:
"""Configuration values for Pi Browser and wallet-security workflows."""
APP_NAME = "PiShield"
APP_VERSION = "1.0.0"
PIOS_COMPATIBLE = True
PROD_APP_URL = "https://pishield.pinet.com"
DEV_APP_URL = "http://localhost:31415"
API_URL = "https://api.pishield.pinet.com"
PRIVACY_POLICY_URL = "https://pishield.pinet.com/privacy-policy"
TERMS_OF_SERVICE_URL = "https://pishield.pinet.com/terms"
SANDBOX_URL = "https://sandbox.minepi.com"
PI_BROWSER_REQUIRED = True
PI_SDK_ENABLED = True
PI_MAINNET_ENABLED = True
ROTATION_DELAY_HOURS = 48
RECOVERY_LOCK_HOURS = 24
THREAT_SCORE_THRESHOLD = 70
HIGH_RISK_THRESHOLD = 90
MAX_RECOVERY_ATTEMPTS = 3- Make aliases trivial and clearly “legacy”
Keep behavior identical (Flask still uses dev URL) but make the mapping obvious and fully driven by PiOSConfig:
# Backwards-compatible aliases used by the existing Flask app/config imports.
# TODO: migrate callers to PiOSConfig and remove these aliases.
PI_SANDBOX = True
PI_APP_NAME = PiOSConfig.APP_NAME
PI_API_KEY = "YOUR_PI_API_KEY"
PI_NETWORK = "Pi Testnet"
# Legacy Flask uses dev URL for the app:
PI_APP_URL = PiOSConfig.DEV_APP_URL
PI_SANDBOX_URL = PiOSConfig.SANDBOX_URL
PRIVACY_POLICY_URL = PiOSConfig.PRIVACY_POLICY_URL
TERMS_OF_SERVICE_URL = PiOSConfig.TERMS_OF_SERVICE_URLThis keeps all current functionality but:
- Eliminates the “which URL is which?” ambiguity (
DEV_APP_URLvsPROD_APP_URL). - Makes it clear that module-level constants are legacy shims, not a second config surface.
- Ensures all URLs live in one place (
PiOSConfig), so future changes don’t have to reconcile literals vs class attributes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Description
PiOSConfigwith Pi Browser / SDK flags and thresholds and preserve previous module-level aliases for backwards compatibility inpishield/backend/config.py.SecurityUtils,PiDeviceProfile,PiWallet, andPiWalletManagerinpishield/backend/wallet_manager.pywith passphrase hashing, wallet creation,rotate_passphrase, andauthenticatethat denies revoked passphrases and records events.SecurityEvent,PiTrustAnalyzer,PiSecurityEngine, andPiSecurityReviewSysteminpishield/backend/security_engine.pyto calculate risk scores, classify attempts (LIKELY_GENUINE_OWNER_RECOVERY,VERIFICATION_REQUIRED,LIKELY_PHISHING_ACTOR), generate security events, trigger recovery/escallation responses, and support analyst review.wallet_db,security_events_db) and keep interfaces compatible with the existing Flask app for incremental integration.Testing
python -m compileall pishield/backendto validate syntax, and the compilation succeeded.PYTHONPATH=pishield/backend python - <<'PY'that exercised wallet creation,rotate_passphrase,authenticatefor active and revoked passphrases, validated generated classifications, checkedsuspicious_attempt_count, and checkedrecovery_locked_until, and all assertions passed.Codex Task
Summary by Sourcery
Introduce PiOS-compatible wallet security architecture with configurable risk thresholds, passphrase rotation, and revoked-passphrase handling, including trust analysis and analyst-driven review workflows.
New Features:
Enhancements: