OIDC adapter has to provide HttpPermissionChecker

[sberyozkin]

Description
Example of the original quarkus-keycloak configuration:

quarkus.keycloak.policy-enforcer.claim-information-point.claims.request-uri={request.relativePath}
quarkus.keycloak.policy-enforcer.claim-information-point.claims.request-method={request.method}
quarkus.keycloak.policy-enforcer.lazy-load-paths=true

quarkus.keycloak.policy-enforcer.paths.1.path=/asset-service/resources/tenants
quarkus.keycloak.policy-enforcer.paths.1.methods.1.method=POST
quarkus.keycloak.policy-enforcer.paths.1.methods.1.scopes=tenants:create

quarkus.keycloak.policy-enforcer.paths.10.path=/asset-service/resources/tenants/{tenantId}
quarkus.keycloak.policy-enforcer.paths.10.methods.1.method=PUT
quarkus.keycloak.policy-enforcer.paths.10.methods.1.scopes=tenant:edit
quarkus.keycloak.policy-enforcer.paths.10.methods.2.method=DELETE
quarkus.keycloak.policy-enforcer.paths.10.methods.2.scopes=tenant:delete
quarkus.keycloak.policy-enforcer.paths.11.path=/asset-service/resources/tenants/{tenantId}/disable
quarkus.keycloak.policy-enforcer.paths.11.methods.1.method=PUT
quarkus.keycloak.policy-enforcer.paths.11.methods.1.scopes=tenant:disable
quarkus.keycloak.policy-enforcer.paths.12.path=/asset-service/resources/tenants/{tenantId}/enable
quarkus.keycloak.policy-enforcer.paths.12.methods.1.method=PUT
quarkus.keycloak.policy-enforcer.paths.12.methods.1.scopes=tenant:enable

@dfranssen:

E.g. Keycloak javascript permission using the request-uri. Our path is like /asset-service/resources/tenants/abc where abc is the tenant name (uriPart[4] in the code below) for which we check the actual authenticated user is part of. In Keycloak we have a group per tenant (e.g. /abc/users) where the user is put into.

var attributes = context.getAttributes();
var httpUri = attributes.getValue('request-uri');
function isInTenantGroup(tenantId) {
    var result = false;
    var wanted = "/" + tenantId + "/users";
    var identity = context.getIdentity();
    var userGroups = identity.getAttributes().getValue('user-groups');
    if (userGroups !== null) {
        for(var i=0; i < userGroups.size(); i++) {
            if(wanted === userGroups.asString(i)) {
                result = true;
                break;
            }
        }
    }
    return result;
}

if (httpUri) {
    var uriParts = httpUri.asString(0).split('/');
    if (isInTenantGroup(uriParts[4])) {
        $evaluation.grant();
    }
}

Implementation ideas
The adapter has to introspect a scope claim (or Keycloak specific claim - in this case we should have a claim name configured to avoid tying the adapter to KC - though we can do it later). PolicyEnforcer configuration group can be introduced if needed

[sberyozkin]

See #4487 about HttpPermissionChecker

[sberyozkin]

@pedroigor FYI. Pedro, is all we need to do here is parse the KC specific scope claim to get it going for @dfranssen ? What are those numbers 1, 10, 12 etc ? May be I can do a PR...

[sberyozkin]

@stuartwdouglas, if we have @RolesAllowed, how would the users configure path/verb, as the configuration in your PR activates the default checker ?
By the way, we can introduce, as part of this issue, or later, @scope annotation, to support the OIDC-centric experience better...
@stianst FYI as well

[pedroigor]

@sberyozkin, it is not really related with the scope claim. But authorization claim which holds the permissions (for resources and scopes) granted by Keycloak.

In a nutshell, what we need to do is somehow parse this authorization claim and enforce permissions accordingly depending on the resources that were granted and how they are mapped to specific paths in your application.

This is the KC specific feature we talked about (fine-grained/contextual permissions). Although it also has support for UMA specs.

Do we want a separate extension for it ? Or have it within the quarkus-oidc ?

[pedroigor]

