Enable per kafka listener sasl #6940

RafalKorepta · 2022-10-26T00:13:30Z

Cover letter

Uptake the following features from core:

Fixes #ISSUE-NUMBER, Fixes #ISSUE-NUMBER, ...

Backport Required

UX changes

Similar to core configuration changes.

Release notes

Features

Allow authentication method per kafka listener.
Allow http basic authentication method for schema registry and panda proxy.

nicolaferraro

Left some comments...

nicolaferraro · 2022-10-26T07:28:55Z

src/go/k8s/apis/redpanda/v1alpha1/cluster_types.go

@@ -1130,3 +1130,10 @@ func defaultTLSConfig() *TLSConfig {
 		NodeSecretRef:     nil,
 	}
 }
+
+func (s Cluster) SASLAuthorizationEnabled() bool {
+	return s.Spec.KafkaEnableAuthorization ||


Wonder if this new flag is needed. Setting authenticationMethod on the listener may be enough.

I followed @BenPope implementation from #5292

In operator we don't have unlimited listener list, so your comment seem right. We have opinionated subset of Redpanda configuration, so I would like to defer this decision to @vladoschreiner.

My implementation is tri-state for kafka_enable_authorization. I.e., if kafka_enable_authorization is false or true, then enable_sasl has no effect.

nicolaferraro · 2022-10-26T07:29:11Z

src/go/k8s/apis/redpanda/v1alpha1/cluster_types.go

+func (s Cluster) SASLAuthorizationEnabled() bool {
+	return s.Spec.KafkaEnableAuthorization ||
+		s.Spec.EnableSASL ||
+		s.InternalListener().AuthenticationMethod == "sasl" ||


Only internal listener?

The SASLAutorizationEnabled function is used only internally to configure:

pandaproxy talking to kafka listener

schema registry talking to kafka listener

console talking to kafka listener

In the code we have assumption to not talk over external listeners.

let's encode that assumption in the method name then? 🙏 IsSASLOnInternalEnabled for example

nicolaferraro · 2022-10-26T07:37:42Z

src/go/k8s/pkg/resources/configmap.go

+	external := r.pandaCluster.PandaproxyAPIExternal()
+	if external != nil {
+		if external.AuthenticationMethod != "" {
+			externalAuthN = &internal.AuthenticationMethod


Probably you meant external

Thank you. Copy-pasta error

alenkacz

Added some initial questions/comments

alenkacz · 2022-10-26T10:42:32Z

src/go/k8s/apis/redpanda/v1alpha1/cluster_types.go

+	// - `false`: authorization is disabled;
+	//
+	// See also `enableSasl` and `configuration.kafkaApi[].authenticationMethod`
+	KafkaEnableAuthorization bool `json:"kafkaEnableAuthorization,omitempty"`


why do we need to have this global flag? Why configuration.kafkaApi[].authenticationMethod is not enough? 🤔

enable_sasl used to:

Enable sasl AuthN on every listener

Enable AuthZ globally.

In order allow different AuthN per listener, but also allow disabling AuthZ with cluster config (on the fly), and keep backwards compatibility, both flags are needed.

alenkacz · 2022-10-26T10:43:29Z

src/go/k8s/apis/redpanda/v1alpha1/cluster_types.go

@@ -562,6 +581,9 @@ type SchemaRegistryAPI struct {
 	External *SchemaRegistryExternalConnectivityConfig `json:"external,omitempty"`
 	// TLS is the configuration for schema registry
 	TLS *SchemaRegistryAPITLS `json:"tls,omitempty"`
+	// AuthenticationMethod can enable authentication method per schema registry
+	// listener. Available options are: none, http_basic.
+	AuthenticationMethod string `json:"authenticationMethod,omitempty"`


for schema registry it enables it on all listeners, right?

We have only one listener.

alenkacz · 2022-10-26T10:44:35Z

src/go/k8s/apis/redpanda/v1alpha1/cluster_types.go

+func (s Cluster) SASLAuthorizationEnabled() bool {
+	return s.Spec.KafkaEnableAuthorization ||
+		s.Spec.EnableSASL ||
+		s.InternalListener().AuthenticationMethod == "sasl" ||


let's encode that assumption in the method name then? 🙏 IsSASLOnInternalEnabled for example

alenkacz · 2022-10-26T10:45:30Z

src/go/k8s/apis/redpanda/v1alpha1/cluster_types.go

+	// AuthenticationMethod can enable authentication method per Kafka
+	// listener. Available options are: none, sasl, mtls_identity.
+	// https://docs.redpanda.com/docs/security/authentication/
+	AuthenticationMethod string `json:"authenticationMethod,omitempty"`


I would like to see webhook validation for the allowed values in AuthenticationMethod 🙏

Ok will add

alenkacz · 2022-10-26T10:50:33Z

src/go/k8s/pkg/resources/configmap.go

@@ -224,18 +224,34 @@ func (r *ConfigMapResource) CreateConfiguration(
 	cr := &cfg.NodeConfiguration.Redpanda

 	internalListener := r.pandaCluster.InternalListener()
+	var internalAuthN *string


can we add something to configmap_test for these?

Ok I will add

nicolaferraro

Looks good overall, just a minor issue and rebasing needed.

nicolaferraro · 2022-10-27T09:44:27Z

src/go/k8s/apis/redpanda/v1alpha1/cluster_types.go

 	EnableSASL bool `json:"enableSasl,omitempty"`
+	// Enable authorization for Kafka connections. Values are:
+	//
+	// - `nil`: Ignored. Authorization is enabled with `enable_sasl: true`


It's a bool, can't be nil.

I changed it to *bool

alenkacz

Just two small comments otherwise LGTM

alenkacz · 2022-10-27T13:14:12Z

src/go/k8s/apis/redpanda/v1alpha1/cluster_webhook.go

@@ -44,6 +44,10 @@ const (
 	transactionCoordinatorReplicationKey = "redpanda.transaction_coordinator_replication"
 	idAllocatorReplicationKey            = "redpanda.id_allocator_replication"

+	noneAuthorizationMechanism         = "none"


I was expecting to see also http_basic here 🤔

alenkacz · 2022-10-27T13:18:15Z

src/go/k8s/apis/redpanda/v1alpha1/cluster_types.go

@@ -129,7 +129,19 @@ type ClusterSpec struct {
 	// List of superusers
 	Superusers []Superuser `json:"superUsers,omitempty"`
 	// SASL enablement flag
+	// +optional
+	// Deprecated: replaced by "kafkaEnableAuthorization"


we should probably either:

verify in webhook someone is not running something like EnableSASL: true, KafkaEnableAuthorization:false

This is unfortunately not possible to prevent from happening. The combination of EnableSASL: true, KafkaEnableAuthorization:false will be possible until core removes enableSASL configuration option.

r-vasquez · 2022-10-27T16:57:36Z

LGTM (rpk- side ) 👍

With changes to authorization configuration options the CRD is updated. REF: redpanda-data#6639

The authentication_method is now available in pandaproxy.

The authentication_method is now available in schema registry.

The ginkgo and gomega tests where asserting too much in one check. Errors where not clear as there were 4 boolean checks.

Previously, the configurator was downloaded from docker hub. Now operator is creating Redpanda clusters with `localhost/configurator:dev` container.

The gofumpt is used now. REF: redpanda-data#4665

When console configuration or config map metadata should be change, then tracing that is hard based on only operator logs. This change adds context to action that are taken in reconciliation function.l

In not so much good internet connection, waiting 2 minutes for config manager to became ready is not enough.

When deployment was changed, so that it uses new kustomize overlay, then the decommission deployment needs to be adjusted as containers where reorder and bring back the 'good' configuration was not valid anymore.

As the current e2e test setup is using localhost/configurator:dev container image definition, the previous step to add configurator tag command argument was not valid anymore. It was easier to not fight with `kubectl patch` command, but declare whole operator deployment to correctly set the configurator image.

The console secret synchronizer was not able to update schema registry secret after cert manager updated schema registry secret for Redpanda cluster. REF: ``` 2022-10-30T21:32:49.741Z ERROR controller.console Reconciler error {"reconciler group": "redpanda.vectorized.io", "reconciler kind": "Console", "name": "console", "namespace": "console", "error": "updating Console synced secret &Secret{ObjectMeta:{console-schema-registry console 2331 0 0001-01-01 00:00:00 +0000 UTC <nil> <nil> map[app.kubernetes.io/component:console app.kubernetes.io/instance:console app.kubernetes.io/managed-by:redpanda-operator app.kubernetes.io/name:redpanda-console app.kubernetes.io/part-of:redpanda-console test.redpanda.vectorized.io/name:updating-console] map[banzaicloud.com/last-applied:UEsDBBQACAAIAAAAAAAAAAAAAAAAAAAAAAAIAAAAb3JpZ2luYWyEkcFO8zAQhN9lz3F+NepPSq5wRwKJC+KwtqetVce27E1RqfLuKNBWpSpws8bf7Oxq9sTJPSMXFwN1tJ1RRRsXLHX0BJMhVFEPYcvC1O3Js4Yv04tTqjeDRg4QlNrFfyb2KQYEoY5MDCV6UHWFc6EIB4M/sJ4Dr2CV3lFHGTZxsKxiQmaJ+aolcI9z+LfxibOouLyOC4rUR73ewkjM7h32LGRIlsWF1ck1VnT4OiiqmDV6VhkrVyTv6AsoiS9uj28B+RFLZASDQt3LRS0/bLKdsU9rnjrTPprNwzTnHh7yaZM8oJpiJEfvkY/Kod+7U/73tamiwU1AaxbNvGla1WozV/NFo5VuWyj9f3FrGTctL1saX8fxIwAA//9QSwcIaDwarhoBAABFAgAAUEsBAhQAFAAIAAgAAAAAAGg8Gq4aAQAARQIAAAgAAAAAAAAAAAAAAAAAAAAAAG9yaWdpbmFsUEsFBgAAAAABAAEANgAAAFABAAAAAA==] [{redpanda.vectorized.io/v1alpha1 Console console 7c824227-7bc4-482b-b77e-b589dae67af7 0xc000db6e01 0xc000db6e00}] [] []},Data:map[string][]byte{},Type:,StringData:map[string]string{},Immutable:nil,}: failed to update resource: secrets \"console-schema-registry\" is forbidden: User \"system:serviceaccount:redpanda-system:default\" cannot update resource \"secrets\" in API group \"\" in the namespace \"console\""} ```

The console config map is immutable and any change into console resource spec will increment console resource generation. That will trigger a operator reconciliation loop which will create new configmap and delete old one if there will be no errors between k8s api server and operator. REF: ``` 2022-10-30T21:52:05.336Z ERROR controller.console Reconciler error {"reconciler group": "redpanda.vectorized.io", "reconciler kind": "Console", "name": "console", "namespace": "console", "error": "deleting unused configmaps: configmaps \"console-7wtlg\" is forbidden: User \"system:serviceaccount:redpanda-system:default\" cannot delete resource \"configmaps\" in API group \"\" in the namespace \"console\""} sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2 /go/pkg/mod/sigs.k8s.io/controller-runtime@v0.9.7/pkg/internal/controller/controller.go:214 ```

alenkacz

LGTM! Thank you

nicolaferraro

Some comments on the validity of the Kuttl tests. I think that they pass even if the API does not reply correctly

nicolaferraro · 2022-10-31T09:16:18Z

src/go/k8s/tests/e2e/redpanda-schema-registry-sasl/02-retrieve-schema.yaml

+                  name: schema-registry-schema-registry-sasl
+                  key: password
+          command:
+            - curl


Unfortunately curl does not fail if the status code is not 2xx, you may need to check it manually

nicolaferraro · 2022-10-31T09:16:44Z

src/go/k8s/tests/e2e/redpanda-schema-registry-sasl/03-delete-schema.yaml

+                  name: schema-registry-schema-registry-sasl
+                  key: password
+          command:
+            - curl


Same, should check http code

nicolaferraro · 2022-10-31T09:17:32Z

src/go/k8s/tests/e2e/redpanda-schema-registry-sasl/05-create-schema-with-tls.yaml

+        - -c
+        args:
+        - >
+          curl -vv --silent --cacert /etc/tls/certs/schema-registry/ca.crt


Same, should check http code

nicolaferraro · 2022-10-31T09:18:24Z

src/go/k8s/tests/e2e/redpanda-schema-registry-sasl/06-retrieve-schema-with-tls.yaml

+            - -c
+          args:
+            - >
+              curl -vv --silent --cacert /etc/tls/certs/schema-registry/ca.crt


Same, should check http code

nicolaferraro · 2022-10-31T09:18:33Z

src/go/k8s/tests/e2e/redpanda-schema-registry-sasl/07-delete-schema-with-tls.yaml

+            - -c
+          args:
+            - >
+              curl -vv --silent --cacert /etc/tls/certs/schema-registry/ca.crt


Same, should check http code

nicolaferraro · 2022-10-31T09:18:57Z

src/go/k8s/tests/e2e/redpanda-schema-registry-sasl/09-create-schema-with-client-tls.yaml

+            - -c
+          args:
+            - >
+              curl -vv --silent


Same, should check http code

nicolaferraro · 2022-10-31T09:19:21Z

src/go/k8s/tests/e2e/redpanda-schema-registry-sasl/10-retrieve-schema.yaml

+            - -c
+          args:
+            - >
+              curl -vv --silent 


Same, should check http code

nicolaferraro · 2022-10-31T09:19:39Z

src/go/k8s/tests/e2e/redpanda-schema-registry-sasl/11-delete-schema.yaml

+            - -c
+          args:
+            - >
+              curl -vv --silent


Same, should check http code

src/go/k8s/tests/e2e/decommission/05-restore-downscaling.yaml

src/go/k8s/controllers/redpanda/cluster_controller.go

BenPope

Sorry, late review.

BenPope · 2023-02-16T21:00:12Z

src/go/k8s/apis/redpanda/v1alpha1/cluster_webhook.go

+	noneAuthorizationMechanism         = "none"
+	saslAuthorizationMechanism         = "sasl"
+	mTLSIdentityAuthorizationMechanism = "mtls_identity"


These are authentication mechanisms.

BenPope · 2023-02-16T21:04:37Z

src/go/k8s/apis/redpanda/v1alpha1/cluster_webhook.go

+
+	for i := range r.Spec.Configuration.KafkaAPI {
+		if r.Spec.Configuration.KafkaAPI[i].AuthenticationMethod == "" {
+			r.Spec.Configuration.KafkaAPI[i].AuthenticationMethod = noneAuthorizationMechanism


There's some difficulty here. The configuration makes this field optional. See: 1c9155a#diff-37dfa5396e57820190992de850ada07cfb605311413bc76aa25d112eccc106d3R469-R495.

Leaving the authentication mechanism as unspecified means:

if kafka_enable_authorization is null: sasl if enable_sasl else none

RafalKorepta requested review from alenkacz, nicolaferraro and pvsune October 26, 2022 00:13

RafalKorepta requested review from twmb, 0x5d, r-vasquez and a team as code owners October 26, 2022 00:13

github-actions bot added area/k8s area/rpk labels Oct 26, 2022

nicolaferraro reviewed Oct 26, 2022

View reviewed changes

RafalKorepta force-pushed the rk/gh-1959/enable-per-kafka-listener-sasl branch 3 times, most recently from 8a29f1a to 4deeb03 Compare October 26, 2022 10:36

alenkacz reviewed Oct 26, 2022

View reviewed changes

RafalKorepta force-pushed the rk/gh-1959/enable-per-kafka-listener-sasl branch 6 times, most recently from 16c438d to a7dd26d Compare October 27, 2022 00:50

nicolaferraro reviewed Oct 27, 2022

View reviewed changes

RafalKorepta force-pushed the rk/gh-1959/enable-per-kafka-listener-sasl branch 2 times, most recently from 07b1f0c to 62e30a5 Compare October 27, 2022 11:06

nicolaferraro previously approved these changes Oct 27, 2022

View reviewed changes

alenkacz previously approved these changes Oct 27, 2022

View reviewed changes

RafalKorepta dismissed stale reviews from alenkacz and nicolaferraro via 333ad64 October 28, 2022 11:24

RafalKorepta force-pushed the rk/gh-1959/enable-per-kafka-listener-sasl branch from 62e30a5 to 333ad64 Compare October 28, 2022 11:24

nicolaferraro previously approved these changes Oct 28, 2022

View reviewed changes

Rafal Korepta added 14 commits October 30, 2022 21:36

k8s: Generate CRD definition after adding pp authentication method

4e60cb7

k8s: Extend schema registry authorization method

d4bc3c8

With changes to authorization configuration options the CRD is updated. REF: redpanda-data#6639

k8s: Generate CRD definition after adding sr authentication method

3739a2c

rpk: Change pandaproxy configuration options

10c1068

The authentication_method is now available in pandaproxy.

rpk: Change schema registry configuration options

bc9f5fa

The authentication_method is now available in schema registry.

k8s: Add authorization method in panda proxy configuration

6dcfcb3

k8s: Add authorization method in schema registry configuration

ffdc589

k8s: Change SASL configuration in pp e2e test

5f9c534

k8s: Run gofmpt against operator code base

6e1dd34

k8s: Clean console controller tests

9ad0859

The ginkgo and gomega tests where asserting too much in one check. Errors where not clear as there were 4 boolean checks.

k8s: Bump kuttle test version

18e0a5c

k8s: Load redpanda container in kindContainers

2831119

k8s: Use configurator from source code

daee21f

Previously, the configurator was downloaded from docker hub. Now operator is creating Redpanda clusters with `localhost/configurator:dev` container.

k8s: Add schema registry e2e tests using http basic authenticationMethod

8cd9034

RafalKorepta dismissed nicolaferraro’s stale review via 27ffbde October 30, 2022 20:36

RafalKorepta force-pushed the rk/gh-1959/enable-per-kafka-listener-sasl branch 2 times, most recently from 27ffbde to 725212d Compare October 30, 2022 21:11

Rafal Korepta added 7 commits October 30, 2022 23:00

k8s: Remove crlfmt formatter

1c150d3

The gofumpt is used now. REF: redpanda-data#4665

k8s: Create more traces for console configuration changes

e4d0bc2

When console configuration or config map metadata should be change, then tracing that is hard based on only operator logs. This change adds context to action that are taken in reconciliation function.l

k8s: Bump certificate manager verification timeout

3afabea

In not so much good internet connection, waiting 2 minutes for config manager to became ready is not enough.

k8s: Change decommission deployment of operator

f8ecfd2

When deployment was changed, so that it uses new kustomize overlay, then the decommission deployment needs to be adjusted as containers where reorder and bring back the 'good' configuration was not valid anymore.

RafalKorepta force-pushed the rk/gh-1959/enable-per-kafka-listener-sasl branch from 725212d to b65e9a1 Compare October 30, 2022 22:07

alenkacz approved these changes Oct 31, 2022

View reviewed changes

nicolaferraro reviewed Oct 31, 2022

View reviewed changes

RafalKorepta merged commit f58c67a into redpanda-data:dev Oct 31, 2022

RafalKorepta mentioned this pull request Dec 20, 2022

Allow pandaproxy and schemaregistry to connect to kafka api over tls #7820

Merged

6 tasks

BenPope reviewed Feb 16, 2023

View reviewed changes

Enable per kafka listener sasl #6940

Enable per kafka listener sasl #6940

Conversation

RafalKorepta commented Oct 26, 2022 • edited by andrewhsu Loading

Cover letter

Backport Required

UX changes

Release notes

Features

nicolaferraro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alenkacz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolaferraro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RafalKorepta Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

alenkacz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-vasquez commented Oct 27, 2022

alenkacz left a comment

Choose a reason for hiding this comment

nicolaferraro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenPope left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RafalKorepta commented Oct 26, 2022 •

edited by andrewhsu

Loading

RafalKorepta Oct 27, 2022 •

edited

Loading