fix: don't resolve HOCON includes in config that arrived in a message - #3505
Open
pjfanning wants to merge 2 commits into
Open
fix: don't resolve HOCON includes in config that arrived in a message#3505pjfanning wants to merge 2 commits into
pjfanning wants to merge 2 commits into
Conversation
Motivation: Three sites parse HOCON that came off the wire with the default parse options: InternalClusterAction.InitJoin and InitJoinAck in ClusterMessageSerializer, and the Config payload in MiscMessageSerializer. HOCON include directives are resolved by the parser rather than by resolve(), so include file(...) and include classpath(...) read from the local filesystem and classpath and include url(...) performs an outbound request, all while deserializing a peer's message. InitJoin is accepted from a node that has not joined, in ClusterDaemon's uninitialized state. Modification: Add WireConfig (@internalapi), which parses with a ConfigIncluder that resolves every include to an empty object, and route the three sites through it. The includer implements ConfigIncluderFile, ConfigIncluderURL and ConfigIncluderClasspath as well as ConfigIncluder: the parser falls back to its own handling, which does read the resource, for any of the typed forms the configured includer does not implement. Every serializer writes config with ConfigRenderOptions.concise, which renders JSON and cannot produce an include, so a well-behaved sender is unaffected. Result: Deserializing a message no longer reads local files or issues outbound requests on behalf of the sender.
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.
Motivation
Three sites parse HOCON that arrived in a message, using the default parse options:
ClusterMessageSerializer:302—deserializeInitJoinInitJoin, from a node that has not joinedClusterMessageSerializer:313—deserializeInitJoinAckInitJoinAck, from a claimed seed nodeMiscMessageSerializer:549—deserializeConfigConfigpayload from an associated peerHOCON
includedirectives are resolved by the parser, not byresolve(), so they takeeffect as soon as the message is deserialized — before
ClusterDaemonforms any opinionabout the sender.
InitJoinis accepted in theuninitializedstate(
ClusterDaemon.scala:501), so for that one the sender need not be a cluster member at all.That gives a peer a blind SSRF primitive from inside the node (
include url("http://…")reaches addresses the sender cannot) and a forced read of local files and classpath
resources.
I checked the behaviour against config 1.4.6 rather than assuming it, and the result changed
the shape of the fix. A plain
ConfigIncluderis not enough — the parser falls back toits own handling, which does read the resource, for any typed form the includer does not
implement:
(Bare
include "..."is already inert here:SimpleIncluderresolves it relative to theincluding source, and a string has none.)
Modification
Add
org.apache.pekko.serialization.WireConfig(@InternalApi), which parses withConfigParseOptions.defaults().setIncluder(...)where the includer resolves every form to anempty object. It implements
ConfigIncluder,ConfigIncluderFile,ConfigIncluderURLandConfigIncluderClasspath— all four are needed, per the table above. Route the three sitesthrough it.
Every serializer writes config with
ConfigRenderOptions.concise(MiscMessageSerializer:172,ClusterMessageSerializer:401), which renders JSON and cannot produce aninclude, so awell-behaved sender loses nothing and no rolling-upgrade path is affected.
Includes are dropped silently rather than logged.
deserializeConfigis on the ordinaryper-message path, so a peer-triggerable log line there is its own small flooding surface, and
the drop is already fail-safe. Happy to add a
LogMarker.Securitydebug line if reviewerswould rather have the signal.
Result
Deserializing a message no longer reads local files or classpath resources, or issues
outbound requests, on behalf of the sender.
Tests
sbt "actor-tests/testOnly org.apache.pekko.serialization.WireConfigSpec"— 7 passed: ordinaryHOCON and concise-rendered config still parse;
file,required(file),urlandclasspathincludes all resolve to nothing; and one test asserts the default parser does resolve all
three, so the others are testing the change rather than an inert directive
sbt "cluster/testOnly org.apache.pekko.cluster.protobuf.ClusterMessageSerializerSpec"— 10 passed,including a new test covering both
InitJoinandInitJoinAcksbt "remote/testOnly org.apache.pekko.remote.serialization.MiscMessageSerializerSpec"— 104 passed,including a new test for the
Configpayloadre-running: each fails with
true did not equal falsesbt "actor/mimaReportBinaryIssues" "remote/mimaReportBinaryIssues" "cluster/mimaReportBinaryIssues"— no issuessbt scalafmtAll headerCreateAll— no changesReferences
None.
WireConfig.scalais new code and carries the standard ASF header.