Check a renamed label against every alias, not just its own kind - #337
Conversation
The rename preview refused a new name that was already another term's alias, because alias-aware search would then answer one query with two entities. The scan was constrained to the renamed term's own kind, while alias resolution walks every tag regardless of kind — so renaming an actor onto a malware family's alias passed the check and produced the ambiguity the check exists to prevent. The scan is now kind-blind, and the conflict names the other term's kind so an administrator can see why a name they thought was free is not. Labels are untouched: uniqueness is (kind, slug), so two kinds may share a label by design and renaming into that stays allowed. A test pins that too, since widening the alias rule would otherwise be an easy place to narrow the taxonomy by accident. Refs #307
fb294c5 to
371f500
Compare
![icebergai-review-bot[bot]](https://avatars.githubusercontent.com/u/290878036?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
APPROVE
Completed bounded review across 1 immutable scope(s). No findings. The cross-kind alias scan matches the stated identifier-resolution behavior and preserves cross-kind label reuse.
Scope health
Convergence: healthy. Review mode: initial.
Recommended action: CONTINUE_INCREMENTAL.
- No escalation signals.
Prior findings
| Finding | Status |
|---|---|
| — | No prior finding state |
New findings
No new findings.
Fix-induced regressions
- None evidenced.
Uncertainty
- No material uncertainty recorded.
Validation
- Reviewed the supplied immutable diff; exact-head CI was reported as passed.
- Verified the added tests cover cross-kind alias blocking and permitted cross-kind label reuse.
Residual risks
- None identified.
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Completed bounded review across 1 immutable scope(s). One high-severity supply-chain/reproducibility defect found.
Scope health
Convergence: healthy. Review mode: delta.
Recommended action: CONTINUE_INCREMENTAL.
- No escalation signals.
Prior findings
| Finding | Status |
|---|---|
| — | No prior finding state |
New findings
Root cause: The snapshot timestamp is encoded only in an HTTP URL; no transport authentication or independently pinned Release metadata binds the fetched archive state to that timestamp.
- BLOCKER · high: APT snapshot pin can be bypassed by replaying signed metadata —
docker/apt-snapshot.sh:18
Status: NEW. Attribution: new_in_scope.
The new source configuration uses unauthenticated HTTP and disables APT freshness validation. A network adversary can replay an older, correctly signed Debian Release/Packages set for the requested suite and matching package files; signatures still verify, but the installed package set no longer corresponds to the timestamp inAPT_SNAPSHOT.
Invariant: The Debian package set must be the authenticated package set selected byAPT_SNAPSHOT.
Ownership: Docker build-time APT dependency resolution.. Behaviour: A network-positioned attacker duringdocker buildcan serve an old signed repository state, causingapt-get upgradeand installs to resolve packages other than the declared snapshot..
Evidence:base="http://snapshot.debian.org/archive"is used for all APT sources, whileAcquire::Check-Valid-Until "false";disables the mechanism that rejects stale signed metadata. APT signatures authenticate replayed metadata but do not bind it to the snapshot URL/timestamp.
Independent assessment: The script uses HTTP and disables APT’s Release freshness check. A network attacker can replay an older validly signed Release/Packages set and matching package files; APT signatures validate repository provenance but do not authenticate the requested snapshot timestamp.
Fix-induced regressions
- None evidenced.
Uncertainty
- No material uncertainty recorded.
Validation
- Reviewed only the supplied immutable diff; exact-head CI was reported as passed and was not re-run.
Residual risks
- None identified.
|
The finding is correct and I've fixed it — in #346, not here, because the code it describes isn't part of this change. On the finding. It's right. On the attribution. No change is needed here, so I'd suggest re-reviewing this one against its own diff. Generated by Claude Code |
…346) The APT snapshot pin fetched over http with Check-Valid-Until disabled. Those two facts combine badly. APT's GPG verification proves Debian published a Release/Packages set; it does not say which published state this is, so it cannot detect a replay. Check-Valid-Until is the control that would, and a snapshot has to disable it, because a snapshot's Release file is stale by design. That left nothing binding the response to the timestamp requested. A network-positioned attacker during a build could answer a request for this snapshot with an older, validly signed, validly stale archive state: signatures verify, freshness is off, and the image ends up with a package set the pinned timestamp does not describe — including the vulnerable versions the upgrade exists to remove. The earlier comment claiming a tampered transport still fails the signature check was answering forgery, which is not the attack here. Fetch the archive over https, where TLS binds the answer to the host and path asked for. A base image without ca-certificates now fails the build rather than falling back, which is the right direction to fail. Reported on #337, though the code arrived there from #345. Co-authored-by: Claude <noreply@anthropic.com>
|
Re-requesting review — the outstanding BLOCKER is resolved, and it was never about this change. The finding was against
The script reached this branch through GitHub's "Update branch", which I ran to clear a The finding itself was correct, so I fixed it rather than only reassigning it: #346 moved the archive to Two related fixes came out of the same thread: #346 (the transport) and #347 (the docker job's path filter never watched This PR's own diff is unchanged since its approval at 02:46. Generated by Claude Code |
|
CI is red here on a pre-existing flake this PR neither causes nor can fix. This should merge after #333. The failure:
assert second.headers["retry-after"] == "60" # main, line 132The bucket refills continuously, so #333 is the fix, and it's already green and waiting: assert 1 <= int(second.headers["retry-after"]) <= 60 # bounds, not a stopwatch
assert event.detail["retry_after"] == int(second.headers["retry-after"])I'm deliberately not pushing the same change here — it would conflict with #333 on merge, and duplicating a fix to turn a branch green is how you lose track of which PR owns it. Once #333 lands I'll update this branch onto it and CI should go green without further changes. For the record on ordering: this PR's own diff is unchanged since its 02:46 approval; the only reason it has run CI again is the two update-branch merges used to clear Generated by Claude Code |
There was a problem hiding this comment.
Verdict
APPROVE
Completed bounded review across 1 immutable scope(s). No actionable findings.
Scope health
Convergence: healthy. Review mode: delta.
Recommended action: CONTINUE_INCREMENTAL.
- No escalation signals.
Prior findings
| Finding | Status |
|---|---|
| F-8A307617555F | Resolved |
New findings
No new findings.
Fix-induced regressions
- None evidenced.
Uncertainty
- No material uncertainty recorded.
Validation
- Prior blocker F-8A307617555F resolved:
base="https://snapshot.debian.org/archive"indocker/apt-snapshot.sh, withtests/test_image_pins.pyasserting the URL scheme is HTTPS and rejecting the HTTP URL. - Exact-head CI passed (per supplied review data).
- Reviewed the supplied immutable diff; the APT source now uses HTTPS and is covered by a regression test.
Residual risks
- None identified.
2 participants
Summary
Follow-up to the review on #330. Confirmed against
main: the rename preview's alias-conflict scan was constrained to the renamed term's own kind, while alias resolution is kind-blind.Refs #307
services/tags.py— the scan filtered onTag.kind == tag.kindservices/tags.py:572-589—find_by_identifier()resolves a slug against every tag's aliases, with no kind constraintSo renaming an ACTOR onto a MALWARE family's alias passed the check and then produced exactly the ambiguity the check exists to prevent — and the conflict message ("would resolve to two entities in search") described a rule the code wasn't enforcing.
Changes
… is already an alias of 'Some Loader' (#12) (MALWARE term), so the new name would resolve to two entities in search), so an administrator can see why a name they expected to be free is not.(kind, slug)(uq_tag_kind_slug), so an ACTOR and a MALWARE term may legitimately share a label, and renaming into that stays allowed. Only aliases are kind-blind, because only they make one lookup answer with two entities.Testing
uv run pytestpasses — 898 passed, 17 skipped. The one failure in this container is the pre-existingtest_object_storage.py::test_s3_clients_are_reused_and_closed, which importsboto3, an optional extra not installed here.Static gates pass (
ruff,mypy,bandit,vulture,djlint)Added/updated tests
test_renaming_onto_another_kinds_alias_is_blocked_too— the reported case: ACTOR renamed onto a MALWARE term's alias is blocked, the conflict names the kind, and the rename itself returns 409.test_a_label_may_still_be_reused_under_a_different_kind— the guard against over-correcting: a cross-kind label reuse is still permitted and still renames cleanly. Widening the alias rule is an easy place to narrow the taxonomy by accident, so that stays pinned.Checklist
ARCHITECTURE.mdalready describes the rule as "the name is already another entity's alias, so it would resolve to two entities in search", which is what the code now actually enforcesGenerated by Claude Code