Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

JENKINS-64662 Create GitHub App Credentials Binding to support owner override #375

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

JENKINS-64662 Create GitHub App Credentials Binding to support owner override #375

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
61765b7
JENKINS-64662 Create GitHub App Credentials Binding to support owner …
nrayapati Jan 23, 2021
4f08223
Merge branch 'master' into JENKINS-64662
nrayapati Feb 6, 2021
55641cb
Merge branch 'master' into JENKINS-64662
nrayapati Feb 12, 2021
213b1c5
Merge branch 'task/formatting-base' into JENKINS-64662
nrayapati Mar 5, 2021
2908b09
Apply autoformatting
nrayapati Mar 5, 2021
1002e8d
Merge remote-tracking branch 'upstream/master' into JENKINS-64662
nrayapati Mar 5, 2021
d393dcf
Merge branch 'master' into JENKINS-64662
nrayapati Mar 10, 2021
4cacb9b
Merge branch 'master' into JENKINS-64662
nrayapati Apr 13, 2021
38635ba
Merge branch 'master' into JENKINS-64662
nrayapati Apr 16, 2021
bb2a559
Merge branch 'master' into JENKINS-64662
nrayapati Apr 23, 2021
094eb41
Merge branch 'master' into JENKINS-64662
nrayapati May 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@
<artifactId>github-api</artifactId>
<version>1.122</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>credentials-binding</artifactId>
</dependency>
<dependency>
<groupId>com.coravy.hudson.plugins.github</groupId>
<artifactId>github</artifactId>
Expand Down
98 changes: 80 additions & 18 deletions src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentials.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@
import java.security.GeneralSecurityException;
import java.time.Duration;
import java.time.Instant;
import java.util.ArrayList;
import java.util.List;
import java.util.Optional;
import java.util.concurrent.TimeUnit;
import java.util.logging.Level;
import java.util.logging.Logger;
Expand Down Expand Up @@ -48,7 +50,7 @@ public class GitHubAppCredentials extends BaseStandardCredentials
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";
"Couldn't authenticate with GitHub app ID: %s for owner: %s";
private static final String NOT_INSTALLED =
", has it been installed to your GitHub organisation / user?";

Expand All @@ -72,7 +74,7 @@ public class GitHubAppCredentials extends BaseStandardCredentials

private String owner;

private transient AppInstallationToken cachedToken;
private transient List<AppInstallationToken> cachedTokens;

@DataBoundConstructor
@SuppressWarnings("unused") // by stapler
Expand Down Expand Up @@ -201,31 +203,35 @@ static AppInstallationToken generateAppInstallationToken(
app = gitHubApp.getApp();
} catch (IOException e) {
throw new IllegalArgumentException(
String.format(ERROR_AUTHENTICATING_GITHUB_APP, appId), e);
String.format(ERROR_AUTHENTICATING_GITHUB_APP, appId, owner), e);
}

List<GHAppInstallation> appInstallations = app.listInstallations().asList();
if (appInstallations.isEmpty()) {
throw new IllegalArgumentException(String.format(ERROR_NOT_INSTALLED, appId));
throw new IllegalArgumentException(String.format(ERROR_NOT_INSTALLED, appId, owner));
}
GHAppInstallation appInstallation;
if (appInstallations.size() == 1) {
appInstallation = appInstallations.get(0);
} else {
appInstallation =
appInstallations.stream()
.filter(installation -> installation.getAccount().getLogin().equals(owner))
.filter(
installation -> installation.getAccount().getLogin().equalsIgnoreCase(owner))
.findAny()
.orElseThrow(
() -> new IllegalArgumentException(String.format(ERROR_NOT_INSTALLED, appId)));
() ->
new IllegalArgumentException(
String.format(ERROR_NOT_INSTALLED, appId, owner)));
}

GHAppInstallationToken appInstallationToken =
appInstallation.createToken(appInstallation.getPermissions()).create();

