JSON configuration format for client policies

[Marek Posolda]

Hi,

We are introducing client policies feature into Keycloak, which is
discussed in more details in the design document
https://github.com/keycloak/keycloak-community/blob/master/design/client-policies.md.
Recently, we discussed within the team how to improve the fine-grained
of the JSON configuration, so that it is great enough to be usable. Once
we polish the format, we can hopefully reuse the same pattern for other
stuff like for example userProfile.

The client policies consists of the configuration of client profiles and
client policies. The difference between them is, that profile contains
list of "Executors" when the policy contains list of "conditions" . I am
adding the example just for the profile configuration, which is
sufficient as same pattern can be applied to policies.

I am adding few possibilities how the configuration can look like. In
this example, there is profile, which has 2 executors. Executor, which
refers to the provider "pkce-enforce" has one configuration option
"is-augment" . The "secure-client-authn" has two configuration options.
Please add the feedback, which option you like the best and eventually
propose your own if you think that all proposals sucks :)


1) This is what I currently have in my fork. The field "executor" refers
to the provider ID of the java provider implementation.

{
  "name": "profile-basic",
  "description": "Some profile description",
  "builtin": false,
  "executors": [
    {
      "executor": "pkce-enforce",
      "configuration": {
        "is-augment": "true"
      }
    },
    {
      "executor": "secure-client-authn",
      "configuration": {
        "client-authn": "client-jwt",
        "is-augment": "true"
      }
    }
  ]
}


2) Add the configuration options as first-level citizens into JSON at
the same level like the "executor" option:


{
  "name": "profile-basic",
  "description": "Some profile description",
  "builtin": false,
  "executors": [
    {
      "executor": "pkce-enforce",
      "is-augment": "true"
    },
    {
      "executor": "secure-client-authn",
      "client-authn": "client-jwt",
      "is-augment": "true"
    }
  ]
}



3) Make "executors" to NOT be an array, but object. And have the ID of
executor provider to be used as the keys inside the "executors" object.
This format is short and friendly IMO, however it has one limitation,
that you can't have more executors referring to same provider inside the
"Executors" field. This should not be an issue for the existing executor
and condition implementations,however I think that in general, this can
be an issue in the future. Due to this limitation, I am not in favor of
this option.


{
  "name": "profile-basic",
  "description": "Some profile description",
  "builtin": false,
  "executors": {
      "pkce-enforce": {
          "is-augment": "true"
      },
      "secure-client-authn": {
          "client-authn": "client-jwt",
          "is-augment": "true"
      }
  }
}




My preference is 2, 1, 3 and hence vote for 2 to be used. WDYT?

Marek

[Schuster Sebastian (IOC/PAU1)]

I would vote for 2) too, purely based on aesthetics __ and the fact that maybe the order of executors in an array can also be used to convey precedence?

Mit freundlichen Grüßen / Best regards

Dr.-Ing. Sebastian Schuster

Project Delivery Berlin 22 (IOC/PDL22)
Bosch.IO GmbH | Ullsteinstr. 128 | 12109 Berlin | GERMANY | www.bosch.io <http://www.bosch.io>
Tel. +49 30 726112-485 | Mobil +49 152 02177668 | Telefax +49 30 726112-100 | Threema <threema://add/?id=MF9VMEAE> / Threema Work <threemawork://add/?id=MF9VMEAE>: MF9VMEAE | Sebastian...@bosch.io

Sitz: Berlin, Registergericht: Amtsgericht Charlottenburg; HRB 148411 B
Aufsichtsratsvorsitzender: Dr.-Ing. Thorsten Lücke; Geschäftsführung: Dr. Stefan Ferber, Dr. Aleksandar Mitrovic, Yvonne Reckling

On 26.04.21, 12:07, "keyclo...@googlegroups.com on behalf of Marek Posolda" <keyclo...@googlegroups.com on behalf of mpos...@redhat.com> wrote:

Hi,

We are introducing client policies feature into Keycloak, which is
discussed in more details in the design document

