OTP base32 decode improvements

Description

While working on a migration of several thousands of existing users from another authentication system into Keycloak, I was tasked with migrating all existing user's google one time passwords (OTP). I think I have found a potential deficiency in the way that Keycloak implements OTP. All other systems I found take the secret and Base32 decode the value when generating the OTP. This decoding creates a binary value instead of a 32 character string.

Keycloak takes a base32 secret, stores it in the DB and generates a time based code off the base32 encoded value. In practice, this means I cannot migrate any of my OTP data into the system. Because the data is used verbatim from db storage.

Discussion

https://lists.jboss.org/pipermail/keycloak-dev/2017-July/009686.html

Motivation

This results in a problem. Migrating systems into Keycloak have to drop use of OTP temporarily while migrating. And ask everyone to re-register OTP after migration. This is not good for security.

Details

Below are some references to other OTP implementations. I found several other libraries as well (all do the same thing). Rather than being boring with details, I'd like to suggest that Keycloak should change to do the same thing. It is a breaking change though. It will invalidate all previously generated OTPs.

Java:

https://github.com/aerogear-attic/aerogear-otp-java/blob/master/src/main/java/org/jboss/aerogear/security/otp/Totp.java#L121

PHP:

https://github.com/antonioribeiro/google2fa/blob/c6d560d4f3be2310b276a1ead697ae971a7de90b/src/Google2FA.php#L272

As I said, I uncovered this while attempting to migrate OTP data from another system into Keycloak. These other systems take the base32 value, then decode it and use its unencoded (binary) value when validating the OTP.

An example of the dilemma is worth a lot:

HFI7QHU7S5BO3ZZ3DOIPTUS4 is the 32bit secret from the other system
To use it in Keycloak, it must be a verbatim value. Ergo the value must be decoded before inserted into storage.
HFI7QHU7S5BO3ZZ3DOIPTUS4 decoded is equal to 9Q��B��;��
Which becomes:
UPDATE CREDENTIAL SET SECRET_DATA='{"value":"9Q��B��;��\"' where ID="463da4e3-160a-45b0-9269-4647db7ff2"

Which results in

SQLSTATE[22007]: Invalid datetime format: 1366 Incorrect string value: '\xF8\x1E\x9F\x97B\xED...' for column `keycloak`.`CREDENTIAL`.`SECRET_DATA` at row 1

The motivation is to be able to import/migrate many thousands of users into keycloak. In the case of OTP, we've stumbled on a few options.

