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

support client_credentials with client_assertion #167

Closed robbertvanwaveren closed 1 year ago

robbertvanwaveren commented 2 years ago

PR for issue #164

All builds and tests are working.

I added the following abilities:

  1. support for client assertion (private_key_jwt) based authentication on the token endpoint as per https://www.rfc-editor.org/rfc/rfc7523#section-2.2
  2. support for acquiring token, refresh token, client assertion (token) from filesystem on-demand (to support refreshment by an external process such as Azure AD Workload Identity).
  3. support for acquiring config values based on references to environment variables (to support pointing to externally injected environment variables such as those from Azure AD Workload Identity)

I was not yet able to test the changing token in real scenario as part of a re-authentication request. So the fact that a re-authentication re-triggers JaasClientOauthLoginCallbackHandler.handle is an untested assumption at this point.

robbertvanwaveren commented 2 years ago

This PR has been tested in a real deployment and hourly re-authentication is working as expected.

mstruk commented 2 years ago

@robbertvanwaveren Thanks for the PR. Could you also add some documentation in README.md to explain the circumstances when one would want to use this, and how the workflow works in such a case?

I also had one problem running the tests. See the proposed fix.

robbertvanwaveren commented 2 years ago

I've made a few suggested additions.

robbertvanwaveren commented 2 years ago

@mstruk Do you require anything further from my side?

mstruk commented 2 years ago

@robbertvanwaveren Not at the moment. This PR still has to be thoroughly reviewed.

Currently I'm busy doing the 0.11.0 release, and integrating the functionality into the main Strimzi Project. I'll get back to this when I find the time.

scholzj commented 1 year ago

@mstruk What is the plan for this?

mstruk commented 1 year ago

@scholzj The plan is to go back to this very soon now.

mstruk commented 1 year ago

@robbertvanwaveren I'm looking at this issue again. There's been a lot of changes by other PRs resulting in conflicts. I can merge the changes in some branch, and you can hard reset your branch from there so we can then continue this PR and get it merged, if that's ok with you.

mstruk commented 1 year ago

@robbertvanwaveren I merged the changes here: https://github.com/mstruk/strimzi-kafka-oauth/commits/oauth-client-assertion-support

Feel free to hard reset your branch to https://github.com/mstruk/strimzi-kafka-oauth/commit/d280c414b6aae0565dbdd2fcba8c8ea6ef2f17ad

scholzj commented 1 year ago

@mstruk Did you managed to make any progress around this? Any ideas what to do with it?

mstruk commented 1 year ago

@robbertvanwaveren Are you still interested in getting this PR merged?

If yes, we can get it merged quickly. I can prepare another rebase to which you just hard reset your PR and we can merge it.

scholzj commented 1 year ago

Discussed on the Community cal of 19.10.2023: There was no reply for a long time. This PR should be closed. If there is some renewed interest later, we can reopen it.

shinji commented 1 year ago

This PR would allow clients to use OpenID Connect with federated identity via client assertion issued by another IdP to get a token. Kubernetes workloads in Azure cloud provider are migrating to this authentication model. It would be great to reopen the review of this PR. Best regards.

https://curity.io/resources/learn/jwt-assertion/ https://www.rfc-editor.org/rfc/rfc7523 https://www.rfc-editor.org/rfc/rfc7521

scholzj commented 1 year ago

@shinji The PR has stalled and without the DCO sign-off there is not much we can do with it. We either need the DCO sign-off to be fixed and then the existing code here might be used. Or it needs to be implemented from scratch.

robbertvanwaveren commented 1 year ago

Any specific action required from me?

scholzj commented 1 year ago

If you could at least fix the DCO signoff, it would be possible to re-use the code. The instructions should be here (assuming the link works): https://github.com/strimzi/strimzi-kafka-oauth/runs/8930450983

mstruk commented 1 year ago

Any specific action required from me?

@robbertvanwaveren You can just use your oauth-client-assertion-support branch and run the following:

git rebase HEAD~3 --signoff
git push --force-with-lease origin oauth-client-assertion-support

I will then do the rebase, because there are many conflicts at this point due to other PRs that were merged in the mean time.

You can read more about DCO here: https://github.com/apps/dco

Another thing you can do is enable commits into your branch as described here which will allow me to also merge the PR. Otherwise I'll have to ask you to hard reset your PR to my rebased branch and force push it again before we can merge.

Thanks.

robbertvanwaveren commented 1 year ago

I had to create a new PR to grant rights: https://github.com/strimzi/strimzi-kafka-oauth/pull/211