CAMEL-24578: Configure Avro ClassSecurityValidator at runtime for camel-avro-rpc - #26026
Conversation
Bugbot reviewResult: No bugs found. Bugbot reviewed the branch changes for CAMEL-24578 (Avro 1.12.2 No actionable defects were reported. AI-generated comment by Cursor Agent on behalf of atiaomar1978-hub |
Implementation summaryThis PR fixes CAMEL-24578 by configuring Apache Avro 1.12.2 Key changes
Files touched
AI-generated comment by Cursor Agent on behalf of atiaomar1978-hub |
Grok code review — findings and resolutionsGrok reviewed the initial implementation and flagged several issues. Below is each finding and how it was addressed in commit Security / correctness
Architecture note (accepted trade-off)
Test improvements made
Remaining follow-ups (non-blocking)
AI-generated comment by Cursor Agent on behalf of atiaomar1978-hub |
Testing evidencemvn test -pl components/camel-avro,components/camel-avro-rpc/camel-avro-rpc-component -amResults:
New test classes:
AI-generated comment by Cursor Agent on behalf of atiaomar1978-hub |
|
the data format needs to add thee new option in the core model as well, and then also to regen many files because of that |
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Reviewed against the project's rules/conventions (this does not replace CodeRabbit/Sourcery/SonarCloud or a dedicated security review). I built and ran camel-avro and camel-avro-rpc-component in a worktree — both compile and all tests (existing + new) pass. I also checked CAMEL-24578 and the prior Avro-1.12.2-bump commit: this is legitimate follow-up work on an intentional stop-gap, not a revert of prior design.
1. Generated metadata is out of sync (will fail CI's uncommitted-changes check). I regenerated the downstream artifacts locally and got real diffs the PR doesn't include:
catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/components/avro.json— missing the newserializablePackagesentry.dsl/camel-endpointdsl/src/generated/java/org/apache/camel/builder/endpoint/dsl/AvroEndpointBuilderFactory.java— missing the fluentserializablePackages(...)builder method.dsl/camel-componentdsl/src/generated/java/org/apache/camel/builder/component/dsl/AvroComponentBuilderFactory.java— same.
These three need mvn install (or the project's catalog/endpoint-dsl/component-dsl regen step) run and the results committed, in that order.
2. The new option is missing from the camel-avro data-format model, so it isn't usable from XML/YAML/fluent-Java DSL. org.apache.camel.model.dataformat.AvroDataFormat in core/camel-core-model (the @XmlRootElement model class backing <avro/> in XML DSL, YAML DSL, and the DataFormatDefinition fluent builder) was not updated to add a serializablePackages field/attribute alongside instanceClassName, schema, etc. As it stands, serializablePackages on the camel-avro data format is only reachable by constructing org.apache.camel.dataformat.avro.AvroDataFormat directly in Java and calling the setter — not from .marshal().avro(...)'s model-backed configuration, XML routes, or YAML routes. This needs the field added to the model class (with @XmlAttribute/@Metadata), its Builder counterpart, and the associated generated resources (core/camel-core-model/src/generated/resources/META-INF/org/apache/camel/model/dataformat/avro.json, YAML DSL deserializer, XML schema) regenerated.
3. (Positive, no action needed) The camel-avro-rpc component-level configuration already works correctly for serializablePackages — it follows the existing AvroConfiguration/getOrCreateConfiguration() pattern shared by reflectionProtocol, singleParameter, etc., so it's already settable once on the component (e.g. camel.component.avro.serializable-packages=...) and inherited by every endpoint created from it, consistent with sibling options. Good reuse of the existing pattern.
4. Security design concern (see inline comments). AvroClassSecuritySupport is a JVM-wide static singleton shared by every CamelContext/route/data-format instance, and AvroDataFormat.marshal() unconditionally trusts the outgoing body's runtime class (and, via prefix matching, its whole package) forever, with no way to revoke in production. That's a meaningful weakening of the exact protection Avro 1.12.2's ClassSecurityValidator introduced, and it can leak trust across otherwise-unrelated routes/contexts in the same JVM. Given this touches the deserialization-security boundary, I'd like a security-minded committer to take a second look at that design before merge, per the project's security review checklist.
5. (Minor) Test convention. Both camel-avro and camel-avro-rpc-component add assertj-core purely for the two new test classes even though every existing test in both modules uses JUnit 5 assertions — a style outlier plus an unjustified new dependency (see inline comments). There's also an avoidable per-call allocation in the trust-check hot path (see inline comment).
None of this is a knock on the effort — the underlying problem (Avro 1.12.2 breaking camel-avro/camel-avro-rpc without a trust config) is real and the fix direction (programmatic ClassSecurityValidator configuration) matches what CAMEL-24578 asked for. It needs the generated-metadata sync, the data-format model gap, and the trust-scoping/lifetime question resolved before this changes production security posture and ships a complete, DSL-usable option.
Separately: CI hasn't run on this PR yet (gh pr checks reports no checks) — worth confirming it's green (likely needs workflow approval for this contributor) independent of the above.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
Review feedback addressed — commit
|
| # | Review item | Resolution |
|---|---|---|
| 1 | Generated metadata out of sync (catalog, endpoint DSL, component DSL) | Regenerated and committed — serializablePackages in avro.json, AvroEndpointBuilderFactory, AvroComponentBuilderFactory |
| 2 | serializablePackages missing from core AvroDataFormat model |
Added @XmlAttribute + @Metadata, Builder, AvroDataFormatReifier wiring; regen includes YAML deserializer + XML schema |
| 3 | setGlobal() clobbers custom validators |
Captures existing ClassSecurityValidator.getGlobal() on first trust and composes; test shouldPreserveExistingGlobalValidator |
| 4 | Per-validate TreeSet allocation |
Cached normalizedPackagePrefixes, rebuilt only on allowlist change |
| 5 | Unconditional marshal-time trust | Trust graph class only when actualSchema == null |
| 6 | AssertJ outlier in test modules | JUnit 5 assertions; removed assertj-core from both POMs |
Tests (against main merge-base)
mvn test -pl components/camel-avro,components/camel-avro-rpc/camel-avro-rpc-component -amAll tests pass without Surefire SERIALIZABLE_PACKAGES vmargs.
Ready for re-review.
✅ Generated files are up to dateAn earlier CI run reported uncommitted generated changes; the latest run no longer does. |
Croway
left a comment
Croway
left a comment
There was a problem hiding this comment.
Thanks for tackling this, it's a much better experience than requiring -Dorg.apache.avro.SERIALIZABLE_PACKAGES. I ran the trust logic against avro-1.12.2 with a few probes and found six issues worth addressing before merge (three are security-relevant since the allowlist is JVM-global and irrevocable, three are functional regressions vs. main). Details inline.
Claude Code on behalf of Croway
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 577 tested, 21 compile-only — current: 571 all testedMaveniverse Scalpel detected 598 affected modules (current approach: 571).
|
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Sound approach to configuring Avro's ClassSecurityValidator at runtime, replacing the JVM-property workaround. The auto-inference of trusted packages from schema/protocol configuration is well-designed. Security annotations and wildcard rejection are correct. Thread safety is properly handled.
A few observations:
Module coupling (medium): The new compile dependency from camel-avro-rpc-component on camel-avro (the data format module) couples two previously independent modules solely to share AvroClassSecuritySupport. Consider placing the utility in a shared location such as camel-avro-rpc-spi (which already exists) or a minimal camel-avro-support module.
FQCN usage (low): Line 158 of AvroClassSecuritySupport.java uses java.util.stream.Collectors and java.util.LinkedHashSet inline instead of import statements, violating the project convention. The build's OpenRewrite step may auto-fix this.
Catalog metadata leak (low): serializablePackages appears in catalog metadata for avroJackson and avroJackson3, but the reifier only passes it for ApacheAvro. Users of Jackson-based Avro data formats would find it silently ignored. Adding serializablePackages to the excludeProperties of JacksonAvroDataFormat's @Metadata annotation would fix the catalog.
Redundant call (low): ensureAvroIpcPackagesTrusted() is called in both AvroComponent.doInit() and AvroEndpoint.configureClassSecurity(). The endpoint call is always redundant since the component initializes first. Harmless but unnecessary.
📋 PR Metadata
| Aspect | Current | Suggested |
|---|---|---|
| Labels | components, core, catalog, docs, dsl |
+ security |
| Milestone | (none) | 4.23.0 |
🤖 This review was generated by Claude Code on behalf of Guillaume Nodet
|
@atiaomar1978-hub can you look at @gnodet comments there are valid and would be good to correct before merging |
515450f to
69f78c2
Compare
Croway + davsclaus review feedback addressed — rebased on
|
| # | Issue | Fix |
|---|---|---|
| 1 | JDK packages leak via reflection protocol error types | trustProtocol() skips type.isError() schemas; isSystemPackage() filters java.*/javax.*/jdk.*/sun.* |
| 2 | Schema-less marshal trusts body package before validation | Marshal now calls loadSchema() first; only then trustClassNameOnly() (exact class, no package) |
| 3 | Later setGlobal() silently discarded |
refreshGlobal() re-adopts getGlobal() when it differs from installedGlobal |
| 4 | Non-named root schemas fail at startup | trustSchema() walks graph; ARRAY/UNION/MAP roots no longer call getNamespace() |
| 5 | Prefix matching shadows parent packages | Iterate all normalized prefixes instead of single lower() lookup |
| 6 | Nested named types not trusted on unmarshal | trustSchema() / trustProtocol() walk fields, array items, map values, union branches |
Other review items
- Removed redundant
ensureAvroIpcPackagesTrusted()from endpoint (component init only) - Excluded
serializablePackagesfrom Jackson Avro catalog metadata (avroJackson/avroJackson3) - Expanded unit tests (prefix matching, global validator preservation, non-named schemas, JDK filtering)
Tests
mvn test -pl components/camel-avro,components/camel-avro-rpc/camel-avro-rpc-componentAll unit tests pass without Surefire SERIALIZABLE_PACKAGES vmargs.
Ready for re-review.
|
Did we fix this
|
|
@davsclaus will double confirm. |
|
Confirmed fixed ✅ — verified against the branch head (
Generated output is consistent with the source, so the catalog metadata leak is resolved. Reviewed by Claude Code on behalf of @davsclaus |
Configure Apache Avro ClassSecurityValidator automatically for camel-avro-rpc and camel-avro so Avro 1.12.2 works without requiring the SERIALIZABLE_PACKAGES JVM property. Trust org.apache.avro IPC packages on component init and infer model packages from configured protocol or schema classes. Add serializablePackages option for extra packages, tests without surefire vmargs, and upgrade guide updates. Co-authored-by: Cursor Agent <noreply@cursor.com>
Store trusted packages in memory instead of a mutable system property, scope IPC trust to org.apache.avro.ipc, mark serializablePackages as insecure:serialization, trust marshal graph classes at runtime, and expand tests with AssertJ and HandshakeRequest coverage. Co-authored-by: Cursor Agent <noreply@cursor.com>
Add serializablePackages to core AvroDataFormat model and regenerate catalog, endpoint/component DSL, YAML deserializer, and XML schema. Preserve pre-existing ClassSecurityValidator on first trust, cache normalized package prefixes, scope marshal-time trust to dynamic schema paths, migrate security tests to JUnit assertions, and remove assertj test dependencies. Co-authored-by: Cursor Agent <noreply@cursor.com>
Walk schema and protocol graphs for trusted types, skip JDK/error schemas, fix prefix matching, preserve externally installed global validators, trust marshal body only after GenericContainer validation using exact class names, and exclude serializablePackages from Jackson Avro catalog metadata. Co-authored-by: Cursor Agent <noreply@cursor.com>
…update Co-authored-by: Cursor Agent <noreply@cursor.com>
69f78c2 to
2af134d
Compare
Rebased onto
|
| Reviewer | Status |
|---|---|
| @davsclaus | All inline threads replied — metadata regen, core model option, global validator composition, cached prefixes, JUnit assertions |
| @Croway | All 6 security/functional threads addressed in 9440ae54 (schema walk, system-package filter, exact-class marshal trust, prefix iteration, external validator preservation) |
| @gnodet | Jackson Avro catalog leak fixed via excludeProperties; FQCN imports cleaned; redundant endpoint IPC call removed (component-only init) |
Tests
mvn test -pl components/camel-avro,components/camel-avro-rpc/camel-avro-rpc-component -amAll pass without -Dorg.apache.avro.SERIALIZABLE_PACKAGES vmargs.
Ready for merge pending CI.
5 participants
Summary
Fixes CAMEL-24578:
camel-avro-rpc(andcamel-avro) fail with Apache Avro 1.12.2 becauseClassSecurityValidatorrejects IPC handshake and application model classes unless theSERIALIZABLE_PACKAGESJVM property is set manually.This PR configures Avro's
ClassSecurityValidatorat runtime so routes work out of the box:AvroClassSecuritySupport(incamel-avro) maintains an in-memory allowlist and extends the global Avro validatorcamel-avro-rpc: trustsorg.apache.avro.ipcon component init; infers model packages from protocol class, namespace, and schema typescamel-avrodata format: trusts packages from schema / instance class; trusts marshal graph class at runtimeserializablePackageson Avro RPC endpoints (and data format@Metadata) for additional trusted packages, annotatedsecurity = "insecure:serialization"-Dorg.apache.avro.SERIALIZABLE_PACKAGES=...vmargs (68 RPC tests + unit tests pass)Review feedback addressed
org.apache.avro.ipc(RPC component only)*rejected explicitly@BeforeEachtest isolation viaresetForTesting()Test plan
mvn test -pl components/camel-avro,components/camel-avro-rpc/camel-avro-rpc-component -amcamel-avro-rpctests pass without JVM serializable-packages vmargsAvroClassSecuritySupportTestandAvroClassSecurityWithoutVmArgsTestAI-generated PR description by Cursor Agent on behalf of atiaomar1978-hub