ARTEMIS-4167 Enhanced deserialization filter #5307
Conversation
Now that Artemis is Java 11+ compatible, there is now the ability to set an ObjectInputFilter on an ObjectInputStream. This allows us to write more advanced filter patterns or plug-in our own object filter implementation.
| * `serialFilter` - A pattern based filter that allows you to define allow/deny lists and constraints limiting graph complexity and size. https://docs.oracle.com/en/java/javase/17/core/serialization-filtering1.html#JSCOR-GUID-8296D8E8-2B93-4B9A-856E-0A65AF9B8C66[Filter Syntax] | ||
| * `serialFilterClassName` - For those who need a custom filtering solution, you can supply an implementation of https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/io/ObjectInputFilter.html[ObjectInputFilter] |
There was a problem hiding this comment.
The links here should likely refer to the Java 11 documentation (e.g. https://docs.oracle.com/en/java/javase/11/core/serialization-filtering1.html).
There was a problem hiding this comment.
A simple example and a blurb to compare & contrast this approach against the existing approach would help users choose what's best for their use-case.
There was a problem hiding this comment.
Sorry, running 17 locally so is the one I'm always referencing. Will update the link.
There was a problem hiding this comment.
I can add a compare and contrast
There was a problem hiding this comment.
Including any details around the impact (e.g precedence) if more than one of the now-several options is specified would also be good. Or else saying, and enforcing, that you cant specify the different approaches at the same time.
There was a problem hiding this comment.
Do you want to continue to support the allow/denyList and custom inputstream? We can say they are deprecated in favor of the new at some point, and list an order of execution if both are specified, although that would be frowned upon
There was a problem hiding this comment.
Regardless how quickly we were to remove the old one, both will be there until then and so what it does or allows / not should be clear and documented. Its not even clear currently what would happen between the 2 new options, or their system properties, let alone once you consider any mix with the older existing stuff.
Even if we deprecate the old, I dont see us removing the old bits particularly quickly (or ever for 2.x) given it is all anyone has ever used, and in many cases will likely continue to use right up until it goes away. We also just deprecated the original names to replace with allow/deny, so would seems especially unfair to then quickly require another update for the same functionality for anyone that did already adapt to that.
| public static final String SERIAL_FILTER_PROPERTY = "org.apache.activemq.artemis.jms.serialFilter"; | ||
| public static final String SERIAL_FILTER_CLASS_NAME_PROPERTY = "org.apache.activemq.artemis.jms.serialFilterClassName"; |
There was a problem hiding this comment.
Neither of these system properties are documented as far as I can tell.
There was a problem hiding this comment.
I wasn't sure the best way of relaying the information as the URL, system properties and resource adapter had separate sections. I stated Configure the same way via url, system properties, resource adapter, or set directly on ActiveMQConnectionFactory in hopes they would know what to do by looking above and substituting denyList or allowList with the one of the two new variable names
| Now that Apache ActiveMQ Artemis requires a minimum JVM version of 11, built-in Java serialization filtering mechanisms can be utilized. | ||
| Instead of providing an `allow list` or `deny list`, you can specify either a `serialFilter` or `serialFilterClassName`. |
There was a problem hiding this comment.
I dont really see a need to include 'Now that Apache ActiveMQ Artemis requires a minimum JVM version of 11'. It conveys nothing particularly useful or necessary about the functionality. Its also just going to go stale.
Simply stating that you can pass a filter string / class name to leverage the built in ObjectInputFilter support is all thats needed.
| Now that Apache ActiveMQ Artemis requires a minimum JVM version of 11, built-in Java serialization filtering mechanisms can be utilized. | ||
| Instead of providing an `allow list` or `deny list`, you can specify either a `serialFilter` or `serialFilterClassName`. | ||
|
|
||
| * `serialFilter` - A pattern based filter that allows you to define allow/deny lists and constraints limiting graph complexity and size. https://docs.oracle.com/en/java/javase/17/core/serialization-filtering1.html#JSCOR-GUID-8296D8E8-2B93-4B9A-856E-0A65AF9B8C66[Filter Syntax] |
There was a problem hiding this comment.
Just as Justin didnt really like "serialFilter" originally on the earlier PR (#4368), I still cant say I am a fan of it.
The more fully elaborated deserializationFilter name would seem more obvious to me.
There was a problem hiding this comment.
I agree, but I was trying to stay consistent with the Oracle system wide property of jdk.serialFilter
There was a problem hiding this comment.
First time that has been mentioned in >18 months, even when you agreed with Justin and said you were renaming it hehe.
I'm not sure that should be a goal personally. Switching to deserializationFilter from deserializationAllowList and deserializationDenyList seems more obvious, and would be more consistent between themselves whilst both are around.
Thoughts @jbertram ?
| * `serialFilter` - A pattern based filter that allows you to define allow/deny lists and constraints limiting graph complexity and size. https://docs.oracle.com/en/java/javase/17/core/serialization-filtering1.html#JSCOR-GUID-8296D8E8-2B93-4B9A-856E-0A65AF9B8C66[Filter Syntax] | ||
| * `serialFilterClassName` - For those who need a custom filtering solution, you can supply an implementation of https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/io/ObjectInputFilter.html[ObjectInputFilter] |
There was a problem hiding this comment.
Including any details around the impact (e.g precedence) if more than one of the now-several options is specified would also be good. Or else saying, and enforcing, that you cant specify the different approaches at the same time.
Now that Artemis is Java 11+ compatible, there is now the ability to set an ObjectInputFilter on an ObjectInputStream. This allows us to write more advanced filter patterns or plug-in our own object filter implementation.