[java-idp-oidc] branch main updated: JOIDC-91 Public clients are not able to access the token endpoint
Henri Mikkonen
henri.mikkonen at iki.fi
Tue Apr 19 11:35:16 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=420dc7ad759f0bff56215a3f9bc7a43f190ca6d3
The following commit(s) were added to refs/heads/main by this push:
new 420dc7ad JOIDC-91 Public clients are not able to access the token endpoint
420dc7ad is described below
commit 420dc7ad759f0bff56215a3f9bc7a43f190ca6d3
Author: Henri Mikkonen <henri.mikkonen at iki.fi>
AuthorDate: Tue Apr 19 14:32:37 2022 +0300
JOIDC-91 Public clients are not able to access the token endpoint
https://shibboleth.atlassian.net/browse/JOIDC-91
Refactored the OAuth2Client flow actions:
- ExtractClientAuthenticationFromRequest: build NO_CREDENTIALS event only if ‘none’ type is not enabled in the profile configuration
- OIDCClientInfoCredentialValidator: if ‘none’ is registered in the RP metadata, populate the subject already in the overriding doValidate() -method, before letting parent class to populate UsernamePasswordContext
- ValidateClientAuthenticationType: Don’t expect clientID to be found in all clientAuthentication methods
Improved flow testing to cover public client use cases.
---
.../ExtractClientAuthenticationFromRequest.java | 53 +++++++++++++++++----
.../impl/OIDCClientInfoCredentialValidator.java | 47 +++++++++++++-----
.../impl/ValidateClientAuthenticationType.java | 3 +-
.../plugin/oidc/op/profile/flow/TokenFlowTest.java | 55 ++++++++++++++++++++--
.../src/test/resources/conf/relying-party.xml | 24 ++++++++++
5 files changed, 155 insertions(+), 27 deletions(-)
diff --git a/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/authn/impl/ExtractClientAuthenticationFromRequest.java b/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/authn/impl/ExtractClientAuthenticationFromRequest.java
index 91b08341..953ffecd 100644
--- a/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/authn/impl/ExtractClientAuthenticationFromRequest.java
+++ b/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/authn/impl/ExtractClientAuthenticationFromRequest.java
@@ -17,6 +17,9 @@
package net.shibboleth.idp.plugin.oidc.op.authn.impl;
+import java.util.Set;
+import java.util.function.Function;
+
import javax.annotation.Nonnull;
import javax.annotation.Nullable;
@@ -39,6 +42,10 @@ import net.shibboleth.idp.authn.context.AuthenticationContext;
import net.shibboleth.idp.authn.context.CertificateContext;
import net.shibboleth.idp.authn.context.UsernamePasswordContext;
import net.shibboleth.oidc.authn.context.OAuth2ClientAuthenticationContext;
+import net.shibboleth.oidc.profile.config.navigate.TokenEndpointAuthMethodLookupFunction;
+import net.shibboleth.utilities.java.support.annotation.constraint.NonnullAfterInit;
+import net.shibboleth.utilities.java.support.component.ComponentSupport;
+import net.shibboleth.utilities.java.support.logic.Constraint;
/**
@@ -62,10 +69,34 @@ public class ExtractClientAuthenticationFromRequest extends AbstractExtractionAc
/** Class logger. */
@Nonnull private final Logger log = LoggerFactory.getLogger(ExtractClientAuthenticationFromRequest.class);
-
+
+ /** Lookup strategy for enabled client authentication methods. */
+ @NonnullAfterInit private Function<ProfileRequestContext, Set<ClientAuthenticationMethod>>
+ clientAuthMethodsLookupStrategy;
+
/** Message to extract credentials from. */
@Nullable private AbstractOptionallyAuthenticatedRequest request;
-
+
+ /**
+ * Constructor.
+ */
+ public ExtractClientAuthenticationFromRequest() {
+ clientAuthMethodsLookupStrategy = new TokenEndpointAuthMethodLookupFunction();
+ }
+
+ /**
+ * Set the lookup strategy for enabled client authentication methods.
+ *
+ * @param strategy What to set.
+ */
+ public void setClientAuthMethodsLookupStrategy(
+ @Nonnull final Function<ProfileRequestContext, Set<ClientAuthenticationMethod>> strategy) {
+ ComponentSupport.ifInitializedThrowUnmodifiabledComponentException(this);
+
+ clientAuthMethodsLookupStrategy = Constraint.isNotNull(strategy,
+ "Client authentication methods lookup strategy cannot be null");
+ }
+
/** {@inheritDoc} */
@Override
protected boolean doPreExecute(@Nonnull final ProfileRequestContext profileRequestContext,
@@ -94,16 +125,22 @@ public class ExtractClientAuthenticationFromRequest extends AbstractExtractionAc
@Nonnull final AuthenticationContext authenticationContext) {
final ClientAuthentication clientAuthentication = request.getClientAuthentication();
- if (clientAuthentication == null) {
- log.debug("{} No OAuth client credentials in request", getLogPrefix());
- ActionSupport.buildEvent(profileRequestContext, AuthnEventIds.NO_CREDENTIALS);
- return;
- }
final OAuth2ClientAuthenticationContext ctx =
authenticationContext.getSubcontext(OAuth2ClientAuthenticationContext.class, true);
ctx.setClientAuthentication(clientAuthentication);
-
+
+ if (clientAuthentication == null) {
+ log.debug("{} No OAuth client credentials in request", getLogPrefix());
+ final Set<ClientAuthenticationMethod> methods =
+ clientAuthMethodsLookupStrategy.apply(profileRequestContext);
+ // Build event only if 'none' is not enabled in the profile configuration
+ if (methods == null || !methods.contains(ClientAuthenticationMethod.NONE)) {
+ ActionSupport.buildEvent(profileRequestContext, AuthnEventIds.NO_CREDENTIALS);
+ }
+ return;
+ }
+
// Note the Nimbus APIs appear to prevent the client ID or secret from being null.
if (ClientAuthenticationMethod.CLIENT_SECRET_BASIC.equals(clientAuthentication.getMethod())) {
diff --git a/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/authn/impl/OIDCClientInfoCredentialValidator.java b/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/authn/impl/OIDCClientInfoCredentialValidator.java
index 9c22bc50..5b7e305f 100644
--- a/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/authn/impl/OIDCClientInfoCredentialValidator.java
+++ b/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/authn/impl/OIDCClientInfoCredentialValidator.java
@@ -29,6 +29,7 @@ import net.shibboleth.idp.authn.AbstractUsernamePasswordCredentialValidator;
import net.shibboleth.idp.authn.AuthnEventIds;
import net.shibboleth.idp.authn.context.AuthenticationContext;
import net.shibboleth.idp.authn.context.UsernamePasswordContext;
+import net.shibboleth.idp.authn.principal.UsernamePrincipal;
import net.shibboleth.oidc.metadata.context.OIDCMetadataContext;
import net.shibboleth.utilities.java.support.annotation.constraint.NonnullAfterInit;
import net.shibboleth.utilities.java.support.annotation.constraint.ThreadSafeAfterInit;
@@ -44,7 +45,9 @@ import org.opensaml.profile.context.navigate.InboundMessageContextLookup;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import com.nimbusds.oauth2.sdk.auth.ClientAuthenticationMethod;
import com.nimbusds.oauth2.sdk.auth.Secret;
+import com.nimbusds.oauth2.sdk.client.ClientMetadata;
import com.nimbusds.openid.connect.sdk.rp.OIDCClientInformation;
/**
@@ -62,7 +65,9 @@ public class OIDCClientInfoCredentialValidator extends AbstractUsernamePasswordC
/** Digester for SHA-1. */
@NonnullAfterInit private StringDigester digester;
-
+
+ @Nullable private OIDCClientInformation clientInformation = null;
+
/** Constructor. */
public OIDCClientInfoCredentialValidator() {
oidcMetadataContextLookupStrategy = new ChildContextLookup<>(OIDCMetadataContext.class).compose(
@@ -93,31 +98,47 @@ public class OIDCClientInfoCredentialValidator extends AbstractUsernamePasswordC
throw new ComponentInitializationException("Error creating digester", e);
}
}
-
+
/** {@inheritDoc} */
@Override
- @Nullable protected Subject doValidate(@Nonnull final ProfileRequestContext profileRequestContext,
+ protected Subject doValidate(@Nonnull final ProfileRequestContext profileRequestContext,
@Nonnull final AuthenticationContext authenticationContext,
- @Nonnull final UsernamePasswordContext usernamePasswordContext,
@Nullable final WarningHandler warningHandler,
@Nullable final ErrorHandler errorHandler) throws Exception {
-
- OIDCClientInformation clientInformation = null;
-
+
final OIDCMetadataContext oidcMetadataContext = oidcMetadataContextLookupStrategy.apply(profileRequestContext);
if (oidcMetadataContext != null) {
clientInformation = oidcMetadataContext.getClientInformation();
}
- if (clientInformation == null) {
+ if (clientInformation == null || clientInformation.getOIDCMetadata() == null) {
log.debug("{} OIDC client metadata is missing", getLogPrefix());
return null;
- } else if (clientInformation.getSecret() == null) {
- log.debug("{} OIDC client metadata for '{}' missing client secret", getLogPrefix(),
- clientInformation.getID());
- return null;
+ } else {
+ final ClientMetadata clientMetadata = clientInformation.getMetadata();
+ if (ClientAuthenticationMethod.NONE.equals(clientMetadata.getTokenEndpointAuthMethod())) {
+ log.debug("{} OIDC client metadata contains 'none' type for endpoint authentication");
+ final Subject subject = new Subject();
+ subject.getPrincipals().add(new UsernamePrincipal(applyTransforms(clientInformation.getID().getValue())));
+ return super.populateSubject(subject);
+ } else if (clientInformation.getSecret() == null) {
+ log.debug("{} OIDC client metadata for '{}' missing client secret", getLogPrefix(),
+ clientInformation.getID());
+ return null;
+ }
}
-
+
+ return super.doValidate(profileRequestContext, authenticationContext, warningHandler, errorHandler);
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ @Nullable protected Subject doValidate(@Nonnull final ProfileRequestContext profileRequestContext,
+ @Nonnull final AuthenticationContext authenticationContext,
+ @Nonnull final UsernamePasswordContext usernamePasswordContext,
+ @Nullable final WarningHandler warningHandler,
+ @Nullable final ErrorHandler errorHandler) throws Exception {
+
final String username = usernamePasswordContext.getTransformedUsername();
log.debug("{} Attempting to authenticate effective client ID '{}' ", getLogPrefix(), username);
diff --git a/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/authn/impl/ValidateClientAuthenticationType.java b/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/authn/impl/ValidateClientAuthenticationType.java
index c52ea947..4b40da4e 100644
--- a/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/authn/impl/ValidateClientAuthenticationType.java
+++ b/idp-oidc-extension-impl/src/main/java/net/shibboleth/idp/plugin/oidc/op/authn/impl/ValidateClientAuthenticationType.java
@@ -159,7 +159,8 @@ public class ValidateClientAuthenticationType extends AbstractAuthenticationActi
if (registeredMethod != null && !registeredMethod.equals(used)) {
log.warn("{} Client '{}' registered {} but attempted {}", getLogPrefix(),
- clientAuthentication.getClientID(), registeredMethod, used);
+ clientAuthentication != null ? clientAuthentication.getClientID() : "<Unknown>",
+ registeredMethod, used);
ActionSupport.buildEvent(profileRequestContext, EventIds.ACCESS_DENIED);
} else if (!enabledMethods.contains(used)) {
log.warn("{} Requested method {} not enabled in profile configuration", getLogPrefix(), used);
diff --git a/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/flow/TokenFlowTest.java b/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/flow/TokenFlowTest.java
index fd4ec46f..adc7467c 100644
--- a/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/flow/TokenFlowTest.java
+++ b/idp-oidc-extension-impl/src/test/java/net/shibboleth/idp/plugin/oidc/op/profile/flow/TokenFlowTest.java
@@ -67,6 +67,9 @@ public class TokenFlowTest extends AbstractOidcClientAuthenticationFlowTest {
String clientIdPkcePlain = "mockClientIdPKCEPlain";
String clientIdPkcePlainUnforced = "mockClientIdPKCEPlainUnforced";
String clientIdPkceS256 = "mockClientIdPKCES256";
+ String clientIdPkcePlainPublic = "mockPublicClientIdPKCEPlain";
+ String clientIdPkcePlainUnforcedPublic = "mockPublicClientIdPKCEPlainUnforced";
+ String clientIdPkceS256Public = "mockPublicClientIdPKCES256";
String codeVerifier = "9234567812345678123456781234567812345678123456781234567812345678";
Scope scope = Scope.parse("openid profile email offline_access");
@@ -141,11 +144,21 @@ public class TokenFlowTest extends AbstractOidcClientAuthenticationFlowTest {
protected void initializeGrantAndRequest(final String clientId, final Map<String, String> requestParameters)
throws IOException {
+ initializeGrantAndRequest(clientId, requestParameters, true);
+ }
+
+ protected void initializeGrantAndRequest(final String clientId, final Map<String, String> requestParameters,
+ final boolean doBasicAuth)
+ throws IOException {
setHttpFormRequest("POST", requestParameters);
- storeMetadata(storageService, clientId, clientSecret, scope);
- setBasicAuth(clientId, clientSecret);
+ if (doBasicAuth) {
+ setBasicAuth(clientId, clientSecret);
+ storeMetadata(storageService, clientId, clientSecret, scope);
+ } else {
+ storeMetadata(storageService, clientId, clientSecret, scope, null, ClientAuthenticationMethod.NONE);
+ }
}
-
+
@Test
public void testValidGrant() throws Exception {
initializeGrantAndRequest(clientId, createRequestParameters(redirectUri, "authorization_code",
@@ -157,7 +170,6 @@ public class TokenFlowTest extends AbstractOidcClientAuthenticationFlowTest {
Assert.assertNotNull(response.getTokens().getRefreshToken());
Assert.assertNotNull(response.getOIDCTokens().getIDToken());
}
-
@Test
public void testValidGrantRefreshTokensDisabledInSSOProfile() throws Exception {
final String clientId = "mockClientIdNoRefreshTokensInSSOProfile";
@@ -341,6 +353,19 @@ public class TokenFlowTest extends AbstractOidcClientAuthenticationFlowTest {
Assert.assertNotNull(response.getTokens().getAccessToken());
}
+ @Test
+ public void testValidGrantValidPlainPKCE_publicClient() throws Exception {
+ initializeGrantAndRequest(clientIdPkcePlainPublic, createRequestParameters(redirectUri, "authorization_code",
+ buildAuthorizationCode(clientIdPkcePlainPublic, plainVerifier()), clientIdPkcePlainPublic, null, null,
+ codeVerifier), false);
+ storeConsent(storageService, "jdoe", clientIdPkcePlainPublic, "mail");
+ final FlowExecutionResult result = flowExecutor.launchExecution(FLOW_ID, null, externalContext);
+ final OIDCTokenResponse response = parseSuccessResponse(result, OIDCTokenResponse.class);
+ Assert.assertNotNull(response.getTokens().getAccessToken());
+ Assert.assertNotNull(response.getTokens().getRefreshToken());
+ Assert.assertNotNull(response.getOIDCTokens().getIDToken());
+ }
+
@Test
public void testValidGrantValidUnforcedPlainPKCE() throws Exception {
initializeGrantAndRequest(clientIdPkcePlainUnforced, createRequestParameters(redirectUri, "authorization_code",
@@ -351,6 +376,16 @@ public class TokenFlowTest extends AbstractOidcClientAuthenticationFlowTest {
Assert.assertNotNull(response.getTokens().getAccessToken());
}
+ @Test
+ public void testValidGrantValidUnforcedPlainPKCE_publicClient() throws Exception {
+ initializeGrantAndRequest(clientIdPkcePlainUnforcedPublic, createRequestParameters(redirectUri, "authorization_code",
+ buildAuthorizationCode(clientIdPkcePlainUnforcedPublic, plainVerifier()), clientIdPkcePlainUnforcedPublic, null,
+ null, codeVerifier), false);
+ final FlowExecutionResult result = flowExecutor.launchExecution(FLOW_ID, null, externalContext);
+ final OIDCTokenResponse response = parseSuccessResponse(result, OIDCTokenResponse.class);
+ Assert.assertNotNull(response.getTokens().getAccessToken());
+ }
+
@Test
public void testValidGrantValidRequestMissingS256PKCE() throws Exception {
initializeGrantAndRequest(clientIdPkceS256, createRequestParameters(redirectUri, "authorization_code",
@@ -390,6 +425,16 @@ public class TokenFlowTest extends AbstractOidcClientAuthenticationFlowTest {
Assert.assertNotNull(response.getTokens().getAccessToken());
}
+ @Test
+ public void testValidGrantValidS256PKCE_publicClient() throws Exception {
+ initializeGrantAndRequest(clientIdPkceS256Public, createRequestParameters(redirectUri, "authorization_code",
+ buildAuthorizationCode(clientIdPkceS256Public, s256Verifier()), clientIdPkceS256Public, null, "S256",
+ codeVerifier), false);
+ final FlowExecutionResult result = flowExecutor.launchExecution(FLOW_ID, null, externalContext);
+ final OIDCTokenResponse response = parseSuccessResponse(result, OIDCTokenResponse.class);
+ Assert.assertNotNull(response.getTokens().getAccessToken());
+ }
+
@Test
public void testValidGrantValidUnforcedS256PKCE() throws Exception {
initializeGrantAndRequest(clientId, createRequestParameters(redirectUri, "authorization_code",
@@ -398,7 +443,7 @@ public class TokenFlowTest extends AbstractOidcClientAuthenticationFlowTest {
final OIDCTokenResponse response = parseSuccessResponse(result, OIDCTokenResponse.class);
Assert.assertNotNull(response.getTokens().getAccessToken());
}
-
+
@Test
public void testInvalidSecretJWT() throws Exception {
final ClientSecretJWT clientAuth = buildSecretJwtAuth(clientSecret + "invalid");
diff --git a/idp-oidc-extension-impl/src/test/resources/conf/relying-party.xml b/idp-oidc-extension-impl/src/test/resources/conf/relying-party.xml
index 0729340c..5339dd47 100644
--- a/idp-oidc-extension-impl/src/test/resources/conf/relying-party.xml
+++ b/idp-oidc-extension-impl/src/test/resources/conf/relying-party.xml
@@ -96,6 +96,30 @@
</list>
</property>
</bean>
+ <bean parent="RelyingPartyByName" c:relyingPartyIds="mockPublicClientIdPKCEPlainUnforced">
+ <property name="profileConfigurations">
+ <list>
+ <bean parent="OIDC.SSO.MDDriven" p:forcePKCE="false" p:allowPKCEPlain="true" p:tokenEndpointAuthMethods="none"/>
+ <bean parent="OAUTH2.Token.MDDriven" p:forcePKCE="false" p:allowPKCEPlain="true" p:tokenEndpointAuthMethods="none"/>
+ </list>
+ </property>
+ </bean>
+ <bean parent="RelyingPartyByName" c:relyingPartyIds="mockPublicClientIdPKCEPlain">
+ <property name="profileConfigurations">
+ <list>
+ <bean parent="OIDC.SSO.MDDriven" p:forcePKCE="true" p:allowPKCEPlain="true" p:tokenEndpointAuthMethods="none"/>
+ <bean parent="OAUTH2.Token.MDDriven" p:forcePKCE="true" p:allowPKCEPlain="true" p:tokenEndpointAuthMethods="none"/>
+ </list>
+ </property>
+ </bean>
+ <bean parent="RelyingPartyByName" c:relyingPartyIds="mockPublicClientIdPKCES256">
+ <property name="profileConfigurations">
+ <list>
+ <bean parent="OIDC.SSO.MDDriven" p:forcePKCE="true" p:allowPKCEPlain="false" p:tokenEndpointAuthMethods="none"/>
+ <bean parent="OAUTH2.Token.MDDriven" p:forcePKCE="true" p:allowPKCEPlain="false" p:tokenEndpointAuthMethods="none"/>
+ </list>
+ </property>
+ </bean>
<bean parent="RelyingPartyByName"
c:relyingPartyIds="#{{'https://rp.example.org', 'https://resource.example.org'}}">
<property name="profileConfigurations">
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.
More information about the commits
mailing list