Email all users of the legacy system to reset their OTP on the legacy system before we migrate. And alter (hack) the OTP system there to match what is happening KC so when they reset OTP, we migrate, they are ready.
The legacy system also supports backup codes (a feature added in KC with https://github.com/thomasdarimont/keycloak-extension-playground/tree/master/auth-backup-codes currently). We are also considering NOT emailing folks and telling all users to login the first-time on KC using a backup code. Then in KC resetting their OTP.
Alter DB to handle/support binary data instead of strings. We would then migrate the base32 decoded values from legacy system.

In all these cases, it involves a lot more moving parts and pieces then really necessary. But the last one is less cumbersome then others. One issue with the last option, the KC web service handles JSON and JSON doesn't really like binary data. If that option were leveraged, we should add a decode option (maybe base64) so we can send in encoded OTP secret data that is decoded after it is received.

Option 2 is temporarily blocked on 16.x because thomasdarimont/keycloak-extension-playground#37. Additionally, some effort is in progress to move backup/recovery codes into KC directly in #8730.

@stianst

This is the exact issue forcing us to not use Keycloak at huge scale.
We have 4+ million existing customers and we want to migrate them to keycloak.
Asking them to reset their 2FA is not an option for us.

@stianst do you suggest any workaround?
Also if getting an enterprise support is an option then we can even opt in for that.

@kpritam

@kpritam does your existing system support recovery codes? And are those recovery codes stored in a reversible encryption instead of hashed? In our case, we've been waiting for v18 of keycloak, which just released yesterday. And I already have working PoC code ready that migrates all of our recovery codes. In v18, native recovery code support is available. And previously our recovery codes were encrypted, not hashed, so while we can't migrate the seed OTP value, we can migrate the recovery code and ask our many users to reset their OTP using a recovery code. Not sure if that works for you, but if it does, that is a possible path forward.

@heddn

Thanks @heddn for the workaround. We do store recovery codes but asking customers to reset their OTP is not an option for us, sadly that is the strict requirement we have.

@mposolda

Keycloak generates a secret that can be stored directly in the database, so doesn't need to base32 encode it or decode it. However, I appreciate the problem when migrating from a different system that has generated secrets that can't be stored directly.

I think we can solve this relatively easily without making it a breaking change as credentials stored by Keycloak can have additional metadata associated with them, so would be possible to optionally base32/64 encode the value when stored in the database.

Would probably need someone to volunteer to contribute this though. @mposolda should be able to give some development hints to someone that would like to give it a go.

@stianst

@stianst adding the base32/64 encoding metadata would be a great asset for migrations from external systems.

@stianst

@stianst i can take a stab at it if i can get some help.

@kpritam

@kpritam Thanks for looking at this. The classes, which you can take a look are:
OTPFormAuthenticator, UpdateTotp and OTPCredentialProvider and related factories used to create them. The model class is OTPCredentialModel , which holds OTPCredentialData and OTPSecretData . You may need to add new field to OTPCredentialData about the format of how the credential should be stored (plain/base32/base64). The default should be "plain" for the backwards compatibility as credentials migrated from Keycloak versions 18 or earlier use this and it is needed that these are not affected anyhow.

@mposolda

Thanks @mposolda , that is almost exactly what I have done.

To start with, I have added encoding field in OTPCredentialData whose default value is set to UTF-8.

Naive version of code looked like this:

String enc = credentialModel.getOTPCredentialData().getEncoding();
byte[] secret = credentialModel.getOTPSecretData().getValue().getBytes();
log.info("Encoding is " + enc);
if (enc.equalsIgnoreCase("base32")) {
    secret = Base32.decode(credentialModel.getOTPSecretData().getValue());
    log.info("UTF-8 Base32 decoded Secret " + new String(secret));
}
log.info("UTF-8 Secret " + new String(secret));
return validator.validateTOTP(token, secret);

Still it did not work in my case for this base32 encoded secret CDLYAYRJ73ORTU4PUWWATWSYQCP4H2QL

For me dev workflow is taking a lot of time. Is there simple way to run KeycloakMain from IDE directly in debug mode.

IDELauncher class is starting app with in memory h2. What changes are required to use postgres in this case?

@kpritam

@kpritam For the development, I am still using KEycloakServer (Keycloak on Undertow): https://github.com/keycloak/keycloak/blob/main/docs/tests.md#keycloak-server . And running the class directly from the IDE. This helps to have good development cycle. I did not yet tried IDELauncher (but I am planning to try it soon).

For the DBs with KeycloakServer, you can use system properties when starting like this: -Dkeycloak.connectionsJpa.url=jdbc:postgresql://docker-postgres/keycloak -Dkeycloak.connectionsJpa.driver=org.postgresql.Driver -Dkeycloak.connectionsJpa.user=keycloak -Dkeycloak.connectionsJpa.password=k eycloak -Dkeycloak.liquibase.logging.level=debug

Your changes looks good in general. Some polishing to them will be probably needed, but that is for later once you have things more polished...

This code not working in case of random binary byte[] secret:

return validator.validateTOTP(token, secret);

Because TimeBasedOTP::validateTOTP(String, byte[]) cast secret from byte[] to String in line.

            String candidate = generateOTP(new String(secret), steps, this.numberDigits, this.algorithm);

at

keycloak/server-spi-private/src/main/java/org/keycloak/models/utils/TimeBasedOTP.java

Line 91 in 3862f82

    
           String candidate = generateOTP(new String(secret), steps, this.numberDigits, this.algorithm);

A quick(dirty) fix with minimal changes can be done in HmacOTP.java after last .getBytes() before final hmac computation.

        // Adding one byte to get the right conversion
        // byte[] k = hexStr2Bytes(key);
        byte[] k = key.getBytes();

        hash = hmac_sha1(crypto, k, msg);

at

keycloak/server-spi/src/main/java/org/keycloak/models/utils/HmacOTP.java

Line 115 in 3862f82

byte[] k = key.getBytes();

New code

        // byte[] k = hexStr2Bytes(key);
        byte[] k = key.getBytes();
        if(key.startsWith("{B32}"))
            k = Base32.decode(key.substring(5));

        hash = hmac_sha1(crypto, k, msg);

Then migrated secretValues should be in Base32 with {B32} prefix.
Will affects only imported secrets, as native secrets never starts with {B32}.

An minimalist RealmResource for test can implemented as:

    @GET()
    @Path("/otp")
    public Response otp(@Context HttpRequest request, @Context final HttpHeaders headers) {
        try {
            RealmModel realm = session.getContext().getRealm();
            UserModel user = session.users().getUserByUsername(realm, request.getUri().getQueryParameters().getFirst("username"));
            session.userCredentialManager().createCredential(realm, user, OTPCredentialModel.createFromPolicy(realm, "{B32}"+request.getUri().getQueryParameters().getFirst("secret"), "realmname"));
            return cors(request, Response.ok("Created"));
        } catch (Throwable e) {
        }
        
        throw new NotFoundException("Page not found");
    }

I have managed some working internal tests with our customers OTP base.

Any possilibity of inclusion of this small patch?

PS. I´m curious of comment line // byte[] k = hexStr2Bytes(key); if secrets are managed/stored/trasmitted hexencoded, this issue should have a workaround without internal code changes.

OTP support for binary secrets encoded in Base32

@heddn

@heddn Would you mind looking at #21117? Does it help to solve the issue?

Support reading base32 encoded OTP secret

Repository navigation

OTP base32 decode improvements #9434

Description

Discussion

Motivation

Details

Metadata

Assignees

Labels

Type

Fields

Projects

Milestone

Relationships

Development

OTP base32 decode improvements #9434

Description

Description

Discussion

Motivation

Details

Activity

heddn commented on Jan 7, 2022

heddn commented on Jan 7, 2022

kpritam commented on Apr 21, 2022

heddn commented on Apr 21, 2022

kpritam commented on Apr 21, 2022

stianst commented on Apr 22, 2022

heddn commented on Apr 22, 2022

kpritam commented on Apr 23, 2022

mposolda commented on Apr 25, 2022

kpritam commented on Apr 25, 2022

mposolda commented on Apr 26, 2022

rhyamada commented on May 31, 2023

pedroigor commented on Jun 21, 2023

Metadata

Metadata

Assignees

Labels

Type

Fields

Projects

Milestone

Relationships

Development

Issue actions