YARN: restrict leveldb assigned-resources deserialization to known types - #8689
YARN: restrict leveldb assigned-resources deserialization to known types#8689nishat-06 wants to merge 2 commits into
Conversation
|
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Hardens NodeManager container recovery by restricting deserialization of assigned-resources records to a known set of safe types, preventing tampered LevelDB recovery entries from instantiating arbitrary classes.
Changes:
- Replace unrestricted
SerializationUtils.deserializewith aValidatingObjectInputStreamallowlist inResourceMappings.AssignedResources.fromBytes. - Add tests to ensure round-trip deserialization works for GPU/FPGA/NUMA plugin value objects and that unexpected types are rejected.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
.../container/TestResourceMappings.java |
Adds regression tests for allowlisted plugin types and rejection of unexpected serialized element types. |
.../container/ResourceMappings.java |
Switches recovery deserialization to ValidatingObjectInputStream with an allowlist of accepted classes/packages. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ois.accept( | ||
| "org.apache.hadoop.yarn.server.nodemanager.*", | ||
| "org.apache.hadoop.thirdparty.com.google.common.collect.*", | ||
| "java.util.*", | ||
| "java.lang.*", | ||
| "[Ljava.lang.Object;"); |
There was a problem hiding this comment.
Good point. Tightened it: the four package wildcards are gone, replaced with the concrete value objects the plugins actually store (Device, GpuDevice, FpgaDevice, NumaResourceAllocation), the ArrayList/UnmodifiableList that wrap them, and the String/Number/Integer/Long those objects hold. The only remaining wildcard is the shaded-guava collect package, because NumaResourceAllocation's ImmutableMaps serialize through guava's internal SerializedForm and pinning those class names would break across guava versions.
There was a problem hiding this comment.
The remaining package wildcard still weakens the stated known types boundary.
For example, a shaded-Guava ImmutableList and its serialization proxy match this pattern and can be accepted even though they are not assigned-resource value types. Likewise, any serializable class in this package can be instantiated as an element of an allowed ArrayList before the top-level instanceof List check.
Could we enumerate the exact ImmutableMap serialization proxy types required by the current and supported upgrade paths, and cover those paths with compatibility fixtures? If those internal names cannot be made sufficiently stable, a versioned non-Java-serialization format may be safer than retaining a package-wide wildcard.
| "java.util.*", | ||
| "java.lang.*", | ||
| "[Ljava.lang.Object;"); | ||
| resources = (List<Serializable>) ois.readObject(); |
There was a problem hiding this comment.
Fixed. It now reads into an Object, checks instanceof List and throws an IOException with the offending type name if it isn't one, so the cast can't produce a stray ClassCastException. Callers see the checked IOException the contract advertises.
| // allowlist rejects it by class name during readObject, before the class | ||
| // is loaded or any of its logic runs. |
There was a problem hiding this comment.
Reworded to 'before the object is instantiated and any of its logic runs', which is the property the allowlist actually guarantees.
… closed on non-list records Signed-off-by: nishat shabbir <nishat@bugqore.com>
|
💔 -1 overall
This message was automatically generated. |
slfan1989
left a comment
slfan1989
left a comment
There was a problem hiding this comment.
Thanks for the contribution! Could you please create or link a YARN JIRA for this change and update the PR title to start with the issue ID?
| ois.accept( | ||
| "org.apache.hadoop.yarn.server.nodemanager.*", | ||
| "org.apache.hadoop.thirdparty.com.google.common.collect.*", | ||
| "java.util.*", | ||
| "java.lang.*", | ||
| "[Ljava.lang.Object;"); |
There was a problem hiding this comment.
The remaining package wildcard still weakens the stated known types boundary.
For example, a shaded-Guava ImmutableList and its serialization proxy match this pattern and can be accepted even though they are not assigned-resource value types. Likewise, any serializable class in this package can be instantiated as an element of an allowed ArrayList before the top-level instanceof List check.
Could we enumerate the exact ImmutableMap serialization proxy types required by the current and supported upgrade paths, and cover those paths with compatibility fixtures? If those internal names cannot be made sufficiently stable, a versioned non-Java-serialization format may be safer than retaining a package-wide wildcard.
Description of PR
ResourceMappings.AssignedResources.fromBytesreads the per-container assigned-resource record withSerializationUtils.deserialize, which is a bareObjectInputStream.readObject()with no type restriction:NMLeveldbStateStoreService.loadContainerStatehands it the bytes stored under the container'sassigned-resourceskey and replays them during container recovery on NM restart. A tampered recovery record can therefore instantiate any serializable class on the NodeManager classpath, not just the resource descriptors this code stores.The read is switched to a commons-io
ValidatingObjectInputStreamrestricted to the types the resource plugins actually write: the gpu/fpga/numa value objects under the nodemanager package, the shaded-guavaImmutableMapthatNumaResourceAllocationholds, and the wrapping collections/strings.toBytesis left as-is so records written by earlier NMs still recover.How was this patch tested?
mvn test -pl hadoop-yarn-project/.../hadoop-yarn-server-nodemanager -Dtest=TestResourceMappingson trunk. Added a round-trip test overGpuDevice,FpgaDevice,NumaResourceAllocationandStringso valid records still deserialize, plus a test that a record carrying an element type the plugins never store is rejected instead of instantiated.For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?AI Tooling
If an AI tool was used:
where is the name of the AI tool used.
https://www.apache.org/legal/generative-tooling.html