Add a draft threat model and wire it for discoverability - #1242
Conversation
|
Thanks Jarek. My plan is to concentrate on apache/pekko#3478 and to return to this when we make progress on that. If you prefer, we can merge this and discuss with the Pekko PMC how to complete it. |
|
Sure. Tak your time - and indeed |
### Motivation The Pekko PMC asked the ASF Security team for a draft threat model to refine, as preparation for a Claude security scan. This is the companion to the same change in apache/pekko. docs/src/main/paradox/security.md already has a "Security model" section, and it makes the most important statement in this document: applications should not be exposed to the public internet directly, and DoS resistance is claimed only as "pretty well under most known" attacks. But that is three sentences, and the quantitative envelope that actually bounds untrusted input -- the parsing limits in http-core's reference.conf -- is not connected to it anywhere. Separately, the AGENTS.md -> SECURITY.md chain a scanning agent follows to locate a project's model does not resolve: there is no SECURITY.md in the repo. ### Modification - THREAT_MODEL.md: a v0 draft following the Scovetta rubric. §5a collects the shipped parsing limits, timeouts and CORS defaults as the model's quantitative spine; §8 states the six default-on properties they underwrite. - SECURITY.md: reporting policy pointing at security@apache.org, plus the three points that catch most reporters. - AGENTS.md: a Security section pointing at both. Nothing security.md asserts has been dropped or weakened; §15 back-maps each existing statement to the section that now carries it. ### Result The chain AGENTS.md -> SECURITY.md -> THREAT_MODEL.md resolves on main, and triagers have a citable document with a closed set of dispositions (§13). §14 holds 10 questions, each stated as a proposed answer. The two worth reading first are Q1 (where exactly is the DoS line, given security.md's deliberately graded claim?) and Q2 (http-cors ships allowed-origins = "*" with allow-credentials = yes, which per its own reference.conf echoes the request Origin -- deliberate, or should the default change?). The document is not ready to be treated as canonical until those are answered. ### Tests Not run - docs only ### References None - requested by the Pekko PMC chair on the ASF security scan thread
Motivation: The draft threat model left ten questions open in §14. Two of them were not lookups but rulings only the PMC could make, and until they were settled a triager had no line to apply: Q1, the boundary of the "behaves pretty well under most known DoS attacks" claim, and Q2, the CORS defaults. Separately, the model had no statement of the project's standing position on configuration defaults, so every "this default should be stricter" report had to be argued from scratch. Modification: - §14 Q1 answered: the DoS line is content vs. volume. A single request within every §5a limit that provokes super-linear CPU or memory is VALID; a finding needing a limit raised is OUT-OF-MODEL: non-default-build; one depending on request volume is BY-DESIGN: property-disclaimed. Propagated to §4, §5, §5a, §7, §8, §9, §11a and §13. - New §5b, "Security posture: hardening, not secure-by-default", mirroring apache/pekko#3478 §5b: defaults are compatibility choices, a request to tighten one is a change request not a vulnerability, proposals belong on the development list, and an implementation that does not do what it documents is a defect this project fixes. - §14 Q2 answered from §5b: the CORS pair is a compatibility default, inherited with the code donated by Lomig Mégard (legal/CorsNotice.txt). A request to change it is BY-DESIGN: default-configuration; a cors() directive that admits an origin its configuration should have rejected remains in scope. - New §13 disposition BY-DESIGN: default-configuration, disambiguated from OUT-OF-MODEL: non-default-build (a real defect reachable only off-default, versus no defect at all). - SECURITY.md states both rulings for reporters, and closes on what the project does want: an implementation that does not match its documentation. Result: Resource exhaustion and default-configuration reports now route to exactly one §13 disposition instead of being judgment calls. Four of the ten §14 questions are closed; the six remaining are confirm-or-correct. Provenance moves from 17 documented / 0 maintainer / 15 inferred to 17 / 6 / 13. Tests: Not run - docs only References: Refs apache#1242
b2f0b29 to
a6d7a2d
Compare
Motivation: Six of the ten §14 questions were still open, and two of them rested on statements that the source contradicts. Q4 asserted that Pekko HTTP "neither parses nor trusts forwarding headers", which is not what extractClientIP does. Q9 held §5's negative claims as the document's last uncited assertions, and the equivalent section in apache/pekko#3478 turned out to be wrong on review, so asserting rather than checking them was not good enough here either. Modification: - Q4 answered, and its stated basis corrected: extractClientIP resolves X-Forwarded-For (first address) -> X-Real-Ip -> the remoteAddress attribute (MiscDirectives.scala:142-145), so forwarding headers are parsed, just never implicitly. The spoofability is documented at the directive and answered by extractDirectClientIP (apache#1219), which reads the attribute alone. Disposition is unchanged: BY-DESIGN: property-disclaimed, with a defect in extractDirectClientIP being VALID. Propagated to §5, §6, §9, §10.5, §11, §11a. - Q5, Q6, Q7 and Q8 answered as proposed: the verify/provideVerify split, the §2 module in/out split, the non-goals and adversary split, and TLS cipher selection as a deployment property. Q8 additionally records that in-process termination is supported while the fronted posture is what the docs recommend. - Q9 resolved by scanning the main sources of http-core, http, parsing, http-caching and http-cors: no addShutdownHook, ProcessBuilder/Runtime.exec, Signal/SignalHandler, file-writing API, System.setProperty/Security.*, or bind outside the public Http().bind* entry points. §5 now cites the scan, and records the inherited caveat that the ActorSystem registers shutdown hooks Pekko HTTP does not. Result: No claim in the document is inferred any more; provenance moves from 17 documented / 6 maintainer / 13 inferred to 20 / 24 / 0. Q10 (coexistence with security.md) is the only question left open. Tests: Not run - docs only References: Refs apache#1242
Motivation: Q9 was resolved against the source but left the disposition open: §5's scan showed Pekko HTTP adds no JVM shutdown hook, while the ActorSystem it runs on does. Whether to state that boundary here or defer the topic to the companion model needed a ruling, and the distinction is easy to misattribute in triage. Modification: - Q9 answered: highlight the ActorSystem's hooks in §5 so integrators are not surprised, but the claim this document makes is that Pekko HTTP registers none of its own. - Promoted "Registers no JVM shutdown hook" from a trailing clause on the signal-handler bullet to its own §5 claim, so the load-bearing statement is stated directly rather than implied. - Sharpened the §5 caveat to separate the two readings: "a Pekko HTTP process has no shutdown hook" is false, "Pekko HTTP registers no shutdown hook" is true and is what this section asserts. - New §11a non-finding: a report that the process registers shutdown hooks is OUT-OF-MODEL: unsupported-component, since the hooks are CoordinatedShutdown's and, with remoting enabled, Artery's. Result: Q10 (coexistence with security.md) is the only question left open. Provenance is 20 documented / 26 maintainer / 0 inferred. Tests: Not run - docs only References: Refs apache#1242
Motivation: Q10 was the last open question: which document is canonical for what, now that THREAT_MODEL.md, SECURITY.md and docs/src/main/paradox/security.md all carry security information. apache/pekko#3478 settled the same question for the companion project, and following its split keeps the two models consistent for anyone triaging across both. Modification: - Q10 answered with the three-document table from apache/pekko#3478: SECURITY.md is canonical for the reporting policy, THREAT_MODEL.md for scope and triage, and docs/src/main/paradox/security.md for announcements and the docs-site index. Every other document links rather than restates. - docs/src/main/paradox/security.md gains links to SECURITY.md and THREAT_MODEL.md, and its reporting paragraph now names security@apache.org rather than "our private security mailing list", which does not exist. This mirrors the equivalent change in apache/pekko#3478. - Corrected the draft's own proposal, which suggested reducing the "Security model" section of security.md to a pointer. It is kept: §4 quotes it as the documented source of the "should not be exposed to the public internet directly" posture and §15 back-maps four claims to it, so reducing it would delete the evidence this model is built on. Result: All ten §14 questions are answered. One item is referred rather than settled, as the ruling requires: security.md carries an upstream-coordination sentence about sharing reports with the Lightbend Akka team that SECURITY.md does not, and a reporting statement is promoted verbatim or dropped by maintainer decision, never silently moved. Provenance is 20 documented / 27 maintainer / 0 inferred. Tests: Not run - docs only References: Refs apache#1242
Motivation: Maintainer review found two claims that do not hold on main and one internal inconsistency: - §5 claimed Pekko HTTP writes no files, but fileUploadAll creates temp files and storeUploadedFile(s) writes entity bytes to an application-chosen destination (FileUploadDirectives.scala:178). - §9 and §14 Q3 described safeDirectoryChildPath as containing traversal, but its canonical-path check compares strings, so a symlink resolving into a sibling directory that shares the served root as a string prefix escapes it (fix in flight in apache#1218). - The status line said both "Q1-Q8 answered" and "all ten answered" while Q3 still ended in an open question, and the pinned commit was the PR's own first commit rather than the main commit reviewed. Modification: Restate the §5 file-system claim with the upload-directive carve-out and correct Q9 accordingly. Answer Q3 as a correction: the escape is VALID under §5b.4 and fixed by apache#1218; update §9 and the §15 back-map to match. Note in §12 that apache#1217 would invalidate the shutdown-hook claim on merge. Fix the status line, the tag tally, and the commit pin (444d939 -> 85d7243, the main commit the branch is based on). Result: Every §5 negative claim matches the source at the pinned commit, Q3 is answered consistently with the "all ten answered" status, and the two in-flight PRs that touch the model's claims (apache#1217, apache#1218) are cross-linked. Tests: Not run - docs only References: Refs apache#1218, Refs apache#1217
|
I verified the model's claims against the source and pushed 17cbbf6 with corrections — three things did not survive the check:
Also added to §12: #1217 proposes replacing the per-upload For the record, the rest verified clean: the full §5a table against |
…laims Motivation: Review of the branch found problems in both halves. The raw Runtime.addShutdownHook ran concurrently with CoordinatedShutdown's hook, so during a graceful drain it could delete temp files that in-flight uploads still use - deleteOnExit provably deleted only after application hooks finished. The memoized upload directory was never recreated if a temp-file reaper removed it while empty, failing all later uploads until restart. In the jar path, the cache-off close was not in a finally (leaking a JarFile per failing request), an explicit setUseCaches(true) silently defeated an application-wide URLConnection.setDefaultUseCaches(false), the public one-arg ResourceFile.apply changed semantics by pinning jars in the JDK cache, getContentLength truncated and turned unknown lengths into silent empty 200s, an exception from the close in fromUrlConnection's finally became a 500 where callers expect a rejection, and the cache decision was spread over three hand-synchronized places. The use-jar-file-cache documentation also wrongly claimed the JDK's jar cache is the class loader's cache and implied disabling it makes classpath jars replaceable. Modification: Make UploadTempFiles a per-actor-system extension whose directory is removed by a CoordinatedShutdown task in the actor-system-terminate phase, after the drain; recreate the directory in create() if it is gone. In ResourceFile, restore the one-arg apply to its historical no-handle-kept semantics (useJarFileCache = false), decide jar ownership from the connection's effective getUseCaches, close the owned jar in a guarded finally, only call setUseCaches when disabling, centralize that rule in one openConnection helper shared with openStream, dispatch on the connection type instead of the protocol string, use getContentLengthLong and reject unknown lengths, and map FileNotFoundException to None with a guarded stream close. Reword the reference.conf entry to scope the replaceability promise to jars no class loader holds open and document the two-parse cost of cache-off mode. Rebased onto main. Result: Upload temp files are deleted only after the system has drained and uploads keep working if the temp directory disappears; the jar path neither leaks handles nor overrides application-wide cache opt-outs; external ResourceFile(url) callers keep the pre-existing behavior; and the configuration text makes no false claims about the JDK. Tests: - sbt "http-tests/testOnly org.apache.pekko.http.scaladsl.server.directives.FileUploadDirectivesSpec org.apache.pekko.http.scaladsl.server.directives.FileAndResourceDirectivesSpec" - pass (73 tests); new tests cover directory recreation after removal and directory deletion when a separate actor system terminates - sbt http/mimaReportBinaryIssues - pass - sbt "+http/compile" - pass on 2.13.18 and 3.3.8 - native scalafmt run on the changed Scala files - clean References: Refs apache#1242 - keeps the threat model's "registers no JVM shutdown hook" claim true
… task Motivation: §12 flagged apache#1217's raw JVM shutdown hook as invalidating the §5 "registers no shutdown hook" claim on merge. That PR has since been reworked to register its temp-file cleanup as a CoordinatedShutdown task on the actor system instead, so no §5 claim is affected. Modification: Restate the §12 bullet as a recorded near-miss rather than a pending invalidation. Result: §12 matches the current state of apache#1217 and the §5/§11a claims stand. Tests: Not run - docs only References: Refs apache#1217
Motivation: Both in-flight PRs the model references changed shape after review. apache#1218 gained a documented platform caveat - File.getCanonicalPath does not resolve NTFS symbolic links or junctions on Windows, so the link-escape class stays open there - and now rejects path segments that no file-system path may contain instead of erroring. apache#1217 replaced the per-file deleteOnExit with one temp directory per actor system removed by a CoordinatedShutdown task. Q3's answer claimed symlink escapes are rejected "whatever its target is named", which overclaims on Windows. Modification: Scope the Q3 and §9 symlink-rejection claims to platforms where canonicalization resolves links, record the Windows residual and the toRealPath follow-up, note the invalid-segment hardening, and update the §5 upload-directive note to describe apache#1217's per-system directory and CoordinatedShutdown cleanup. Result: The model's containment and file-writing claims match what apache#1217 and apache#1218 actually implement, on every platform they address. Tests: Not run - docs only References: Refs apache#1217, Refs apache#1218
|
Pushed 12c07b5 folding in the post-review state of the two in-flight PRs the model references:
|
…#1217) * serve jar resources without reopening the jar per request Motivation: `ResourceFile` opened a `java.util.zip.ZipFile` for every request to a resource that lives in a jar, only to read the entry's size and time. That parses the whole central directory of the jar again per request, and `getFromResource`/`getFromResourceDirectory` served from a jar is the usual production layout for static resources. The result of `getEntry` was also dereferenced without a null check. Modification: Read the metadata from the `JarURLConnection` instead and leave its cache enabled, so the JDK reuses the same open jar file that the class loader already holds. Guard against a null entry, and share the plain `URLConnection` handling with the fallback branch. Result: No jar is opened or parsed per request for resources served from a jar, and a missing entry rejects the request instead of throwing. Tests: - sbt "http-tests/testOnly org.apache.pekko.http.scaladsl.server.directives.FileAndResourceDirectivesSpec" - pass, 1 new test asserting the entry metadata matches the bytes served - sbt http-tests/test - pass - sbt +http/compile - pass - sbt http/mimaReportBinaryIssues - pass - sbt http/scalafmt http-tests/Test/scalafmt - clean References: None - avoids reopening jars for every resource request * don't register every uploaded temp file with deleteOnExit Motivation: `fileUploadAll` called `File.deleteOnExit()` for each temporary upload file. The JVM keeps every path passed to `deleteOnExit` in a global set for the lifetime of the process, and the entry is not removed when the file itself is deleted after the stream is consumed. A long-running server accepting uploads therefore grows its heap by one entry per upload, forever. Modification: Put the temporary upload files in a directory of their own and register a single shutdown hook that removes that directory recursively on exit. Result: The on-exit cleanup that the directive documents is unchanged, but it now costs one shutdown hook per JVM instead of one permanent global entry per uploaded file. The dedicated directory is created with the owner-only permissions that `Files.createTempDirectory` applies. Tests: - sbt "http-tests/testOnly org.apache.pekko.http.scaladsl.server.directives.FileUploadDirectivesSpec" - pass, 1 new test asserting the temp files share one directory - sbt http-tests/test - pass - sbt +http/compile - pass - sbt http/mimaReportBinaryIssues - pass - sbt http/scalafmt http-tests/Test/scalafmt - clean References: None - removes an unbounded deleteOnExit registration per upload * make the jar file cache for resource serving configurable Motivation: Reading jar resource metadata through the JDK's jar file cache means the jar file stays open for the lifetime of the process, which prevents the jar from being replaced while the server runs (on Windows an open file cannot be replaced). That should be a choice rather than something the directives decide. Modification: Add a `pekko.http.routing.use-jar-file-cache` setting, on by default, and pass it from `getFromResource` into `ResourceFile`. With the setting off, the connection that reads the entry metadata owns its jar file and closes it again, and the entity stream is opened through a connection with caches disabled as well, so that nothing keeps the jar open between requests. `ResourceFile.apply(url)` keeps its previous meaning and uses the cache. Result: The default is the cached behaviour, and deployments that need to replace jar files at runtime can turn the cache off. Note that the previous implementation could not offer that at all: it opened its own `ZipFile` for the metadata but still streamed the content through `URL.openStream`, which uses the JDK caches. Tests: - sbt "http-tests/testOnly org.apache.pekko.http.scaladsl.server.directives.FileAndResourceDirectivesSpec" - pass, 1 new test serving a jar resource with the cache disabled - sbt http-tests/test - pass (TimeoutDirectivesSpec flaked in the full run, passes on its own) - sbt +http/mimaReportBinaryIssues - pass - sbt http/scalafmt http-tests/Test/scalafmt - clean References: None - follow-up to the jar resource change on this branch * address review: shutdown ordering, jar connection ownership, config claims Motivation: Review of the branch found problems in both halves. The raw Runtime.addShutdownHook ran concurrently with CoordinatedShutdown's hook, so during a graceful drain it could delete temp files that in-flight uploads still use - deleteOnExit provably deleted only after application hooks finished. The memoized upload directory was never recreated if a temp-file reaper removed it while empty, failing all later uploads until restart. In the jar path, the cache-off close was not in a finally (leaking a JarFile per failing request), an explicit setUseCaches(true) silently defeated an application-wide URLConnection.setDefaultUseCaches(false), the public one-arg ResourceFile.apply changed semantics by pinning jars in the JDK cache, getContentLength truncated and turned unknown lengths into silent empty 200s, an exception from the close in fromUrlConnection's finally became a 500 where callers expect a rejection, and the cache decision was spread over three hand-synchronized places. The use-jar-file-cache documentation also wrongly claimed the JDK's jar cache is the class loader's cache and implied disabling it makes classpath jars replaceable. Modification: Make UploadTempFiles a per-actor-system extension whose directory is removed by a CoordinatedShutdown task in the actor-system-terminate phase, after the drain; recreate the directory in create() if it is gone. In ResourceFile, restore the one-arg apply to its historical no-handle-kept semantics (useJarFileCache = false), decide jar ownership from the connection's effective getUseCaches, close the owned jar in a guarded finally, only call setUseCaches when disabling, centralize that rule in one openConnection helper shared with openStream, dispatch on the connection type instead of the protocol string, use getContentLengthLong and reject unknown lengths, and map FileNotFoundException to None with a guarded stream close. Reword the reference.conf entry to scope the replaceability promise to jars no class loader holds open and document the two-parse cost of cache-off mode. Rebased onto main. Result: Upload temp files are deleted only after the system has drained and uploads keep working if the temp directory disappears; the jar path neither leaks handles nor overrides application-wide cache opt-outs; external ResourceFile(url) callers keep the pre-existing behavior; and the configuration text makes no false claims about the JDK. Tests: - sbt "http-tests/testOnly org.apache.pekko.http.scaladsl.server.directives.FileUploadDirectivesSpec org.apache.pekko.http.scaladsl.server.directives.FileAndResourceDirectivesSpec" - pass (73 tests); new tests cover directory recreation after removal and directory deletion when a separate actor system terminates - sbt http/mimaReportBinaryIssues - pass - sbt "+http/compile" - pass on 2.13.18 and 3.3.8 - native scalafmt run on the changed Scala files - clean References: Refs #1242 - keeps the threat model's "registers no JVM shutdown hook" claim true
Motivation
The Pekko PMC asked the ASF Security team for a draft threat model to refine, as
preparation for a Claude security scan. This is the companion to
the same change in
apache/pekko— that modelcovers the actor, stream, remoting and cluster layers, and this one does not restate them.
docs/src/main/paradox/security.mdalready carries a "Security model" section, and itmakes the most consequential statement in this document: applications "should not be
exposed to the public internet directly", and DoS resistance is claimed only as
"pretty well under most known Denial of Service attacks". But that is three sentences,
and the quantitative envelope that actually bounds untrusted input — the parsing limits in
http-core'sreference.conf— is not connected to it anywhere.Separately, the
AGENTS.md→SECURITY.mdchain that a scanning agent follows to locatea project's model does not resolve today: there is no
SECURITY.mdin the repo.Modification
THREAT_MODEL.md— a v0 draft. §5a collects the shipped parsing limits,timeouts and CORS defaults as the model's quantitative spine; §8 states the six
default-on properties they underwrite. Every claim is tagged
(documented)with acitation or
(inferred)with a matching question in §14.SECURITY.md— reporting policy (security@apache.org; Pekko does not operate aseparate project security list), plus the three points that catch most reporters.
AGENTS.md— a two-line## Securitysection pointing at both.Nothing
security.mdasserts has been dropped or weakened — §15 back-maps eachexisting statement to the section that now carries it.
Result
The chain
AGENTS.md→SECURITY.md→THREAT_MODEL.mdresolves onmain, and triagersget a citable document with a closed set of dispositions (§13).
Worth noting in contrast to the
apache/pekkomodel: every §8 property here isdefault-on. Pekko HTTP's parsing limits, timeouts, strict URI/cookie parsing and
response-splitting protection all ship enabled. The soft spot is not the defaults, it is
the boundary of the DoS claim.
§14 holds 10 questions, each stated as a proposed answer — confirming or correcting is
enough. The two worth reading first:
security.md's claim is deliberately graded, andtriage needs a split. Proposed: a request within all §5a limits that causes
super-linear CPU or memory is
VALID; anything requiring limits to be raised, orrequiring request volume rather than request content, is out of model because volume
defence is the fronting proxy's job. Is that the intended reading?
http-corsdefaults. The module shipsallowed-origins = "*"together withallow-credentials = yes. Per its ownreference.conf, that combination does notsend a literal
*— it echoes the request'sOriginand allows credentials. Two thingsbound how alarming that is:
cors()is opt-in, and the interaction is documented. Is thedefault deliberate, or should it change?
This is a draft and should not be treated as canonical until at least Q1 and Q2 are
answered. Happy to fold answers in, or hand it over entirely — the PMC owns the document.
Tests
Not run - docs only
References
None - requested by the Pekko PMC chair on the ASF security scan thread