From b68cae9fd14f139ff062154a83fc270869a230dd Mon Sep 17 00:00:00 2001 From: Thomas Darimont Date: Wed, 11 Sep 2024 23:43:22 +0200 Subject: [PATCH] Revise KeyURI generation in OTPPolicy - More idiomatic URLEncoder usage - Replace `:` character with space in issuer if present in OTP label - Ease testing for OTP KeyURI generation Fixes #32833 Signed-off-by: Thomas Darimont --- .../java/org/keycloak/models/OTPPolicy.java | 62 ++++++++++++------- .../org/keycloak/models/OtpPolicyTest.java | 40 ++++++++++++ 2 files changed, 79 insertions(+), 23 deletions(-) create mode 100644 server-spi/src/test/java/org/keycloak/models/OtpPolicyTest.java diff --git a/server-spi/src/main/java/org/keycloak/models/OTPPolicy.java b/server-spi/src/main/java/org/keycloak/models/OTPPolicy.java index 982fbe2b6b..fd836bef7c 100755 --- a/server-spi/src/main/java/org/keycloak/models/OTPPolicy.java +++ b/server-spi/src/main/java/org/keycloak/models/OTPPolicy.java @@ -21,13 +21,15 @@ import org.jboss.logging.Logger; import org.keycloak.models.credential.OTPCredentialModel; import org.keycloak.models.utils.Base32; import org.keycloak.models.utils.HmacOTP; +import org.keycloak.utils.StringUtil; import java.io.Serializable; -import java.io.UnsupportedEncodingException; import java.net.URLEncoder; import java.util.HashMap; import java.util.Map; +import static java.nio.charset.StandardCharsets.UTF_8; + /** * @author Bill Burke * @version $Revision: 1 $ @@ -145,35 +147,49 @@ public class OTPPolicy implements Serializable { */ public String getKeyURI(RealmModel realm, UserModel user, String secret) { - try { + String issuerName = !StringUtil.isNullOrEmpty(realm.getDisplayName()) ? realm.getDisplayName() : realm.getName(); + String accountName = user.getUsername(); - String displayName = realm.getDisplayName() != null && !realm.getDisplayName().isEmpty() ? realm.getDisplayName() : realm.getName(); + return getKeyURI(issuerName, accountName, secret); + } - String accountName = URLEncoder.encode(user.getUsername(), "UTF-8"); - String issuerName = URLEncoder.encode(displayName, "UTF-8") .replaceAll("\\+", "%20"); + /** + * Constructs the otpauth:// URI based on the Key-Uri-Format. + * + * @param rawIssuerName + * @param rawAccountName + * @param secret + * @return the otpauth:// URI + */ + public String getKeyURI(String rawIssuerName, String rawAccountName, String secret) { - /* - * The issuerName component in the label is usually shown in a authenticator app, such as - * Google Authenticator or FreeOTP, as a hint for the user to which system an username - * belongs to. - */ - String label = issuerName + ":" + accountName; + String accountName = URLEncoder.encode(rawAccountName, UTF_8); + /* + * Replacing ':' in issuerName with a space because the ':' is not allowed in the issuer part of the label. + * See: https://github.com/google/google-authenticator/wiki/Key-Uri-Format#label + */ + String issuerName = rawIssuerName.replaceAll("[:]", " "); + issuerName = URLEncoder.encode(issuerName, UTF_8).replaceAll("\\+", "%20"); - String parameters = "secret=" + Base32.encode(secret.getBytes()) // - + "&digits=" + digits // - + "&algorithm=" + algToKeyUriAlg.get(algorithm) // - + "&issuer=" + issuerName; + /* + * The issuerName component in the label is usually shown in a authenticator app, such as + * Google Authenticator or FreeOTP, as a hint for the user to which system an username + * belongs to. + */ + String label = issuerName + ":" + accountName; - if (type.equals(OTPCredentialModel.HOTP)) { - parameters += "&counter=" + initialCounter; - } else if (type.equals(OTPCredentialModel.TOTP)) { - parameters += "&period=" + period; - } + String parameters = "secret=" + Base32.encode(secret.getBytes()) // + + "&digits=" + digits // + + "&algorithm=" + algToKeyUriAlg.get(algorithm) // + + "&issuer=" + issuerName; - return "otpauth://" + type + "/" + label+ "?" + parameters; - } catch (UnsupportedEncodingException e) { - throw new RuntimeException(e); + if (type.equals(OTPCredentialModel.HOTP)) { + parameters += "&counter=" + initialCounter; + } else if (type.equals(OTPCredentialModel.TOTP)) { + parameters += "&period=" + period; } + + return "otpauth://" + type + "/" + label + "?" + parameters; } } diff --git a/server-spi/src/test/java/org/keycloak/models/OtpPolicyTest.java b/server-spi/src/test/java/org/keycloak/models/OtpPolicyTest.java new file mode 100644 index 0000000000..25b2b80977 --- /dev/null +++ b/server-spi/src/test/java/org/keycloak/models/OtpPolicyTest.java @@ -0,0 +1,40 @@ +package org.keycloak.models; + +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.keycloak.models.credential.OTPCredentialModel; +import org.keycloak.models.utils.HmacOTP; + +import java.net.URI; + +public class OtpPolicyTest { + + OTPPolicy totpPolicy; + + @Before + public void setup() { + totpPolicy = new OTPPolicy(); + totpPolicy.setAlgorithm(HmacOTP.HMAC_SHA1); + totpPolicy.setDigits(6); + totpPolicy.setType(OTPCredentialModel.TOTP); + } + + @Test + public void keyUriShouldBeValidForRealmDisplayNameWithColon() { + + String keyURI = totpPolicy.getKeyURI("Test:Realm", "tester", "secret"); + Assert.assertEquals("Test Realm", getLabelComponent(keyURI)); + } + + @Test + public void keyUriShouldBeValidForRealmDisplayNameWithSlash() { + + String keyURI = totpPolicy.getKeyURI("Test/Realm", "tester", "secret"); + Assert.assertEquals("Test/Realm", getLabelComponent(keyURI)); + } + + static String getLabelComponent(String keyURI) { + return URI.create(keyURI).getPath().substring(1).split(":")[0]; + } +}