[Enhancement] Enable overriding ssl.principal.mapping.rules when not using User Operator

[yyaari]

When managing SSL users without using the User Operator, it is sometimes desired to be able to map a long SSL user (of the form: "CN=writeuser,OU=Unknown,O=Unknown,L=Unknown,ST=Unknown,C=Unknown"). This is possible by setting the ssl.principal.mapping.rules property in the broker config file, however, this is currently not possible as the cluster operator is not allowing this.

I think that when the User Operator is not in use, the option to set this property should be enabled.

[scholzj]

I have been thinking abotu this. It is not so easy, since Strimzi is using TLS authentication internally. So if you change the way the user name is built, you will break it completely. So this needs to be done in a way that the internal listeners keep using the default mapping.

[yyaari]

You can add the DEFAULT rule at the end of user provided rules so if all rules fails, kafka will resolve the ssl user by the subject like today

[yyaari]

Hi, any news about this?

[scholzj]

No. As I said, I do not think this is easy since the same afects the internal interfaces and communication.

[scholzj]

I looked into this in more detail. The ssl.principal.mapping.rules option can be configure only at a cluster level. It cannot be overridden at listener level. So overriding it affects not only the user listeners but also the replication, communication between Strimzi components etc.

So I guess there are three options:

• Making it configurable, but at users own risk => if the user does not make sure the internal accounts keep the right names, the Kafka cluster will break
• Make it configurable, but automatically inject our own rule into the first place. This could in general conflict with the user defined rules, but since we use our own organization (O=io.strimzi) in the certificate subjects, it is not likely.
• Not do anything and keep this option locked.

I will raise this on the next community call to discuss with other how do we want to proceed here.

[scholzj]

Discussed on the Community Call: We should investigate the a custom principal builder might be useful here. It might add another option to this.

[sreejesh-radhakrishnan-db]

HI @scholzj any update on this please ?

[scholzj]

In 0.28.0 you will be able to add your own principal.builder.class if you would want - that was done as part of #6162. That perhaps a bit unintentionally does what was mentioned in the last point. I do not think anything else is planned at this point. Use at your own risk.

[sreejesh-radhakrishnan-db]

just to check, "Use at your own risk" - what you see happening wrong? so that I am prepared

[scholzj]

The principal builder class is cluster wide => it applies for the whole cluster including the replication connections between brokers, connections from the operator etc. So as described int he docs, you have to make sure the usernames for those connection remain unchange, otherwise you will break the operators / the Kafka cluster.

[scholzj]

Triaged on 17.3.2022: This might make things easier then overriding the principal builder class. But it is not as easy as allowing the configuration option because it would need to ensure that the internal authentication and authorization still works. So proposal for how to do this would be needed first.

[eh-steve]

Just resurrecting this issue, as we've come across this use case where we want HashiCorp Vault issued certificates to have a custom principal mapping rule to extract the common name to avoid the DN's OU and O needing to be included in the ACL principal names.

Would the below be enough:

Add an extra validation step into cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java which parses any provided mapping rules using org.apache.kafka.common.security.ssl.SslPrincipalMapper directly, and applies the rules onto a collection of strimzi internal distinguished names like:

• CN=cluster-ca v1,O=io.strimzi
• CN=blah-kafka-exporter,O=io.strimzi
• CN=blah-entity-user-operator,O=io.strimzi
• etc.

Then test that the resulting principal names come out unaltered, and reject the CRD if not. The error message could even suggest prefixing the user's custom rule with something like:

RULE:^CN=(.*?),O=io\.strimzi$/CN=$1,O=io.strimzi/,

Then the ssl.principal.mapping.rules property can be allowed directly, without risking breaking the cluster (as the CRD would fail validation).

We could use the actual generated common names instead of placeholders since the cluster name will be known by this point.

I'd be happy to implement and PR if this is acceptable?

Subject: [PATCH] WIP SslPrincipalMappingValidator
---
Index: cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java
===================================================================
diff --git a/cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java b/cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java
--- a/cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java    (revision c2008aaabe09dfcea9bdd107a468c2eaabb53d63)
+++ b/cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java    (date 1690885049860)
@@ -317,6 +317,11 @@
         ListenersValidator.validate(reconciliation, result.brokerNodes(), listeners);
         result.listeners = listeners;

