strimzi / strimzi-kafka-oauth

OAuth2 support for Apache Kafka® to work with many OAuth2 authorization servers
Apache License 2.0
147 stars 90 forks source link

OAuth with KafkaUser and simple authorization #135

Open corlettb opened 2 years ago

corlettb commented 2 years ago

I'm trying to set up oauth for authentication (Azure AD) but use simple authorization.

I have the authenitcation/authorisation working. My issues are around linking the simple authorization through KafkaUser crds.

I realised you could set the KafkaUser without an authentication section and it would just create the permissions.

Sadly my issue comes down to the lack of a spec.kafkaUser setting like the way the KafkaTopic has. I understand there are reasons not to have this to avoid conflicts. However without this functionaltion it is causing me more issues.

  1. I need to run multiple clusters in the same namespace (sadly not up to me). This will create conflicts if I try to add the same user to multiple clusters.

  2. Oauth user unique_name in Azure AD come out as email addresses. So username@company.com. When trying to apply a KafkaUser with that name you get the classic:

failed to create resource: KafkaUser.kafka.strimzi.io "username@company.com" is invalid: metadata.name: Invalid value: "username@company.com": a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is 'a-z0-9?(.a-z0-9?)*')

I tried experimenting with the following settings: userNameClaim: unique_name fallbackUserNameClaim: sub fallbackUserNamePrefix: clustername-

This would mean jwk auth requests without a unique_name would avoid point 1. (e.g. username comes out as clustername-11111111-1111-1111-1111-111111111111), however not so much on point 2.

I tried forking with the following hack:

--- a/oauth-common/src/main/java/io/strimzi/kafka/oauth/common/PrincipalExtractor.java
+++ b/oauth-common/src/main/java/io/strimzi/kafka/oauth/common/PrincipalExtractor.java
@@ -28,7 +28,8 @@ public class PrincipalExtractor {
         if (usernameClaim != null) {
             result = getClaimFromJWT(json, usernameClaim);
             if (result != null) {
-                return result;
+                result = result.replaceAll("@.+$", "");
+                return fallbackUsernamePrefix + result;
             }

             if (fallbackUsernameClaim != null) {

Which made the usernames come out like clustername-username. Which I can make KafkaUsers from.

Although I think having a spec.KafkaUser would be a better solution do you think we could add a "substitution regex" option in the code and crds?

This would allow us to map our usernames to ones that we can create with KafkaUsers.

mstruk commented 2 years ago

Thank you for a thorough description of your problem.

Actually, could you open another issue in Strimzi Operator since it's an issue that also concerns KafkaUser CR, and a possibly more elegant solution could be achieved through changes there?

Looks like the substitution regex config option would make perfect sense.

scholzj commented 2 years ago

Although I think having a spec.KafkaUser would be a better solution do you think we could add a "substitution regex" option in the code and crds?

We do not plan to support any custom usernames in the KafkaUser resource. It adds a lot of complexity and causes problems because it means there is no unique identification for the user. Please keep in mind that you do not need to use the User Operator. You can just disable it (by removing the userOperator section in the Kafka CR) and managing the ACLs and Quotas using the Kafka APIs and use whatever username you prefer.

mstruk commented 2 years ago

We do not plan to support any custom usernames in the KafkaUser resource.

Ok, we can still add it in strimzi-kafka-oauth and expose extra config in Kafka CR oauth authentication section.

scholzj commented 2 years ago

Ok, we can still add it in strimzi-kafka-oauth and expose extra config in Kafka CR oauth authentication section.

It is road to hell as proved by the Topic Operator. As I said, the User Operator is optional. It might not be able to cover all usecases, but users don't have to use it.

mstruk commented 2 years ago

As I said, the User Operator is optional. It might not be able to cover all usecases, but users don't have to use it.

So in that case the username containing the '@' would not be a problem and the need for the substitution goes away.

corlettb commented 2 years ago

Thank you for your fast responses.

I would like to use strimzi for managing ACL's through kubernetes if I can rather than going directly to the kafka api.

Maybe another idea for a better fix. A KafkaAcl crd which is similar to the KafkaUser crd however it has a spec.KafkaUser option and no authentication option.

KafkaAcl will be applied as a union. So you could have one KafkaUser and 10 KafkaAcls for example. Same sort of pattern as terraform and aws_security_group and aws_security_group_rule.

Think of a scenario like you have a connect cluster but seperate deploys for your connectors. Each of the connectors need additional access to topics so would deploy their own KafkaAcl.

This would also mean for OAuth users they could just use KafkaAcls.

Granted you could get yourself into trouble if two KafkaAcls gives the same permission. You could dectect this in the code and throw out an error and refuse to do anything with the user until the conflict is resolved.

Failing that would you be okay with an oauth regex option?

Thoughts?

scholzj commented 2 years ago

But that causes the same problems in whatever operator processes the KafkaAcl resource. The key if you want to use the KafkaUser for it is to make sure your usernames are valid Kubernetes names. That is the right way to use it.

corlettb commented 2 years ago

Obviously oauth users are outside of kubernetes. Not something I can change. Would you be okay with adding an optional regex to the oauth options in the kafka crd then?

The regex can be used to transform the oauth user to a kubernetes friendly name.

scholzj commented 2 years ago

I do not have problem with that => but for the Oauth library, @mstruk is the one to consider it.

corlettb commented 2 years ago

There would need to be some changes in the strimzi operator. Add the option to the Kafka CRD and write it into the kafka server.properties file if the option is set. @mstruk is this solution something you would accept?

mstruk commented 2 years ago

That's the approach I had in mind when I said:

Ok, we can still add it in strimzi-kafka-oauth and expose extra config in Kafka CR oauth authentication section.

It is really just another principal name mapping facility in addition to existing fallbackXXX mechanisms. It didn't seem like something anyone would need or want at the time, but now there is a real use-case for it.

scholzj commented 2 years ago

That's the approach I had in mind when I said: ...

Ahh, sorry. I misunderstood that.