fix: guard against NPE when hashIterations/OTP fields are null in deprecated credentials import by nandinisharma3120 · Pull Request #51008 · keycloak/keycloak

nandinisharma3120 · 2026-07-18T06:44:23Z

Summary

When importing users via the deprecated credentials JSON format, convertDeprecatedCredentialsFormat() passes CredentialRepresentation fields directly to constructors that expect primitive int parameters. Since these fields are declared as Integer (nullable boxed type) in CredentialRepresentation, Java auto-unboxing throws NullPointerException when any field is absent from the import payload.

Root Cause

CredentialRepresentation.getHashIterations() (and getDigits(), getCounter(), getPeriod()) returns Integer, which is nullable. The PasswordCredentialData and OTPCredentialData constructors both take primitive int — auto-unboxing a null Integer to int causes the NPE reported in #41640.

Fix

Null-safe extraction with sensible defaults before constructing the credential data objects:

Password credentials: hashIterations defaults to -1, which is Keycloak's existing sentinel value meaning "use provider default" (consistent with PasswordCredentialProvider and PasswordPolicy).
OTP credentials: digits defaults to 6, counter to 0, period to 30 — standard TOTP/HOTP defaults. This proactively fixes the same NPE pattern in the OTP path.

How to reproduce (before fix)

POST to /admin/realms/{realm}/users with:

{
  "username": "testuser",
  "credentials": [{
    "type": "password",
    "hashedSaltedValue": "someHash",
    "salt": "someSalt"
  }]
}

Previously throws NPE at RepresentationToModel.java:261, now succeeds.

