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

Keycloak backend plugin leads to DOS of Keycloak server #2471

Open
JohannesWill opened this issue Oct 31, 2024 · 11 comments
Open

Keycloak backend plugin leads to DOS of Keycloak server #2471

JohannesWill opened this issue Oct 31, 2024 · 11 comments
Labels

Comments

@JohannesWill
Copy link

Describe the bug

I have setup the Keycloak backend plugin by following the steps given on below this link - https://janus-idp.io/plugins/keycloak/ .

Syncing the users and Groups will overload our Keycloak instance by doing many parrallel requests

Expected Behavior

  • All users and groups from keyclock realm should be synced
  • The Keycloak server should not experience a DoS (Denial of Service).

What are the steps to reproduce this bug?

  1. Keycloak with many users and groups (in my case ~2500 users and 850 groups)
  2. Configure the Keycloak plugin for new backup configuration (using steps from https://janus-idp.io/plugins/keycloak/)

Versions of software used and environment
@janus-idp/backstage-plugin-keycloak-backend": "^2.0.8",

Backstage - 1.30.4 ([email protected])

Temporary Workaround:
During investigation of that problem I used patch-package to patch @janus-idp/[email protected] for the project I'm working on.
With that pacht, where I serialized the calls to Keycloak, the problem is not reproduceable anymore.

Here is the diff that solved my problem:

diff --git a/node_modules/@janus-idp/backstage-plugin-keycloak-backend/dist/index.cjs.js b/node_modules/@janus-idp/backstage-plugin-keycloak-backend/dist/index.cjs.js
index a0e323e..180abfc 100644
--- a/node_modules/@janus-idp/backstage-plugin-keycloak-backend/dist/index.cjs.js
+++ b/node_modules/@janus-idp/backstage-plugin-keycloak-backend/dist/index.cjs.js
@@ -153,17 +153,19 @@ async function getEntities(entities, config, logger, entityQuerySize = KEYCLOAK_
   const rawEntityCount = await entities.count({ realm: config.realm });
   const entityCount = typeof rawEntityCount === "number" ? rawEntityCount : rawEntityCount.count;
   const pageCount = Math.ceil(entityCount / entityQuerySize);
-  const entityPromises = Array.from(
-    { length: pageCount },
-    (_, i) => entities.find({
-      realm: config.realm,
-      max: entityQuerySize,
-      first: i * entityQuerySize
-    }).catch(
-      (err) => logger.warn("Failed to retieve Keycloak entities.", err)
-    )
-  );
-  const entityResults = (await Promise.all(entityPromises)).flat();
+  const entityResults = [];
+  for (let i = 0; i < pageCount; i++) {
+    try {
+      const entitiesPage = await entities.find({
+        realm: config.realm,
+        max: entityQuerySize,
+        first: i * entityQuerySize
+      });
+      entityResults.push(...entitiesPage);
+    } catch (err) {
+      logger.warn("Failed to retrieve Keycloak entities.", err);
+    }
+  }
   return entityResults;
 }
 async function getAllGroupMembers(groups, groupId, config, options) {
@@ -254,35 +256,36 @@ const readKeycloakRealm = async (client, config, logger, options) => {
       []
     );
   }
-  const kGroups = await Promise.all(
-    rawKGroups.map(async (g) => {
-      g.members = await getAllGroupMembers(
-        client.groups,
-        g.id,
-        config,
-        options
-      );
-      if (isVersion23orHigher) {
-        if (g.subGroupCount > 0) {
-          g.subGroups = await client.groups.listSubGroups({
-            parentId: g.id,
-            first: 0,
-            max: g.subGroupCount,
-            briefRepresentation: false,
-            realm: config.realm
-          });
-        }
-        if (g.parentId) {
-          const groupParent = await client.groups.findOne({
-            id: g.parentId,
-            realm: config.realm
-          });
-          g.parent = groupParent?.name;
-        }
+  const kGroups = [];
+  for (const g of rawKGroups) {
+    g.members = await getAllGroupMembers(
+      client.groups,
+      g.id,
+      config,
+      options,
+    );
+
+    if (isVersion23orHigher) {
+      if (g.subGroupCount > 0) {
+        g.subGroups = await client.groups.listSubGroups({
+          parentId: g.id,
+          first: 0,
+          max: g.subGroupCount,
+          briefRepresentation: false,
+          realm: config.realm,
+        });
+
+      if (g.parentId) {
+        const groupParent = await client.groups.findOne({
+          id: g.parentId,
+          realm: config.realm,
+        });
+        g.parent = groupParent?.name;
       }
-      return g;
-    })
-  );
+    }
+
+    kGroups.push(g);
+  }
   const parsedGroups = await kGroups.reduce(
     async (promise, g) => {
       const partial = await promise;

This issue body was partially generated by patch-package.

@github-actions github-actions bot added the jira label Oct 31, 2024
@JohannesWill
Copy link
Author

I have prepared a fix for the issue.
Should I fix it in this repo or https://github.com/backstage/community-plugins/tree/main/workspaces/keycloak
@04kash @AndrienkoAleksandr

@04kash
Copy link
Member

04kash commented Nov 15, 2024

Hi @JohannesWill, this plugin has been migrated to https://github.com/backstage/community-plugins/tree/main/workspaces/keycloak and won't be maintained in this repository, so feel free to open a PR with your fix there!

@04kash
Copy link
Member

04kash commented Nov 15, 2024

Additionally, could you raise this bug in the https://github.com/backstage/community-plugins repo and close this one?

@PatAKnight
Copy link
Member

@JohannesWill I opened up a PR and set you as author but I wanted to double check with you on a couple of things right quick as I haven't been able to reproduce the problem, yet.

First, I see that you mentioned using @janus-idp/backstage-plugin-keycloak-backend": "^2.0.8", is there any way you can confirm the version / try a newer version of the keycloak backend plugin? We currently have "@backstage-community/plugin-catalog-backend-module-keycloak": "3.2.2" out as our latest version.

Second, I tested your changes with just 2500 users first, as that was easier for me to create at the time, and didn't notice any issues with the Keycloak server. I was also able to read them in relatively quickly. Could you give me a breakdown of how the groups look in your particular setup? Something along the lines of how many users on average are a part of a group.

Finally, is there anyway you could provide info on how often you are making the sync requests with the app config value catalog.providers.keycloakOrg.key.schedule?

@PatAKnight
Copy link
Member

Another update, as I continue to attempt to reproduce. At the moment, I do not see any apparent issues with my Keycloak server during ingestion. My most recent test involved 2550 users and 850 groups where every user was assigned to every group. In the logs I can see:

catalog info Read 2550 Keycloak users and 850 Keycloak groups in 7.3 seconds. Committing... 
catalog info Committed 2550 Keycloak users and 850 Keycloak groups in 0.8 seconds.

Could you also provide what version of Keycloak you are using.

@JohannesWill
Copy link
Author

We are using Keycloak v20.0.3

@PatAKnight
Copy link
Member

Were you able to test with a new version of the Keycloak plugin? I am not seeing any issues whenever I use "@backstage-community/plugin-catalog-backend-module-keycloak": "3.2.2"

@JohannesWill
Copy link
Author

I tested with "@backstage-community/plugin-catalog-backend-module-keycloak": "3.2.2" and I observe the same as with
@janus-idp/backstage-plugin-keycloak-backend": "^2.0.8",

@PatAKnight
Copy link
Member

So strange, can you explain some of the setup for your users and groups? Are there any subgroups, how many users to a group, and so on. Are there any logs? Either the above that I shown in a comment or any errors?

Sorry for all of the questions. For a little bit of background, we are having a hard time reproducing at the moment and the changes that are suggested are leading to timeout errors when we test them on our end. So, we are trying to figure out a fix that works for you and doesn't lead to any other problems.

@AndrienkoAleksandr
Copy link
Collaborator

I think @JohannesWill reworked the code to use sequential requests instead of parallel operations by utilizing Promise.all().
@PatAKnight, perhaps we could adjust the fix to support both parallel and sequential modes? By default, we can use parallel mode, but if needed, users could enable sequential fetching through plugin configuration.

Also, I'm considering how to reproduce the issue. Maybe the Keycloak server has limited CPU or memory in the deployment, which prevents it from handling a large number of HTTP requests. This could be the key to reproducing the issue...

@JohannesWill
Copy link
Author

My patch was only a quick fix.
Maybe we should consider to make the level of concurency configurable
e.g. add opional parameter maxConcurrency to KeycloakProviderConfig
and use p-limit

import pLimit from 'p-limit';
//...

 const limit = pLimit(config.maxConcurrency ?? Number.POSITIVE_INFINITY);

  // The next line acts like range in python
  const entityPromises = Array.from({ length: pageCount }, (_, i) =>
    limit(() =>
      entities
        .find({
          realm: config.realm,
          max: entityQuerySize,
          first: i * entityQuerySize,
        })
        .catch(err => {
          logger.warn('Failed to retrieve Keycloak entities.', err);
          return [];
        }),
    ),
  );

and

const limit = pLimit(config.maxConcurrency ?? Number.POSITIVE_INFINITY);

  const kGroups = await Promise.all(
    rawKGroups.map(g =>
      limit(async () => {
        g.members = await getAllGroupMembers(
          client.groups as Groups,
          g.id!,
          config,
          options,
        );

        if (isVersion23orHigher) {
          if (g.subGroupCount! > 0) {
            g.subGroups = await client.groups.listSubGroups({
              parentId: g.id!,
              first: 0,
              max: g.subGroupCount,
              briefRepresentation: false,
              realm: config.realm,
            });
          }
          if (g.parentId) {
            const groupParent = await client.groups.findOne({
              id: g.parentId,
              realm: config.realm,
            });
            g.parent = groupParent?.name;
          }
        }

        return g;
      }),
    ),
  );
`

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

Successfully merging a pull request may close this issue.

4 participants