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

WIP: Allow roles to be specified for Keycloak #767

Closed
wants to merge 19 commits into from

Conversation

khawaga
Copy link

@khawaga khawaga commented Sep 8, 2020

Description

This PR adds the ability to specify specific roles for use with Keycloak, which offers more granular control compared to groups.

Motivation and Context

The motivation behind this feature was that a similar project, Louketo Proxy, has reached EOL, and that project had the ability to specify roles which was important for several applications. OAuth2 Proxy would be a suitable replacement if it were to offer that same feature.

How Has This Been Tested?

The environment consisted of a Kubernetes cluster with a live Keycloak server.
I have tested the following:

  • Not providing the new keycloak-roles flag to ensure that previous functionality is maintained.
  • Providing a role that my test user did not have. User was not granted access.
  • Providing a realm role that my test user had. User was granted access.
  • Providing a list consisting of two roles, one of which my test user did not have. User was not granted access. This is the desired behaviour as all roles need to exist in order for a user to be granted access.
  • Providing a client role that my test user had. User was granted access.

Checklist:

  • My change requires a change to the documentation or CHANGELOG.
  • I have updated the documentation/CHANGELOG accordingly.
  • I have created a feature (non-master) branch for my PR.

@khawaga khawaga requested a review from a team as a code owner September 8, 2020 09:15
@khawaga khawaga force-pushed the add-keycloak-roles branch 2 times, most recently from b1f11d2 to 148c308 Compare September 8, 2020 09:37
@@ -24,7 +24,7 @@ _oauth2_proxy() {
COMPREPLY=( $(compgen -W 'X-Real-IP X-Forwarded-For X-ProxyUser-IP' -- ${cur}) )
return 0
;;
--@(http-address|https-address|redirect-url|upstream|basic-auth-password|skip-auth-regex|flush-interval|extra-jwt-issuers|email-domain|whitelist-domain|trusted-ip|keycloak-group|azure-tenant|bitbucket-team|bitbucket-repository|github-org|github-team|github-repo|github-token|gitlab-group|github-user|google-group|google-admin-email|google-service-account-json|client-id|client_secret|banner|footer|proxy-prefix|ping-path|cookie-name|cookie-secret|cookie-domain|cookie-path|cookie-expire|cookie-refresh|cookie-samesite|redist-sentinel-master-name|redist-sentinel-connection-urls|redist-cluster-connection-urls|logging-max-size|logging-max-age|logging-max-backups|standard-logging-format|request-logging-format|exclude-logging-paths|auth-logging-format|oidc-issuer-url|oidc-jwks-url|login-url|redeem-url|profile-url|resource|validate-url|scope|approval-prompt|signature-key|acr-values|jwt-key|pubjwk-url))
--@(http-address|https-address|redirect-url|upstream|basic-auth-password|skip-auth-regex|flush-interval|extra-jwt-issuers|email-domain|whitelist-domain|trusted-ip|keycloak-group|keycloak-roles|azure-tenant|bitbucket-team|bitbucket-repository|github-org|github-team|github-repo|github-token|gitlab-group|github-user|google-group|google-admin-email|google-service-account-json|client-id|client_secret|banner|footer|proxy-prefix|ping-path|cookie-name|cookie-secret|cookie-domain|cookie-path|cookie-expire|cookie-refresh|cookie-samesite|redist-sentinel-master-name|redist-sentinel-connection-urls|redist-cluster-connection-urls|logging-max-size|logging-max-age|logging-max-backups|standard-logging-format|request-logging-format|exclude-logging-paths|auth-logging-format|oidc-issuer-url|oidc-jwks-url|login-url|redeem-url|profile-url|resource|validate-url|scope|approval-prompt|signature-key|acr-values|jwt-key|pubjwk-url))
Copy link
Member

Choose a reason for hiding this comment

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

Ha - I didn't even realize this was a thing. I bet this way out of date, I've seen many PRs adding flags that didn't touch this 🤣

Copy link
Author

Choose a reason for hiding this comment

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

I did not know that 😂, I just did a search for one of the existing flags and that's how I found this! :)

@@ -95,5 +128,29 @@ func (p *KeycloakProvider) GetEmailAddress(ctx context.Context, s *sessions.Sess
}
}

if len(p.Roles) > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Can you refactor this into another method that does the role authorization? This addition makes this GetEmailAddress method too complex: https://codeclimate.com/github/oauth2-proxy/oauth2-proxy/pull/767

