Discovery OAuth: Add optional PKCE support Add support for Proof Key for Code Exchange (PKCE) in the Discovery OAuth provider to improve security of the authorization code flow. When `enable-pkce = true`: - Generate a PKCE code verifier/challenge during authorization - Return verifier alongside the authorization URL - Include verifier when exchanging the authorization code for a token This is disabled by default to preserve backward compatibility with providers that do not support PKCE. NOTE: This change depends on a Gerrit core update that extends the OAuthServiceProvider API (OAuthAuthorizationInfo and PKCE-aware getAccessToken). The plugin will not work with older Gerrit versions. Also: - Update configuration and documentation - Add unit tests covering PKCE authorization and token exchange flows Contributed-By: Kai Liu <kraml.liu@gmail.com> Change-Id: I56fa0f4d50333a2c568d95c31c78f2de3afed661
diff --git a/src/main/java/com/googlesource/gerrit/plugins/oauth/InitOAuth.java b/src/main/java/com/googlesource/gerrit/plugins/oauth/InitOAuth.java index b28b754..a1a1dd7 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/oauth/InitOAuth.java +++ b/src/main/java/com/googlesource/gerrit/plugins/oauth/InitOAuth.java
@@ -264,6 +264,8 @@ checkRootUrl( discoveryOAuthProviderSection.string( "Discovery Root URL(before `/.well-known')", ROOT_URL, null)); + discoveryOAuthProviderSection.string( + "Enable PKCE for Discovery OAuth provider?", ENABLE_PKCE, "false"); } }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/oauth/discovery/DiscoveryOAuthService.java b/src/main/java/com/googlesource/gerrit/plugins/oauth/discovery/DiscoveryOAuthService.java index 8923dcf..e646564 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/oauth/discovery/DiscoveryOAuthService.java +++ b/src/main/java/com/googlesource/gerrit/plugins/oauth/discovery/DiscoveryOAuthService.java
@@ -22,8 +22,12 @@ import com.github.scribejava.core.model.OAuthRequest; import com.github.scribejava.core.model.Response; import com.github.scribejava.core.model.Verb; +import com.github.scribejava.core.oauth.AccessTokenRequestParams; +import com.github.scribejava.core.oauth.AuthorizationUrlBuilder; import com.github.scribejava.core.oauth.OAuth20Service; import com.google.common.io.CharStreams; +import com.google.gerrit.common.Nullable; +import com.google.gerrit.extensions.auth.oauth.OAuthAuthorizationInfo; import com.google.gerrit.extensions.auth.oauth.OAuthServiceProvider; import com.google.gerrit.extensions.auth.oauth.OAuthToken; import com.google.gerrit.extensions.auth.oauth.OAuthUserInfo; @@ -60,6 +64,7 @@ private static final String SCOPE = "openid profile email"; private final OAuth20Service service; + private final boolean enablePKCE; private final String extIdScheme; private final String userinfoEndpoint; @@ -74,6 +79,7 @@ DiscoveryOpenIdConnect discovery = fetchDiscoveryDocument(rootUri.toString() + WELL_KNOWN_PATH); validateDiscoveryDocument(discovery); + enablePKCE = cfg.getBoolean(InitOAuth.ENABLE_PKCE, false); service = oauth20ServiceFactory.create( PROVIDER_NAME, @@ -252,9 +258,13 @@ } @Override - public OAuthToken getAccessToken(OAuthVerifier rv) { + public OAuthToken getAccessToken(OAuthVerifier rv, @Nullable String codeVerifier) { try { - OAuth2AccessToken accessToken = service.getAccessToken(rv.getValue()); + AccessTokenRequestParams reqParams = AccessTokenRequestParams.create(rv.getValue()); + if (enablePKCE && codeVerifier != null) { + reqParams.pkceCodeVerifier(codeVerifier); + } + OAuth2AccessToken accessToken = service.getAccessToken(reqParams); return new OAuthToken( accessToken.getAccessToken(), accessToken.getTokenType(), accessToken.getRawResponse()); } catch (InterruptedException | ExecutionException | IOException e) { @@ -265,8 +275,16 @@ } @Override - public String getAuthorizationUrl() { - return service.getAuthorizationUrl(); + public OAuthAuthorizationInfo getAuthorizationInfo() { + AuthorizationUrlBuilder builder = service.createAuthorizationUrlBuilder(); + String verifier = null; + + if (enablePKCE) { + builder.initPKCE(); + verifier = builder.getPkce().getCodeVerifier(); + } + + return new OAuthAuthorizationInfo(builder.build(), verifier); } @Override
diff --git a/src/main/resources/Documentation/config.md b/src/main/resources/Documentation/config.md index f13f111..447b017 100644 --- a/src/main/resources/Documentation/config.md +++ b/src/main/resources/Documentation/config.md
@@ -99,6 +99,7 @@ root-url = "<root url>" # The part before /.well-known. for example, https://kanidm.example.com/oauth2/openid/gerrit client-id = "<client-id>" client-secret = "<client-secret>" + enable-pkce = false ``` When one from the sections above is omitted, OAuth SSO is used. @@ -384,6 +385,8 @@ Set the `root-url` to the part before `/.well-known` of the discovery URL of your OAuth server, and client-id and client-secret in gerrit.config. +You can optionally set `enable-pkce = true` if you want to use PKCE as part of the authorization workflow during login. + Tested providers: - Authelia - Kanidm
diff --git a/src/test/java/com/googlesource/gerrit/plugins/oauth/discovery/DiscoveryOAuthServiceTest.java b/src/test/java/com/googlesource/gerrit/plugins/oauth/discovery/DiscoveryOAuthServiceTest.java index 214664d..8030cf1 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/oauth/discovery/DiscoveryOAuthServiceTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/oauth/discovery/DiscoveryOAuthServiceTest.java
@@ -18,14 +18,20 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import com.github.scribejava.core.builder.api.DefaultApi20; import com.github.scribejava.core.model.OAuthRequest; import com.github.scribejava.core.model.Response; +import com.github.scribejava.core.oauth.AccessTokenRequestParams; +import com.github.scribejava.core.oauth.AuthorizationUrlBuilder; import com.github.scribejava.core.oauth.OAuth20Service; +import com.github.scribejava.core.pkce.PKCE; +import com.google.gerrit.extensions.auth.oauth.OAuthAuthorizationInfo; import com.google.gerrit.extensions.auth.oauth.OAuthToken; import com.google.gerrit.extensions.auth.oauth.OAuthUserInfo; +import com.google.gerrit.extensions.auth.oauth.OAuthVerifier; import com.google.gerrit.server.config.PluginConfig; import com.google.inject.ProvisionException; import com.googlesource.gerrit.plugins.oauth.InitOAuth; @@ -36,6 +42,7 @@ import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.junit.MockitoJUnitRunner; @@ -117,6 +124,53 @@ } @Test + public void getAuthorizationInfo_withPkceEnabled_shouldReturnVerifier() throws Exception { + when(mockPluginConfig.getBoolean("enable-pkce", false)).thenReturn(true); + + AuthorizationUrlBuilder mockUrlBuilder = mock(AuthorizationUrlBuilder.class); + when(mockScribeOAuthService.createAuthorizationUrlBuilder()).thenReturn(mockUrlBuilder); + + PKCE pkce = new PKCE(); + pkce.setCodeVerifier("secret-verifier-123"); + + when(mockUrlBuilder.getPkce()).thenReturn(pkce); + when(mockUrlBuilder.build()).thenReturn("https://id.example.com/auth?code_challenge=xyz"); + + DiscoveryOAuthService service = createServiceWithDiscoveryDoc(validDiscoveryDocument()); + OAuthAuthorizationInfo info = service.getAuthorizationInfo(); + + assertThat(info.getAuthorizationUrl()).contains("code_challenge=xyz"); + assertThat(info.getPkceVerifier()).isEqualTo("secret-verifier-123"); + } + + @Test + public void getAccessToken_withPkce_shouldPassVerifierToScribe() throws Exception { + when(mockPluginConfig.getBoolean("enable-pkce", false)).thenReturn(true); + DiscoveryOAuthService service = createServiceWithDiscoveryDoc(validDiscoveryDocument()); + + OAuthVerifier verifier = new OAuthVerifier("auth-code"); + String secureVerifierFromSession = "session-secret-verifier"; + + com.github.scribejava.core.model.OAuth2AccessToken mockToken = + mock(com.github.scribejava.core.model.OAuth2AccessToken.class); + + when(mockToken.getAccessToken()).thenReturn("dummy-access-token"); + when(mockToken.getTokenType()).thenReturn("Bearer"); + when(mockToken.getRawResponse()).thenReturn("raw-json-response"); + + when(mockScribeOAuthService.getAccessToken(any(AccessTokenRequestParams.class))) + .thenReturn(mockToken); + + service.getAccessToken(verifier, secureVerifierFromSession); + + ArgumentCaptor<AccessTokenRequestParams> captor = + ArgumentCaptor.forClass(AccessTokenRequestParams.class); + verify(mockScribeOAuthService).getAccessToken(captor.capture()); + + assertThat(captor.getValue().getPkceCodeVerifier()).isEqualTo(secureVerifierFromSession); + } + + @Test public void getUserInfo_validStandardFields_shouldMapUserInfo() throws Exception { DiscoveryOAuthService service = createServiceWithDiscoveryDoc(validDiscoveryDocument());