Skip to content

Commit 3f1668f

Browse files
bitwisemanbitwiseman
committed
Use credential snapshotting
1 parent cd85b85 commit 3f1668f

File tree

2 files changed

+92
-104
lines changed

2 files changed

+92
-104
lines changed

‎src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentials.java

Lines changed: 87 additions & 101 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package org.jenkinsci.plugins.github_branch_source;
22

33
import com.cloudbees.plugins.credentials.CredentialsScope;
4+
import com.cloudbees.plugins.credentials.CredentialsSnapshotTaker;
45
import com.cloudbees.plugins.credentials.common.StandardUsernamePasswordCredentials;
56
import com.cloudbees.plugins.credentials.impl.BaseStandardCredentials;
67
import edu.umd.cs.findbugs.annotations.CheckForNull;
@@ -24,7 +25,6 @@
2425

2526
import jenkins.security.SlaveToMasterCallable;
2627
import jenkins.util.JenkinsJVM;
27-
import net.sf.json.JSONObject;
2828
import org.jenkinsci.plugins.workflow.support.concurrent.Timeout;
2929
import org.kohsuke.accmod.Restricted;
3030
import org.kohsuke.accmod.restrictions.NoExternalUse;
@@ -76,6 +76,13 @@ public GitHubAppCredentials(
7676
this.privateKey = privateKey;
7777
}
7878

