[SECURITY] Credential rotation scheduler + API permission validation - closes #58 #59

[Ibrahim-3d]

Summary

• [SECURITY] Implement credential rotation mechanism #58 — Adds CredentialRotationScheduler daemon thread that checks rotation status every N hours and fires a notification callback (GUI alert, log, etc.)
• [SECURITY] Add API key permission validation #59 — Adds PermissionValidator that validates exchange API permissions on startup against required/trading permission sets, with full JSONL audit trail
• validate_credentials_on_startup() convenience function handles both checks in one call
• Rotation uses backup/restore pattern — zero data loss if new credentials fail to encrypt

Changes

• Modified: app/pt_credentials.py — CredentialMetadata, rotation methods, PermissionValidator, CredentialRotationScheduler, validate_credentials_on_startup()
• New: app/test_credentials_rotation.py — 22 tests covering all scenarios

Usage

from pt_credentials import validate_credentials_on_startup, CredentialRotationScheduler

# Startup check (permissions + rotation warning)
ok, msg = validate_credentials_on_startup(
    permission_fetcher=lambda: exchange.get_permissions(),
    require_trading=True,
    notify_rotation=lambda w: show_alert(w),
)

# Background rotation scheduler
scheduler = CredentialRotationScheduler(
    notification_callback=lambda msg: logger.warning(msg),
    check_interval_hours=24,
)
scheduler.start()

Test plan

• 22 unit tests pass
• Rotation backup/restore tested
• Overdue and near-due warnings verified
• Permission audit JSONL log verified
• Fetcher exception handling tested

Closes #58
Closes #59

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds credential rotation scheduling, metadata tracking, and API permission validation to the credential management subsystem, plus unit tests.

Changes:

• Introduces CredentialMetadata, PermissionAuditResult, PermissionValidator, and CredentialRotationScheduler along with rotation/backup logic in SecureCredentialManager.
• Adds validate_credentials_on_startup convenience entry point and structured logging throughout.
• Adds unit tests covering metadata, encryption roundtrip, rotation, permission validation, and scheduler lifecycle.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 14 comments.

File	Description
app/pt_credentials.py	New rotation/permission classes + refactored encrypt/decrypt/migrate flow.
app/test_credentials_rotation.py	New tests for metadata, manager, validator, and scheduler.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[sjackson0109]

Hello @Ibrahim-3d, thank you for your great efforts here. I can see what you are striving to achieve, I would like to pull this branch and test it, just no time lately.

Can I ask you to review the copilot comments here #80 (review) and respond to each with either justification to resolve the dispute, or counter/dismiss it with a valid reason.

I’ll try and get some time towards the end of the week to look at your copy of this code. Looks comprehensive from my initial view.

[Ibrahim-3d]

All 14 Copilot review comments addressed in commit bf8a6bf. Inline replies posted directly on each thread. All threads resolved.

Note: Threads are resolved and collapsed. To view any thread, open the Files changed tab and click "Show resolved" or the collapsed thread indicator.

#	File	Topic	Status
1	pt_credentials.py	all_ok expression always evaluates False - dead code	Resolved
2	pt_credentials.py	Rotation rollback does not restore metadata file	Resolved
3	pt_credentials.py	Partial write leaves mismatched key/secret pair on disk	Resolved
4	pt_credentials.py	rotation_interval_days not updated in existing metadata on re-encrypt	Resolved
5	pt_credentials.py	_get_machine_password uses Windows-only env vars	Resolved
6	pt_credentials.py	Rotation scheduler fires callback every tick while overdue	Resolved
7	pt_credentials.py	timedelta imported but never used	Resolved
8	pt_credentials.py	Audit log has no size cap or rotation	Resolved
9	pt_credentials.py	Plaintext fallback removed, silently locking out users on vault failure	Resolved
10	test_credentials_rotation.py	tearDown missing, scheduler callback not asserted	Resolved
11	test_credentials_rotation.py	import shutil local inside tearDown	Resolved
12	pt_credentials.py	import shutil local inside rotate_credentials	Resolved
13	pt_credentials.py	from_dict silently drops unknown keys but raises TypeError on missing required fields	Resolved
14	pt_credentials.py	%d format truncates float check_interval_hours in log output	Resolved

[Ibrahim-3d]

Local CI Simulation Results

Ran all workflow steps locally against this branch prior to owner approval. Results:

Code Quality & Testing

Check	Tool	Result
Code formatting	black --check	✅ Pass — 0 files need reformatting
Import ordering	isort --check	✅ Pass
Linting (hard errors)	flake8 --select=E9,F63,F7,F82	✅ Pass — 0 syntax/undefined-name errors
Linting (extended)	flake8 --exit-zero	✅ Pass — 0 warnings in new files
Unit tests	pytest	✅ All tests pass (see table below)

Unit Test Results

File	Tests	Result
test_circuit_breaker.py	20	✅ All pass
test_credentials_rotation.py	30	✅ All pass
test_error_handler.py	22	✅ All pass
test_backup_validation.py	35	✅ All pass
test_database_manager.py	29	✅ All pass
test_security_logger.py	25	✅ All pass

CI/CD Pipeline

Workflow	Status	Reason
Code Quality & Testing	⏳ Awaiting owner approval	Fork PR — first-time contributor gate
PowerTrader AI+ CI/CD	⏳ Awaiting owner approval	Fork PR — first-time contributor gate
Project Management	⏳ Awaiting owner approval	Fork PR — first-time contributor gate

All three workflows are queued with action_required status. This is GitHub's security gate for PRs from fork contributors. Once approved by @sjackson0109, they will execute against the same code that was verified locally above.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 19 comments.

[sjackson0109]

@Ibrahim-3d we have another large series of copilot reviewer comments. Any chance you can read and respond to them? #80 (review)

[Ibrahim-3d]

Manual Testing Guide — PR #80: Credential Rotation + Permission Validation

Prerequisites

git fetch fork && git checkout feat/58-59-credential-rotation-permission-validation
cd app
pip install cryptography

1. Run unit tests

python -m pytest test_credentials_rotation.py -v

Expected: All 30 tests pass.

2. Smoke test — full rotation with rollback protection

python -c "
import tempfile
from pt_credentials import SecureCredentialManager
with tempfile.TemporaryDirectory() as d:
    m = SecureCredentialManager(d)
    # Initial encrypt
    m.encrypt_credentials('key_v1', 'secret_v1')
    print('v1 encrypted:', m.decrypt_credentials())
    # Rotate to v2
    ok = m.rotate_credentials('key_v2', 'secret_v2')
    print('rotation ok:', ok)
    print('v2 decrypts:', m.decrypt_credentials())
    # Verify created_at preserved
    meta1 = m._load_metadata()
    m.rotate_credentials('key_v3', 'secret_v3')
    meta2 = m._load_metadata()
    assert meta1.created_at == meta2.created_at, 'created_at changed!'
    print('PASS: created_at preserved across rotation')
"

3. Verify atomic rollback (key change)

python -c "
import tempfile, unittest.mock
from pt_credentials import SecureCredentialManager
with tempfile.TemporaryDirectory() as d:
    m = SecureCredentialManager(d)
    m.encrypt_credentials('orig_key', 'orig_secret')
    # Force encrypt_credentials to fail mid-way
    with unittest.mock.patch.object(m, 'encrypt_credentials', return_value=False):
        result = m.rotate_credentials('new_key', 'new_secret')
    print('rotation result (should be False):', result)
    # Original creds must still be intact
    creds = m.decrypt_credentials()
    print('creds after failed rotation:', creds)
    assert creds == ('orig_key', 'orig_secret'), f'FAIL: got {creds}'
    print('PASS: rollback preserved original credentials')
"

4. Verify rotation warning fires once (not every tick)

python -c "
import tempfile, time
from pt_credentials import SecureCredentialManager, CredentialRotationScheduler
fires = []
with tempfile.TemporaryDirectory() as d:
    m = SecureCredentialManager(d)
    m.encrypt_credentials('k', 's', rotation_interval_days=0)  # immediately overdue
    sched = CredentialRotationScheduler(m, check_interval_hours=0.0001,
                                        callback=lambda w: fires.append(w))
    sched.start()
    time.sleep(0.1)
    sched.stop()
    print('callback fired', len(fires), 'time(s)')
    # Should fire once (warning text unchanged = no re-fire)
    assert len(fires) >= 1, 'callback never fired'
    print('PASS')
"

Rollback

git checkout main -- app/pt_credentials.py app/test_credentials_rotation.py

[Ibrahim-3d]

All 19 round-2 Copilot threads addressed in commit cd45203 and resolved. Inline replies posted on each thread with the specific fix or rationale. Tests passing locally (32 tests, all green).

Highlights:

• [FEATURE] Strategy: Bollinger mean reversion with regime filter #124 (machine-password compat): decrypt now falls back to legacy COMPUTERNAME/USERNAME derivation and re-encrypts with new key — existing Windows vaults migrate transparently.
• #633 (plaintext fallback): log level raised to error with explicit SECURITY DEGRADATION message.
• #137 (tmp collision): atomic writes use tempfile.mkstemp for unique per-writer names.
• #514 (audit log O(n)): replaced read-rewrite with O(1) size-based rotation to .1.
• Scheduler dedup tests: extracted _tick() method; tests now exercise real code path.

Ready for re-review.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Ibrahim-3d]

@sjackson0109 all 19 round-2 Copilot threads addressed in cd45203, replied inline, resolved. 32 tests pass. Ready for re-review.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

[sjackson0109]

@Ibrahim-3d This PR review #80 (review) has left us with 4x open comments/concerns of varying risk-levels. Can you review and repy to each (inline, or all at once).
Aside from this, the PR looks clean and ready to merge.

[Ibrahim-3d]

@sjackson0109 round-3 done — addressed in 4c5c927:

#	Issue	Fix
1	encrypt_credentials non-atomic key+secret writes — partial-failure could yield new-key/old-secret pair	Two-phase commit via new _stage_temp_binary: both ciphertexts staged first, then both os.replaced. If second rename fails, previous key ciphertext restored from in-memory snapshot
2	missing_required / missing_trading had set-ordered output — noisy audit log	All four list constructions use sorted(...) now
3	AUDIT_ROTATION_KEEP defined but unused	_rotate_audit_if_needed now shifts .N-1 → .N and drops anything past the keep window
4	_tick updated _last_warning after callback — broken callback caused spam	Dedup state set before callback; callback exceptions caught + logged with exc_info=True, isolated from dedup state

32 unit tests pass locally. Threads resolved.

[Ibrahim-3d]

Hey - please hold off on merging this one for a moment. I want to add a round-trip migration test for the new vault derivation before this lands, since changing how credentials are scrambled could lock users out if we get it wrong. Will follow up here as soon as the test is in.

[Ibrahim-3d]

Test is in. The new test (test_legacy_vault_auto_migrates_to_new_derivation) walks the whole upgrade path: encrypt a vault with the old Windows-only password, upgrade to the new cross-platform derivation, decrypt successfully via the legacy fallback, and confirm the vault gets quietly rewritten under the new key on disk so the next decrypt does not need the fallback. 33 tests passing. Good to review when you have a minute.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Ibrahim-3d]

@copilot review

[sjackson0109]

Really happy to see this change go in...

[Copilot]

Pull request overview

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

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.