Add --host CLI flag and opt-in TLS to plugin gRPC server (CWE-319) - #162
Open
OffByQuant wants to merge 1 commit into
Open
OffByQuant wants to merge 1 commit into
OffByQuant wants to merge 1 commit into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Author
|
@googlebot I signed it! |
The Python plugin server bound the gRPC PluginService surface via
add_insecure_port on a hardcoded _HOST = '127.0.0.1', with no TLS
plumbing and no flag for the bind address. Two practical issues
flowed from that:
* Operators who need a non-loopback bind (distributed deployments
where the Java orchestrator on host A drives plugin servers on
hosts B/C/D) had to edit plugin_server.py to change _HOST. They
will not know to add TLS in that diff because the project ships
no scaffolding for it — and the loopback bind is currently the
only thing keeping the unauthenticated PluginService surface
off the network.
* Container deployments that publish the gRPC port (`docker run -p
34567:34567`, NodePort/LoadBalancer Services), or any iptables
rule forwarding inbound traffic to the loopback port, similarly
erase the loopback mitigation without obviously triggering any
security review. The existing code offers no encrypted-transport
knob to fall back on once that happens (CWE-319).
This patch makes both configuration changes explicit:
* --host: replaces the hardcoded constant. Default '127.0.0.1' so
existing deployments are unaffected. Operators who need a
non-loopback bind do so via flag rather than source edit, which
means the help text can flag the security tradeoff and a
startup WARNING fires when the combination of non-loopback +
no-TLS is selected.
* --tls_cert_path / --tls_key_path: opt-in TLS. When both are
set, _bind_server_port reads the PEM files and binds via
add_secure_port with grpc.ssl_server_credentials. When neither
is set, falls back to add_insecure_port (unchanged behavior).
When exactly one is set, the server refuses to start so an
operator who fat-fingered one flag does not silently get
plaintext.
What this patch does NOT do, deliberately:
* No --auth_token / authentication interceptor. Adding one
requires a coordinated change to the Java orchestrator's
PluginServiceClient (Metadata header attaching the token) and
is a project-policy decision (secure-by-default refusal-to-start
is a breaking change for every existing operator). Out of scope
for this PR; happy to follow up with a separate one if
maintainers want to take that route.
* No client-side TLS enablement on the Java side. Operators who
flip the new TLS flags also need to configure the Java
PluginServiceClient to dial with .useTransportSecurity() and a
matching cert trust. That is a parallel change and should land
with maintainer guidance on whether the Java side wants a
flag-driven or config-driven knob.
Default behavior is unchanged for any operator running with the
flags unset. The patch is local to plugin_server.py.
OffByQuant
force-pushed
the
security/grpc-host-tls-flags
branch
from
April 29, 2026 07:17
bd1525d to
6d96b57
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The Python plugin server (
plugin_server.py) bound the gRPCPluginServicesurface viaadd_insecure_porton a hardcoded_HOST = '127.0.0.1', with no TLS plumbing and no CLI flag for the bind address. Two practical issues flow from that combination:plugin_server.pyto change_HOST. They will not know to also add TLS in that diff because the project ships no scaffolding for it — and the loopback bind is currently the only thing keeping the unauthenticatedPluginServicesurface off the network.docker run -p 34567:34567, aNodePort/LoadBalancerService, or any iptables rule forwarding inbound traffic to the loopback port erases the loopback mitigation without obviously triggering any security review. There is no opt-in to TLS to fall back on once that happens (CWE-319 — Cleartext Transmission of Sensitive Information).This PR makes both configuration changes explicit and adds a defensive WARNING for the dangerous combination.
Changes
All in
plugin_server/py/plugin_server.py:--hostflag (default127.0.0.1). Replaces the hardcoded constant. Default is unchanged so existing deployments are unaffected. Operators who need a non-loopback bind do so via flag rather than source edit, which lets the help text flag the security tradeoff.--tls_cert_pathand--tls_key_pathflags (both default empty). Opt-in TLS. When both are set, the new_bind_server_porthelper reads the PEM files and binds viaadd_secure_portwithgrpc.ssl_server_credentials. When neither is set, falls back toadd_insecure_port(unchanged behavior). When exactly one of the two is set, the server refuses to start so an operator who fat-fingered one flag does not silently get plaintext.--hostis non-loopback and TLS is not configured. The message names both flags and the loopback fallback so the operator has actionable next steps.Default behavior is unchanged
For any operator running with the flags unset:
--hostdefaults to127.0.0.1(matches the prior_HOSTconstant).--tls_cert_path/--tls_key_pathdefault to empty →add_insecure_portis called exactly as before.grpc.ssl_server_credentialsis part of the gRPC Python package the project already imports.What this PR deliberately does NOT do
The disclosure that motivated this patch also called for an authentication interceptor (shared-secret token via
authorizationmetadata, constant-time comparison) and a secure-by-default--allow_unauthenticatedopt-out. Both were excluded from this PR for two reasons:PluginServiceClientso it attaches the token as aMetadataheader on every outbound RPC. That is a parallel patch and should land with maintainer guidance on whether the Java side wants a flag-driven or config-driven knob.--auth_tokenis a breaking change for every existing operator on upgrade. That is a project-policy decision, not a unilateral fix to ship in a security PR.Happy to follow up with a separate PR for the auth piece (Python interceptor + Java client metadata + coordinated CLI flag) once maintainers signal which direction they want to take.
Likewise on the Java client side: operators who flip the new TLS flags will also need to configure the Java
PluginServiceClientto dial with.useTransportSecurity()and a matching cert trust. That parallel change is out of scope here; the help text on--tls_cert_pathand--tls_key_pathcould be extended in a follow-up to call this out once the Java-side flag exists.Test plan
add_insecure_porton127.0.0.1:34567, Java orchestrator dials it via plaintext as before.--host 0.0.0.0(without TLS) starts the server, logs the WARNING, and is reachable from non-loopback callers (matching today's behavior for operators who edited_HOST).--tls_cert_path /path/cert.pem --tls_key_path /path/key.pem(with a self-signed cert generated viaopenssl req -x509 ...) starts the server withadd_secure_portand serves TLS. (Java client coordination is out of scope; verified viagrpcurl -insecure -cacert ...against the running server.)--tls_cert_path /path/cert.pemalone (key omitted) refuses to start with the message "must both be set or both empty."plugin_server.pyitself (plugin_service_test.pycovers the servicer, not bootstrapping); the existingplugin_service_testsuite continues to pass since this PR does not touch the servicer.References
grpc.ssl_server_credentials)