https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fkeycloak%2Fkeycloak-community%2Fblob%2Fmaster%2Fdesign%2Fclient-policies.md&amp;data=04%7C01%7Csebastian.schuster%40bosch.io%7C352df79014e640d3e5ae08d9089b21e6%7C0ae51e1907c84e4bbb6d648ee58410f4%7C0%7C0%7C637550284642683216%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=47SXTuFWmCTlhHybhGy0vU7wTviPSDJTkAg0QHX1F6I%3D&amp;reserved=0.

--
You received this message because you are subscribed to the Google Groups "Keycloak Dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to keycloak-dev...@googlegroups.com.
To view this discussion on the web visit https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fd%2Fmsgid%2Fkeycloak-dev%2F2ff18472-082b-a364-9f57-a2a0dcdd4076%2540redhat.com&amp;data=04%7C01%7Csebastian.schuster%40bosch.io%7C352df79014e640d3e5ae08d9089b21e6%7C0ae51e1907c84e4bbb6d648ee58410f4%7C0%7C0%7C637550284642693213%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=UCxHzYveMOdoGtDJRBeX6KXIGBhx0i1s8rp2ZVhNPho%3D&amp;reserved=0.

[Stian Thorgersen]

Why is there a "builtin" field?

 

   "executors": [
     {
       "executor": "pkce-enforce",
       "configuration": {
         "is-augment": "true"
       }
     },
     {
       "executor": "secure-client-authn",
       "configuration": {
         "client-authn": "client-jwt",
         "is-augment": "true"
       }
     }
   ]
}


2) Add the configuration options as first-level citizens into JSON at
the same level like the "executor" option:


{
   "name": "profile-basic",
   "description": "Some profile description",
   "builtin": false,
   "executors": [
     {
       "executor": "pkce-enforce",
       "is-augment": "true"

Can we come up with a better name for "is-augment" something that is a bit more obvious? "auto-configure" or something?

 

     },
     {
       "executor": "secure-client-authn",
       "client-authn": "client-jwt",
       "is-augment": "true"
     }
   ]
}



3) Make "executors" to NOT be an array, but object. And have the ID of
executor provider to be used as the keys inside the "executors" object.
This format is short and friendly IMO, however it has one limitation,
that you can't have more executors referring to same provider inside the
"Executors" field. This should not be an issue for the existing executor
and condition implementations,however I think that in general, this can
be an issue in the future. Due to this limitation, I am not in favor of
this option.


{
   "name": "profile-basic",
   "description": "Some profile description",
   "builtin": false,
   "executors": {
       "pkce-enforce": {
           "is-augment": "true"
       },
       "secure-client-authn": {
           "client-authn": "client-jwt",
           "is-augment": "true"
       }
   }
}




My preference is 2, 1, 3 and hence vote for 2 to be used. WDYT?

I have the same preference order

 

Marek

--
You received this message because you are subscribed to the Google Groups "Keycloak Dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to keycloak-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/keycloak-dev/2ff18472-082b-a364-9f57-a2a0dcdd4076%40redhat.com.

[Marek Posolda]

The "builtin" profiles are always set by default when you create new realm. We discussed before that "builtin" profiles should not be saved as realm attributes as they just always exists. Administrator cannot CRUD builtin profiles, however he should be able to see builtin profiles in admin console IMO.

Currently it works like this:

- In the "Form" view in the admin console, the builtin profiles are shown to administrator and they are read-only to him

- In the JSON editor view, builtin profiles are also shown right now in my admin console prototype. Hence this "builtin" field is also shown to differentiate builtin from those, which were manually added.

