docs: add a draft threat model for GeaFlow - #25
Conversation
GeaFlow has no security model on the record: SECURITY.md is purely
disclosure process, community/{en,zh}/security.md are empty stubs, and
the project is not listed at security.apache.org/projects. A triager
handed a vulnerability report has nothing to cite, and an integrator has
no way to tell which threats the project has taken on.
Add a first draft produced against the ASF Security team's
threat-model-producer rubric. It covers the engine and the console as two
component families and leaves the operator, MCP and AI surfaces out of
model. Roughly half of it is inferred from public artifacts rather than
confirmed, so every inferred claim is tagged and routed to one of the 14
open questions in section 14. It is marked DRAFT and is not PPMC policy
until those are answered.
Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three claims in section 9 carried no provenance tag, which the rubric's self-check requires. Tag them and correct the draft-confidence count. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Went through this against apache/geaflow at 958b803, which is still master HEAD, so the citations line up with what is there today. Every code reference in the document checks out, including the 20/0/30 provenance count. That is rare, thanks for doing it properly.
A few corrections first, then my take on the §14 questions.
Corrections
§5a: the agent server is gated on the master. AbstractMaster.java:99-104 starts it inside if (config.getBoolean(HTTP_REST_SERVICE_ENABLE)), together with HttpServer and MetricServer. The ungated path is Supervisor.java:79-83, and KubernetesEnvironment.java:32 forces SUPERVISOR_ENABLE=true, so on k8s every worker gets an agent with no off switch. The supervisor row has the same problem: default false is true of the ConfigKey (ExecutionConfigKeys.java:357) but not of a k8s deployment, where the endpoint that stops and restarts the worker is on. RayEnvironment.java:33 forces it off, so k8s is the odd one out. Q4 should be asking why the supervisor path does not honour a flag when the master path does.
§6 and §9: there is a traversal limit. geaflow.dsl.max.traversal defaults to 64 (DSLConfigKeys.java:80-83, enforced at GeaFlowRuntimeGraph.java:160 and :355), and geaflow.job.max.parallel defaults to 1024. What is genuinely missing is a result size or memory bound. Narrowing the claim to that also makes Q6 answerable.
CORS is not modelled anywhere and it weakens P1. GeaflowWebConfig.java:51-54 does addMapping("/**") with allowedOriginPatterns("*") and allowCredentials(true). Any origin can make credentialed requests to /api/**, and §6 already notes geaflow-token is accepted as a cookie. P1 says requests need a token, which is true but not the whole story. This is the one real gap rather than an inaccuracy: there is nothing on CORS in §8, §9, §11 or §11a.
§9 is too generous about the analytics token. It says the key is referenced only by client-side configuration. ANALYTICS_CLIENT_ACCESS_TOKEN (AnalyticsClientConfigKeys.java:68) is not referenced anywhere at all, it is declared and never read. The client-side thing that does work is a separate JDBC connect property (ConnectProperties.java:40,135). "Declared but never read" is both more accurate and a better argument for removing it.
Two smaller ones. The page will not exist on /community/zh, since zh-community is a separate plugin instance rooted at community/zh with its own autogenerated sidebar. And nothing links to it: community/en/security.md is still a five line stub, so the model is reachable only from the sidebar.
I did not run yarn build locally. Nothing in the file should trouble CommonMark under markdown.format: "md" (no angle brackets or braces outside code spans) and the autogenerated sidebar picks it up without config changes, but CI is the authority on that.
§14
Wave 1
Q1, is the cluster network a trust boundary: no. There is no auth code path on any engine listener, not even a disabled one. The analytics handler only sets CORS headers, the gRPC client hardcodes usePlaintext(), brpc and shuffle carry Kryo with registration off. Spark and Flink both document the same posture. The only condition I would attach is that §10 item 2 becomes item 1, and that we stop steering people to NODE_PORT in the k8s quick start.
Q2, is submitting a query executing code: yes. Three ways, any one of them sufficient. CREATE FUNCTION does getContextClassLoader().loadClass() (FunctionUtil.java:59, GeaFlowUserDefinedScalarFunction.java:58, syntax documented at 2.ddl.md:186). Console UDF upload puts a 500MB jar on the engine classpath. And getConfigWithGlobal merges query WITH properties over global config, so the query picks the connector, its endpoint and its credentials. §10 item 1 should be the first line of §10 and repeated in §2.
Q3, are peers trusted: yes, but split the claim. Trusting peers is right, all processes come from one image and one operator and there is no membership concept to hang an adversary on. But §9 lumps state and checkpoint stores in with shuffle and RPC, and those are not the same thing. Shuffle and RPC are job scoped; a shared RocksDB, Paimon, Redis or DFS path outlives the job and can be written by something that was never a peer, which §6 already half says. Left as is, "we trust our peers" quietly becomes "we trust anything ever written to a checkpoint path".
Q4, agent and dashboard posture: diagnostic surfaces, once the premise above is corrected. Two things I would not close along with it: whether Supervisor.startAgent() should honour a flag the way the master path does, and the supervisor RPC endpoint that stops and restarts workers, which is control plane rather than diagnostics and is on by default on k8s.
Q5, is NODE_PORT the production default: no. Fine as OUT-OF-MODEL: non-default-build. But I would go further and change the default to CLUSTER_IP (KubernetesConfigKeys.java:74, one line plus a release note). With the Q1 answer, the current default is unauthenticated query execution published on every node address, and defaults are what people actually deploy. If we keep it, §10 item 6 needs to lose any optional phrasing.
Wave 2
Q6, where is the line on resource consumption. Given Q2, a resource exhaustion report from someone entitled to submit queries is not a vulnerability: they could call System.exit(0) in a UDF instead. It only escalates if reachable from a client we do not trust, which Q1 rules out. So BY-DESIGN: property-disclaimed, P4 narrows to "malformed GQL is rejected rather than mis-planned", and the resource question moves entirely into §9. Better than a number, since it does not need revising every release.
Q7, is the out of scope list right: yes. The operator case is stronger than the draft puts it: helm/.../rbac.yaml grants verbs: ["*"] across several apiGroups at ClusterRole scope, so a separate model is right on the merits rather than just convenient. Two refinements. For geaflow-mcp and geaflow-ai I would add an expiry rather than a standing exclusion, something like "out of model as of version N, revisit each release", with a matching §12 trigger. And the demo image carve out should attach to geaflow.deploy.mode=local rather than to the Dockerfiles, since local mode starts embedded datastores with bundled credentials and is reachable outside those images. tools/ also ships and is not named.
Q8, does tenant isolation extend below the console: no. Nothing in the engine carries a tenant identity. It is console DB rows plus the token resolved at GeaflowWebConfig.java:47, and job config arrives as a flat geaflow.* map with no tenant field. I would put the negative sentence directly in P2 rather than leaving it to §9, since it is the assumption integrators are most likely to get wrong and it belongs where they will read it.
Q9, are side channels out of scope: yes. We implement no crypto of our own. Worth naming the corollary in §3, that token and password comparison are ordinary equality, so timing reports are answered before anyone files one.
Wave 3
Q10, the backing stores. Agreed as proposed. One thing to fold in: JdbcPluginConfigClass.java:47 hardcodes &useSSL=false into the default JDBC URL, so §10 item 10 should say own the access control and provide transport security.
Q11, is the absence of TLS deliberate: not yet built. There is no TLS config surface anywhere, no keystore or truststore keys, no server.ssl.*, plus usePlaintext() hardcoded in RpcQueryRunner.java:139 and useSSL=false in the JDBC default. A deliberate decision normally leaves a disabled switch behind. I would answer it as both: disclaimed for the model as it stands, so a TLS report closes as BY-DESIGN, and filed as a roadmap item rather than won't-fix. The first half alone reads as a design stance we have not actually taken.
Q12, the analytics token: remove it. It is not merely unenforced, it is never read anywhere in the repo. The JDBC connect property that does work is separate and unaffected by deleting the ConfigKey. A key called ...access.token described as "for auth" that nothing consumes is worth less than nothing, and removing it is a smaller change than documenting it. If we would rather not touch config keys before graduation, at least change the description string to say client-side only, not verified by the server.
Q13, is masked a UI hint: yes. ConfigDescItem.java:80 copies it into a descriptor the console serves for rendering, four production fields carry it, nothing touches storage. §9 is already right, I would just add that the values are stored and transmitted in the clear, since "masked" invites the opposite reading.
Q14, where it lives: here. Two conditions: merge #828 first, and fill community/en/security.md so the Security page links here instead of the model being sidebar-only. On the Chinese translation I would not block. §12 designs this document to change, and a stale translated threat model is worse than none, so point community/zh/security.md at the English page for now and revisit once the questions are answered. Ownership by PPMC vote, revised per §12, the trigger list is already good.
One thing §14 does not ask: we are still not listed at security.apache.org/projects. The whole Q1 posture depends on the ASF Security Team being able to close out-of-model reports without pulling in the PPMC, so getting listed is what makes this document pay off. Worth a Q15, or an action item next to ratification.
|
Hi @Loognqiang, I'll ask Claude to fold in @oscerd's comments into the model. |
Andrea Cosentino reviewed the draft against apache/geaflow 958b803, corrected five claims and proposed answers to all fourteen open questions. This folds both in, so the answers' consequences land in the body sections rather than sitting in §14. Every code citation was re-verified against the tree. Seven of the review's claims needed adjusting before use: - AbstractMaster:99-104 gates the agent and HttpServer only; the metric server gates itself inside its own constructor. - KubernetesEnvironment sets SUPERVISOR_ENABLE as a constructor default, which EnvironmentContext.withConfig can still override, so "no off switch" overstates it. - geaflow.job.max.parallel lives in FrameworkConfigKeys, not the DSL keys. - geaflow.dsl.max.traversal caps iteration depth, not work: it is raised internally for incremental traversal and accepts Integer.MAX_VALUE. - GeaflowWebConfig:47 registers the auth interceptor; token resolution is in GeaflowApiRequest.getSessionToken. - The CORS block spans :50-54 and also sets allowedHeaders and allowedMethods. - Both access tokens are dead, not just the ConfigKey: the JDBC accesstoken connect property is parsed and then dropped, since AnalyticsDriverURI consumes only CUSTOM_HEADERS and SESSION_PROPERTIES. Three points were stronger than stated and are now written at full strength: the supervisor RPC kills a caller-supplied PID; the operator's wildcard RBAC covers core secrets and the cluster-scoped branch is the default; and `masked` has no server-side redaction at all. Answers are tagged with a new *(review)* provenance level, distinct from *(maintainer)*. The document stays a draft: the PPMC has not voted, so §14 now records the answers and what is still open, including the one substantive gap the review surfaced (wildcard CORS with credentials over a cookie-acceptable token) and the ASF security project listing. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FJm2ahzEGFcVrmCkdtuZeA
Adds
community/en/threat-model.md, a draft threat model for GeaFlow. It renders at/community/threat-model.Why
GeaFlow currently has no security model on the record anywhere.
SECURITY.mdis purely disclosure process,community/en/security.mdandcommunity/zh/security.mdare empty stubs, and the project is not listed atsecurity.apache.org/projects. The practical cost is that a triager handed a vulnerability report has nothing to cite, and an integrator has no way to tell which threats GeaFlow has taken on and which are left to them.
This is the follow-up promised in apache/geaflow#828, which routes reports to
security@apache.org. That triage only pays off once the project has written down what it considers in scope, so the ASF Security Team can close out-of-model reports without involving the PPMC every time.How it was produced
This PR was produced using the ASF Security team's
threat-model-producerrubric (https://gist.github.com/potiuk/da14a826283038ddfe38cc9fe6310573).You can improve it by: