Repository navigation
Establish Kubernetes context and runtime destination allowlists - #263
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07ad653942
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| valid_port = parsed.port is None or 1 <= parsed.port <= 65535 | ||
| except ValueError as err: |
There was a problem hiding this comment.
When an allowed API hostname exposes multiple listeners, this accepts every port from 1 through 65535: for example, changing an approved https://api.cluster.internal:6443 server to :12345 still produces an approved handoff, and the output drops the port entirely. This permits an unapproved HTTPS origin to pass the scope check, so the policy should allowlist and emit the canonical origin, including its effective port, rather than authorizing only the hostname.
AGENTS.md reference: AGENTS.md:L103-L104
Useful? React with 👍 / 👎.
| namespace = context.get("namespace", "default") | ||
| if not isinstance(namespace, str) or not namespace: | ||
| raise DestinationScopeError("Kubernetes namespace is invalid") | ||
| try: | ||
| _exact(namespace, "Kubernetes namespace") |
There was a problem hiding this comment.
For a namespace-scoped engagement, changing the kubeconfig context from an authorized namespace to kube-system still succeeds because the namespace is checked only for syntax and there is no corresponding policy allowlist. The resulting handoff presents that namespace as selected for the approved cluster, so bind it to an exact authorized namespace and reject mismatches before routing the handoff.
AGENTS.md reference: AGENTS.md:L103-L104
Useful? React with 👍 / 👎.
| @dataclass(frozen=True) | ||
| class DeveloperDestinationPolicy: | ||
| """Explicit exact-match allowlists for all developer workflow destinations.""" |
There was a problem hiding this comment.
This new policy and kubeconfig parser establishes a security-sensitive DNS-to-socket authorization and Kubernetes handoff boundary, but the commit leaves docs/THREAT_MODEL.md at its earlier baseline without the new data flow, attacker-controlled inputs, controls, or residual risks. Update the threat model in this change so reviews and releases do not rely on a security model that predates this enforcement path.
AGENTS.md reference: AGENTS.md:L187-L191
Useful? React with 👍 / 👎.
| "specialist": routes[kind], | ||
| "kind": kind, | ||
| "target_host": _host(target_host), | ||
| "repository": repository, |
There was a problem hiding this comment.
When a repository identifier is supplied as a common authenticated URL such as https://user:token@example.test/team/repo.git, _exact accepts it and this field copies the credential verbatim into the generated JSON file or stdout. Validate repository and other destination identifiers into credential-free canonical forms, or reject/redact URL userinfo before serializing the handoff, so configuration mistakes do not turn the output into a secret-bearing artifact.
AGENTS.md reference: AGENTS.md:L101-L104
Useful? React with 👍 / 👎.
| def resolve_target(self, target_host: str) -> str: | ||
| return socket.gethostbyname(target_host) | ||
| self.destination_policy.authorize_host(target_host) | ||
| resolved_ip = socket.gethostbyname(target_host) | ||
| self.destination_policy.authorize_ip(resolved_ip) | ||
| return resolved_ip |
There was a problem hiding this comment.
When the policy contains multiple approved hosts and IPs, DNS can resolve one allowed hostname to an IP intended for another host and both independent membership checks still succeed; the collector then connects to that other resource while attributing the assessment to the requested hostname. Represent and enforce host-to-address bindings so an approved IP is valid only for the hostname whose resolution was authorized.
AGENTS.md reference: AGENTS.md:L103-L104
Useful? React with 👍 / 👎.
| @classmethod | ||
| def load(cls, path: str | Path) -> DeveloperDestinationPolicy: | ||
| return cls.from_dict(json.loads(Path(path).read_text(encoding="utf-8"))) |
There was a problem hiding this comment.
A policy such as {"hosts":["approved.internal"],"hosts":["evil.internal"]} is accepted because json.loads silently keeps the final occurrence, allowing a visually reviewed restrictive allowlist to be replaced by a later duplicate key. Load this security policy with duplicate-key detection and fail closed on any repeated object member before constructing the allowlists.
AGENTS.md reference: AGENTS.md:L103-L104
Useful? React with 👍 / 👎.
1 participant
Closes #214.
Acceptance evidence
Validation and review
ruff check .,ruff format --checkfor changed Python files, andgit diff --check origin/main..HEAD: passed.gitleaks git --log-opts origin/main..HEAD --redact: no leaks.make checkand the repository's full pre-push gate: passed on the published revision.The local push gate required Ruff formatting of the touched Python files, which expands the diff. No live Kubernetes or container runtime was contacted in validation.