In theory, we can hide builtin profiles in JSON editor. This will allow us to remove "builtin" field as all the profiles shown in JSON would never be builtin. However my vote is to show both "builtin" and "non-builtin" in the JSON and just throw the error in case administrator tries to change something in the "builtin" profile. Showing JSON for builtin profile can be useful for example when administrator wants to copy some "builtin" profile into new profile as he wants to just do some small change in the builtin profile (Similar use-case why people use "copy authentication flow" to copy from some of our builtin authentication flows).

   "executors": [
     {
       "executor": "pkce-enforce",
       "configuration": {
         "is-augment": "true"
       }
     },
     {
       "executor": "secure-client-authn",
       "configuration": {
         "client-authn": "client-jwt",
         "is-augment": "true"
       }
     }
   ]
}


2) Add the configuration options as first-level citizens into JSON at
the same level like the "executor" option:


{
   "name": "profile-basic",
   "description": "Some profile description",
   "builtin": false,
   "executors": [
     {
       "executor": "pkce-enforce",
       "is-augment": "true"

Can we come up with a better name for "is-augment" something that is a bit more obvious? "auto-configure" or something?

+1, so let's change to "auto-configure" :)

I also did not know what term "augment" means since this year when first saw this term used by Quarkus.

Marek

[Marek Posolda]

On 26. 04. 21 12:28, Schuster Sebastian (IOC/PAU1) wrote:
> I would vote for 2) too, purely based on aesthetics __ and the fact that maybe the order of executors in an array can also be used to convey precedence?

Yes, true. Ordering is another reason why option 3 won't be ideal...

Thanks for the feedback!

Marek

[Pedro Igor Craveiro e Silva]

Could you elaborate on why a single provider in executors is a limitation? I mean, why you need the same provider multiple times?

I also agree that this design is more friendly and we could make `executors` an array too.

 

{

   "name": "profile-basic",
   "description": "Some profile description",
   "builtin": false,
   "executors": {
       "pkce-enforce": {
           "is-augment": "true"
       },
       "secure-client-authn": {
           "client-authn": "client-jwt",
           "is-augment": "true"
       }
   }
}




My preference is 2, 1, 3 and hence vote for 2 to be used. WDYT?

Marek

--
You received this message because you are subscribed to the Google Groups "Keycloak Dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to keycloak-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/keycloak-dev/2ff18472-082b-a364-9f57-a2a0dcdd4076%40redhat.com.

[Marek Posolda]

For example, assume that in theory you have something like "Role validator", which evaluates to true if user is member of specified role. In case that this validator implementation supports just single role, but administrator wants to have rule like "Validate to true if user is member of role foo AND role bar", you would need to add two instances of "role validator" - one for role "foo" and one role "bar" .

This is not an issue for any of existing condition providers or executor providers, as for example "Condition role" allows to have array of roles. But I suppose it can be limitation in the future for some use-cases or implementations.

Also as pointed by Sebastian, ordering can be an issue with key-value pairs. To avoid potential issues with ordering etc, we can convert "executors" to an array as you pointed. So the variant of proposal 3 with executors as an array would look something like this:

{
   "name": "profile-basic",
   "description": "Some profile description",
   "builtin": false,
    "executors": [       
        {
            "pkce-enforce": {
                "is-augment": "true"
            }
        },    
        {
            "secure-client-authn": {
                "client-authn": "client-jwt",
                "is-augment": "true"
            }
        }

    ]

}

This means "executors" as an array of JSONs where each JSON has just one key (like "pkce-enforce") and wrapping another inline JSON with the configuration. TBH the proposal 2 looks better to me than this one.

Marek

[Pedro Igor Craveiro e Silva]

Yeah, JSON is probably not the most friendly format, as Hynek likes to point out.

If you transform that to YAML it would be simple as:

name: "profile-basic"
description: "Some profile description"
builtin: false
executors:
  - pkce-enforcer:
        is-augment: true
  - secure-client-authn:
        client-authn: true
        is-augment: true

Eliminating one level and, IMO, making it more readable (even for JSON).

I understand your point around the limitation. But as you gave the example for roles, it should be possible to come up with a similar approach for future executors.

[Stian Thorgersen]

I agree that it would be useful to the UI to be able to view information about the built-in profiles, but don't think that should be included in the same JSON. It has a few downsides as it means the JSON I see is not what I have configured, and makes it also then slightly confusing as you need to keep the "default" stuff in the JSON, while not touching it.

Builtin is also a slightly odd name. What about "global", "read-only", or something?

Could we perhaps have one endpoint to fetch the custom per-realm, and another to see the built-in read-only profiles? Or a query parameter.

GET /client-policies - would only return per-realm

GET /client-policies?include-global=true - would return both, then in the JSON the "global" or "read-only" or "built-in" field is not included for custom policies, only for the built-ins?

[Marek Posolda]

Yes, it should be fine with changing the name to "global" instead of "built-in" . However the "read-only" does not work IMO as the "global" client policies are not strictly "read-only" .

Global profiles are read-only, but global policies have "enabled" field, which should be possible to change as administrator should have some way to disable the global (default) policies. For client profiles, this is not needed as profile is effectively disabled by the fact that it is not used in any enabled policy. So client profiles are read-only. This is more described in the design https://github.com/keycloak/keycloak-community/blob/master/design/client-policies.md#managing-half-and-half-data .

Regarding this, I suppose we want to return the default policy to make possible for administrator to disable it? So something like this:

{
  "name": "global-default-policy",
  "enabled": true
}

Hence "include-global" field may have 3 states then like:

- all-fields (Include full JSON of global policies including all conditions etc)

- editable-fields-only (Only include name and enabled field as shown above. This can be the default setting)

- nothing (Not include global at all)

WDYT?

Marek

[Marek Posolda]

Another option is to have separate 1st level entries for "global-policies" and "policies" . This will allow to remove "global" flag from individual policies:

{

    "global-policies": [

        ...

    ],

    "policies": [

        ...

    ]

}

For "global-policies", we can display the "name" and "enabled" field by default to be possible for administrator to disable the particular global policy for the realm.

Marek

[Stian Thorgersen]

Q - Why do we have built-in policies? I would have assumed we'd only have built-in profiles, then folks used a policy to enable them?

[Marek Posolda]

ATM we don't need it. For example for FAPI, my thinking is to go with built-in profiles as described in the other thread.

However I think that for the future, we may want to have some global (default) client policies in the realm enabled by default to enforce some client settings to be "secured by default" ? You added good example in this JIRA https://issues.redhat.com/browse/KEYCLOAK-14279 . Another example are client registration policies, which we want to remove in the future in favor of client policies. However currently the "built-in" client registration policies makes sure that for example OIDC dynamic client registration without initial token is disabled by default.

Possible solution to have this by default and avoid "global" policies might be to "auto-create" default policy always when you create new realm. But isn't "auto-create" stuff exactly one of the things, which we want to avoid? So have "global" client policy may be way to go, but at the same time, administrator should have a way to disable it IMO.

Marek

[Marek Posolda]

One thing, which I did not yet touched in this email, is the versioning of JSON configuration. This may be needed as the format of individual providers or name of the configuration option can change between versions.

Few questions:

- Should we have versioning like "1, 2, 3" or use Keycloak version? I think Keycloak version can be better to make it clear from which version we are migrating. We already use something like this for DB migrators (support for RHSSO is there as well)

- Should we keep version number at the root level or at the level of each provider?

I think that to avoid mess and make format clear, it will be good to stick with root level and use Keycloak version. So something like this:

{
    "policies": [
        ....
    ],
    "keycloak-version": "14.0.0"
}

WDYT?

Marek

[Marek Posolda]

We had the call within the team today. Just adding that some points we discussed was:

- Do not have built-in policies for now. It is not needed for FAPI. If we want to have the support for some built-in (global) policies in the future, like the usecase KEYCLOAK-14279, we will go with "auto-creating" the policies in the realm for now. In the future, we can possibly improve this with some advanced "Keycloak next" concepts like "Realm of realms" (Realm inheritance).

- Not use versioning in the JSON for now

Marek

On 29. 04. 21 14:07, Marek Posolda wrote: