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

KEYCLOAK-5811 OIDC Client Authentication by JWS Client Assertion in client_secret_jwt #4835

Closed
wants to merge 5 commits into from
Closed

KEYCLOAK-5811 OIDC Client Authentication by JWS Client Assertion in client_secret_jwt #4835

wants to merge 5 commits into from

Conversation

tnorimat
Copy link
Contributor

@tnorimat tnorimat commented Dec 11, 2017

I've implemented JWS Client Assertion On Client Authentication in "client_secret_jwt" on both authorization server and client adapter (keycloak-adapter-core) side.

JIRA issue is as follows.
https://issues.jboss.org/browse/KEYCLOAK-5811

And I've also wrote and executed Arquillian integration tests for both authorization server and client adapter (keycloak-adapter-core) side.

I recognized that it also needs documentation on the manual. I'd like to do it afterwards if this PR be accepted.

here is the brief summary of its specification.

[Concept]

  • Add new Authenticator "Signed Jwt with Client Secret".
  • Use client credential string the same as in the Authenticator "Client Id and Secret".
  • Use existing implementation for JWT Client Assertion.
    • RFC 7521 Assertion Framework for OAuth 2.0 Client Authentication and Authorization Grants
      Section: 4.2. Using Assertions for Client Authentication
    • RFC 7523 JSON Web Token (JWT) Profile for OAuth 2.0 Client Authentication and Authorization Grants
      Section: 2.2. Using JWTs for Client Authentication
  • Use existing implementation for JWS in HMAC SHA-256.

[Authorization Server Realm Representation]

 "clientAuthenticatorType": "client-secret-jwt",
 "secret": <client credential string>

[Client Adapter Representation(keycloak.json)]

  "credentials": {
    "secret-jwt": {
      "secret": <client credential string>
    }
  }

[Following Standard]
URL: http://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication
Title: OpenID Connect Core 1.0 incorporating errata set 1
Section: 9. Client Authentication
Part: client_secret_jwt

@stianst stianst added the 4.x label Dec 14, 2017
@mposolda mposolda self-assigned this Jan 11, 2018
@mposolda
Copy link
Contributor

@tnorimat Thanks for the PR. Great work! Just few things:

  • Is it possible to move testrealm-jwt-client-secret.json file under directory "client-auth-test" ?
  • I wonder why new tests need to be ignored in AbstractDemoFilterServletAdapterTest.java . IMO the new client authentication method should work with OIDC Filter adapter too? Any specific issue with the OIDC Filter adapter?

@tnorimat
Copy link
Contributor Author

Thank you for your comments.

I've moved testrealm-jwt-client-secret.json file under directory "client-auth-test".

And as you said, I've revised test codes for client adapters so that OIDC Filter adapter also executes this tests.

@tnorimat
Copy link
Contributor Author

Due to my other pull request (#4719), the unit test failed.
Solved this problem by removing test codes for checking default PKCE setting of client adapter(keycloak.json).

@mposolda
Copy link
Contributor

Great work, Thanks! Few last things:

@tnorimat
Copy link
Contributor Author

tnorimat commented Jan 23, 2018

I've tried to squash 5 commits onto one commit by rebase and squash but failed.
Do you have any other good idea?

Aside from that, I'm now preparing documentation.

@tnorimat
Copy link
Contributor Author

The PR for documentation is keycloak/keycloak-documentation#297

@mposolda
Copy link
Contributor

@tnorimat For squash commits, I am using command (number 5 refers to your 5 commits):
git rebase -i HEAD~5
then keep just first line text on "pick" and change the other 4 lines to "squash" instead of "pick" . Then change the commit message. Some more details (See the first answer with 1029 votes): https://stackoverflow.com/questions/5189560/squash-my-last-x-commits-together-using-git

Thanks for the docs! Going to take a look.

@tnorimat
Copy link
Contributor Author

Thank you for your advice. I have followed what you had said but failed.

I'm afraid that the reason why rebase failed is due to merging master branch into my topic branch on 2b11af3 .

At that time, I've resolved the conflicts and merged on github so that this PR includs several commits other than my 5 commits (131 in total)

It might be resolved if you close this PR and I newly issue PR by squashed one commit.

Would you have any other good idea?

@mposolda
Copy link
Contributor

@tnorimat Yes, I see. That should work if you prefer it. That's why I like "rebase" much more than "merge" . So closing this now.

BTV. Very useful command is also "git cherry-pick" . You can checkout latest master, then cherry-pick the commits from this your branch (excluding merge commits) and finally squash the cherry-picked commits into single one.

Good luck.

@mposolda mposolda closed this Jan 25, 2018
@mposolda
Copy link
Contributor

BTV. your documentation PR is fine and approved. So squashing commits here is only remaining thing.

@tnorimat
Copy link
Contributor Author

Thank you for your kindly advice. I'll try again.

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.

None yet

3 participants