@sberyozkin about those numbers. They are the index for each path you are configuring (backed by a list of paths).

[sberyozkin]

@pedroigor OK, thanks. I believe it should work for any IDP, not only Keycloak, at the adapter level we don't really mind I guess where the token is coming from.
Can you please type an example of the KC authorization claim ?
I suppose for a complete UMA we may consider an extension, but for simpler cases we may ship few checkers with the quarkus-oidc

[sberyozkin]

@pedroigor If it is really Keycloak specific then may be we can introduce quarkus-oidc-keycloak (configuration group quarkus.oidc.keycloak) which would only ship KC specific bits.

I reckon we should try to keep generalizing i.e we can have a simple checker which would work with all IDPs which would say only checks scope and/or groups claim and then we can have something like:

quarkus.oidc.permissions.scopes=a,b,c
quarkus.oidc.permissions.scopes.a.method=PUT,DELETE
quarkus.oidc.permissions.scopes.a.paths=path1,path2
# etc for every other scope

This obviously does not match the requirement of @dfranssen. To be honest, I don't see a way for a generic equivalent of the original keycloak.policy-enforcer because it does look like it is very specific to Keycloak (especially the numbers which can likely be nicely mapped in the KC management console but are tricky to understand by just looking at them in the config).
Stian, @stianst Pedro @pedroigor, can you please decide if this is something we absolutely have to support, if yes, then IMHO shipping a new extension, quarkus-oidc-keycloak would be the cleanest way forward, it would only ship HttpPermissionChecker enforcing the original keycloak.policy-enforcer configuration.
But may be you can see how it can be generically enforced at the quarkus-oidc level

Thanks

[pedroigor]

Can you please type an example of the KC authorization claim ?

"authorization": {
    "permissions": [
      {
        "scopes": [
          "profile:view"
        ],
        "rsid": "8e052f06-a543-4b9c-959d-7b59ca99d1f1",
        "rsname": "User Profile Resource"
      }
    ]
  }

The mapping of resources (rsid/rsname) to paths is what makes possible to enforce access accordingly to the resource/path being requested.

The PEP also allows you to enforce access by querying Keycloak in case the token is a regular access token. So that clients don't need beforhand to obtain a token with permissions.

I suppose for a complete UMA we may consider an extension, but for simpler cases we may ship few checkers with the quarkus-oidc

If you make this a priority, I would say that we should start without UMA.

[sberyozkin]

@pedroigor Thanks (though still not sure how the above authorization is mapped to something like what is posted by the user :-) ) and totally agree, lets deal with UMA later on.

But we need to decide what to do for 0.25.0 per my earlier comment.

I'm going offline now, but lets try to finalize tomorrow, may be Stian will have some comments too

Thanks

[pedroigor]

@sberyozkin, sure. Let's sync this.

In a nutshell, we just need to re-use the policy enforcer from the Keycloak Adapter.

[stianst]

As the policy enforcer is very Keycloak specific I think the ideal solution would be to have a quarkus-keycloak-authz extension that depends on quarkus-oidc to do the rest

[sberyozkin]

@pedroigor, @stianst thanks, so we are going with the keycloak specific extension of the oidc adapter, quarkus-keycloak-authz is fine if it will only deal with this policy-enforcer issue, but perhaps there will be more requirements to support something KC specific not related to the actual authorization ? Though quarkus-keycloak-authz can cover all the cases I guess.

[stianst]

@pedroigor would need to look into how feasible it is to create a pure policy enforcement extension, but for OIDC in general we shouldn't need KC specific stuff. Also, I think if we'd have quarkus-keycloak that would confuse folks thinking that they didn't need quarkus-oidc, and that is somehow did more than just authz

[sberyozkin]

@stianst +1, we are all in total agreement that quarkus-oidc is going the generic way now :-). quarkus-keycloak-authz is just fine for me as well if you and Pedro like this name.

[pedroigor]

We are aligned. We should go for a new extension. The name quarkus-keycloak-authz sounds good for me.

