From fd58e5e10975947f1e33cee2f7982d5528876918 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 20 Aug 2020 14:56:05 -0400 Subject: [PATCH 01/18] GitHubAppCredentials on remote side to use SlaveToMasterCallable --- .../GitHubAppCredentials.java | 126 +++++++++++------- 1 file changed, 81 insertions(+), 45 deletions(-) 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 1dccb8e11..e28e52fbe 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 @@ -14,15 +14,14 @@ import hudson.util.ListBoxModel; import hudson.util.Secret; import java.io.IOException; -import java.io.Serializable; import java.util.List; +import jenkins.security.SlaveToMasterCallable; 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; @@ -139,22 +138,22 @@ static String generateAppInstallationToken(String appId, String appPrivateKey, S } + @NonNull String actualApiUri() { + return Util.fixEmpty(apiUri) == null ? "https://api.github.com" : apiUri; + } + /** * {@inheritDoc} */ @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); + appInstallationToken = generateAppInstallationToken(appID, privateKey.getPlainText(), actualApiUri(), owner); cachedToken = appInstallationToken; tokenCacheTime = now; } @@ -172,12 +171,16 @@ public String getUsername() { } /** - * 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: * + * Drawbacks: + * * @see CredentialsSnapshotTaker */ @@ -185,43 +188,76 @@ private Object writeReplace() { if (/* XStream */Channel.current() == null) { return this; } - return new Replacer(this); + return new AgentSide(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; - } + private static final class AgentSide extends BaseStandardCredentials implements StandardUsernamePasswordCredentials { - } + static final String SEP = "%%%"; + + private final String data; + private Channel ch; + + AgentSide(GitHubAppCredentials onMaster) { + super(onMaster.getScope(), onMaster.getId(), onMaster.getDescription()); + data = Secret.fromString(onMaster.appID + SEP + onMaster.privateKey.getPlainText() + SEP + onMaster.actualApiUri() + SEP + onMaster.owner).getEncryptedValue(); + } + + private Object readResolve() { + ch = Channel.currentOrFail(); + return this; + } + + @Override + public String getUsername() { + try { + return ch.call(new GetUsername(data)); + } catch (IOException | InterruptedException x) { + throw new RuntimeException(x); + } + } + + @Override + public Secret getPassword() { + try { + return Secret.fromString(ch.call(new GetPassword(data))); + } catch (IOException | InterruptedException x) { + throw new RuntimeException(x); + } + } + + private static final class GetUsername extends SlaveToMasterCallable { + + private final String data; + + GetUsername(String data) { + this.data = data; + } + + @Override + public String call() throws RuntimeException { + return Secret.fromString(data).getPlainText().split(SEP)[0]; + } + + } + + private static final class GetPassword extends SlaveToMasterCallable { + + private final String data; + + GetPassword(String data) { + this.data = data; + } + + @Override + public String call() throws RuntimeException { + String[] fields = Secret.fromString(data).getPlainText().split(SEP); + return generateAppInstallationToken(fields[0], fields[1], fields[2], fields[3]); + } + + } + + } /** * {@inheritDoc} From 22ef38db8c0c61986df8b0cafa6103c34dc993b0 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 20 Aug 2020 15:07:33 -0400 Subject: [PATCH 02/18] SpotBugs --- .../plugins/github_branch_source/GitHubAppCredentials.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 e28e52fbe..72ae28b21 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 @@ -196,7 +196,7 @@ private static final class AgentSide extends BaseStandardCredentials implements static final String SEP = "%%%"; private final String data; - private Channel ch; + private transient Channel ch; AgentSide(GitHubAppCredentials onMaster) { super(onMaster.getScope(), onMaster.getId(), onMaster.getDescription()); From 74902c38d8dc16502b39fdef60212ace150b1fde Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 20 Aug 2020 15:30:50 -0400 Subject: [PATCH 03/18] No need to contact controller just to get appID --- .../GitHubAppCredentials.java | 23 +++---------------- 1 file changed, 3 insertions(+), 20 deletions(-) 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 72ae28b21..01fc08fe5 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 @@ -195,11 +195,13 @@ private static final class AgentSide extends BaseStandardCredentials implements static final String SEP = "%%%"; + private final String appID; private final String data; private transient Channel ch; AgentSide(GitHubAppCredentials onMaster) { super(onMaster.getScope(), onMaster.getId(), onMaster.getDescription()); + appID = onMaster.appID; data = Secret.fromString(onMaster.appID + SEP + onMaster.privateKey.getPlainText() + SEP + onMaster.actualApiUri() + SEP + onMaster.owner).getEncryptedValue(); } @@ -210,11 +212,7 @@ private Object readResolve() { @Override public String getUsername() { - try { - return ch.call(new GetUsername(data)); - } catch (IOException | InterruptedException x) { - throw new RuntimeException(x); - } + return appID; } @Override @@ -226,21 +224,6 @@ public Secret getPassword() { } } - private static final class GetUsername extends SlaveToMasterCallable { - - private final String data; - - GetUsername(String data) { - this.data = data; - } - - @Override - public String call() throws RuntimeException { - return Secret.fromString(data).getPlainText().split(SEP)[0]; - } - - } - private static final class GetPassword extends SlaveToMasterCallable { private final String data; From 7f23a79a332f45ad99fa3c7bb6a4755eba02152f Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 20 Aug 2020 15:31:59 -0400 Subject: [PATCH 04/18] Secret.fromString will not work on the agent side prior to 2.235.x --- Jenkinsfile | 2 +- pom.xml | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Jenkinsfile b/Jenkinsfile index 18cd978f6..f791c4a59 100644 --- a/Jenkinsfile +++ b/Jenkinsfile @@ -1,5 +1,5 @@ buildPlugin(configurations: [ [ platform: "linux", jdk: "8"], [ platform: "windows", jdk: "8"], - [ platform: "linux", jdk: "11", jenkins: "2.176.4", javaLevel: "8" ] + [ platform: "linux", jdk: "11"] ]) diff --git a/pom.xml b/pom.xml index c0f0fb42d..ff9658fe7 100644 --- a/pom.xml +++ b/pom.xml @@ -25,7 +25,7 @@ -SNAPSHOT 2.2.0 8 - 2.176.4 + 2.235.1 true 0.11.2 @@ -114,7 +114,7 @@ io.jenkins.tools.bom - bom-2.176.x + bom-2.235.x 11 import pom From de2860b32fe078065c23e8775aed286772edec25 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 20 Aug 2020 15:51:23 -0400 Subject: [PATCH 05/18] Maybe it works to call the Secret ctor on the controller side (the serial form is plaintext) --- Jenkinsfile | 2 +- pom.xml | 4 ++-- .../github_branch_source/GitHubAppCredentials.java | 8 ++++---- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/Jenkinsfile b/Jenkinsfile index f791c4a59..18cd978f6 100644 --- a/Jenkinsfile +++ b/Jenkinsfile @@ -1,5 +1,5 @@ buildPlugin(configurations: [ [ platform: "linux", jdk: "8"], [ platform: "windows", jdk: "8"], - [ platform: "linux", jdk: "11"] + [ platform: "linux", jdk: "11", jenkins: "2.176.4", javaLevel: "8" ] ]) diff --git a/pom.xml b/pom.xml index ff9658fe7..c0f0fb42d 100644 --- a/pom.xml +++ b/pom.xml @@ -25,7 +25,7 @@ -SNAPSHOT 2.2.0 8 - 2.235.1 + 2.176.4 true 0.11.2 @@ -114,7 +114,7 @@ io.jenkins.tools.bom - bom-2.235.x + bom-2.176.x 11 import pom 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 01fc08fe5..c846dcd8d 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 @@ -218,13 +218,13 @@ public String getUsername() { @Override public Secret getPassword() { try { - return Secret.fromString(ch.call(new GetPassword(data))); + return ch.call(new GetPassword(data)); } catch (IOException | InterruptedException x) { throw new RuntimeException(x); } } - private static final class GetPassword extends SlaveToMasterCallable { + private static final class GetPassword extends SlaveToMasterCallable { private final String data; @@ -233,9 +233,9 @@ private static final class GetPassword extends SlaveToMasterCallable Date: Thu, 20 Aug 2020 15:54:02 -0400 Subject: [PATCH 06/18] Adding some JenkinsJVM calls to try to clarify the control flow --- .../plugins/github_branch_source/GitHubAppCredentials.java | 5 +++++ 1 file changed, 5 insertions(+) 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 c846dcd8d..752ae6624 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 @@ -16,6 +16,7 @@ import java.io.IOException; import java.util.List; import jenkins.security.SlaveToMasterCallable; +import jenkins.util.JenkinsJVM; import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.restrictions.NoExternalUse; import org.kohsuke.github.GHApp; @@ -201,11 +202,13 @@ private static final class AgentSide extends BaseStandardCredentials implements AgentSide(GitHubAppCredentials onMaster) { super(onMaster.getScope(), onMaster.getId(), onMaster.getDescription()); + JenkinsJVM.checkJenkinsJVM(); appID = onMaster.appID; data = Secret.fromString(onMaster.appID + SEP + onMaster.privateKey.getPlainText() + SEP + onMaster.actualApiUri() + SEP + onMaster.owner).getEncryptedValue(); } private Object readResolve() { + JenkinsJVM.checkNotJenkinsJVM(); ch = Channel.currentOrFail(); return this; } @@ -217,6 +220,7 @@ public String getUsername() { @Override public Secret getPassword() { + JenkinsJVM.checkNotJenkinsJVM(); try { return ch.call(new GetPassword(data)); } catch (IOException | InterruptedException x) { @@ -234,6 +238,7 @@ private static final class GetPassword extends SlaveToMasterCallable Date: Thu, 20 Aug 2020 16:11:13 -0400 Subject: [PATCH 07/18] Possibly more descriptive name --- .../plugins/github_branch_source/GitHubAppCredentials.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 752ae6624..2fc888c58 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 @@ -189,10 +189,10 @@ private Object writeReplace() { if (/* XStream */Channel.current() == null) { return this; } - return new AgentSide(this); + return new DelegatingGitHubAppCredentials(this); } - private static final class AgentSide extends BaseStandardCredentials implements StandardUsernamePasswordCredentials { + private static final class DelegatingGitHubAppCredentials extends BaseStandardCredentials implements StandardUsernamePasswordCredentials { static final String SEP = "%%%"; @@ -200,7 +200,7 @@ private static final class AgentSide extends BaseStandardCredentials implements private final String data; private transient Channel ch; - AgentSide(GitHubAppCredentials onMaster) { + DelegatingGitHubAppCredentials(GitHubAppCredentials onMaster) { super(onMaster.getScope(), onMaster.getId(), onMaster.getDescription()); JenkinsJVM.checkJenkinsJVM(); appID = onMaster.appID; From 8367aebfa5ca4f3e386c8fa08729f2c748ea5886 Mon Sep 17 00:00:00 2001 From: Liam Newman Date: Mon, 24 Aug 2020 07:56:36 -0700 Subject: [PATCH 08/18] Add accurate token expiration and agent-side caching --- .../GitHubAppCredentials.java | 166 +++++++++++++++--- 1 file changed, 138 insertions(+), 28 deletions(-) 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 2fc888c58..65f14458d 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 @@ -14,7 +14,13 @@ import hudson.util.ListBoxModel; 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; @@ -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 String cachedToken; - private transient long tokenCacheTime; + 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 String generateAppInstallationToken(String appId, String appPrivateKey, String apiUrl, String owner) { + static AppInstallationToken generateAppInstallationToken(String appId, String appPrivateKey, String apiUrl, String owner) { try { String jwtToken = createJWT(appId, appPrivateKey); GitHub gitHubApp = Connector @@ -132,11 +139,29 @@ 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) { + // 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() { @@ -149,15 +174,15 @@ static String generateAppInstallationToken(String appId, String appPrivateKey, S @NonNull @Override public Secret getPassword() { - 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(), actualApiUri(), owner); - cachedToken = appInstallationToken; - tokenCacheTime = now; + synchronized (this) { + 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,18 +196,83 @@ 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 calls back to the controller. * Benefits: *
    + *
  • The token is cached locally and used until it is stale. *
  • The agent never needs to have access to the plaintext private key. - *
  • We can avoid the considerable amount of class loading associated with the JWT library, Jackson data binding, Bouncy Castle, etc. + *
  • We avoid the considerable amount of class loading associated with the JWT library, Jackson data binding, Bouncy Castle, etc. *
  • The agent need not be able to contact GitHub. *
- * Drawbacks: - *
    - *
  • There is no caching, so every access requires GitHub API traffic as well as Remoting traffic. - *
* @see CredentialsSnapshotTaker */ private Object writeReplace() { @@ -190,29 +280,37 @@ private Object writeReplace() { return this; } return new DelegatingGitHubAppCredentials(this); - } + } private static final class DelegatingGitHubAppCredentials extends BaseStandardCredentials implements StandardUsernamePasswordCredentials { - static final String SEP = "%%%"; + private static final String SEP = "%%%"; private final String appID; - private final String data; + 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; - data = Secret.fromString(onMaster.appID + SEP + onMaster.privateKey.getPlainText() + SEP + onMaster.actualApiUri() + SEP + onMaster.owner).getEncryptedValue(); + 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(); - ch = Channel.currentOrFail(); + synchronized (this) { + ch = Channel.currentOrFail(); + } return this; } + @NonNull @Override public String getUsername() { return appID; @@ -222,29 +320,41 @@ public String getUsername() { public Secret getPassword() { JenkinsJVM.checkNotJenkinsJVM(); try { - return ch.call(new GetPassword(data)); + 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 GetPassword extends SlaveToMasterCallable { + private static final class GetToken extends SlaveToMasterCallable { private final String data; - GetPassword(String data) { + GetToken(String data) { this.data = data; } @Override - public Secret call() throws RuntimeException { + public AppInstallationToken call() throws RuntimeException { JenkinsJVM.checkJenkinsJVM(); String[] fields = Secret.fromString(data).getPlainText().split(SEP); - return Secret.fromString(generateAppInstallationToken(fields[0], fields[1], fields[2], fields[3])); + 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]); } - } - } /** From 14437303982583f517181e686005615867a46168 Mon Sep 17 00:00:00 2001 From: Liam Newman Date: Mon, 24 Aug 2020 16:43:52 -0700 Subject: [PATCH 09/18] Add more logging --- .../plugins/github_branch_source/GitHubAppCredentials.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) 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 65f14458d..e22528313 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 @@ -140,10 +140,12 @@ static AppInstallationToken generateAppInstallationToken(String appId, String ap .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 new AppInstallationToken(appInstallationToken.getToken(), expiration); + return token; } 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); @@ -240,6 +242,7 @@ public AppInstallationToken(String token, long tokenExpirationEpochSeconds) { // Tokens have a maximum age at which they go stale tokenStaleEpochSeconds = Math.min(tokenExpirationEpochSeconds, nextSecond + MAXIMUM_AGE_SECONDS); } + LOGGER.log(Level.FINER, "Token stale time epoch seconds: {0}", tokenStaleEpochSeconds); this.token = token; this.tokenStaleEpochSeconds = tokenStaleEpochSeconds; From b76dbdbdee51e0206b305ed2dad8588712d4e5cb Mon Sep 17 00:00:00 2001 From: Liam Newman Date: Mon, 24 Aug 2020 17:33:56 -0700 Subject: [PATCH 10/18] Add test for AppInstallationToken --- .../GitHubAppCredentials.java | 24 +++++---- .../GitHubAppCredentialsTest.java | 51 +++++++++++++++++++ 2 files changed, 65 insertions(+), 10 deletions(-) create mode 100644 src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentialsTest.java 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 e22528313..fb7a1388d 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 @@ -210,7 +210,7 @@ static class AppInstallationToken implements Serializable { * 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(); + static final long MINIMUM_SECONDS_UNTIL_EXPIRATION = Duration.ofMinutes(45).getSeconds(); /** * Any token older than this is considered stale. @@ -218,7 +218,7 @@ static class AppInstallationToken implements Serializable { * 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(); + static final long MAXIMUM_AGE_SECONDS = Duration.ofMinutes(30).getSeconds(); private final String token; private final long tokenStaleEpochSeconds; @@ -227,25 +227,25 @@ static class AppInstallationToken implements Serializable { * Create a AppInstallationToken instance. * * @param token the token string - * @param tokenExpirationEpochSeconds the time in epoch seconds that this token will expire + * @param expirationEpochSeconds the time in epoch seconds that this token will expire */ - public AppInstallationToken(String token, long tokenExpirationEpochSeconds) { + public AppInstallationToken(String token, long expirationEpochSeconds) { long nextSecond = Instant.now().getEpochSecond() + 1; // Tokens go stale a while before they will expire - long tokenStaleEpochSeconds = tokenExpirationEpochSeconds - MINIMUM_SECONDS_UNTIL_EXPIRATION; + long staleEpochSeconds = expirationEpochSeconds - MINIMUM_SECONDS_UNTIL_EXPIRATION; // Tokens are not stale as soon as they are made - if (tokenStaleEpochSeconds < nextSecond) { - tokenStaleEpochSeconds = nextSecond; + if (staleEpochSeconds < nextSecond) { + staleEpochSeconds = nextSecond; } else { // Tokens have a maximum age at which they go stale - tokenStaleEpochSeconds = Math.min(tokenExpirationEpochSeconds, nextSecond + MAXIMUM_AGE_SECONDS); + staleEpochSeconds = Math.min(staleEpochSeconds, nextSecond + MAXIMUM_AGE_SECONDS); } - LOGGER.log(Level.FINER, "Token stale time epoch seconds: {0}", tokenStaleEpochSeconds); + LOGGER.log(Level.FINER, "Token stale time epoch seconds: {0}", staleEpochSeconds); this.token = token; - this.tokenStaleEpochSeconds = tokenStaleEpochSeconds; + this.tokenStaleEpochSeconds = staleEpochSeconds; } public String getToken() { @@ -265,6 +265,10 @@ public boolean isStale() { return Instant.now().getEpochSecond() >= tokenStaleEpochSeconds; } + long getTokenStaleEpochSeconds() { + return tokenStaleEpochSeconds; + } + } /** 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 new file mode 100644 index 000000000..5dd0883ab --- /dev/null +++ b/src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentialsTest.java @@ -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)); + } +} From 3ab69ebb2f265dcdc87597d6eeb7f20b2345b59c Mon Sep 17 00:00:00 2001 From: Liam Newman Date: Tue, 25 Aug 2020 12:27:33 -0700 Subject: [PATCH 11/18] Update src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentials.java Co-authored-by: Devin Nusbaum --- .../plugins/github_branch_source/GitHubAppCredentials.java | 4 ++++ 1 file changed, 4 insertions(+) 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 fb7a1388d..c481d9285 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 @@ -294,6 +294,10 @@ private static final class DelegatingGitHubAppCredentials extends BaseStandardCr private static final String SEP = "%%%"; private final String appID; + /** + * An encrypted form of all data needed to refresh the token. + * Used to prevent {@link GetToken} from being abused by compromised build agents. + */ private final String tokenRefreshData; private AppInstallationToken cachedToken; From 23773cd1eaef5e039d1ce54c18f1788384a71bed Mon Sep 17 00:00:00 2001 From: Liam Newman Date: Fri, 28 Aug 2020 15:32:44 -0700 Subject: [PATCH 12/18] Renamed token test --- ...bAppCredentialsAppInstallationTokenTest.java} | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) rename src/test/java/org/jenkinsci/plugins/github_branch_source/{GitHubAppCredentialsTest.java => GithubAppCredentialsAppInstallationTokenTest.java} (77%) diff --git a/src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentialsTest.java b/src/test/java/org/jenkinsci/plugins/github_branch_source/GithubAppCredentialsAppInstallationTokenTest.java similarity index 77% rename from src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentialsTest.java rename to src/test/java/org/jenkinsci/plugins/github_branch_source/GithubAppCredentialsAppInstallationTokenTest.java index 5dd0883ab..e58f632cf 100644 --- a/src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentialsTest.java +++ b/src/test/java/org/jenkinsci/plugins/github_branch_source/GithubAppCredentialsAppInstallationTokenTest.java @@ -1,6 +1,8 @@ package org.jenkinsci.plugins.github_branch_source; +import org.junit.ClassRule; import org.junit.Test; +import org.jvnet.hudson.test.JenkinsRule; import java.time.Duration; import java.time.Instant; @@ -8,7 +10,7 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.*; -public class GitHubAppCredentialsTest { +public class GithubAppCredentialsAppInstallationTokenTest { @Test public void testAppInstallationTokenStale() throws Exception { @@ -38,14 +40,18 @@ public void testAppInstallationTokenStale() throws Exception { now = Instant.now().getEpochSecond(); token = new GitHubAppCredentials.AppInstallationToken("", - now + GitHubAppCredentials.AppInstallationToken.MINIMUM_SECONDS_UNTIL_EXPIRATION + Duration.ofMinutes(7).getSeconds()); + 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())); + 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)); + assertThat(token.getTokenStaleEpochSeconds(), + equalTo(now + GitHubAppCredentials.AppInstallationToken.MAXIMUM_AGE_SECONDS + 1)); } -} +} \ No newline at end of file From 81c9163ffe8c9f86debab879ee6e4cf674b27563 Mon Sep 17 00:00:00 2001 From: Liam Newman Date: Sun, 30 Aug 2020 00:30:36 -0700 Subject: [PATCH 13/18] Add test for GitHubAppCredentials --- pom.xml | 33 ++++ .../GitHubAppCredentials.java | 37 ++-- ...ppCredentialsAppInstallationTokenTest.java | 34 ++-- .../GithubAppCredentialsTest.java | 181 ++++++++++++++++++ .../github_branch_source/JwtHelperTest.java | 4 +- ...bapp-create-installation-accesstokens.json | 105 ++++++++++ .../files/body-mapping-githubapp-app.json | 39 ++++ .../body-mapping-githubapp-installations.json | 84 ++++++++ 8 files changed, 492 insertions(+), 25 deletions(-) create mode 100644 src/test/java/org/jenkinsci/plugins/github_branch_source/GithubAppCredentialsTest.java create mode 100644 src/test/resources/api/AppCredentials/files/body-githubapp-create-installation-accesstokens.json create mode 100644 src/test/resources/api/AppCredentials/files/body-mapping-githubapp-app.json create mode 100644 src/test/resources/api/AppCredentials/files/body-mapping-githubapp-installations.json diff --git a/pom.xml b/pom.xml index c0f0fb42d..1fca462bb 100644 --- a/pom.xml +++ b/pom.xml @@ -92,6 +92,39 @@ mockito-core test + + org.jenkins-ci.plugins.workflow + workflow-durable-task-step + + + org.jenkins-ci.plugins + durable-task + + + org.jenkins-ci.plugins + credentials + + + org.jenkins-ci.plugins.workflow + workflow-basic-steps + + + org.jenkins-ci.plugins + credentials-binding + + + org.jenkins-ci.plugins + git + tests + test + + + org.apache.httpcomponents + httpclient + + + + 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 c481d9285..1c4f7c8d1 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 @@ -162,7 +162,7 @@ private static long getExpirationSeconds(GHAppInstallationToken appInstallationT LOGGER.log(Level.WARNING, "Unable to get GitHub App installation token expiration", e); - return Instant.now().getEpochSecond() + AppInstallationToken.MAXIMUM_AGE_SECONDS; + return Instant.now().getEpochSecond() + AppInstallationToken.STALE_AFTER_SECONDS; } } @@ -210,7 +210,7 @@ static class AppInstallationToken implements Serializable { * 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(); + static final long STALE_WHEN_SECONDS_UNTIL_EXPIRATION = Duration.ofMinutes(45).getSeconds(); /** * Any token older than this is considered stale. @@ -218,7 +218,18 @@ static class AppInstallationToken implements Serializable { * 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(); + static final long STALE_AFTER_SECONDS = Duration.ofMinutes(30).getSeconds(); + + /** + * When a token is retrieved it cannot got stale for at least this many seconds. + * + * Prevents continuous refreshing of credentials. + * Non-final for testing purposes. + * This value takes precedence over {@link #STALE_WHEN_SECONDS_UNTIL_EXPIRATION}. + * {@link #STALE_WHEN_SECONDS_UNTIL_EXPIRATION} takes precedence over this value. + * Minimum value of 1. + */ + static long NOT_STALE_FOR_ATLEAST_SECONDS = Duration.ofMinutes(1).getSeconds(); private final String token; private final long tokenStaleEpochSeconds; @@ -230,18 +241,20 @@ static class AppInstallationToken implements Serializable { * @param expirationEpochSeconds the time in epoch seconds that this token will expire */ public AppInstallationToken(String token, long expirationEpochSeconds) { - long nextSecond = Instant.now().getEpochSecond() + 1; + long now = Instant.now().getEpochSecond(); + long minimumAge = now + Math.max(1, NOT_STALE_FOR_ATLEAST_SECONDS); // Tokens go stale a while before they will expire - long staleEpochSeconds = expirationEpochSeconds - MINIMUM_SECONDS_UNTIL_EXPIRATION; + long staleEpochSeconds = expirationEpochSeconds - STALE_WHEN_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); + if (staleEpochSeconds < minimumAge) { + staleEpochSeconds = minimumAge; } + + // Tokens have a maximum age at which they go stale + staleEpochSeconds = Math.min(staleEpochSeconds, now + 1 + STALE_AFTER_SECONDS); + LOGGER.log(Level.FINER, "Token stale time epoch seconds: {0}", staleEpochSeconds); this.token = token; @@ -256,8 +269,8 @@ public String getToken() { * 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}. + * If a token is "stale" if it has expired, exceeded {@link #STALE_AFTER_SECONDS}, or + * will expire in less than {@link #STALE_WHEN_SECONDS_UNTIL_EXPIRATION}. * * @return {@code true} if token should be refreshed, otherwise {@code false}. */ diff --git a/src/test/java/org/jenkinsci/plugins/github_branch_source/GithubAppCredentialsAppInstallationTokenTest.java b/src/test/java/org/jenkinsci/plugins/github_branch_source/GithubAppCredentialsAppInstallationTokenTest.java index e58f632cf..534299b35 100644 --- a/src/test/java/org/jenkinsci/plugins/github_branch_source/GithubAppCredentialsAppInstallationTokenTest.java +++ b/src/test/java/org/jenkinsci/plugins/github_branch_source/GithubAppCredentialsAppInstallationTokenTest.java @@ -1,8 +1,6 @@ package org.jenkinsci.plugins.github_branch_source; -import org.junit.ClassRule; import org.junit.Test; -import org.jvnet.hudson.test.JenkinsRule; import java.time.Duration; import java.time.Instant; @@ -21,26 +19,23 @@ public void testAppInstallationTokenStale() throws Exception { 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)); + assertThat(token.getTokenStaleEpochSeconds(), equalTo(now + GitHubAppCredentials.AppInstallationToken.NOT_STALE_FOR_ATLEAST_SECONDS)); now = Instant.now().getEpochSecond(); token = new GitHubAppCredentials.AppInstallationToken("", now + Duration.ofMinutes(15).getSeconds()); assertThat(token.isStale(), is(false)); - assertThat(token.getTokenStaleEpochSeconds(), equalTo(now + 1)); + assertThat(token.getTokenStaleEpochSeconds(), equalTo(now + GitHubAppCredentials.AppInstallationToken.NOT_STALE_FOR_ATLEAST_SECONDS)); now = Instant.now().getEpochSecond(); token = new GitHubAppCredentials.AppInstallationToken("", - now + GitHubAppCredentials.AppInstallationToken.MINIMUM_SECONDS_UNTIL_EXPIRATION + 2); + now + GitHubAppCredentials.AppInstallationToken.STALE_WHEN_SECONDS_UNTIL_EXPIRATION + 2); assertThat(token.isStale(), is(false)); - assertThat(token.getTokenStaleEpochSeconds(), equalTo(now + 2)); + assertThat(token.getTokenStaleEpochSeconds(), equalTo(now + GitHubAppCredentials.AppInstallationToken.NOT_STALE_FOR_ATLEAST_SECONDS)); now = Instant.now().getEpochSecond(); token = new GitHubAppCredentials.AppInstallationToken("", - now + GitHubAppCredentials.AppInstallationToken.MINIMUM_SECONDS_UNTIL_EXPIRATION + Duration + now + GitHubAppCredentials.AppInstallationToken.STALE_WHEN_SECONDS_UNTIL_EXPIRATION + Duration .ofMinutes(7) .getSeconds()); assertThat(token.isStale(), is(false)); @@ -52,6 +47,23 @@ public void testAppInstallationTokenStale() throws Exception { now + Duration.ofMinutes(90).getSeconds()); assertThat(token.isStale(), is(false)); assertThat(token.getTokenStaleEpochSeconds(), - equalTo(now + GitHubAppCredentials.AppInstallationToken.MAXIMUM_AGE_SECONDS + 1)); + equalTo(now + GitHubAppCredentials.AppInstallationToken.STALE_AFTER_SECONDS + 1)); + + long notStaleSeconds = GitHubAppCredentials.AppInstallationToken.NOT_STALE_FOR_ATLEAST_SECONDS; + try { + // Should revert to 1 second minimum + GitHubAppCredentials.AppInstallationToken.NOT_STALE_FOR_ATLEAST_SECONDS = -10; + + now = Instant.now().getEpochSecond(); + token = new GitHubAppCredentials.AppInstallationToken("", now); + assertThat(token.isStale(), is(false)); + assertThat(token.getTokenStaleEpochSeconds(), equalTo(now + 1)); + + // Verify goes stale + Thread.sleep(1000); + assertThat(token.isStale(), is(true)); + } finally { + GitHubAppCredentials.AppInstallationToken.NOT_STALE_FOR_ATLEAST_SECONDS = notStaleSeconds; + } } } \ No newline at end of file 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 new file mode 100644 index 000000000..df584a6fe --- /dev/null +++ b/src/test/java/org/jenkinsci/plugins/github_branch_source/GithubAppCredentialsTest.java @@ -0,0 +1,181 @@ +package org.jenkinsci.plugins.github_branch_source; + +import com.cloudbees.hudson.plugins.folder.computed.FolderComputation; +import com.cloudbees.plugins.credentials.CredentialsProvider; +import com.cloudbees.plugins.credentials.CredentialsScope; +import com.cloudbees.plugins.credentials.CredentialsStore; +import com.cloudbees.plugins.credentials.domains.Domain; +import hudson.model.Slave; +import hudson.util.LogTaskListener; +import hudson.util.RingBufferLogHandler; +import hudson.util.Secret; +import jenkins.branch.BranchSource; +import jenkins.plugins.git.GitSCMSource; +import jenkins.plugins.git.GitSampleRepoRule; +import jenkins.plugins.git.GitStep; +import org.jenkinsci.plugins.workflow.cps.CpsScmFlowDefinition; +import org.jenkinsci.plugins.workflow.job.WorkflowJob; +import org.jenkinsci.plugins.workflow.job.WorkflowRun; +import org.jenkinsci.plugins.workflow.multibranch.WorkflowMultiBranchProject; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Rule; +import org.junit.Test; +import org.jvnet.hudson.test.JenkinsRule; + +import javax.annotation.Nonnull; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.logging.Formatter; +import java.util.logging.Level; +import java.util.logging.LogRecord; +import java.util.logging.Logger; +import java.util.logging.SimpleFormatter; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +import static org.hamcrest.MatcherAssert.assertThat; +import static com.github.tomakehurst.wiremock.client.WireMock.*; +import static org.hamcrest.Matchers.*; + +public class GithubAppCredentialsTest extends AbstractGitHubWireMockTest { + + private RingBufferLogHandler handler; + + private static Slave agent; + private WorkflowMultiBranchProject owner; + private static final String myAppCredentialsId = "myAppCredentialsId"; + private static CredentialsStore store; + private static GitHubAppCredentials appCredentials; + + @Rule + public GitSampleRepoRule sampleRepo = new GitSampleRepoRule(); + + @BeforeClass + public static void setUpJenkins() throws Exception { + //Add credential (Must have valid private key for Jwt to work, but App doesn't have to actually exist) + store = CredentialsProvider.lookupStores(r.jenkins).iterator().next(); + appCredentials = new GitHubAppCredentials( + CredentialsScope.GLOBAL, myAppCredentialsId, "sample", "54321", Secret.fromString(JwtHelperTest.PKCS8_PRIVATE_KEY)); + appCredentials.setOwner("cloudbeers"); + store.addCredentials(Domain.global(), appCredentials); + + // Add agent + agent = r.createOnlineSlave(); + agent.setLabelString("my-agent"); + } + + @Before + public void setUpLogging() throws Exception { + handler = new RingBufferLogHandler(1000); + handler.setFormatter(new SimpleFormatter()); + final Logger logger = Logger.getLogger(GitHubAppCredentials.class.getName()); + logger.setLevel(Level.FINE); + logger.addHandler(handler); + } + + @Before + public void setUpWireMock() throws Exception { + //Add wiremock responses for App, App Installation, and App Installation Token + githubApi.stubFor( + get(urlEqualTo("/app")) + .willReturn( + aResponse() + .withHeader("Content-Type", "application/json; charset=utf-8") + .withBodyFile("../AppCredentials/files/body-mapping-githubapp-app.json"))); + githubApi.stubFor( + get(urlEqualTo("/app/installations")) + .willReturn( + aResponse() + .withHeader("Content-Type", "application/json; charset=utf-8") + .withBodyFile("../AppCredentials/files/body-mapping-githubapp-installations.json"))); + githubApi.stubFor( + post(urlEqualTo("/app/installations/654321/access_tokens")) + .withRequestBody( + equalToJson( + "{\"permissions\":{\"pull_requests\":\"write\",\"metadata\":\"read\",\"checks\":\"write\",\"contents\":\"read\"}}", true, false)) + .willReturn( + aResponse() + .withHeader("Content-Type", "application/json; charset=utf-8") + .withBodyFile("../AppCredentials/files/body-githubapp-create-installation-accesstokens.json"))); + } + + @Test + public void testAgentRefresh() throws Exception { + long notStaleSeconds = GitHubAppCredentials.AppInstallationToken.NOT_STALE_FOR_ATLEAST_SECONDS; + try { + appCredentials.setApiUri(githubApi.baseUrl()); + + // We want to demonstrate successful caching without waiting for a the default 1 minute + // Must set this to a large enough number to avoid flaky test + GitHubAppCredentials.AppInstallationToken.NOT_STALE_FOR_ATLEAST_SECONDS = 15; + + final String jenkinsfile = String.join( + "\n", + "// run checkout several times", + "node ('my-agent') {", + // First Checkout on agent should use cached token passed via remoting + " checkout scm", + // Multiple checkouts in quick succession should cached token + " checkout scm", + " sleep " + (GitHubAppCredentials.AppInstallationToken.NOT_STALE_FOR_ATLEAST_SECONDS + 2), + // Checkout after token expires refreshes via remoting + " checkout scm", + "}"); + + + sampleRepo.init(); + sampleRepo.write("Jenkinsfile", jenkinsfile); + sampleRepo.git("add", "Jenkinsfile"); + sampleRepo.git("commit", "--message=init"); + + WorkflowJob job = r.createProject(WorkflowJob.class, "repo-master"); + GitStep gitStep = new GitStep(sampleRepo.toString()); + gitStep.setCredentialsId(myAppCredentialsId); + gitStep.setPoll(false); + job.setDefinition(new CpsScmFlowDefinition(gitStep.createSCM(), "Jenkinsfile")); + WorkflowRun run = job.scheduleBuild2(0).waitForStart(); + r.waitForCompletion(run); + + // System.out.println(JenkinsRule.getLog(run)); + List credentialsLog = getOutputLines(); + + + //Verify correct messages from GitHubAppCredential logger indicating token was retrieved on agent + assertThat("Creds should cache on master, pass to agent, and refresh agent from master once", + credentialsLog, contains( + "Generating App Installation Token for app ID 54321", + "Generated App Installation Token for app ID 54321", + "Generating App Installation Token for app ID 54321 for agent", + "Generated App Installation Token for app ID 54321")); + } finally { + GitHubAppCredentials.AppInstallationToken.NOT_STALE_FOR_ATLEAST_SECONDS = notStaleSeconds; + } + } + + public static @Nonnull + WorkflowJob findBranchProject(@Nonnull WorkflowMultiBranchProject mp, @Nonnull String name) throws Exception { + WorkflowJob p = mp.getItem(name); + showIndexing(mp); + if (p == null) { + fail(name + " project not found"); + } + return p; + } + + private List getOutputLines() { + final Formatter formatter = handler.getFormatter(); + List result = new ArrayList<>(handler.getView()); + Collections.reverse(result); + return result.stream().map(formatter::formatMessage).collect(Collectors.toList()); + } + + static void showIndexing(@Nonnull WorkflowMultiBranchProject mp) throws Exception { + FolderComputation indexing = mp.getIndexing(); + System.out.println("---%<--- " + indexing.getUrl()); + indexing.writeWholeLogTo(System.out); + System.out.println("---%<--- "); + } + +} \ No newline at end of file diff --git a/src/test/java/org/jenkinsci/plugins/github_branch_source/JwtHelperTest.java b/src/test/java/org/jenkinsci/plugins/github_branch_source/JwtHelperTest.java index 6ede20073..9dacf2a57 100644 --- a/src/test/java/org/jenkinsci/plugins/github_branch_source/JwtHelperTest.java +++ b/src/test/java/org/jenkinsci/plugins/github_branch_source/JwtHelperTest.java @@ -23,7 +23,7 @@ public class JwtHelperTest { public ExpectedException expectedException = ExpectedException.none(); // https://stackoverflow.com/a/22176759/4951015 - private static final String PKCS8_PRIVATE_KEY = "-----BEGIN PRIVATE KEY-----\n" + + public static final String PKCS8_PRIVATE_KEY = "-----BEGIN PRIVATE KEY-----\n" + "MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQD7vHsVwyDV8cj7\n" + "5yR4WWl6rlgf/e5zmeBgtm0PCgnitcSbD5FU33301DPY5a7AtqVBOwEnE14L9XS7\n" + "ov61U+x1m4aQmqR/dPQaA2ayh2cYPszWNQMp42ArDIfg7DhSrvsRJKHsbPXlPjqe\n" + @@ -52,7 +52,7 @@ public class JwtHelperTest { "Nw9bewRvqjySBlDJ9/aNSeEY\n" + "-----END PRIVATE KEY-----"; - private static final String PKCS8_PUBLIC_KEY = "-----BEGIN PUBLIC KEY-----\n" + + public static final String PKCS8_PUBLIC_KEY = "-----BEGIN PUBLIC KEY-----\n" + "MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA+7x7FcMg1fHI++ckeFlp\n" + "eq5YH/3uc5ngYLZtDwoJ4rXEmw+RVN999NQz2OWuwLalQTsBJxNeC/V0u6L+tVPs\n" + "dZuGkJqkf3T0GgNmsodnGD7M1jUDKeNgKwyH4Ow4Uq77ESSh7Gz15T46nnNLnS6o\n" + diff --git a/src/test/resources/api/AppCredentials/files/body-githubapp-create-installation-accesstokens.json b/src/test/resources/api/AppCredentials/files/body-githubapp-create-installation-accesstokens.json new file mode 100644 index 000000000..cc37ff474 --- /dev/null +++ b/src/test/resources/api/AppCredentials/files/body-githubapp-create-installation-accesstokens.json @@ -0,0 +1,105 @@ +{ + "token": "super-secret-token", + "expires_at": "2019-08-10T05:54:58Z", + "permissions": { + "checks": "write", + "pull_requests": "write", + "contents": "read", + "metadata": "read" + }, + "repository_selection": "selected", + "repositories": [ + { + "id": 1010, + "node_id": "asdfasdf", + "name": "repo", + "full_name": "cloudbeers/repo", + "private": true, + "owner": { + "login": "cloudbeers", + "id": 4181899, + "node_id": "asdfasdfasdf", + "url": "https://api.github.com/orgs/cloudbeers", + "html_url": "https://github.com/cloudbeers", + "followers_url": "https://api.github.com/users/cloudbeers/followers", + "following_url": "https://api.github.com/users/cloudbeers/following{/other_user}", + "gists_url": "https://api.github.com/users/cloudbeers/gists{/gist_id}", + "starred_url": "https://api.github.com/users/cloudbeers/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/cloudbeers/subscriptions", + "organizations_url": "https://api.github.com/users/cloudbeers/orgs", + "repos_url": "https://api.github.com/users/cloudbeers/repos", + "events_url": "https://api.github.com/users/cloudbeers/events{/privacy}", + "received_events_url": "https://api.github.com/users/cloudbeers/received_events", + "type": "Organization", + "site_admin": false + }, + "html_url": "https://github.com/cloudbeers/repo", + "description": null, + "fork": false, + "url": "https://api.github.com/repos/cloudbeers/repo", + "forks_url": "https://api.github.com/repos/cloudbeers/repo/forks", + "keys_url": "https://api.github.com/repos/cloudbeers/repo/keys{/key_id}", + "collaborators_url": "https://api.github.com/repos/cloudbeers/repo/collaborators{/collaborator}", + "teams_url": "https://api.github.com/repos/cloudbeers/repo/teams", + "hooks_url": "https://api.github.com/repos/cloudbeers/repo/hooks", + "issue_events_url": "https://api.github.com/repos/cloudbeers/repo/issues/events{/number}", + "events_url": "https://api.github.com/repos/cloudbeers/repo/events", + "assignees_url": "https://api.github.com/repos/cloudbeers/repo/assignees{/user}", + "branches_url": "https://api.github.com/repos/cloudbeers/repo/branches{/branch}", + "tags_url": "https://api.github.com/repos/cloudbeers/repo/tags", + "blobs_url": "https://api.github.com/repos/cloudbeers/repo/git/blobs{/sha}", + "git_tags_url": "https://api.github.com/repos/cloudbeers/repo/git/tags{/sha}", + "git_refs_url": "https://api.github.com/repos/cloudbeers/repo/git/refs{/sha}", + "trees_url": "https://api.github.com/repos/cloudbeers/repo/git/trees{/sha}", + "statuses_url": "https://api.github.com/repos/cloudbeers/repo/statuses/{sha}", + "languages_url": "https://api.github.com/repos/cloudbeers/repo/languages", + "stargazers_url": "https://api.github.com/repos/cloudbeers/repo/stargazers", + "contributors_url": "https://api.github.com/repos/cloudbeers/repo/contributors", + "subscribers_url": "https://api.github.com/repos/cloudbeers/repo/subscribers", + "subscription_url": "https://api.github.com/repos/cloudbeers/repo/subscription", + "commits_url": "https://api.github.com/repos/cloudbeers/repo/commits{/sha}", + "git_commits_url": "https://api.github.com/repos/cloudbeers/repo/git/commits{/sha}", + "comments_url": "https://api.github.com/repos/cloudbeers/repo/comments{/number}", + "issue_comment_url": "https://api.github.com/repos/cloudbeers/repo/issues/comments{/number}", + "contents_url": "https://api.github.com/repos/cloudbeers/repo/contents/{+path}", + "compare_url": "https://api.github.com/repos/cloudbeers/repo/compare/{base}...{head}", + "merges_url": "https://api.github.com/repos/cloudbeers/repo/merges", + "archive_url": "https://api.github.com/repos/cloudbeers/repo/{archive_format}{/ref}", + "downloads_url": "https://api.github.com/repos/cloudbeers/repo/downloads", + "issues_url": "https://api.github.com/repos/cloudbeers/repo/issues{/number}", + "pulls_url": "https://api.github.com/repos/cloudbeers/repo/pulls{/number}", + "milestones_url": "https://api.github.com/repos/cloudbeers/repo/milestones{/number}", + "notifications_url": "https://api.github.com/repos/cloudbeers/repo/notifications{?since,all,participating}", + "labels_url": "https://api.github.com/repos/cloudbeers/repo/labels{/name}", + "releases_url": "https://api.github.com/repos/cloudbeers/repo/releases{/id}", + "deployments_url": "https://api.github.com/repos/cloudbeers/repo/deployments", + "created_at": "2018-09-06T03:25:38Z", + "updated_at": "2018-09-30T22:04:06Z", + "pushed_at": "2019-08-08T22:22:34Z", + "git_url": "git://github.com/cloudbeers/repo.git", + "ssh_url": "git@github.com:cloudbeers/repo.git", + "clone_url": "https://github.com/cloudbeers/repo.git", + "svn_url": "https://github.com/cloudbeers/repo", + "homepage": null, + "size": 618, + "stargazers_count": 0, + "watchers_count": 0, + "language": "Java", + "has_issues": true, + "has_projects": true, + "has_downloads": true, + "has_wiki": true, + "has_pages": false, + "forks_count": 0, + "mirror_url": null, + "archived": false, + "disabled": false, + "open_issues_count": 5, + "license": null, + "forks": 0, + "open_issues": 5, + "watchers": 0, + "default_branch": "main" + } + ] +} \ No newline at end of file diff --git a/src/test/resources/api/AppCredentials/files/body-mapping-githubapp-app.json b/src/test/resources/api/AppCredentials/files/body-mapping-githubapp-app.json new file mode 100644 index 000000000..eb2aaee6e --- /dev/null +++ b/src/test/resources/api/AppCredentials/files/body-mapping-githubapp-app.json @@ -0,0 +1,39 @@ +{ + "id": 54321, + "node_id": "MDM6QXBwMzI2MTY=", + "owner": { + "login": "cloudbeers", + "id": 4181899, + "node_id": "asdfasdfasdf", + "url": "https://api.github.com/orgs/cloudbeers", + "html_url": "https://github.com/cloudbeers", + "followers_url": "https://api.github.com/users/cloudbeers/followers", + "following_url": "https://api.github.com/users/cloudbeers/following{/other_user}", + "gists_url": "https://api.github.com/users/cloudbeers/gists{/gist_id}", + "starred_url": "https://api.github.com/users/cloudbeers/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/cloudbeers/subscriptions", + "organizations_url": "https://api.github.com/users/cloudbeers/orgs", + "repos_url": "https://api.github.com/users/cloudbeers/repos", + "events_url": "https://api.github.com/users/cloudbeers/events{/privacy}", + "received_events_url": "https://api.github.com/users/cloudbeers/received_events", + "type": "Organization", + "site_admin": false + }, + "name": "Bogus-Development", + "description": "", + "external_url": "https://bogus.domain.com", + "html_url": "https://github.com/apps/bogus-development", + "created_at": "2019-06-10T04:21:41Z", + "updated_at": "2019-06-10T04:21:41Z", + "permissions": { + "checks": "write", + "contents": "read", + "metadata": "read", + "pull_requests": "write" + }, + "events": [ + "pull_request", + "push" + ], + "installations_count": 1 +} \ No newline at end of file diff --git a/src/test/resources/api/AppCredentials/files/body-mapping-githubapp-installations.json b/src/test/resources/api/AppCredentials/files/body-mapping-githubapp-installations.json new file mode 100644 index 000000000..7a463e643 --- /dev/null +++ b/src/test/resources/api/AppCredentials/files/body-mapping-githubapp-installations.json @@ -0,0 +1,84 @@ +[ + { + "id": 654321, + "account": { + "login": "cloudbeers", + "id": 4181899, + "node_id": "asdfasdfasdf", + "url": "https://api.github.com/orgs/cloudbeers", + "html_url": "https://github.com/cloudbeers", + "followers_url": "https://api.github.com/users/cloudbeers/followers", + "following_url": "https://api.github.com/users/cloudbeers/following{/other_user}", + "gists_url": "https://api.github.com/users/cloudbeers/gists{/gist_id}", + "starred_url": "https://api.github.com/users/cloudbeers/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/cloudbeers/subscriptions", + "organizations_url": "https://api.github.com/users/cloudbeers/orgs", + "repos_url": "https://api.github.com/users/cloudbeers/repos", + "events_url": "https://api.github.com/users/cloudbeers/events{/privacy}", + "received_events_url": "https://api.github.com/users/cloudbeers/received_events", + "type": "Organization", + "site_admin": false + }, + "repository_selection": "selected", + "access_tokens_url": "https://api.github.com/app/installations/654321/access_tokens", + "repositories_url": "https://api.github.com/installation/repositories", + "html_url": "https://github.com/organizations/cloudbeers/settings/installations/654321", + "app_id": 54321, + "target_id": 4181899, + "target_type": "Organization", + "permissions": { + "checks": "write", + "pull_requests": "write", + "contents": "read", + "metadata": "read" + }, + "events": [ + "pull_request", + "push" + ], + "created_at": "2019-07-04T01:19:36.000Z", + "updated_at": "2019-07-30T22:48:09.000Z", + "single_file_name": null + }, + { + "id": 754321, + "account": { + "login": "bogus", + "id": 0, + "node_id": "asdfasdfasdff", + "url": "https://api.github.com/orgs/bogus", + "html_url": "https://github.com/bogus", + "followers_url": "https://api.github.com/users/bogus/followers", + "following_url": "https://api.github.com/users/bogus/following{/other_user}", + "gists_url": "https://api.github.com/users/bogus/gists{/gist_id}", + "starred_url": "https://api.github.com/users/bogus/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/bogus/subscriptions", + "organizations_url": "https://api.github.com/users/bogus/orgs", + "repos_url": "https://api.github.com/users/v/repos", + "events_url": "https://api.github.com/users/bogus/events{/privacy}", + "received_events_url": "https://api.github.com/users/bogus/received_events", + "type": "Organization", + "site_admin": false + }, + "repository_selection": "selected", + "access_tokens_url": "https://api.github.com/app/installations/754321/access_tokens", + "repositories_url": "https://api.github.com/installation/repositories", + "html_url": "https://github.com/organizations/bogus/settings/installations/754321", + "app_id": 54321, + "target_id": 4181899, + "target_type": "Organization", + "permissions": { + "checks": "write", + "pull_requests": "write", + "contents": "read", + "metadata": "read" + }, + "events": [ + "pull_request", + "push" + ], + "created_at": "2019-07-04T01:19:36.000Z", + "updated_at": "2019-07-30T22:48:09.000Z", + "single_file_name": null + } +] \ No newline at end of file From dc3b565575f7e142ab03d1d40d821c6bd2c12100 Mon Sep 17 00:00:00 2001 From: Liam Newman Date: Sun, 30 Aug 2020 02:47:48 -0700 Subject: [PATCH 14/18] Addtional testing revealed poor behavior --- .../GithubAppCredentialsTest.java | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) 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 df584a6fe..0eb023140 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 @@ -122,6 +122,15 @@ public void testAgentRefresh() throws Exception { " sleep " + (GitHubAppCredentials.AppInstallationToken.NOT_STALE_FOR_ATLEAST_SECONDS + 2), // Checkout after token expires refreshes via remoting " checkout scm", + // ISSUE: This should not refresh the agent credentials again, but it does. + // The previous call refreshes credentials is on the agent, + // but each step gets a new instance from the controller. + // Refreshing the token on the agent does not refresh the token held on the controller. + // We could mitigate this by checking staleness during writeReplace(), but that causes refresh from agent + // to be skipped (rarely stale on agent) and also seems like it could cause instability. + // What we really want is for the controller's cached token to be updated when the agent requests a new token. + // Multiple checkouts in quick succession should use newly cached token + " checkout scm", "}"); @@ -145,10 +154,14 @@ public void testAgentRefresh() throws Exception { //Verify correct messages from GitHubAppCredential logger indicating token was retrieved on agent assertThat("Creds should cache on master, pass to agent, and refresh agent from master once", credentialsLog, contains( - "Generating App Installation Token for app ID 54321", - "Generated App Installation Token for app ID 54321", - "Generating App Installation Token for app ID 54321 for agent", - "Generated App Installation Token for app ID 54321")); + "Generating App Installation Token for app ID 54321", + "Generated App Installation Token for app ID 54321", + "Generating App Installation Token for app ID 54321 for agent", + "Generated App Installation Token for app ID 54321", + // ISSUE: as noted above, this is not terrible but it should not be happening + "Generating App Installation Token for app ID 54321 for agent", + "Generated App Installation Token for app ID 54321" + )); } finally { GitHubAppCredentials.AppInstallationToken.NOT_STALE_FOR_ATLEAST_SECONDS = notStaleSeconds; } From 52526b4ebc30113c81d692699f6270d6dd9a4fc3 Mon Sep 17 00:00:00 2001 From: Liam Newman Date: Mon, 31 Aug 2020 16:30:31 -0700 Subject: [PATCH 15/18] Check validity of app token before sending to agents --- pom.xml | 21 +- .../GitHubAppCredentials.java | 162 +++++++++++---- ...ppCredentialsAppInstallationTokenTest.java | 16 +- .../GithubAppCredentialsTest.java | 192 ++++++++++++------ 4 files changed, 265 insertions(+), 126 deletions(-) diff --git a/pom.xml b/pom.xml index 1fca462bb..dc849b73f 100644 --- a/pom.xml +++ b/pom.xml @@ -52,6 +52,10 @@ github 1.31.0 + + org.jenkins-ci.plugins.workflow + workflow-support + io.jsonwebtoken jjwt-api @@ -92,25 +96,10 @@ mockito-core test - - org.jenkins-ci.plugins.workflow - workflow-durable-task-step - - - org.jenkins-ci.plugins - durable-task - - - org.jenkins-ci.plugins - credentials - org.jenkins-ci.plugins.workflow workflow-basic-steps - - - org.jenkins-ci.plugins - credentials-binding + test org.jenkins-ci.plugins 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 1c4f7c8d1..b7325cb9d 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 @@ -1,7 +1,6 @@ package org.jenkinsci.plugins.github_branch_source; import com.cloudbees.plugins.credentials.CredentialsScope; -import com.cloudbees.plugins.credentials.CredentialsSnapshotTaker; import com.cloudbees.plugins.credentials.common.StandardUsernamePasswordCredentials; import com.cloudbees.plugins.credentials.impl.BaseStandardCredentials; import edu.umd.cs.findbugs.annotations.CheckForNull; @@ -18,11 +17,13 @@ import java.time.Duration; import java.time.Instant; import java.util.List; +import java.util.concurrent.TimeUnit; import java.util.logging.Level; import java.util.logging.Logger; import jenkins.security.SlaveToMasterCallable; import jenkins.util.JenkinsJVM; +import org.jenkinsci.plugins.workflow.support.concurrent.Timeout; import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.restrictions.NoExternalUse; import org.kohsuke.github.GHApp; @@ -112,7 +113,8 @@ 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 { + // We expect this to be fast but if anything hangs in here we do not want to block indefinitely + try (Timeout timeout = Timeout.limit(30, TimeUnit.SECONDS)) { String jwtToken = createJWT(appId, appPrivateKey); GitHub gitHubApp = Connector .createGitHubBuilder(apiUrl) @@ -130,23 +132,30 @@ static AppInstallationToken generateAppInstallationToken(String appId, String ap appInstallation = appInstallations.get(0); } else { appInstallation = appInstallations.stream() - .filter(installation -> installation.getAccount().getLogin().equals(owner)) - .findAny() - .orElseThrow(() -> new IllegalArgumentException(String.format(ERROR_NOT_INSTALLED, appId))); + .filter(installation -> installation.getAccount().getLogin().equals(owner)) + .findAny() + .orElseThrow(() -> new IllegalArgumentException(String.format( + ERROR_NOT_INSTALLED, + appId))); } GHAppInstallationToken appInstallationToken = appInstallation - .createToken(appInstallation.getPermissions()) - .create(); + .createToken(appInstallation.getPermissions()) + .create(); long expiration = getExpirationSeconds(appInstallationToken); - LOGGER.log(Level.FINEST, "Token raw expiration epoch seconds: {0}", expiration); + LOGGER.log(Level.FINEST, + "Token raw expiration epoch seconds: {0,number,#}", + expiration); - AppInstallationToken token = new AppInstallationToken(appInstallationToken.getToken(), expiration); - LOGGER.log(Level.FINE, "Generated App Installation Token for app ID {0}", appId); + 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) { + } catch (Exception 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); } @@ -162,7 +171,7 @@ private static long getExpirationSeconds(GHAppInstallationToken appInstallationT LOGGER.log(Level.WARNING, "Unable to get GitHub App installation token expiration", e); - return Instant.now().getEpochSecond() + AppInstallationToken.STALE_AFTER_SECONDS; + return Instant.now().getEpochSecond() + AppInstallationToken.NOT_STALE_MINIMUM_SECONDS; } } @@ -178,13 +187,30 @@ private static long getExpirationSeconds(GHAppInstallationToken appInstallationT public Secret getPassword() { String appInstallationToken; synchronized (this) { - 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); + try { + 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); + LOGGER.log(Level.FINER, "Retrieved GitHub App Installation Token for app ID {0}", appID); + } + } catch (Exception e) { + if (cachedToken != null && !cachedToken.isExpired()) { + // Requesting a new token failed. If the cached token is not expired, continue to use it. + // This minimizes failures due to occasional network instability, + // while only slightly increasing the chance that tokens will expire while in use. + LOGGER.log(Level.WARNING, + "Keeping cached GitHub App Installation Token for app ID {0}: token is stale but has not expired", appID); + } else { + throw e; + } } appInstallationToken = cachedToken.getToken(); } - LOGGER.log(Level.FINER, "Returned GitHub App Installation Token for app ID {0}", appID); + + LOGGER.log(Level.FINEST, "Returned GitHub App Installation Token for app ID {0}", appID); return Secret.fromString(appInstallationToken); } @@ -198,19 +224,26 @@ public String getUsername() { return appID; } + private AppInstallationToken getCachedToken() { + synchronized (this) { + return cachedToken; + } + } + 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. + * The token generally 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. + * it is returned from GitHub is less than this or if the token is kept and due to failures + * while retrieving a new token. */ - static final long STALE_WHEN_SECONDS_UNTIL_EXPIRATION = Duration.ofMinutes(45).getSeconds(); + static final long STALE_BEFORE_EXPIRATION_SECONDS = Duration.ofMinutes(45).getSeconds(); /** * Any token older than this is considered stale. @@ -225,40 +258,51 @@ static class AppInstallationToken implements Serializable { * * Prevents continuous refreshing of credentials. * Non-final for testing purposes. - * This value takes precedence over {@link #STALE_WHEN_SECONDS_UNTIL_EXPIRATION}. - * {@link #STALE_WHEN_SECONDS_UNTIL_EXPIRATION} takes precedence over this value. + * This value takes precedence over {@link #STALE_BEFORE_EXPIRATION_SECONDS}. + * {@link #STALE_BEFORE_EXPIRATION_SECONDS} takes precedence over this value. * Minimum value of 1. */ - static long NOT_STALE_FOR_ATLEAST_SECONDS = Duration.ofMinutes(1).getSeconds(); + static long NOT_STALE_MINIMUM_SECONDS = Duration.ofMinutes(1).getSeconds(); private final String token; - private final long tokenStaleEpochSeconds; + private final long expirationEpochSeconds; + private final long staleEpochSeconds; /** * Create a AppInstallationToken instance. * + * Tokens will always become stale after {@link #STALE_AFTER_SECONDS} seconds. + * Tokens will not become stale for at least {@link #NOT_STALE_MINIMUM_SECONDS}, + * as long as that does not exceed {@link #STALE_AFTER_SECONDS}. + * Within the bounds of {@link #NOT_STALE_MINIMUM_SECONDS} and {@link #STALE_AFTER_SECONDS}, + * tokens will become stale {@link #STALE_BEFORE_EXPIRATION_SECONDS} seconds before they expire. + * * @param token the token string * @param expirationEpochSeconds the time in epoch seconds that this token will expire */ public AppInstallationToken(String token, long expirationEpochSeconds) { long now = Instant.now().getEpochSecond(); - long minimumAge = now + Math.max(1, NOT_STALE_FOR_ATLEAST_SECONDS); + long minimumAllowedAge = Math.max(1, NOT_STALE_MINIMUM_SECONDS); + long maximumAllowedAge = Math.max(1, 1 + STALE_AFTER_SECONDS); // Tokens go stale a while before they will expire - long staleEpochSeconds = expirationEpochSeconds - STALE_WHEN_SECONDS_UNTIL_EXPIRATION; + long secondsUntilStale = (expirationEpochSeconds - now) - STALE_BEFORE_EXPIRATION_SECONDS; - // Tokens are not stale as soon as they are made - if (staleEpochSeconds < minimumAge) { - staleEpochSeconds = minimumAge; + // Tokens are never stale as soon as they are made + if (secondsUntilStale < minimumAllowedAge) { + secondsUntilStale = minimumAllowedAge; } // Tokens have a maximum age at which they go stale - staleEpochSeconds = Math.min(staleEpochSeconds, now + 1 + STALE_AFTER_SECONDS); + if (secondsUntilStale > maximumAllowedAge) { + secondsUntilStale = maximumAllowedAge; + } - LOGGER.log(Level.FINER, "Token stale time epoch seconds: {0}", staleEpochSeconds); + LOGGER.log(Level.FINER, "Token will become stale after {0,number,#} seconds", secondsUntilStale); this.token = token; - this.tokenStaleEpochSeconds = staleEpochSeconds; + this.expirationEpochSeconds = expirationEpochSeconds; + this.staleEpochSeconds = now + secondsUntilStale; } public String getToken() { @@ -270,18 +314,21 @@ public String getToken() { * * {@link #getPassword()} checks that the token is not "stale" before returning it. * If a token is "stale" if it has expired, exceeded {@link #STALE_AFTER_SECONDS}, or - * will expire in less than {@link #STALE_WHEN_SECONDS_UNTIL_EXPIRATION}. + * will expire in less than {@link #STALE_BEFORE_EXPIRATION_SECONDS}. * * @return {@code true} if token should be refreshed, otherwise {@code false}. */ public boolean isStale() { - return Instant.now().getEpochSecond() >= tokenStaleEpochSeconds; + return Instant.now().getEpochSecond() >= staleEpochSeconds; } - long getTokenStaleEpochSeconds() { - return tokenStaleEpochSeconds; + public boolean isExpired() { + return Instant.now().getEpochSecond() >= expirationEpochSeconds; } + long getTokenStaleEpochSeconds() { + return staleEpochSeconds; + } } /** @@ -293,7 +340,6 @@ long getTokenStaleEpochSeconds() { *
  • We avoid the considerable amount of class loading associated with the JWT library, Jackson data binding, Bouncy Castle, etc. *
  • The agent need not be able to contact GitHub. * - * @see CredentialsSnapshotTaker */ private Object writeReplace() { if (/* XStream */Channel.current() == null) { @@ -321,9 +367,20 @@ private static final class DelegatingGitHubAppCredentials extends BaseStandardCr 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; + + // Check token is valid before sending it to the agent. + // Ensuring the cached token is not stale before sending it to agents keeps agents from having to + // immediately refresh the token. + // This is intentionally only a best-effort attempt. + // If this fails, the agent will fallback to making the request (which may or may not fail). + try { + LOGGER.log(Level.FINEST, "Checking App Installation Token for app ID {0} before sending to agent", onMaster.appID); + onMaster.getPassword(); + } catch (Exception e) { + LOGGER.log(Level.WARNING, "Failed to refresh stale GitHub App installation token before sending to agent for app ID " + onMaster.getAppID(), e); } + + cachedToken = onMaster.getCachedToken(); } private Object readResolve() { @@ -346,12 +403,29 @@ public Secret getPassword() { try { String appInstallationToken; synchronized (this) { - if (cachedToken == null || cachedToken.isStale()) { - cachedToken = ch.call(new GetToken(tokenRefreshData)); + try { + if (cachedToken == null || cachedToken.isStale()) { + cachedToken = ch.call(new GetToken(tokenRefreshData)); + LOGGER.log(Level.INFO, + "Retrieved GitHub App Installation Token for app ID {0} on agent", + appID); + } + } catch (Exception e) { + if (cachedToken != null && !cachedToken.isExpired()) { + // Requesting a new token failed. If the cached token is not expired, continue to use it. + // This minimizes failures due to occasional network instability, + // while only slightly increasing the chance that tokens will expire while in use. + LOGGER.log(Level.WARNING, + "Keeping cached GitHub App Installation Token for app ID {0} on agent: token is stale but has not expired", + appID); + } else { + throw e; + } } appInstallationToken = cachedToken.getToken(); } - LOGGER.log(Level.FINER, "Returned GitHub App Installation Token for app ID {0} on agent", appID); + + LOGGER.log(Level.FINEST, "Returned GitHub App Installation Token for app ID {0} on agent", appID); return Secret.fromString(appInstallationToken); } catch (IOException | InterruptedException x) { @@ -373,10 +447,14 @@ 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], + AppInstallationToken token = generateAppInstallationToken(fields[0], fields[1], fields[2], fields[3]); + LOGGER.log(Level.FINER, + "Retrieved GitHub App Installation Token for app ID {0} for agent", + fields[0]); + return token; } } } diff --git a/src/test/java/org/jenkinsci/plugins/github_branch_source/GithubAppCredentialsAppInstallationTokenTest.java b/src/test/java/org/jenkinsci/plugins/github_branch_source/GithubAppCredentialsAppInstallationTokenTest.java index 534299b35..2c36704e0 100644 --- a/src/test/java/org/jenkinsci/plugins/github_branch_source/GithubAppCredentialsAppInstallationTokenTest.java +++ b/src/test/java/org/jenkinsci/plugins/github_branch_source/GithubAppCredentialsAppInstallationTokenTest.java @@ -19,23 +19,23 @@ public void testAppInstallationTokenStale() throws Exception { now = Instant.now().getEpochSecond(); token = new GitHubAppCredentials.AppInstallationToken("", now); assertThat(token.isStale(), is(false)); - assertThat(token.getTokenStaleEpochSeconds(), equalTo(now + GitHubAppCredentials.AppInstallationToken.NOT_STALE_FOR_ATLEAST_SECONDS)); + assertThat(token.getTokenStaleEpochSeconds(), equalTo(now + GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS)); now = Instant.now().getEpochSecond(); token = new GitHubAppCredentials.AppInstallationToken("", now + Duration.ofMinutes(15).getSeconds()); assertThat(token.isStale(), is(false)); - assertThat(token.getTokenStaleEpochSeconds(), equalTo(now + GitHubAppCredentials.AppInstallationToken.NOT_STALE_FOR_ATLEAST_SECONDS)); + assertThat(token.getTokenStaleEpochSeconds(), equalTo(now + GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS)); now = Instant.now().getEpochSecond(); token = new GitHubAppCredentials.AppInstallationToken("", - now + GitHubAppCredentials.AppInstallationToken.STALE_WHEN_SECONDS_UNTIL_EXPIRATION + 2); + now + GitHubAppCredentials.AppInstallationToken.STALE_BEFORE_EXPIRATION_SECONDS + 2); assertThat(token.isStale(), is(false)); - assertThat(token.getTokenStaleEpochSeconds(), equalTo(now + GitHubAppCredentials.AppInstallationToken.NOT_STALE_FOR_ATLEAST_SECONDS)); + assertThat(token.getTokenStaleEpochSeconds(), equalTo(now + GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS)); now = Instant.now().getEpochSecond(); token = new GitHubAppCredentials.AppInstallationToken("", - now + GitHubAppCredentials.AppInstallationToken.STALE_WHEN_SECONDS_UNTIL_EXPIRATION + Duration + now + GitHubAppCredentials.AppInstallationToken.STALE_BEFORE_EXPIRATION_SECONDS + Duration .ofMinutes(7) .getSeconds()); assertThat(token.isStale(), is(false)); @@ -49,10 +49,10 @@ public void testAppInstallationTokenStale() throws Exception { assertThat(token.getTokenStaleEpochSeconds(), equalTo(now + GitHubAppCredentials.AppInstallationToken.STALE_AFTER_SECONDS + 1)); - long notStaleSeconds = GitHubAppCredentials.AppInstallationToken.NOT_STALE_FOR_ATLEAST_SECONDS; + long notStaleSeconds = GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS; try { // Should revert to 1 second minimum - GitHubAppCredentials.AppInstallationToken.NOT_STALE_FOR_ATLEAST_SECONDS = -10; + GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS = -10; now = Instant.now().getEpochSecond(); token = new GitHubAppCredentials.AppInstallationToken("", now); @@ -63,7 +63,7 @@ public void testAppInstallationTokenStale() throws Exception { Thread.sleep(1000); assertThat(token.isStale(), is(true)); } finally { - GitHubAppCredentials.AppInstallationToken.NOT_STALE_FOR_ATLEAST_SECONDS = notStaleSeconds; + GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS = notStaleSeconds; } } } \ No newline at end of file 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 0eb023140..71e029f2e 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 @@ -1,53 +1,48 @@ package org.jenkinsci.plugins.github_branch_source; -import com.cloudbees.hudson.plugins.folder.computed.FolderComputation; import com.cloudbees.plugins.credentials.CredentialsProvider; import com.cloudbees.plugins.credentials.CredentialsScope; import com.cloudbees.plugins.credentials.CredentialsStore; import com.cloudbees.plugins.credentials.domains.Domain; +import hudson.logging.LogRecorder; +import hudson.logging.LogRecorderManager; import hudson.model.Slave; -import hudson.util.LogTaskListener; -import hudson.util.RingBufferLogHandler; import hudson.util.Secret; -import jenkins.branch.BranchSource; -import jenkins.plugins.git.GitSCMSource; import jenkins.plugins.git.GitSampleRepoRule; import jenkins.plugins.git.GitStep; import org.jenkinsci.plugins.workflow.cps.CpsScmFlowDefinition; import org.jenkinsci.plugins.workflow.job.WorkflowJob; import org.jenkinsci.plugins.workflow.job.WorkflowRun; -import org.jenkinsci.plugins.workflow.multibranch.WorkflowMultiBranchProject; import org.junit.Before; import org.junit.BeforeClass; import org.junit.Rule; import org.junit.Test; import org.jvnet.hudson.test.JenkinsRule; -import javax.annotation.Nonnull; +import java.time.Duration; +import java.time.Instant; +import java.time.format.DateTimeFormatter; +import java.time.temporal.ChronoUnit; import java.util.ArrayList; -import java.util.Collections; +import java.util.Comparator; +import java.util.Date; import java.util.List; import java.util.logging.Formatter; import java.util.logging.Level; import java.util.logging.LogRecord; -import java.util.logging.Logger; import java.util.logging.SimpleFormatter; import java.util.stream.Collectors; -import java.util.stream.Stream; -import static org.hamcrest.MatcherAssert.assertThat; import static com.github.tomakehurst.wiremock.client.WireMock.*; import static org.hamcrest.Matchers.*; public class GithubAppCredentialsTest extends AbstractGitHubWireMockTest { - private RingBufferLogHandler handler; - private static Slave agent; - private WorkflowMultiBranchProject owner; private static final String myAppCredentialsId = "myAppCredentialsId"; private static CredentialsStore store; private static GitHubAppCredentials appCredentials; + private static LogRecorder logRecorder; @Rule public GitSampleRepoRule sampleRepo = new GitSampleRepoRule(); @@ -61,20 +56,19 @@ public static void setUpJenkins() throws Exception { appCredentials.setOwner("cloudbeers"); store.addCredentials(Domain.global(), appCredentials); + LogRecorderManager mgr = r.jenkins.getLog(); + logRecorder = new LogRecorder(GitHubAppCredentials.class.getName()); + mgr.logRecorders.put(GitHubAppCredentials.class.getName(), logRecorder); + LogRecorder.Target t = new LogRecorder.Target(GitHubAppCredentials.class.getName(), Level.FINER); + logRecorder.targets.add(t); + logRecorder.save(); + t.enable(); + // Add agent agent = r.createOnlineSlave(); agent.setLabelString("my-agent"); } - @Before - public void setUpLogging() throws Exception { - handler = new RingBufferLogHandler(1000); - handler.setFormatter(new SimpleFormatter()); - final Logger logger = Logger.getLogger(GitHubAppCredentials.class.getName()); - logger.setLevel(Level.FINE); - logger.addHandler(handler); - } - @Before public void setUpWireMock() throws Exception { //Add wiremock responses for App, App Installation, and App Installation Token @@ -90,8 +84,79 @@ public void setUpWireMock() throws Exception { aResponse() .withHeader("Content-Type", "application/json; charset=utf-8") .withBodyFile("../AppCredentials/files/body-mapping-githubapp-installations.json"))); + + final String scenarioName = "credentials-accesstoken"; + githubApi.stubFor( post(urlEqualTo("/app/installations/654321/access_tokens")) + .inScenario(scenarioName) + .whenScenarioStateIs("Started") + .willSetStateTo("1") + .withRequestBody( + equalToJson( + "{\"permissions\":{\"pull_requests\":\"write\",\"metadata\":\"read\",\"checks\":\"write\",\"contents\":\"read\"}}", true, false)) + .willReturn( + aResponse() + .withHeader("Content-Type", "application/json; charset=utf-8") + .withBody("{\n" + + " \"token\": \"super-secret-token\",\n" + + // This token will go stale at the soonest allowed time but will no be expired for the duration of the test + " \"expires_at\": \"" + printDate(new Date(System.currentTimeMillis() + Duration.ofMinutes(10).toMillis())) + "\"" + // 2019-08-10T05:54:58Z + "}"))); + + // Force an error to test fallback refreshing from agent + githubApi.stubFor( + post(urlEqualTo("/app/installations/654321/access_tokens")) + .inScenario(scenarioName) + .whenScenarioStateIs("1") + .willSetStateTo("2") + .withRequestBody( + equalToJson( + "{\"permissions\":{\"pull_requests\":\"write\",\"metadata\":\"read\",\"checks\":\"write\",\"contents\":\"read\"}}", true, false)) + .willReturn( + aResponse() + .withStatus(404) + .withStatusMessage("404 Not Found") + .withHeader("Content-Type", "application/json; charset=utf-8") + .withBody("{\"message\": \"File not found\"}"))); + + // Force an error to test fallback to returning unexpired token + githubApi.stubFor( + post(urlEqualTo("/app/installations/654321/access_tokens")) + .inScenario(scenarioName) + .whenScenarioStateIs("1") + .willSetStateTo("2") + .withRequestBody( + equalToJson( + "{\"permissions\":{\"pull_requests\":\"write\",\"metadata\":\"read\",\"checks\":\"write\",\"contents\":\"read\"}}", true, false)) + .willReturn( + aResponse() + .withStatus(404) + .withStatusMessage("404 Not Found") + .withHeader("Content-Type", "application/json; charset=utf-8") + .withBody("{\"message\": \"File not found\"}"))); + + // Force an error to test fallback refreshing from agent + githubApi.stubFor( + post(urlEqualTo("/app/installations/654321/access_tokens")) + .inScenario(scenarioName) + .whenScenarioStateIs("2") + .willSetStateTo("3") + .withRequestBody( + equalToJson( + "{\"permissions\":{\"pull_requests\":\"write\",\"metadata\":\"read\",\"checks\":\"write\",\"contents\":\"read\"}}", true, false)) + .willReturn( + aResponse() + .withStatus(404) + .withStatusMessage("404 Not Found") + .withHeader("Content-Type", "application/json; charset=utf-8") + .withBody("{\"message\": \"File not found\"}"))); + + githubApi.stubFor( + post(urlEqualTo("/app/installations/654321/access_tokens")) + .inScenario(scenarioName) + .whenScenarioStateIs("3") + .willSetStateTo("4") .withRequestBody( equalToJson( "{\"permissions\":{\"pull_requests\":\"write\",\"metadata\":\"read\",\"checks\":\"write\",\"contents\":\"read\"}}", true, false)) @@ -99,17 +164,18 @@ public void setUpWireMock() throws Exception { aResponse() .withHeader("Content-Type", "application/json; charset=utf-8") .withBodyFile("../AppCredentials/files/body-githubapp-create-installation-accesstokens.json"))); + } @Test public void testAgentRefresh() throws Exception { - long notStaleSeconds = GitHubAppCredentials.AppInstallationToken.NOT_STALE_FOR_ATLEAST_SECONDS; + long notStaleSeconds = GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS; try { appCredentials.setApiUri(githubApi.baseUrl()); // We want to demonstrate successful caching without waiting for a the default 1 minute // Must set this to a large enough number to avoid flaky test - GitHubAppCredentials.AppInstallationToken.NOT_STALE_FOR_ATLEAST_SECONDS = 15; + GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS = 15; final String jenkinsfile = String.join( "\n", @@ -119,17 +185,12 @@ public void testAgentRefresh() throws Exception { " checkout scm", // Multiple checkouts in quick succession should cached token " checkout scm", - " sleep " + (GitHubAppCredentials.AppInstallationToken.NOT_STALE_FOR_ATLEAST_SECONDS + 2), - // Checkout after token expires refreshes via remoting + " sleep " + (GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS + 2), + // Checkout after token expires refreshes via remoting - error on controller is not catastrophic " checkout scm", - // ISSUE: This should not refresh the agent credentials again, but it does. - // The previous call refreshes credentials is on the agent, - // but each step gets a new instance from the controller. - // Refreshing the token on the agent does not refresh the token held on the controller. - // We could mitigate this by checking staleness during writeReplace(), but that causes refresh from agent - // to be skipped (rarely stale on agent) and also seems like it could cause instability. - // What we really want is for the controller's cached token to be updated when the agent requests a new token. - // Multiple checkouts in quick succession should use newly cached token + // Checkout after error will refresh again on controller + " checkout scm", + // Multiple checkouts in quick succession should use cached token " checkout scm", "}"); @@ -147,48 +208,59 @@ public void testAgentRefresh() throws Exception { WorkflowRun run = job.scheduleBuild2(0).waitForStart(); r.waitForCompletion(run); - // System.out.println(JenkinsRule.getLog(run)); - List credentialsLog = getOutputLines(); + System.out.println(JenkinsRule.getLog(run)); + List credentialsLog = getOutputLines(); //Verify correct messages from GitHubAppCredential logger indicating token was retrieved on agent assertThat("Creds should cache on master, pass to agent, and refresh agent from master once", credentialsLog, contains( + // node ('my-agent') { "Generating App Installation Token for app ID 54321", + "Token will become stale after 15 seconds", "Generated App Installation Token for app ID 54321", + "Retrieved GitHub App Installation Token for app ID 54321", + // checkout scm + // checkout scm + // sleep + // (^^^ No token generation for these three steps) + // checkout scm + "Generating App Installation Token for app ID 54321", + // (error forced by wiremock) + "Failed to retrieve GitHub App installation token for app ID 54321", + "Keeping cached GitHub App Installation Token for app ID 54321: token is stale but has not expired", + // (error forced by wiremock - failed refresh on the agent) "Generating App Installation Token for app ID 54321 for agent", + "Failed to retrieve GitHub App installation token for app ID 54321", + "Keeping cached GitHub App Installation Token for app ID 54321 on agent: token is stale but has not expired", + // checkout scm - refresh on controller + "Generating App Installation Token for app ID 54321", + "Token will become stale after 15 seconds", "Generated App Installation Token for app ID 54321", - // ISSUE: as noted above, this is not terrible but it should not be happening - "Generating App Installation Token for app ID 54321 for agent", - "Generated App Installation Token for app ID 54321" + "Retrieved GitHub App Installation Token for app ID 54321" + // checkout scm + // (No token generation) )); } finally { - GitHubAppCredentials.AppInstallationToken.NOT_STALE_FOR_ATLEAST_SECONDS = notStaleSeconds; - } - } - - public static @Nonnull - WorkflowJob findBranchProject(@Nonnull WorkflowMultiBranchProject mp, @Nonnull String name) throws Exception { - WorkflowJob p = mp.getItem(name); - showIndexing(mp); - if (p == null) { - fail(name + " project not found"); + GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS = notStaleSeconds; + logRecorder.doClear(); } - return p; } private List getOutputLines() { - final Formatter formatter = handler.getFormatter(); - List result = new ArrayList<>(handler.getView()); - Collections.reverse(result); - return result.stream().map(formatter::formatMessage).collect(Collectors.toList()); + final Formatter formatter = new SimpleFormatter(); + List result = new ArrayList<>(logRecorder.getLogRecords()); + result.addAll(logRecorder.getSlaveLogRecords().get(agent.toComputer())); + return result.stream() + // order by millis maintaining sequence within milli + .sorted(Comparator.comparingLong(record -> record.getMillis() * 100L + record.getSequenceNumber())) + .map(formatter::formatMessage) + .collect(Collectors.toList()); } - static void showIndexing(@Nonnull WorkflowMultiBranchProject mp) throws Exception { - FolderComputation indexing = mp.getIndexing(); - System.out.println("---%<--- " + indexing.getUrl()); - indexing.writeWholeLogTo(System.out); - System.out.println("---%<--- "); + static String printDate(Date dt) { + return DateTimeFormatter.ISO_INSTANT.format(Instant.ofEpochMilli(dt.getTime()).truncatedTo( + ChronoUnit.SECONDS)); } } \ No newline at end of file From 3ac6082acd736b8c9299d35ba55a6ea7c5510ba2 Mon Sep 17 00:00:00 2001 From: Liam Newman Date: Tue, 1 Sep 2020 01:38:18 -0700 Subject: [PATCH 16/18] Simplified log checking Removed sorting that was likely to result in flaky results. There is only one way the agent log makes sense and the output is still checked. --- .../github_branch_source/GithubAppCredentialsTest.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) 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 71e029f2e..6d6e0b823 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 @@ -24,6 +24,7 @@ import java.time.format.DateTimeFormatter; import java.time.temporal.ChronoUnit; import java.util.ArrayList; +import java.util.Collections; import java.util.Comparator; import java.util.Date; import java.util.List; @@ -215,6 +216,8 @@ public void testAgentRefresh() throws Exception { //Verify correct messages from GitHubAppCredential logger indicating token was retrieved on agent assertThat("Creds should cache on master, pass to agent, and refresh agent from master once", credentialsLog, contains( + // (agent log added out of order, see below) + "Keeping cached GitHub App Installation Token for app ID 54321 on agent: token is stale but has not expired", // node ('my-agent') { "Generating App Installation Token for app ID 54321", "Token will become stale after 15 seconds", @@ -232,7 +235,7 @@ credentialsLog, contains( // (error forced by wiremock - failed refresh on the agent) "Generating App Installation Token for app ID 54321 for agent", "Failed to retrieve GitHub App installation token for app ID 54321", - "Keeping cached GitHub App Installation Token for app ID 54321 on agent: token is stale but has not expired", + // (agent log added out of order) "Keeping cached GitHub App Installation Token for app ID 54321 on agent: token is stale but has not expired", // checkout scm - refresh on controller "Generating App Installation Token for app ID 54321", "Token will become stale after 15 seconds", @@ -240,7 +243,7 @@ credentialsLog, contains( "Retrieved GitHub App Installation Token for app ID 54321" // checkout scm // (No token generation) - )); + )); } finally { GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS = notStaleSeconds; logRecorder.doClear(); @@ -251,9 +254,8 @@ private List getOutputLines() { final Formatter formatter = new SimpleFormatter(); List result = new ArrayList<>(logRecorder.getLogRecords()); result.addAll(logRecorder.getSlaveLogRecords().get(agent.toComputer())); + Collections.reverse(result); return result.stream() - // order by millis maintaining sequence within milli - .sorted(Comparator.comparingLong(record -> record.getMillis() * 100L + record.getSequenceNumber())) .map(formatter::formatMessage) .collect(Collectors.toList()); } From d80588e4235426ac5fc47f6eb2bf975e77918f49 Mon Sep 17 00:00:00 2001 From: Liam Newman Date: Tue, 1 Sep 2020 12:22:31 -0700 Subject: [PATCH 17/18] Update based on review feedback --- .../GitHubAppCredentials.java | 40 ++--- .../GithubAppCredentialsTest.java | 157 ++++++++++++------ ...bapp-create-installation-accesstokens.json | 105 ------------ 3 files changed, 128 insertions(+), 174 deletions(-) delete mode 100644 src/test/resources/api/AppCredentials/files/body-githubapp-create-installation-accesstokens.json 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 b7325cb9d..7ed1bf11e 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 @@ -7,6 +7,7 @@ import edu.umd.cs.findbugs.annotations.NonNull; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import hudson.Extension; +import hudson.Functions; import hudson.Util; import hudson.remoting.Channel; import hudson.util.FormValidation; @@ -121,7 +122,12 @@ static AppInstallationToken generateAppInstallationToken(String appId, String ap .withJwtToken(jwtToken) .build(); - GHApp app = gitHubApp.getApp(); + GHApp app; + try { + app = gitHubApp.getApp(); + } catch (IOException e) { + throw new IllegalArgumentException(String.format(ERROR_AUTHENTICATING_GITHUB_APP, appId), e); + } List appInstallations = app.listInstallations().asList(); if (appInstallations.isEmpty()) { @@ -134,9 +140,7 @@ static AppInstallationToken generateAppInstallationToken(String appId, String ap appInstallation = appInstallations.stream() .filter(installation -> installation.getAccount().getLogin().equals(owner)) .findAny() - .orElseThrow(() -> new IllegalArgumentException(String.format( - ERROR_NOT_INSTALLED, - appId))); + .orElseThrow(() -> new IllegalArgumentException(String.format(ERROR_NOT_INSTALLED, appId))); } GHAppInstallationToken appInstallationToken = appInstallation @@ -144,20 +148,15 @@ static AppInstallationToken generateAppInstallationToken(String appId, String ap .create(); long expiration = getExpirationSeconds(appInstallationToken); - LOGGER.log(Level.FINEST, - "Token raw expiration epoch seconds: {0,number,#}", - expiration); - AppInstallationToken token = new AppInstallationToken(appInstallationToken.getToken(), expiration); - LOGGER.log(Level.FINE, + LOGGER.log(Level.FINER, "Generated App Installation Token for app ID {0}", appId); return token; } catch (Exception 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); + throw new IllegalArgumentException("Failed to generate GitHub App installation token for app ID " + appId , e); } } @@ -202,7 +201,8 @@ public Secret getPassword() { // This minimizes failures due to occasional network instability, // while only slightly increasing the chance that tokens will expire while in use. LOGGER.log(Level.WARNING, - "Keeping cached GitHub App Installation Token for app ID {0}: token is stale but has not expired", appID); + "Failed to generate new GitHub App Installation Token for app ID " + appID + ": cached token is stale but has not expired", + e); } else { throw e; } @@ -298,7 +298,7 @@ public AppInstallationToken(String token, long expirationEpochSeconds) { secondsUntilStale = maximumAllowedAge; } - LOGGER.log(Level.FINER, "Token will become stale after {0,number,#} seconds", secondsUntilStale); + LOGGER.log(Level.FINER, "Token will become stale after " + secondsUntilStale + " seconds"); this.token = token; this.expirationEpochSeconds = expirationEpochSeconds; @@ -377,7 +377,7 @@ private static final class DelegatingGitHubAppCredentials extends BaseStandardCr LOGGER.log(Level.FINEST, "Checking App Installation Token for app ID {0} before sending to agent", onMaster.appID); onMaster.getPassword(); } catch (Exception e) { - LOGGER.log(Level.WARNING, "Failed to refresh stale GitHub App installation token before sending to agent for app ID " + onMaster.getAppID(), e); + LOGGER.log(Level.WARNING, "Failed to update stale GitHub App installation token for app ID " + onMaster.getAppID() + " before sending to agent", e); } cachedToken = onMaster.getCachedToken(); @@ -405,10 +405,9 @@ public Secret getPassword() { synchronized (this) { try { if (cachedToken == null || cachedToken.isStale()) { + LOGGER.log(Level.FINE, "Generating App Installation Token for app ID {0} on agent", appID); cachedToken = ch.call(new GetToken(tokenRefreshData)); - LOGGER.log(Level.INFO, - "Retrieved GitHub App Installation Token for app ID {0} on agent", - appID); + LOGGER.log(Level.FINER, "Retrieved GitHub App Installation Token for app ID {0} on agent", appID); } } catch (Exception e) { if (cachedToken != null && !cachedToken.isExpired()) { @@ -416,8 +415,10 @@ public Secret getPassword() { // This minimizes failures due to occasional network instability, // while only slightly increasing the chance that tokens will expire while in use. LOGGER.log(Level.WARNING, - "Keeping cached GitHub App Installation Token for app ID {0} on agent: token is stale but has not expired", - appID); + "Failed to generate new GitHub App Installation Token for app ID " + appID + " on agent: cached token is stale but has not expired"); + // Logging the exception here caused a security exeception when trying to read the agent logs during testing + // Added the exception to a secondary log message that can be viewed if it is needed + LOGGER.log(Level.FINER, () -> Functions.printThrowable(e)); } else { throw e; } @@ -429,7 +430,6 @@ public Secret getPassword() { 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); } } 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 6d6e0b823..0730593ee 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 @@ -6,11 +6,13 @@ import com.cloudbees.plugins.credentials.domains.Domain; import hudson.logging.LogRecorder; import hudson.logging.LogRecorderManager; +import hudson.model.ParametersDefinitionProperty; +import hudson.model.Result; import hudson.model.Slave; +import hudson.model.StringParameterDefinition; import hudson.util.Secret; import jenkins.plugins.git.GitSampleRepoRule; -import jenkins.plugins.git.GitStep; -import org.jenkinsci.plugins.workflow.cps.CpsScmFlowDefinition; +import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition; import org.jenkinsci.plugins.workflow.job.WorkflowJob; import org.jenkinsci.plugins.workflow.job.WorkflowRun; import org.junit.Before; @@ -25,7 +27,6 @@ import java.time.temporal.ChronoUnit; import java.util.ArrayList; import java.util.Collections; -import java.util.Comparator; import java.util.Date; import java.util.List; import java.util.logging.Formatter; @@ -57,17 +58,18 @@ public static void setUpJenkins() throws Exception { appCredentials.setOwner("cloudbeers"); store.addCredentials(Domain.global(), appCredentials); + // Add agent + agent = r.createOnlineSlave(); + agent.setLabelString("my-agent"); + + // Would use LoggerRule, but need to get agent logs as well LogRecorderManager mgr = r.jenkins.getLog(); logRecorder = new LogRecorder(GitHubAppCredentials.class.getName()); mgr.logRecorders.put(GitHubAppCredentials.class.getName(), logRecorder); - LogRecorder.Target t = new LogRecorder.Target(GitHubAppCredentials.class.getName(), Level.FINER); + LogRecorder.Target t = new LogRecorder.Target(GitHubAppCredentials.class.getName(), Level.FINE); logRecorder.targets.add(t); logRecorder.save(); t.enable(); - - // Add agent - agent = r.createOnlineSlave(); - agent.setLabelString("my-agent"); } @Before @@ -101,7 +103,7 @@ public void setUpWireMock() throws Exception { .withHeader("Content-Type", "application/json; charset=utf-8") .withBody("{\n" + " \"token\": \"super-secret-token\",\n" + - // This token will go stale at the soonest allowed time but will no be expired for the duration of the test + // This token will go stale at the soonest allowed time but will not expire for the duration of the test " \"expires_at\": \"" + printDate(new Date(System.currentTimeMillis() + Duration.ofMinutes(10).toMillis())) + "\"" + // 2019-08-10T05:54:58Z "}"))); @@ -121,12 +123,12 @@ public void setUpWireMock() throws Exception { .withHeader("Content-Type", "application/json; charset=utf-8") .withBody("{\"message\": \"File not found\"}"))); - // Force an error to test fallback to returning unexpired token + // Force an error to test fallback to returning unexpired token on agent githubApi.stubFor( post(urlEqualTo("/app/installations/654321/access_tokens")) .inScenario(scenarioName) - .whenScenarioStateIs("1") - .willSetStateTo("2") + .whenScenarioStateIs("2") + .willSetStateTo("3") .withRequestBody( equalToJson( "{\"permissions\":{\"pull_requests\":\"write\",\"metadata\":\"read\",\"checks\":\"write\",\"contents\":\"read\"}}", true, false)) @@ -137,12 +139,31 @@ public void setUpWireMock() throws Exception { .withHeader("Content-Type", "application/json; charset=utf-8") .withBody("{\"message\": \"File not found\"}"))); - // Force an error to test fallback refreshing from agent + // return an expired token on controller githubApi.stubFor( post(urlEqualTo("/app/installations/654321/access_tokens")) .inScenario(scenarioName) - .whenScenarioStateIs("2") - .willSetStateTo("3") + .whenScenarioStateIs("3") + .willSetStateTo("4") + .withRequestBody( + equalToJson( + "{\"permissions\":{\"pull_requests\":\"write\",\"metadata\":\"read\",\"checks\":\"write\",\"contents\":\"read\"}}", true, false)) + .willReturn( + aResponse() + .withHeader("Content-Type", "application/json; charset=utf-8") + .withBody("{\n" + + " \"token\": \"super-secret-token\",\n" + + // token is already expired, but will not go stale for at least the minimum time + // This is a valid scenario - clocks are not always properly synchronized. + " \"expires_at\": \"" + printDate(new Date()) + "\"" + // 2019-08-10T05:54:58Z + "}"))); + + // Force an error to test non-fallback scenario and refreshing on agent + githubApi.stubFor( + post(urlEqualTo("/app/installations/654321/access_tokens")) + .inScenario(scenarioName) + .whenScenarioStateIs("4") + .willSetStateTo("5") .withRequestBody( equalToJson( "{\"permissions\":{\"pull_requests\":\"write\",\"metadata\":\"read\",\"checks\":\"write\",\"contents\":\"read\"}}", true, false)) @@ -153,19 +174,39 @@ public void setUpWireMock() throws Exception { .withHeader("Content-Type", "application/json; charset=utf-8") .withBody("{\"message\": \"File not found\"}"))); + // Valid token retirieved on agent githubApi.stubFor( post(urlEqualTo("/app/installations/654321/access_tokens")) .inScenario(scenarioName) - .whenScenarioStateIs("3") - .willSetStateTo("4") + .whenScenarioStateIs("5") + .willSetStateTo("6") .withRequestBody( equalToJson( "{\"permissions\":{\"pull_requests\":\"write\",\"metadata\":\"read\",\"checks\":\"write\",\"contents\":\"read\"}}", true, false)) .willReturn( aResponse() .withHeader("Content-Type", "application/json; charset=utf-8") - .withBodyFile("../AppCredentials/files/body-githubapp-create-installation-accesstokens.json"))); + .withBody("{\n" + + " \"token\": \"super-secret-token\",\n" + + " \"expires_at\": \"" + printDate(new Date()) + "\"" + // 2019-08-10T05:54:58Z + "}"))); + // Valid token retirieved on controller + githubApi.stubFor( + post(urlEqualTo("/app/installations/654321/access_tokens")) + .inScenario(scenarioName) + .whenScenarioStateIs("6") + .willSetStateTo("7") // setting this to non-existant state means any extra requests will fail + .withRequestBody( + equalToJson( + "{\"permissions\":{\"pull_requests\":\"write\",\"metadata\":\"read\",\"checks\":\"write\",\"contents\":\"read\"}}", true, false)) + .willReturn( + aResponse() + .withHeader("Content-Type", "application/json; charset=utf-8") + .withBody("{\n" + + " \"token\": \"super-secret-token\",\n" + + " \"expires_at\": \"" + printDate(new Date()) + "\"" + // 2019-08-10T05:54:58Z + "}"))); } @Test @@ -176,39 +217,50 @@ public void testAgentRefresh() throws Exception { // We want to demonstrate successful caching without waiting for a the default 1 minute // Must set this to a large enough number to avoid flaky test - GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS = 15; + GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS = 5; + + final String gitCheckoutStep = String.format( + " git url: REPO, credentialsId: '%s'", + myAppCredentialsId); final String jenkinsfile = String.join( "\n", "// run checkout several times", "node ('my-agent') {", - // First Checkout on agent should use cached token passed via remoting - " checkout scm", - // Multiple checkouts in quick succession should cached token - " checkout scm", + " echo 'First Checkout on agent should use cached token passed via remoting'", + gitCheckoutStep, + " echo 'Multiple checkouts in quick succession should use cached token'", + gitCheckoutStep, + " sleep " + (GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS + 2), + " echo 'Checkout after token is stale refreshes via remoting - fallback due to unexpired token'", + gitCheckoutStep, + " echo 'Checkout after error will refresh again on controller - new token expired but not stale'", + gitCheckoutStep, " sleep " + (GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS + 2), - // Checkout after token expires refreshes via remoting - error on controller is not catastrophic - " checkout scm", - // Checkout after error will refresh again on controller - " checkout scm", - // Multiple checkouts in quick succession should use cached token - " checkout scm", + " echo 'Checkout after token is stale refreshes via remoting - error on controller is not catastrophic'", + gitCheckoutStep, + " echo 'Checkout after error will refresh again on controller - new token expired but not stale'", + gitCheckoutStep, + " echo 'Multiple checkouts in quick succession should use cached token'", + gitCheckoutStep, "}"); + // Create a repo with the above Jenkinsfile sampleRepo.init(); sampleRepo.write("Jenkinsfile", jenkinsfile); sampleRepo.git("add", "Jenkinsfile"); sampleRepo.git("commit", "--message=init"); - WorkflowJob job = r.createProject(WorkflowJob.class, "repo-master"); - GitStep gitStep = new GitStep(sampleRepo.toString()); - gitStep.setCredentialsId(myAppCredentialsId); - gitStep.setPoll(false); - job.setDefinition(new CpsScmFlowDefinition(gitStep.createSCM(), "Jenkinsfile")); + // Create a pipeline job that points the above repo + WorkflowJob job = r.createProject(WorkflowJob.class, "test-creds"); + job.setDefinition(new CpsFlowDefinition(jenkinsfile, false)); + job.addProperty(new ParametersDefinitionProperty( + new StringParameterDefinition("REPO", sampleRepo.toString()))); WorkflowRun run = job.scheduleBuild2(0).waitForStart(); - r.waitForCompletion(run); + r.waitUntilNoActivity(); + assertThat(run.getResult(), equalTo(Result.SUCCESS)); System.out.println(JenkinsRule.getLog(run)); List credentialsLog = getOutputLines(); @@ -217,30 +269,34 @@ public void testAgentRefresh() throws Exception { assertThat("Creds should cache on master, pass to agent, and refresh agent from master once", credentialsLog, contains( // (agent log added out of order, see below) - "Keeping cached GitHub App Installation Token for app ID 54321 on agent: token is stale but has not expired", + "Generating App Installation Token for app ID 54321 on agent", // 1 + "Failed to generate new GitHub App Installation Token for app ID 54321 on agent: cached token is stale but has not expired", // 2 + "Generating App Installation Token for app ID 54321 on agent", // 3 // node ('my-agent') { - "Generating App Installation Token for app ID 54321", - "Token will become stale after 15 seconds", - "Generated App Installation Token for app ID 54321", - "Retrieved GitHub App Installation Token for app ID 54321", // checkout scm + "Generating App Installation Token for app ID 54321", // checkout scm + // (No token generation) // sleep - // (^^^ No token generation for these three steps) // checkout scm "Generating App Installation Token for app ID 54321", // (error forced by wiremock) - "Failed to retrieve GitHub App installation token for app ID 54321", - "Keeping cached GitHub App Installation Token for app ID 54321: token is stale but has not expired", + "Failed to generate new GitHub App Installation Token for app ID 54321: cached token is stale but has not expired", // (error forced by wiremock - failed refresh on the agent) + // "Generating App Installation Token for app ID 54321 on agent", // 1 "Generating App Installation Token for app ID 54321 for agent", - "Failed to retrieve GitHub App installation token for app ID 54321", - // (agent log added out of order) "Keeping cached GitHub App Installation Token for app ID 54321 on agent: token is stale but has not expired", + // (agent log added out of order) "Keeping cached GitHub App Installation Token for app ID 54321 on agent: token is stale but has not expired", // 2 // checkout scm - refresh on controller "Generating App Installation Token for app ID 54321", - "Token will become stale after 15 seconds", - "Generated App Installation Token for app ID 54321", - "Retrieved GitHub App Installation Token for app ID 54321" + // sleep + // checkout scm + "Generating App Installation Token for app ID 54321", + // (error forced by wiremock) + "Failed to update stale GitHub App installation token for app ID 54321 before sending to agent", + // "Generating App Installation Token for app ID 54321 on agent", // 3 + "Generating App Installation Token for app ID 54321 for agent", + // checkout scm - refresh on controller + "Generating App Installation Token for app ID 54321" // checkout scm // (No token generation) )); @@ -253,7 +309,10 @@ credentialsLog, contains( private List getOutputLines() { final Formatter formatter = new SimpleFormatter(); List result = new ArrayList<>(logRecorder.getLogRecords()); - result.addAll(logRecorder.getSlaveLogRecords().get(agent.toComputer())); + List agentLogs = logRecorder.getSlaveLogRecords().get(agent.toComputer()); + if (agentLogs != null) { + result.addAll(agentLogs); + } Collections.reverse(result); return result.stream() .map(formatter::formatMessage) diff --git a/src/test/resources/api/AppCredentials/files/body-githubapp-create-installation-accesstokens.json b/src/test/resources/api/AppCredentials/files/body-githubapp-create-installation-accesstokens.json deleted file mode 100644 index cc37ff474..000000000 --- a/src/test/resources/api/AppCredentials/files/body-githubapp-create-installation-accesstokens.json +++ /dev/null @@ -1,105 +0,0 @@ -{ - "token": "super-secret-token", - "expires_at": "2019-08-10T05:54:58Z", - "permissions": { - "checks": "write", - "pull_requests": "write", - "contents": "read", - "metadata": "read" - }, - "repository_selection": "selected", - "repositories": [ - { - "id": 1010, - "node_id": "asdfasdf", - "name": "repo", - "full_name": "cloudbeers/repo", - "private": true, - "owner": { - "login": "cloudbeers", - "id": 4181899, - "node_id": "asdfasdfasdf", - "url": "https://api.github.com/orgs/cloudbeers", - "html_url": "https://github.com/cloudbeers", - "followers_url": "https://api.github.com/users/cloudbeers/followers", - "following_url": "https://api.github.com/users/cloudbeers/following{/other_user}", - "gists_url": "https://api.github.com/users/cloudbeers/gists{/gist_id}", - "starred_url": "https://api.github.com/users/cloudbeers/starred{/owner}{/repo}", - "subscriptions_url": "https://api.github.com/users/cloudbeers/subscriptions", - "organizations_url": "https://api.github.com/users/cloudbeers/orgs", - "repos_url": "https://api.github.com/users/cloudbeers/repos", - "events_url": "https://api.github.com/users/cloudbeers/events{/privacy}", - "received_events_url": "https://api.github.com/users/cloudbeers/received_events", - "type": "Organization", - "site_admin": false - }, - "html_url": "https://github.com/cloudbeers/repo", - "description": null, - "fork": false, - "url": "https://api.github.com/repos/cloudbeers/repo", - "forks_url": "https://api.github.com/repos/cloudbeers/repo/forks", - "keys_url": "https://api.github.com/repos/cloudbeers/repo/keys{/key_id}", - "collaborators_url": "https://api.github.com/repos/cloudbeers/repo/collaborators{/collaborator}", - "teams_url": "https://api.github.com/repos/cloudbeers/repo/teams", - "hooks_url": "https://api.github.com/repos/cloudbeers/repo/hooks", - "issue_events_url": "https://api.github.com/repos/cloudbeers/repo/issues/events{/number}", - "events_url": "https://api.github.com/repos/cloudbeers/repo/events", - "assignees_url": "https://api.github.com/repos/cloudbeers/repo/assignees{/user}", - "branches_url": "https://api.github.com/repos/cloudbeers/repo/branches{/branch}", - "tags_url": "https://api.github.com/repos/cloudbeers/repo/tags", - "blobs_url": "https://api.github.com/repos/cloudbeers/repo/git/blobs{/sha}", - "git_tags_url": "https://api.github.com/repos/cloudbeers/repo/git/tags{/sha}", - "git_refs_url": "https://api.github.com/repos/cloudbeers/repo/git/refs{/sha}", - "trees_url": "https://api.github.com/repos/cloudbeers/repo/git/trees{/sha}", - "statuses_url": "https://api.github.com/repos/cloudbeers/repo/statuses/{sha}", - "languages_url": "https://api.github.com/repos/cloudbeers/repo/languages", - "stargazers_url": "https://api.github.com/repos/cloudbeers/repo/stargazers", - "contributors_url": "https://api.github.com/repos/cloudbeers/repo/contributors", - "subscribers_url": "https://api.github.com/repos/cloudbeers/repo/subscribers", - "subscription_url": "https://api.github.com/repos/cloudbeers/repo/subscription", - "commits_url": "https://api.github.com/repos/cloudbeers/repo/commits{/sha}", - "git_commits_url": "https://api.github.com/repos/cloudbeers/repo/git/commits{/sha}", - "comments_url": "https://api.github.com/repos/cloudbeers/repo/comments{/number}", - "issue_comment_url": "https://api.github.com/repos/cloudbeers/repo/issues/comments{/number}", - "contents_url": "https://api.github.com/repos/cloudbeers/repo/contents/{+path}", - "compare_url": "https://api.github.com/repos/cloudbeers/repo/compare/{base}...{head}", - "merges_url": "https://api.github.com/repos/cloudbeers/repo/merges", - "archive_url": "https://api.github.com/repos/cloudbeers/repo/{archive_format}{/ref}", - "downloads_url": "https://api.github.com/repos/cloudbeers/repo/downloads", - "issues_url": "https://api.github.com/repos/cloudbeers/repo/issues{/number}", - "pulls_url": "https://api.github.com/repos/cloudbeers/repo/pulls{/number}", - "milestones_url": "https://api.github.com/repos/cloudbeers/repo/milestones{/number}", - "notifications_url": "https://api.github.com/repos/cloudbeers/repo/notifications{?since,all,participating}", - "labels_url": "https://api.github.com/repos/cloudbeers/repo/labels{/name}", - "releases_url": "https://api.github.com/repos/cloudbeers/repo/releases{/id}", - "deployments_url": "https://api.github.com/repos/cloudbeers/repo/deployments", - "created_at": "2018-09-06T03:25:38Z", - "updated_at": "2018-09-30T22:04:06Z", - "pushed_at": "2019-08-08T22:22:34Z", - "git_url": "git://github.com/cloudbeers/repo.git", - "ssh_url": "git@github.com:cloudbeers/repo.git", - "clone_url": "https://github.com/cloudbeers/repo.git", - "svn_url": "https://github.com/cloudbeers/repo", - "homepage": null, - "size": 618, - "stargazers_count": 0, - "watchers_count": 0, - "language": "Java", - "has_issues": true, - "has_projects": true, - "has_downloads": true, - "has_wiki": true, - "has_pages": false, - "forks_count": 0, - "mirror_url": null, - "archived": false, - "disabled": false, - "open_issues_count": 5, - "license": null, - "forks": 0, - "open_issues": 5, - "watchers": 0, - "default_branch": "main" - } - ] -} \ No newline at end of file From b605c642cf79c04d35fb61b7569bc7d1db3a9ecb Mon Sep 17 00:00:00 2001 From: Liam Newman Date: Tue, 1 Sep 2020 13:26:16 -0700 Subject: [PATCH 18/18] Switch to JSONObject for data blob --- .../GitHubAppCredentials.java | 25 +++++++++++-------- 1 file changed, 15 insertions(+), 10 deletions(-) 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 7ed1bf11e..843178454 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 @@ -24,6 +24,7 @@ import jenkins.security.SlaveToMasterCallable; import jenkins.util.JenkinsJVM; +import net.sf.json.JSONObject; import org.jenkinsci.plugins.workflow.support.concurrent.Timeout; import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.restrictions.NoExternalUse; @@ -350,8 +351,6 @@ private Object writeReplace() { private static final class DelegatingGitHubAppCredentials extends BaseStandardCredentials implements StandardUsernamePasswordCredentials { - private static final String SEP = "%%%"; - private final String appID; /** * An encrypted form of all data needed to refresh the token. @@ -366,7 +365,12 @@ private static final class DelegatingGitHubAppCredentials extends BaseStandardCr 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(); + JSONObject j = new JSONObject(); + j.put("appID", appID); + j.put("privateKey", onMaster.privateKey.getPlainText()); + j.put("apiUri", onMaster.actualApiUri()); + j.put("owner", onMaster.owner); + tokenRefreshData = Secret.fromString(j.toString()).getEncryptedValue(); // Check token is valid before sending it to the agent. // Ensuring the cached token is not stale before sending it to agents keeps agents from having to @@ -445,15 +449,16 @@ private static final class GetToken extends SlaveToMasterCallable