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.

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

[JENKINS-33256] Untrusted PRs #29

Merged
merged 10 commits into from Mar 14, 2016
14 changes: 14 additions & 0 deletions pom.xml
Expand Up @@ -21,6 +21,7 @@

<properties>
<jenkins.version>1.609.3</jenkins.version>
<workflow.version>1.15</workflow.version>
</properties>

<scm>
Expand Down Expand Up @@ -62,6 +63,19 @@
<artifactId>github</artifactId>
<version>1.14.0</version>
</dependency>
<!-- Currently just here for interactive testing via hpi:run: -->
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-aggregator</artifactId>
<version>${workflow.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-multibranch</artifactId>
<version>${workflow.version}</version>
<scope>test</scope>
</dependency>
</dependencies>
<repositories>
<repository>
Expand Down
Expand Up @@ -272,7 +272,6 @@ private void doRetrieve(SCMHeadObserver observer, TaskListener listener, GHRepos
}
listener.getLogger().format("%n %d branches were processed%n", branches);

if (repo.isPrivate()) {
listener.getLogger().format("%n Getting remote pull requests...%n");
int pullrequests = 0;
for (GHPullRequest ghPullRequest : repo.getPullRequests(GHIssueState.OPEN)) {
Expand All @@ -298,17 +297,21 @@ private void doRetrieve(SCMHeadObserver observer, TaskListener listener, GHRepos
continue;
}
}
SCMRevision hash = new SCMRevisionImpl(head, ghPullRequest.getHead().getSha());
String trustedBase = trustedReplacement(repo, ghPullRequest);
SCMRevision hash;
if (trustedBase == null) {
hash = new SCMRevisionImpl(head, ghPullRequest.getHead().getSha());
} else {
listener.getLogger().format(" (not from a trusted source)%n");
hash = new UntrustedPullRequestSCMRevision(head, ghPullRequest.getHead().getSha(), trustedBase);
}
observer.observe(head, hash);
if (!observer.isObserving()) {
return;
}
pullrequests++;
}
listener.getLogger().format("%n %d pull requests were processed%n", pullrequests);
} else {
listener.getLogger().format("%n Skipping pull requests for public repositories%n");
}

}

Expand Down Expand Up @@ -375,12 +378,50 @@ protected SCMRevision doRetrieve(SCMHead head, TaskListener listener, GHReposito
if (head instanceof PullRequestSCMHead) {
int number = ((PullRequestSCMHead) head).getNumber();
ref = repo.getRef("pull/" + number + "/merge");
// getPullRequests makes an extra API call, but we need its current .base.sha
String trustedBase = trustedReplacement(repo, repo.getPullRequest(number));
if (trustedBase != null) {
return new UntrustedPullRequestSCMRevision(head, ref.getObject().getSha(), trustedBase);
}
} else {
ref = repo.getRef("heads/" + head.getName());
}
return new SCMRevisionImpl(head, ref.getObject().getSha());
}

@Override
public SCMRevision getTrustedRevision(SCMRevision revision, TaskListener listener) throws IOException, InterruptedException {
if (revision instanceof UntrustedPullRequestSCMRevision) {
PullRequestSCMHead head = (PullRequestSCMHead) revision.getHead();
UntrustedPullRequestSCMRevision rev = (UntrustedPullRequestSCMRevision) revision;
listener.getLogger().println("Loading trusted files from target branch at " + rev.baseHash + " rather than " + rev.getHash());
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that rev.getHash() here is actually the synthetic merge commit, which is confusing. When I fix this branch source to store precise information in its SCMRevisions, this will need to be rewritten to use two kinds of revisions:

  • origin-branch revisions, consisting simply of a commit hash (SCMRevisionImpl works for this purpose)
  • PR revisions, consisting of a PR head commit plus a base branch commit, and a trusted flag

getTrustedRevision would convert an untrusted PR revision into an origin revision using the base commit, but leave other revisions alone. build on a PR revision would check out the base commit and merge the PR head commit.

Copy link

Choose a reason for hiding this comment

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

So, the 'synthetic' merge commit, is this something that points to a revision with the 'trusted' files replaced on it somehow? I seem to be missing the key part of these 3 PRs that somehow specifies the Jenkinsfile from the trusted branch is the only thing used from that source, rather than the entire commit. The test seems to confirm that's the behavior, as it prints data in the simulated PR branch, but what decides which files need to be trusted / replaced? Or is that something it's already doing elsewhere, unrelated to these PRs?

Copy link
Member Author

Choose a reason for hiding this comment

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

So, the 'synthetic' merge commit, is this something that points to a revision with the 'trusted' files replaced on it somehow?

No, this exists prior to this PR. The comment about the synthetic commit was more for @recena’s benefit; my proposal to fix the way this plugin represents revisions of PRs would involve changes to some of the same code, so it is blocked by this PR in that sense.

I seem to be missing the key part of these 3 PRs that somehow specifies the Jenkinsfile from the trusted branch is the only thing used from that source, rather than the entire commit.

Done here. Compare this, used for checkout scm, which remains unmodified by the PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

When I fix this branch source to store precise information

…which should implicitly fix JENKINS-33237 and make it straightforward to implement JENKINS-33161.

return new SCMRevisionImpl(head, rev.baseHash);
}
return revision;
}