@stianst a generic PEP should be possible and it is pretty much possible based on the permission checker Stuart is doing. So that you can have custom checks or interact with a PDP by providing your custom checker implementation.

But yeah, we still need to provide something that works OOTB and that is KC specific (but based on specs).

[sberyozkin]

@pedroigor
If it is based on specs then it should be in quarkus-oidc per our strategy. quarkus-keycloak-authz would keep the old quarkus-keycloak users happy with the KC only specific feature, but once your idea of PEP is realized at some later stage we can may be deprecate quarkus-keycloak-authz given a more generic solution (I guess you were referring to UMA) is available

[sberyozkin]

Hi Pedro, David, copying some comments here:

David: "This is just a thought, but I was thinking there might be a point where it's better for the user to specify things using a builder of some sort (like the router API) rather than having super complex configurations"

Pedro: "Good point. It would certainly provide a better experience. The configuration I mentioned fits better in a JSON format. But in a properties file, it is not so nice. So, what you are proposing is to basically expect a configuration object instance that is produced by the application (using a builder/fluent API provided by the extension) and used by my custom permission checker during initialization? Do we have something similar in any other extension ?"

Sergey: "One possible issue here is that by improving the option for this KC specific configuration we can make it more difficult to eventually migrate users to a generic adapter, especially if they will have something in their code :-). JSON format or a custom builder would be nice, but in this case we are dealing with the users of the original Keycloak adapter who can't just move to the new one yet and already have the configuration prepared. For UMA we can do something really nice with what David and Pedro suggests, perhaps we can prioritize on UMA soon enough for the new users be starting with it. Especially given that quarkus-oidc is expected to work not only with Keycloak"

Sergey: "Pedro, you can probably have something like this, have .runtime.PolicyEnforcer bean and get it initialized in a new extension's deployment processor, and pass it to the runtime recorder where you'd set this PolicyEnforcer instance on the CDI producer which produces a custom HttpPermissionChecker":

