[java-idp-oidc] branch main updated: JOIDC-82 - Dyn.reg. profile config setting secretExpirationPeriod is not honored
Henri Mikkonen
henri.mikkonen at iki.fi
Fri Mar 25 10:42:54 UTC 2022
This is an automated email from the git hooks/post-receive script.
hjmikkon pushed a commit to branch main
in repository java-idp-oidc.
View the commit online:
http://git.shibboleth.net/view/?p=java-idp-oidc.git;a=commit;h=bd83f1da8236a5a2473d0a011fbc36b087cb4ceb
The following commit(s) were added to refs/heads/main by this push:
new bd83f1da JOIDC-82 - Dyn.reg. profile config setting secretExpirationPeriod is not honored
bd83f1da is described below
commit bd83f1da8236a5a2473d0a011fbc36b087cb4ceb
Author: Henri Mikkonen <henri.mikkonen at iki.fi>
AuthorDate: Fri Mar 25 12:40:03 2022 +0200
JOIDC-82 - Dyn.reg. profile config setting secretExpirationPeriod is not honored
https://shibboleth.atlassian.net/browse/JOIDC-82
"client_secret_expires_at":0 is now added to the dynamic registration responses
involving secrets. A WARN message is logged by BuildClientInformation if the expiration
has been set to the profile configuration. The message tells that the expiration time
is ignored.
---
.../op/profile/impl/BuildClientInformation.java | 75 +++++++--
.../oidc/op/profile/impl/GenerateClientSecret.java | 4 +-
.../idp/service/relying-party/postconfig.xml | 4 +-
.../oidc/op/profile/flow/RegistrationFlowTest.java | 1 +
.../profile/impl/BuildClientInformationTest.java | 173 +++++++++++++++++++++
5 files changed, 240 insertions(+), 17 deletions(-)
diff --git a/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/profile/impl/BuildClientInformation.java b/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/profile/impl/BuildClientInformation.java
index a58b3f68..e49e9e1a 100644
--- a/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/profile/impl/BuildClientInformation.java
+++ b/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/profile/impl/BuildClientInformation.java
@@ -25,6 +25,8 @@ import javax.annotation.Nonnull;
import org.opensaml.messaging.context.MessageContext;
import org.opensaml.messaging.context.navigate.ChildContextLookup;
+import org.opensaml.profile.action.ActionSupport;
+import org.opensaml.profile.action.EventIds;
import org.opensaml.profile.context.ProfileRequestContext;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -41,6 +43,7 @@ import net.shibboleth.idp.plugin.oidc.op.messaging.context.OIDCClientRegistratio
import net.shibboleth.idp.profile.AbstractProfileAction;
import net.shibboleth.utilities.java.support.component.ComponentSupport;
import net.shibboleth.utilities.java.support.logic.Constraint;
+import net.shibboleth.utilities.java.support.primitive.StringSupport;
/**
* An action that uses the information from {@link OIDCClientRegistrationResponseContext} attached to the message
@@ -57,6 +60,12 @@ public class BuildClientInformation extends AbstractProfileAction {
*/
@Nonnull private Function<MessageContext,OIDCClientRegistrationResponseContext> oidcResponseContextLookupStrategy;
+ /** The {@link MessageContext} to operate on. */
+ private MessageContext messageContext;
+
+ /** The {@link OIDCClientRegistrationResponseContext} to operate on. */
+ private OIDCClientRegistrationResponseContext oidcResponseContext;
+
/** Constructor. */
public BuildClientInformation() {
oidcResponseContextLookupStrategy = new ChildContextLookup<>(OIDCClientRegistrationResponseContext.class);
@@ -77,28 +86,68 @@ public class BuildClientInformation extends AbstractProfileAction {
"OIDCClientRegistrationResponseContext lookup strategy cannot be null");
}
+ /** {@inheritDoc} */
+ @Override
+ protected boolean doPreExecute(@Nonnull final ProfileRequestContext profileRequestContext) {
+ if (!super.doPreExecute(profileRequestContext)) {
+ return false;
+ }
+
+ messageContext = profileRequestContext.getOutboundMessageContext();
+ if (messageContext == null) {
+ log.error("{} No message context found", getLogPrefix());
+ ActionSupport.buildEvent(profileRequestContext, EventIds.INVALID_PROFILE_CTX);
+ return false;
+ }
+
+ oidcResponseContext = oidcResponseContextLookupStrategy.apply(messageContext);
+ if (oidcResponseContext == null) {
+ log.error("{} No OIDC response context found", getLogPrefix());
+ ActionSupport.buildEvent(profileRequestContext, EventIds.INVALID_MSG_CTX);
+ return false;
+ }
+
+ return true;
+ }
+
/** {@inheritDoc} */
@Override
protected void doExecute(@Nonnull final ProfileRequestContext profileRequestContext) {
- final OIDCClientRegistrationResponseContext oidcContext =
- oidcResponseContextLookupStrategy.apply(profileRequestContext.getOutboundMessageContext());
- final ClientID clientId = new ClientID(oidcContext.getClientId());
- final OIDCClientMetadata metadata = oidcContext.getClientMetadata();
- final ClientAuthenticationMethod tokenAuthMethod = metadata.getTokenEndpointAuthMethod();
+ final String id = oidcResponseContext.getClientId();
+ if (StringSupport.trimOrNull(id) == null) {
+ log.error("{} No client ID in the OIDC response context", getLogPrefix());
+ ActionSupport.buildEvent(profileRequestContext, EventIds.INVALID_MSG_CTX);
+ return;
+ }
+ final ClientID clientId = new ClientID(id);
- final boolean secretNeeded = tokenAuthMethod.equals(ClientAuthenticationMethod.CLIENT_SECRET_BASIC) ||
+ final OIDCClientMetadata metadata = oidcResponseContext.getClientMetadata();
+ if (metadata == null) {
+ log.error("{} No client metadata in the OIDC response context", getLogPrefix());
+ ActionSupport.buildEvent(profileRequestContext, EventIds.INVALID_MSG_CTX);
+ return;
+ }
+
+ final ClientAuthenticationMethod tokenAuthMethod = metadata.getTokenEndpointAuthMethod();
+
+ final boolean secretNeeded = tokenAuthMethod == null ||
+ tokenAuthMethod.equals(ClientAuthenticationMethod.CLIENT_SECRET_BASIC) ||
tokenAuthMethod.equals(ClientAuthenticationMethod.CLIENT_SECRET_JWT) ||
tokenAuthMethod.equals(ClientAuthenticationMethod.CLIENT_SECRET_POST);
-
+
final Secret clientSecret;
- if (secretNeeded && oidcContext.getClientSecret() != null) {
- final Instant secretExpiresAt = oidcContext.getClientSecretExpiresAt();
+ if (secretNeeded) {
+ if (StringSupport.trimOrNull(oidcResponseContext.getClientSecret()) == null) {
+ log.error("{} No required client secret in the OIDC response context", getLogPrefix());
+ ActionSupport.buildEvent(profileRequestContext, EventIds.INVALID_MSG_CTX);
+ return;
+ }
+ final Instant secretExpiresAt = oidcResponseContext.getClientSecretExpiresAt();
if (secretExpiresAt != null) {
- clientSecret = new Secret(oidcContext.getClientSecret(), Date.from(secretExpiresAt));
- } else {
- clientSecret = new Secret(oidcContext.getClientSecret());
+ log.warn("{} client secret expiration time {} is ignored", getLogPrefix(), secretExpiresAt);
}
+ clientSecret = new Secret(oidcResponseContext.getClientSecret());
} else {
clientSecret = null;
}
@@ -106,7 +155,7 @@ public class BuildClientInformation extends AbstractProfileAction {
final OIDCClientInformation clientInformation = new OIDCClientInformation(clientId, new Date(),
metadata, clientSecret);
final OIDCClientInformationResponse response = new OIDCClientInformationResponse(clientInformation, true);
- profileRequestContext.getOutboundMessageContext().setMessage(response);
+ messageContext.setMessage(response);
log.info("{} Client information successfully added to the outbound context", getLogPrefix());
}
diff --git a/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/profile/impl/GenerateClientSecret.java b/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/profile/impl/GenerateClientSecret.java
index 12f5dbdc..988de688 100644
--- a/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/profile/impl/GenerateClientSecret.java
+++ b/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/profile/impl/GenerateClientSecret.java
@@ -150,8 +150,8 @@ public class GenerateClientSecret extends AbstractProfileAction {
Duration lifetime = secretExpirationPeriodStrategy != null ?
secretExpirationPeriodStrategy.apply(profileRequestContext) : null;
if (lifetime == null) {
- log.debug("{} No secret expiration period supplied, using default", getLogPrefix());
- lifetime = Duration.ofDays(365);
+ log.debug("{} No secret expiration period supplied, using default (non-expiring)", getLogPrefix());
+ lifetime = Duration.ZERO;
}
final Instant now = Instant.now();
final Instant expiration = now.plus(lifetime);
diff --git a/idp-oidc-extension-impl/src/main/resources/META-INF/net/shibboleth/idp/service/relying-party/postconfig.xml b/idp-oidc-extension-impl/src/main/resources/META-INF/net/shibboleth/idp/service/relying-party/postconfig.xml
index 3e626786..e79e5955 100644
--- a/idp-oidc-extension-impl/src/main/resources/META-INF/net/shibboleth/idp/service/relying-party/postconfig.xml
+++ b/idp-oidc-extension-impl/src/main/resources/META-INF/net/shibboleth/idp/service/relying-party/postconfig.xml
@@ -46,7 +46,7 @@
class="net.shibboleth.oidc.profile.config.OIDCDynamicRegistrationConfiguration"
p:issuer-ref="shibboleth.oidc.issuer"
p:registrationValidityPeriod="%{idp.oidc.dynreg.defaultRegistrationValidity:PT24H}"
- p:secretExpirationPeriod="%{idp.oidc.dynreg.defaultSecretExpiration:P12M}"
+ p:secretExpirationPeriod="%{idp.oidc.dynreg.defaultSecretExpiration:0}"
p:tokenEndpointAuthMethods="%{idp.oidc.dynreg.tokenEndpointAuthMethods:client_secret_basic,client_secret_post,client_secret_jwt,private_key_jwt}"
p:claimsValidator-ref="DefaultJWTClaimsValidator"
p:metadataPolicyLookupStrategy-ref="shibboleth.oidc.dynreg.DefaultMetadataPolicyLookupStrategy" />
@@ -332,7 +332,7 @@
</property>
<property name="secretExpirationPeriodLookupStrategy">
<bean parent="shibboleth.MDDrivenDurationProperty" p:propertyName="secretExpirationPeriod"
- p:defaultValue="%{idp.oidc.dynreg.defaultSecretExpiration:P12M}" />
+ p:defaultValue="%{idp.oidc.dynreg.defaultSecretExpiration:0}" />
</property>
<property name="metadataPolicyLookupStrategy">
<bean parent="shibboleth.MDDrivenBeanProperty" p:propertyName="metadataPolicy"
diff --git a/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/flow/RegistrationFlowTest.java b/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/flow/RegistrationFlowTest.java
index db9418a2..b86faa7e 100644
--- a/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/flow/RegistrationFlowTest.java
+++ b/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/flow/RegistrationFlowTest.java
@@ -281,6 +281,7 @@ public class RegistrationFlowTest extends AbstractOidcFlowTest {
Assert.fail();
}
Assert.assertEquals(storedMetadata.getPolicyURIEntries(), metadata.getPolicyURIEntries());
+ Assert.assertNull(clientInfo.getSecret().getExpirationDate());
}
protected BearerAccessToken buildRegistrationAccessToken(final boolean replacement, final String redirectUriSubset,
diff --git a/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/impl/BuildClientInformationTest.java b/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/impl/BuildClientInformationTest.java
new file mode 100644
index 00000000..cb670a7f
--- /dev/null
+++ b/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/impl/BuildClientInformationTest.java
@@ -0,0 +1,173 @@
+/*
+ * Licensed to the University Corporation for Advanced Internet Development,
+ * Inc. (UCAID) under one or more contributor license agreements. See the
+ * NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The UCAID licenses this file to You 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 net.shibboleth.idp.plugin.oidc.op.profile.impl;
+
+import java.time.Instant;
+
+import org.opensaml.messaging.context.MessageContext;
+import org.opensaml.profile.action.EventIds;
+import org.opensaml.profile.context.ProfileRequestContext;
+import org.opensaml.profile.testing.ActionTestingSupport;
+import org.testng.Assert;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import com.nimbusds.oauth2.sdk.auth.ClientAuthenticationMethod;
+import com.nimbusds.oauth2.sdk.auth.Secret;
+import com.nimbusds.oauth2.sdk.id.ClientID;
+import com.nimbusds.openid.connect.sdk.rp.OIDCClientInformationResponse;
+import com.nimbusds.openid.connect.sdk.rp.OIDCClientMetadata;
+
+import net.shibboleth.idp.plugin.oidc.op.messaging.context.OIDCClientRegistrationResponseContext;
+import net.shibboleth.utilities.java.support.component.ComponentInitializationException;
+
+/**
+ * Unit tests for {@link BuildClientInformation}.
+ */
+public class BuildClientInformationTest {
+
+ protected BuildClientInformation action;
+
+ protected ProfileRequestContext profileRequestCtx;
+
+ protected MessageContext messageCtx;
+
+ protected OIDCClientRegistrationResponseContext registrationCtx;
+
+ protected OIDCClientMetadata metadata;
+
+ protected String clientId;
+
+ protected String clientSecret;
+
+ @BeforeMethod
+ public void setup() throws ComponentInitializationException {
+ action = new BuildClientInformation();
+ action.initialize();
+ profileRequestCtx = new ProfileRequestContext();
+ messageCtx = new MessageContext();
+ Assert.assertNull(messageCtx.getMessage());
+ profileRequestCtx.setOutboundMessageContext(messageCtx);
+ registrationCtx = profileRequestCtx.getOutboundMessageContext().getSubcontext(OIDCClientRegistrationResponseContext.class, true);
+ clientId = "mockClientId";
+ clientSecret = "mockSecret";
+ metadata = new OIDCClientMetadata();
+ registrationCtx.setClientId(clientId);
+ registrationCtx.setClientSecret(clientSecret);
+ registrationCtx.setClientMetadata(metadata);
+ }
+
+ @Test
+ public void noOutboundMessageContext() {
+ final ProfileRequestContext localPrc = new ProfileRequestContext();
+ action.execute(localPrc);
+ ActionTestingSupport.assertEvent(localPrc, EventIds.INVALID_PROFILE_CTX);
+ }
+
+ @Test
+ public void noMetadataContext() {
+ final ProfileRequestContext localPrc = new ProfileRequestContext();
+ localPrc.setOutboundMessageContext(new MessageContext());
+ action.execute(localPrc);
+ ActionTestingSupport.assertEvent(localPrc, EventIds.INVALID_MSG_CTX);
+ }
+
+ @Test
+ public void noClientIdInContext() {
+ registrationCtx.setClientId(null);
+ action.execute(profileRequestCtx);
+ ActionTestingSupport.assertEvent(profileRequestCtx, EventIds.INVALID_MSG_CTX);
+ }
+
+ @Test
+ public void noClientMetadataInContext() {
+ registrationCtx.setClientMetadata(null);
+ action.execute(profileRequestCtx);
+ ActionTestingSupport.assertEvent(profileRequestCtx, EventIds.INVALID_MSG_CTX);
+ }
+
+ @Test
+ public void noClientSecretInContextWhenRequired() {
+ registrationCtx.setClientSecret(null);
+ action.execute(profileRequestCtx);
+ ActionTestingSupport.assertEvent(profileRequestCtx, EventIds.INVALID_MSG_CTX);
+ }
+
+ @Test
+ public void noClientSecretInContextRequiredWithPrivateKeyJWT() {
+ registrationCtx.setClientSecret(null);
+ metadata.setTokenEndpointAuthMethod(ClientAuthenticationMethod.PRIVATE_KEY_JWT);
+ action.execute(profileRequestCtx);
+ assertSuccessfulResponse(false);
+ }
+
+ @Test
+ public void noTokenEndpointAuthMethodCreatesSecret() {
+ action.execute(profileRequestCtx);
+ assertSuccessfulResponse();
+ }
+
+ @Test
+ public void basicTokenEndpointAuthMethodCreatesSecret() {
+ metadata.setTokenEndpointAuthMethod(ClientAuthenticationMethod.CLIENT_SECRET_BASIC);
+ action.execute(profileRequestCtx);
+ assertSuccessfulResponse();
+ }
+
+ @Test
+ public void postTokenEndpointAuthMethodCreatesSecret() {
+ metadata.setTokenEndpointAuthMethod(ClientAuthenticationMethod.CLIENT_SECRET_POST);
+ action.execute(profileRequestCtx);
+ assertSuccessfulResponse();
+ }
+
+ @Test
+ public void jwtSecretTokenEndpointAuthMethodCreatesSecret() {
+ metadata.setTokenEndpointAuthMethod(ClientAuthenticationMethod.CLIENT_SECRET_JWT);
+ action.execute(profileRequestCtx);
+ assertSuccessfulResponse();
+ }
+
+ @Test
+ public void noTokenEndpointAuthMethodCreatesSecret_expirationTimeIgnored() {
+ registrationCtx.setClientSecretExpiresAt(Instant.now().plusSeconds(60));
+ action.execute(profileRequestCtx);
+ assertSuccessfulResponse();
+ }
+
+ protected void assertSuccessfulResponse() {
+ assertSuccessfulResponse(true);
+ }
+
+ protected void assertSuccessfulResponse(boolean secret) {
+ ActionTestingSupport.assertProceedEvent(profileRequestCtx);
+ final OIDCClientInformationResponse response = (OIDCClientInformationResponse) messageCtx.getMessage();
+ Assert.assertNotNull(response);
+ Assert.assertEquals(response.getOIDCClientInformation().getID(), new ClientID(clientId));
+ if (secret) {
+ assertSecret(response);
+ }
+ }
+
+ protected void assertSecret(final OIDCClientInformationResponse response) {
+ final Secret secret = response.getOIDCClientInformation().getSecret();
+ Assert.assertNotNull(secret);
+ Assert.assertEquals(secret.getValue(), clientSecret);
+ Assert.assertNull(secret.getExpirationDate());
+ }
+}
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.
More information about the commits
mailing list