fix(issue-12): scrub-secrets selects only records with a stored password
Review of4176d75(planner-reviewer-claude-01, N1): _cmd_scrub_secrets chose records by the presence of broker.password, not by its value. The old to_registry_block wrote "password": null even for an unauthenticated broker, so on this repo's registry (901 of 903 records carry the null key, none a real password) --dry-run reported "would scrub 901 record(s)" and a real run would rewrite every one of them while holding the registry lock. - registry.py: select on broker.get("password"); an empty key is no secret, and _atomic_write_record drops it on the record's next write anyway - tests: the migration test gains a legacy "password": null record that must be neither listed nor rewritten (fails on4176d75with "would scrub 2") - VERSIONS.md v4.4.4 item 2 and registry.md now describe the stored-value rule Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HCGpJXhSZasJLU38ksDghe
This commit is contained in:
1 parent
4176d7570f
commit
0fedae2f46
4 files changed
+23
-5
No files matched your search
@@ -345,13 +345,19 @@ def _build_parser() -> argparse.ArgumentParser:
|
|||||||
|
|
||||||
def _cmd_scrub_secrets(args) -> int:
|
def _cmd_scrub_secrets(args) -> int:
|
||||||
"""Issue #12: strip broker.password from records written before
|
"""Issue #12: strip broker.password from records written before
|
||||||
to_registry_block stopped persisting it. Every other field is kept."""
|
to_registry_block stopped persisting it. Every other field is kept.
|
||||||
|
|
||||||
|
Only a stored value counts. The old rule wrote `"password": null` even
|
||||||
|
for an unauthenticated broker; that empty key is no secret, and
|
||||||
|
_atomic_write_record drops it on the record's next write anyway, so
|
||||||
|
reporting or rewriting it here would list phantom secrets and hold the
|
||||||
|
registry lock over every record in a PoC registry."""
|
||||||
rd = args.registry_dir
|
rd = args.registry_dir
|
||||||
found = []
|
found = []
|
||||||
with registry_lock(rd):
|
with registry_lock(rd):
|
||||||
for record in list(_iter_records(rd)):
|
for record in list(_iter_records(rd)):
|
||||||
broker = record.get("broker")
|
broker = record.get("broker")
|
||||||
if not (isinstance(broker, dict) and "password" in broker):
|
if not (isinstance(broker, dict) and broker.get("password")):
|
||||||
continue
|
continue
|
||||||
found.append(record.get("job_id", "?"))
|
found.append(record.get("job_id", "?"))
|
||||||
if not args.dry_run:
|
if not args.dry_run:
|
||||||
|
|||||||
@@ -57,8 +57,9 @@ Reference implementation: [`./scripts/registry.py`](./scripts/registry.py)
|
|||||||
overrides toggles like `MQTT_TLS`). It never carries the password (Issue #12):
|
overrides toggles like `MQTT_TLS`). It never carries the password (Issue #12):
|
||||||
every sub-agent can read the record, so `MQTT_PASSWORD` is layered back from
|
every sub-agent can read the record, so `MQTT_PASSWORD` is layered back from
|
||||||
the environment (`.mam.env`) at connect time. Records written before that rule
|
the environment (`.mam.env`) at connect time. Records written before that rule
|
||||||
are cleaned on their next write; `registry.py scrub-secrets [--dry-run]` cleans
|
are cleaned on their next write; `registry.py scrub-secrets [--dry-run]` strips
|
||||||
a whole registry at once. `auth_token` and `dispatch_token` stay in the record
|
every stored password from a whole registry at once (an empty `null` key left
|
||||||
|
by the old rule is not a secret; it goes on the record's next write). `auth_token` and `dispatch_token` stay in the record
|
||||||
because the agent's own `publish_event` needs them to sign and to pin its
|
because the agent's own `publish_event` needs them to sign and to pin its
|
||||||
attempt.
|
attempt.
|
||||||
- `topic_prefix` → the events topic is `<topic_prefix>/events`.
|
- `topic_prefix` → the events topic is `<topic_prefix>/events`.
|
||||||
|
|||||||
+1
-1
@@ -43,7 +43,7 @@
|
|||||||
|
|
||||||
#### 🌟 주요 개선 사항
|
#### 🌟 주요 개선 사항
|
||||||
1. **잡 레코드의 브로커 비밀번호 제거 (Issue #12, `mqtt_common.BrokerConfig.to_registry_block`, `registry.py`)**: `to_registry_block` 이 더 이상 `password` 를 레코드의 `broker` 블록에 넣지 않는다. `broker_config_from_job` 은 레코드 블록을 환경 설정 위에 겹치므로 접속 시 비밀번호는 `MQTT_PASSWORD`(`.mam.env`)에서 채워지고, `publish_event` 는 여전히 레코드만으로 접속한다. `_atomic_write_record` 는 쓰는 모든 레코드에서 `broker.password` 만 지우므로(다른 필드는 그대로) 이전 규칙으로 쓰인 레코드는 다음 쓰기 때 정리된다. `auth_token`·`dispatch_token` 은 에이전트 자신의 `publish_event` 가 서명과 attempt 고정에 쓰므로 레코드에 남긴다.
|
1. **잡 레코드의 브로커 비밀번호 제거 (Issue #12, `mqtt_common.BrokerConfig.to_registry_block`, `registry.py`)**: `to_registry_block` 이 더 이상 `password` 를 레코드의 `broker` 블록에 넣지 않는다. `broker_config_from_job` 은 레코드 블록을 환경 설정 위에 겹치므로 접속 시 비밀번호는 `MQTT_PASSWORD`(`.mam.env`)에서 채워지고, `publish_event` 는 여전히 레코드만으로 접속한다. `_atomic_write_record` 는 쓰는 모든 레코드에서 `broker.password` 만 지우므로(다른 필드는 그대로) 이전 규칙으로 쓰인 레코드는 다음 쓰기 때 정리된다. `auth_token`·`dispatch_token` 은 에이전트 자신의 `publish_event` 가 서명과 attempt 고정에 쓰므로 레코드에 남긴다.
|
||||||
2. **`registry.py scrub-secrets [--dry-run]` 마이그레이션 (Issue #12)**: 레지스트리 lock 아래에서 `broker.password` 를 가진 레코드만 골라 비밀번호를 지우고 재기록한다. `--dry-run` 은 대상 잡 목록만 출력하고 아무것도 바꾸지 않는다. `registry.md` 가 브로커 블록 규칙(비밀번호 비저장, 환경에서 복원, 토큰 유지 이유)을 문서화한다.
|
2. **`registry.py scrub-secrets [--dry-run]` 마이그레이션 (Issue #12)**: 레지스트리 lock 아래에서 **비밀번호 값이 실제로 저장된** 레코드만 골라 지우고 재기록한다. 이전 규칙이 무인증 모드에서도 남긴 `"password": null` 키는 비밀이 아니므로 대상도 보고도 아니며, 그 레코드의 다음 쓰기 때 `_atomic_write_record` 가 정리한다. `--dry-run` 은 대상 잡 목록만 출력하고 아무것도 바꾸지 않는다. `registry.md` 가 브로커 블록 규칙(비밀번호 비저장, 환경에서 복원, 토큰 유지 이유)을 문서화한다.
|
||||||
3. **업데이트 후 설치 스탬프 보존 (Issue #8, `deploy/update.sh`)**: 성공 경로가 업데이트 전 `asset_hashes.txt`·`version.txt` 를 되돌리지 않는다. 그 복원 탓에 `version.txt` 가 이전 커밋을 가리키고 `remove.sh` 가 갱신된 원본 파일을 "로컬 수정"으로 오판해 백업하던 문제가 사라진다. `install_state` 는 재설치가 쓰지 않은 키(예: `gitignore_created`)만 `key=value` 줄 단위로 병합하며(이미 있는 키는 건너뜀, 마지막 줄에 개행이 없어도 처리), 실패 경로(`restore_on_failure`)는 종전대로 세 파일을 모두 그대로 복원한다.
|
3. **업데이트 후 설치 스탬프 보존 (Issue #8, `deploy/update.sh`)**: 성공 경로가 업데이트 전 `asset_hashes.txt`·`version.txt` 를 되돌리지 않는다. 그 복원 탓에 `version.txt` 가 이전 커밋을 가리키고 `remove.sh` 가 갱신된 원본 파일을 "로컬 수정"으로 오판해 백업하던 문제가 사라진다. `install_state` 는 재설치가 쓰지 않은 키(예: `gitignore_created`)만 `key=value` 줄 단위로 병합하며(이미 있는 키는 건너뜀, 마지막 줄에 개행이 없어도 처리), 실패 경로(`restore_on_failure`)는 종전대로 세 파일을 모두 그대로 복원한다.
|
||||||
4. **같은 cwd 형제 워크스페이스 패킹 (Issue #7, `lib.sh` 쉼 `new-session`)**: 첫 번째로 일치한 pane 의 워크스페이스만 보던 탓에 가득 찬 첫 워크스페이스를 매번 overflow 로 판정해 에이전트마다 새 워크스페이스를 열던 문제(4 + 1 + 1)를 고쳤다. 이제 같은 cwd 의 모든 워크스페이스를 pane 순서대로 순회해 idle pane 이 있거나 2xK 그리드에 여유가 있는 첫 워크스페이스에 배치하고(overflow 인 후보는 다음 후보로 넘어감), 모두 가득 찼을 때만 새 워크스페이스를 만든다(4 + 2).
|
4. **같은 cwd 형제 워크스페이스 패킹 (Issue #7, `lib.sh` 쉼 `new-session`)**: 첫 번째로 일치한 pane 의 워크스페이스만 보던 탓에 가득 찬 첫 워크스페이스를 매번 overflow 로 판정해 에이전트마다 새 워크스페이스를 열던 문제(4 + 1 + 1)를 고쳤다. 이제 같은 cwd 의 모든 워크스페이스를 pane 순서대로 순회해 idle pane 이 있거나 2xK 그리드에 여유가 있는 첫 워크스페이스에 배치하고(overflow 인 후보는 다음 후보로 넘어감), 모두 가득 찼을 때만 새 워크스페이스를 만든다(4 + 2).
|
||||||
|
|
||||||
|
|||||||
@@ -79,12 +79,22 @@ def test_scrub_secrets_cli_migrates_a_registry(mam_sandbox, monkeypatch):
|
|||||||
rec["broker"]["password"] = "leaked-earlier"
|
rec["broker"]["password"] = "leaked-earlier"
|
||||||
dirty_path.write_text(json.dumps(rec))
|
dirty_path.write_text(json.dumps(rec))
|
||||||
clean_before = (Path(rd) / f"{clean}.json").read_text()
|
clean_before = (Path(rd) / f"{clean}.json").read_text()
|
||||||
|
# What the old to_registry_block wrote for every unauthenticated job: the key
|
||||||
|
# with a null value. No secret, so the migration must neither report nor
|
||||||
|
# rewrite it (a real PoC registry held ~900 of these and no password at all).
|
||||||
|
empty = registry.register_job("empty-key", "claude-code", "herdr:claude", registry_dir=rd)
|
||||||
|
empty_path = Path(rd) / f"{empty}.json"
|
||||||
|
rec_empty = json.loads(empty_path.read_text())
|
||||||
|
rec_empty["broker"]["password"] = None
|
||||||
|
empty_path.write_text(json.dumps(rec_empty))
|
||||||
|
empty_before = empty_path.read_text()
|
||||||
|
|
||||||
env = dict(os.environ, PYTHONPATH=f"{SKILLS}:{os.environ.get('PYTHONPATH', '')}")
|
env = dict(os.environ, PYTHONPATH=f"{SKILLS}:{os.environ.get('PYTHONPATH', '')}")
|
||||||
dry = subprocess.run([sys.executable, str(REGISTRY_PY), "--registry-dir", rd, "scrub-secrets", "--dry-run"],
|
dry = subprocess.run([sys.executable, str(REGISTRY_PY), "--registry-dir", rd, "scrub-secrets", "--dry-run"],
|
||||||
capture_output=True, text=True, env=env, cwd=str(mam_sandbox))
|
capture_output=True, text=True, env=env, cwd=str(mam_sandbox))
|
||||||
assert dry.returncode == 0, dry.stderr
|
assert dry.returncode == 0, dry.stderr
|
||||||
assert f"would scrub 1 record(s)" in dry.stdout and dirty in dry.stdout
|
assert f"would scrub 1 record(s)" in dry.stdout and dirty in dry.stdout
|
||||||
|
assert empty not in dry.stdout, "an empty password key is not a stored secret"
|
||||||
assert "leaked-earlier" in dirty_path.read_text(), "--dry-run must not write"
|
assert "leaked-earlier" in dirty_path.read_text(), "--dry-run must not write"
|
||||||
|
|
||||||
res = subprocess.run([sys.executable, str(REGISTRY_PY), "--registry-dir", rd, "scrub-secrets"],
|
res = subprocess.run([sys.executable, str(REGISTRY_PY), "--registry-dir", rd, "scrub-secrets"],
|
||||||
@@ -95,3 +105,4 @@ def test_scrub_secrets_cli_migrates_a_registry(mam_sandbox, monkeypatch):
|
|||||||
assert "password" not in scrubbed["broker"]
|
assert "password" not in scrubbed["broker"]
|
||||||
assert scrubbed["prompt"] == "dirty" and scrubbed["auth_token"] == rec["auth_token"]
|
assert scrubbed["prompt"] == "dirty" and scrubbed["auth_token"] == rec["auth_token"]
|
||||||
assert (Path(rd) / f"{clean}.json").read_text() == clean_before, "untouched records are not rewritten"
|
assert (Path(rd) / f"{clean}.json").read_text() == clean_before, "untouched records are not rewritten"
|
||||||
|
assert empty_path.read_text() == empty_before, "a null password key is left for the record's next write"
|
||||||
Reference in new issue
Block a user