AVRO-3985: [JAVA] adding default serializable pacakge to trusted package list #3333

abhilakshyadobhal · 2025-03-08T13:36:58Z

What is the purpose of the change

With this change, the default packages will always be going to be part of trusted packages.

What is the fix?

Static Block Enhancement:
The static block has been updated to always include both user-defined packages (if any) and the default packages (java.lang,java.math,java.io,java.net,org.apache.avro.reflect), while removing duplicate entries.
Constructor Updates:
Constructors in SpecificDatumReader now call initializeTrustedPackages(), ensuring that every instance is correctly populated with the trusted packages.

Verifying this change

This change is already covered by existing tests, such as (please describe tests).

Documentation

Feature Impact:
This commit is a bug fix for the package initialization in SpecificDatumReader and does not introduce any new features.
Code Documentation:
Code comments, especially in the initializeTrustedPackages() method, have been updated to clearly explain the logic behind the inclusion of both user-defined and default packages.

JIRA Ticket

JIRA Ticket: HADOOP-19315 , https://issues.apache.org/jira/browse/AVRO-3985

KalleOlaviNiemitalo · 2025-03-08T14:19:08Z

Previously, if the org.apache.avro.SERIALIZABLE_PACKAGES system property was set as "*", then only that was added to SERIALIZABLE_PACKAGES and to trustedPackages, and trustAllPackages() saw that trustedPackages was a one-element list containing only "*" so it returned true.

Now with this PR, trustedPackages will contain the default trusted packages as well, so it won't be a one-element list, and trustAllPackages() will return false.

That's why I think this should not be merged as is.

Is it possible to add a test case for that regression? I'm not sure how to isolate system property changes from other tests running in parallel.

KalleOlaviNiemitalo · 2025-03-08T14:23:28Z

HADOOP-19315 "Bump avro from 1.9.2 to 1.11.4" was already marked as fixed. If an Avro change is still needed then perhaps that should be a separate AVRO issue in Jira.

KalleOlaviNiemitalo · 2025-03-08T14:34:48Z

lang/java/avro/src/main/java/org/apache/avro/specific/SpecificDatumReader.java

+   * and any duplicate entries are avoided.</p>
+   *
+   * <p>Before adding the packages, the list is cleared to prevent duplicate entries if this method is invoked
+   * multiple times, ensuring that the list remains consistent and up-to-date across all instances.</p>


What does "up-to-date across all instances" mean here?

AFAICS, SERIALIZABLE_PACKAGES is not modified after it has been initialised, so there is no question of the trustedPackages list being "up-to-date" with SERIALIZABLE_PACKAGES.

As the method only modified the trustedPackages list in one instance, the "across all instances" wording seems misleading.

Out of curiosity, were parts of the PR description and the javadoc comment created using generative AI? The Apache Software Foundation publishes some guidelines for that at ASF GENERATIVE TOOLING GUIDANCE.

KalleOlaviNiemitalo · 2025-03-11T12:45:51Z

#3330 fixes AVRO-3985 so that trustedPackages is initialized no matter which constructor of SpecificDatumReader is used. So if that pull request is merged first, then the only user-visible change remaining in this #3333 will be that the default serializable packages are always included. At that point, this PR should be retitled.

KalleOlaviNiemitalo

Wait for #3330 to be merged first, which has an issue in the AVRO project.

Delete the initializeTrustedPackages() calls from those constructors that call this(…).

Retitle this pull request when it no longer adds initializeTrustedPackages() calls in constructors due to the changes above.

Fix the org.apache.avro.SERIALIZABLE_PACKAGES = * case so that trustAllPackages() returns true again. Add a test for that if possible.

KalleOlaviNiemitalo · 2025-03-11T12:48:58Z

lang/java/avro/src/main/java/org/apache/avro/specific/SpecificDatumReader.java

  }

  private final List<String> trustedPackages = new ArrayList<>();

  public SpecificDatumReader() {
    this(null, null, SpecificData.get());
+    initializeTrustedPackages();
  }



Calling initializeTrustedPackages() here is not necessary because this constructor calls another constructor of the same class this(null, null, SpecificData.get()) and that constructor will call initializeTrustedPackages(). It should be called only after the super constructor.

This PR makes similar unnecessary changes to other delegating constructors too.

Have updated the PR, and will rebase once the other change #3330 gets merged.

@KalleOlaviNiemitalo
The PR #3330 has been merged and I have rebased this branch with the main. Can you review the changes again.

…icDatumReader This commit addresses an issue in Avro's SpecificDatumReader where the trustedPackages list was not properly initialized in every constructor. Although the issue is tracked under HADOOP-19315 in the Hadoop JIRA board, the underlying problem exists in Avro. Changes include: - Modifying the static initialization block to combine both user-defined packages from the org.apache.avro.SERIALIZABLE_PACKAGES system property and the default packages ("java.lang,java.math,java.io,java.net,org.apache.avro.reflect"), ensuring that duplicates are removed. - Updating the initializeTrustedPackages() method to call clear() before adding packages, preventing duplicate entries. - Ensuring that initializeTrustedPackages() is invoked in every constructor of SpecificDatumReader so that each instance always has the correct trustedPackages list. This improvement ensures that custom packages specified via the system property are correctly recognized as trusted, preventing security errors during deserialization.