/**
* Evaluates whether this pull request is coming from a trusted source.
* Quickest is to check whether the author of the PR
* <a href="https://developer.github.com/v3/repos/collaborators/#check-if-a-user-is-a-collaborator">is a collaborator of the repository</a>.
* By checking <a href="https://developer.github.com/v3/repos/collaborators/#list-collaborators">all collaborators</a>
* it is possible to further ascertain if they are in a team which was specifically granted push permission,
Copy link
Member Author

Choose a reason for hiding this comment

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

Cf. this tip. To check its effectiveness:

if curl -u YOU:ACCESSTOKEN -s -f https://api.github.com/repos/ORG/REPO/collaborators/SOMEONE; then echo trusted; else echo untrusted; fi

Currently gives a false positive for people in a read-only team (which almost certainly means a private organization). I think this is a low risk; presumably such people are known to the administrator of the organization and would be leaving a clear audit trail if they attempted to file a PR with any kind of malicious content.

Since this API does not return a permissions set, the only way to verify that push permission is granted is to retrieve the full collaborators list and search for the user.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 @jglick I'd like to review the PR this night, please, give some of time before merge.

Copy link
Member Author

Choose a reason for hiding this comment

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

Specifically you can use jq

curl -u YOU:ACCESSTOKEN -s https://api.github.com/repos/ORG/REPO/collaborators | jq '.[] | .login, .permissions.push'

but you need to manually follow Link headers with rel="next".

* but this is potentially expensive as there might be multiple pages of collaborators to retrieve.
* TODO since the GitHub API wrapper currently supports neither, we list all collaborator names and check for membership,
* paying the performance penalty without the benefit of the accuracy.
Copy link
Member Author

Choose a reason for hiding this comment

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

Would like to fix this but it does not seem urgent, and getting a new github-api release including the wrapper plugin is a bit time-consuming.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jglick Have you file as issue to improve that in github-api?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not yet. Can work on it.

* @param ghPullRequest a PR
* @return the base revision, for an untrusted PR; null for a trusted PR
* @see <a href="https://developer.github.com/v3/pulls/#get-a-single-pull-request">PR metadata</a>
* @see <a href="http://stackoverflow.com/questions/15096331/github-api-how-to-find-the-branches-of-a-pull-request#comment54931031_15096596">base revision oddity</a>
*/
private @CheckForNull String trustedReplacement(@Nonnull GHRepository repo, @Nonnull GHPullRequest ghPullRequest) throws IOException {
if (repo.getCollaboratorNames().contains(ghPullRequest.getUser().getLogin())) {
return null;
} else {
return ghPullRequest.getBase().getSha();
}
}

@Extension public static class DescriptorImpl extends SCMSourceDescriptor {

private static final Logger LOGGER = Logger.getLogger(DescriptorImpl.class.getName());
Expand Down
@@ -0,0 +1,52 @@
/*
* The MIT License
*
* Copyright 2016 CloudBees, Inc.
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*/

package org.jenkinsci.plugins.github_branch_source;

import jenkins.plugins.git.AbstractGitSCMSource;
import jenkins.scm.api.SCMHead;

/**
* Revision of a pull request which should load sensitive files from the base branch.
*/
class UntrustedPullRequestSCMRevision extends AbstractGitSCMSource.SCMRevisionImpl {
Copy link
Contributor

Choose a reason for hiding this comment

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

@jglick Why not simplify PullRequestSCMRevision with a field boolean trusted?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no such class. Now as previously mentioned, it might be valuable to create such a class to solve things like JENKINS-33161 and JENKINS-33237 as well as correcting the retrieve overload bug, so it might be advisable to do that now to simplify the settings migration. (It is always possible to deprecate this class and readResolve to something else, but leaves behind a small mess in the code.)


final String baseHash;

UntrustedPullRequestSCMRevision(SCMHead head, String hash, String baseHash) {
super(head, hash);
this.baseHash = baseHash;
}

@Override
public boolean equals(Object o) {
return super.equals(o) && baseHash.equals(((UntrustedPullRequestSCMRevision) o).baseHash);
}

@Override
public int hashCode() {
return super.hashCode(); // good enough
}

}