79+
80+
private GitHubAppCredentials(GitHubAppCredentials base) {
81+
this(base.getScope(), base.getId(), base.getDescription(), base.getAppID(), base.getPrivateKey());
82+
this.apiUri = base.getApiUri();
83+
this.owner = base.getOwner();
84+
}
85+
7986
public String getApiUri() {
8087
return apiUri;
8188
}
@@ -116,6 +123,7 @@ public void setOwner(String owner) {
116123
@SuppressWarnings("deprecation") // preview features are required for GitHub app integration, GitHub api adds deprecated to all preview methods
117124
static AppInstallationToken generateAppInstallationToken(String appId, String appPrivateKey, String apiUrl, String owner) {
118125
// We expect this to be fast but if anything hangs in here we do not want to block indefinitely
126+
apiUrl = Util.fixEmpty(apiUrl) == null ? "https://api.github.com" : apiUrl;
119127
try (Timeout timeout = Timeout.limit(30, TimeUnit.SECONDS)) {
120128
String jwtToken = createJWT(appId, appPrivateKey);
121129
GitHub gitHubApp = Connector
@@ -175,45 +183,53 @@ private static long getExpirationSeconds(GHAppInstallationToken appInstallationT
175183
}
176184
}
177185

178-
@NonNull String actualApiUri() {
179-
return Util.fixEmpty(apiUri) == null ? "https://api.github.com" : apiUri;
180-
}
181-
182186
/**
183187
* {@inheritDoc}
184188
*/
185189
@NonNull
186190
@Override
187191
public Secret getPassword() {
188-
String appInstallationToken;
192+
AppInstallationToken token = getValidToken();
193+
Secret password = Secret.fromString(token.getToken());
194+
195+
LOGGER.log(Level.FINEST, "Returned GitHub App Installation Token for app ID {0}", appID);
196+
197+
return password;
198+
}
199+
200+
@NonNull
201+
private AppInstallationToken getValidToken() {
189202
synchronized (this) {
190203
try {
191204
if (cachedToken == null || cachedToken.isStale()) {
192-
LOGGER.log(Level.FINE, "Generating App Installation Token for app ID {0}", appID);
193-
cachedToken = generateAppInstallationToken(appID,
194-
privateKey.getPlainText(),
195-
actualApiUri(),
196-
owner);
197-
LOGGER.log(Level.FINER, "Retrieved GitHub App Installation Token for app ID {0}", appID);
205+
refreshStaleToken();
206+
LOGGER.log(Level.FINER,
207+
"Retrieved GitHub App Installation Token for app ID {0}",
208+
appID);
198209
}
199210
} catch (Exception e) {
200211
if (cachedToken != null && !cachedToken.isExpired()) {
201212
// Requesting a new token failed. If the cached token is not expired, continue to use it.
202213
// This minimizes failures due to occasional network instability,
203214
// while only slightly increasing the chance that tokens will expire while in use.
204215
LOGGER.log(Level.WARNING,
205-
"Failed to generate new GitHub App Installation Token for app ID " + appID + ": cached token is stale but has not expired",
206-
e);
216+
"Failed to generate new GitHub App Installation Token for app ID " + getAppID() + ": cached token is stale but has not expired");
217+
// Logging the exception here caused a security exception when trying to read the agent logs during testing
218+
// Added the exception to a secondary log message that can be viewed if it is needed
219+
LOGGER.log(Level.FINER, () -> Functions.printThrowable(e));
207220
} else {
208221
throw e;
209222
}
210223
}
211-
appInstallationToken = cachedToken.getToken();
224+
return cachedToken;
212225
}
226+
}
213227

214-
LOGGER.log(Level.FINEST, "Returned GitHub App Installation Token for app ID {0}", appID);
215-
216-
return Secret.fromString(appInstallationToken);
228+
void refreshStaleToken() {
229+
LOGGER.log(Level.FINE, "Generating App Installation Token for app ID {0}", appID);
230+
synchronized (this) {
231+
cachedToken = generateAppInstallationToken(appID, privateKey.getPlainText(), apiUri, owner);
232+
}
217233
}
218234

219235
/**
@@ -225,6 +241,12 @@ public String getUsername() {
225241
return appID;
226242
}
227243

244+
void setCachedToken(AppInstallationToken token) {
245+
synchronized (this) {
246+
this.cachedToken = token;
247+
}
248+
}
249+
228250
private AppInstallationToken getCachedToken() {
229251
synchronized (this) {
230252
return cachedToken;
@@ -342,123 +364,87 @@ long getTokenStaleEpochSeconds() {
342364
* <li>The agent need not be able to contact GitHub.
343365
* </ul>
344366
*/
345-
private Object writeReplace() {
346-
if (/* XStream */Channel.current() == null) {
347-
return this;
367+
@Extension
368+
public static final class SnapshotTaker extends CredentialsSnapshotTaker<GitHubAppCredentials> {
369+
@Override
370+
public Class<GitHubAppCredentials> type() {
371+
return GitHubAppCredentials.class;
348372
}
349-
return new DelegatingGitHubAppCredentials(this);
350-
}
351-
352-
private static final class DelegatingGitHubAppCredentials extends BaseStandardCredentials implements StandardUsernamePasswordCredentials {
353-
354-
private final String appID;
355-
/**
356-
* An encrypted form of all data needed to refresh the token.
357-
* Used to prevent {@link GetToken} from being abused by compromised build agents.
358-
*/
359-
private final String tokenRefreshData;
360-
private AppInstallationToken cachedToken;
361-
362-
private transient Channel ch;
363-
364-
DelegatingGitHubAppCredentials(GitHubAppCredentials onMaster) {
365-
super(onMaster.getScope(), onMaster.getId(), onMaster.getDescription());
366-
JenkinsJVM.checkJenkinsJVM();
367-
appID = onMaster.appID;
368-
JSONObject j = new JSONObject();
369-
j.put("appID", appID);
370-
j.put("privateKey", onMaster.privateKey.getPlainText());
371-
j.put("apiUri", onMaster.actualApiUri());
372-
j.put("owner", onMaster.owner);
373-
tokenRefreshData = Secret.fromString(j.toString()).getEncryptedValue();
374-
373+
@Override
374+
public GitHubAppCredentials snapshot(GitHubAppCredentials credentials) {
375375
// Check token is valid before sending it to the agent.
376376
// Ensuring the cached token is not stale before sending it to agents keeps agents from having to
377377
// immediately refresh the token.
378378
// This is intentionally only a best-effort attempt.
379379
// If this fails, the agent will fallback to making the request (which may or may not fail).
380380
try {
381-
LOGGER.log(Level.FINEST, "Checking App Installation Token for app ID {0} before sending to agent", onMaster.appID);
382-
onMaster.getPassword();
381+
LOGGER.log(Level.FINEST, "Checking App Installation Token for app ID {0} before sending to agent", credentials.appID);
382+
credentials.getPassword();
383383
} catch (Exception e) {
384-
LOGGER.log(Level.WARNING, "Failed to update stale GitHub App installation token for app ID " + onMaster.getAppID() + " before sending to agent", e);
384+
LOGGER.log(Level.WARNING, "Failed to update stale GitHub App installation token for app ID " + credentials.appID + " before sending to agent", e);
385385
}
386+
return new DelegatingGitHubAppCredentials(credentials);
387+
}
388+
}
389+
390+
private static final class DelegatingGitHubAppCredentials extends GitHubAppCredentials {
391+
private AppInstallationToken snapshotToken;
392+
private transient Channel ch;
386393

387-
cachedToken = onMaster.getCachedToken();
394+
DelegatingGitHubAppCredentials(GitHubAppCredentials baseCredentials) {
395+
super(baseCredentials);
396+
this.snapshotToken = baseCredentials.getCachedToken();
388397
}
389398

390399
private Object readResolve() {
391-
JenkinsJVM.checkNotJenkinsJVM();
392-
synchronized (this) {
393-
ch = Channel.currentOrFail();
400+
if (!JenkinsJVM.isJenkinsJVM()) {
401+
synchronized (this) {
402+
ch = Channel.currentOrFail();
403+
this.setCachedToken(snapshotToken);
404+
}
405+
// snapshot is only needed during transport
406+
snapshotToken = null;
407+
return this;
408+
} else {
409+
GitHubAppCredentials resolved = new GitHubAppCredentials(this);
410+
resolved.setCachedToken(this.snapshotToken);
411+
return resolved;
394412
}
395-
return this;
396-
}
397-
398-
@NonNull
399-
@Override
400-
public String getUsername() {
401-
return appID;
402413
}
403414

404415
@Override
405-
public Secret getPassword() {
416+
void refreshStaleToken() {
406417
JenkinsJVM.checkNotJenkinsJVM();
407-
try {
408-
String appInstallationToken;
409-
synchronized (this) {
410-
try {
411-
if (cachedToken == null || cachedToken.isStale()) {
412-
LOGGER.log(Level.FINE, "Generating App Installation Token for app ID {0} on agent", appID);
413-
cachedToken = ch.call(new GetToken(tokenRefreshData));
414-
LOGGER.log(Level.FINER, "Retrieved GitHub App Installation Token for app ID {0} on agent", appID);
415-
}
416-
} catch (Exception e) {
417-
if (cachedToken != null && !cachedToken.isExpired()) {
418-
// Requesting a new token failed. If the cached token is not expired, continue to use it.
419-
// This minimizes failures due to occasional network instability,
420-
// while only slightly increasing the chance that tokens will expire while in use.
421-
LOGGER.log(Level.WARNING,
422-
"Failed to generate new GitHub App Installation Token for app ID " + appID + " on agent: cached token is stale but has not expired");
423-
// Logging the exception here caused a security exeception when trying to read the agent logs during testing
424-
// Added the exception to a secondary log message that can be viewed if it is needed
425-
LOGGER.log(Level.FINER, () -> Functions.printThrowable(e));
426-
} else {
427-
throw e;
428-
}
429-
}
430-
appInstallationToken = cachedToken.getToken();
418+
LOGGER.log(Level.FINE, "Generating App Installation Token for app ID {0} on agent", getAppID());
419+
synchronized (this) {
420+
try {
421+
setCachedToken(ch.call(new GetToken(this)));
422+
} catch (IOException | InterruptedException e) {
423+
throw new RuntimeException(e);
431424
}
432-
433-
LOGGER.log(Level.FINEST, "Returned GitHub App Installation Token for app ID {0} on agent", appID);
434-
435-
return Secret.fromString(appInstallationToken);
436-
} catch (IOException | InterruptedException x) {
437-
throw new RuntimeException(x);
438425
}
439426
}
440427

441428
private static final class GetToken extends SlaveToMasterCallable<AppInstallationToken, RuntimeException> {
442429

443-
private final String data;
430+
private final GitHubAppCredentials data;
444431

445-
GetToken(String data) {
432+
GetToken(GitHubAppCredentials data) {
446433
this.data = data;
447434
}
448435

449436
@Override
450437
public AppInstallationToken call() throws RuntimeException {
451438
JenkinsJVM.checkJenkinsJVM();
452-
JSONObject fields = JSONObject.fromObject(Secret.fromString(data).getPlainText());
453-
LOGGER.log(Level.FINE, "Generating App Installation Token for app ID {0} for agent", fields.get("appID"));
439+
LOGGER.log(Level.FINE, "Generating App Installation Token for app ID {0} for agent", data.appID);
454440
AppInstallationToken token = generateAppInstallationToken(
455-
(String)fields.get("appID"),
456-
(String)fields.get("privateKey"),
457-
(String)fields.get("apiUri"),
458-
(String)fields.get("owner"));
441+
data.appID,
442+
data.privateKey.getPlainText(),
443+
data.apiUri,
444+
data.owner);
459445
LOGGER.log(Level.FINER,
460446
"Retrieved GitHub App Installation Token for app ID {0} for agent",
461-
fields.get("appID"));
447+
data.appID);
462448
return token;
463449
}
464450
}

‎src/test/java/org/jenkinsci/plugins/github_branch_source/GithubAppCredentialsTest.java

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -260,7 +260,6 @@ public void testAgentRefresh() throws Exception {
260260
WorkflowRun run = job.scheduleBuild2(0).waitForStart();
261261
r.waitUntilNoActivity();
262262

263-
assertThat(run.getResult(), equalTo(Result.SUCCESS));
264263
System.out.println(JenkinsRule.getLog(run));
265264

266265
List<String> credentialsLog = getOutputLines();
@@ -270,7 +269,7 @@ public void testAgentRefresh() throws Exception {
270269
credentialsLog, contains(
271270
// (agent log added out of order, see below)
272271
"Generating App Installation Token for app ID 54321 on agent", // 1
273-
"Failed to generate new GitHub App Installation Token for app ID 54321 on agent: cached token is stale but has not expired", // 2
272+
"Failed to generate new GitHub App Installation Token for app ID 54321: cached token is stale but has not expired", // 2
274273
"Generating App Installation Token for app ID 54321 on agent", // 3
275274
// node ('my-agent') {
276275
// checkout scm
@@ -285,7 +284,7 @@ credentialsLog, contains(
285284
// (error forced by wiremock - failed refresh on the agent)
286285
// "Generating App Installation Token for app ID 54321 on agent", // 1
287286
"Generating App Installation Token for app ID 54321 for agent",
288-
// (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
287+
// (agent log added out of order) "Keeping cached GitHub App Installation Token for app ID 54321: token is stale but has not expired", // 2
289288
// checkout scm - refresh on controller
290289
"Generating App Installation Token for app ID 54321",
291290
// sleep
@@ -300,6 +299,9 @@ credentialsLog, contains(
300299
// checkout scm
301300
// (No token generation)
302301
));
302+
303+
assertThat(run.getResult(), equalTo(Result.SUCCESS));
304+
303305
} finally {
304306
GitHubAppCredentials.AppInstallationToken.NOT_STALE_MINIMUM_SECONDS = notStaleSeconds;
305307
logRecorder.doClear();

0 commit comments

Comments
 (0)