Use Authentication for ZeebeClient #559

stephanpelikan · 2023-12-14T16:24:37Z

As mentioned in #558 we want to use spring-zeebe to connect to a self-managed cluster having tenants enabled. This also requires to enable Identity based authentication (see https://github.com/camunda/camunda-platform#multi-tenancy).

I used the current main-branch (17e1fc4) to test this, but failed due to two lacks in implementation:

At

spring-zeebe/spring-boot-starter-camunda/src/main/java/io/camunda/zeebe/spring/client/configuration/CommonClientConfiguration.java

Line 41 in 17e1fc4

if (zeebeClientConfigurationProperties.getCloud() != null) {

it is tested for getCloud() != null but there is always an object given.
At

spring-zeebe/spring-boot-starter-camunda/src/main/java/io/camunda/zeebe/spring/client/properties/ZeebeClientConfigurationProperties.java

Line 766 in 17e1fc4

if (hasText(cloud.clientId) && hasText(cloud.clientSecret)) {

the new authentication classes (like

spring-zeebe/camunda-sdk-java/java-common/src/main/java/io/camunda/common/auth/SelfManagedAuthentication.java

Line 23 in 17e1fc4

public class SelfManagedAuthentication extends JwtAuthentication {

) are not used for Zeebe-client authentication.

In this pull-request I resolved this by

Fixing CommonClientConfiguration
Detaching io.camunda.zeebe.client.ZeebeClientConfiguration and ZeebeClientConfigurationProperties
Using any other authentication than DefaultNoopAuthentication as CredentialsProvider for the ZeebeClient

With this in place I'm able to start self-managed C8 and use it with tenants enabled:

local C8 started like this: https://github.com/camunda/camunda-platform#multi-tenancy
Configured Zeebe-API to allow default-tenant
Use Spring config:
application.yaml:

logging:
  level:
    io.camunda.zeebe.client.impl.ZeebeCallCredentials: ERROR

zeebe:
  client:
    broker:
      gateway-address: 127.0.0.1:26500
    security:
       plaintext: true

common:
  keycloak:
    url: http://localhost:18080
    realm: camunda-platform
  client-id: zeebe
  client-secret: zecret

1nb0und

Thank you for the PR! A couple of thoughts

I like where you separated the ZeebeConfig from the ZeebeProperties. It makes it consistent with the rest of the clients as well.
The way you defined the application yaml is concise and is inlined to the goal of single Authentication across multiple modules thru the common. prefix

I'm good to merge this PR and I get the intent. But I have the following concerns:

We would need to preserve compatibility with the old way of authenticating with zeebe (i.e. without the common.). Not sure if its preserved but can you still do Self Managed Authentication without utilizing the common. properties?

zeebe:
  client:
    client-id: zeebe
    client-secret: zecret
    broker:
      gateway-address: 127.0.0.1:26500
    security:
       plaintext: true

Overall we would need to sort out the Authentication piece which is moving towards the upcoming Identity SDK. This could merge (with some clarifications), but knowing that its coming soon, most of the authentication refactoring here would either be no longer applicable.

A recap:

Fixing CommonClientConfiguration

looks good!

Detaching io.camunda.zeebe.client.ZeebeClientConfiguration and ZeebeClientConfigurationProperties

this looks good too!

Using any other authentication than DefaultNoopAuthentication as CredentialsProvider for the ZeebeClient

need to look at this closely given this will change with upcoming Identity SDK

Let me know your thoughts.

1nb0und · 2023-12-14T22:47:00Z

...nda/src/main/java/io/camunda/zeebe/spring/client/configuration/ZeebeClientConfiguration.java

+    if (!(authentication instanceof DefaultNoopAuthentication)) {
+      return new CredentialsProvider() {
+        @Override
+        public void applyCredentials(Metadata headers) throws IOException {
+          final var authHeader = authentication.getTokenHeader(Product.ZEEBE);
+          final var authHeaderKey = Metadata.Key.of(authHeader.getKey(), Metadata.ASCII_STRING_MARSHALLER);
+          headers.put(authHeaderKey, authHeader.getValue());
+        }
+
+        @Override
+        public boolean shouldRetryRequest(Throwable throwable) {
+          return ((StatusRuntimeException) throwable).getStatus() == Status.DEADLINE_EXCEEDED;
+        }
+      };
+    }


Will your use-case still work without relying on any recent Authentication that I created? I think you should still be able to use tenancy with Zeebe still using its own CredentialsProvider as oppose to the Authentication wrapper.

I get your intent, and this is how it should be done. But few things I'd like to share:

Sooner of later, the entire Authentication piece in Spring SDK will be replaced with Identity SDK. The commons.auth module was created with the same goals in mind as the Identity SDK which is to unify authentication across all products. However, the Identity SDK will be used not only in Spring SDK, but also in other core products as well so it will make sense to ditch the custom Authentication module here and use that.

We need to set a custom auth-url (KeyCloak) for the Zeebe connection which seems not to be implemented yet in 8.3.x what is to reason for introducing this wrapper. I simply tried to reuse what is prepared in SelfManageAuthentication since there is the Product.ZEEBE enum. I thought this is already prepared to be used in the near future, so I took this right now to get our scenario working. I didn't recognize what's the target picture for the authentication stuff added recently.

1nb0und · 2023-12-14T22:57:20Z

...nda/src/main/java/io/camunda/zeebe/spring/client/configuration/ZeebeClientConfiguration.java

+    if (hasText(properties.getCloud().getClientId()) && hasText(properties.getCloud().getClientSecret())) {
+//        log.debug("Client ID and secret are configured. Creating OAuthCredientialsProvider with: {}", this);
+      return CredentialsProvider.newCredentialsProviderBuilder()
+        .clientId(properties.getCloud().getClientId())
+        .clientSecret(properties.getCloud().getClientSecret())
+        .audience(properties.getCloud().getAudience())
+        .scope(properties.getCloud().getScope())
+        .authorizationServerUrl(properties.getCloud().getAuthUrl())
+        .credentialsCachePath(properties.getCloud().getCredentialsCachePath())
+        .build();
+    }
+    if (Environment.system().get("ZEEBE_CLIENT_ID") != null && Environment.system().get("ZEEBE_CLIENT_SECRET") != null) {
+      // Copied from ZeebeClientBuilderImpl
+      OAuthCredentialsProviderBuilder builder = CredentialsProvider.newCredentialsProviderBuilder();
+      int separatorIndex = properties.getBroker().getGatewayAddress().lastIndexOf(58); //":"
+      if (separatorIndex > 0) {
+        builder.audience(properties.getBroker().getGatewayAddress().substring(0, separatorIndex));
+      }
+      return builder.build();
+    }


At what instance can the user still use the old way of Authenticating with Zeebe? If this gets merged in main here is my concern:

In coming version of Zeebe 8.4, there will be an optional field scope for SelfManaged users (see this PR) and right now its not integrated with the SelfManageAuthentication in the commons.auth module

You may refactor this as you like having the target of the new authentications introduced in your mind. I was not my goal to keep the old way of authentication. I just wanted to add the bits necessary to make it work in our situation. I would appreciate if you could accept this for now because we need to go ahead with our Camunda based project. Of course I'm willing to support you, but our primary goal is to deliver our business software in time.

@stephanpelikan I'm happy to work as well with you and really value your contributions. Looking forward to great collaborations ahead!

@stephanpelikan 8.3.4.2 and 8.4.0-alpha2-rc3 has been released and should be publicly available shortly

stephanpelikan · 2023-12-18T07:36:32Z

I rebased my branch due to a conflict in CommonClientConfiguration.java caused by this PR #563. I dropped my changes in favor to the changes of the PR request since they also include the fix I did and I guess it is likely to see more changes until my PR is merged. The rest of the changes is still necessary.

.../main/java/io/camunda/zeebe/spring/client/properties/ZeebeClientConfigurationProperties.java

1nb0und · 2023-12-18T17:47:59Z

...nda/src/main/java/io/camunda/zeebe/spring/client/configuration/ZeebeClientConfiguration.java

+
+  @Override
+  public CredentialsProvider getCredentialsProvider() {
+    if (!(authentication instanceof DefaultNoopAuthentication)) {


I appreciate reverting the change. I'm happy to accept the PR, but would need to correct this condition in subsequent PR so that users will not be stuck to always use override credentials headers in Zeebe. As it stands, there will be always Authentication object and it will not be DefaultNoopAuthentication provided they set the right properties.

My temporary plan / work-around would be to change the condition to check for flag common.enabled and then use this functionality. I'll make a release so we can have something usable for you but not breaking for others.

Expect this to be change substantially after that.

Thanks again!

1nb0und · 2023-12-18T19:07:48Z

...nda/src/main/java/io/camunda/zeebe/spring/client/configuration/ZeebeClientConfiguration.java

+          final var authHeader = authentication.getTokenHeader(Product.ZEEBE);
+          final var authHeaderKey = Metadata.Key.of(authHeader.getKey(), Metadata.ASCII_STRING_MARSHALLER);


I'm curious, what is this var keyword? It sounds like a Kotlin syntax but it works on my end with plain Java.

I see now, it sounds like it was introduced in Java 10. Will probably refactor or replace this for consistency

stephanpelikan · 2023-12-21T11:40:27Z

Thank you for finalizing (#567), merging and releasing!

1nb0und added this to the 8.4.0 milestone Dec 14, 2023

1nb0und added self-managed enhancement labels Dec 14, 2023

1nb0und reviewed Dec 14, 2023

View reviewed changes

Use Authentication for ZeebeClient

8fdd81d

stephanpelikan force-pushed the feature/self-managed-cluster-authentication branch from e9335b2 to 8fdd81d Compare December 18, 2023 06:57

1nb0und reviewed Dec 18, 2023

View reviewed changes

.../main/java/io/camunda/zeebe/spring/client/properties/ZeebeClientConfigurationProperties.java Show resolved Hide resolved

1nb0und approved these changes Dec 18, 2023

View reviewed changes

1nb0und merged commit 93fb474 into camunda-community-hub:main Dec 18, 2023
3 checks passed

1nb0und mentioned this pull request Dec 18, 2023

feat: adding correct condition to avoid being stuck on zeebe header auth #567

Merged

1nb0und reviewed Dec 18, 2023

View reviewed changes

nicpuppa mentioned this pull request May 7, 2024

Update authentication flow camunda/camunda#17886

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Authentication for ZeebeClient #559

Use Authentication for ZeebeClient #559

stephanpelikan commented Dec 14, 2023

1nb0und left a comment

1nb0und Dec 14, 2023

stephanpelikan Dec 15, 2023

1nb0und Dec 14, 2023

stephanpelikan Dec 15, 2023

1nb0und Dec 18, 2023

1nb0und Dec 18, 2023

stephanpelikan commented Dec 18, 2023

1nb0und Dec 18, 2023 •

edited

Loading

1nb0und Dec 18, 2023

1nb0und Dec 18, 2023

stephanpelikan commented Dec 21, 2023

		final var authHeader = authentication.getTokenHeader(Product.ZEEBE);
		final var authHeaderKey = Metadata.Key.of(authHeader.getKey(), Metadata.ASCII_STRING_MARSHALLER);

Use Authentication for ZeebeClient #559

Use Authentication for ZeebeClient #559

Conversation

stephanpelikan commented Dec 14, 2023

1nb0und 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

stephanpelikan commented Dec 18, 2023

1nb0und Dec 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephanpelikan commented Dec 21, 2023

1nb0und Dec 18, 2023 •

edited

Loading