Skip to content

Commit 762a261

Browse files
bitwisemanbitwiseman
committed
Use credential snapshotting
1 parent cd85b85 commit 762a261

File tree

2 files changed

+96
-105
lines changed

2 files changed

+96
-105
lines changed

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

Lines changed: 91 additions & 102 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,56 @@ 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,
232+
privateKey.getPlainText(),
233+
apiUri,
234+
owner);
235+
}
217236
}
218237

219238
/**
@@ -225,6 +244,12 @@ public String getUsername() {
225244
return appID;
226245
}
227246

247+
void setCachedToken(AppInstallationToken token) {
248+
synchronized (this) {
249+
this.cachedToken = token;
250+
}
251+
}
252+
228253
private AppInstallationToken getCachedToken() {
229254
synchronized (this) {
230255
return cachedToken;
@@ -342,128 +367,92 @@ long getTokenStaleEpochSeconds() {
342367
* <li>The agent need not be able to contact GitHub.
343368
* </ul>
344369
*/
345-
private Object writeReplace() {
346-
if (/* XStream */Channel.current() == null) {
347-
return this;
370+
@Extension
371+
public static final class SnapshotTaker extends CredentialsSnapshotTaker<GitHubAppCredentials> {
372+
@Override
373+
public Class<GitHubAppCredentials> type() {
374+
return GitHubAppCredentials.class;
348375
}
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-
376+
@Override
377+
public GitHubAppCredentials snapshot(GitHubAppCredentials credentials) {
375378
// Check token is valid before sending it to the agent.
376379
// Ensuring the cached token is not stale before sending it to agents keeps agents from having to
377380
// immediately refresh the token.
378381
// This is intentionally only a best-effort attempt.
379382
// If this fails, the agent will fallback to making the request (which may or may not fail).
380383
try {
381-
LOGGER.log(Level.FINEST, "Checking App Installation Token for app ID {0} before sending to agent", onMaster.appID);
382-
onMaster.getPassword();
384+
LOGGER.log(Level.FINEST, "Checking App Installation Token for app ID {0} before sending to agent", credentials.appID);
385+
credentials.getPassword();
383386
} 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);
387+
LOGGER.log(Level.WARNING, "Failed to update stale GitHub App installation token for app ID " + credentials.appID + " before sending to agent", e);
385388
}
389+
return new GitHubAppCredentialsSnapshot(credentials);
390+
}
391+
}
392+
393+
private static final class GitHubAppCredentialsSnapshot extends GitHubAppCredentials {
394+
private AppInstallationToken snapshotToken;
395+
private transient Channel ch;
386396

387-
cachedToken = onMaster.getCachedToken();
397+
GitHubAppCredentialsSnapshot(GitHubAppCredentials baseCredentials) {
398+
super(baseCredentials);
399+
this.snapshotToken = baseCredentials.getCachedToken();
388400
}
389401

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

404418
@Override
405-
public Secret getPassword() {
419+
void refreshStaleToken() {
406420
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();
421+
LOGGER.log(Level.FINE, "Generating App Installation Token for app ID {0} on agent", getAppID());
422+
synchronized (this) {
423+
try {
424+
setCachedToken(ch.call(new GetToken(this)));
425+
} catch (IOException | InterruptedException e) {
426+
throw new RuntimeException(e);
431427
}
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);
438428
}
439429
}
440430

441431
private static final class GetToken extends SlaveToMasterCallable<AppInstallationToken, RuntimeException> {
442432

443-
private final String data;
433+
private final GitHubAppCredentials data;
444434

445-
GetToken(String data) {
435+
GetToken(GitHubAppCredentials data) {
446436
this.data = data;
447437
}
448438

449439
@Override
450440
public AppInstallationToken call() throws RuntimeException {
451441
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"));
442+
LOGGER.log(Level.FINE, "Generating App Installation Token for app ID {0} for agent", data.appID);
454443
AppInstallationToken token = generateAppInstallationToken(
455-
(String)fields.get("appID"),
456-
(String)fields.get("privateKey"),
457-
(String)fields.get("apiUri"),
458-
(String)fields.get("owner"));
444+
data.appID,
445+
data.privateKey.getPlainText(),
446+
data.apiUri,
447+
data.owner);
459448
LOGGER.log(Level.FINER,
460449
"Retrieved GitHub App Installation Token for app ID {0} for agent",
461-
fields.get("appID"));
450+
data.appID);
462451
return token;
463452
}
464453
}
465454
}
466-
455+
467456
/**
468457
* {@inheritDoc}
469458
*/

‎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)