[pip][design] PIP 289: Secure Pulsar Connector Configuration

[michaeljmarshall]

Fixes: #20862

Motivation

PIP for improving connector configuration security. See content for details.

Modifications

Add the design doc for PIP 289.

Current implementation PRs:

• [feat][io] Support configuration secret interpolation #20901
• [fix][io] Improve loading sensitive fields for many connectors #20902

Documentation

• doc

[shibd]

On the three problems you mentioned:

• Elasticsearch Config(Nested configurations): We can improve IOConfigUtils to support it. Right?
• RabbitMQ Config(Not using IOConfigUtils to load config): We can find and fix connectors for these same problems. Right?
• Kafka Config(Use kafka properties map):
  • Perhaps we should refine these keys to create configurations separately
  • Or we can enhance annotations to support set sensitive key names, such as:

    @FieldDoc(
            defaultValue = "",
            help = "Config properties to pass to the kafka connector."
            sensitiveNames="password-1, password-2"
    )
    private Map<String, String> kafkaConnectorConfigProperties;

My point is to avoid introducing complicated configurations for users to understand.

Thanks.

[michaeljmarshall]

Elasticsearch Config(Nested configurations): We can improve IOConfigUtils to support it. Right?

It's possible, but it would likely require a change/expansion to the way the FieldDoc annotation works. However, fixing it for Elasticsearch does not solve the general problem.

RabbitMQ Config(Not using IOConfigUtils to load config): We can find and fix connectors for these same problems. Right?

I already proposed this as part of the PIP here: #20902.

• Kafka Config(Use kafka properties map):

  * Perhaps we should refine these keys to create configurations separately
  * Or we can enhance annotations to support set sensitive key names, such as:
  

    @FieldDoc(
            defaultValue = "",
            help = "Config properties to pass to the kafka connector."
            sensitiveNames="password-1, password-2"
    )
    private Map<String, String> kafkaConnectorConfigProperties;

The Kafka Config is the real reason for this PIP. We need arbitrary interpolation because the Kafka Config works as a wrapper. For example, it is possible to configure it with any connector that works with Kafka. See https://github.com/datastax/pulsar-3rdparty-connector for many third party connectors that are simply plugged into the Kafka Adapter Connector without any additional work. Since it is a wrapper, it is not possible to annotate the relevant fields with a custom Pulsar annotation (FieldDoc). Instead, I propose we find a more general solution.

[shibd]

Hi, @michaeljmarshall. Thanks for your explanation. I got it.

I think we can avoid adding a new config: interpolate_secrets_into_config_map.

What's the problem with us calling the method by default?

    private static void interpolateSecretsIntoConfigs(SecretsProvider secretsProvider, Map<String, Object> configs)

Only when the user uses placeholders will it merge secrets to the config map, right?

      "username": "${username}",
      "password": "${password}",

This way we just need to explain to the user that we provide a new more general way to set secrets.

[michaeljmarshall]

What's the problem with us calling the method by default?

Great question, thanks @shibd. I wrote this text in the PIP explaining why I didn't want to enable it by default:

The primary security consideration is whether there is any risk in giving users a way to interpolate environment variables into their connector. Note that this kind of feature led to Log4Shell. Unlike Log4Shell, the risk is negligible when running in a containerized environment, like Kubernetes. Further, this feature will be disabled by default, so users can evaluate the security risks on their own.

In taking a closer look, the proposed change will only affect k8s based deployments, which probably means we can enable it by default. However, the only remaining risk could be to users that have created their own extensions and are using these classes in unknown ways. Given your comments, I think we should remove the configuration option and always enable it. Let me know what you think, thanks.

[eolivelli]

Lgtm

+1 (binding)

[michaeljmarshall]

@shibd - I updated the text of the PIP. PTAL

[shibd]

Nice work!