From 1344fc80e56315c5c99f58993f9ee02ce79e8a76 Mon Sep 17 00:00:00 2001 From: tegwick Date: Mon, 12 Oct 2026 01:23:29 +0200 Subject: [PATCH] fix: preserve directory subjects in factor recovery Assistant: codex Assistant-Model: gpt-6-astra Assistant-Session: 01a12d13-27f2-7163-b8c0-f682af02772c --- scripts/factor_recovery.py | 18 +++++- scripts/recovery_service.py | 48 +++++++++----- scripts/test_recovery_service.py | 64 +++++++++++++++++++ .../KEY-WP-0040-mfa-attended-run-findings.md | 37 +++++++++-- 4 files changed, 145 insertions(+), 22 deletions(-) diff --git a/scripts/factor_recovery.py b/scripts/factor_recovery.py index a945173..1083231 100644 --- a/scripts/factor_recovery.py +++ b/scripts/factor_recovery.py @@ -2,7 +2,18 @@ import hashlib,json,re ACTION='keycape.factor.recovery' -class RecoveryError(Exception):pass +class RecoveryError(Exception): + def __init__(self, code, field=None): + super().__init__(code) + self.field=field + +def validate_actor(value): + # The issuer's subject is an opaque identity (LLDAP uses a DN), not a login. + # Preserve its exact bytes for ticket/receipt binding; never normalize it. + if (not isinstance(value,str) or not value or len(value)>150 + or not value.isascii() or not value.isprintable() or not value.strip()): + raise RecoveryError('invalid_actor',field='actor') + return value def fingerprint(snapshot): return hashlib.sha256(json.dumps(snapshot,sort_keys=True,separators=(',',':')).encode()).hexdigest() @@ -13,9 +24,10 @@ def recover(store, request): return _recover(store,request) def _recover(store, request): - for key in ('user','serial','realm','actor','reference'): + validate_actor(request.get('actor')) + for key in ('user','serial','realm','reference'): value=request.get(key) - if not isinstance(value,str) or not value or len(value)>200 or not re.fullmatch(r'[A-Za-z0-9_.@:/-]+',value):raise RecoveryError('invalid_request') + if not isinstance(value,str) or not value or len(value)>200 or not re.fullmatch(r'[A-Za-z0-9_.@:/-]+',value):raise RecoveryError('invalid_request',field=key) if request['realm']!=store.realm:raise RecoveryError('wrong_realm') snapshot=store.snapshot(request['user'],request['serial']) if snapshot is None:raise RecoveryError('factor_not_owned_by_target') diff --git a/scripts/recovery_service.py b/scripts/recovery_service.py index 9ae95b7..ca0bb2a 100644 --- a/scripts/recovery_service.py +++ b/scripts/recovery_service.py @@ -4,7 +4,21 @@ import hashlib import hmac import json import time -from factor_recovery import ProviderStore, RecoveryError, recover, fingerprint +from factor_recovery import ProviderStore, RecoveryError, recover, fingerprint, validate_actor + + +def report_validation_failure(error): + # Keep provider logging disabled. This separate diagnostic contains only + # closed field names and codes, never claims, bodies, tokens or exceptions. + import sys + if str(error) not in {'invalid_request', 'invalid_actor'}: + return + field = error.field if error.field in { + 'actor', 'user', 'reference', 'serial', 'realm', 'action', + 'request', 'content_length', 'body', + } else 'request' + sys.stderr.write(json.dumps(dict(event='factor_recovery_validation_failed', + code=str(error), field=field))+'\n') def authorized_actor(claims, now=None): @@ -17,10 +31,7 @@ def authorized_actor(claims, now=None): or not isinstance(assurance.get('at'), (int, float)) or not 0 <= now - assurance['at'] <= 300): raise RecoveryError('fresh_platform_mfa_required') - subject = claims.get('sub') - if not isinstance(subject, str) or not subject or len(subject) > 150: - raise RecoveryError('invalid_actor') - return subject + return validate_actor(claims.get('sub')) class RecoveryService: @@ -52,9 +63,9 @@ class RecoveryService: user, reference = body.get('user', ''), body.get('reference', '') # Validate the target and support reference before any directory query. import re - if any(not isinstance(v, str) or not re.fullmatch(r'[A-Za-z0-9_.@:/-]{1,150}', v) - for v in (user, reference)): - raise RecoveryError('invalid_request') + for field, value in (('user', user), ('reference', reference)): + if not isinstance(value, str) or not re.fullmatch(r'[A-Za-z0-9_.@:/-]{1,150}', value): + raise RecoveryError('invalid_request', field=field) factors = [] for serial in self.store.serials(user): request = dict(user=user, reference=reference, serial=serial, @@ -68,7 +79,7 @@ class RecoveryService: import re reference = body.get('reference', '') if not isinstance(reference,str) or not re.fullmatch(r'[A-Za-z0-9_.@:/-]{1,150}',reference): - raise RecoveryError('invalid_request') + raise RecoveryError('invalid_request', field='reference') row = self.store.receipt(reference) if row is None: return dict(success=True,status='not_found',reference=reference) @@ -90,7 +101,7 @@ class RecoveryService: raise RecoveryError('identity_verification_required') request = self.unticket(body.get('confirmation', ''), actor) return dict(recover(self.store, dict(request, apply=True, identity_verified=True)), success=True) - raise RecoveryError('invalid_request') + raise RecoveryError('invalid_request', field='action') def __call__(self, environ, start_response): if environ.get('PATH_INFO') == '/healthz' and environ.get('REQUEST_METHOD') == 'GET': @@ -99,18 +110,25 @@ class RecoveryService: status = '200 OK' try: if environ.get('PATH_INFO') != '/recover' or environ.get('REQUEST_METHOD') != 'POST': - raise RecoveryError('invalid_request') - length = int(environ.get('CONTENT_LENGTH') or 0) + raise RecoveryError('invalid_request', field='request') + try: + length = int(environ.get('CONTENT_LENGTH') or 0) + except (ValueError, TypeError): + raise RecoveryError('invalid_request', field='content_length') from None if not 0 < length <= 16384: - raise RecoveryError('invalid_request') + raise RecoveryError('invalid_request', field='content_length') auth = environ.get('HTTP_AUTHORIZATION', '') if not auth.startswith('Bearer ') or len(auth) > 16384: raise RecoveryError('fresh_platform_mfa_required') - body = json.loads(environ['wsgi.input'].read(length)) + try: + body = json.loads(environ['wsgi.input'].read(length)) + except (ValueError, UnicodeError): + raise RecoveryError('invalid_request', field='body') from None if not isinstance(body, dict): - raise RecoveryError('invalid_request') + raise RecoveryError('invalid_request', field='body') result = self.operation(auth[7:], body) except RecoveryError as exc: + report_validation_failure(exc) status = '403 Forbidden' if str(exc) == 'fresh_platform_mfa_required' else '409 Conflict' result = dict(success=False, failure=str(exc)) except Exception: diff --git a/scripts/test_recovery_service.py b/scripts/test_recovery_service.py index f07f184..6cdb5ac 100644 --- a/scripts/test_recovery_service.py +++ b/scripts/test_recovery_service.py @@ -1,6 +1,7 @@ import io import json import unittest +from contextlib import redirect_stderr from recovery_service import RecoveryService, authorized_actor from factor_recovery import RecoveryError from test_factor_recovery import Store @@ -59,3 +60,66 @@ class RecoveryServiceTests(unittest.TestCase): self.service.operation('signed',dict(action='apply',confirmation=status['confirmation'],identity_verified=True)) self.assertEqual('recovered',self.service.operation('signed',dict(action='status',reference='case-1'))['status']) self.assertEqual(1,self.store.writes) + + def test_directory_subject_preview_apply_and_replay(self): + # LLDAP adapter returns entry.DN; OIDC emits that exact user.ID as sub. + subject='uid=platform-root,ou=people,dc=netkingdom,dc=local' + self.claims['sub']=subject + preview=self.preview() + self.assertEqual(subject,preview['actor']) + self.assertEqual(0,self.store.writes) + body=dict(action='apply',confirmation=preview['confirmation'],identity_verified=True) + self.assertTrue(self.service.operation('signed',body)['changes_applied']) + self.assertTrue(self.service.operation('signed',body)['replayed']) + self.assertEqual(subject,self.store.rows['case-1']['actor']) + self.assertEqual(1,self.store.writes) + + def test_directory_subject_ticket_is_bound_to_exact_actor(self): + self.claims['sub']='uid=admin,ou=people,dc=example' + ticket=self.preview()['confirmation'] + self.claims['sub']='uid=other,ou=people,dc=example' + with self.assertRaisesRegex(RecoveryError,'preview_expired_or_changed'): + self.service.operation('signed',dict(action='apply',confirmation=ticket,identity_verified=True)) + self.assertEqual(0,self.store.writes) + + def test_actor_validation_preserves_identity_and_rejects_controls(self): + from factor_recovery import validate_actor + subject='cn=Platform Root,ou=people,dc=example' + self.assertEqual(subject,validate_actor(subject)) + for value in [None,{},'', ' ', 'a'*151, 'uid=admin\nsecret', 'admin\x00', 'admin\x7f', 'ü']: + with self.subTest(value=repr(value)): + with self.assertRaisesRegex(RecoveryError,'invalid_actor'): + authorized_actor(dict(self.claims,sub=value),1000) + self.assertEqual(0,self.store.writes) + + def test_wsgi_validation_diagnostics_never_contain_values(self): + secret='private-value-must-not-appear' + cases=[ + (dict(action='preview',user=secret+'!',reference='case-1'),None,'user'), + (dict(action='preview',user='alice',reference=secret+'!'),None,'reference'), + (dict(action=secret),None,'action'), + ([secret],None,'body'), + (dict(action='preview',user='alice',reference='case-1'),secret,'content_length'), + ] + for body,length,field in cases: + data=json.dumps(body).encode();diagnostic=io.StringIO() + with redirect_stderr(diagnostic): + result=b''.join(self.service(dict(PATH_INFO='/recover',REQUEST_METHOD='POST', + HTTP_AUTHORIZATION='Bearer '+secret,CONTENT_LENGTH=length or str(len(data)), + **{'wsgi.input':io.BytesIO(data)}),lambda s,h:None)) + record=json.loads(diagnostic.getvalue()) + self.assertEqual(dict(event='factor_recovery_validation_failed',code='invalid_request',field=field),record) + self.assertNotIn(secret,diagnostic.getvalue()+result.decode()) + self.assertEqual(0,self.store.writes) + + def test_provider_field_failure_is_identifiable_without_serial_value(self): + self.store.serials=lambda user:['private-serial!'] + data=json.dumps(dict(action='preview',user='alice',reference='case-1')).encode() + diagnostic=io.StringIO() + with redirect_stderr(diagnostic): + result=b''.join(self.service(dict(PATH_INFO='/recover',REQUEST_METHOD='POST', + HTTP_AUTHORIZATION='Bearer signed',CONTENT_LENGTH=str(len(data)), + **{'wsgi.input':io.BytesIO(data)}),lambda s,h:None)) + self.assertEqual('serial',json.loads(diagnostic.getvalue())['field']) + self.assertNotIn('private-serial',diagnostic.getvalue()+result.decode()) + self.assertEqual(0,self.store.writes) diff --git a/workplans/KEY-WP-0040-mfa-attended-run-findings.md b/workplans/KEY-WP-0040-mfa-attended-run-findings.md index d356c17..d65929b 100644 --- a/workplans/KEY-WP-0040-mfa-attended-run-findings.md +++ b/workplans/KEY-WP-0040-mfa-attended-run-findings.md @@ -4,11 +4,11 @@ type: workplan title: "Fix findings from the first attended MFA run (factor recovery preview, OTP prompt help)" domain: infotech repo: key-cape -status: ready +status: active owner: claude-code topic_slug: netkingdom created: "2026-10-11" -updated: "2026-10-11" +updated: "2026-10-12" flavor: implementation depends_on: [] state_hub_workstream_id: "a67f9d60-9b8f-5111-9fe5-9f64e631b1c3" @@ -21,7 +21,7 @@ U05–U07 passed. U08 is blocked by T01. ```task id: KEY-WP-0040-T01 -status: todo +status: progress priority: high state_hub_task_id: "b385602d-714a-5fdb-b40b-bd870420f28e" ``` @@ -44,11 +44,23 @@ Done when: Then hand back to user-engine, which re-runs U08 (USER-WP-0028-T03). The fix and roll-out of the sidecar are blue lane under the build-mode change rule. +2026-10-12 source repair: LLDAP adapter returns entry.DN as user.ID and the token +issuer emits that exact value as sub. Recovery incorrectly applied the login +regex to actor, rejecting comma/equal characters. Two DN-shaped subject tests +reproduce invalid_request against the old source. The repair validates the +bounded printable ASCII actor separately, preserves exact ticket/audit binding, +and keeps target/serial/realm/reference validation strict. Validation diagnostics +emit only closed field names and error codes to stderr; provider logging stays +disabled. Twenty recovery tests pass, including identity switching, malformed +input, provider field failures, replay and mutation/audit guards. Go OIDC and +LLDAP suites pass. No live factor was read or changed. T01 stays progress until +the deployed authenticated preview succeeds and user-engine can rerun U08. + ## Link the OTP prompt to lost-authenticator help (F3) ```task id: KEY-WP-0040-T02 -status: todo +status: progress priority: medium state_hub_task_id: "3ad155dd-15ef-5522-bdf9-c78832728553" ``` @@ -57,3 +69,20 @@ The OTP input offers no way out for a user whose code is rejected or whose devic "Code rejected or lost your authenticator?" linking to `https://users.coulomb.social/security` (the lost-authenticator section). The same link serves users who have no factor yet (the dead end noted in `mfa-attended-run.md`). + +2026-10-12: source link added to the MFA challenge with noreferrer, preserving +challenge state and requiring no query/token in the destination. Go OIDC tests +pass. Runtime publication and attended journey verification remain open; T02 +is not represented as deployed. + +2026-10-12 blue-lane runtime repair completed: immutable ConfigMap +factor-recovery-01119187d90d7b76 replaced only the recovery-code volume through +UID/resourceVersion CAS. Server dry-run proved no other pod-template change. +privacyIDEA generation 11 is Ready; recovery health returns 200 and anonymous +recovery is denied (403). All Go tests pass in the actual checkout. An early +ready=false observation was retained while the rollout completed; no apply was +replayed. Abort owner: key-cape; rollback restores prior immutable ConfigMap +factor-recovery-79e2964c112f3688 with the same CAS helper. Receipt and exact +proposal: the-custodian/docs/evidence/2026-10-12-netkingdom-security/. +T01 remains progress for the founder's authenticated tegwick preview and U08. +T02 is a local source edit only; its source/image publication remains pending.