@recorder
public class KeycloakPolicyEnforcerRecorder {

public void init(BeanContainer container, PolicyEnforcer enforcer) {
    KeycloakPECheckerProducer producer = container.instance(KeycloakPECheckerProducer.class);
    producer.initialize(enforcer);

}
@ApplicationScoped
public class KeycloakPECheckerProducer {

private HttpPremissionChecker checker;

void initialize(PolicyEnforcer enforcer) {
    this.checker = new KeycloakPEChecker(enforcer);
}

@Singleton
@Produces
public KeycloakPEChecker checker() {
    return checker;
}

}

[pedroigor]

@stuartwdouglas @sberyozkin In order to plug the policy enforcer, I'm exposing a custom HttpAuthorizer so that I can perform the necessary steps and integration to enforce access using Keycloak Authorization Services. The custom implementation is marked as AlternativePriority.

That would mean replace the default authorizer, which I think is fine. Any thoughts?

[pedroigor]

I'm also not sure whether or not we should rewrite the implementation we have in Keycloak in order to better fit the reactive model in Quarkus. One of the things being that we use Apache HttpClient and would be nice if we could use vertx client instead.

However, it is quite a lot of work (I tried to start something) and error-prone. In addition to having two distinct codebases for the same purposes.

I would like then to reuse as much as possible what we have in Keycloak and adjusting where posible the implementation to properly use the async model provided by Vert.x.

[gsmet]

OK, so it's not 0.26 material, will move it to next release, thanks!

[sberyozkin]

@pedroigor I'm not sure about KeycloakSecurityContext. Lets not tie the users again to the KC API in their user code. What does it offer that JsonWebToken or SecurityIdentiy does not ? If needed we should enhance the latter for example. Thanks

[sberyozkin]

@pedroigor Hi Pedro, re the multi-tenancy, this is also an orthogonal issue (#4448). We have already said KeycloakConfigResolver is no longer supported in the 0.24.0 release notes. The concept of loading the configuration dynamically should work generically (not sure now how to do it without keycloak.json :-), but I can imagine we can use config profiles instead, etc) - can talk more about it at #4448

[pedroigor]

@pedroigor I'm not sure about KeycloakSecurityContext. Lets not tie the users again to the KC API in their user code. What does it offer that JsonWebToken or SecurityIdentiy does not ? If needed we should enhance the latter for example. Thanks

I've considered it just because this is a Keycloak-specific extension and users are already used to it.

No problem if you want to re-use JsonWebToken or SecurityIdentity. However, there is one thing that we would need to do to make the updated token available once permissions are checked by the policy enforcer.

FYI, this enforcer can run in two modes. One mode is all about parsing the granted permission carried by the token. The second mode is exchanging an incoming bearer token with another one with the necessary permissions to access a protected resource. In the second mode, we need to somehow update the security identity with the token that was exchanged. But yeah, the authorizer kicks in after the identity is already established.

The permissions granted by Keycloak are stored in a authorization claim which today we encapsulate through a specific Authorization type that in addition to giving access to the permissions carried by the token it also provides some utility permissions to check whether or not permission was granted as well as to obtain an AuthzClient instance (a client API to the authorization API in Keycloak).

[pedroigor]

@sberyozkin @stuartwdouglas I'm about to send a PR (including quickstart) considering all the suggestions and issues herein suggested.

I'm still not sure about how to proceed with the commons-logging error I mentioned before. The behaviour is quite weird. When running tests, everything works fine (JVM mode) but when running using a separated application (e.g.: following the guide/quickstart) it does not work in JVM mode. To make it happen users would need to include commons-logging in their pom.xml. Probably a not good thing.

Like I mentioned before, removing the library from the list of excluded deps in Quarkus would make things work. But I'm not sure if that is something I'm allowed to do. What do you suggest ?

[sberyozkin]

@pedroigor Hi Pedro, I see, I propose that we discuss this concept, to show which permission was granted as part of some follow up PR, because once the oidc adapter will support UMA, etc, the same kind of utility method check should work as well. We'll add some nice typed methods to TokenCredentail for example.
The most important thing is that we'll have the users able to migrate, but this keycloak adapter is really just about helping them to migrate as opposed to growing in some parallel way with the oidc one :-), Please check if Stian is fine with this interpretation, my understanding was that yes....

The other thing, that 2nd part, the token exchange, that should be eventually done at the quarkus-oidclevel as per our earlier discussions as it is not KC specific ? We can do in the next phase though.
FYI, I'm at Apache Con so may be slow in replying

[pedroigor]

@sberyozkin The part that makes available the granted permissions through the SI is already implemented. I've even added a PermissionChecker so that users can use SI related methods to check whether or a specific permission is granted (if they want to check programmatically).

The exchange part is also working and is a core capability of the extension.

The missing bit now is just this commons-logging issue that I'm not sure how to proceed ...

[sberyozkin]

@pedro, sure, that the KC policy enforcer needs it; but in time we'll have the oidc adapter having a similar requirement, exchange a token (to delegate or impersonate) using the token exchange protocol - the concept has to work with the 3rd party IDPs. At that point we'll most likely want to reuse the code which will be located in the KC extension but without the KC specific API :-)

I propose to open a PR for us to discuss for few days, and then once it makes it - add a follow up PR.
Or just not use KC specific API and Apache HTTP Client - do few simple gets/posts with the API which requires fewer deps ? Actually, it rings a bell, @vsevel had to replace the Apache HTTP client with OK HTTP client as part of the Vault work which is now on master

[vsevel]

some (@starksm64 @gsmet) have argued that rest calls should be done using the default quarkus jax-rs client. I created #4162 as a result of this feedback.
as far as okhttp is concerned, it seemed smaller than the apache httpclient, and I saw that the fabric8 kubernetes client was already using it.
the new java 11 HTTP Client might offer an alternative at some point, but I suspect quarkus will have to support java 8 for a while, so that might not be an option in the foreseeable future?

[dfranssen]

@pedroigor @sberyozkin I tested this weekend with v0.27.0 and all remarks I have made have been resolved. Thanks a lot!

[pedroigor]

@dfranssen tk u for checking it !