Require opt-in for plugin deployment over private HTTP - #1682
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. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (15)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (3)
🧰 Additional context used📓 Path-based instructions (3)Source excerpt: Do not translate an inspection request into a configuration operation.📄 CodeRabbit inference engine (docs/control-agent-contract.md) Files:
Source excerpt: Do not raise these by patching around validation; correct the topology or connectivity problem instead.📄 CodeRabbit inference engine (docs/control-connector.md) Files:
Source excerpt: `auto-create-vote-sites` is intentionally narrower than `common-settings`: it reads/writes only `Config.yml -> AutoCreateVoteSites`.📄 CodeRabbit inference engine (AGENTS.md) Files:
🪛 LanguageTooldocs/control-agent-contract.md[style] ~203-~203: The double modal “requires proven” is nonstandard (only accepted in certain dialects). Consider “to be proven”. (NEEDS_FIXED) 🔇 Additional comments (15)
📝 WalkthroughWalkthroughPrivate-network HTTP plugin deployment now requires an explicit setting that defaults to false. Endpoint policy and backend and proxy connectors use this setting to decide whether to prepare and execute deployment staging. HTTPS, loopback HTTP, and proven same-node HTTP retain their existing allowances. ChangesPlugin deployment endpoint policy
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant BungeeConfig
participant ControlConnector
participant PluginDeploymentService
BungeeConfig->>ControlConnector: supplies private HTTP deployment setting
ControlConnector->>PluginDeploymentService: evaluates endpoint policy
PluginDeploymentService-->>ControlConnector: returns allow decision and initialization message
ControlConnector->>PluginDeploymentService: passes setting when executing deployment
Merge Risk: ⚪ Minimal · up to Private-network HTTP plugin deployment now requires an explicit opt-in that is off by default, which tightens security. Nothing in the supplied change indicates a merge-blocking problem. Users who deploy over private HTTP must set the option after upgrading. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change reduces default exposure by requiring explicit approval for private-network HTTP deployment. Enabled plaintext deployment still permits interception of credentials and executable content. No introduced or materially worsened attack path was established, but some authority and recovery assumptions remain unresolved. Retained concerns Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 16.28% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 43 functions across 11 files. (4 skipped: 4 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 participant
Summary
Require an explicit
Control.AllowInsecureHttpPluginDeployment: truebefore executable VotingPlugin staging is prepared, advertised or polled over a non-local literal private-network/link-local HTTP endpoint. The setting defaults to false when absent and is documented in both backend and proxy defaults.Normal private-network HTTP Control registration, configuration, inspection and presence remain supported without the opt-in. Proxy voting is unchanged. HTTPS, literal loopback (including IPv6), and existing proven direct same-node hosted Control remain eligible by default.
localhoststill requires the existing hosting proof. Public HTTP and arbitrary HTTP hostnames remain prohibited even with the opt-in.Security boundary
A deployment's expected SHA-256 and matching JAR travel over the same Control connection. A MITM on plaintext private HTTP can substitute both, so a checksum alone does not authenticate the executable source. Opted-in private HTTP connectors emit a prominent initialization warning covering node credentials, deployment metadata, executable artifacts and the recommendation to use HTTPS. Disabled deployment routes emit an actionable initialization diagnostic without continuous polling noise.
The centralized deployment endpoint policy distinguishes HTTPS, local HTTP, opted-in private HTTP, private HTTP without opt-in, and unsupported endpoints. Both connectors use that policy for preparation and capability admission; the actual artifact deployment checks it again with the captured opt-in. Existing overloads remain available and default to the safe policy.
The separate credential-endpoint eligibility contract is unchanged: HTTPS or proven same-node loopback HTTP. This option does not relax credential endpoint authorization, redirects, staging destinations, JAR validation or SHA-256 verification.
Compatibility
Validation
mvn -B -f VotingPlugin/pom.xml clean package: 1,683 tests passed, plus 4 packaged-artifact checks, 57.536 seconds.git diff --check: passed.Coverage includes IPv4 private ranges and boundaries, IPv6 unique-local/link-local/loopback, proven same-node hosting, HTTPS with both flag values, rejection of public/arbitrary HTTP even with opt-in, unsupported protocols, stricter credential eligibility, actual backend/proxy connector construction without deployment capability or staging readiness, config defaults and explicit opt-in readers, and rejection before artifact network access.
No live Control deployment was performed. Private HTTP opted in by the administrator remains vulnerable to network-path interception/modification; HTTPS is the recommended protection.
Summary by CodeRabbit