Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add basic auth to Zeebe deployments #4269

Merged
merged 5 commits into from
May 8, 2024
Merged

Conversation

philippfromme
Copy link
Contributor

@philippfromme philippfromme commented May 6, 2024

electron_kMPMFQn9xa

  • adds basic auth to self-managed deployments
  • aligns naming of endpoint types and authentication types across codebase

Closes #4160

@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label May 6, 2024
@philippfromme philippfromme marked this pull request as draft May 6, 2024 08:45
@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels May 6, 2024
@philippfromme philippfromme marked this pull request as ready for review May 6, 2024 10:37
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels May 6, 2024
@nikku
Copy link
Member

nikku commented May 6, 2024

Looks good. How did you E2E-test your solution?

Copy link
Collaborator

@barmac barmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The credentials should be redacted from the log:

ERROR app:zeebe-api connection check failed {
  parameters: {
    endpoint: {
      type: 'selfHosted',
      authType: 'basic',
      url: 'http://localhost:8000',
      basicAuthUsername: 'abc',
      basicAuthPassword: 'def'
    }
  }
}

@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels May 6, 2024
@philippfromme
Copy link
Contributor Author

@nikku No success testing this end-to-end. I'm trying to get help from the Zeebe team. Otherwise I'd need your help.

@philippfromme
Copy link
Contributor Author

@barmac Done: b346b79

@nikku
Copy link
Member

nikku commented May 7, 2024

I was able to end-to-end test this using a standard reverse proxy setup, based off our zeebe-connection-test project, basic-auth branch.

Topology (test setup)

Camunda Modeler -> [NGINX; enforces basic auth] -> Zeebe

Test it yourself

You can use the following script to test it yourself ™️:

# Clone repository
git clone git@github.com:camunda/zeebe-connection-test.git
cd zeebe-connection-test
git checkout -b basic-auth

Proceed with the run instructions to spin up Zeebe.

Then to test with the Camunda Modeler specify self-managed, http://sub.example.com, basic auth and pass demo:demo as credentials.


Test results

  • Initially when no credentials are provided we pass undefined for both basic auth username and password; I suggest we rather pass '' (an empty string). Otherwise zeebe-node will assume no basic auth is set

  • When testing for errors we do not properly handle authentication failures. What I see is the standard "Cannot connect to Zeebe cluster" error, but no indication that this may be due to wrong or missing credentials.

    screenshot pwqbVl screenshot PmVY6q

Besides these two things it works; I can deploy successfully.

Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be followed-up, cf. #4269 (comment).

@philippfromme
Copy link
Contributor Author

philippfromme commented May 7, 2024

I'm not sure how to deal with the errors. After setting up a reverse proxy with basic auth the error I get when using the wrong credentials is

Error: 13 INTERNAL: Received RST_STREAM with code 0\n at callErrorFromStatus (C:\workspace\camunda\camunda-modeler\node_modules\@grpc\grpc-js\build\src\call.js:31:19)\n at Object.onReceiveStatus (C:\workspace\camunda\camunda-modeler\node_modules\@grpc\grpc-js\build\src\client.js:192:76)\n at C:\workspace\camunda\camunda-modeler\node_modules\@grpc\grpc-js\build\src\call-interface.js:78:35\n at Object.onReceiveStatus (C:\workspace\camunda\camunda-modeler\node_modules\zeebe-node\dist\lib\GrpcClient.js:97:36)\n at InterceptingListenerImpl.onReceiveStatus (C:\workspace\camunda\camunda-modeler\node_modules\@grpc\grpc-js\build\src\call-interface.js:73:23)\n at Object.onReceiveStatus (C:\workspace\camunda\camunda-modeler\node_modules\@grpc\grpc-js\build\src\client-interceptors.js:360:141)\n at Object.onReceiveStatus (C:\workspace\camunda\camunda-modeler\node_modules\@grpc\grpc-js\build\src\client-interceptors.js:323:181)\n at C:\workspace\camunda\camunda-modeler\node_modules… …

which is just a GRPC error but doesn't provide any hint that this might be related to wrong credentials.

I could just map code === 13 && authType === 'basic' to an unauthorized error but I suspect that this error can also occur unrelated to wrong credentials. 🤔

@philippfromme
Copy link
Contributor Author

@nikku @barmac We could decide to fix the error handling as a follow-up.

@nikku
Copy link
Member

nikku commented May 8, 2024

Agreed that it makes sense to maybe improve the error handling as a follow-up.

@philippfromme philippfromme merged commit 86747f8 into develop May 8, 2024
12 checks passed
@philippfromme philippfromme deleted the basic-auth branch May 8, 2024 09:07
@bpmn-io-tasks bpmn-io-tasks bot removed the in progress Currently worked on label May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support HTTP Basic authentication for C8 SM deployment
3 participants