LDAP Map storage / how to map read(String key) to realm? #10045

ahus1 · 2022-02-07T14:28:14Z

ahus1
Feb 7, 2022
Collaborator

Problem statement

Map Storage currently retrieves entities by ID using read(String key)

An LDAP Map store will have separate LDAP directories based on the realm being queried. The realm is not part of the signature.

Therefore the LDAP Map store will not have the realm to search for the key at first glance.

Approach

The ID of an entity could be a string containing the realm as a prefix, and the UUID of the entity as a suffix:

REALM#UUID

The LDAP Map storage will first parse the key to identify the realm. Then it knowns where to look up the UUID.

Benefits

API of map storage can stay the same.

Alternatives

Change the API to include the realm

ahus1 · 2022-02-07T16:25:41Z

ahus1
Feb 7, 2022
Collaborator Author

Some second thoughts: storing the realm in the key might work when the element was retrieved from the LDAP store before. It might cause problems in a tree store scenario, when the key would be the same for all stores in the tree (I assume), and different stores want to store different additional information in the key.

1 reply

ahus1 Feb 7, 2022
Collaborator Author

Found that "#" is a bad character as the old admin UI uses the IDs to create URLs from them, and therefore it shouldn't contain anything that could mess up a URL. Using a "." instead.

ahus1 · 2022-02-08T18:00:27Z

ahus1
Feb 8, 2022
Collaborator Author

A similar problem exists on how to find the client when a client role is retrieved by ID.

0 replies

hmlnarik · 2022-02-10T10:27:11Z

hmlnarik
Feb 10, 2022

The realm data should be strictly separated in a storage for safety reasons. Realm in LDAP view probably best resembles a dc or ou.

Hence it should not be part of the user ID. The question is very valid though: we need to obtain the realm upon query.

Let's first follow this path:

KeycloakSession contains reference to context of the request.
The context contains reference to the "current realm". Note that in exceptional cases, this might be null.
MapKeycloakTransaction is always instantiated in the context of a particular KeycloakSession, where it can obtain the realm from.

There is a case though when this won't work is when a user is logged in the admin realm and administering other realms. In that case, the realm from the context would be the admin realm rather than the one being edited.

Hence the operations within MapKeycloakTransaction including read cannot refer to the current realm obtained from that transaction.

Hence we need a step back. Facts:

LDAP configuration is either system-wide or per-realm (component)
In either case, it targets the same LDAP server (shares the connection parameters)
LDAP's GUID is unique in the same LDAP server
LDAP configuration has a an entry for search base DN
If a user / client is searched by user name / clientID then the read(QueryParameters) method is used where realm ID is known
For read(id), plain GUID search can be used. Once the object is found, realm ID would be derived either from one of its attributes, from its DN, or from the component owner of LDAP configuration.

For the initial implementation, let's stick only with a single predefined realm.

0 replies

ahus1 · 2022-02-10T10:52:09Z

ahus1
Feb 10, 2022
Collaborator Author

Thanks for reviewing this.

In either case, it targets the same LDAP server (shares the connection parameters)

I think this is too restrictive. From an admin's point of view there is no reason to restrict a Keycloak instance to one LDAP server IMHO. Each realm could connect to a separate LDAP server. This is also the case for the current Keycloak configuration, where an admin configures federation on a realm level with connection parameters.

