diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentials.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentials.java index 1619c784e..3597d1a51 100644 --- a/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentials.java +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentials.java @@ -23,6 +23,7 @@ import java.io.IOException; import java.io.Serial; import java.io.Serializable; +import java.net.URI; import java.security.GeneralSecurityException; import java.time.Duration; import java.time.Instant; @@ -32,6 +33,7 @@ import java.util.Optional; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.TimeUnit; +import java.util.function.Consumer; import java.util.logging.Level; import java.util.logging.Logger; import java.util.stream.Collectors; @@ -96,6 +98,44 @@ public class GitHubAppCredentials extends BaseStandardCredentials implements Sta public static boolean ALLOW_UNSAFE_REPOSITORY_INFERENCE = Boolean.getBoolean(GitHubAppCredentials.class.getName() + ".ALLOW_UNSAFE_REPOSITORY_INFERENCE"); + /** + * On Windows agents, clears the Windows Credential Manager cache entry for the GitHub host + * before each Git credential use. This prevents Git from serving an expired GitHub App + * installation token that was cached by a previous build, and ensures Git falls through to + * {@code GIT_ASKPASS} to receive the fresh token Jenkins is about to provide. + * + *

Disable only if the {@code cmdkey} invocations cause problems in your environment. + * Non-final so it can be adjusted from the Jenkins script console if needed. + */ + @SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "Non-final for script console override") + public static boolean CLEAR_WINDOWS_CREDENTIAL_MANAGER_CACHE = Boolean.parseBoolean(System.getProperty( + GitHubAppCredentials.class.getName() + ".CLEAR_WINDOWS_CREDENTIAL_MANAGER_CACHE", "true")); + + /** + * Replaceable executor for Windows Credential Manager key deletion. + * The string parameter is the credential key (e.g. {@code git:https://github.com}). + * Non-final to allow replacement in tests. + */ + @Restricted(NoExternalUse.class) + static Consumer windowsCredentialCleaner = key -> { + try { + Process process = new ProcessBuilder("cmdkey", "/delete:" + key) + .redirectErrorStream(true) + .start(); + boolean finished = process.waitFor(5, TimeUnit.SECONDS); + if (!finished) { + process.destroyForcibly(); + LOGGER.log(Level.WARNING, "Timed out clearing Windows Credential Manager entry: {0}", key); + } else { + LOGGER.log(Level.FINE, "Cleared Windows Credential Manager entry: {0} (exit: {1})", new Object[] { + key, process.exitValue() + }); + } + } catch (IOException | InterruptedException e) { + LOGGER.log(Level.WARNING, "Failed to clear Windows Credential Manager entry: " + key, e); + } + }; + @NonNull private final String appID; @@ -613,6 +653,45 @@ Object readResolve() { return this; } + /** + * Derives the Git repository host from a GitHub API URI. + * + *

Returns {@code github.com} for the standard endpoint ({@code https://api.github.com}), or + * the host component of the URI for GitHub Enterprise Server instances. + * + * @param apiUri the GitHub API URI (e.g. {@code https://api.github.com}) + * @return the corresponding Git repository host (e.g. {@code github.com}) + */ + static String deriveGitHostFromApiUri(String apiUri) { + try { + String host = new URI(apiUri).getHost(); + if (host == null) { + return "github.com"; + } + return "api.github.com".equals(host) ? "github.com" : host; + } catch (Exception e) { + LOGGER.log(Level.FINE, "Could not parse API URI to derive git host: " + apiUri, e); + return "github.com"; + } + } + + /** + * Clears cached GitHub credentials from the Windows Credential Manager for the host + * corresponding to {@code apiUri}. + * + *

Removes both the modern ({@code git:https://host}) and legacy + * ({@code LegacyGenericCredential:https://host}) key formats used by the Windows git credential + * helpers, so that Git falls through to {@code GIT_ASKPASS} and uses the fresh token Jenkins is + * about to provide. + * + * @param apiUri the GitHub API URI used to derive the Git repository host + */ + static void clearWindowsCredentialManagerCache(String apiUri) { + String httpsUrl = "https://" + deriveGitHostFromApiUri(apiUri); + windowsCredentialCleaner.accept("git:" + httpsUrl); + windowsCredentialCleaner.accept("LegacyGenericCredential:" + httpsUrl); + } + /** * Ensures that the credentials state as serialized via Remoting to an agent calls back to the * controller. Benefits: @@ -637,6 +716,8 @@ private static final class DelegatingGitHubAppCredentials extends BaseStandardCr implements StandardUsernamePasswordCredentials { private final String appID; + /** The GitHub API URI, used to derive the git host for Windows Credential Manager clearing. */ + private final String apiUri; /** * An encrypted form of all data needed to refresh the token. Used to prevent {@link GetToken} * from being abused by compromised build agents. @@ -651,6 +732,7 @@ private static final class DelegatingGitHubAppCredentials extends BaseStandardCr super(onMaster.getScope(), onMaster.getId(), onMaster.getDescription()); JenkinsJVM.checkJenkinsJVM(); appID = onMaster.getAppID(); + apiUri = onMaster.actualApiUri(); JSONObject j = new JSONObject(); j.put("appID", appID); j.put("privateKey", onMaster.getPrivateKey().getPlainText()); @@ -707,6 +789,7 @@ public String getUsername() { public Secret getPassword() { JenkinsJVM.checkNotJenkinsJVM(); try { + final Secret token; synchronized (this) { try { if (cachedToken == null || cachedToken.isStale()) { @@ -742,10 +825,22 @@ public Secret getPassword() { } } LOGGER.log(Level.FINEST, "Returned GitHub App Installation Token for app ID {0} on agent", appID); + token = cachedToken.getToken(); + } - return cachedToken.getToken(); + // On Windows agents, evict the cached credential from Windows Credential Manager + // so that Git does not serve the previously-cached (possibly expired) token to the + // next Git operation instead of calling GIT_ASKPASS for the fresh token we just + // obtained above. This is the Windows equivalent of the token-refresh fix on + // Linux; see also CLEAR_WINDOWS_CREDENTIAL_MANAGER_CACHE. + if (CLEAR_WINDOWS_CREDENTIAL_MANAGER_CACHE + && System.getProperty("os.name", "") + .toLowerCase(Locale.ROOT) + .startsWith("windows")) { + clearWindowsCredentialManagerCache(apiUri); } + return token; } catch (IOException | InterruptedException x) { throw new RuntimeException(x); } diff --git a/src/test/java/org/jenkinsci/plugins/github_branch_source/GithubAppCredentialsTest.java b/src/test/java/org/jenkinsci/plugins/github_branch_source/GithubAppCredentialsTest.java index 7d9a8fb36..8fa9ce8bd 100644 --- a/src/test/java/org/jenkinsci/plugins/github_branch_source/GithubAppCredentialsTest.java +++ b/src/test/java/org/jenkinsci/plugins/github_branch_source/GithubAppCredentialsTest.java @@ -353,6 +353,7 @@ public void testProviderRefresh() throws Exception { @Test public void testAgentRefresh() throws Exception { final long notStaleSeconds = GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS; + final boolean originalClearWindowsCache = GitHubAppCredentials.CLEAR_WINDOWS_CREDENTIAL_MANAGER_CACHE; try { appCredentials.setApiUri(githubApi.baseUrl()); @@ -360,6 +361,11 @@ public void testAgentRefresh() throws Exception { // Must set this to a large enough number to avoid flaky test GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS = 10; + // Disable Windows Credential Manager cache clearing so its log messages do not + // interfere with the strict log-sequence assertions below. The clearing behaviour + // is tested separately in GithubAppCredentialsWindowsAgentTest. + GitHubAppCredentials.CLEAR_WINDOWS_CREDENTIAL_MANAGER_CACHE = false; + // Ensure we are working from sufficiently clean cache state Thread.sleep(Duration.ofSeconds(GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS + 2) .toMillis()); @@ -463,6 +469,7 @@ public void testAgentRefresh() throws Exception { 0, RequestPatternBuilder.newRequestPattern(RequestMethod.GET, urlPathEqualTo("/rate_limit"))); } finally { GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS = notStaleSeconds; + GitHubAppCredentials.CLEAR_WINDOWS_CREDENTIAL_MANAGER_CACHE = originalClearWindowsCache; logRecorder.doClear(); } } @@ -509,11 +516,12 @@ private List getOutputLines() { result.addAll(agentLogs); } - // sort the logs into chronological order - // then just format the message. + // sort the logs into chronological order, then just format the message. + // Agent JVM has its own static field copy (defaults true), so filter its wincred messages. return result.stream() .sorted(Comparator.comparingLong(LogRecord::getMillis)) .map(formatter::formatMessage) + .filter(msg -> !msg.startsWith("Cleared Windows Credential Manager")) .collect(Collectors.toList()); } diff --git a/src/test/java/org/jenkinsci/plugins/github_branch_source/GithubAppCredentialsWindowsAgentTest.java b/src/test/java/org/jenkinsci/plugins/github_branch_source/GithubAppCredentialsWindowsAgentTest.java new file mode 100644 index 000000000..0c26dab56 --- /dev/null +++ b/src/test/java/org/jenkinsci/plugins/github_branch_source/GithubAppCredentialsWindowsAgentTest.java @@ -0,0 +1,120 @@ +package org.jenkinsci.plugins.github_branch_source; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.empty; +import static org.hamcrest.Matchers.is; + +import java.util.ArrayList; +import java.util.List; +import java.util.function.Consumer; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +/** + * Unit tests for the Windows Credential Manager cache-clearing logic added to + * {@link GitHubAppCredentials}. + * + *

These tests exercise the static helper methods + * ({@link GitHubAppCredentials#deriveGitHostFromApiUri} and + * {@link GitHubAppCredentials#clearWindowsCredentialManagerCache}) and verify that the right + * credential keys are evicted. They run on any OS because the {@link + * GitHubAppCredentials#windowsCredentialCleaner} field is replaced with a recording stub. + */ +public class GithubAppCredentialsWindowsAgentTest { + + private Consumer originalCleaner; + private boolean originalClearFlag; + private List deletedKeys; + + @Before + public void setUp() { + originalCleaner = GitHubAppCredentials.windowsCredentialCleaner; + originalClearFlag = GitHubAppCredentials.CLEAR_WINDOWS_CREDENTIAL_MANAGER_CACHE; + deletedKeys = new ArrayList<>(); + GitHubAppCredentials.windowsCredentialCleaner = deletedKeys::add; + GitHubAppCredentials.CLEAR_WINDOWS_CREDENTIAL_MANAGER_CACHE = true; + } + + @After + public void tearDown() { + GitHubAppCredentials.windowsCredentialCleaner = originalCleaner; + GitHubAppCredentials.CLEAR_WINDOWS_CREDENTIAL_MANAGER_CACHE = originalClearFlag; + } + + // ------------------------------------------------------------------------- + // deriveGitHostFromApiUri + // ------------------------------------------------------------------------- + + @Test + public void deriveGitHost_standardGitHub() { + assertThat(GitHubAppCredentials.deriveGitHostFromApiUri("https://api.github.com"), is("github.com")); + } + + @Test + public void deriveGitHost_githubEnterprise() { + assertThat( + GitHubAppCredentials.deriveGitHostFromApiUri("https://ghe.example.com/api/v3"), is("ghe.example.com")); + } + + @Test + public void deriveGitHost_enterpriseWithPort() { + assertThat( + GitHubAppCredentials.deriveGitHostFromApiUri("https://github.corp.example.com:8443/api/v3"), + is("github.corp.example.com")); + } + + @Test + public void deriveGitHost_malformedUri_fallsBackToGithubCom() { + assertThat(GitHubAppCredentials.deriveGitHostFromApiUri("not a uri ://???"), is("github.com")); + } + + @Test + public void deriveGitHost_emptyString_fallsBackToGithubCom() { + assertThat(GitHubAppCredentials.deriveGitHostFromApiUri(""), is("github.com")); + } + + // ------------------------------------------------------------------------- + // clearWindowsCredentialManagerCache – key format + // ------------------------------------------------------------------------- + + @Test + public void clearCache_standardGitHub_deletesExpectedKeys() { + GitHubAppCredentials.clearWindowsCredentialManagerCache("https://api.github.com"); + + assertThat(deletedKeys, contains("git:https://github.com", "LegacyGenericCredential:https://github.com")); + } + + @Test + public void clearCache_githubEnterprise_deletesExpectedKeys() { + GitHubAppCredentials.clearWindowsCredentialManagerCache("https://ghe.example.com/api/v3"); + + assertThat( + deletedKeys, + contains("git:https://ghe.example.com", "LegacyGenericCredential:https://ghe.example.com")); + } + + @Test + public void clearCache_alwaysDeletesBothKeyFormats() { + GitHubAppCredentials.clearWindowsCredentialManagerCache("https://api.github.com"); + + assertThat("Both wincred and GCM key formats must be cleared", deletedKeys.size(), is(2)); + } + + // ------------------------------------------------------------------------- + // CLEAR_WINDOWS_CREDENTIAL_MANAGER_CACHE flag + // ------------------------------------------------------------------------- + + @Test + public void clearCache_flagDisabled_doesNotDeleteKeys() { + GitHubAppCredentials.CLEAR_WINDOWS_CREDENTIAL_MANAGER_CACHE = false; + + // Simulate what getPassword() does when the flag is false + if (GitHubAppCredentials.CLEAR_WINDOWS_CREDENTIAL_MANAGER_CACHE) { + GitHubAppCredentials.clearWindowsCredentialManagerCache("https://api.github.com"); + } + + assertThat(deletedKeys, is(empty())); + } +}