Copy link
Author

@khawaga khawaga Sep 8, 2020

Choose a reason for hiding this comment

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

Sure thing, but I believe after refactoring this block into its own method it would still exceed the number of allowed returns (it would become 1 less, leaving it at 5, which still exceeds the 4 allowed) so that would not fix the issue. Would you still suggest to refactor it?

Copy link
Member

Choose a reason for hiding this comment

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

Code climate is advisory, take it with a pinch of salt. Would be good still to have a dedicated role check function so basically just extract the contents of this if to a function I would say

Copy link
Author

Choose a reason for hiding this comment

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

Got it, I've extracted the logic into a separate function.


var roles = ExtractRolesFromClaims(claims)

if !isSubarray(roles, p.Roles) {
Copy link
Member

@NickMeves NickMeves Sep 8, 2020

Choose a reason for hiding this comment

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

So we might need to think about how this aligns with other implementations. In general, in other group/role AuthZ logic in other providers, we've made this an if any roles match the list, access is allowed (instead of all must be present).

Copy link
Author

@khawaga khawaga Sep 8, 2020

Choose a reason for hiding this comment

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

Oh, are roles being used in any other provider? The only thing I've seen is groups, and for that it makes sense that access would be granted for a user belonging to any specified group. This was based on the Louketo Proxy implementation, but that one also had an additional require-any-role flag that enables the behavior you described. I'm not sure what the standard is when it comes to this so I would highly appreciate your input.

I am open for suggestions, would you like me to change it so that it grants access if any of the provided roles are present?

Copy link
Member

Choose a reason for hiding this comment

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

Somewhere buried in the discussion of this PR, the issue came up: #616

At least Google, Github, & Gitlab also have AuthZ logic where it was an ANY operation instead of an ALL. Let's keep that concept consistent.

Once #616 merges, I have a TODO item to dig through all the disjoint Group/Role methods the providers have setup and streamline the logic. I think a lot of these providers would benefit from the X-Forwarded-Groups header logic that is being added for OIDC that has been a frequent request by users (to be passed with User + Email)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah -- Please convert this to ANY logic instead of ALL for multi-role scenarios to match the other implementations.

p.Roles = roles
}