…d credentials import When importing users via the deprecated 'credentials' JSON format, CredentialRepresentation fields (hashIterations, digits, counter, period) are nullable Integer types. Auto-unboxing a null Integer to primitive int throws NullPointerException when any field is absent from the payload. Fix: null-safe extraction with sensible defaults before constructing PasswordCredentialData (-1 for hashIterations — Keycloak's existing "use provider default" sentinel) and OTPCredentialData (6 digits, 0 counter, 30s period — standard TOTP/HOTP defaults). Fixes keycloak#41640 Signed-off-by: Nandini Sharma <46066660+nandinisharma3120@users.noreply.github.com>

Copilot

Pull request overview

Prevents null auto-unboxing during deprecated credential imports.

Changes:

Defaults missing password hash iterations.
Defaults missing OTP digits, counter, and period.


                        if (PasswordCredentialModel.TYPE.equals(cred.getType()) || PasswordCredentialModel.PASSWORD_HISTORY.equals(cred.getType())) {
-                            PasswordCredentialData credentialData = new PasswordCredentialData(cred.getHashIterations(), cred.getAlgorithm());
+                            int hashIterations = cred.getHashIterations() != null ? cred.getHashIterations() : -1;


+                            int hashIterations = cred.getHashIterations() != null ? cred.getHashIterations() : -1;
+                            PasswordCredentialData credentialData = new PasswordCredentialData(hashIterations, cred.getAlgorithm());


nandinisharma3120 · 2026-07-18T09:37:36Z

+                            int digits = cred.getDigits() != null ? cred.getDigits() : 6;
+                            int counter = cred.getCounter() != null ? cred.getCounter() : 0;
+                            int period = cred.getPeriod() != null ? cred.getPeriod() : 30;


Fixed. Added createUserWithDeprecatedOtpCredentialsMissingFields() in UserCreateTest.java which imports a deprecated TOTP credential with all optional fields omitted (digits, counter, period, algorithm) and asserts the stored values match the defaults: digits=6, counter=0, period=30, algorithm=HmacOTP.DEFAULT_ALGORITHM. The RepresentationToModel change now defaults the algorithm to HmacOTP.DEFAULT_ALGORITHM when cred.getAlgorithm() is null, so Mac.getInstance() will never receive null.

… hashIterations Addresses Copilot review comment (line 261) on PR keycloak#51008: - Changed method signature to accept RealmModel realm - When hashIterations is null, fall back to realm.getPasswordPolicy().getHashIterations() - If realm policy returns -1, skip the credential explicitly with a warning - Updated call site in createCredentials() to pass the realm parameter Signed-off-by: Nandini Sharma <46066660+nandinisharma3120@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (4)

server-spi-private/src/main/java/org/keycloak/models/utils/RepresentationToModel.java:265

hashIterations describes how the existing hash was produced, so substituting the destination realm's current policy silently makes imported passwords unverifiable whenever the source used a different iteration count. Missing hash metadata should be rejected/skipped rather than guessed, unless the deprecated format defines a source-algorithm default that can be derived reliably.

                                int policyIterations = realm.getPasswordPolicy().getHashIterations();

server-spi-private/src/main/java/org/keycloak/models/utils/RepresentationToModel.java:270

continue only exits this conversion loop; createCredentials() subsequently iterates the unchanged representation and stores it with null credential/secret data, so the warning's claim that it is skipped is false and authentication later encounters a malformed credential. Remove the rejected representation from the credentials passed to the creation loop, or return an explicit conversion result that the caller filters.

                                    logger.warnf("Cannot convert deprecated password credential for user '%s': " +
                                            "'hashIterations' is absent and no realm password policy provides a default. " +
                                            "The credential will be skipped.", user.getUsername());
                                    continue;

server-spi-private/src/main/java/org/keycloak/models/utils/RepresentationToModel.java:266

Add a regression test beside UserCreateTest.createUserWithDeprecatedCredentialsFormat() for an omitted hashIterations, including a realm without an explicit iteration policy, and assert the resulting credential behavior. The reported NPE path and this new fallback/skip branch are currently untested.

                            if (cred.getHashIterations() != null) {
                                hashIterations = cred.getHashIterations();
                            } else {
                                int policyIterations = realm.getPasswordPolicy().getHashIterations();
                                if (policyIterations < 0) {

server-spi-private/src/main/java/org/keycloak/models/utils/RepresentationToModel.java:282

The new null defaults for deprecated OTP credentials have no regression coverage. Add an import test with omitted digits, counter, and period that verifies the persisted OTP data and successful validation for both TOTP/HOTP as applicable.

                            int digits = cred.getDigits() != null ? cred.getDigits() : 6;
                            int counter = cred.getCounter() != null ? cred.getCounter() : 0;
                            int period = cred.getPeriod() != null ? cred.getPeriod() : 30;

…ed credentials import Addresses Copilot review comments 2 and 3 on PR keycloak#51008: - Test for null hashIterations: sets realm policy to 30000 iterations, imports without hashIterations, verifies 30000 stored - Test for null OTP fields: imports TOTP without digits/counter/period, verifies standard defaults (6/0/30) Signed-off-by: Nandini Sharma <46066660+nandinisharma3120@users.noreply.github.com>

nandinisharma3120 · 2026-07-18T08:48:48Z

Thanks for the thorough review! I've pushed two follow-up commits addressing all three points:

Comment 1 (line 261 — invalid -1 default)
Changed convertDeprecatedCredentialsFormat to accept a RealmModel realm parameter and updated the call site in createCredentials() to pass it. When hashIterations is null, the method now calls realm.getPasswordPolicy().getHashIterations() to resolve the actual default. If the realm policy also returns -1 (no iterations configured), the credential is skipped explicitly with a warning rather than storing an invalid value that PBEKeySpec would reject.

Comments 2 & 3 (missing tests)
Added two new @Test methods to UserCreateTest:

createUserWithDeprecatedCredentialsFormatMissingHashIterations: sets realm policy to hashIterations(30000), imports a deprecated password credential without hashIterations, and asserts the stored credential has 30000 iterations.
createUserWithDeprecatedOtpCredentialsMissingFields: imports a deprecated TOTP credential without digits/counter/period, and asserts standard defaults (6 / 0 / 30).

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

server-spi-private/src/main/java/org/keycloak/models/utils/RepresentationToModel.java:270

This continue only exits the conversion iteration; the unchanged representation remains in user.getCredentials() and is subsequently passed to createCredentialThroughProvider() with null credential/secret data. The warning therefore claims the credential is skipped, but a malformed password credential is stored; actually remove it from the collection (or fail the import) before createCredentials() processes it.

                                    continue;

+                            int digits = cred.getDigits() != null ? cred.getDigits() : 6;
+                            int counter = cred.getCounter() != null ? cred.getCounter() : 0;
+                            int period = cred.getPeriod() != null ? cred.getPeriod() : 30;
+                            OTPCredentialData credentialData = new OTPCredentialData(cred.getType(), digits, counter, period, cred.getAlgorithm(), null);


When converting deprecated OTP credentials, cred.getAlgorithm() can be null, causing Mac.getInstance(null) to throw at OTP validation time. Default the algorithm to HmacOTP.DEFAULT_ALGORITHM when the field is absent, consistent with the digits/counter/period defaulting already in place. Also add import for org.keycloak.models.utils.HmacOTP. Signed-off-by: Nandini Sharma <46066660+nandinisharma3120@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

+                            OTPCredentialData credentialData = new OTPString algorithm = cred.getAlgorithm() != null ? cred.getAlgorithm() : HmacOTP.DEFAULT_ALGORITHM;
+                            OTPCredentialData credentialData = new OTPCredentialData(cred.getType(), digits, counter, period, algorithm, null); secretData = new OTPSecretData(cred.getHashedSaltedValue());


nandinisharma3120 · 2026-07-18T09:38:14Z

+                                int policyIterations = realm.getPasswordPolicy().getHashIterations();
+                                if (policyIterations < 0) {
+                                    logger.warnf("Cannot convert deprecated password credential for user '%s': " +
+                                            "'hashIterations' is absent and no realm password policy provides a default. " +
+                                            "The credential will be skipped.", user.getUsername());


Thanks for the detailed feedback. We chose the skip-with-warning approach here because storing -1 in PasswordCredentialData.hashIterations would cause PBEKeySpec to throw at verification time (iteration count must be >= 1). Skipping the credential cleanly avoids importing data that cannot be used.

That said, if the preference is to keep the credential importable in the no-policy case, we could instead use a hardcoded safe default (e.g. 27500 for pbkdf2-sha256). We're happy to adjust the approach based on maintainer preference — please let us know which direction to take and we'll update accordingly.

Addresses Copilot review comment: assert that a null algorithm in a deprecated OTP credential is defaulted to HmacOTP.DEFAULT_ALGORITHM, confirming that Mac.getInstance() will not receive null. Also adds import for org.keycloak.models.utils.HmacOTP. Signed-off-by: Nandini Sharma <46066660+nandinisharma3120@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

server-spi-private/src/main/java/org/keycloak/models/utils/RepresentationToModel.java:271

This does not implement the documented fallback for the reported payload: when the realm has no explicit hashIterations policy, getHashIterations() returns -1, so the credential is silently skipped and the new user has no password. Resolve the algorithm/provider default (and cover the unconfigured-policy case) rather than dropping the credential.

                                if (policyIterations < 0) {
                                    logger.warnf("Cannot convert deprecated password credential for user '%s': " +
                                            "'hashIterations' is absent and no realm password policy provides a default. " +
                                            "The credential will be skipped.", user.getUsername());
                                    continue;

server-spi-private/src/main/java/org/keycloak/models/utils/RepresentationToModel.java:285

These declarations are syntactically merged and duplicated: OTPString does not exist, credentialData is declared twice, and secretData is undeclared, so this module cannot compile.

                            OTPCredentialData credentialData = new OTPString algorithm = cred.getAlgorithm() != null ? cred.getAlgorithm() : HmacOTP.DEFAULT_ALGORITHM;
                            OTPCredentialData credentialData = new OTPCredentialData(cred.getType(), digits, counter, period, algorithm, null); secretData = new OTPSecretData(cred.getHashedSaltedValue());

…ntialData constructor The previous commit incorrectly merged the new "String algorithm" line with the OTPCredentialData constructor call, producing "new OTPString algorithm = ..." which does not compile. This commit removes the spurious OTPCredentialData prefix so that line 284 is a clean standalone String declaration and line 285 remains the correct OTPCredentialData constructor using the algorithm variable. Signed-off-by: Nandini Sharma <46066660+nandinisharma3120@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

server-spi-private/src/main/java/org/keycloak/models/utils/RepresentationToModel.java:285

secretData is assigned without being declared, so this module will not compile. Restore the OTPSecretData local declaration as a separate statement.

                            OTPCredentialData credentialData = new OTPCredentialData(cred.getType(), digits, counter, period, algorithm, null); secretData = new OTPSecretData(cred.getHashedSaltedValue());

server-spi-private/src/main/java/org/keycloak/models/utils/RepresentationToModel.java:271

This continue only exits the conversion iteration; createCredentials() subsequently iterates the unchanged representation and passes its null credentialData/secretData to the password provider. Thus a realm without an explicit hashIterations policy does not actually skip this credential as logged and can persist an unusable password credential; remove it from the credentials collection (using an iterator) or otherwise exclude it from the later creation loop.

                                    continue;

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Nandini Sharma <46066660+nandinisharma3120@users.noreply.github.com>

…entials

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

server-spi-private/src/main/java/org/keycloak/models/utils/RepresentationToModel.java:285

secretData is assigned without being declared, so this module will not compile. Split the statement and restore the OTPSecretData local declaration.

                            OTPCredentialData credentialData = new OTPCredentialData(cred.getType(), digits, counter, period, algorithm, null); secretData = new OTPSecretData(cred.getHashedSaltedValue());

server-spi-private/src/main/java/org/keycloak/models/utils/RepresentationToModel.java:271

This continue only exits the conversion loop iteration; createCredentials() subsequently submits the same unconverted representation, and the password provider stores it with null credential/secret data. The credential therefore is not actually skipped as the warning claims; remove rejected credentials from the later creation path or return an explicit rejection result.

                                if (policyIterations < 0) {
                                    logger.warnf("Cannot convert deprecated password credential for user '%s': " +
                                            "'hashIterations' is absent and no realm password policy provides a default. " +
                                            "The credential will be skipped.", user.getUsername());
                                    continue;

tests/base/src/test/java/org/keycloak/tests/admin/user/UserCreateTest.java:286

The cleanup lambda captures realmRep, which is mutated on the next line, so cleanup reapplies the 30000-iteration policy instead of restoring the original realm and leaks state into later tests. Use the managed realm's cloning cleanup helper for this update.

        managedRealm.admin().update(realmRep);

        UserRepresentation user = new UserRepresentation();
        user.setUsername("user_creds_no_iter");

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

server-spi-private/src/main/java/org/keycloak/models/utils/RepresentationToModel.java:285

This does not compile because secretData is assigned without being declared. Declare the OTPSecretData local separately before serializing it.

                            OTPCredentialData credentialData = new OTPCredentialData(cred.getType(), digits, counter, period, algorithm, null); secretData = new OTPSecretData(cred.getHashedSaltedValue());

sguilhen · 2026-07-20T20:40:27Z

Hi @nandinisharma3120, thank you for the effort and the thorough investigation of the root cause - your PR description explaining the auto-unboxing issue is well written.

However, we're going to go with the validation approach from PR #49316 instead. This is a deprecated credential import path - the password was already hashed by an external source, and the OTP authenticator was already configured externally. Defaulting missing fields creates credentials that import successfully but fail silently at verification time:

Password: storing the realm policy's iterations labels the imported hash with the wrong count, so verify() rehashes with different iterations and never matches.
OTP: defaulting to 6/30/HmacSHA1 when the original authenticator used different parameters means the generated codes won't match. The user simply can't log in with no indication why.

Rejecting with a clear ModelException gives the admin an immediate 400 telling them exactly which fields are missing, which is better than a silent failure later.

Closing this as a duplicate - thank you for the contribution, and we hope you'll pick up another issue!

Copilot AI review requested due to automatic review settings July 18, 2026 06:44

nandinisharma3120 requested a review from a team as a code owner July 18, 2026 06:44

Copilot started reviewing on behalf of nandinisharma3120 July 18, 2026 06:44 View session