libre.sh/keycloak-scim
2
0
Fork 0
Code Issues 14 Pull requests Releases Packages Activity Actions 6

Avoid NPE if RelayState is null and return a proper error

Browse source 
Closes https://github.com/keycloak/keycloak/issues/24079

Signed-off-by: rmartinc <rmartinc@redhat.com>
This commit is contained in:
rmartinc 2023-11-07 09:31:46 +01:00 committed by Alexander Schwartz
parent 4a8f65286c
commit cca33baac3
2 changed files with 52 additions and 4 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

10
services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java
View file

@ -109,6 +109,7 @@ import org.keycloak.saml.validators.ConditionsValidator;
import org.keycloak.saml.validators.DestinationValidator;
import org.keycloak.services.util.CacheControlUtil;
import org.keycloak.sessions.AuthenticationSessionModel;
import org.keycloak.utils.StringUtil;
import org.w3c.dom.Element;
import org.w3c.dom.NodeList;
@ -412,10 +413,15 @@ public class SAMLEndpoint {
try {
AuthenticationSessionModel authSession;
if (clientId != null && ! clientId.trim().isEmpty()) {
if (StringUtil.isNotBlank(clientId)) {
authSession = samlIdpInitiatedSSO(clientId);
} else {
} else if (StringUtil.isNotBlank(relayState)) {
authSession = callback.getAndVerifyAuthenticationSession(relayState);
} else {
logger.error("SAML RelayState parameter was null when it should be returned by the IDP");
event.event(EventType.LOGIN);
event.error(Errors.INVALID_SAML_RESPONSE);
return ErrorPage.error(session, null, Response.Status.BAD_REQUEST, Messages.IDENTITY_PROVIDER_UNEXPECTED_ERROR);
}
session.getContext().setAuthenticationSession(authSession);

46
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlBrokerTest.java
View file

@ -18,6 +18,7 @@ import org.keycloak.models.IdentityProviderMapperSyncMode;
import org.keycloak.representations.idm.IdentityProviderMapperRepresentation;
import org.keycloak.representations.idm.RoleRepresentation;
import org.keycloak.representations.idm.UserRepresentation;
import org.keycloak.saml.common.constants.GeneralConstants;
import org.keycloak.saml.common.constants.JBossSAMLURIConstants;
import org.keycloak.saml.common.exceptions.ConfigurationException;
import org.keycloak.saml.common.exceptions.ParsingException;
@ -31,6 +32,7 @@ import org.keycloak.testsuite.util.AccountHelper;
import org.keycloak.testsuite.util.SamlClient;
import org.keycloak.testsuite.util.SamlClient.Binding;
import org.keycloak.testsuite.util.SamlClientBuilder;
import org.keycloak.testsuite.util.saml.ModifySamlResponseStepBuilder;
import java.io.Closeable;
import java.util.Arrays;
@ -40,22 +42,27 @@ import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import jakarta.ws.rs.core.Response;
import java.net.URI;
import java.util.List;
import org.apache.http.NameValuePair;
import org.apache.http.client.methods.HttpUriRequest;
import org.hamcrest.Matchers;
import org.junit.Assert;
import org.junit.Test;
import org.w3c.dom.Document;
import org.w3c.dom.Element;
import static org.hamcrest.Matchers.hasItems;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.keycloak.testsuite.broker.BrokerTestTools.getConsumerRoot;
import static org.keycloak.testsuite.saml.RoleMapperTest.ROLE_ATTRIBUTE_NAME;
import static org.keycloak.testsuite.util.Matchers.isSamlResponse;
import static org.keycloak.testsuite.util.Matchers.statusCodeIsHC;
import static org.keycloak.testsuite.util.SamlStreams.assertionsUnencrypted;
import static org.keycloak.testsuite.util.SamlStreams.attributeStatements;
import static org.keycloak.testsuite.util.SamlStreams.attributesUnecrypted;
import static org.keycloak.testsuite.broker.BrokerTestTools.getConsumerRoot;
import static org.keycloak.testsuite.util.Matchers.bodyHC;
/**
* Final class as it's not intended to be overriden. Feel free to remove "final" if you really know what you are doing.
@ -491,6 +498,41 @@ public final class KcSamlBrokerTest extends AbstractAdvancedBrokerTest {
.execute(hr -> assertThat(hr, statusCodeIsHC(Response.Status.BAD_REQUEST))); // Response from consumer IdP
}
// helper builder class that removes the RelayState parameter to simulate the error
private static class ModifySamlResponseStepRemoveRelayStateBuilder extends ModifySamlResponseStepBuilder {
public ModifySamlResponseStepRemoveRelayStateBuilder(Binding binding, SamlClientBuilder clientBuilder) {
super(binding, clientBuilder);
}
@Override
protected HttpUriRequest createRequest(URI locationUri, String attributeName, String samlDoc, List<NameValuePair> parameters) throws Exception {
// remove the RelayState parameter to trigger the error
return super.createRequest(locationUri, attributeName, samlDoc,
parameters.stream().filter(p -> !GeneralConstants.RELAY_STATE.equals(p.getName())).collect(Collectors.toList()));
}
}
@Test
public void loginInResponseNoRelayState() throws Exception {
AuthnRequestType loginRep = SamlClient.createLoginRequestDocument(AbstractSamlTest.SAML_CLIENT_ID_SALES_POST, getConsumerRoot() + "/sales-post/saml", null);
Document doc = SAML2Request.convert(loginRep);
SamlClientBuilder builder = new SamlClientBuilder()
.authnRequest(getConsumerSamlEndpoint(bc.consumerRealmName()), doc, Binding.POST).build() // Request to consumer IdP
.login().idp(bc.getIDPAlias()).build()
.processSamlResponse(Binding.POST).build() // AuthnRequest to producer IdP
.login().user(bc.getUserLogin(), bc.getUserPassword()).build();
builder.addStepBuilder(new ModifySamlResponseStepRemoveRelayStateBuilder(Binding.POST, builder)) // Response from producer IdP but remove RelayState
.build()
.execute(hr -> {
assertThat(hr, statusCodeIsHC(Response.Status.BAD_REQUEST));
assertThat(hr, bodyHC(Matchers.containsString("Unexpected error when authenticating with identity provider")));
});
}
private Document tamperInResponseTo(Document orig) {
Element rootElement = orig.getDocumentElement();
rootElement.setAttribute(SAMLProtocolQNames.ATTR_IN_RESPONSE_TO.getQName().getLocalPart(), "TAMPERED_" + rootElement.getAttribute("InResponseTo"));