func ExtractRolesFromClaims(claims map[string]interface{}) []string {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add Unit Tests for this new method? (And add new cases for the additions you made to existing methods)

Copy link
Author

Choose a reason for hiding this comment

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

Sure thing, would it make sense to change auth_test.go in order to allow passing an access token when calling CreateAuthorizedSession? Since the roles are extracted from the access token so it needs to be valid otherwise token parsing will fail.

Copy link
Member

Choose a reason for hiding this comment

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

Yep go for it. Or just change this to have what you need:

var authorizedAccessToken = "imaginary_access_token"

It obviously looks like the other test helper methods that pull that don't care about its actual value.

Copy link
Author

Choose a reason for hiding this comment

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

I've added two more tests.

Copy link
Author

Choose a reason for hiding this comment

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

I've edited the token in auth_test.go in order to contain a valid token instead of the dummy that was being used. It's currently hardcoded which is not exactly ideal, but if you'd like I can add another method to create the token (that can then be called from the tests in order to create and then set it).

Copy link
Member

Choose a reason for hiding this comment

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

If you want it more dynamic, you can take a look at oidc_test.go -- it had a couple examples of dynamic tokens and stubbed token verifiers:

func newSignedTestIDToken(tokenClaims idTokenClaims) (string, error) {

func (fakeKeySetStub) VerifySignature(_ context.Context, jwt string) (payload []byte, err error) {

Copy link
Member

Choose a reason for hiding this comment

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

Does this function need to be exported?

Copy link
Author

Choose a reason for hiding this comment

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

It does not, I unexported it :)

Copy link
Member

@NickMeves NickMeves left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this! I left some notes to try to get this aligned with the other group/role work that is ongoing in other providers.

@khawaga khawaga force-pushed the add-keycloak-roles branch 2 times, most recently from 41703c1 to 621de9b Compare September 14, 2020 09:39
@khawaga
Copy link
Author

khawaga commented Sep 14, 2020

@NickMeves I've added some tests and I've also replied to your comments. Thank you for the prompt review!

Comment on lines +11 to +12
// The following is a valid JWT, it can be decoded, edited and re-encoded using https://jwt.io/
var authorizedAccessToken = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkhvbWVyIFNpbXBzb24iLCJlbWFpbCI6IkNodW5reUxvdmVyNTNAYW9sLmNvbSIsImlhdCI6MTUxNjIzOTAyMiwicmVhbG1fYWNjZXNzIjp7InJvbGVzIjpbInRlc3QtcmVhbG1yb2xlMSIsInRlc3QtcmVhbG1yb2xlMiJdfSwicmVzb3VyY2VfYWNjZXNzIjp7ImNsaWVudCI6eyJyb2xlcyI6WyJ0ZXN0LWNsaWVudHJvbGUxIiwidGVzdC1jbGllbnRyb2xlMiJdfX19.DqKfcantBn7B8acd9rl0LK8FL3sVhUcrM_AHztWI2A0"
Copy link
Member

Choose a reason for hiding this comment

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

What key was used to sign this?

Copy link
Author

@khawaga khawaga Sep 21, 2020

Choose a reason for hiding this comment

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

It's using the default values on jwt.io and the default secret which is "your-256-bit-secret" 😅

Should I change it to something else? Or add it as a comment?

Copy link
Member

Choose a reason for hiding this comment

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

I'm centralizing OIDC logic in this PR and making sample IDTokens more accessible to test cases across providers here: https://github.com/oauth2-proxy/oauth2-proxy/pull/936/files#diff-e719a8f54d2fe3115f6fcaabd056be5b53e86a3f04004d3c53f0b6c5b0409d68R34

Might be of use to this PR if you need valid IDTokens for testing.

p.Roles = roles
}

func ExtractRolesFromClaims(claims map[string]interface{}) []string {
Copy link
Member

Choose a reason for hiding this comment

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

Does this function need to be exported?

providers/keycloak.go Show resolved Hide resolved
@@ -95,5 +128,29 @@ func (p *KeycloakProvider) GetEmailAddress(ctx context.Context, s *sessions.Sess
}
}

if len(p.Roles) > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Code climate is advisory, take it with a pinch of salt. Would be good still to have a dedicated role check function so basically just extract the contents of this if to a function I would say

providers/keycloak.go Outdated Show resolved Hide resolved
providers/keycloak.go Outdated Show resolved Hide resolved

// Return true if secondArray is a subset of firstArray
func isSubarray(firstArray, secondArray []string) bool {
arraySet := make(map[string]bool)
Copy link
Member

Choose a reason for hiding this comment

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

Nit, better to use map[string]struct{} and then arraySet[value] = struct{}{} when setting it and if _, found := arraySet[value]; !found when checking, this is more explicit and idiomatic in Go.

Copy link
Author

Choose a reason for hiding this comment

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

I truly appreciate the tip! I changed it now.

@khawaga
Copy link
Author

khawaga commented Sep 21, 2020

@NickMeves I have made the requested changes and I've also replied to your comments, thanks!

@khawaga khawaga force-pushed the add-keycloak-roles branch 2 times, most recently from 9294d33 to 9ab7a7e Compare September 22, 2020 14:05
Copy link
Member

@NickMeves NickMeves left a comment

Choose a reason for hiding this comment

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

Sorry if these comments didn't get submitted before... I guess I just left them pending somehow and didn't get them over to you. Forgive me if some are outdated at this point -- but I see a few issues that might still need to be resolved.


var roles = ExtractRolesFromClaims(claims)

if !isSubarray(roles, p.Roles) {
Copy link
Member

Choose a reason for hiding this comment

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

Somewhere buried in the discussion of this PR, the issue came up: #616

At least Google, Github, & Gitlab also have AuthZ logic where it was an ANY operation instead of an ALL. Let's keep that concept consistent.

Once #616 merges, I have a TODO item to dig through all the disjoint Group/Role methods the providers have setup and streamline the logic. I think a lot of these providers would benefit from the X-Forwarded-Groups header logic that is being added for OIDC that has been a frequent request by users (to be passed with User + Email)

p.Roles = roles
}

func ExtractRolesFromClaims(claims map[string]interface{}) []string {
Copy link
Member

Choose a reason for hiding this comment

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

Yep go for it. Or just change this to have what you need:

var authorizedAccessToken = "imaginary_access_token"

It obviously looks like the other test helper methods that pull that don't care about its actual value.

p.Roles = roles
}

func ExtractRolesFromClaims(claims map[string]interface{}) []string {
Copy link
Member

Choose a reason for hiding this comment

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

If you want it more dynamic, you can take a look at oidc_test.go -- it had a couple examples of dynamic tokens and stubbed token verifiers:

func newSignedTestIDToken(tokenClaims idTokenClaims) (string, error) {

func (fakeKeySetStub) VerifySignature(_ context.Context, jwt string) (payload []byte, err error) {


if clientRoles, found := claims["resource_access"].(map[string]interface{}); found {
for name, list := range clientRoles {
scopes := list.(map[string]interface{})
Copy link
Member

Choose a reason for hiding this comment

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

Are you confident in the safety of these type assertions?

Do we need an ok check on these? I'd lean toward including them even if you are fairly confident, in case all environment aren't as we expect or things change -- we just had a similar issue in another PR with the groups claim from certain IdPs behaving in non-standard ways.


var roles = ExtractRolesFromClaims(claims)

if !isSubarray(roles, p.Roles) {
Copy link
Member

Choose a reason for hiding this comment

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

Yeah -- Please convert this to ANY logic instead of ALL for multi-role scenarios to match the other implementations.

@khawaga
Copy link
Author

khawaga commented Sep 23, 2020

@NickMeves I've changed the logic (and the docs) to use ANY instead of ALL for role-matching, and I've also added some ok checks to the type assertions. Are there any other issues that need to be resolved?

@NickMeves
Copy link
Member

Adding in some minor scope creep:

PR #616 which just merged did very similar AuthZ work as this for the OIDC provider. It additionally added in a global Groups field to all sessions that when set would result in X-Forwarded-Groups HTTP header to backend applications (so they could use it for fine-grained route based RBAC themselves):

https://github.com/oauth2-proxy/oauth2-proxy/pull/616/files#diff-6fd8df33d9f8086bc31e28c375fbc0abR951

I think we should dump the roles you extract into the session so users get access to these details in their backend applications easily.

I know the names don't fully line up, but the bazillion providers all have different group-like concepts with different names -- so I think Groups is the ideal area to centralize them letting our first class provider OIDC get the naming that mirrors its conventions.

@NickMeves
Copy link
Member

@NickMeves I've changed the logic (and the docs) to use ANY instead of ALL for role-matching, and I've also added some ok checks to the type assertions. Are there any other issues that need to be resolved?

I'll give everything another look this afternoon. For now just take care of the code climate findings: https://codeclimate.com/github/oauth2-proxy/oauth2-proxy/pull/767

Are these the same findings we talked about before that might've been challenging to refactor? Or are these new?

@NickMeves NickMeves mentioned this pull request Sep 23, 2020
3 tasks
@NickMeves
Copy link
Member

So I did some work this weekend to cleanup the session enrichment process after token redeem (#796) & centralizing authorization logic to be more global and uniform across providers (#797) since we are having so many authorization PRs lately taking their providers in different directions.

This PR caught my eye since it looks like the previous Keycloak roles logic was just tossed into GetEmailAddress since no better place existed even though it ideally shouldn't be there. The authZ logic also looks very similar to other places, so hopefully it can just hook into the default Authorize provider method (or implement its own and have it called at the right moments by oauthproxy.

I'll check back once those PRs merge.

@NickMeves NickMeves added this to Pull Requests (In Progress) in Release v7.0.0 via automation Nov 2, 2020
@NickMeves
Copy link
Member

Sorry for the delay. If you still want to get this over the finish line:

#797 merged.

The refactor to EnrichSessionState & Authorize provider interface methods are now in master. This should be completely unblocked now.

@khawaga
Copy link
Author

khawaga commented Nov 19, 2020

@NickMeves I would still like to see this through, definitely.
I'll take a look at the required changes and refactor. Thanks for the heads-up!

CHANGELOG.md Outdated Show resolved Hide resolved
@NickMeves
Copy link
Member

As a heads up, we are likely looking to make a V7 release sometime in mid-January. Just wanted to give you a timeline update in case you were looking to get this in for the next release.

func extractRolesFromClaims(claims map[string]interface{}) ([]string, error) {
var roleList []string

if realmRoles, found := claims["realm_access"].(map[string]interface{}); found {
Copy link
Member

Choose a reason for hiding this comment

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

By chance do you have a sample of what Keycloak IDToken claims are present? I'm curious if some of the data pulled via the userinfo/profileURL endpoint can potentially be extracted from IDToken claims and we don't need that extra HTTP call.

Copy link
Member

Choose a reason for hiding this comment

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

(This is definitely scope creep and not needed for your PR -- I'm just curious for my own understanding).

Copy link
Author

@khawaga khawaga Dec 9, 2020

Choose a reason for hiding this comment

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

Not sure about the IDToken, but the AccessToken contains both the email and the groups which should be sufficient to get the info used here without the extra HTTP call.

Copy link
Author

@khawaga khawaga Dec 9, 2020

Choose a reason for hiding this comment

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

Although that might require a breaking change since I believe that's configurable, if groups are returned via userinfo but not included in the access/id token.

Copy link
Author

@khawaga khawaga Dec 9, 2020

Choose a reason for hiding this comment

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

I checked, and it's possible to get the required info from the IDToken as well with the right configuration but it looks like roles are only included in the access token by default.

Copy link
Author

@khawaga khawaga Dec 10, 2020

Choose a reason for hiding this comment

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

Keycloak has a JWKS endpoint at <server_url>/auth/realms/<realm_name>/protocol/openid-connect/certs, it should be possible to verify it using that, should I reuse oidc-jwks-url for this purpose? If so, what would be the best way to pass it to the provider? Also, do we keep the non-verified behavior if it's not provided, or should this be required?

Copy link
Member

Choose a reason for hiding this comment

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

So I need your help explaining something 😅

Keycloak as a provider confuses me. When I look at it, it has barebones implementations as if it were vanilla OAuth2. (and should what it calls the validateURL be the profileURL...).

BUT - I get sample configs from users using OIDC config options with it set as the provider in questions frequently. If it is truly OIDC compliant in all cases, I feel like it should be baked into the provider more.

This way we can take advantage of all the new OIDC based features we are making global and Keycloak can take advantage of them (bearer token to session logic, generic claims to session fields - future adhoc claims -> session -> header support).

The reason Keycloak would still be a distinct provider instead of just using Keycloak with an OIDC provider: your custom roles from access tokens logic here, any special quirks in the schema of the userinfo endpoint etc.

Global OIDC logic refactor: #936

CC: @JoelSpeed -- if you have some history context or experience with this provider yourself, I would love your thoughts.

Copy link
Author

Choose a reason for hiding this comment

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

My understanding is that it is OIDC-compliant (it's OpenID-certified: https://openid.net/developers/certified/), I believe validateURL should be userInfoURL.
It should work with the standard OIDC provider implementation (although I have not tried that myself), but yeah basically the additional role functionality is probably worth having it as an independent provider.

Copy link
Member

Choose a reason for hiding this comment

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

So I think to get at an OIDC IDToken Verifier (if that works on the Access Token) or underlying raw JWKS, we use the existing flags to set it: https://oauth2-proxy.github.io/oauth2-proxy/docs/configuration/oauth_provider/#skip-oidc-discovery

During options validation, if you see --keycloak-roles you'd need to assert you also have the OIDC URLs setup so you have the tools at your disposal to verify the signature of the Access Token.

Copy link
Member

@NickMeves NickMeves Dec 11, 2020

Choose a reason for hiding this comment

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

And on the userinfo... I guess technically it works as a validateURL since it will take an access token and return 200, which is all this is looking for:

func validateToken(ctx context.Context, p Provider, accessToken string, header http.Header) bool {

But it is primarily used as a profileURL in the Keycloak provider itself. I'll see if I can get a PR up to clean that up in a backwards compatible way.

Don't worry about that question in this PR.

Copy link
Member

@NickMeves NickMeves left a comment

Choose a reason for hiding this comment

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

So I think all my refactors since you originally submitted this have changed the lay of the land a bit 😅

I've tried to leave notes on how to integrate with the new standardized/global ways we've setup to do this. Please ask whatever question you need, I'll try to respond quickly to get you going in the right direction with some of the new architectures.

I haven't looked at the new test cases at all -- I'll review those once we get the code aligned to the new standards.

@khawaga khawaga changed the title Allow roles to be specified for Keycloak WIP: Allow roles to be specified for Keycloak Dec 9, 2020
if err != nil {
logger.Printf("failed to extract client roles: %s", err)
}
return append(customClaims.RealmAccess.Roles, clientRoles...), nil
Copy link
Member

@NickMeves NickMeves Dec 10, 2020

Choose a reason for hiding this comment

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

It feels weird to pollute & extend the underlying object we extracted from the token claims.

I understand the performance motivations of reusing the existing slice since it is done at the end of the function anyway, but since these are small & therefore performance is negligible, maybe clarity is more ideal?

Took me a second on first glance to realize the desired output was intended to build the final roles list from RealmAccess + ResourceAccess

Copy link
Author

Choose a reason for hiding this comment

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

You're right, I will change this for better clarity.

return nil, errors.New("error parsing client roles from claims")
}
if roles, found := scopes["roles"]; found {
for _, r := range roles.([]interface{}) {
Copy link
Member

Choose a reason for hiding this comment

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

Do you need to handle when this isn't a []interface{} or have fallback code that somehow marshals non-slices if you get a singleton? We have to handle that case in the OIDC side: 48baecb

Copy link
Author

Choose a reason for hiding this comment

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

From what I've seen it always returns a slice even when if it only contains a single element, is there any other case that I'm missing?

Copy link
Member

Choose a reason for hiding this comment

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

Cool - that makes sense that it is uniform since it is all provided by the singular Keycloak. OIDC is all over the place cause identity providers do some crazy stuff with groups since it isn't a totally official claim.

}
if roles, found := scopes["roles"]; found {
for _, r := range roles.([]interface{}) {
clientRoles = append(clientRoles, fmt.Sprintf("%s:%s", name, r))
Copy link
Member

Choose a reason for hiding this comment

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

Will name always be role? Should we make that prefix explicit here instead of from a variable so we don't get surprised later?

Copy link
Member

Choose a reason for hiding this comment

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

Oh wait, nevermind - I see the role prefix later. So this has a different name:role structure that represents the full role name?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, so effectively this would end up in allowed groups as role:clientname:clientrole. These are client roles which are namespaced by client, so while the structure is predictable the client name could be anything, hence the additional prefix.

@khawaga
Copy link
Author

khawaga commented Jan 1, 2021

@NickMeves since #953 is now merged I'll rebase my branch on top of that, I'll post an update here once that's done.

@NickMeves
Copy link
Member

@NickMeves since #953 is now merged I'll rebase my branch on top of that, I'll post an update here once that's done.

So take a look at this issue: #956

Long term, there's a few providers that I want to move from being OAuth based to OIDC based to embrace all that functionality. Keycloak is one of them (especially with the features you want to add). The existing api scope OAuth provider sorta sucks and I've realized alot of Keycloak users are hooking it into the oidc provider (after getting burned by thinking keycloak provider is for them but is missing functionality).

So after working on the existing keycloak provider. I think it eventually needs to get deprecated in favor of an OIDC based variant.

I think we make a new keycloak-oidc provider for the time being (in v8 it will replace the keycloak). We can have it be based on the OIDCProvider as a base. Then I think you get nearly all the functionality you need in the OIDC methods and you just need to add a few methods that extracts the roles from the Access Token.

That also gets you the OIDC supporting methods to appropriately verify your Access Token before extracting claims from it.

What do you think?

Let me know if you need a hand spiking out what a base keycloak-oidc provider would look like.

@khawaga
Copy link
Author

khawaga commented Jan 14, 2021

@NickMeves what you're suggesting makes perfect sense, is there currently any provider that's following this pattern (using the OIDC provider as a base, then extending it with additional functionality) that I can look at? I would appreciate your help on this. :)

@NickMeves
Copy link
Member

@NickMeves what you're suggesting makes perfect sense, is there currently any provider that's following this pattern (using the OIDC provider as a base, then extending it with additional functionality) that I can look at? I would appreciate your help on this. :)

@khawaga You would be the first - the trailblazer 😉

Let me know if you need a hand. I can find time over the weekend to spike out a baseline keycloak-oidc provider that would completely inherit OIDCProvider methods (it would have OIDCProvider in its struct composition instead of ProviderData in essence).

Then from that you could add on the few overrides for things like EnrichSession to handle the custom Access Token role logic.

@NickMeves NickMeves removed this from Pull Requests (In Progress) in Release v7.0.0 Jan 14, 2021
@NickMeves NickMeves mentioned this pull request Mar 15, 2021
3 tasks
@github-actions
Copy link
Contributor

This pull request has been inactive for 60 days. If the pull request is still relevant please comment to re-activate the pull request. If no action is taken within 7 days, the pull request will be marked closed.

@github-actions github-actions bot added the Stale label Mar 16, 2021
@github-actions github-actions bot closed this Mar 23, 2021
@NickMeves
Copy link
Member

I will finish this work in #1107 since this PR has gone stale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants