Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support OIDC authorization code flow nonce #35039

Merged
merged 1 commit into from
Jul 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -469,15 +469,15 @@
While PKCE is of primary importance to public OpenID Connect clients, such as SPA scripts running in a browser, it can also provide an extra level of protection to Quarkus OIDC `web-app` applications.
With PKCE, Quarkus OIDC `web-app` applications are confidential OpenID Connect clients capable of securely storing the client secret and using it to exchange the code for the tokens.

You can enable `PKCE` for your OIDC `web-app` endpoint with a `quarkus.oidc.authentication.pkce-required` property and a 32-character secret, as shown in the following example:
You can enable `PKCE` for your OIDC `web-app` endpoint with a `quarkus.oidc.authentication.pkce-required` property and a 32-character secret which is required to encrypt the PKCE code verifier in the state cookie, as shown in the following example:

Check warning on line 472 in docs/src/main/asciidoc/security-oidc-code-flow-authentication.adoc

View workflow job for this annotation

GitHub Actions / Linting with Vale

[vale] reported by reviewdog 🐶 [Quarkus.TermsSuggestions] Depending on the context, consider using ', which (non restrictive clause preceded by a comma)' or 'that (restrictive clause without a comma)' rather than 'which'. Raw Output: {"message": "[Quarkus.TermsSuggestions] Depending on the context, consider using ', which (non restrictive clause preceded by a comma)' or 'that (restrictive clause without a comma)' rather than 'which'.", "location": {"path": "docs/src/main/asciidoc/security-oidc-code-flow-authentication.adoc", "range": {"start": {"line": 472, "column": 141}}}, "severity": "INFO"}

[source, properties]
----
quarkus.oidc.authentication.pkce-required=true
quarkus.oidc.authentication.pkce-secret=eUk1p7UB3nFiXZGUXi0uph1Y9p34YhBU
quarkus.oidc.authentication.state-secret=eUk1p7UB3nFiXZGUXi0uph1Y9p34YhBU
----

If you already have a 32-characters client secret then you do not need to set the `quarkus.oidc.authentication.pkce-secret` property unless you prefer to use a different secret key.
If you already have a 32-characters client secret then you do not need to set the `quarkus.oidc.authentication.pkce-secret` property unless you prefer to use a different secret key. This secret will be auto-generated if it is not configured and if the fallback to the client secret is not possible in case of the client secret being less than 16 characters long.

Check warning on line 480 in docs/src/main/asciidoc/security-oidc-code-flow-authentication.adoc

View workflow job for this annotation

GitHub Actions / Linting with Vale

[vale] reported by reviewdog 🐶 [Quarkus.Fluff] Depending on the context, consider using 'Rewrite the sentence, or use 'must', instead of' rather than 'need to'. Raw Output: {"message": "[Quarkus.Fluff] Depending on the context, consider using 'Rewrite the sentence, or use 'must', instead of' rather than 'need to'.", "location": {"path": "docs/src/main/asciidoc/security-oidc-code-flow-authentication.adoc", "range": {"start": {"line": 480, "column": 67}}}, "severity": "INFO"}

Check warning on line 480 in docs/src/main/asciidoc/security-oidc-code-flow-authentication.adoc

View workflow job for this annotation

GitHub Actions / Linting with Vale

[vale] reported by reviewdog 🐶 [Quarkus.SentenceLength] Try to keep sentences to an average of 32 words or fewer. Raw Output: {"message": "[Quarkus.SentenceLength] Try to keep sentences to an average of 32 words or fewer.", "location": {"path": "docs/src/main/asciidoc/security-oidc-code-flow-authentication.adoc", "range": {"start": {"line": 480, "column": 184}}}, "severity": "INFO"}

The secret key is required for encrypting a randomly generated `PKCE` `code_verifier` while the user is being redirected with the `code_challenge` query parameter to an OIDC provider to authenticate.
The `code_verifier` is decrypted when the user is redirected back to Quarkus and sent to the token endpoint alongside the `code`, client secret, and other parameters to complete the code exchange.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -386,8 +386,8 @@
Quarkus has to encrypt the current PKCE code verifier in a state cookie while the authorization code flow with Twitter is in progress and it will
generate a secure random secret key for encrypting it.

