mirror of
https://github.com/keycloak/keycloak.git
synced 2026-01-09 23:12:06 -03:30
Use optional realm attribute for request param max size/number (#25007)
closes #25006 Enable fail-fast toggle for additional request parameter parsing Enable configuration of an overall size of additional request parameters Everything is backwardscompatible. No configuration necessary when upgrading. Signed-off-by: Manuel Schallar <manuel.schallar@prime-sign.com> Co-authored-by: Manuel Schallar <manuel.schallar@prime-sign.com>
This commit is contained in:
Manuel Schallar
GitHub
parent
9f13b271ec
commit
7e08b095a3
@ -22,6 +22,7 @@ import static org.junit.Assert.assertNull;
|
||||
import static org.junit.Assert.assertTrue;
|
||||
import static org.keycloak.quarkus.runtime.Environment.isWindows;
|
||||
|
||||
import java.nio.file.FileSystem;
|
||||
import java.nio.file.FileSystems;
|
||||
import java.nio.file.Path;
|
||||
import java.util.Arrays;
|
||||
@ -500,7 +501,7 @@ public class ConfigurationTest extends AbstractConfigurationTest {
|
||||
ConfigArgsConfigSource.setCliArgs("--https-certificates-reload-period=2h");
|
||||
assertEquals("2h", createConfig().getConfigValue("quarkus.http.ssl.certificate.reload-period").getValue());
|
||||
}
|
||||
|
||||
|
||||
@Test
|
||||
public void testHttpsPaths() {
|
||||
ConfigArgsConfigSource.setCliArgs("--https-certificate-file=\\some\\file");
|
||||
@ -511,7 +512,7 @@ public class ConfigurationTest extends AbstractConfigurationTest {
|
||||
}
|
||||
assertEquals(expected, createConfig().getConfigValue("quarkus.http.ssl.certificate.files").getValue());
|
||||
}
|
||||
|
||||
|
||||
@Test
|
||||
public void testCacheMaxCount() {
|
||||
int maxCount = 500;
|
||||
|
||||
@ -153,6 +153,8 @@ public class OIDCLoginProtocol implements LoginProtocol {
|
||||
protected OIDCResponseType responseType;
|
||||
protected OIDCResponseMode responseMode;
|
||||
|
||||
protected OIDCProviderConfig providerConfig;
|
||||
|
||||
public OIDCLoginProtocol(KeycloakSession session, RealmModel realm, UriInfo uriInfo, HttpHeaders headers, EventBuilder event) {
|
||||
this.session = session;
|
||||
this.realm = realm;
|
||||
@ -161,8 +163,8 @@ public class OIDCLoginProtocol implements LoginProtocol {
|
||||
this.event = event;
|
||||
}
|
||||
|
||||
public OIDCLoginProtocol() {
|
||||
|
||||
public OIDCLoginProtocol(OIDCProviderConfig providerConfig) {
|
||||
this.providerConfig = providerConfig;
|
||||
}
|
||||
|
||||
private void setupResponseTypeAndMode(String responseType, String responseMode) {
|
||||
@ -202,6 +204,10 @@ public class OIDCLoginProtocol implements LoginProtocol {
|
||||
return this;
|
||||
}
|
||||
|
||||
public OIDCProviderConfig getConfig() {
|
||||
return this.providerConfig;
|
||||
}
|
||||
|
||||
@Override
|
||||
public Response authenticated(AuthenticationSessionModel authSession, UserSessionModel userSession, ClientSessionContext clientSessionCtx) {
|
||||
AuthenticatedClientSessionModel clientSession = clientSessionCtx.getClientSession();
|
||||
|
||||
@ -110,14 +110,22 @@ public class OIDCLoginProtocolFactory extends AbstractLoginProtocolFactory {
|
||||
public static final String ROLES_SCOPE_CONSENT_TEXT = "${rolesScopeConsentText}";
|
||||
public static final String ORGANIZATION_SCOPE_CONSENT_TEXT = "${organizationScopeConsentText}";
|
||||
|
||||
public static final String CONFIG_OIDC_REQ_PARAMS_MAX_NUMBER = "add-req-params-max-number";
|
||||
public static final String CONFIG_OIDC_REQ_PARAMS_MAX_SIZE = "add-req-params-max-size";
|
||||
public static final String CONFIG_OIDC_REQ_PARAMS_MAX_OVERALL_SIZE = "add-req-params-max-overall-size";
|
||||
public static final String CONFIG_OIDC_REQ_PARAMS_FAIL_FAST = "add-req-params-fail-fast";
|
||||
|
||||
private OIDCProviderConfig providerConfig;
|
||||
|
||||
@Override
|
||||
public void init(Config.Scope config) {
|
||||
this.providerConfig = new OIDCProviderConfig(config);
|
||||
initBuiltIns();
|
||||
}
|
||||
|
||||
@Override
|
||||
public LoginProtocol create(KeycloakSession session) {
|
||||
return new OIDCLoginProtocol().setSession(session);
|
||||
return new OIDCLoginProtocol(this.providerConfig).setSession(session);
|
||||
}
|
||||
|
||||
@Override
|
||||
|
||||
@ -0,0 +1,74 @@
|
||||
package org.keycloak.protocol.oidc;
|
||||
|
||||
import org.keycloak.Config;
|
||||
|
||||
/**
|
||||
* @author <a href="mailto:patrick.weiner@prime-sign.com">Patrick Weiner</a>
|
||||
*/
|
||||
public class OIDCProviderConfig {
|
||||
|
||||
/**
|
||||
* Default value for {@link #additionalReqParamsMaxNumber} if case no configuration property is set.
|
||||
*/
|
||||
public static final int DEFAULT_ADDITIONAL_REQ_PARAMS_MAX_NUMBER = 5;
|
||||
|
||||
/**
|
||||
* Max number of additional request parameters copied into client session note to prevent DoS attacks.
|
||||
*/
|
||||
private final int additionalReqParamsMaxNumber;
|
||||
|
||||
/**
|
||||
* Default value for {@link #additionalReqParamsMaxSize} if case no configuration property is set.
|
||||
*/
|
||||
public static final int DEFAULT_ADDITIONAL_REQ_PARAMS_MAX_SIZE = 2000;
|
||||
|
||||
/**
|
||||
* Max size of additional request parameters value copied into client session note to prevent DoS attacks.
|
||||
*/
|
||||
private final int additionalReqParamsMaxSize;
|
||||
|
||||
/**
|
||||
* Default value for {@link #additionalReqParamsFailFast} in case no configuration property is set.
|
||||
*/
|
||||
public static final boolean DEFAULT_ADDITIONAL_REQ_PARAMS_FAIL_FAST = false;
|
||||
|
||||
/**
|
||||
* Whether the fail-fast strategy should be enforced. If <code>false</code> all additional request parameters
|
||||
* that to not meet the configuration are silently ignored. If <code>true</code> an exception will be raised.
|
||||
*/
|
||||
private final boolean additionalReqParamsFailFast;
|
||||
|
||||
/**
|
||||
* Default value for {@link #additionalReqParamsMaxOverallSize} in case no configuration property is set.
|
||||
*/
|
||||
public static final int DEFAULT_ADDITIONAL_REQ_PARAMS_MAX_OVERALL_SIZE = Integer.MAX_VALUE;
|
||||
|
||||
/**
|
||||
* Max size of all additional request parameters value copied into client session note to prevent DoS attacks.
|
||||
*/
|
||||
private final int additionalReqParamsMaxOverallSize;
|
||||
|
||||
|
||||
public OIDCProviderConfig(Config.Scope config) {
|
||||
this.additionalReqParamsMaxNumber = config.getInt(OIDCLoginProtocolFactory.CONFIG_OIDC_REQ_PARAMS_MAX_NUMBER, DEFAULT_ADDITIONAL_REQ_PARAMS_MAX_NUMBER);
|
||||
this.additionalReqParamsMaxSize = config.getInt(OIDCLoginProtocolFactory.CONFIG_OIDC_REQ_PARAMS_MAX_SIZE, DEFAULT_ADDITIONAL_REQ_PARAMS_MAX_SIZE);
|
||||
this.additionalReqParamsMaxOverallSize = config.getInt(OIDCLoginProtocolFactory.CONFIG_OIDC_REQ_PARAMS_MAX_OVERALL_SIZE, DEFAULT_ADDITIONAL_REQ_PARAMS_MAX_OVERALL_SIZE);
|
||||
this.additionalReqParamsFailFast = config.getBoolean(OIDCLoginProtocolFactory.CONFIG_OIDC_REQ_PARAMS_FAIL_FAST, DEFAULT_ADDITIONAL_REQ_PARAMS_FAIL_FAST);
|
||||
}
|
||||
|
||||
public int getAdditionalReqParamsMaxNumber() {
|
||||
return additionalReqParamsMaxNumber;
|
||||
}
|
||||
|
||||
public int getAdditionalReqParamsMaxSize() {
|
||||
return additionalReqParamsMaxSize;
|
||||
}
|
||||
|
||||
public boolean isAdditionalReqParamsFailFast() {
|
||||
return additionalReqParamsFailFast;
|
||||
}
|
||||
|
||||
public int getAdditionalReqParamsMaxOverallSize() {
|
||||
return additionalReqParamsMaxOverallSize;
|
||||
}
|
||||
}
|
||||
@ -53,7 +53,7 @@ public class AuthorizationEndpointRequestParserProcessor {
|
||||
try {
|
||||
AuthorizationEndpointRequest request = new AuthorizationEndpointRequest();
|
||||
boolean isResponseTypeParameterRequired = isResponseTypeParameterRequired(requestParams, endpointType);
|
||||
AuthzEndpointQueryStringParser parser = new AuthzEndpointQueryStringParser(requestParams, isResponseTypeParameterRequired);
|
||||
AuthzEndpointQueryStringParser parser = new AuthzEndpointQueryStringParser(session, requestParams, isResponseTypeParameterRequired);
|
||||
parser.parseRequest(request);
|
||||
|
||||
if (parser.getInvalidRequestMessage() != null) {
|
||||
|
||||
@ -22,6 +22,7 @@ import jakarta.ws.rs.core.MultivaluedMap;
|
||||
import java.util.Set;
|
||||
|
||||
import org.jboss.logging.Logger;
|
||||
import org.keycloak.models.KeycloakSession;
|
||||
import org.keycloak.protocol.oidc.OIDCLoginProtocol;
|
||||
|
||||
/**
|
||||
@ -39,7 +40,8 @@ public class AuthzEndpointQueryStringParser extends AuthzEndpointRequestParser {
|
||||
|
||||
private String invalidRequestMessage = null;
|
||||
|
||||
public AuthzEndpointQueryStringParser(MultivaluedMap<String, String> requestParams, boolean isResponseTypeParameterRequired) {
|
||||
public AuthzEndpointQueryStringParser(KeycloakSession keycloakSession, MultivaluedMap<String, String> requestParams, boolean isResponseTypeParameterRequired) {
|
||||
super(keycloakSession);
|
||||
this.requestParams = requestParams;
|
||||
this.isResponseTypeParameterRequired = isResponseTypeParameterRequired;
|
||||
}
|
||||
|
||||
@ -41,6 +41,7 @@ public class AuthzEndpointRequestObjectParser extends AuthzEndpointRequestParser
|
||||
private final JsonNode requestParams;
|
||||
|
||||
public AuthzEndpointRequestObjectParser(KeycloakSession session, String requestObject, ClientModel client) {
|
||||
super(session);
|
||||
this.requestParams = session.tokens().decodeClientJWT(requestObject, client, createRequestObjectValidator(session), JsonNode.class);
|
||||
|
||||
if (this.requestParams == null) {
|
||||
|
||||
@ -19,32 +19,51 @@ package org.keycloak.protocol.oidc.endpoints.request;
|
||||
|
||||
import org.jboss.logging.Logger;
|
||||
import org.keycloak.OAuth2Constants;
|
||||
import org.keycloak.OAuthErrorException;
|
||||
import org.keycloak.constants.AdapterConstants;
|
||||
import org.keycloak.models.Constants;
|
||||
import org.keycloak.models.KeycloakSession;
|
||||
import org.keycloak.protocol.LoginProtocol;
|
||||
import org.keycloak.protocol.oidc.OIDCLoginProtocol;
|
||||
import org.keycloak.protocol.oidc.OIDCProviderConfig;
|
||||
import org.keycloak.services.ErrorResponseException;
|
||||
|
||||
import jakarta.ws.rs.core.Response;
|
||||
|
||||
import java.util.HashSet;
|
||||
import java.util.Map;
|
||||
import java.util.Set;
|
||||
|
||||
/**
|
||||
* This endpoint parser supports, per default, up to
|
||||
* {@value org.keycloak.protocol.oidc.OIDCProviderConfig#DEFAULT_ADDITIONAL_REQ_PARAMS_MAX_NUMBER} parameters with each
|
||||
* having a total size of {@value org.keycloak.protocol.oidc.OIDCProviderConfig#DEFAULT_ADDITIONAL_REQ_PARAMS_MAX_SIZE}.
|
||||
* If there are more authentication request parameters, or a parameter has a size
|
||||
* than allowed, those parameters are silently ignored.
|
||||
* <p>
|
||||
* You can toggle the behavior by setting ({@code additionalReqParamsFailFast}) that enables the fail-fast principle.
|
||||
* Any request parameter in violation of the configuration results in an
|
||||
* error response, e.g.,
|
||||
* <ul>
|
||||
* <li>for a Pushed Authorization Request (PAR) this results in a JSON response.</li>
|
||||
* <li>For openid/auth in an error page with an "Back to Application" button using the client's base URL. (if valid) as redirect target.</li>
|
||||
* </ul>
|
||||
*
|
||||
* <p>
|
||||
* Additionally, ({@code additionalReqParamMaxOverallSize}) can be configured
|
||||
* that sets the maximum of size of all parameters combined. If not provided, {@link Integer#MAX_VALUE} will be used.
|
||||
*
|
||||
* @author <a href="mailto:manuel.schallar@prime-sign.com">Manuel Schallar</a>
|
||||
* @author <a href="mailto:mposolda@redhat.com">Marek Posolda</a>
|
||||
*/
|
||||
public abstract class AuthzEndpointRequestParser {
|
||||
|
||||
private static final Logger logger = Logger.getLogger(AuthzEndpointRequestParser.class);
|
||||
|
||||
/**
|
||||
* Max number of additional req params copied into client session note to prevent DoS attacks
|
||||
*
|
||||
*/
|
||||
public static final int ADDITIONAL_REQ_PARAMS_MAX_MUMBER = 5;
|
||||
|
||||
/**
|
||||
* Max size of additional req param value copied into client session note to prevent DoS attacks - params with longer value are ignored
|
||||
*
|
||||
*/
|
||||
public static final int ADDITIONAL_REQ_PARAMS_MAX_SIZE = 2000;
|
||||
protected final int additionalReqParamsMaxNumber;
|
||||
protected final int additionalReqParamsMaxSize;
|
||||
protected final boolean additionalReqParamsFailFast;
|
||||
protected final int additionalReqParamsMaxOverallSize;
|
||||
|
||||
public static final String AUTHZ_REQUEST_OBJECT = "ParsedRequestObject";
|
||||
public static final String AUTHZ_REQUEST_OBJECT_ENCRYPTED = "EncryptedRequestObject";
|
||||
@ -83,6 +102,15 @@ public abstract class AuthzEndpointRequestParser {
|
||||
KNOWN_REQ_PARAMS.add(OAuth2Constants.CLIENT_SECRET);
|
||||
}
|
||||
|
||||
protected AuthzEndpointRequestParser(KeycloakSession keycloakSession) {
|
||||
OIDCLoginProtocol loginProtocol = (OIDCLoginProtocol) keycloakSession.getProvider(LoginProtocol.class, OIDCLoginProtocol.LOGIN_PROTOCOL);
|
||||
OIDCProviderConfig config = loginProtocol.getConfig();
|
||||
this.additionalReqParamsMaxNumber = config.getAdditionalReqParamsMaxNumber();
|
||||
this.additionalReqParamsMaxSize = config.getAdditionalReqParamsMaxSize();
|
||||
this.additionalReqParamsFailFast = config.isAdditionalReqParamsFailFast();
|
||||
this.additionalReqParamsMaxOverallSize = config.getAdditionalReqParamsMaxOverallSize();
|
||||
}
|
||||
|
||||
public void parseRequest(AuthorizationEndpointRequest request) {
|
||||
String clientId = getParameter(OIDCLoginProtocol.CLIENT_ID_PARAM);
|
||||
if (clientId != null && request.clientId != null && !request.clientId.equals(clientId)) {
|
||||
@ -130,23 +158,61 @@ public abstract class AuthzEndpointRequestParser {
|
||||
}
|
||||
|
||||
protected void extractAdditionalReqParams(Map<String, String> additionalReqParams) {
|
||||
int currentAdditionalReqParamMaxOverallSize = 0;
|
||||
for (String paramName : keySet()) {
|
||||
if (!KNOWN_REQ_PARAMS.contains(paramName)) {
|
||||
String value = getParameter(paramName);
|
||||
if (value != null && value.trim().isEmpty()) {
|
||||
value = null;
|
||||
}
|
||||
if (value != null && value.length() <= ADDITIONAL_REQ_PARAMS_MAX_SIZE) {
|
||||
if (additionalReqParams.size() >= ADDITIONAL_REQ_PARAMS_MAX_MUMBER) {
|
||||
logger.debug("Maximal number of additional OIDC params (" + ADDITIONAL_REQ_PARAMS_MAX_MUMBER + ") exceeded, ignoring rest of them!");
|
||||
break;
|
||||
}
|
||||
additionalReqParams.put(paramName, value);
|
||||
} else {
|
||||
logger.debug("OIDC Additional param " + paramName + " ignored because value is empty or longer than " + ADDITIONAL_REQ_PARAMS_MAX_SIZE);
|
||||
}
|
||||
|
||||
if (KNOWN_REQ_PARAMS.contains(paramName)) {
|
||||
logger.debugv("The additional OIDC param ''{0}'' is well known. Continue with the other additional parameters.", paramName);
|
||||
continue;
|
||||
}
|
||||
|
||||
final String value = getParameter(paramName);
|
||||
|
||||
if (value == null || value.trim().isEmpty()) {
|
||||
logger.debugv("The additional OIDC param ''{0}'' ignored because it's value is null or blank.", paramName);
|
||||
continue;
|
||||
}
|
||||
|
||||
// Compare with ">=", as the currently processed parameter will be added at the END of this method.
|
||||
if (additionalReqParams.size() >= additionalReqParamsMaxNumber) {
|
||||
|
||||
if (additionalReqParamsFailFast) {
|
||||
logger.debugv("The maximum number of allowed parameters ({0}) is exceeded.", additionalReqParamsMaxNumber);
|
||||
throw new ErrorResponseException(OAuthErrorException.INVALID_REQUEST, "The maximum number of allowed parameters (" + additionalReqParamsMaxNumber + ") is exceeded.", Response.Status.BAD_REQUEST);
|
||||
} else {
|
||||
logger.debugv("The maximum number of allowed parameters ({0}) is exceeded.", additionalReqParamsMaxNumber);
|
||||
break;
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
if (value.length() + currentAdditionalReqParamMaxOverallSize > additionalReqParamsMaxOverallSize) {
|
||||
|
||||
if (additionalReqParamsFailFast) {
|
||||
logger.debugv("The OIDC additional parameter '{0}''s size ({1}) exceeds the maximum allowed size of all parameters ({2}).", paramName, value.length(), additionalReqParamsMaxOverallSize);
|
||||
throw new ErrorResponseException(OAuthErrorException.INVALID_REQUEST, "The OIDC additional parameter '" + paramName + "'s size (" + value.length() + ") exceeds the maximum allowed size of all parameters (" + additionalReqParamsMaxOverallSize + ").", Response.Status.BAD_REQUEST);
|
||||
} else {
|
||||
logger.debugv("The OIDC additional parameter '{0}''s size exceeds ({1}) the maximum allowed size of all parameters ({2}).", paramName, value.length(), additionalReqParamsMaxOverallSize);
|
||||
break;
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
if (value.length() > additionalReqParamsMaxSize) {
|
||||
|
||||
if (additionalReqParamsFailFast) {
|
||||
logger.debugv("The OIDC additional parameter '{0}''s size is longer ({1}) than allowed ({2}).", paramName, value.length(), additionalReqParamsMaxSize);
|
||||
throw new ErrorResponseException(OAuthErrorException.INVALID_REQUEST, "The OIDC additional parameter '" + paramName + "'s size is longer (" + value.length() + ") than allowed (" + additionalReqParamsMaxSize + ").", Response.Status.BAD_REQUEST);
|
||||
} else {
|
||||
logger.debugv("The OIDC additional parameter '{0}''s size is longer ({1}) than allowed ({2}).", paramName, value.length(), additionalReqParamsMaxSize);
|
||||
break;
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
logger.debugv("Adding OIDC additional parameter ''{0}'' as additional parameter.", paramName);
|
||||
currentAdditionalReqParamMaxOverallSize += value.length();
|
||||
additionalReqParams.put(paramName, value);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@ -48,6 +48,7 @@ public class AuthzEndpointParParser extends AuthzEndpointRequestParser {
|
||||
private String invalidRequestMessage = null;
|
||||
|
||||
public AuthzEndpointParParser(KeycloakSession session, ClientModel client, String requestUri) {
|
||||
super(session);
|
||||
this.session = session;
|
||||
this.client = client;
|
||||
SingleUseObjectProvider singleUseStore = session.singleUseObjects();
|
||||
|
||||
@ -48,7 +48,7 @@ public class ParEndpointRequestParserProcessor {
|
||||
try {
|
||||
AuthorizationEndpointRequest request = new AuthorizationEndpointRequest();
|
||||
|
||||
AuthzEndpointQueryStringParser parser = new AuthzEndpointQueryStringParser(requestParams, false);
|
||||
AuthzEndpointQueryStringParser parser = new AuthzEndpointQueryStringParser(session, requestParams, false);
|
||||
parser.parseRequest(request);
|
||||
|
||||
if (parser.getInvalidRequestMessage() != null) {
|
||||
|
||||
@ -0,0 +1,49 @@
|
||||
package org.keycloak.testsuite.authz;
|
||||
|
||||
import jakarta.ws.rs.client.Client;
|
||||
import jakarta.ws.rs.core.Response;
|
||||
import org.apache.commons.lang.RandomStringUtils;
|
||||
import org.junit.Test;
|
||||
import org.keycloak.representations.idm.RealmRepresentation;
|
||||
import org.keycloak.testsuite.AbstractTestRealmKeycloakTest;
|
||||
import org.keycloak.testsuite.util.AdminClientUtil;
|
||||
import org.keycloak.testsuite.util.Matchers;
|
||||
import org.keycloak.testsuite.util.RealmBuilder;
|
||||
|
||||
import java.util.HashMap;
|
||||
|
||||
import static org.hamcrest.MatcherAssert.assertThat;
|
||||
import static org.hamcrest.Matchers.containsString;
|
||||
import static org.hamcrest.Matchers.equalTo;
|
||||
import static org.hamcrest.Matchers.is;
|
||||
|
||||
public class AuthzEndpointRequestParserTest extends AbstractTestRealmKeycloakTest {
|
||||
|
||||
@Override
|
||||
public void configureTestRealm(RealmRepresentation testRealm) {
|
||||
}
|
||||
|
||||
@Test
|
||||
public void test_authentication_backwards_compatible() {
|
||||
|
||||
try (Client client = AdminClientUtil.createResteasyClient()) {
|
||||
|
||||
oauth.addCustomParameter("paramkey1_too_long", RandomStringUtils.random(2000 + 1));
|
||||
oauth.addCustomParameter("paramkey2", "paramvalue2");
|
||||
oauth.addCustomParameter("paramkey3", "paramvalue3");
|
||||
oauth.addCustomParameter("paramkey4", "paramvalue4");
|
||||
oauth.addCustomParameter("paramkey5", "paramvalue5");
|
||||
oauth.addCustomParameter("paramkey6_too_many", "paramvalue6");
|
||||
|
||||
try (Response response = client.target(oauth.getLoginFormUrl()).request().get()) {
|
||||
|
||||
assertThat(response.getStatus(), is(equalTo(200)));
|
||||
assertThat(response, Matchers.body(containsString("Sign in")));
|
||||
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
}
|
||||
Loading…
x
Reference in New Issue
Block a user