davsclaus commented on code in PR #26026:
URL: https://github.com/apache/camel/pull/26026#discussion_r3910989387
##########
components/camel-avro/src/main/java/org/apache/camel/dataformat/avro/AvroDataFormat.java:
##########
@@ -135,6 +158,7 @@ protected Schema loadSchema(String className) throws
CamelException, ClassNotFou
@Override
public void marshal(Exchange exchange, Object graph, OutputStream
outputStream) throws Exception {
+ AvroClassSecuritySupport.trustClassName(graph.getClass().getName());
Review Comment:
This unconditionally trusts the *outgoing* message body's runtime class —
and, via `AvroClassSecuritySupport`'s prefix-matched package trust, its entire
package — in a **JVM-wide, process-lifetime** allowlist shared by every
CamelContext/route/data-format instance, with no way to revoke in production.
If the class marshalled here can ever be influenced (directly or indirectly)
by untrusted input in a route, that class/package becomes permanently accepted
for Avro *unmarshalling* everywhere else in the same JVM, including unrelated
routes/contexts — silently widening the allowlist that Avro 1.12.2's
`ClassSecurityValidator` was added to enforce. Since `useSchema` already falls
back to `loadSchema(graph.getClass().getName())` only when `actualSchema` is
null, could the trust here be scoped to only the case where the schema/class
was explicitly configured (data-format construction time), rather than
re-trusting on every `marshal()` call based on whatever the exchange body
happens to be at runtime?
Given this touches the deserialization-security boundary, this seems worth a
security-focused second look before merge.
##########
components/camel-avro/src/main/java/org/apache/camel/avro/support/AvroClassSecuritySupport.java:
##########
@@ -0,0 +1,152 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.avro.support;
+
+import java.util.Arrays;
+import java.util.NavigableSet;
+import java.util.Set;
+import java.util.TreeSet;
+import java.util.concurrent.ConcurrentHashMap;
+
+import org.apache.avro.util.ClassSecurityValidator;
+import org.apache.avro.util.ClassSecurityValidator.ClassSecurityPredicate;
+
+/**
+ * Configures Apache Avro {@link ClassSecurityValidator} with Camel trusted
packages.
+ * <p>
+ * Avro 1.12+ validates classes resolved from schemas. Camel automatically
trusts packages derived from configured
+ * protocol or schema classes. Additional packages can be configured through
the {@code serializablePackages} endpoint
+ * option.
+ */
+public final class AvroClassSecuritySupport {
+
+ private static final Set<String> TRUSTED_PACKAGES =
ConcurrentHashMap.newKeySet();
+
+ private static final Set<String> TRUSTED_CLASSES =
ConcurrentHashMap.newKeySet();
+
+ private static final Object LOCK = new Object();
+
+ private static final ClassSecurityPredicate CAMEL_TRUSTED =
AvroClassSecuritySupport::isCamelTrusted;
+
+ private AvroClassSecuritySupport() {
+ }
+
+ /**
+ * Trusts Avro IPC classes required for camel-avro-rpc handshake.
+ */
+ public static void ensureAvroIpcPackagesTrusted() {
+ trustPackages("org.apache.avro.ipc");
+ }
+
+ /**
+ * Trusts the exact class name and its package for schema resolution.
+ */
+ public static void trustClassName(String className) {
+ if (className == null || className.isBlank()) {
+ return;
+ }
+ synchronized (LOCK) {
+ TRUSTED_CLASSES.add(className);
+ int lastDot = className.lastIndexOf('.');
+ if (lastDot > 0) {
+ TRUSTED_PACKAGES.add(normalizePackage(className.substring(0,
lastDot)));
+ }
+ refreshGlobal();
+ }
+ }
+
+ /**
+ * Trusts the comma-separated list of packages.
+ */
+ public static void trustPackages(String packages) {
+ if (packages == null || packages.isBlank()) {
+ return;
+ }
+ trustPackages(parsePackages(packages).toArray(String[]::new));
+ }
+
+ /**
+ * Trusts the given packages.
+ */
+ public static void trustPackages(String... packages) {
+ if (packages == null || packages.length == 0) {
+ return;
+ }
+ synchronized (LOCK) {
+ for (String pkg : packages) {
+ if (pkg != null && !pkg.isBlank()) {
+ TRUSTED_PACKAGES.add(normalizePackage(pkg));
+ }
+ }
+ refreshGlobal();
+ }
+ }
+
+ /**
+ * Clears Camel-managed trusted classes and packages. Intended for tests.
+ */
+ public static void resetForTesting() {
+ synchronized (LOCK) {
+ TRUSTED_PACKAGES.clear();
+ TRUSTED_CLASSES.clear();
+ ClassSecurityValidator.setGlobal(ClassSecurityValidator.DEFAULT);
+ }
+ }
+
+ private static void refreshGlobal() {
+ ClassSecurityValidator.setGlobal(
+
ClassSecurityValidator.composite(ClassSecurityValidator.DEFAULT,
CAMEL_TRUSTED));
+ }
+
+ private static boolean isCamelTrusted(Class<?> clazz) {
+ String className = clazz.getName();
+ if (TRUSTED_CLASSES.contains(className)) {
+ return true;
+ }
+ NavigableSet<String> packages = normalizedPackages(TRUSTED_PACKAGES);
+ String lower = packages.lower(className);
+ return lower != null && className.startsWith(lower);
+ }
+
+ private static Set<String> parsePackages(String packages) {
+ return Arrays.stream(packages.split(","))
+ .map(String::trim)
+ .filter(s -> !s.isEmpty())
+ .map(AvroClassSecuritySupport::normalizePackage)
+
.collect(java.util.stream.Collectors.toCollection(java.util.LinkedHashSet::new));
+ }
+
+ private static String normalizePackage(String pkg) {
+ String normalized = pkg.trim();
+ if ("*".equals(normalized)) {
+ throw new IllegalArgumentException(
+ "Wildcard '*' is not supported in serializablePackages
because it disables Avro class-loading protection");
+ }
+ if (normalized.endsWith(".")) {
+ normalized = normalized.substring(0, normalized.length() - 1);
+ }
+ return normalized;
+ }
+
+ private static NavigableSet<String> normalizedPackages(Set<String>
packages) {
Review Comment:
`normalizedPackages(TRUSTED_PACKAGES)` builds a brand-new `TreeSet` from
scratch on every call to `isCamelTrusted`, which runs on every
`ClassSecurityValidator.validate()` invocation — i.e. potentially once per
class per Avro message during marshal/unmarshal. That's an avoidable allocation
+ O(n log n) rebuild in a hot serialization path. Consider caching the sorted
view and only rebuilding it when `TRUSTED_PACKAGES` actually changes (e.g.
recompute inside `trustPackages`/`trustClassName` under the existing `LOCK`,
store in a `volatile` field, and have `isCamelTrusted` just read it).
##########
components/camel-avro/src/main/java/org/apache/camel/avro/support/AvroClassSecuritySupport.java:
##########
@@ -0,0 +1,152 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.avro.support;
+
+import java.util.Arrays;
+import java.util.NavigableSet;
+import java.util.Set;
+import java.util.TreeSet;
+import java.util.concurrent.ConcurrentHashMap;
+
+import org.apache.avro.util.ClassSecurityValidator;
+import org.apache.avro.util.ClassSecurityValidator.ClassSecurityPredicate;
+
+/**
+ * Configures Apache Avro {@link ClassSecurityValidator} with Camel trusted
packages.
+ * <p>
+ * Avro 1.12+ validates classes resolved from schemas. Camel automatically
trusts packages derived from configured
+ * protocol or schema classes. Additional packages can be configured through
the {@code serializablePackages} endpoint
+ * option.
+ */
+public final class AvroClassSecuritySupport {
+
+ private static final Set<String> TRUSTED_PACKAGES =
ConcurrentHashMap.newKeySet();
+
+ private static final Set<String> TRUSTED_CLASSES =
ConcurrentHashMap.newKeySet();
+
+ private static final Object LOCK = new Object();
+
+ private static final ClassSecurityPredicate CAMEL_TRUSTED =
AvroClassSecuritySupport::isCamelTrusted;
+
+ private AvroClassSecuritySupport() {
+ }
+
+ /**
+ * Trusts Avro IPC classes required for camel-avro-rpc handshake.
+ */
+ public static void ensureAvroIpcPackagesTrusted() {
+ trustPackages("org.apache.avro.ipc");
+ }
+
+ /**
+ * Trusts the exact class name and its package for schema resolution.
+ */
+ public static void trustClassName(String className) {
+ if (className == null || className.isBlank()) {
+ return;
+ }
+ synchronized (LOCK) {
+ TRUSTED_CLASSES.add(className);
+ int lastDot = className.lastIndexOf('.');
+ if (lastDot > 0) {
+ TRUSTED_PACKAGES.add(normalizePackage(className.substring(0,
lastDot)));
+ }
+ refreshGlobal();
+ }
+ }
+
+ /**
+ * Trusts the comma-separated list of packages.
+ */
+ public static void trustPackages(String packages) {
+ if (packages == null || packages.isBlank()) {
+ return;
+ }
+ trustPackages(parsePackages(packages).toArray(String[]::new));
+ }
+
+ /**
+ * Trusts the given packages.
+ */
+ public static void trustPackages(String... packages) {
+ if (packages == null || packages.length == 0) {
+ return;
+ }
+ synchronized (LOCK) {
+ for (String pkg : packages) {
+ if (pkg != null && !pkg.isBlank()) {
+ TRUSTED_PACKAGES.add(normalizePackage(pkg));
+ }
+ }
+ refreshGlobal();
+ }
+ }
+
+ /**
+ * Clears Camel-managed trusted classes and packages. Intended for tests.
+ */
+ public static void resetForTesting() {
+ synchronized (LOCK) {
+ TRUSTED_PACKAGES.clear();
+ TRUSTED_CLASSES.clear();
+ ClassSecurityValidator.setGlobal(ClassSecurityValidator.DEFAULT);
+ }
+ }
+
+ private static void refreshGlobal() {
Review Comment:
`ClassSecurityValidator.setGlobal(...)` is unconditionally overwritten here
whenever Camel trusts a new package/class. If the embedding application (or
another library) had set its own custom global `ClassSecurityValidator`, this
silently replaces it with `composite(DEFAULT, CAMEL_TRUSTED)`, dropping the
application's own trust rules. Worth composing with whatever was previously
installed (if it wasn't Camel's own) rather than always starting from `DEFAULT`.
##########
components/camel-avro/pom.xml:
##########
@@ -53,6 +48,11 @@
<artifactId>camel-test-junit6</artifactId>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.assertj</groupId>
+ <artifactId>assertj-core</artifactId>
Review Comment:
Every existing test in `camel-avro` (e.g.
`AvroGenericMarshalAndUnmarshalTest`, `AvroDateMarshalAndUnmarshalTest`) uses
JUnit 5 `Assertions`, so this module has an established assertion-style
convention. Per project convention, new tests should follow that existing style
rather than introducing AssertJ as an outlier, and a new test dependency for
two files' worth of style preference doesn't meet the "no new dependencies
without justification" bar. Could `AvroClassSecuritySupportTest` be rewritten
with `org.junit.jupiter.api.Assertions` (`assertThrows`, etc.) instead, and
this dependency dropped?
##########
components/camel-avro-rpc/camel-avro-rpc-component/pom.xml:
##########
@@ -62,6 +61,11 @@
<artifactId>camel-test-junit6</artifactId>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.assertj</groupId>
+ <artifactId>assertj-core</artifactId>
Review Comment:
Same as the note on `camel-avro/pom.xml`: this module's existing tests (e.g.
`AvroSettingsTest`) use JUnit 5 assertions, so introducing `assertj-core` here
just for `AvroClassSecurityWithoutVmArgsTest` is a style outlier and an
unjustified new dependency. Consider using `org.junit.jupiter.api.Assertions`
instead and dropping this dependency.
--
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]