You can provide your own secret key for encrypting the PKCE code verifier if you prefer with the `quarkus.oidc.authentication.pkce-secret` property but
You can provide your own secret key for encrypting the PKCE code verifier if you prefer with the `quarkus.oidc.authentication.state-secret` property but

Check warning on line 389 in docs/src/main/asciidoc/security-openid-connect-providers.adoc

View workflow job for this annotation

GitHub Actions / Linting with Vale

[vale] reported by reviewdog 🐶 [Quarkus.SentenceLength] Try to keep sentences to an average of 32 words or fewer. Raw Output: {"message": "[Quarkus.SentenceLength] Try to keep sentences to an average of 32 words or fewer.", "location": {"path": "docs/src/main/asciidoc/security-openid-connect-providers.adoc", "range": {"start": {"line": 389, "column": 1}}}, "severity": "INFO"}
note that this secret should be 32 characters long, and an error will be reported if it is less than 16 characters long.

Check warning on line 390 in docs/src/main/asciidoc/security-openid-connect-providers.adoc

View workflow job for this annotation

GitHub Actions / Linting with Vale

[vale] reported by reviewdog 🐶 [Quarkus.Fluff] Depending on the context, consider using 'Be concise: rewrite the sentence to not use' rather than 'note that'. Raw Output: {"message": "[Quarkus.Fluff] Depending on the context, consider using 'Be concise: rewrite the sentence to not use' rather than 'note that'.", "location": {"path": "docs/src/main/asciidoc/security-openid-connect-providers.adoc", "range": {"start": {"line": 390, "column": 1}}}, "severity": "INFO"}
====

[[spotify]]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,4 +73,5 @@ public final class OidcConstants {
public static final String ID_TOKEN_SID_CLAIM = "sid";

public static final String OPENID_SCOPE = "openid";
public static final String NONCE = "nonce";
}
Original file line number Diff line number Diff line change
Expand Up @@ -181,14 +181,14 @@ public void run() throws Throwable {
String line = null;
while ((line = reader.readLine()) != null) {
if (line.contains(
"Secret key for encrypting PKCE code verifier is missing, auto-generating it")) {
"Secret key for encrypting state cookie is missing, auto-generating it")) {
checkPassed.set(true);
}
}
}
}
});
assertTrue(checkPassed.get(), "Can not confirm Secret key for encrypting PKCE code verifier has been generated");
assertTrue(checkPassed.get(), "Can not confirm Secret key for encrypting state cookie has been generated");
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -781,6 +781,15 @@ public enum ResponseMode {
@ConfigItem
public Optional<List<String>> scopes = Optional.empty();

/**
* Require that ID token includes `nonce` claim which must match `nonce` authentication request query parameter.
* Enabling this property can help mitigate replay attacks.
* Do not enable this property if your OpenId Connect provider does not support setting `nonce` in ID token
* or if you work with OAuth2 provider such as `GitHub` which does not issue ID tokens.
*/
@ConfigItem(defaultValue = "false")
public boolean nonceRequired = false;

/**
* Add the 'openid' scope automatically to the list of scopes. This is required for OpenId Connect providers
* but will not work for OAuth2 providers such as Twitter OAuth2 which does not accept that scope and throws an error.
Expand Down Expand Up @@ -945,19 +954,30 @@ public enum ResponseMode {
/**
* Secret which will be used to encrypt a Proof Key for Code Exchange (PKCE) code verifier in the code flow state.
* This secret should be at least 32 characters long.
*
* @deprecated Use {@link #stateSecret} property instead.
*/
@ConfigItem
@Deprecated(forRemoval = true)
public Optional<String> pkceSecret = Optional.empty();

/**
* Secret which will be used to encrypt Proof Key for Code Exchange (PKCE) code verifier and/or nonce in the code flow
* state.
* This secret should be at least 32 characters long.
* <p/>
* If this secret is not set, the client secret configured with
* either `quarkus.oidc.credentials.secret` or `quarkus.oidc.credentials.client-secret.value` will be checked.
* Finally, `quarkus.oidc.credentials.jwt.secret` which can be used for `client_jwt_secret` authentication will be
* checked. Client secret will not be used as a PKCE code verifier encryption secret if it is less than 32 characters
* checked. Client secret will not be used as a state encryption secret if it is less than 32 characters
* long.
* </p>
* The secret will be auto-generated if it remains uninitialized after checking all of these properties.
* <p/>
* Error will be reported if the secret length is less than 16 characters.
*/
@ConfigItem
public Optional<String> pkceSecret = Optional.empty();
public Optional<String> stateSecret = Optional.empty();

public Optional<Duration> getInternalIdTokenLifespan() {
return internalIdTokenLifespan;
Expand All @@ -975,10 +995,12 @@ public void setPkceRequired(boolean pkceRequired) {
this.pkceRequired = Optional.of(pkceRequired);
}

@Deprecated(forRemoval = true)
public Optional<String> getPkceSecret() {
return pkceSecret;
}

@Deprecated(forRemoval = true)
public void setPkceSecret(String pkceSecret) {
this.pkceSecret = Optional.of(pkceSecret);
}
Expand Down Expand Up @@ -1158,6 +1180,22 @@ public boolean isAllowMultipleCodeFlows() {
public void setAllowMultipleCodeFlows(boolean allowMultipleCodeFlows) {
this.allowMultipleCodeFlows = allowMultipleCodeFlows;
}

public boolean isNonceRequired() {
return nonceRequired;
}

public void setNonceRequired(boolean nonceRequired) {
this.nonceRequired = nonceRequired;
}

public Optional<String> getStateSecret() {
return stateSecret;
}

public void setStateSecret(Optional<String> stateSecret) {
this.stateSecret = stateSecret;
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -623,9 +623,12 @@ && isRedirectFromProvider(context, configContext)) {
PkceStateBean pkceStateBean = createPkceStateBean(configContext);

// state
String nonce = configContext.oidcConfig.authentication.nonceRequired ? UUID.randomUUID().toString()
: null;

codeFlowParams.append(AMP).append(OidcConstants.CODE_FLOW_STATE).append(EQ)
.append(generateCodeFlowState(context, configContext, redirectPath, requestQueryParams,
pkceStateBean != null ? pkceStateBean.getCodeVerifier() : null));
(pkceStateBean != null ? pkceStateBean.getCodeVerifier() : null), nonce));

if (pkceStateBean != null) {
codeFlowParams
Expand All @@ -636,6 +639,10 @@ && isRedirectFromProvider(context, configContext)) {
.append(OidcConstants.PKCE_CODE_CHALLENGE_S256);
}

if (nonce != null) {
codeFlowParams.append(AMP).append(OidcConstants.NONCE).append(EQ).append(nonce);
}

// extra redirect parameters, see https://openid.net/specs/openid-connect-core-1_0.html#AuthRequests
addExtraParamsToUri(codeFlowParams, configContext.oidcConfig.authentication.getExtraParams());

Expand Down Expand Up @@ -739,6 +746,9 @@ public Uni<SecurityIdentity> apply(final AuthorizationCodeTokens tokens, final T
internalIdToken = true;
}
} else {
if (!verifyNonce(configContext.oidcConfig, stateBean, tokens.getIdToken())) {
return Uni.createFrom().failure(new AuthenticationCompletionException());
}
internalIdToken = false;
}

Expand Down Expand Up @@ -814,6 +824,21 @@ public Throwable apply(Throwable tInner) {
});
}

private static boolean verifyNonce(OidcTenantConfig oidcConfig, CodeAuthenticationStateBean stateBean, String idToken) {
if (oidcConfig.authentication.nonceRequired) {
if (stateBean != null && stateBean.getNonce() != null) {
JsonObject idTokenClaims = OidcUtils.decodeJwtContent(idToken);
if (stateBean.getNonce().equals(idTokenClaims.getString(OidcConstants.NONCE))) {
return true;
}
}
LOG.errorf("ID token 'nonce' does not match the authentication request 'nonce' value");
return false;
} else {
return true;
}
}

