Authorization pitfalls: what does Keycloak cloak?
Tags: java, programming, opensource
Published: 2024-07-12 09:11:46

Author: Valerii Filatov 

User authorization and registration are important parts of any application, not only for users but also for security\. What pitfalls does the source code of a popular open\-source identity management solution hide? How do they affect the application?

![](https://import.viva64.com/docx/blog/1142_Keycloak_checking/image1.png)

If you've ever implemented authorization for web apps, you probably know all the frustrating issues that can arise\. I'm no exception, either\.

Once, I implemented the messenger\-based authorization in the frontend part of one project\. It seemed like the world's easiest task but turned out to be the opposite: deadlines were looming, code was tripping over messenger APIs, and people around me were yelling\.

After this case, my colleague showed me a cool tool that would streamline the implementation of authorization in our future projects\. This project was Keycloak, an open\-source Java solution to enable single sign\-on \(SSO\) with identity and access management aimed at modern apps and services\.

As I use the solution myself, I find it interesting to get into the source code and use the PVS\-Studio static analyzer to look for the bugs hidden there\.

## He\-he, classic NullPointerException


> Someone knocked on the door\. The Junior Dev tried to open a door and crashed\. "NullPointerException\!" concluded the Senior Dev\.



Errors related to checking for *null* are encountered in almost every project we've checked before\. So, let's start with this old but gold error\.

**Fragment N1**

```cpp
private void checkRevocationUsingOCSP(X509Certificate[] certs) 
  throws GeneralSecurityException {
    ....    
    if (rs == null) {
      if (_ocspFailOpen)
        logger.warnf(....);
      else
        throw new GeneralSecurityException(....);
    }

    if (rs.getRevocationStatus() == 
      OCSPProvider.RevocationStatus.UNKNOWN) { // <=
        if (_ocspFailOpen)
            logger.warnf(....);
        else
            throw new GeneralSecurityException(....);
    ....
}
```

Warning:

[V6008](https://pvs-studio.com/en/docs/warnings/v6008/) Potential null dereference of 'rs'\.

This code snippet has a *null* check, but we need to understand what's going on inside\. If the *\_oscpFailOpen* variable is true, the program won't throw an exception\. It'll only save the log about it and continue execution—it receives a *NullPointerException* in the following *if*, since the *rs* variable has already been used inside *if*\.

It might seem like the program would just throw another exception if there weren't the *NullPointerException*\. But that's not the case because the *\_oscpFailOpen* variable is true, and the program should continue execution\. No fate, it tripped over the *null* pointer and fell into the exception\.

**Fragment N2**

```cpp
public void writeDateAttributeValue(XMLGregorianCalendar attributeValue) 
  throws ProcessingException {
    ....
    StaxUtil.writeAttribute(
      writer, 
      "xsi", 
      JBossSAMLURIConstants.XSI_NSURI.get(), 
      "type", 
      "xs:" + attributeValue.getXMLSchemaType().getLocalPart()   // <=
    ); 

    if (attributeValue == null) {
      StaxUtil.writeAttribute(
        writer, 
        "xsi", 
        JBossSAMLURIConstants.XSI_NSURI.get(), 
        "nil", 
        "true"
      );
    ....
}
```

Warning*:*

[V6060](https://pvs-studio.com/en/docs/warnings/v6060/) The 'attributeValue' reference was utilized before it was verified against null\.

"Better late than never," this phrase is good enough for many cases, but unfortunately, not for the *null* check\. In the above code snippet, the *attributeValue* object is used before it's checked for existence, which leads to the *NullPointerException*\.


> **Note**\. If you want to check out other examples of such bugs, we've put together [a list](https://pvs-studio.com/en/blog/examples/v6060/) for you\!



## Arguments?

I don't know how it has happened, but Keycloak has errors related to the number of arguments in the format string functions\. It isn't an illustrative statistic—just a fun fact, though\.

**Fragment N3**

```cpp
protected void process() {
  ....
  if (f == null) {
    ....
  } else {
    ....
    if (isListType(f.getType()) && t instanceof ParameterizedType) {
      t = ((ParameterizedType) t).getActualTypeArguments()[0];
      if (!isBasicType(t) && t instanceof Class) {
        ....
        out.printf(", where value is:\n", ts);              // <=
        ....
      }
    } else if (isMapType(f.getType()) && t instanceof ParameterizedType) {
      ....
      out.printf(", where value is:\n", ts);                // <=
      ....
    }
  }
}
```

Warning:

[V6046](https://pvs-studio.com/en/docs/warnings/v6046/) Incorrect format\. A different number of format items is expected\. Arguments not used: 1\.

In the above fragment, when the *printf* function is called, we pass a format string and a value to be substituted into the string\. There's only one problem: there's simply no place in the string to substitute arguments\.

It's pretty interesting that a dev not only copied and pasted both the code fragment from *if* to *else if*, but a dev also copied and pasted the error inside *else if*\.

In the next snippet, we have the opposite case: developers have spared arguments\.

**Fragment N4**

```cpp
public String toString() {
  return String.format(
    "AuthenticationSessionAuthNoteUpdateEvent 
      [ authSessionId=%s,
        tabId=%s,
        clientUUID=%s,
        authNotesFragment=%s ]", 
    authSessionId, 
    clientUUID, 
    authNotesFragment);      // <=
}
```

Warning:

[V6046](https://pvs-studio.com/en/docs/warnings/v6046/) Incorrect format\. A different number of format items is expected\. Missing arguments: 4\.

Although devs passed the *authSessionId*, *clientUUID* and *authNotesFragment* arguments to the function, the fourth *tabld* argument is a bit missed\.

In this case, the *String\.format* method will throw an *IllegalFormatException*, which can be a nasty surprise\.

## Unclean code

The following PVS\-Studio warnings aren't related to code health errors\. These warnings are more about how high\-quality the code is\. I just pointed out that these aren't firm errors\.

"What's the point of looking at them?" you may think\. First, I believe that code should be clean and neat\. It's not a matter of tastes: clean code is not only about the visual experience but also about code readability\. Imho, it's important for any project, especially Open Source\. Secondly, I'd like to show that the PVS\-Studio static analyzer helps not only fix errors in code but also makes it beautiful and clean\.

### Not enough variables, My Lord\! 

For some reason, the following code fragment looks in my mind like an evil plan of someone who has a bad attitude to use variables: "Let's adopt variables and leave them waiting in horror for the garbage collector to gobble them up\.\.\."

**Fragment N5**

```cpp
 private void onUserRemoved(RealmModel realm, String userId) {
    int num = em.createNamedQuery("deleteClientSessionsByUser")
      .setParameter("userId", userId).executeUpdate();             // <=
    num = em.createNamedQuery("deleteUserSessionsByUser")
      .setParameter("userId", userId).executeUpdate();
}
```

Warning:

[V6021](https://pvs-studio.com/en/docs/warnings/v6021/) The value is assigned to the 'num' variable but is not used\.

From the point of program operation, nothing terrible happens in this code snippet\. But it's still not clear why developers wrote that\.

The *num* variable contains the number of set parameters that the *executeUpdate* method returns\. So, I thought that the method might have had a check for changes\. Even if I rewind in time, I'll only find that calls to the method aren't written to a variable but accept the current state a little later\.

The result is a useless assignment to the variable—just like in the next fragment\.

**Fragment N6**

```cpp
private void verifyCodeVerifier(String codeVerifier, String codeChallenge, 
  String codeChallengeMethod) throws ClientPolicyException {
    ....
    String codeVerifierEncoded = codeVerifier;

    try {
      if (codeChallengeMethod != null &&
        codeChallengeMethod.equals(OAuth2Constants.PKCE_METHOD_S256)) {

        codeVerifierEncoded = generateS256CodeChallenge(codeVerifier);

        } else {
          codeVerifierEncoded = codeVerifier; // <=
        }
    } catch (Exception nae) {
      ....
    }
}
```

Warning:

[V6026](https://pvs-studio.com/en/docs/warnings/v6026/) This value is already assigned to the 'codeVerifierEncoded' variable\.

If you look at the code, you can see that before this, the *codeVerifierEncoded* variable has already assigned the same value in the *else* branch\. A developer just did redundant action: and useless, and overcomplicating\.

The next code fragment just amuses me\.

**Fragment N7**

```cpp
private static Type[] extractTypeVariables(Map<String, Type> typeVarMap, 
  Type[] types){
    for (int j = 0; j < types.length; j++){
      if (types[j] instanceof TypeVariable){
        TypeVariable tv = (TypeVariable) types[j];
        types[j] = typeVarMap.get(tv.getName());
      } else {
        types[j] = types[j];     // <=
      }
    }
    return types;
}
```

Warning: 

[V6005](https://pvs-studio.com/en/docs/warnings/v6005/) The variable 'types\[j\]' is assigned to itself\.

It looks just like the previous snippet, but honestly, I'm totally lost on what this code is trying to do\.

At first, I thought we were facing a nested loop here, and the author had just mixed up the variables *i* and *j*\. But eventually I realized that there's only one loop here\.

I also thought the assignment appeared when developers were refactoring the code, which might have been even more complicated before\. In the end, I found that the function was originally created this way \([commit](https://github.com/keycloak/keycloak/commit/72d134748cc200108a41075b209aaabb27d96d09)\)\.

### So sweet copy\-paste

Copy\-paste errors are quite common\. I'm sure you've seen them even in your own code\.

Keycloak has some traces of the copy\-paste use, too\.

**Fragment 8**

```cpp
public class IDFedLSInputResolver implements LSResourceResolver {
  ....

  static {
    ....
    schemaLocations.put("saml-schema-metadata-2.0.xsd", 
      "schema/saml/v2/saml-schema-metadata-2.0.xsd");
    schemaLocations.put("saml-schema-x500-2.0.xsd", 
      "schema/saml/v2/saml-schema-x500-2.0.xsd");
    schemaLocations.put("saml-schema-xacml-2.0.xsd", 
      "schema/saml/v2/saml-schema-xacml-2.0.xsd");

    schemaLocations.put("saml-schema-xacml-2.0.xsd", 
      "schema/saml/v2/saml-schema-xacml-2.0.xsd");          // <=

    schemaLocations.put("saml-schema-authn-context-2.0.xsd", 
      "schema/saml/v2/saml-schema-authn-context-2.0.xsd");
    ....
  }
  ....
}
```

Warning*:*

[V6033](https://pvs-studio.com/en/docs/warnings/v6033/) An item with the same key '"saml\-schema\-xacml\-2\.0\.xsd"' has already been added\.

Honestly, even though I knew there was a typo in the source code, I had a hard time finding it right away in the code\.

If you notice, in the *schemaLocations\.put* method calls, the passed arguments are quite similar\. So, I assume that the dev who wrote this code simply copied a string as a template and then just changed values\. The problem is that during copy\-pasting, one line that repeats the previous one has crept into the project\.

Such "typos" can either lead to serious consequences or have no effect at all\. This copy\-pasting example has been in the project since November 21, 2017 \([commit](https://github.com/keycloak/keycloak/commit/a993f6fb7545d24fb9e7b868bfe48ea66bc47543)\), and I don't think it causes any serious problems\.

We're including this error in the article to remind developers to be careful when copying and pasting code fragments and to keep an eye on any changes in code\. Want to read more about it? Here's [the article](https://pvs-studio.com/en/blog/posts/csharp/0708/) about the copy\-paste typos\.

### Unreachable code

The headline gives us a little clue as to what kind of warning awaits us in the following snippet\. I suggest you use your detective skills to spot the flaw yourself\.

**Fragment N9**

```cpp
public void executeOnEvent(ClientPolicyContext context) 
  throws ClientPolicyException {
    switch (context.getEvent()) {
      case REGISTER:
      case UPDATE:
        ....
      case RESOURCE_OWNER_PASSWORD_CREDENTIALS_REQUEST:
        ....
        executeOnAuthorizationRequest(ropcContext.getParams());
        return;
      default:
        return;
    }
}
```

It's not that easy to detect, isn't it? I'm not gloating over you, I just give you a chance to roughly access the situation\. I show it because a small flaw makes it harder for the developer to find the error without examining the rest of the code\. To find out what error is lurking in this code snippet, we need to look at what is cloaked in the *executeOnAuthorizationRequest* method:

```cpp
private void executeOnAuthorizationRequest(MultivaluedMap<String, 
  String> params) throws ClientPolicyException {
    ....
    throw new ClientPolicyException(....);
}
```

Yes, this method throws an exception\. That is, all the code written after calling this method will be unreachable—the PVS\-Studio analyzer detected it\.

```cpp
public void executeOnEvent(ClientPolicyContext context) 
  throws ClientPolicyException {
    switch (context.getEvent()) {
      case REGISTER:
      case UPDATE:
        ....
      case RESOURCE_OWNER_PASSWORD_CREDENTIALS_REQUEST:
        ....
        executeOnAuthorizationRequest(ropcContext.getParams());
        return;       // <=
      default:
        return;
    }
}
```

Warning:

[V6019](https://pvs-studio.com/en/docs/warnings/v6019/) Unreachable code detected\. It is possible that an error is present\.

Even if this flaw is quite small, a similar case could lead to more serious consequences\. I can only note here that a static analyzer will help you avoid such unpleasant things\.

### I dictate CONDITION

Now, let's look at conditional statements and cases when they execute their code\.

**Fragment N10**

```cpp
public boolean validatePassword(AuthenticationFlowContext context, 
  UserModel user, MultivaluedMap<String, String> inputData, 
  boolean clearUser) {
  
    ....

    if (password == null || password.isEmpty()) {
      return badPasswordHandler(context, user, clearUser,true);
    }

    ....

    if (password != null && !password.isEmpty() &&    // <=
      user.credentialManager()
        .isValid(UserCredentialModel.password(password))) { 
      ....
    }
}
```

Warning:

[V6007](https://pvs-studio.com/en/docs/warnings/v6007/) Expression 'password \!= null' is always true\.

[V6007](https://pvs-studio.com/en/docs/warnings/v6007/) Expression '\!password\.isEmpty\(\)' is always true\.

Here are two warnings in one line\! What does the analyzer warn us about? The first conditional statement checks that the password is non\-*null* and non\-empty\. If the opposite occurs, the function is no longer executed\. In the line the analyzer highlighted, both of these checks are repeated, so the conditions are always true\.

On the one hand, it's better to check than not to check\. On the other, such duplicates may lead to missing the part of the condition that may be equal to *false*—it can really affect the program operation\.

Let's repeat the exercise\.

**Fragment N11**

```cpp
public KeycloakUriBuilder schemeSpecificPart(String ssp)
  throws IllegalArgumentException {
    if (ssp == null) 
      throw new IllegalArgumentException(....);
    ....
    if (ssp != null) // <=
      sb.append(ssp);
    ....
}
```

Warning:

[V6007](https://pvs-studio.com/en/docs/warnings/v6007/) Expression 'ssp \!= null' is always true\.

In general, the case is the same\. If the *ssp* variable is *null*, the program throws an exception\. So, the condition below isn't only true all the time but is also redundant because the corresponding code block will always be executed\.

The condition in the next fragment is also redundant\.

**Fragment N12**

```cpp
protected String changeSessionId(HttpScope session) {
  if (!deployment.turnOffChangeSessionIdOnLogin()) 
    return session.getID();     // <=
  else return session.getID();
}
```

Warning:

[V6004](https://pvs-studio.com/en/docs/warnings/v6004/) The 'then' statement is equivalent to the 'else' statement\.

In this method, the same code is executed under different seasons, the moon phases, and, most importantly, under different conditions\. So, again, the condition is redundant here\.

Digging into the code, I found the code snippet that is like two drops of water similar to the one above:

```cpp
protected String changeSessionId(HttpSession session) {
  if (!deployment.turnOffChangeSessionIdOnLogin()) 
    return ChangeSessionId.changeSessionId(exchange, false);
  else return session.getId();
}
```

As you can see, when the condition is executed, the method that changes the session ID is called\.

So, we can make two guesses: either devs just copied the code and changed the condition result, or the first condition still should have updated the session, and this error goes way beyond the "sloppy" code\.

But we mustn't live by redundant conditions alone**\!**

**Fragment N13**

```cpp
static String getParameter(String source, String messageIfNotFound) {
  Matcher matcher = PLACEHOLDER_PARAM_PATTERN.matcher(source);

  while (matcher.find()) {
    return matcher.group(1).replaceAll("'", ""); // <=
  }

  if (messageIfNotFound != null) {
    throw new RuntimeException(messageIfNotFound);
  }

  return null;
}
```

Warning:

[V6037](https://pvs-studio.com/en/docs/warnings/v6037/) An unconditional 'return' within a loop\.

I have a feeling this *while *was raised by *ifs*\. This code may have some hidden intentions, but the analyzer and I see the same thing here: a loop that always performs one iteration\.

The code author might have wanted a different behavioral outcome\. Even if they don't want, we'll find this code a bit harder to understand than if there is just a conditional operator\.

## String comparison

In the following snippet, a developer suggests everything is so easy\. But it turns out it's not\.

**Fragment N14**

```cpp
public boolean equals(Object o) {
  if (this == o) return true;
  if (o == null || getClass() != o.getClass()) return false;

  Key key = (Key) o;

  if (action != key.action) return false;         // <=
  ....
}
```

Warning:

[V6013](https://pvs-studio.com/en/docs/warnings/v6013/) Strings 'action' and 'key\.action' are compared by reference\. Possibly an equality comparison was intended\.

Comparing strings implies that we compare their contents\. We actually compare object references\. This also applies to arrays and collections, not only strings\. I think it's clear that in certain cases, code operations can lead to unexpected consequences\. Most importantly, it's pretty easy to fix such an error:

```cpp
public boolean equals(Object o) {
  ....
  if (!action.equals(key.action)) return false;
  ....
}
```

The *equals* method returns a comparison exactly to the contents of two strings\. Victory\!

I'll draw your attention to the fact that the static analyzer has detected the error, which a developer would most likely have missed when reviewing the code\. You can read about this and other reasons for using static analysis in this [article](https://pvs-studio.com/en/blog/posts/0687/)\.

## Double\-checked locking

Double\-checked locking is a parallel design pattern used to reduce the overhead of acquiring a lock\. We first check the lock condition without synchronization\. If it's encountered, the thread tries to acquire the lock\.

If we streamline it, this pattern helps get the lock only when it's actually needed\.

I think you've already guessed that there are bugs in the implementation of this template as I've started talking about it\. Actually, they are\.

**Fragment N15**

```cpp
public class WelcomeResource {
  private AtomicBoolean shouldBootstrap;

  ....

  private boolean shouldBootstrap() {
    if (shouldBootstrap == null) {
      synchronized (this) {
        if (shouldBootstrap == null) {
          shouldBootstrap = new AtomicBoolean(....);
        }
      }
    }
    return shouldBootstrap.get();
  }
```

Warning:

[V6082](https://pvs-studio.com/en/docs/warnings/v6082/) Unsafe double\-checked locking\. The field should be declared as volatile\.

The analyzer warns that the *shouldBootstrap* field doesn't have the *volatile* modifier\. What does it affect? In such code, it's likely that different threads use an object until they're fully initialized\.

This fact doesn't seem to be that significant, does it? In the next example, the compiler may change the action order with non\-*volatile* fields\.

**Fragment N16**

```cpp
public class DefaultFreeMarkerProviderFactory 
  implements FreeMarkerProviderFactory {

    private DefaultFreeMarkerProvider provider;   // <=

    ....

    public DefaultFreeMarkerProvider create(KeycloakSession session) {
      if (provider == null) {
        synchronized (this) {
          if (provider == null) {
            if (Config.scope("theme").getBoolean("cacheTemplates", true)) {
              cache = new ConcurrentHashMap<>();
            }
            kcSanitizeMethod = new KeycloakSanitizerMethod();
            provider = new DefaultFreeMarkerProvider(cache, kcSanitizeMethod);
          }
        }
      }
      return provider;
    }
}
```

Warning:

[V6082](https://pvs-studio.com/en/docs/warnings/v6082/) Unsafe double\-checked locking\. The field should be declared as volatile\.

Why don't developers fix these code fragments if the bug is so dangerous? The error is not only dangerous but also sneaky\. Everything works as it should most of the time\. There are lots of different reasons why bad behavior can occur, due to, let's say, used JVM or the thread scheduler operations\. So, it can be quite hard to reproduce the error conditions\.

You can read more about such errors and their reasons in the [article](https://pvs-studio.com/en/blog/posts/java/1128/)\.

## Conclusion

At the end of this article, I'd like to point out that I used the analyzer a little out of order\. I just wanted to entertain you and show you the bugs in the project\. However, to fix errors and prevent them, it's better to use the static analyzer regularly while writing code\. You can read more about it [here](https://pvs-studio.com/en/blog/posts/0669/)\. 

However, the analyzer helped us spot various errors related both to the program operation and insufficient code cleanliness \(I still think it's important, and you'll hardly change my mind\)\. Errors are not the end of the world if you spot and fix them in time\. Static analysis helps with this\. Try PVS\-Studio and use it to check your project for [free](https://pvs-studio.com/en/pvs-studio/try-free/?utm_source=website&utm_medium=devto&utm_campaign=article&utm_content=1142)\.