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

support client_credentials with client_assertion #167

Conversation

robbertvanwaveren
Copy link
Contributor

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.

@scholzj scholzj requested a review from mstruk October 13, 2022 08:56
@robbertvanwaveren
Copy link
Contributor Author

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

@mstruk
Copy link
Contributor

mstruk commented Oct 17, 2022

@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 and others added 2 commits October 17, 2022 10:34
…ProviderTest.java

Co-authored-by: Marko Strukelj <marko.strukelj@gmail.com>
Signed-off-by: Robbert van Waveren <robbert1@gmail.com>
Signed-off-by: Robbert van Waveren <robbert.van.waveren@alliander.com>
@robbertvanwaveren robbertvanwaveren force-pushed the oauth-client-assertion-support branch from b037f47 to 31d8670 Compare October 17, 2022 09:49
@robbertvanwaveren
Copy link
Contributor Author

I've made a few suggested additions.

@robbertvanwaveren
Copy link
Contributor Author

@mstruk Do you require anything further from my side?

@mstruk
Copy link
Contributor

mstruk commented Oct 24, 2022

@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
Copy link
Member

scholzj commented Mar 9, 2023

@mstruk What is the plan for this?

@mstruk
Copy link
Contributor

mstruk commented Mar 13, 2023

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

@mstruk
Copy link
Contributor

mstruk commented Jul 14, 2023

@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
Copy link
Contributor

mstruk commented Jul 25, 2023

@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 mstruk@d280c41

@scholzj
Copy link
Member

scholzj commented Sep 21, 2023

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

@mstruk
Copy link
Contributor

mstruk commented Sep 21, 2023

@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
Copy link
Member

scholzj commented Oct 19, 2023

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.

@scholzj scholzj closed this Oct 19, 2023
@shinji
Copy link

shinji commented Oct 25, 2023

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
Copy link
Member

scholzj commented Oct 26, 2023

@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
Copy link
Contributor Author

Any specific action required from me?

@scholzj
Copy link
Member

scholzj commented Oct 27, 2023

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
Copy link
Contributor

mstruk commented Oct 30, 2023

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
Copy link
Contributor Author

I had to create a new PR to grant rights: #211

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.

4 participants