Skip to content

[CALCITE-7773] Kafka adapter should validate class-naming keys in the "consumer.params" operand - #5256

Open
rubenada wants to merge 1 commit into
apache:mainfrom
rubenada:CALCITE-7773
Open

[CALCITE-7773] Kafka adapter should validate class-naming keys in the "consumer.params" operand#5256
rubenada wants to merge 1 commit into
apache:mainfrom
rubenada:CALCITE-7773

Conversation

@rubenada

Copy link
Copy Markdown
Contributor

Jira Link

CALCITE-7773

Changes Proposed

The Kafka adapter's consumer.params model operand is forwarded verbatim to KafkaConsumer in KafkaTableFactory.create(...) (currently via KafkaTableOptions.setConsumerParams(...), then Properties.putAll(...) in KafkaStreamTable). Several Kafka consumer configuration keys cause the client to load and instantiate classes named in the operand as part of new KafkaConsumer<>(config): keys inside it are forwarded without any check, which means a model with, for example, consumer.params.value.deserializer=some.unrelated.Class runs the static initializer of some.unrelated.Class inside the Kafka client before any validation.

Proposal:
Add validation in KafkaTableFactory.create(...) (fail-fast at table creation):

  • key.deserializer / value.deserializer are accepted only after Class.forName(name, false, loader) and Deserializer.class.isAssignableFrom(klass) (the class is not initialized during the check).
  • The other class-naming keys listed above are rejected with a SecurityException naming the offending key and pointing at the system property that can be used to revert to the legacy behavior (see below).
  • All non-class-naming keys (group.id, max.poll.records, fetch.min.bytes, timeouts, security.protocol=PLAINTEXT, etc.) are forwarded unchanged.

Add a new system property calcite.kafka.consumer.params.trusted, registered as CalciteSystemProperty.KAFKA_CONSUMER_PARAMS_TRUSTED, default false. Setting it to true restores the previous forwarding behavior for deployments where the model is entirely operator-supplied (i.e. trusted).

@sonarqubecloud

Copy link
Copy Markdown

@xuzifu666 xuzifu666 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants