[JENKINS-62249] Delegate GitHub App token generation from agents and improve caching #326
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,14 +15,20 @@ | |
| import hudson.util.Secret; | ||
| import java.io.IOException; | ||
| import java.io.Serializable; | ||
| import java.time.Duration; | ||
| import java.time.Instant; | ||
| import java.util.List; | ||
| import java.util.logging.Level; | ||
| import java.util.logging.Logger; | ||
|
|
||
| import jenkins.security.SlaveToMasterCallable; | ||
| import jenkins.util.JenkinsJVM; | ||
| import org.kohsuke.accmod.Restricted; | ||
| import org.kohsuke.accmod.restrictions.NoExternalUse; | ||
| import org.kohsuke.github.GHApp; | ||
| import org.kohsuke.github.GHAppInstallation; | ||
| import org.kohsuke.github.GHAppInstallationToken; | ||
| import org.kohsuke.github.GitHub; | ||
| import org.kohsuke.stapler.DataBoundConstructor; | ||
| import org.kohsuke.stapler.DataBoundSetter; | ||
| import org.kohsuke.stapler.QueryParameter; | ||
|
|
@@ -34,6 +40,8 @@ | |
| @SuppressFBWarnings(value = "SE_NO_SERIALVERSIONID", justification = "XStream") | ||
| public class GitHubAppCredentials extends BaseStandardCredentials implements StandardUsernamePasswordCredentials { | ||
|
|
||
| private static final Logger LOGGER = Logger.getLogger(GitHubAppCredentials.class.getName()); | ||
|
|
||
| private static final String ERROR_AUTHENTICATING_GITHUB_APP = "Couldn't authenticate with GitHub app ID %s"; | ||
| private static final String NOT_INSTALLED = ", has it been installed to your GitHub organisation / user?"; | ||
|
|
||
|
|
@@ -49,8 +57,7 @@ public class GitHubAppCredentials extends BaseStandardCredentials implements Sta | |
|
|
||
| private String owner; | ||
|
|
||
| private transient AppInstallationToken cachedToken; | ||
|
|
||
| @DataBoundConstructor | ||
| @SuppressWarnings("unused") // by stapler | ||
|
|
@@ -104,7 +111,7 @@ public void setOwner(String owner) { | |
| } | ||
|
|
||
| @SuppressWarnings("deprecation") // preview features are required for GitHub app integration, GitHub api adds deprecated to all preview methods | ||
| static AppInstallationToken generateAppInstallationToken(String appId, String appPrivateKey, String apiUrl, String owner) { | ||
| try { | ||
| String jwtToken = createJWT(appId, appPrivateKey); | ||
| GitHub gitHubApp = Connector | ||
|
|
@@ -132,11 +139,35 @@ static String generateAppInstallationToken(String appId, String appPrivateKey, S | |
| .createToken(appInstallation.getPermissions()) | ||
| .create(); | ||
|
|
||
| long expiration = getExpirationSeconds(appInstallationToken); | ||
| LOGGER.log(Level.FINEST, "Token raw expiration epoch seconds: {0}", expiration); | ||
|
|
||
| AppInstallationToken token = new AppInstallationToken(appInstallationToken.getToken(), expiration); | ||
| LOGGER.log(Level.FINE, "Generated App Installation Token for app ID {0}", appId); | ||
|
|
||
| return token; | ||
| } catch (IOException e) { | ||
| LOGGER.log(Level.WARNING, "Failed to retrieve GitHub App installation token for app ID " + appId, e); | ||
bitwiseman marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| throw new IllegalArgumentException(String.format(ERROR_AUTHENTICATING_GITHUB_APP, appId), e); | ||
| } | ||
| } | ||
|
|
||
| private static long getExpirationSeconds(GHAppInstallationToken appInstallationToken) { | ||
| try { | ||
| return appInstallationToken.getExpiresAt() | ||
| .toInstant() | ||
| .getEpochSecond(); | ||
| } catch (Exception e) { | ||
|
There was a problem hiding this comment. I was confused about this earlier too. There was a problem hiding this comment. Implementation has changed over time, but changing the It is also possible for GitHub to hand us values that don't parse. Regardless of why it fails, we want to recover and continue. |
||
| // if we fail to calculate the expiration, guess at a reasonable value. | ||
| LOGGER.log(Level.WARNING, | ||
| "Unable to get GitHub App installation token expiration", | ||
| e); | ||
| return Instant.now().getEpochSecond() + AppInstallationToken.MAXIMUM_AGE_SECONDS; | ||
| } | ||
| } | ||
|
|
||
| @NonNull String actualApiUri() { | ||
| return Util.fixEmpty(apiUri) == null ? "https://api.github.com" : apiUri; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -145,19 +176,15 @@ static String generateAppInstallationToken(String appId, String appPrivateKey, S | |
| @NonNull | ||
| @Override | ||
| public Secret getPassword() { | ||
| String appInstallationToken; | ||
| synchronized (this) { | ||
|
There was a problem hiding this comment. If multiple requests for a password are made concurrently, we want to just get one refreshed token. |
||
| if (cachedToken == null || cachedToken.isStale()) { | ||
| LOGGER.log(Level.FINE, "Generating App Installation Token for app ID {0}", appID); | ||
| cachedToken = generateAppInstallationToken(appID, privateKey.getPlainText(), actualApiUri(), owner); | ||
| } | ||
| appInstallationToken = cachedToken.getToken(); | ||
| } | ||
| LOGGER.log(Level.FINER, "Returned GitHub App Installation Token for app ID {0}", appID); | ||
|
|
||
| return Secret.fromString(appInstallationToken); | ||
| } | ||
|
|
@@ -171,57 +198,171 @@ public String getUsername() { | |
| return appID; | ||
| } | ||
|
|
||
| static class AppInstallationToken implements Serializable { | ||
| /** | ||
| * {@link #getPassword()} checks that the token is still valid before returning it. | ||
| * The token will not expire for at least this amount of time after it is returned. | ||
| * | ||
| * Using a larger value will result in longer time-to-live for the token, but also more network | ||
| * calls related to getting new tokens. Setting a smaller value will result in less token generation | ||
| * but runs the the risk of the token expiring while it is still being used. | ||
| * | ||
| * The time-to-live for the token may be less than this if the initial expiration for the token when | ||
| * it is returned from GitHub is less than this. | ||
| */ | ||
| static final long MINIMUM_SECONDS_UNTIL_EXPIRATION = Duration.ofMinutes(45).getSeconds(); | ||
|
|
||
| /** | ||
| * Any token older than this is considered stale. | ||
| * | ||
| * This is a back stop to ensure that, in case of unforeseen error, | ||
| * expired tokens are not accidentally retained past their expiration. | ||
| */ | ||
| static final long MAXIMUM_AGE_SECONDS = Duration.ofMinutes(30).getSeconds(); | ||
|
|
||
| private final String token; | ||
| private final long tokenStaleEpochSeconds; | ||
|
|
||
| /** | ||
| * Create a AppInstallationToken instance. | ||
| * | ||
| * @param token the token string | ||
| * @param expirationEpochSeconds the time in epoch seconds that this token will expire | ||
| */ | ||
| public AppInstallationToken(String token, long expirationEpochSeconds) { | ||
| long nextSecond = Instant.now().getEpochSecond() + 1; | ||
|
|
||
| // Tokens go stale a while before they will expire | ||
| long staleEpochSeconds = expirationEpochSeconds - MINIMUM_SECONDS_UNTIL_EXPIRATION; | ||
|
|
||
| // Tokens are not stale as soon as they are made | ||
| if (staleEpochSeconds < nextSecond) { | ||
| staleEpochSeconds = nextSecond; | ||
| } else { | ||
| // Tokens have a maximum age at which they go stale | ||
| staleEpochSeconds = Math.min(staleEpochSeconds, nextSecond + MAXIMUM_AGE_SECONDS); | ||
| } | ||
| LOGGER.log(Level.FINER, "Token stale time epoch seconds: {0}", staleEpochSeconds); | ||
|
|
||
| this.token = token; | ||
| this.tokenStaleEpochSeconds = staleEpochSeconds; | ||
| } | ||
|
|
||
| public String getToken() { | ||
| return token; | ||
| } | ||
|
|
||
| /** | ||
| * Whether a token is stale and should be replaced with a new token. | ||
| * | ||
| * {@link #getPassword()} checks that the token is not "stale" before returning it. | ||
| * If a token is "stale" if it has expired, exceeded {@link #MAXIMUM_AGE_SECONDS}, or | ||
| * will expire in less than {@link #MINIMUM_SECONDS_UNTIL_EXPIRATION}. | ||
| * | ||
| * @return {@code true} if token should be refreshed, otherwise {@code false}. | ||
| */ | ||
| public boolean isStale() { | ||
| return Instant.now().getEpochSecond() >= tokenStaleEpochSeconds; | ||
| } | ||
|
|
||
| long getTokenStaleEpochSeconds() { | ||
| return tokenStaleEpochSeconds; | ||
| } | ||
|
|
||
| } | ||
|
|
||
| /** | ||
| * Ensures that the credentials state as serialized via Remoting to an agent calls back to the controller. | ||
| * Benefits: | ||
| * <ul> | ||
| * <li>The token is cached locally and used until it is stale. | ||
| * <li>The agent never needs to have access to the plaintext private key. | ||
| * <li>We avoid the considerable amount of class loading associated with the JWT library, Jackson data binding, Bouncy Castle, etc. | ||
| * <li>The agent need not be able to contact GitHub. | ||
| * </ul> | ||
| * @see CredentialsSnapshotTaker | ||
| */ | ||
| private Object writeReplace() { | ||
| if (/* XStream */Channel.current() == null) { | ||
| return this; | ||
| } | ||
| return new DelegatingGitHubAppCredentials(this); | ||
| } | ||
|
|
||
| private static final class DelegatingGitHubAppCredentials extends BaseStandardCredentials implements StandardUsernamePasswordCredentials { | ||
|
|
||
| private static final String SEP = "%%%"; | ||
|
||
|
|
||
| private final String appID; | ||
| private final String tokenRefreshData; | ||
bitwiseman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| private AppInstallationToken cachedToken; | ||
|
|
||
| private transient Channel ch; | ||
|
|
||
| DelegatingGitHubAppCredentials(GitHubAppCredentials onMaster) { | ||
| super(onMaster.getScope(), onMaster.getId(), onMaster.getDescription()); | ||
| JenkinsJVM.checkJenkinsJVM(); | ||
| appID = onMaster.appID; | ||
| tokenRefreshData = Secret.fromString(onMaster.appID + SEP + onMaster.privateKey.getPlainText() + SEP + onMaster.actualApiUri() + SEP + onMaster.owner).getEncryptedValue(); | ||
| synchronized (onMaster) { | ||
| cachedToken = onMaster.cachedToken; | ||
| } | ||
| } | ||
|
|
||
| private Object readResolve() { | ||
| JenkinsJVM.checkNotJenkinsJVM(); | ||
| synchronized (this) { | ||
| ch = Channel.currentOrFail(); | ||
| } | ||
| return this; | ||
| } | ||
|
|
||
| @NonNull | ||
| @Override | ||
| public String getUsername() { | ||
| return appID; | ||
| } | ||
|
|
||
| @Override | ||
| public Secret getPassword() { | ||
| JenkinsJVM.checkNotJenkinsJVM(); | ||
| try { | ||
| String appInstallationToken; | ||
| synchronized (this) { | ||
| if (cachedToken == null || cachedToken.isStale()) { | ||
| cachedToken = ch.call(new GetToken(tokenRefreshData)); | ||
| } | ||
| appInstallationToken = cachedToken.getToken(); | ||
| } | ||
| LOGGER.log(Level.FINER, "Returned GitHub App Installation Token for app ID {0} on agent", appID); | ||
|
|
||
| return Secret.fromString(appInstallationToken); | ||
| } catch (IOException | InterruptedException x) { | ||
| LOGGER.log(Level.WARNING, "Failed to get GitHub App Installation token on agent: " + getId(), x); | ||
| throw new RuntimeException(x); | ||
| } | ||
| } | ||
|
|
||
| private static final class GetToken extends SlaveToMasterCallable<AppInstallationToken, RuntimeException> { | ||
|
|
||
| private final String data; | ||
|
|
||
| GetToken(String data) { | ||
| this.data = data; | ||
| } | ||
|
|
||
| @Override | ||
| public AppInstallationToken call() throws RuntimeException { | ||
| JenkinsJVM.checkJenkinsJVM(); | ||
| String[] fields = Secret.fromString(data).getPlainText().split(SEP); | ||
| LOGGER.log(Level.FINE, "Generating App Installation Token for app ID {0} for agent", fields[0]); | ||
| return generateAppInstallationToken(fields[0], | ||
| fields[1], | ||
| fields[2], | ||
| fields[3]); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * {@inheritDoc} | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| package org.jenkinsci.plugins.github_branch_source; | ||
|
|
||
| import org.junit.Test; | ||
|
|
||
| import java.time.Duration; | ||
| import java.time.Instant; | ||
|
|
||
| import static org.hamcrest.MatcherAssert.assertThat; | ||
| import static org.hamcrest.Matchers.*; | ||
|
|
||
| public class GitHubAppCredentialsTest { | ||
|
|
||
| @Test | ||
| public void testAppInstallationTokenStale() throws Exception { | ||
|
|
||
| GitHubAppCredentials.AppInstallationToken token; | ||
| long now; | ||
|
|
||
| now = Instant.now().getEpochSecond(); | ||
| token = new GitHubAppCredentials.AppInstallationToken("", now); | ||
| assertThat(token.isStale(), is(false)); | ||
| assertThat(token.getTokenStaleEpochSeconds(), equalTo(now + 1)); | ||
|
|
||
| Thread.sleep(1000); | ||
| assertThat(token.isStale(), is(true)); | ||
|
|
||
| now = Instant.now().getEpochSecond(); | ||
| token = new GitHubAppCredentials.AppInstallationToken("", | ||
| now + Duration.ofMinutes(15).getSeconds()); | ||
| assertThat(token.isStale(), is(false)); | ||
| assertThat(token.getTokenStaleEpochSeconds(), equalTo(now + 1)); | ||
|
|
||
| now = Instant.now().getEpochSecond(); | ||
| token = new GitHubAppCredentials.AppInstallationToken("", | ||
| now + GitHubAppCredentials.AppInstallationToken.MINIMUM_SECONDS_UNTIL_EXPIRATION + 2); | ||
| assertThat(token.isStale(), is(false)); | ||
| assertThat(token.getTokenStaleEpochSeconds(), equalTo(now + 2)); | ||
|
|
||
| now = Instant.now().getEpochSecond(); | ||
| token = new GitHubAppCredentials.AppInstallationToken("", | ||
| now + GitHubAppCredentials.AppInstallationToken.MINIMUM_SECONDS_UNTIL_EXPIRATION + Duration.ofMinutes(7).getSeconds()); | ||
| assertThat(token.isStale(), is(false)); | ||
| assertThat(token.getTokenStaleEpochSeconds(), equalTo(now + Duration.ofMinutes(7).getSeconds())); | ||
|
|
||
| now = Instant.now().getEpochSecond(); | ||
| token = new GitHubAppCredentials.AppInstallationToken("", | ||
| now + Duration.ofMinutes(90).getSeconds()); | ||
| assertThat(token.isStale(), is(false)); | ||
| assertThat(token.getTokenStaleEpochSeconds(), equalTo(now + GitHubAppCredentials.AppInstallationToken.MAXIMUM_AGE_SECONDS + 1)); | ||
| } | ||
| } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of returning the bare token, return a serializable instance of the token and when it should be refreshed.