8000 Recovery codes modifications to not tamper sent values by rmartinc · Pull Request #38245 · keycloak/keycloak · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Recovery codes modifications to not tamper sent values #38245

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 19, 2025
Merged

Recovery codes modifications to not tamper sent values #38245

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions ...g/keycloak/authentication/authenticators/browser/RecoveryAuthnCodesFormAuthenticator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@

public class RecoveryAuthnCodesFormAuthenticator implements Authenticator {

public static final String GENERATED_RECOVERY_AUTHN_CODES_NOTE = "RecoveryAuthnCodes.generatedRecoveryAuthnCodes";
public static final String GENERATED_AT_NOTE = "RecoveryAuthnCodes.generatedAt";

public RecoveryAuthnCodesFormAuthenticator(KeycloakSession keycloakSession) {
}

Expand Down
24 changes: 16 additions & 8 deletions ...s/src/main/java/org/keycloak/authentication/requiredactions/RecoveryAuthnCodesAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package org.keycloak.authentication.requiredactions;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import org.keycloak.Config;
Expand All @@ -10,6 +9,7 @@
import org.keycloak.authentication.RequiredActionContext;
import org.keycloak.authentication.RequiredActionFactory;
import org.keycloak.authentication.RequiredActionProvider;
import org.keycloak.authentication.authenticators.browser.RecoveryAuthnCodesFormAuthenticator;
import org.keycloak.common.Profile;
import org.keycloak.credential.RecoveryAuthnCodesCredentialProviderFactory;
import org.keycloak.credential.CredentialPr 10000 ovider;
Expand Down Expand Up @@ -89,21 +89,29 @@ public void processAction(RequiredActionContext reqActionContext) {

CredentialProvider recoveryCodeCredentialProvider;
MultivaluedMap<String, String> httpReqParamsMap;
Long generatedAtTime;
String generatedUserLabel;

recoveryCodeCredentialProvider = reqActionContext.getSession().getProvider(CredentialProvider.class,
RecoveryAuthnCodesCredentialProviderFactory.PROVIDER_ID);

event.detail(Details.CREDENTIAL_TYPE, RecoveryAuthnCodesCredentialModel.TYPE);

httpReqParamsMap = reqActionContext.getHttpRequest().getDecodedFormParameters();
List<String> generatedCodes = new ArrayList<>(
Arrays.asList(httpReqParamsMap.getFirst(FIELD_GENERATED_RECOVERY_AUTHN_CODES_HIDDEN).split(",")));
generatedAtTime = Long.parseLong(httpReqParamsMap.getFirst(FIELD_GENERATED_AT_HIDDEN));
generatedUserLabel = httpReqParamsMap.getFirst(FIELD_USER_LABEL_HIDDEN);
final String generatedCodesString = httpReqParamsMap.getFirst(FIELD_GENERATED_RECOVERY_AUTHN_CODES_HIDDEN);
final String generatedAtTimeString = httpReqParamsMap.getFirst(FIELD_GENERATED_AT_HIDDEN);
final String generatedUserLabel = httpReqParamsMap.getFirst(FIELD_USER_LABEL_HIDDEN);

if (!generatedAtTimeString.equals(reqActionContext.getAuthenticationSession().getAuthNote(RecoveryAuthnCodesFormAuthenticator.GENERATED_AT_NOTE))
|| !generatedCodesString.equals(reqActionContext.getAuthenticationSession().getAuthNote(RecoveryAuthnCodesFormAuthenticator.GENERATED_RECOVERY_AUTHN_CODES_NOTE))) {
// authn codes have been tampered, sent them again
requiredActionChallenge(reqActionContext);
return;
}

reqActionContext.getAuthenticationSession().removeAuthNote(RecoveryAuthnCodesFormAuthenticator.GENERATED_AT_NOTE);
reqActionContext.getAuthenticationSession().removeAuthNote(RecoveryAuthnCodesFormAuthenticator.GENERATED_RECOVERY_AUTHN_CODES_NOTE);

RecoveryAuthnCodesCredentialModel credentialModel = createFromValues(generatedCodes, generatedAtTime, generatedUserLabel);
List<String> generatedCodes = Arrays.asList(generatedCodesString.split(","));
RecoveryAuthnCodesCredentialModel credentialModel = createFromValues(generatedCodes, Long.valueOf(generatedAtTimeString), generatedUserLabel);

if ("on".equals(httpReqParamsMap.getFirst("logout-sessions"))) {
AuthenticatorUtil.logoutOtherSessions(reqActionContext);
Expand Down
7 changes: 6 additions & 1 deletion services/src/main/java/org/keycloak/forms/login/freemarker/FreeMarkerLoginFormsProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import org.keycloak.authentication.AuthenticationProcessor;
import org.keycloak.authentication.authenticators.browser.AbstractUsernameFormAuthenticator;
import org.keycloak.authentication.authenticators.browser.OTPFormAuthenticator;
import org.keycloak.authentication.authenticators.browser.RecoveryAuthnCodesFormAuthenticator;
import org.keycloak.authentication.forms.RegistrationPage;
import org.keycloak.authentication.requiredactions.util.UpdateProfileContext;
import org.keycloak.authentication.requiredactions.util.UserUpdateProfileContext;
Expand Down Expand Up @@ -261,7 +262,11 @@ protected Response createResponse(LoginFormsPages page) {
attributes.put("totp", totpBean);
break;
case LOGIN_RECOVERY_AUTHN_CODES_CONFIG:
attributes.put("recoveryAuthnCodesConfigBean", new RecoveryAuthnCodesBean());
// generate the recovery codes and assign to the auth session
RecoveryAuthnCodesBean recoveryAuthnCodesBean = new RecoveryAuthnCodesBean();
attributes.put("recoveryAuthnCodesConfigBean", recoveryAuthnCodesBean);
authenticationSession.setAuthNote(RecoveryAuthnCodesFormAuthenticator.GENERATED_RECOVERY_AUTHN_CODES_NOTE, recoveryAuthnCodesBean.getGeneratedRecoveryAuthnCodesAsString());
authenticationSession.setAuthNote(RecoveryAuthnCodesFormAuthenticator.GENERATED_AT_NOTE, Long.toString(recoveryAuthnCodesBean.getGeneratedAt()));
break;
case LOGIN_RECOVERY_AUTHN_CODES_INPUT:
attributes.put("recoveryAuthnCodesInputBean", new RecoveryAuthnCodeInputLoginBean(session, realm, user));
Expand Down
27 changes: 26 additions & 1 deletion ...an/tests/base/src/main/java/org/keycloak/testsuite/pages/SetupRecoveryAuthnCodesPage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import org.keycloak.testsuite.util.UIUtils;
import org.openqa.selenium.By;
import org.openqa.selenium.JavascriptExecutor;
import org.openqa.selenium.NoSuchElementException;
import org.openqa.selenium.WebElement;
import org.openqa.selenium.support.FindBy;
Expand All @@ -18,14 +19,38 @@ public class SetupRecoveryAuthnCodesPage extends LogoutSessionsPage {
@FindBy(id = "saveRecoveryAuthnCodesBtn")
private WebElement saveRecoveryAuthnCodesButton;

@FindBy(id="kcRecoveryCodesConfirmationCheck")
@FindBy(id = "kcRecoveryCodesConfirmationCheck")
private WebElement kcRecoveryCodesConfirmationCheck;

@FindBy(name = "generatedRecoveryAuthnCodes")
private WebElement generatedRecoveryAuthnCodesHidden;

@FindBy(name = "generatedAt")
private WebElement generatedAtHidden;

public void clickSaveRecoveryAuthnCodesButton() {
UIUtils.switchCheckbox(kcRecoveryCodesConfirmationCheck, true);
UIUtils.clickLink(saveRecoveryAuthnCodesButton);
}

public String getGeneratedRecoveryAuthnCodesHidden() {
return generatedRecoveryAuthnCodesHidden.getAttribute("value");
}

public void setGeneratedRecoveryAuthnCodesHidden(String codes) {
final JavascriptExecutor js = (JavascriptExecutor) driver;
js.executeScript("document.getElementsByName('generatedRecoveryAuthnCodes')[0].value='" + codes + "'");
}

public String getGeneratedAtHidden() {
return generatedAtHidden.getAttribute("value");
}

public void setGeneratedAtHidden(String at) {
final JavascriptExecutor js = (JavascriptExecutor) driver;
js.executeScript("document.getElementsByName('generatedAt')[0].value='" + at + "'");
}

public List<String> getRecoveryAuthnCodes() {
String recoveryAuthnCodesText = recoveryAuthnCodesList.getText();
List<String> recoveryAuthnCodesList = new ArrayList<>();
Expand Down
72 changes: 68 additions & 4 deletions .../base/src/test/java/org/keycloak/testsuite/forms/RecoveryAuthnCodesAuthenticatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import org.keycloak.credential.CredentialModel;
import org.keycloak.events.Details;
import org.keycloak.events.EventType;
import org.keycloak.forms.login.freemarker.model.RecoveryAuthnCodesBean;
import org.keycloak.models.AuthenticationExecutionModel;
import org.keycloak.models.RealmModel;
import org.keycloak.models.UserModel;
Expand Down Expand Up @@ -185,9 +186,73 @@ public void test02SetupRecoveryAuthnCodesLogoutOtherSessionsNotChecked() {
testSetupRecoveryAuthnCodesLogoutOtherSessions(false);
}

@Test
public void test03SetupRecoveryAuthnCodesModifyGeneratedAt() {
// add the configure recovery codes action
UserResource testUser = ApiUtil.findUserByUsernameId(testRealm(), "test-user@localhost");
UserRepresentation userRepresentation = testUser.toRepresentation();
userRepresentation.setRequiredActions(Arrays.asList(UserModel.RequiredAction.CONFIGURE_RECOVERY_AUTHN_CODES.name()));
testUser.update(userRepresentation);

oauth.openLoginForm();
loginPage.assertCurrent();
loginPage.login("test-user@localhost", "password");
setupRecoveryAuthnCodesPage.assertCurrent();

// modify generatedAt to a fixed value
setupRecoveryAuthnCodesPage.setGeneratedAtHidden("10000");
setupRecoveryAuthnCodesPage.clickSaveRecoveryAuthnCodesButton();

// the recovery codes are regerated as they were tampered
setupRecoveryAuthnCodesPage.assertCurrent();
setupRecoveryAuthnCodesPage.clickSaveRecoveryAuthnCodesButton();

assertEquals(AppPage.RequestType.AUTH_RESPONSE, appPage.getRequestType());
EventRepresentation event = events.expectRequiredAction(EventType.UPDATE_CREDENTIAL)
.user(userRepresentation.getId())
.detail(Details.USERNAME, "test-user@localhost")
.detail(Details.CREDENTIAL_TYPE, RecoveryAuthnCodesCredentialModel.TYPE)
.assertEvent();
events.expectLogin().user(event.getUserId()).session(event.getDetails().get(Details.CODE_ID))
.detail(Details.USERNAME, "test-user@localhost")
.assertEvent();
}

@Test
public void test04SetupRecoveryAuthnCodesModifyGeneratedCodes() {
// add the configure recovery codes action
UserResource testUser = ApiUtil.findUserByUsernameId(testRealm(), "test-user@localhost");
UserRepresentation userRepresentation = testUser.toRepresentation();
userRepresentation.setRequiredActions(Arrays.asList(UserModel.RequiredAction.CONFIGURE_RECOVERY_AUTHN_CODES.name()));
testUser.update(userRepresentation);

oauth.openLoginForm();
loginPage.assertCurrent();
loginPage.login("test-user@localhost", "password");
setupRecoveryAuthnCodesPage.assertCurrent();

// modify the codes with a new generated ones
setupRecoveryAuthnCodesPage.setGeneratedRecoveryAuthnCodesHidden(new RecoveryAuthnCodesBean().getGeneratedRecoveryAuthnCodesAsString());
setupRecoveryAuthnCodesPage.clickSaveRecoveryAuthnCodesButton();

// the recovery codes are regerated as they were tampered
setupRecoveryAuthnCodesPage.assertCurrent();
setupRecoveryAuthnCodesPage.clickSaveRecoveryAuthnCodesButton();

assertEquals(AppPage.RequestType.AUTH_RESPONSE, appPage.getRequestType());
EventRepresentation event = events.expectRequiredAction(EventType.UPDATE_CREDENTIAL)
.user(userRepresentation.getId())
.detail(Details.USERNAME, "test-user@localhost")
.detail(Details.CREDENTIAL_TYPE, RecoveryAuthnCodesCredentialModel.TYPE)
.assertEvent();
events.expectLogin().user(event.getUserId()).session(event.getDetails().get(Details.CODE_ID))
.detail(Details.USERNAME, "test-user@localhost")
.assertEvent();
}

// In a sub-flow with alternative credential executors, test whether Recovery Authentication Codes are working
@Test
public void test03AuthenticateRecoveryAuthnCodes() {
public void test05AuthenticateRecoveryAuthnCodes() {
try {
configureBrowserFlowWithRecoveryAuthnCodes(testingClient);
testRealm().flows().removeRequiredAction(UserModel.RequiredAction.CONFIGURE_RECOVERY_AUTHN_CODES.name());
Expand Down Expand Up @@ -231,7 +296,7 @@ public void test03AuthenticateRecoveryAuthnCodes() {
@Test
@IgnoreBrowserDriver(FirefoxDriver.class)
@IgnoreBrowserDriver(ChromeDriver.class)
public void test04SetupRecoveryAuthnCodes() {
public void test06SetupRecoveryAuthnCodes() {
try {
configureBrowserFlowWithRecoveryAuthnCodes(testingClient);
RequiredActionProviderSimpleRepresentation simpleRepresentation = new RequiredActionProviderSimpleRepresentation();
Expand Down Expand Up @@ -262,7 +327,7 @@ public void test04SetupRecoveryAuthnCodes() {
@Test
@IgnoreBrowserDriver(FirefoxDriver.class) // TODO: https://github.com/keycloak/keycloak/issues/13543
@IgnoreBrowserDriver(ChromeDriver.class)
public void test05BruteforceProtectionRecoveryAuthnCodes() {
public void test07BruteforceProtectionRecoveryAuthnCodes() {
try {
configureBrowserFlowWithRecoveryAuthnCodes(testingClient);
RealmRepresentation rep = testRealm().toRepresentation();
Expand Down Expand Up @@ -314,5 +379,4 @@ public void test05BruteforceProtectionRecoveryAuthnCodes() {
BrowserFlowTest.revertFlows(testRealm(), BROWSER_FLOW_WITH_RECOVERY_AUTHN_CODES);
}
}

}
Loading
0