Skip to content

[JENKINS-62249] Delegate GitHub App token generation from agents and improve caching #326

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.

Sign up for GitHub

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

Merged
merged 19 commits into from
Sep 1, 2020
Merged

[JENKINS-62249] Delegate GitHub App token generation from agents and improve caching #326

Changes from 8 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
fd58e5e
GitHubAppCredentials on remote side to use SlaveToMasterCallable
jglick Aug 20, 2020
22ef38d
SpotBugs
jglick Aug 20, 2020
74902c3
No need to contact controller just to get appID
jglick Aug 20, 2020
7f23a79
Secret.fromString will not work on the agent side prior to 2.235.x
jglick Aug 20, 2020
de2860b
Maybe it works to call the Secret ctor on the controller side (the se…
jglick Aug 20, 2020
790ec23
Adding some JenkinsJVM calls to try to clarify the control flow
jglick Aug 20, 2020
e48eb83
Possibly more descriptive name
jglick Aug 20, 2020
8367aeb
Add accurate token expiration and agent-side caching
bitwiseman Aug 24, 2020
1443730
Add more logging
bitwiseman Aug 24, 2020
b76dbdb
Add test for AppInstallationToken
bitwiseman Aug 25, 2020
3ab69eb
Update src/main/java/org/jenkinsci/plugins/github_branch_source/GitHu…
bitwiseman Aug 25, 2020
23773cd
Renamed token test
bitwiseman Aug 28, 2020
81c9163
Add test for GitHubAppCredentials
bitwiseman Aug 30, 2020
dc3b565
Addtional testing revealed poor behavior
bitwiseman Aug 30, 2020
52526b4
Check validity of app token before sending to agents
bitwiseman Aug 31, 2020
54e01d5
Merge remote-tracking branch 'upstream/master' into issue/appid-expir…
bitwiseman Sep 1, 2020
3ac6082
Simplified log checking
bitwiseman Sep 1, 2020
d80588e
Update based on review feedback
bitwiseman Sep 1, 2020
b605c64
Switch to JSONObject for data blob
bitwiseman Sep 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
250 changes: 192 additions & 58 deletions src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentials.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.github.GitHubBuilder;
import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.DataBoundSetter;
import org.kohsuke.stapler.QueryParameter;
Expand All @@ -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?";

Expand All @@ -49,8 +57,7 @@ public class GitHubAppCredentials extends BaseStandardCredentials implements Sta

private String owner;

private transient String cachedToken;
private transient long tokenCacheTime;
private transient AppInstallationToken cachedToken;

@DataBoundConstructor
@SuppressWarnings("unused") // by stapler
Expand Down Expand Up @@ -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 String generateAppInstallationToken(String appId, String appPrivateKey, String apiUrl, String owner) {
static AppInstallationToken generateAppInstallationToken(String appId, String appPrivateKey, String apiUrl, String owner) {
Copy link
Contributor Author

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.

try {
String jwtToken = createJWT(appId, appPrivateKey);
GitHub gitHubApp = Connector
Expand Down Expand Up @@ -132,11 +139,33 @@ static String generateAppInstallationToken(String appId, String appPrivateKey, S
.createToken(appInstallation.getPermissions())
.create();

return appInstallationToken.getToken();
long expiration = getExpirationSeconds(appInstallationToken);

LOGGER.log(Level.FINE, "Generated App Installation Token for app ID {0}", appId);

return new AppInstallationToken(appInstallationToken.getToken(), expiration);
} catch (IOException e) {
LOGGER.log(Level.WARNING, "Failed to retrieve GitHub App installation token for app ID " + appId, e);
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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What exception is expected here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was confused about this earlier too. IOException is declared to be thrown by GHAppInstallationToken.getExpiresAt, but looking at GitHubClient.parseDate I think the throws clause on GHAppInstallationToken.getExpiresAt could just be dropped. Date parsing could throw runtime exceptions like DateTimeParseException (not sure what would cause it in practice, perhaps changes on GitHub's side), so maybe that is the kind of thing this is trying to catch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation has changed over time, but changing the throws is an API change that will cause compilation to fail, right?

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;
}

/**
Expand All @@ -145,19 +174,15 @@ static String generateAppInstallationToken(String appId, String appPrivateKey, S
@NonNull
@Override
public Secret getPassword() {
if (Util.fixEmpty(apiUri) == null) {
apiUri = "https://api.github.com";
}

long now = System.currentTimeMillis();
String appInstallationToken;
if (cachedToken != null && now - tokenCacheTime < JwtHelper.VALIDITY_MS /* extra buffer */ / 2) {
appInstallationToken = cachedToken;
} else {
appInstallationToken = generateAppInstallationToken(appID, privateKey.getPlainText(), apiUri, owner);
cachedToken = appInstallationToken;
tokenCacheTime = now;
synchronized (this) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}
Expand All @@ -171,57 +196,166 @@ 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.
*/
private final static 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.
*/
private 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 tokenExpirationEpochSeconds the time in epoch seconds that this token will expire
*/
public AppInstallationToken(String token, long tokenExpirationEpochSeconds) {
long nextSecond = Instant.now().getEpochSecond() + 1;

// Tokens go stale a while before they will expire
long tokenStaleEpochSeconds = tokenExpirationEpochSeconds - MINIMUM_SECONDS_UNTIL_EXPIRATION;

// Tokens are not stale as soon as they are made
if (tokenStaleEpochSeconds < nextSecond) {
tokenStaleEpochSeconds = nextSecond;
} else {
// Tokens have a maximum age at which they go stale
tokenStaleEpochSeconds = Math.min(tokenExpirationEpochSeconds, nextSecond + MAXIMUM_AGE_SECONDS);
}

this.token = token;
this.tokenStaleEpochSeconds = tokenStaleEpochSeconds;
}

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;
}

}

/**
* Ensures that the credentials state as serialized via Remoting to an agent includes fields which are {@code transient} for purposes of XStream.
* This provides a ~2× performance improvement over reconstructing the object without that state,
* in the normal case that {@link #cachedToken} is valid and will remain valid for the brief time that elapses before the agent calls {@link #getPassword}:
* Ensures that the credentials state as serialized via Remoting to an agent calls back to the controller.
* Benefits:
* <ul>
* <li>We do not need to make API calls to GitHub to obtain a new token.
* <li>We can avoid the considerable amount of class loading associated with the JWT library, Jackson data binding, Bouncy Castle, etc.
* <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 Replacer(this);
}

private static final class Replacer implements Serializable {

private final CredentialsScope scope;
private final String id;
private final String description;
private final String appID;
private final Secret privateKey;
private final String apiUri;
private final String owner;
private final String cachedToken;
private final long tokenCacheTime;

Replacer(GitHubAppCredentials onMaster) {
scope = onMaster.getScope();
id = onMaster.getId();
description = onMaster.getDescription();
appID = onMaster.appID;
privateKey = onMaster.privateKey;
apiUri = onMaster.apiUri;
owner = onMaster.owner;
cachedToken = onMaster.cachedToken;
tokenCacheTime = onMaster.tokenCacheTime;
}

private Object readResolve() {
GitHubAppCredentials clone = new GitHubAppCredentials(scope, id, description, appID, privateKey);
clone.apiUri = apiUri;
clone.owner = owner;
clone.cachedToken = cachedToken;
clone.tokenCacheTime = tokenCacheTime;
return clone;
}

}
return new DelegatingGitHubAppCredentials(this);
}

private static final class DelegatingGitHubAppCredentials extends BaseStandardCredentials implements StandardUsernamePasswordCredentials {

private static final String SEP = "%%%";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried to pick something unlikely to occur in any of the field values. I guess you could use JSONObject to be a little safer.


private final String appID;
private final String tokenRefreshData;
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}
Expand Down