YARN: restrict leveldb assigned-resources deserialization to known types - #8689
Open
nishat-06 wants to merge 2 commits into
Open
YARN: restrict leveldb assigned-resources deserialization to known types#8689nishat-06 wants to merge 2 commits into
nishat-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.
Comment on lines
+104
to
+109
| ois.accept( | ||
| "org.apache.hadoop.yarn.server.nodemanager.*", | ||
| "org.apache.hadoop.thirdparty.com.google.common.collect.*", | ||
| "java.util.*", | ||
| "java.lang.*", | ||
| "[Ljava.lang.Object;"); |
Contributor
Author
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.
| "java.util.*", | ||
| "java.lang.*", | ||
| "[Ljava.lang.Object;"); | ||
| resources = (List<Serializable>) ois.readObject(); |
Contributor
Author
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.
Comment on lines
+126
to
+127
| // allowlist rejects it by class name during readObject, before the class | ||
| // is loaded or any of its logic runs. |
Contributor
Author
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. |
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.
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