From 5b9eb0fe190eaa34ec914f9383bcc798d8ebaefa Mon Sep 17 00:00:00 2001 From: rmartinc Date: Mon, 27 Jan 2020 09:37:37 +0100 Subject: [PATCH] KEYCLOAK-10884: Need clock skew for SAML identity provider --- .../models/IdentityProviderModel.java | 2 + .../oidc/OIDCIdentityProviderConfig.java | 4 +- .../keycloak/broker/saml/SAMLEndpoint.java | 3 +- .../saml/SAMLIdentityProviderConfig.java | 23 ++++ .../KcSamlBrokerAllowedClockSkewTest.java | 105 ++++++++++++++++++ .../theme/base/admin/resources/js/app.js | 15 +++ .../realm-identity-provider-saml.html | 9 +- 7 files changed, 157 insertions(+), 4 deletions(-) create mode 100644 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlBrokerAllowedClockSkewTest.java diff --git a/server-spi/src/main/java/org/keycloak/models/IdentityProviderModel.java b/server-spi/src/main/java/org/keycloak/models/IdentityProviderModel.java index 63b8a475ba..6fb5c59679 100755 --- a/server-spi/src/main/java/org/keycloak/models/IdentityProviderModel.java +++ b/server-spi/src/main/java/org/keycloak/models/IdentityProviderModel.java @@ -28,6 +28,8 @@ import java.util.Map; */ public class IdentityProviderModel implements Serializable { + public static final String ALLOWED_CLOCK_SKEW = "allowedClockSkew"; + private String internalId; /** diff --git a/services/src/main/java/org/keycloak/broker/oidc/OIDCIdentityProviderConfig.java b/services/src/main/java/org/keycloak/broker/oidc/OIDCIdentityProviderConfig.java index 436d322fbd..bb3d286f60 100755 --- a/services/src/main/java/org/keycloak/broker/oidc/OIDCIdentityProviderConfig.java +++ b/services/src/main/java/org/keycloak/broker/oidc/OIDCIdentityProviderConfig.java @@ -111,12 +111,12 @@ public class OIDCIdentityProviderConfig extends OAuth2IdentityProviderConfig { } public int getAllowedClockSkew() { - String allowedClockSkew = getConfig().get("allowedClockSkew"); + String allowedClockSkew = getConfig().get(ALLOWED_CLOCK_SKEW); if (allowedClockSkew == null || allowedClockSkew.isEmpty()) { return 0; } try { - return Integer.parseInt(getConfig().get("allowedClockSkew")); + return Integer.parseInt(getConfig().get(ALLOWED_CLOCK_SKEW)); } catch (NumberFormatException e) { // ignore it and use default return 0; diff --git a/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java b/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java index df4986cb98..971d941311 100755 --- a/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java +++ b/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java @@ -433,7 +433,8 @@ public class SAMLEndpoint { identity.setToken(samlResponse); } - ConditionsValidator.Builder cvb = new ConditionsValidator.Builder(assertion.getID(), assertion.getConditions(), destinationValidator); + ConditionsValidator.Builder cvb = new ConditionsValidator.Builder(assertion.getID(), assertion.getConditions(), destinationValidator) + .clockSkewInMillis(1000 * config.getAllowedClockSkew()); try { String issuerURL = getEntityId(session.getContext().getUri(), realm); cvb.addAllowedAudience(URI.create(issuerURL)); diff --git a/services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProviderConfig.java b/services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProviderConfig.java index c6d999c3e2..173c4814ac 100755 --- a/services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProviderConfig.java +++ b/services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProviderConfig.java @@ -230,4 +230,27 @@ public class SAMLIdentityProviderConfig extends IdentityProviderModel { : xmlSigKeyInfoKeyNameTransformer.name()); } + public int getAllowedClockSkew() { + int result = 0; + String allowedClockSkew = getConfig().get(ALLOWED_CLOCK_SKEW); + if (allowedClockSkew != null && !allowedClockSkew.isEmpty()) { + try { + result = Integer.parseInt(allowedClockSkew); + if (result < 0) { + result = 0; + } + } catch (NumberFormatException e) { + // ignore it and use 0 + } + } + return result; + } + + public void setAllowedClockSkew(int allowedClockSkew) { + if (allowedClockSkew < 0) { + getConfig().remove(ALLOWED_CLOCK_SKEW); + } else { + getConfig().put(ALLOWED_CLOCK_SKEW, String.valueOf(allowedClockSkew)); + } + } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlBrokerAllowedClockSkewTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlBrokerAllowedClockSkewTest.java new file mode 100644 index 0000000000..fb85f8d26f --- /dev/null +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcSamlBrokerAllowedClockSkewTest.java @@ -0,0 +1,105 @@ +/* + * Copyright 2020 Red Hat, Inc. and/or its affiliates + * and other contributors as indicated by the @author tags. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.keycloak.testsuite.broker; + +import java.io.Closeable; +import javax.ws.rs.core.Response; +import org.hamcrest.Matchers; +import org.junit.Assert; +import static org.junit.Assert.assertThat; +import org.junit.Test; +import org.keycloak.broker.saml.SAMLIdentityProviderConfig; +import org.keycloak.dom.saml.v2.protocol.AuthnRequestType; +import org.keycloak.saml.common.constants.JBossSAMLURIConstants; +import org.keycloak.saml.processing.api.saml.v2.request.SAML2Request; +import org.keycloak.saml.processing.core.saml.v2.common.SAMLDocumentHolder; +import org.keycloak.testsuite.saml.AbstractSamlTest; +import org.keycloak.testsuite.updaters.IdentityProviderAttributeUpdater; +import static org.keycloak.testsuite.util.Matchers.isSamlResponse; +import static org.keycloak.testsuite.util.Matchers.statusCodeIsHC; +import org.keycloak.testsuite.util.SamlClient; +import org.keycloak.testsuite.util.SamlClientBuilder; +import org.w3c.dom.Document; + +/** + * + * @author rmartinc + */ +public class KcSamlBrokerAllowedClockSkewTest extends AbstractInitializedBaseBrokerTest { + + @Override + protected BrokerConfiguration getBrokerConfiguration() { + return KcSamlBrokerConfiguration.INSTANCE; + } + + @Test + public void loginClientExpiredResponseFromIdP() throws Exception { + AuthnRequestType loginRep = SamlClient.createLoginRequestDocument(AbstractSamlTest.SAML_CLIENT_ID_SALES_POST, AbstractSamlTest.SAML_ASSERTION_CONSUMER_URL_SALES_POST, null); + + Document doc = SAML2Request.convert(loginRep); + + new SamlClientBuilder() + .authnRequest(getAuthServerSamlEndpoint(bc.consumerRealmName()), doc, SamlClient.Binding.POST).build() // Request to consumer IdP + .login().idp(bc.getIDPAlias()).build() + + .processSamlResponse(SamlClient.Binding.POST) // AuthnRequest to producer IdP + .targetAttributeSamlRequest() + .build() + + .login().user(bc.getUserLogin(), bc.getUserPassword()).build() + + .addStep(() -> KcSamlBrokerAllowedClockSkewTest.this.setTimeOffset(-30)) // offset to the past to invalidate the request + .processSamlResponse(SamlClient.Binding.POST) // Response from producer IdP should fail + .build() + .execute(hr -> assertThat(hr, statusCodeIsHC(Response.Status.BAD_REQUEST))); + } + + @Test + public void loginClientExpiredResponseFromIdPWithClockSkew() throws Exception { + try (Closeable idpUpdater = new IdentityProviderAttributeUpdater(identityProviderResource) + .setAttribute(SAMLIdentityProviderConfig.ALLOWED_CLOCK_SKEW, "60") + .update()) { + + AuthnRequestType loginRep = SamlClient.createLoginRequestDocument(AbstractSamlTest.SAML_CLIENT_ID_SALES_POST, AbstractSamlTest.SAML_ASSERTION_CONSUMER_URL_SALES_POST, null); + + Document doc = SAML2Request.convert(loginRep); + + SAMLDocumentHolder samlResponse = new SamlClientBuilder() + .authnRequest(getAuthServerSamlEndpoint(bc.consumerRealmName()), doc, SamlClient.Binding.POST).build() // Request to consumer IdP + .login().idp(bc.getIDPAlias()).build() + + .processSamlResponse(SamlClient.Binding.POST) // AuthnRequest to producer IdP + .targetAttributeSamlRequest() + .build() + + .login().user(bc.getUserLogin(), bc.getUserPassword()).build() + + .addStep(() -> KcSamlBrokerAllowedClockSkewTest.this.setTimeOffset(-30)) // offset to the past but inside the clock skew + .processSamlResponse(SamlClient.Binding.POST) // Response from producer IdP expired but valid with the clock skew + .build() + + // first-broker flow + .updateProfile().firstName("a").lastName("b").email(bc.getUserEmail()).username(bc.getUserLogin()).build() + .followOneRedirect() + + .getSamlResponse(SamlClient.Binding.POST); // Response from consumer IdP + + Assert.assertThat(samlResponse, Matchers.notNullValue()); + Assert.assertThat(samlResponse.getSamlObject(), isSamlResponse(JBossSAMLURIConstants.STATUS_SUCCESS)); + } + } +} \ No newline at end of file diff --git a/themes/src/main/resources/theme/base/admin/resources/js/app.js b/themes/src/main/resources/theme/base/admin/resources/js/app.js index eab16462ca..4cd91fa95f 100755 --- a/themes/src/main/resources/theme/base/admin/resources/js/app.js +++ b/themes/src/main/resources/theme/base/admin/resources/js/app.js @@ -3282,6 +3282,21 @@ module.directive('kcValidPage', function() { } }); +// Directive to parse/format strings into numbers +module.directive('stringToNumber', function() { + return { + require: 'ngModel', + link: function(scope, element, attrs, ngModel) { + ngModel.$parsers.push(function(value) { + return (typeof value === 'undefined' || value === null)? '' : '' + value; + }); + ngModel.$formatters.push(function(value) { + return parseFloat(value); + }); + } + }; +}); + // filter used for paged tables module.filter('startFrom', function () { return function (input, start) { diff --git a/themes/src/main/resources/theme/base/admin/resources/partials/realm-identity-provider-saml.html b/themes/src/main/resources/theme/base/admin/resources/partials/realm-identity-provider-saml.html index aa6733a3ba..1c74371d1d 100755 --- a/themes/src/main/resources/theme/base/admin/resources/partials/realm-identity-provider-saml.html +++ b/themes/src/main/resources/theme/base/admin/resources/partials/realm-identity-provider-saml.html @@ -229,6 +229,13 @@ {{:: 'validating-x509-certificate.tooltip' | translate}} +
+ +
+ +
+ {{:: 'identity-provider.allowed-clock-skew.tooltip' | translate}} +
{{:: 'import-external-idp-config' | translate}} {{:: 'import-external-idp-config.tooltip' | translate}} @@ -274,4 +281,4 @@ - \ No newline at end of file +