Skip to content

Commit

Permalink
Fix NPE when OIDC token must be verified with the chain with OIDC ser…
Browse files Browse the repository at this point in the history
…ver returning no JWKs
  • Loading branch information
sberyozkin committed Feb 21, 2024
1 parent 85c8a77 commit c995914
Show file tree
Hide file tree
Showing 12 changed files with 133 additions and 27 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ public static class CertificateChain {
* A parameter to specify the password of the truststore file if it is configured with {@link #trustStoreFile}.
*/
@ConfigItem
public Optional<String> trustStorePassword;
public Optional<String> trustStorePassword = Optional.empty();

/**
* A parameter to specify the alias of the truststore certificate.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
package io.quarkus.oidc.runtime;

import java.security.Key;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.function.Function;

import jakarta.enterprise.event.Observes;
Expand All @@ -24,6 +26,9 @@

public class DynamicVerificationKeyResolver {
private static final Logger LOG = Logger.getLogger(DynamicVerificationKeyResolver.class);
private static final Set<String> KEY_HEADERS = Set.of(HeaderParameterNames.KEY_ID,
HeaderParameterNames.X509_CERTIFICATE_SHA256_THUMBPRINT,
HeaderParameterNames.X509_CERTIFICATE_THUMBPRINT);

private final OidcProviderClient client;
private final MemoryCache<Key> cache;
Expand All @@ -46,6 +51,12 @@ public Uni<VerificationKeyResolver> resolve(TokenCredential tokenCred) {
if (key != null) {
return Uni.createFrom().item(new SingleKeyVerificationKeyResolver(key));
}
if (chainResolverFallback != null && headers.containsKey(HeaderParameterNames.X509_CERTIFICATE_CHAIN)
&& Collections.disjoint(KEY_HEADERS, headers.fieldNames())) {
// If none of the key headers is available which can be used to resolve JWK then do
// not try to get another JWK set but delegate to the chain resolver fallback if it is available
return getChainResolver();
}

return client.getJsonWebKeySet(new OidcRequestContextProperties(
Map.of(OidcRequestContextProperties.TOKEN, tokenCred.getToken(),
Expand Down Expand Up @@ -105,9 +116,7 @@ public Uni<? extends VerificationKeyResolver> apply(JsonWebKeySet jwks) {
}

if (newKey == null && chainResolverFallback != null) {
LOG.debug("JWK is not available, neither 'kid' nor 'x5t#S256' nor 'x5t' token headers are set,"
+ " falling back to the certificate chain resolver");
return Uni.createFrom().item(chainResolverFallback);
return getChainResolver();
}

if (newKey == null) {
Expand All @@ -121,6 +130,12 @@ public Uni<? extends VerificationKeyResolver> apply(JsonWebKeySet jwks) {
});
}

private Uni<VerificationKeyResolver> getChainResolver() {
LOG.debug("JWK is not available, neither 'kid' nor 'x5t#S256' nor 'x5t' token headers are set,"
+ " falling back to the certificate chain resolver");
return Uni.createFrom().item(chainResolverFallback);
}

private static Key getKeyWithId(JsonWebKeySet jwks, String kid) {
if (kid != null) {
return jwks.getKeyWithId(kid);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import io.quarkus.oidc.AccessTokenCredential;
import io.quarkus.oidc.IdTokenCredential;
import io.quarkus.oidc.OIDCException;
import io.quarkus.oidc.OidcTenantConfig;
import io.quarkus.oidc.OidcTenantConfig.Roles.Source;
import io.quarkus.oidc.TokenIntrospection;
Expand Down Expand Up @@ -98,14 +99,16 @@ protected Map<String, Object> getRequestData(TokenAuthenticationRequest request)

private Uni<SecurityIdentity> authenticate(TokenAuthenticationRequest request, Map<String, Object> requestData,
TenantConfigContext resolvedContext) {
if (resolvedContext.oidcConfig.publicKey.isPresent()) {
LOG.debug("Performing token verification with a configured public key");
return validateTokenWithoutOidcServer(request, resolvedContext);
if (resolvedContext.oidcConfig.authServerUrl.isPresent()) {
return validateAllTokensWithOidcServer(requestData, request, resolvedContext);
} else if (resolvedContext.oidcConfig.getCertificateChain().trustStoreFile.isPresent()) {
LOG.debug("Performing token verification with a public key inlined in the certificate chain");
return validateTokenWithoutOidcServer(request, resolvedContext);
} else if (resolvedContext.oidcConfig.publicKey.isPresent()) {
LOG.debug("Performing token verification with a configured public key");
return validateTokenWithoutOidcServer(request, resolvedContext);
} else {
return validateAllTokensWithOidcServer(requestData, request, resolvedContext);
return Uni.createFrom().failure(new OIDCException("Unexpected authentication request"));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,15 @@ public OidcProvider(OidcProviderClient client, OidcTenantConfig oidcConfig, Json
this.client = client;
this.oidcConfig = oidcConfig;
this.tokenCustomizer = tokenCustomizer;
this.asymmetricKeyResolver = jwks == null ? null
: new JsonWebKeyResolver(jwks, oidcConfig.token.forcedJwkRefreshInterval, oidcConfig.certificateChain);
if (jwks != null) {
this.asymmetricKeyResolver = new JsonWebKeyResolver(jwks, oidcConfig.token.forcedJwkRefreshInterval,
oidcConfig.certificateChain);
} else if (oidcConfig != null && oidcConfig.certificateChain.trustStoreFile.isPresent()) {
this.asymmetricKeyResolver = new CertChainPublicKeyResolver(oidcConfig.certificateChain);
} else {
this.asymmetricKeyResolver = null;
}

if (client != null && oidcConfig != null && !oidcConfig.jwks.resolveEarly) {
this.keyResolverProvider = new DynamicVerificationKeyResolver(client, oidcConfig);
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,22 +10,22 @@

import io.quarkus.oidc.OIDCException;
import io.quarkus.oidc.TokenCustomizer;
import io.quarkus.oidc.common.runtime.OidcConstants;
import io.quarkus.oidc.runtime.OidcUtils;

@Named("azure-access-token-customizer")
@ApplicationScoped
public class AzureAccessTokenCustomizer implements TokenCustomizer {
private static final String NONCE = "nonce";

@Override
public JsonObject customizeHeaders(JsonObject headers) {
try {
String nonce = headers.getString(NONCE);
String nonce = headers.containsKey(OidcConstants.NONCE) ? headers.getString(OidcConstants.NONCE) : null;
if (nonce != null) {
byte[] nonceSha256 = OidcUtils.getSha256Digest(nonce.getBytes(StandardCharsets.UTF_8));
byte[] newNonceBytes = Base64.getUrlEncoder().withoutPadding().encode(nonceSha256);
return Json.createObjectBuilder(headers)
.add(NONCE, new String(newNonceBytes, StandardCharsets.UTF_8)).build();
.add(OidcConstants.NONCE, new String(newNonceBytes, StandardCharsets.UTF_8)).build();
}
return null;
} catch (Exception ex) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ public String adminRequiredAlgorithm() {
@Authenticated
@Produces(MediaType.APPLICATION_JSON)
public String adminAzure() {
return "Issuer:" + ((JsonWebToken) identity.getPrincipal()).getIssuer();
return "Name:" + identity.getPrincipal().getName() + ",Issuer:" + ((JsonWebToken) identity.getPrincipal()).getIssuer();
}

@Path("bearer-no-introspection")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
import jakarta.ws.rs.GET;
import jakarta.ws.rs.Path;

import org.eclipse.microprofile.jwt.JsonWebToken;

import io.quarkus.oidc.TokenIntrospection;
import io.quarkus.security.Authenticated;
import io.quarkus.security.identity.SecurityIdentity;
Expand All @@ -20,6 +22,10 @@ public class CodeFlowTokenIntrospectionResource {

@GET
public String access() {
return identity.getPrincipal().getName() + ":" + tokenIntrospection.getUsername();
if (identity.getPrincipal() instanceof JsonWebToken) {
return identity.getPrincipal().getName();
} else {
return identity.getPrincipal().getName() + ":" + tokenIntrospection.getUsername();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,15 @@
@Authenticated
public class CodeFlowUserInfoResource {

// UserInfo stubs are available for code flow JWT and binary access tokens and bearer JWT tokens
@Inject
UserInfo userInfo;

// Custom test augmentor changes Principal to use UserInfo which has a preferred `alice` name
@Inject
SecurityIdentity identity;

// current access token
@Inject
JsonWebToken accessToken;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ quarkus.oidc.bearer-user-info-github-service.client-id=quarkus-web-app
quarkus.oidc.bearer-user-info-github-service.credentials.secret=AyM1SysPpbyDfgZld3umj1qzKObwVMkoqQ-EstJQLr_T-1qS0gZH75aKtMN3Yj0iPS4hcgUuTwjAzZr1Z9CAow

quarkus.oidc.code-flow-user-info-github-cached-in-idtoken.provider=github
quarkus.oidc.code-flow-user-info-github-cached-in-idtoken.application-type=hybrid
quarkus.oidc.code-flow-user-info-github-cached-in-idtoken.auth-server-url=${keycloak.url}/realms/quarkus/
quarkus.oidc.code-flow-user-info-github-cached-in-idtoken.authorization-path=/
quarkus.oidc.code-flow-user-info-github-cached-in-idtoken.token-path=access_token_refreshed
Expand All @@ -101,6 +102,9 @@ quarkus.oidc.code-flow-user-info-github-cached-in-idtoken.authentication.verify-
quarkus.oidc.code-flow-user-info-github-cached-in-idtoken.client-id=quarkus-web-app
quarkus.oidc.code-flow-user-info-github-cached-in-idtoken.credentials.client-secret.value=AyM1SysPpbyDfgZld3umj1qzKObwVMkoqQ-EstJQLr_T-1qS0gZH75aKtMN3Yj0iPS4hcgUuTwjAzZr1Z9CAow
quarkus.oidc.code-flow-user-info-github-cached-in-idtoken.credentials.client-secret.method=query
quarkus.oidc.code-flow-user-info-github-cached-in-idtoken.certificate-chain.trust-store-file=truststore.p12
quarkus.oidc.code-flow-user-info-github-cached-in-idtoken.certificate-chain.trust-store-password=storepassword
quarkus.oidc.code-flow-user-info-github-cached-in-idtoken.certificate-chain.trust-store-cert-alias=fullchain


quarkus.oidc.code-flow-token-introspection.provider=github
Expand Down Expand Up @@ -150,6 +154,9 @@ quarkus.oidc.bearer-azure.jwks-path=${keycloak.url}/azure/jwk
quarkus.oidc.bearer-azure.jwks.resolve-early=false
quarkus.oidc.bearer-azure.token.lifespan-grace=2147483647
quarkus.oidc.bearer-azure.token.customizer-name=azure-access-token-customizer
quarkus.oidc.bearer-azure.certificate-chain.trust-store-file=truststore.p12
quarkus.oidc.bearer-azure.certificate-chain.trust-store-password=storepassword
quarkus.oidc.bearer-azure.certificate-chain.trust-store-cert-alias=fullchain

quarkus.oidc.bearer-role-claim-path.auth-server-url=${keycloak.url}/realms/quarkus/
quarkus.oidc.bearer-role-claim-path.client-id=quarkus-app
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,20 @@ public void testAccessResourceAzure() throws Exception {
wireMockServer.stubFor(WireMock.get("/auth/azure/jwk")
.withHeader("Authorization", matching("Access token: " + azureToken))
.willReturn(WireMock.aResponse().withBody(azureJwk)));
RestAssured.given().auth().oauth2(azureToken)
// RestAssured.given().auth().oauth2(azureToken)
// .when().get("/api/admin/bearer-azure")
// .then()
// .statusCode(200)
// .body(Matchers.equalTo(
// "Name:jondoe@quarkusoidctest.onmicrosoft.com,Issuer:https://sts.windows.net/e7861267-92c5-4a03-bdb2-2d3e491e7831/"));

String accessTokenWithCert = TestUtils.createTokenWithInlinedCertChain("alice-certificate");

RestAssured.given().auth().oauth2(accessTokenWithCert)
.when().get("/api/admin/bearer-azure")
.then()
.statusCode(200)
.body(Matchers.equalTo("Issuer:https://sts.windows.net/e7861267-92c5-4a03-bdb2-2d3e491e7831/"));
.body(Matchers.equalTo("Name:alice-certificate,Issuer:https://server.example.com"));
}

private String readFile(String filePath) throws Exception {
Expand Down Expand Up @@ -292,7 +301,7 @@ public void testAccessAdminResourceWithKidOrChain() throws Exception {
List.of(subjectCert, intermediateCert, rootCert),
subjectPrivateKey);

assertX5cOnlyIsPresent(token);
TestUtils.assertX5cOnlyIsPresent(token);

RestAssured.given().auth().oauth2(token)
.when().get("/api/admin/bearer-kid-or-chain")
Expand All @@ -311,7 +320,7 @@ public void testAccessAdminResourceWithKidOrChain() throws Exception {
List.of(intermediateCert, subjectCert, rootCert),
subjectPrivateKey);

assertX5cOnlyIsPresent(token);
TestUtils.assertX5cOnlyIsPresent(token);

RestAssured.given().auth().oauth2(token)
.when().get("/api/admin/bearer-kid-or-chain")
Expand Down Expand Up @@ -364,14 +373,6 @@ private void assertKidOnlyIsPresent(String token, String kid) {
assertFalse(headers.containsKey("x5t#S256"));
}

private void assertX5cOnlyIsPresent(String token) {
JsonObject headers = OidcUtils.decodeJwtHeaders(token);
assertTrue(headers.containsKey("x5c"));
assertFalse(headers.containsKey("kid"));
assertFalse(headers.containsKey("x5t"));
assertFalse(headers.containsKey("x5t#S256"));
}

@Test
public void testAccessAdminResourceWithCustomRolePathForbidden() {
RestAssured.given().auth().oauth2(getAccessTokenWithCustomRolePath("admin", Set.of("admin")))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import static com.github.tomakehurst.wiremock.client.WireMock.matching;
import static com.github.tomakehurst.wiremock.client.WireMock.notContaining;
import static com.github.tomakehurst.wiremock.client.WireMock.urlPathMatching;
import static org.hamcrest.Matchers.equalTo;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
Expand All @@ -23,6 +24,7 @@
import javax.crypto.SecretKey;
import javax.crypto.spec.SecretKeySpec;

import org.hamcrest.Matchers;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;

Expand Down Expand Up @@ -271,6 +273,16 @@ public void testCodeFlowUserInfoCachedInIdToken() throws Exception {

webClient.getCookieManager().clearCookies();
}

// Now send a bearer access token with the inline chain
String bearerAccessToken = TestUtils.createTokenWithInlinedCertChain("alice-certificate");

RestAssured.given().auth().oauth2(bearerAccessToken)
.when().get("/code-flow-user-info-github-cached-in-idtoken")
.then()
.statusCode(200)
.body(Matchers.equalTo("alice:alice:alice-certificate, cache size: 0"));

clearCache();
}

Expand All @@ -296,6 +308,7 @@ public void testCodeFlowTokenIntrospection() throws Exception {

webClient.getCookieManager().clearCookies();
}

clearCache();
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
package io.quarkus.it.keycloak;

import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

import java.security.PrivateKey;
import java.security.cert.X509Certificate;
import java.util.List;

import io.quarkus.oidc.runtime.OidcUtils;
import io.smallrye.jwt.build.Jwt;
import io.smallrye.jwt.util.KeyUtils;
import io.smallrye.jwt.util.ResourceUtils;
import io.vertx.core.json.JsonObject;

public class TestUtils {

public static String createTokenWithInlinedCertChain(String preferredUserName) throws Exception {
X509Certificate rootCert = KeyUtils.getCertificate(ResourceUtils.readResource("/ca.cert.pem"));
X509Certificate intermediateCert = KeyUtils.getCertificate(ResourceUtils.readResource("/intermediate.cert.pem"));
X509Certificate subjectCert = KeyUtils.getCertificate(ResourceUtils.readResource("/www.quarkustest.com.cert.pem"));
PrivateKey subjectPrivateKey = KeyUtils.readPrivateKey("/www.quarkustest.com.key.pem");

String bearerAccessToken = getAccessTokenWithCertChain(
List.of(subjectCert, intermediateCert, rootCert),
subjectPrivateKey,
preferredUserName);

assertX5cOnlyIsPresent(bearerAccessToken);
return bearerAccessToken;
}

public static String getAccessTokenWithCertChain(List<X509Certificate> chain,
PrivateKey privateKey, String preferredUserName) throws Exception {
return Jwt.preferredUserName(preferredUserName)
.groups("admin")
.issuer("https://server.example.com")
.audience("https://service.example.com")
.jws().chain(chain)
.sign(privateKey);
}

public static void assertX5cOnlyIsPresent(String token) {
JsonObject headers = OidcUtils.decodeJwtHeaders(token);
assertTrue(headers.containsKey("x5c"));
assertFalse(headers.containsKey("kid"));
assertFalse(headers.containsKey("x5t"));
assertFalse(headers.containsKey("x5t#S256"));
}

}

0 comments on commit c995914

Please sign in to comment.