From fd58e5e10975947f1e33cee2f7982d5528876918 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 20 Aug 2020 14:56:05 -0400 Subject: [PATCH 1/7] 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 2/7] 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 3/7] 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 4/7] 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 5/7] 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 6/7] 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 7/7] 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;