An option could be to iterate over all available realms, and return the data found in the first matching realm with GUID + search base DN. Still I would like to discard this option as this is slow an increases with the number of connected LDAP realms. The edge cases are also non-trivial: this would require GUIDs to be different across LDAPs, and connecting to one LDAP from multiple realms that the search base DNs are disjunctive. The latter might be determined at runtime. Different GUIDs across LDAPs sounds easy for a start up until the point where someone starts to clone an LDAP for a test setup :-(

I'll also open a new discussion on whether to use the GUID or the DN as the ID. We briefly touched on this in #10076. Once I created the first role in LDAP via Keycloak yesterday, this presented some downsides for GUID.

Regarding "The realm data should be strictly separated in a storage for safety reasons [...] Hence it should not be part of the user ID.": We could encrypt the ID used in the storage to make it opaque. [yes, and there are downsides for that: getting encryption right, performance, size of ID]

For now, I'll then hard code it to "master", and keep this discussion open to find a real solution.

1 reply

hmlnarik Feb 10, 2022

I think we're aligned in the requirement on the overall functionality, the capabilities that you described are needed.

However: They will be handled by the tree storage, not by a single map storage implementation.

I think this is too restrictive.

Tree storage handles storage composition, e.g. multiple LDAP map storages. Single LDAP map storage handles a single LDAP server.

ahus1 · 2022-02-24T14:38:20Z

ahus1
Feb 24, 2022
Collaborator Author

Earlier today we discussed in the storage team that the realmId could be added by the MapRoleProvider if the store can't determine it itself when calling getRoleById().

As an alternative, using a criteria query was discussed as a possible solution. This might make the query possibly slower, but would provide the additional information to the store as part of the query.

A draft code example of the to-be code (between START/END) from ModelRoleProvider:

    @Override
    public RoleModel getRoleById(RealmModel realm, String id) {
        if (id == null || realm == null || realm.getId() == null) {
            return null;
        }

        LOG.tracef("getRoleById(%s, %s)%s", realm, id, getShortStackTrace());

        MapRoleEntity entity = tx.read(id);
        String realmId = realm.getId();
        // START: ADDED
        if (entity != null && entity.getRealmId() == null) {
            // when a store doesn't store information about all realms, it doesn't have the information about
            // the associated realm for a given instance. Therefore, provide this from the Map provider's context.
            // TODO: create envelope that adds the realmId, or move it to entityToAdapterFunc()
            entity.setRealmId(realmId);
        }
        // END: ADDED
        return (entity == null || ! Objects.equals(realmId, entity.getRealmId()))
          ? null
          : entityToAdapterFunc(realm).apply(entity);
    }

1 reply

ahus1 Feb 28, 2022
Collaborator Author

Update: A wrapper provided more difficult than at first sight: without a setter the transaction will not know about the realm for this entity. It will then return the roles cached in the transaction via query. When returning them for the query, it will use the realmId filter for the cached entities. That filter will fail, as the cached entities don't contain the realm.

ahus1 · 2022-03-04T07:10:37Z

ahus1
Mar 4, 2022
Collaborator Author

This has now been resolved as follows / cc: @hmlnarik, @mhajas

the store returns the entity with the realmId if it has it hat hand.
MapRoleProvider adds an envelope around the entity for get(id) / 1f62093f0c56940fc96c68e4c7ebb1d0d3061620
when executing read(query) on a transaction, all entities in the transaction are equipped with the realmId so that read(query) always returns entities with the realmId. / 17f6f1fb054c5553c25c0b6a9c9e545acec01835

A previous attempt to use a modified MapModelCriteriaBuilder to search entities in the transaction and ignoring the realmId was discarded as this results in returning entities from read(query) without a realmId that would then need to be wrapped.

0 replies

ahus1 · 2022-03-04T10:08:42Z

ahus1
Mar 4, 2022
Collaborator Author

Another update after a new discussion with @hmlnarik / cc: @mhajas

The LdapRoleMap transaction will return all its entities without a realmId, both for read(key) and read(query). This enables then caching and invalidation in the caching layer above on entities. Backfilling the realmIds would break that, and make even things inconsistent with caching.

This would then require MapRoleAdapter to use the values returned here without a realmId in all places. A first thought has been to wrap all entities returned. Looking deeper the MapRoleAdapter doesn't use the realmId, therefore no envelope is needed. All tests in RoleModelTest pass without an envelop and just an adjusted condition in getRoleById(). A manual test to login to the frontend passes as well.

This is part of commit 6d1517b23b6dc371f96d2d2c348de91ad5eb218f

0 replies

LDAP Map storage / how to map read(String key) to realm? #10045

Uh oh!

ahus1 Feb 7, 2022 Collaborator

Problem statement

Approach

Benefits

Alternatives

Replies: 7 comments · 3 replies

Uh oh!

ahus1 Feb 7, 2022 Collaborator Author

Uh oh!

ahus1 Feb 7, 2022 Collaborator Author

Uh oh!

ahus1 Feb 8, 2022 Collaborator Author

Uh oh!

hmlnarik Feb 10, 2022

Uh oh!

Uh oh!

ahus1 Feb 10, 2022 Collaborator Author

Uh oh!

hmlnarik Feb 10, 2022

Uh oh!

ahus1 Feb 24, 2022 Collaborator Author

Uh oh!

ahus1 Feb 28, 2022 Collaborator Author

Uh oh!

ahus1 Mar 4, 2022 Collaborator Author

Uh oh!

ahus1 Mar 4, 2022 Collaborator Author

ahus1
Feb 7, 2022
Collaborator

Replies: 7 comments 3 replies

ahus1
Feb 7, 2022
Collaborator Author

ahus1 Feb 7, 2022
Collaborator Author

ahus1
Feb 8, 2022
Collaborator Author

hmlnarik
Feb 10, 2022

ahus1
Feb 10, 2022
Collaborator Author

ahus1
Feb 24, 2022
Collaborator Author

ahus1 Feb 28, 2022
Collaborator Author

ahus1
Mar 4, 2022
Collaborator Author

ahus1
Mar 4, 2022
Collaborator Author