Skip to content
Permalink
Browse files Browse the repository at this point in the history
reinforce security on OIDC
  • Loading branch information
leleuj committed Dec 6, 2021
1 parent d3c5f61 commit 22b82ff
Show file tree
Hide file tree
Showing 6 changed files with 119 additions and 12 deletions.
6 changes: 6 additions & 0 deletions documentation/docs/clients/openid-connect.md
Expand Up @@ -145,3 +145,9 @@ The additional param `TokenExpirationAdvance` allows to set the time in seconds,
```java
config.setTokenExpirationAdvance(10);
```

Since version 5.2 and to reinforce security, the `none` alogithm for ID tokens (meaning no signature validation) must be explictly accepted by using:

```java
config.setAllowUnsignedIdTokens(true);
```
1 change: 1 addition & 0 deletions documentation/docs/release-notes.md
Expand Up @@ -6,6 +6,7 @@ title: Release notes:
**v5.2**:

- The JEE core components are now in the `pac4j-jee` dependency (and no longer in the `pac4j-core` dependency)
- Reinforce security on the OIDC protocol support: the `none` algorithm must be explicitly accepted on client side (`allowUnsignedIdTokens`)

**v5.1.5**:

Expand Down
Expand Up @@ -140,6 +140,8 @@ public class OidcConfiguration extends BaseClientConfiguration {

private TokenValidator tokenValidator;

private boolean allowUnsignedIdTokens;

@Override
protected void internalInit() {
// checks
Expand Down Expand Up @@ -486,6 +488,14 @@ public void setMappedClaims(Map<String, String> mappedClaims) {
this.mappedClaims = mappedClaims;
}

public boolean isAllowUnsignedIdTokens() {
return allowUnsignedIdTokens;
}

public void setAllowUnsignedIdTokens(final boolean allowUnsignedIdTokens) {
this.allowUnsignedIdTokens = allowUnsignedIdTokens;
}

@Override
public String toString() {
return toNiceString(this.getClass(), "clientId", clientId, "secret", "[protected]",
Expand All @@ -495,6 +505,6 @@ public String toString() {
"connectTimeout", connectTimeout, "readTimeout", readTimeout, "resourceRetriever", resourceRetriever,
"responseType", responseType, "responseMode", responseMode, "logoutUrl", logoutUrl,
"withState", withState, "stateGenerator", stateGenerator, "logoutHandler", logoutHandler,
"tokenValidator", tokenValidator, "mappedClaims", mappedClaims);
"tokenValidator", tokenValidator, "mappedClaims", mappedClaims, "allowUnsignedIdTokens", allowUnsignedIdTokens);
}
}
Expand Up @@ -6,8 +6,6 @@
import java.io.IOException;
import java.util.Map;

import javax.naming.AuthenticationException;

import org.pac4j.core.context.WebContext;
import org.pac4j.core.context.session.SessionStore;
import org.pac4j.core.credentials.Credentials;
Expand Down Expand Up @@ -99,9 +97,8 @@ private JWTClaimsSet fetchOidcProfile(BearerAccessToken accessToken) {
httpResponse.getContent());
final var userInfoResponse = UserInfoResponse.parse(httpResponse);
if (userInfoResponse instanceof UserInfoErrorResponse) {
logger.error("Bad User Info response, error={}",
((UserInfoErrorResponse) userInfoResponse).getErrorObject().toJSONObject());
throw new AuthenticationException();
throw new TechnicalException("Bad User Info response, error="
+ ((UserInfoErrorResponse) userInfoResponse).getErrorObject().toJSONObject());
} else {
final var userInfoSuccessResponse = (UserInfoSuccessResponse) userInfoResponse;
final JWTClaimsSet userInfoClaimsSet;
Expand All @@ -112,7 +109,7 @@ private JWTClaimsSet fetchOidcProfile(BearerAccessToken accessToken) {
}
return userInfoClaimsSet;
}
} catch (IOException | ParseException | java.text.ParseException | AuthenticationException e) {
} catch (IOException | ParseException | java.text.ParseException e) {
throw new TechnicalException(e);
}
}
Expand Down
Expand Up @@ -51,13 +51,13 @@ public TokenValidator(final OidcConfiguration configuration) {
final var _clientID = new ClientID(configuration.getClientId());

for (var jwsAlgorithm : jwsAlgorithms) {
if ("none".equals(jwsAlgorithm.getName())) {
jwsAlgorithm = null;
}

// build validator
final IDTokenValidator idTokenValidator;
if (jwsAlgorithm == null) {
if ("none".equals(jwsAlgorithm.getName())) {
if (!configuration.isAllowUnsignedIdTokens()) {
throw new TechnicalException("Unsigned ID tokens are not allowed");
}
logger.warn("Allowing unsigned ID tokens");
idTokenValidator = new IDTokenValidator(configuration.findProviderMetadata().getIssuer(), _clientID);
} else if (CommonHelper.isNotBlank(configuration.getSecret()) && (JWSAlgorithm.HS256.equals(jwsAlgorithm) ||
JWSAlgorithm.HS384.equals(jwsAlgorithm) || JWSAlgorithm.HS512.equals(jwsAlgorithm))) {
Expand Down Expand Up @@ -112,4 +112,9 @@ public IDTokenClaimsSet validate(final JWT idToken, final Nonce expectedNonce)
throw new TechnicalException("Unable to validate the ID token");
}
}

// for tests
List<IDTokenValidator> getIdTokenValidators() {
return idTokenValidators;
}
}
@@ -0,0 +1,88 @@
package org.pac4j.oidc.profile.creator;

import com.nimbusds.jose.JWSAlgorithm;
import com.nimbusds.oauth2.sdk.id.Issuer;
import com.nimbusds.openid.connect.sdk.op.OIDCProviderMetadata;
import com.nimbusds.openid.connect.sdk.validators.IDTokenValidator;
import org.junit.Before;
import org.junit.Test;
import org.pac4j.core.exception.TechnicalException;
import org.pac4j.core.util.TestsConstants;
import org.pac4j.core.util.TestsHelper;
import org.pac4j.oidc.config.OidcConfiguration;

import java.net.URI;
import java.net.URISyntaxException;
import java.util.ArrayList;
import java.util.List;

import static org.mockito.Mockito.*;
import static org.junit.Assert.*;

/**
* Tests {@link TokenValidator}.
*
* @author Jerome LELEU
* @since 5.2.0
*/
public final class TokenValidatorTests implements TestsConstants {

private OidcConfiguration configuration;

private List<JWSAlgorithm> algorithms;

@Before
public void setUp() throws URISyntaxException {
configuration = mock(OidcConfiguration.class);
final OIDCProviderMetadata metadata = mock(OIDCProviderMetadata.class);
when(metadata.getIssuer()).thenReturn(new Issuer(PAC4J_URL));
when(metadata.getJWKSetURI()).thenReturn(new URI(PAC4J_BASE_URL));
when(configuration.findProviderMetadata()).thenReturn(metadata);
when(configuration.getClientId()).thenReturn(ID);
when(configuration.getSecret()).thenReturn(SECRET);
algorithms = new ArrayList<>();
when(metadata.getIDTokenJWSAlgs()).thenReturn(algorithms);
}

@Test
public void testNoAlgoDefinedAtProvider() {
TestsHelper.expectException(() -> new TokenValidator(configuration), TechnicalException.class,
"There must at least one JWS algorithm supported on the OpenID Connect provider side");
}

@Test
public void testNoneAlgoNotAllowed() {
algorithms.add(JWSAlgorithm.parse("none"));
TestsHelper.expectException(() -> new TokenValidator(configuration), TechnicalException.class,
"Unsigned ID tokens are not allowed");
}

@Test
public void testNoneAlgoAllowed() {
algorithms.add(JWSAlgorithm.parse("none"));
when(configuration.isAllowUnsignedIdTokens()).thenReturn(true);
final TokenValidator validator = new TokenValidator(configuration);
final List<IDTokenValidator> validators = validator.getIdTokenValidators();
assertEquals(1, validators.size());
assertTrue(validators.get(0) instanceof IDTokenValidator);
}

@Test
public void testTwoAlgorithms() {
algorithms.add(JWSAlgorithm.HS256);
algorithms.add(JWSAlgorithm.RS256);
final TokenValidator validator = new TokenValidator(configuration);
final List<IDTokenValidator> validators = validator.getIdTokenValidators();
assertEquals(2, validators.size());
}

@Test
public void testTwoAlgorithmsOnePreferred() {
algorithms.add(JWSAlgorithm.HS256);
algorithms.add(JWSAlgorithm.RS256);
when(configuration.getPreferredJwsAlgorithm()).thenReturn(JWSAlgorithm.HS256);
final TokenValidator validator = new TokenValidator(configuration);
final List<IDTokenValidator> validators = validator.getIdTokenValidators();
assertEquals(1, validators.size());
}
}

0 comments on commit 22b82ff

Please sign in to comment.