private static Object errorMessage(Throwable t) {
return t.getCause() != null ? t.getCause().getMessage() : t.getMessage();
}
Expand All @@ -822,21 +847,24 @@ private CodeAuthenticationStateBean getCodeAuthenticationBean(String[] parsedSta
TenantConfigContext configContext) {
if (parsedStateCookieValue.length == 2) {
CodeAuthenticationStateBean bean = new CodeAuthenticationStateBean();
if (!configContext.oidcConfig.authentication.pkceRequired.orElse(false)) {
Authentication authentication = configContext.oidcConfig.authentication;
boolean pkceRequired = authentication.pkceRequired.orElse(false);
if (!pkceRequired && !authentication.nonceRequired) {
bean.setRestorePath(parsedStateCookieValue[1]);
return bean;
}

JsonObject json = null;
try {
json = OidcUtils.decryptJson(parsedStateCookieValue[1], configContext.getPkceSecretKey());
json = OidcUtils.decryptJson(parsedStateCookieValue[1], configContext.getStateEncryptionKey());
} catch (Exception ex) {
LOG.errorf("State cookie value can not be decrypted for the %s tenant",
configContext.oidcConfig.tenantId.get());
throw new AuthenticationCompletionException(ex);
}
bean.setRestorePath(json.getString(STATE_COOKIE_RESTORE_PATH));
bean.setCodeVerifier(json.getString(OidcConstants.PKCE_CODE_VERIFIER));
bean.setNonce(json.getString(OidcConstants.NONCE));
return bean;
}
return null;
Expand Down Expand Up @@ -943,12 +971,13 @@ private String getRedirectPath(OidcTenantConfig oidcConfig, RoutingContext conte
}

private String generateCodeFlowState(RoutingContext context, TenantConfigContext configContext,
String redirectPath, MultiMap requestQueryWithoutForwardedParams, String pkceCodeVerifier) {
String redirectPath, MultiMap requestQueryWithoutForwardedParams, String pkceCodeVerifier, String nonce) {
String uuid = UUID.randomUUID().toString();
String cookieValue = uuid;

boolean restorePath = isRestorePath(configContext.oidcConfig.getAuthentication());
if (restorePath || pkceCodeVerifier != null) {
Authentication authentication = configContext.oidcConfig.getAuthentication();
boolean restorePath = isRestorePath(authentication);
if (restorePath || pkceCodeVerifier != null || nonce != null) {
CodeAuthenticationStateBean extraStateValue = new CodeAuthenticationStateBean();
if (restorePath) {
String requestQuery = context.request().query();
Expand Down Expand Up @@ -978,6 +1007,7 @@ private String generateCodeFlowState(RoutingContext context, TenantConfigContext
}
}
extraStateValue.setCodeVerifier(pkceCodeVerifier);
extraStateValue.setNonce(nonce);
if (!extraStateValue.isEmpty()) {
cookieValue += (COOKIE_DELIM + encodeExtraStateValue(extraStateValue, configContext));
}
Expand All @@ -997,14 +1027,19 @@ private boolean isRestorePath(Authentication auth) {
}

private String encodeExtraStateValue(CodeAuthenticationStateBean extraStateValue, TenantConfigContext configContext) {
if (extraStateValue.getCodeVerifier() != null) {
if (extraStateValue.getCodeVerifier() != null || extraStateValue.getNonce() != null) {
JsonObject json = new JsonObject();
json.put(OidcConstants.PKCE_CODE_VERIFIER, extraStateValue.getCodeVerifier());
if (extraStateValue.getCodeVerifier() != null) {
json.put(OidcConstants.PKCE_CODE_VERIFIER, extraStateValue.getCodeVerifier());
}
if (extraStateValue.getNonce() != null) {
json.put(OidcConstants.NONCE, extraStateValue.getNonce());
}
if (extraStateValue.getRestorePath() != null) {
json.put(STATE_COOKIE_RESTORE_PATH, extraStateValue.getRestorePath());
}
try {
return OidcUtils.encryptJson(json, configContext.getPkceSecretKey());
return OidcUtils.encryptJson(json, configContext.getStateEncryptionKey());
} catch (Exception ex) {
LOG.errorf("State containing the code verifier can not be encrypted: %s", ex.getMessage());
throw new AuthenticationCompletionException(ex);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ public class CodeAuthenticationStateBean {

private String codeVerifier;

private String nonce;

public String getRestorePath() {
return restorePath;
}
Expand All @@ -22,8 +24,16 @@ public void setCodeVerifier(String codeVerifier) {
this.codeVerifier = codeVerifier;
}

public String getNonce() {
return nonce;
}

public void setNonce(String nonce) {
this.nonce = nonce;
}

public boolean isEmpty() {
return this.restorePath == null && this.codeVerifier == null;
return this.restorePath == null && this.codeVerifier == null && nonce == null;
}

}