long expiration = getExpirationSeconds(appInstallationToken);
AppInstallationToken token =
new AppInstallationToken(Secret.fromString(appInstallationToken.getToken()), expiration);
new AppInstallationToken(
owner, Secret.fromString(appInstallationToken.getToken()), expiration);
LOGGER.log(Level.FINER, "Generated App Installation Token for app ID {0}", appId);
LOGGER.log(
Level.FINEST,
Expand Down Expand Up @@ -260,14 +266,32 @@ String actualApiUri() {
return Util.fixEmpty(apiUri) == null ? "https://api.github.com" : apiUri;
}

private AppInstallationToken getToken(GitHub gitHub) {
private AppInstallationToken getToken(final GitHub gitHub) {
return getToken(gitHub, owner);
}

private AppInstallationToken getToken(final GitHub gitHub, final String owner) {
AppInstallationToken cachedToken = null;
synchronized (this) {
try {
if (cachedTokens == null) {
cachedTokens = new ArrayList<>();
} else {
Optional<AppInstallationToken> tempToken =
cachedTokens.stream()
Copy link
Contributor

Choose a reason for hiding this comment

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

if you are just going to keep one token wouldn't a Map make more sense and be more clear?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried Map and changed it to List, I think I did that to avoid adding overrides to serialize/deserialize as this is transient field.

I think it is good to have that owner field with actual class just in case if we are going to use this token for future use cases across other classes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried Map and changed it to List, I think I did that to avoid adding overrides to serialize/deserialize as this is transient field.

Not sure why this is a reason to use List instead of Map.

I think it is good to have that owner field with actual class just in case if we are going to use this token for future use cases across other classes.

I'm not sure what you mean here.

.filter(token -> token.getOwner().equalsIgnoreCase(owner))
.findAny();
if (tempToken.isPresent()) {
cachedToken = tempToken.get();
}
}
if (cachedToken == null || cachedToken.isStale()) {
LOGGER.log(Level.FINE, "Generating App Installation Token for app ID {0}", appID);
cachedToken =
generateAppInstallationToken(
gitHub, appID, privateKey.getPlainText(), actualApiUri(), owner);
cachedTokens.removeIf(token -> token.getOwner().equalsIgnoreCase(owner));
cachedTokens.add(cachedToken);
LOGGER.log(Level.FINER, "Retrieved GitHub App Installation Token for app ID {0}", appID);
}
} catch (Exception e) {
Expand Down Expand Up @@ -298,16 +322,26 @@ public Secret getPassword() {
return this.getToken(null).getToken();
}

/**
* Returns the Password.
*
* @param owner, owner of the repo.
* @return the password
*/
public Secret getPassword(final String owner) {
return this.getToken(null, owner).getToken();
}

/** {@inheritDoc} */
@NonNull
@Override
public String getUsername() {
return appID;
}

private AppInstallationToken getCachedToken() {
private List<AppInstallationToken> getCachedTokens() {
synchronized (this) {
return cachedToken;
return cachedTokens;
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to other recent changes, we should look at using concurrent hash map instead.

}
}

Expand Down Expand Up @@ -350,6 +384,7 @@ static class AppInstallationToken implements Serializable {
GitHubAppCredentials.class.getName() + ".NOT_STALE_MINIMUM_SECONDS",
Duration.ofMinutes(1).getSeconds());

private final String owner;
private final Secret token;
private final long expirationEpochSeconds;
private final long staleEpochSeconds;
Expand All @@ -366,7 +401,7 @@ static class AppInstallationToken implements Serializable {
* @param token the token string
* @param expirationEpochSeconds the time in epoch seconds that this token will expire
*/
public AppInstallationToken(Secret token, long expirationEpochSeconds) {
public AppInstallationToken(String owner, Secret token, long expirationEpochSeconds) {
long now = Instant.now().getEpochSecond();
long secondsUntilExpiration = expirationEpochSeconds - now;

Expand All @@ -388,6 +423,7 @@ public AppInstallationToken(Secret token, long expirationEpochSeconds) {

LOGGER.log(Level.FINER, "Token will become stale after " + secondsUntilStale + " seconds");

this.owner = owner;
this.token = token;
this.expirationEpochSeconds = expirationEpochSeconds;
this.staleEpochSeconds = now + secondsUntilStale;
Expand All @@ -397,6 +433,10 @@ public Secret getToken() {
return token;
}

public String getOwner() {
return owner;
}

/**
* Whether a token is stale and should be replaced with a new token.
*
Expand Down Expand Up @@ -443,25 +483,26 @@ private static final class DelegatingGitHubAppCredentials extends BaseStandardCr
implements StandardUsernamePasswordCredentials {

private final String appID;
private final String owner;
/**
* 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;
private List<AppInstallationToken> cachedTokens;

private transient Channel ch;

DelegatingGitHubAppCredentials(GitHubAppCredentials onMaster) {
super(onMaster.getScope(), onMaster.getId(), onMaster.getDescription());
JenkinsJVM.checkJenkinsJVM();
appID = onMaster.appID;
owner = onMaster.owner;
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.
Expand All @@ -485,7 +526,7 @@ private static final class DelegatingGitHubAppCredentials extends BaseStandardCr
e);
}

cachedToken = onMaster.getCachedToken();
cachedTokens = onMaster.getCachedTokens();
}

private Object readResolve() {
Expand All @@ -504,14 +545,32 @@ public String getUsername() {

@Override
public Secret getPassword() {
return getPassword(owner);
}

public Secret getPassword(final String owner) {
JenkinsJVM.checkNotJenkinsJVM();
try {
synchronized (this) {
AppInstallationToken cachedToken = null;
try {
if (cachedTokens == null) {
cachedTokens = new ArrayList<>();
} else {
Optional<AppInstallationToken> tempToken =
cachedTokens.stream()
.filter(token -> token.getOwner().equalsIgnoreCase(owner))
.findAny();
if (tempToken.isPresent()) {
cachedToken = tempToken.get();
}
}
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));
cachedToken = ch.call(new GetToken(owner, tokenRefreshData));
cachedTokens.removeIf(token -> token.getOwner().equalsIgnoreCase(owner));
cachedTokens.add(cachedToken);
LOGGER.log(
Level.FINER,
"Retrieved GitHub App Installation Token for app ID {0} on agent",
Expand Down Expand Up @@ -558,9 +617,11 @@ public Secret getPassword() {
private static final class GetToken
extends SlaveToMasterCallable<AppInstallationToken, RuntimeException> {

private final String owner;
private final String data;

GetToken(String data) {
GetToken(String owner, String data) {
this.owner = owner;
this.data = data;
}

Expand All @@ -578,7 +639,7 @@ public AppInstallationToken call() throws RuntimeException {
(String) fields.get("appID"),
(String) fields.get("privateKey"),
(String) fields.get("apiUri"),
(String) fields.get("owner"));
owner);
LOGGER.log(
Level.FINER,
"Retrieved GitHub App Installation Token for app ID {0} for agent",
Expand Down Expand Up @@ -660,7 +721,8 @@ public FormValidation doTestConnection(
Connector.release(connect);
}
} catch (Exception e) {
return FormValidation.error(e, String.format(ERROR_AUTHENTICATING_GITHUB_APP, appID));
return FormValidation.error(
e, String.format(ERROR_AUTHENTICATING_GITHUB_APP, appID, owner));
}
}
}
Expand Down
Loading