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

Application Integration: OIDC endpoints #869

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

FerdinandvHagen
Copy link
Contributor

Description

This PR implements OIDC provider endpoints for Hanko using the zitadel/oidc package, briefly also described in this discussion post. Applications can then be integrated with Hanko as their identity provider using whatever standard OAuth 2.0 / OIDC client the language provides.

The PR could serve as an alternative integration pattern for applications instead of the currently used JWT based cookie / Header authentication. Because OAuth 2.0 / OIDC is a fairly widespread and standardized protocol - and most languages implement OAuth 2.0 / OIDC clients - integration of Hanko and therefore adoption would be much easier than implementing a custom JWT system.

References:

Implementation

Used the excellent zitadel/oidc package that handles all OIDC related tasks. Tried to keep the storage backend as close to the current Hanko implementation as possible.

There is one issue in the current setup that Hanko doesn't provide automated redirects on successful login, so some Client side work is needed to get this working properly.

Besides that it should fit quite well with the existing Hanko program flow.

Tests

Currently only the happy path is tested. OIDC implementation itself is tested in the underlying package.
Planning on adding more tests if/when there is a general consensus if such implementation would get merged or not.

Todos

  • Is this generally interesting - or do you have other plans regarding session management?
  • What needs to be done in order to get this merged? Are there any parts of the current implementation that need larger changes?

Additional context

This PR is in DRAFT stage and the implementation is not finished. I requested it to get an early 👍 👎 if there is a chance this lands in Hanko or not - and understand where I need to adhere better to Hanko principles.

Implements the core storage interface for the Zitadel OIDC implementation.
Intended to be used with applications that use Hanko for authentication.
This is NOT a generic OIDC implementation for login to Hanko.
First full runthrough is implemented as a test. Most functionality should work now.
Major errors are eliminated.

var opKeys []op.Key
for _, key := range keys {
opKeys = append(opKeys, &key)
Copy link

Choose a reason for hiding this comment

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

39% of developers fix this issue

G601: Implicit memory aliasing in for loop.


ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


storage := oidc.NewStorage(persister)
for _, client := range cfg.OIDC.Clients {
err := storage.AddClient(&client)
Copy link

Choose a reason for hiding this comment

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

39% of developers fix this issue

G601: Implicit memory aliasing in for loop.


ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.

@like-a-bause
Copy link
Contributor

Hi Ferdinand,
thanks for your contribution and sorry we didn't answer earlier on the discussion.
We talked about this and think that right now we don't want to support OIDC Provider capabilities with hanko.
Let me elaborate:
In a previous iteration of hanko(not open source) we did integrate OIDC and while it worked it isn't the best solution for what we are trying to solve. I'd like to refer to this excellent article written at ory: https://www.ory.sh/oauth2-openid-connect-do-you-need-use-cases-examples

We are trying to solve Authentication for first party applications and one of our core values is developer experience. With OIDC this would suffer a lot. Not only do you as a user need to do the whole redirect, save state, callback dance there is also the issue of styling. The Login screen and components would suddenly have to be hosted at the hanko-backend and not in your application.

On top of that we currently do not have the capacities to overview and maintain the huge code surface OIDC/OAuth would put on our shoulders.

We are currently looking into a session like system offered as an alternative to the stateless JWT session we currently have and think that this will be the way to go in the future.

Regardless, thanks for your effort. If you have any more input on that matter feel free to respond.

@FerdinandvHagen
Copy link
Contributor Author

Thank you @like-a-bause for the reply! I fully understand the reasoning behind it.

Do you happen to have some more information on the session system you are planning? I'm expecting you will introduce a custom endpoint to validate the (session) token against and then just pass the (session) token via Cookie or Header?

@like-a-bause
Copy link
Contributor

We recently had a talk about this and imagined a somehow hybrid approach: Give out a server stored session/refresh token and a short lived JWT session. This way relaying-party backend implementations don't have to do anything but we will get the benefit of being able to cancel a session server side.

@ghstahl
Copy link

ghstahl commented Feb 5, 2024

Can we move this to a discussion?

For my downstream systems to work I need id_tokens. Thats why I like OIDC. I don't need care about the access_tokens hanko may generate as long as I get my id_token. I don't care about sessions, etc.

Just proof of life and how that was achieved.

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.

3 participants