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

[ENHANCEMENT]: Some improvements around OIDC/OAuth provider configuration #1685

Conversation

celian-garcia
Copy link
Member

@celian-garcia celian-garcia commented Jan 2, 2024

Description

This PR comes after having tested several external OIDC provider.

So far I tested Azure, Google, Gitlab, Linkedin as OIDC providers.
This last one seems to not support all the features like the others.

That's why this PR allow more fine grain in configuration:

discovery_url config

Set a custom Discovery URL (the famous "/.well-known/openid-configuration" where all the config can be accessed)

With linkedin the problem is that the lib we use expected the discovery url to be {issuer}/.well-known/openid-configuration, which is not the case

// 20240102161055
// https://www.linkedin.com/oauth/.well-known/openid-configuration

{
  "issuer": "https://www.linkedin.com",

PKCE check

By default my first implementatio assumed that PKCE verification is supported by all the OIDC providers, which is not the case.
So we can now disable it through disable_pkce configuration.

More efficient user info parsing

In anticipation, I also include in that PR an interface and a service that will have to be implemented to upsert user at login time (see userinfo.go file)

Important Note: the left part of the email is now used to generate username, and I am planning in a first place to forbid the login with same username and different providers. (this is a best effort solution and will have to be rethink later)
It is also still subject to change in the following PRs when we'll do the sync.
image

Screenshots

Checklist

  • Pull request has a descriptive title and context useful to a reviewer.
  • Pull request title follows the [<catalog_entry>] <commit message> naming convention using one of the
    following catalog_entry values: FEATURE, ENHANCEMENT, BUGFIX, BREAKINGCHANGE, DOC,IGNORE.
  • All commits have DCO signoffs.

UI Changes

  • Changes that impact the UI include screenshots and/or screencasts of the relevant changes.
  • Code follows the UI guidelines.
  • Visual tests are stable and unlikely to be flaky.
    See Storybook
    and e2e docs for more details. Common issues
    include:
    • Is the data inconsistent? You need to mock API requests.
    • Does the time change? You need to use consistent time values or mock time utilities.
    • Does it have loading states? You need to wait for loading to complete.

@celian-garcia celian-garcia changed the title [FEATURE]: OIDC provider's discovery_url configurable [FEATURE]: Some improvements around OIDC provider configuration Jan 2, 2024
@Nexucis Nexucis changed the title [FEATURE]: Some improvements around OIDC provider configuration [ENHANCEMENT]: Some improvements around OIDC provider configuration Jan 3, 2024
var errToken error
if token != nil {
// TODO(cegarcia): Is it really necessary?, claims that we are searching for are id_token claims,
// and if there is an id token, then it is OIDC
Copy link
Member Author

Choose a reason for hiding this comment

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

Putting back into draft mode in order to resolve this comment.
I will try more oauth providers as for now only github has been tried and doesn't have jwt access_token neither id_token.
I have a good hope that this token parsing is actually not necessary at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

I made the choice to not parse the token here and rely only on API /userinfos data of the provider to retrieve needful information. This will be acceptable as first shot and we'll see with usages if we need to improve.

Anyway, with generic oauth, access_token is not guaranteed to be a jwt token neither to contain the right claims at the right place. A true id_token would guarantee that but if we receive id_token, then it is OIDC.

From what I see in the web, the mainstream will be more and more OIDC and this is what we should push for as much as possible.

Signed-off-by: Celian GARCIA <celian.garcia@amadeus.com>
…default)

Signed-off-by: Celian GARCIA <celian.garcia@amadeus.com>
Signed-off-by: Celian GARCIA <celian.garcia@amadeus.com>
Signed-off-by: Celian GARCIA <celian.garcia@amadeus.com>
Signed-off-by: Celian GARCIA <celian.garcia@amadeus.com>
Signed-off-by: Celian GARCIA <celian.garcia@amadeus.com>
@celian-garcia celian-garcia changed the title [ENHANCEMENT]: Some improvements around OIDC provider configuration [ENHANCEMENT]: Some improvements around OIDC/OAuth provider configuration Jan 5, 2024
@celian-garcia celian-garcia self-assigned this Jan 5, 2024
Signed-off-by: Celian GARCIA <celian.garcia@amadeus.com>
@celian-garcia celian-garcia marked this pull request as ready for review January 5, 2024 14:04
pkg/model/api/v1/common/url.go Outdated Show resolved Hide resolved
pkg/model/api/config/authentication_test.go Show resolved Hide resolved
internal/api/impl/auth/oidc.go Outdated Show resolved Hide resolved
internal/api/impl/auth/userinfo.go Outdated Show resolved Hide resolved
Signed-off-by: Celian GARCIA <celian.garcia@amadeus.com>
…eds and 500 otherwise)

Signed-off-by: Celian GARCIA <celian.garcia@amadeus.com>
Copy link
Member

@Nexucis Nexucis left a comment

Choose a reason for hiding this comment

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

excepting my last comment, lgtm !

Signed-off-by: Celian GARCIA <celian.garcia@amadeus.com>
@celian-garcia celian-garcia merged commit 67a5f36 into perses:feat/social-authentication Jan 9, 2024
18 checks passed
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

2 participants