L1nq0 commented on PR #9075: URL: https://github.com/apache/storm/pull/9075#issuecomment-5547645035
Thanks for the thorough review, and for saying up front that the mechanism is the shape you'd have asked for. Taking the points in order: 1. Empty default: agreed, keeping option (2) pure. The defaults.yaml entry goes away, the Config javadoc keeps documenting the key, and the recommended pattern moves into docs/SECURITY.md as a copy-paste block operators adopt deliberately. 2. Deny-list completeness: agreed on the false-confidence read; a twelve-entry list of the names we happened to think of is not a security boundary. The SECURITY.md block will fold in the entries you listed (BadAttributeValueExpException, AnnotationInvocationHandler, com.sun.jndi, java.rmi, clojure, commons-fileupload, bsh, org.python, org.jboss) alongside the original set, carry an explicit not-exhaustive note, and be worded per JEP-290's allow-list-first guidance: a starting point for someone who has consciously enabled the fallback and knows their payload classes, not a boundary. 3. Factory scope: I'd rather close the gap than footnote it, so the plan is to move the installation into SerializationFactory.getKryo(), right after the kryoFactory.getKryo(conf) call, so a custom IKryoFactory gets the filter instead of silently ignoring the key; the Config javadoc will state where the filter applies. If that turns out to fight the extension point in some way I'm not seeing yet, I'll say so on this thread and fall back to documenting the DefaultKryoFactory-only scope explicitly. 4. Submit-time validation: agreed, a nimbus rejection at submit is strictly better than a supervisor restart loop. I'll add a ConfigValidation validator that runs the pattern through ObjectInputFilter.Config.createFilter, and getJavaSerializationFilter will throw IllegalArgumentException instead of RuntimeException. 5. Doc scoping: agreed. The prose in Serialization.md and SECURITY.md will name the tuple fallback bridge specifically and state that DefaultStateSerializer, the escape hatch for pre-kryo state, is not covered by this filter. Smaller things, all accepted: the test fixtures stop squatting third-party package names; coverage keeps the end-to-end round-trips on Storm-owned classes with topology-scoped patterns, and checks the shipped pattern by implementing ObjectInputFilter.FilterInfo in the test and calling checkInput directly. setJavaSerializationFilter becomes a constructor parameter instead of a public setter. The maxbytes allocation stays as is, noted. Merge order: agreed, and thanks for laying out the interaction. Once #9076 lands I'll rebase this on it, so what reviewers see is the composed behaviour: a filtered payload dropped and counted rather than killing the worker. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
