[bitnami/kafka] Setup of sasl authentication to external zookeeper in server.properties not working

ph311o commented 8 months ago

Name and Version

bitnami/kafka

What architecture are you using?

amd64

What steps will reproduce the bug?

Upgrading Helm-Chart from 23.0.7 to 26.6.2

kraft:
  enabled: false

broker:
  replicaCount: 5
  zookeeperMigrationMode: false

controller:
  replicaCount: 0

zookeeper:
  enabled: false
externalZookeeper:
  servers=[ip-1, ip-2, ip-3 ...]

listeners:
  client:
    protocol: SSL
  interbroker:
    protocol: SSL
  external:
    protocol: SSL

sasl:
  zookeeper:
    user: zookeeper-user
  existingSecret: name-of-the-secret

extraConfig:
...
  authorizer.class.name=kafka.security.authorizer.AclAuthorizer
...

The secret of sasl.existingSecret has a key "zookeeper-password". All keystores are setup properly.

What is the expected behavior?

Kafka brokers connect to external zookeeper through sasl username and password.

What do you see instead?

Kafka brokers do not connect to external zookeeper through sasl username and password and shutdown.

Additional information

After startup the server.properties file of each broker includes these elements:

sasl.jaas.config=org.apache.kafka.common.security.plain.PlainLoginModule required
    username="zookeeper-user"
    password="zookeeper-password-placeholder";

generated at https://github.com/bitnami/charts/blob/main/bitnami/kafka/templates/_helpers.tpl#L679-L683

{{- if .Values.sasl.zookeeper.user }}
sasl.jaas.config=org.apache.kafka.common.security.plain.PlainLoginModule required \
    username="{{ .Values.sasl.zookeeper.user }}" \
    password="zookeeper-password-placeholder";
{{- end }}

Problem 1: The password placeholder is not replaced with proper password due to {{- if (include "kafka.saslEnabled" .) }} in https://github.com/bitnami/charts/blob/main/bitnami/kafka/templates/scripts-configmap.yaml#L286-L316

    {{- if (include "kafka.saslEnabled" .) }}
    configure_kafka_sasl() {

      # Replace placeholders with passwords
      {{- if regexFind "SASL" (upper .Values.listeners.interbroker.protocol) }}
      {{- if (include "kafka.saslUserPasswordsEnabled" .) }}
      replace_placeholder "interbroker-password-placeholder" "$KAFKA_INTER_BROKER_PASSWORD"
      {{- end }}
      {{- if (include "kafka.saslClientSecretsEnabled" .) }}
      replace_placeholder "interbroker-client-secret-placeholder" "$KAFKA_INTER_BROKER_CLIENT_SECRET"
      {{- end }}
      {{- end -}}
      {{- if and .Values.kraft.enabled (regexFind "SASL" (upper .Values.listeners.controller.protocol)) }}
      {{- if (include "kafka.saslUserPasswordsEnabled" .) }}
      replace_placeholder "controller-password-placeholder" "$KAFKA_CONTROLLER_PASSWORD"
      {{- end }}
      {{- if (include "kafka.saslClientSecretsEnabled" .) }}
      replace_placeholder "controller-client-secret-placeholder" "$KAFKA_CONTROLLER_CLIENT_SECRET"
      {{- end }}
      {{- end }}
      {{- if (include "kafka.client.saslEnabled" .)}}
      read -r -a passwords <<<"$(tr ',;' ' ' <<<"${KAFKA_CLIENT_PASSWORDS:-}")"
      for ((i = 0; i < ${#passwords[@]}; i++)); do
          replace_placeholder "password-placeholder-${i}" "${passwords[i]}"
      done
      {{- end }}
      {{- if .Values.sasl.zookeeper.user }}
      replace_placeholder "zookeeper-password-placeholder" "$KAFKA_ZOOKEEPER_PASSWORD"
      {{- end }}
    }
    {{- end }}

kafka.saslEnabled checks only for SASL in listeners but not for zookeeper connection: https://github.com/bitnami/charts/blob/main/bitnami/kafka/templates/_helpers.tpl#L125-L146

{{/*
Return true if SASL connections should be configured
*/}}
{{- define "kafka.saslEnabled" -}}
{{- $res := "" -}}
{{- if (include "kafka.client.saslEnabled" .) -}}
{{- $res = "true" -}}
{{- else -}}
{{- $listeners := list .Values.listeners.interbroker -}}
{{- if and .Values.kraft.enabled -}}
{{- $listeners = append $listeners .Values.listeners.controller -}}
{{- end -}}
{{- range $listener := $listeners -}}
{{- if regexFind "SASL" (upper $listener.protocol) -}}
{{- $res = "true" -}}
{{- end -}}
{{- end -}}
{{- end -}}
{{- if $res -}}
{{- true -}}
{{- end -}}
{{- end -}}

Problem 2: In my opinion there is no property sasl.jaas.config for brokers in server.properties file: https://kafka.apache.org/documentation/#brokerconfigs_sasl.jaas.config

For brokers, the config must be prefixed with listener prefix and SASL mechanism name in lower-case. For example, listener.name.sasl_ssl.scram-sha-256.sasl.jaas.config=com.example.ScramLoginModule required;

So if I include a test kafka_jaas.conf file as workaround with following content sasl connection to zookeeper works as expected:

Client {
  org.apache.kafka.common.security.plain.PlainLoginModule required
  username="zookeeper-user"
  password="replaced password";
}

In order to create a proper PR I need to discuss the following points:

Am I right that we need kafka_jaas.conf file instead of sasl.jaas.config for external zookeeper sasl authentication?
I would say it would be better to use {{- if .Values.sasl.zookeeper.user }} as condition for creating zookeeper config and password replacement in a separate code block instead of {{- if (include "kafka.saslEnabled" .) }} . Any objections?

fevisera commented 8 months ago

Hi @ph311o,

Would you be so kind as to open a PR so our team can discuss the implementation of it?

Thank you!

ph311o commented 8 months ago

Hi @fevisera ,

thank you for response. I will create a PR during week as I am busy at the moment.

Cheers

github-actions[bot] commented 7 months ago

This Issue has been automatically marked as "stale" because it has not had recent activity (for 15 days). It will be closed if no further activity occurs. Thanks for the feedback.

ph311o commented 7 months ago

I added PR waiting for review.

bitnami / charts