+        String sslPrincipalMappingRules = result.configuration.getConfigOption("ssl.principal.mapping.rules");
+        if (sslPrincipalMappingRules != null) {
+            SslPrincipalMappingValidator.validate(sslPrincipalMappingRules, kafka.getMetadata().getName() );
+        }
+
         // Set authorization
         if (kafkaClusterSpec.getAuthorization() instanceof KafkaAuthorizationKeycloak) {
             if (!ListenersUtils.hasListenerWithOAuth(listeners)) {
Index: cluster-operator/src/main/java/io/strimzi/operator/cluster/model/SslPrincipalMappingValidator.java
===================================================================
diff --git a/cluster-operator/src/main/java/io/strimzi/operator/cluster/model/SslPrincipalMappingValidator.java b/cluster-operator/src/main/java/io/strimzi/operator/cluster/model/SslPrincipalMappingValidator.java
new file mode 100644
--- /dev/null   (date 1690885222876)
+++ b/cluster-operator/src/main/java/io/strimzi/operator/cluster/model/SslPrincipalMappingValidator.java    (date 1690885222876)
@@ -0,0 +1,33 @@
+package io.strimzi.operator.cluster.model;
+
+import io.strimzi.api.kafka.model.KafkaResources;
+import org.apache.kafka.common.security.ssl.SslPrincipalMapper;
+
+import java.io.IOException;
+import java.util.List;
+
+public class SslPrincipalMappingValidator {
+
+    public static void validate(String mappingRules, String clusterName) throws InvalidResourceException {
+        List<String> INTERNAL_NAMES = List.of(
+                String.format("CN=%s,O=io.strimzi", KafkaResources.kafkaStatefulSetName(clusterName)),
+                String.format("CN=%s-%s,O=io.strimzi", clusterName, "entity-topic-operator"),
+                String.format("CN=%s-%s,O=io.strimzi", clusterName, "entity-user-operator"),
+                String.format("CN=%s-%s,O=io.strimzi", clusterName, "kafka-exporter"),
+                String.format("CN=%s-%s,O=io.strimzi", clusterName, "cruise-control"),
+                String.format("CN=%s,O=io.strimzi", "cluster-operator")
+                // TODO - add any more internal names
+        );
+        SslPrincipalMapper mapper = new SslPrincipalMapper(mappingRules);
+        try {
+            for (String name : INTERNAL_NAMES) {
+                String newName = mapper.getName(name);
+                if (!newName.equals(name)) {
+                    throw new InvalidResourceException(String.format("expected \"ssl.principal.mapping.rules\" to leave internal names unchanged, %s mapped to %s. Try prepending a rule such as 'RULE:^CN=(.*?),O=io\\.strimzi$/CN=$1,O=io.strimzi/', to your rules", name, newName));
+                }
+            }
+        } catch (IOException ignored) {
+
+        }
+    }
+}
Index: api/src/main/java/io/strimzi/api/kafka/model/KafkaClusterSpec.java
===================================================================
diff --git a/api/src/main/java/io/strimzi/api/kafka/model/KafkaClusterSpec.java b/api/src/main/java/io/strimzi/api/kafka/model/KafkaClusterSpec.java
--- a/api/src/main/java/io/strimzi/api/kafka/model/KafkaClusterSpec.java    (revision c2008aaabe09dfcea9bdd107a468c2eaabb53d63)
+++ b/api/src/main/java/io/strimzi/api/kafka/model/KafkaClusterSpec.java    (date 1690887307132)
@@ -48,7 +48,7 @@
             + "node.id, process.roles, controller."; // KRaft options

     public static final String FORBIDDEN_PREFIX_EXCEPTIONS = "zookeeper.connection.timeout.ms, sasl.server.max.receive.size,"
-            + "ssl.cipher.suites, ssl.protocol, ssl.enabled.protocols, ssl.secure.random.implementation,"
+            + "ssl.cipher.suites, ssl.protocol, ssl.enabled.protocols, ssl.secure.random.implementation, ssl.principal.mapping.rules,"
             + "cruise.control.metrics.topic.num.partitions, cruise.control.metrics.topic.replication.factor, cruise.control.metrics.topic.retention.ms,"
             + "cruise.control.metrics.topic.auto.create.retries, cruise.control.metrics.topic.auto.create.timeout.ms,"
             + "cruise.control.metrics.topic.min.insync.replicas,"

[bmaximuml]

@scholzj would be great to get your view on this. We're happy to PR if that's useful

[scholzj]

To be honest, that seems a bit hacky. What if it stops working one day? What if more entities need to be added and it suddenly breaks your cluster because they do not match your rules anymore and you will be unable to upgrade for example? What if someone forgets to add a name to the list and it breaks your cluster right when you need some important CVE update? I'm not really sure this provides a stable solution.

That might happen with the custom principal builder class as well. But that has a much lower risk of unintentionally falling into a trap (by not understanding the risks and consequences) because you need to intentionally add your custom principal builder knowing the risks it has.

In any case, in the triage it was decided that this would require a proposal. So if you are convinced that this is a good solution, feel free to open one: https://github.com/strimzi/proposals

[eh-steve]

Would it help if there was a single list where strimzi internal principal names were added, and the same list was used in all places (CAs, ACLs, this check etc)?

I’m not sure I understand how a change could break a cluster that wouldn’t be caught during CRD validation, as a version update requires a CRD change?

[scholzj]

I think that would be quite hard to do. And it still does not guarantee that the names would not change or be modified in the next release.

I’m not sure I understand how a change could break a cluster that wouldn’t be caught during CRD validation, as a version update requires a CRD change?

This has nothing to do with the CRDs. Imagine the names change the format completely. Or new names not matching your pattern are added. Suddenly, it will be broken and it might not be possible to fix it without for example renaming all your users. There is no guarantee that the Strimzi names will remain the same and I do not think we can give one.

[eh-steve]

Ah I think I understand what you mean. Is there anywhere we could intercept before an upgrade to validate the configuration before attempting to reconcile?

[scholzj]

No, you cannot intercept it. You need the new operator to validate it. But the problem is bigger -> what if it is not possible anymore to do this? What if you need to rename all users for example?

[eh-steve]

Sorry, I’m not following - who would rename all the users, and where does the principal mapping come into it?

[scholzj]

The names of the Strimzi internal users are not public API, they are an internal implementation detail. They can change in the next release if there is some reason for it and break your mapping rules in any way you can imagine. There is no guarantee that they will look the same in the long term.

[eh-steve]

Gotcha, I understand now. I guess I’m asking, is there no part of the upgrade lifecycle where an upgrade process can be prevented from starting by validating an existing cluster config before it begins the upgrade? Surely if all the internal principal names changed, something would have to create the new principals and grant them cluster admin ACLs etc - is there no way to validate and fail before that process starts?

[bmaximuml]

@scholzj perhaps there's a different way to achieve what we're after? The purpose of this is to avoid having the DN's Organisational Unit and Organisation included in the ACL principal names. It looks like this is possible with a custom principal builder class, but we're keen to avoid having to recompile to receive updates. I understand the existing authentication methods for listeners are tls, scram-sha-512, oauth, and custom. Perhaps we could add another which performs this principal mapping? That way users still aren't given the option of fully customising the principal mapping rules.

[bmaximuml]

@scholzj any further thoughts on this?

[scholzj]

I don't think I have any new comments. I think the old comments still apply. This cannot be done in a special listener type because AFAIK it is cluster wide.

[omersiar]

I'm in favor of this option from scholzj

Make it configurable, but automatically inject our own rule into the first place. This could in general conflict with the user defined rules, but since we use our own organization (O=io.strimzi) in the certificate subjects, it is not likely.

Every organisation would require different expression for matching the said information, I think Strimzi is no other. Because Strimzi can still validate that:

^CN=[a-zA-Z0-9.-]*,O=io\.strimzi$

above regular expression matches with internally issued certificates, then we can have a rule that covers them:

RULE:^CN=[a-zA-Z0-9.-]*,O=io\.strimzi$/$1/,
DEFAULT

Every other rule that organisations may have be interested then would be only addition to this Strimzi specific rule without breaking backwards compatibility or intervening other rules

The other consideration should be the user certificates, since they only have Common Name, this also can be validated the same way

^[C][N]=([a-zA-Z0-9.-]*)$

This expression would only capture the ones that have only CN, rule would be

RULE:^[C][N]=([a-zA-Z0-9.-]*)$/CN=$1/,
DEFAULT

⬆️ This rule based on the examples shared here, modified to strictly looking for uppercase C and N, it can be optimised with a cheaper alternative.

Also I wanted to use below rule until finding out Strimzi is not allowing it

RULE:CN=([^,]+)/CN=$1/,DEFAULT

⬆️ This rule captures CN regardless of its position in the DN and captures all chars until to a comma.

In my case CN somehow was at the end of the string

C=NA,O=Some Org With Space,CN=reverse-ordered-user

Also another thing to consider is people definitely would include "DEFAULT" word in their rule set, which may be handled by the injection mechanism if this method chosen.

Here is a tool for testing (don't forget to select Java flavor)

https://regex101.com/r/VVcMpq/1

Here is my test list

CN=user.company.com,OU=unknown
CN=strimzi-user-entity-issued-user-without-org
CN=kafka-cluster-entity-topic-operator,O=io.strimzi
CN=user-with-a-long-dn,OU=Unknown,O=Unknown,L=Unknown,ST=Unknown,C=Unknown
CN=adminUser,OU=Admin,O=Unknown,L=Unknown,ST=Unknown,C=Unknown
CN=regularUser,OU=Devs,O=Unknown,L=Unknown,ST=Unknown,C=Unknown
C=NA,O=Some Org With Space,CN=reverse-ordered-user
CN=DN-with-spaces, OU=Devs, O=Unknown, L=Unknown, ST=Unknown, C=Unknown

Thank you.