Potential bug introduced to JavaKeystoreKeyProvider in #26936

[sps-campbellwray]

Before reporting an issue

• I have read and understood the above terms for submitting issues, and I understand that my issue may be closed without action if I do not follow them.

Area

admin/api

Describe the bug

Hi,

I am one of the maintainers of the https://github.com/silentpartnersoftware/Keycloak.Net/ repo, which provides a .NET client for interacting with the Keycloak API.

Starting in version 24 one of our users noticed that the serverinfo endpoint is no longer parsing correctly. I have traced this back to the "Intended algorithm for the key" property which used to have a list of options, with a default value of "RS256" but now has a list of default values and no options.

v23.0.4

v24.0.4

I am not sure if this change was intentional or not, but it appears as though this change was likely introduced in the PR:
"Support EC Key-Imports for the JavaKeystoreKeyProvider #26936" (#27030)

Specifically at this line:

    // merge the algorithms supported for RSA and EC keys and provide them as one configuration property
    private static ProviderConfigProperty mergedAlgorithmProperties() {
        List<String> ecAlgorithms = List.of(Algorithm.ES256, Algorithm.ES384, Algorithm.ES512);
        List<String> algorithms = Stream.concat(Attributes.RS_ALGORITHM_PROPERTY.getOptions().stream(), ecAlgorithms.stream()).toList();
        return new ProviderConfigProperty(Attributes.ALGORITHM_KEY, "Algorithm", "Intended algorithm for the key", LIST_TYPE, algorithms.toArray());

    }

I believe that the incorrect ProviderConfigProperty constructor was called, and instead of passing in a defaultValue, followed by the options, the defaultValue was omitted and the options became the defaultValue. The type for the defaultValue parameter is Object, so it would be possible to accidentally pass in a List instead of a String.

The constructor used was the one on line 84, but I suspect the intention was to use the one on line 92

If this change was intentional then this issue can be closed, if not it would be appreciated if this could be resolved.

Thanks,
Campbell

Version

24.0.4

Regression

• The issue is a regression

Expected behavior

"Intended algorithm for the key" should have a defaultValue and options

Actual behavior

"Intended algorithm for the key" options are unintentionally stored in the defaultValue property, and no options (or correct defaultValue) are provided

How to Reproduce?

Call the /admin/serverinfo endpoint and look at the structure of "Intended algorithm for the key" in both 23.0.4 and 24.0.4.

Path componentTypes['org.keycloak.keys.KeyProvider'][4].properties[3].defaultValue

Anything else?

No response

[rmartinc]

Hi @sps-campbellwray!

No, it was not intentional, the list is passed as the default value instead of the options. I think it should be:

--- a/services/src/main/java/org/keycloak/keys/JavaKeystoreKeyProviderFactory.java
+++ b/services/src/main/java/org/keycloak/keys/JavaKeystoreKeyProviderFactory.java
@@ -119,7 +119,9 @@ public class JavaKeystoreKeyProviderFactory implements KeyProviderFactory {
     private static ProviderConfigProperty mergedAlgorithmProperties() {
         List<String> ecAlgorithms = List.of(Algorithm.ES256, Algorithm.ES384, Algorithm.ES512);
         List<String> algorithms = Stream.concat(Attributes.RS_ALGORITHM_PROPERTY.getOptions().stream(), ecAlgorithms.stream()).toList();
-        return new ProviderConfigProperty(Attributes.ALGORITHM_KEY, "Algorithm", "Intended algorithm for the key", LIST_TYPE, algorithms.toArray());
+        return new ProviderConfigProperty(Attributes.RS_ALGORITHM_PROPERTY.getName(), Attributes.RS_ALGORITHM_PROPERTY.getLabel(),
+                Attributes.RS_ALGORITHM_PROPERTY.getHelpText(), Attributes.RS_ALGORITHM_PROPERTY.getType(),
+                Attributes.RS_ALGORITHM_PROPERTY.getDefaultValue(), algorithms.toArray(String[]::new));
 
     }

Can you please try with that change and send a PR if OK? I don't think we need a test for this. If you don't want to send the PR I'll try to do it in a few days.

[keycloak-github-bot]

Due to the amount of issues reported by the community we are not able to prioritise resolving this issue at the moment.

If you are affected by this issue, upvote it by adding a 👍 to the description. We would also welcome a contribution to fix the issue.

[sps-campbellwray]

Hi @rmartinc

Thanks for looking into this, unfortunately I mostly do C# development, and don't have much experience with Java, so I don't really have any tools set up for building and testing the change you suggested. For this reason I am not really comfortable opening the PR as I don't have any way to check that it builds or runs.

With that said, I looked at the change that you suggested and I would agree that I looks like the correct approach so long as it's acceptable to use the RS_ALGORITHM_PROPERTY attributes now that there are also EC algorithms in there too.

I can also see that you made a commit for this a few hours ago, so I guess you found some time to work on it.

Thanks again, your help with this issue is much appreciated,
Campbell