jbonofre · 2025-03-22T05:25:06Z

lang/java/avro/src/main/java/org/apache/avro/specific/SpecificDatumReader.java

-        "java.lang,java.math,java.io,java.net,org.apache.avro.reflect").split(",");
+    String defaultPackages = "java.lang,java.math,java.io,java.net,org.apache.avro.reflect";
+
+    String userDefinedPackages = System.getProperty("org.apache.avro.SERIALIZABLE_PACKAGES", "");


Why not simply returning defaultPackages in default value of System.getProperty() ?

If the user defines his own value, maybe it would be surprising to always add defaultPackages.

Mabye more "logic" to have defaultPackages by default, but not if the user defines his own packages.

It's noted that we can do that.
Even though Avro already knows some default packages that are considered trustable, if a user defines custom packages and forgets to include these default ones, it may lead to errors when deserializing objects that rely on them.

I got your point, but if the user doesn't want the default packages (for security reasons), he can't really remove these packages from the trusted ones.
Maybe more a warn message or documentation ?

Hey @jbonofre I have updated the PR, pls check if it looks good.

Thanks, it looks good to me !

github-actions bot added the Java Pull Requests for Java binding label Mar 8, 2025

martin-g requested a review from jbonofre March 8, 2025 13:56

abhilakshyadobhal force-pushed the patch-1 branch from 0da2588 to f20ed6f Compare March 8, 2025 13:59

KalleOlaviNiemitalo reviewed Mar 8, 2025

View reviewed changes

KalleOlaviNiemitalo suggested changes Mar 11, 2025

View reviewed changes

abhilakshyadobhal changed the title ~~HADOOP-19315: [Java] Ensure trustedPackages is initialized in all constructors of SpecificDatumReader~~ AVRO-3985: [JAVA] adding default serializable pacakge to trusted package list Mar 11, 2025

abhilakshyadobhal added 2 commits March 22, 2025 09:48

AVRO-3985: adding default serializable pacakge to trusted package list

0beafc0

abhilakshyadobhal force-pushed the patch-1 branch from 68e9421 to 0beafc0 Compare March 22, 2025 04:20

jbonofre reviewed Mar 22, 2025

View reviewed changes

abhilakshyadobhal added 2 commits March 22, 2025 15:08

added-documentation-for-default-packages

bdfff49

added-documentation-for-default-packages

0393725

jbonofre approved these changes Mar 23, 2025

View reviewed changes

abhilakshyadobhal requested a review from KalleOlaviNiemitalo March 23, 2025 15:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AVRO-3985: [JAVA] adding default serializable pacakge to trusted package list #3333

AVRO-3985: [JAVA] adding default serializable pacakge to trusted package list #3333

abhilakshyadobhal commented Mar 8, 2025 •

edited

Loading

KalleOlaviNiemitalo commented Mar 8, 2025 •

edited

Loading

KalleOlaviNiemitalo commented Mar 8, 2025

KalleOlaviNiemitalo Mar 8, 2025

KalleOlaviNiemitalo commented Mar 11, 2025

KalleOlaviNiemitalo left a comment

KalleOlaviNiemitalo Mar 11, 2025

abhilakshyadobhal Mar 11, 2025 •

edited

Loading

abhilakshyadobhal Mar 22, 2025

jbonofre Mar 22, 2025

abhilakshyadobhal Mar 22, 2025 •

edited

Loading

jbonofre Mar 22, 2025

abhilakshyadobhal Mar 22, 2025

jbonofre Mar 23, 2025

AVRO-3985: [JAVA] adding default serializable pacakge to trusted package list #3333

Are you sure you want to change the base?

AVRO-3985: [JAVA] adding default serializable pacakge to trusted package list #3333

Conversation

abhilakshyadobhal commented Mar 8, 2025 • edited Loading

What is the purpose of the change

What is the fix?

Verifying this change

Documentation

JIRA Ticket

KalleOlaviNiemitalo commented Mar 8, 2025 • edited Loading

KalleOlaviNiemitalo commented Mar 8, 2025

KalleOlaviNiemitalo Mar 8, 2025

Choose a reason for hiding this comment

KalleOlaviNiemitalo commented Mar 11, 2025

KalleOlaviNiemitalo left a comment

Choose a reason for hiding this comment

KalleOlaviNiemitalo Mar 11, 2025

Choose a reason for hiding this comment

abhilakshyadobhal Mar 11, 2025 • edited Loading

Choose a reason for hiding this comment

abhilakshyadobhal Mar 22, 2025

Choose a reason for hiding this comment

jbonofre Mar 22, 2025

Choose a reason for hiding this comment

abhilakshyadobhal Mar 22, 2025 • edited Loading

Choose a reason for hiding this comment

jbonofre Mar 22, 2025

Choose a reason for hiding this comment

abhilakshyadobhal Mar 22, 2025

Choose a reason for hiding this comment

jbonofre Mar 23, 2025

Choose a reason for hiding this comment

abhilakshyadobhal commented Mar 8, 2025 •

edited

Loading

KalleOlaviNiemitalo commented Mar 8, 2025 •

edited

Loading

abhilakshyadobhal Mar 11, 2025 •

edited

Loading

abhilakshyadobhal Mar 22, 2025 •

edited

Loading