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

Added GitHub signature checking feature + tests #77

Merged
merged 7 commits into from
Jun 19, 2015
35 changes: 35 additions & 0 deletions src/main/java/org/jenkinsci/plugins/ghprb/GhprbRepository.java
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import hudson.model.TaskListener;
import jenkins.model.Jenkins;

import org.apache.commons.codec.binary.Hex;
import org.jenkinsci.plugins.ghprb.extensions.GhprbCommentAppender;
import org.jenkinsci.plugins.ghprb.extensions.GhprbCommitStatusException;
import org.jenkinsci.plugins.ghprb.extensions.GhprbExtension;
Expand All @@ -14,6 +15,9 @@
import org.kohsuke.github.GHEventPayload.IssueComment;
import org.kohsuke.github.GHEventPayload.PullRequest;

import javax.crypto.Mac;
import javax.crypto.spec.SecretKeySpec;

import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.PrintStream;
Expand Down Expand Up @@ -302,6 +306,37 @@ void onPullRequestHook(PullRequest pr) {
GhprbTrigger.getDscp().save();
}

boolean checkSignature(String body, String signature) {
String secret = helper.getTrigger().getSecret();
if (secret != null && ! secret.isEmpty()) {
if (signature != null && signature.startsWith("sha1=")) {
String expected = signature.substring(5);
String algorithm = "HmacSHA1";
try {
SecretKeySpec keySpec = new SecretKeySpec(secret.getBytes(), algorithm);
Mac mac = Mac.getInstance(algorithm);
mac.init(keySpec);
byte[] localSignatureBytes = mac.doFinal(body.getBytes("UTF-8"));
String localSignature = Hex.encodeHexString(localSignatureBytes);
if (! localSignature.equals(expected)) {
logger.log(Level.SEVERE, "Local signature {0} does not match external signature {1}",
new Object[] {localSignature, expected});
return false;
}
} catch (Exception e) {
logger.log(Level.SEVERE, "Couldn't match both signatures");
return false;
}
} else {
logger.log(Level.SEVERE, "Request doesn't contain a signature. Check that github has a secret that should be attached to the hook");
return false;
}
}

logger.log(Level.INFO, "Signatures checking OK");
return true;
}

@VisibleForTesting
void setHelper(Ghprb helper) {
this.helper = helper;
Expand Down
42 changes: 32 additions & 10 deletions src/main/java/org/jenkinsci/plugins/ghprb/GhprbRootAction.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@

import java.io.BufferedReader;
import java.io.IOException;
import java.io.StringReader;
import java.io.UnsupportedEncodingException;
import java.net.URLDecoder;
import java.util.HashSet;
import java.util.Set;
import java.util.logging.Level;
Expand Down Expand Up @@ -47,27 +50,31 @@ public String getUrlName() {

public void doIndex(StaplerRequest req, StaplerResponse resp) {
String event = req.getHeader("X-GitHub-Event");
String signature = req.getHeader("X-Hub-Signature");
String type = req.getContentType();
String payload = null;
String body = null;

if ("application/json".equals(type)) {
BufferedReader br = null;
try {
br = req.getReader();
payload = IOUtils.toString(req.getReader());
} catch (IOException e) {
body = extractRequestBody(req);
if (body == null) {
logger.log(Level.SEVERE, "Can't get request body for application/json.");
return;
} finally {
IOUtils.closeQuietly(br);
}
payload = body;
} else if ("application/x-www-form-urlencoded".equals(type)) {
payload = req.getParameter("payload");
if (payload == null) {
body = extractRequestBody(req);
if (body == null || body.length() <= 8) {
logger.log(Level.SEVERE, "Request doesn't contain payload. "
+ "You're sending url encoded request, so you should pass github payload through 'payload' request parameter");
return;
}
try {
payload = URLDecoder.decode(body.substring(8), req.getCharacterEncoding());
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove this check?

Copy link
Author

Choose a reason for hiding this comment

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

Because the payload is no longer directly taken "as is". Instead, the full body is taken. If the POST is a json then payload == actual json == body, but if it is an encoded URL then the body is something like:
payload=
The reason we discard the payload and we take the body is because the signature has been made by github using the full body, so now I extract the body, and from there I extract the payload.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed anyway. Now it has the old checking, just in case.

} catch (UnsupportedEncodingException e) {
logger.log(Level.SEVERE, "Error while trying to encode the payload");
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be decode, not encode

return;
}
}

if (payload == null) {
Expand All @@ -79,13 +86,28 @@ public void doIndex(StaplerRequest req, StaplerResponse resp) {

for (GhprbWebHook webHook : getWebHooks()) {
try {
webHook.handleWebHook(event, payload);
webHook.handleWebHook(event, payload, body, signature);
} catch (Exception e) {
logger.log(Level.SEVERE, "Unable to process web hook for: " + webHook.getProjectName(), e);
}
}

}

private String extractRequestBody(StaplerRequest req) {
String body = null;
BufferedReader br = null;
try {
br = req.getReader();
body = IOUtils.toString(br);
} catch (IOException e) {
body = null;
} finally {
IOUtils.closeQuietly(br);
}
return body;
}


private Set<GhprbWebHook> getWebHooks() {
final Set<GhprbWebHook> webHooks = new HashSet<GhprbWebHook>();
Expand Down
24 changes: 16 additions & 8 deletions src/main/java/org/jenkinsci/plugins/ghprb/GhprbTrigger.java
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ public class GhprbTrigger extends GhprbTriggerBackwardsCompatible {
private List<GhprbBranch> whiteListTargetBranches;
private transient Ghprb helper;
private String project;
private String secret;
private GhprbGitHubAuth gitHubApiAuth;


Expand Down Expand Up @@ -94,23 +95,25 @@ private void setExtensions(List<GhprbExtension> extensions) {
}

@DataBoundConstructor
public GhprbTrigger(String adminlist,
String whitelist,
String orgslist,
String cron,
String triggerPhrase,

public GhprbTrigger(String adminlist,
String whitelist,
String orgslist,
String cron,
String triggerPhrase,
Boolean onlyTriggerPhrase,
Boolean useGitHubHooks,
Boolean useGitHubHooks,
Boolean permitAll,
Boolean autoCloseFailedPullRequests,
Boolean displayBuildErrorsOnDownstreamBuilds,
Boolean autoCloseFailedPullRequests,
Boolean displayBuildErrorsOnDownstreamBuilds,
String commentFilePath,
List<GhprbBranch> whiteListTargetBranches,
Boolean allowMembersOfWhitelistedOrgsAsAdmin,
String msgSuccess,
String msgFailure,
String commitStatusContext,
String gitHubAuthId,
String secret,
List<GhprbExtension> extensions
) throws ANTLRException {
super(cron);
Expand All @@ -127,6 +130,7 @@ public GhprbTrigger(String adminlist,
this.whiteListTargetBranches = whiteListTargetBranches;
this.gitHubApiAuth = getDescriptor().getGitHubAuth(gitHubAuthId);
this.allowMembersOfWhitelistedOrgsAsAdmin = allowMembersOfWhitelistedOrgsAsAdmin;
this.secret = secret;
setExtensions(extensions);
configVersion = 1;
}
Expand Down Expand Up @@ -357,6 +361,10 @@ public String getCron() {
return cron;
}


public String getSecret() {
return secret;
}
public String getProject() {
return project;
}
Expand Down
8 changes: 5 additions & 3 deletions src/main/java/org/jenkinsci/plugins/ghprb/GhprbWebHook.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ public GhprbWebHook(GhprbTrigger trigger) {
this.trigger = trigger;
}

public void handleWebHook(String event, String payload) {
public void handleWebHook(String event, String payload, String body, String signature) {

GhprbRepository repo = trigger.getRepository();

Expand All @@ -41,7 +41,8 @@ public void handleWebHook(String event, String payload) {
logger.log(Level.INFO, "Checking issue comment ''{0}'' for repo {1}",
new Object[] { issueComment.getComment(), repo.getName() }
);
repo.onIssueCommentHook(issueComment);
if (repo.checkSignature(body, signature))
repo.onIssueCommentHook(issueComment);
}

} else if ("pull_request".equals(event)) {
Expand All @@ -50,7 +51,8 @@ public void handleWebHook(String event, String payload) {
GHEventPayload.PullRequest.class);
if (matchRepo(repo, pr.getPullRequest().getRepository())) {
logger.log(Level.INFO, "Checking PR #{1} for {0}", new Object[] { repo.getName(), pr.getNumber() });
repo.onPullRequestHook(pr);
if (repo.checkSignature(body, signature))
repo.onPullRequestHook(pr);
}

} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,15 @@
<f:checkbox />
</f:entry>
<f:advanced>
<f:entry title="${%Shared secret}" field="secret">
<f:password />
</f:entry>
<f:entry title="${%Success message}" field="msgSuccess">
<f:textarea default="${descriptor.msgSuccess}"/>
</f:entry>
<f:entry title="${%Failure message}" field="msgFailure">
<f:textarea default="${descriptor.msgFailure}"/>
</f:entry>
<f:entry title="${%Trigger phrase}" field="triggerPhrase">
<f:textbox />
</f:entry>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ public class GhprbPullRequestMergeTest {
private final String mergeComment = "merge";

private final Integer pullId = 1;

private Map<String, Object> triggerValues;

@Before
Expand Down
34 changes: 34 additions & 0 deletions src/test/java/org/jenkinsci/plugins/ghprb/GhprbRepositoryTest.java
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
package org.jenkinsci.plugins.ghprb;

import org.apache.commons.codec.binary.Hex;
import org.jenkinsci.plugins.ghprb.extensions.status.GhprbSimpleStatus;
import org.joda.time.DateTime;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
Expand All @@ -22,12 +24,18 @@
import org.mockito.Mockito;
import org.mockito.runners.MockitoJUnitRunner;

import javax.crypto.Mac;
import javax.crypto.spec.SecretKeySpec;
import hudson.model.AbstractProject;
import hudson.model.queue.QueueTaskFuture;

import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.UnsupportedEncodingException;
import java.net.URL;
import java.net.URLEncoder;
import java.security.InvalidKeyException;
import java.security.NoSuchAlgorithmException;
import java.util.ArrayList;
import java.util.Date;
import java.util.List;
Expand Down Expand Up @@ -474,6 +482,32 @@ public void testExceedRateLimit() throws IOException {
verifyZeroInteractions(gt);
}

@Test
public void testSignature() throws IOException, InvalidKeyException, NoSuchAlgorithmException {
String body = URLEncoder.encode("payload=" + GhprbTestUtil.PAYLOAD, "UTF-8");
String actualSecret = "123";
String actualSignature = createSHA1Signature(actualSecret, body);
String fakeSignature = createSHA1Signature("abc", body);

given(helper.getTrigger()).willReturn(trigger);
given(trigger.getSecret()).willReturn(actualSecret);

Assert.assertFalse(actualSignature.equals(fakeSignature));
Assert.assertTrue(ghprbRepository.checkSignature(body, actualSignature));
Assert.assertFalse(ghprbRepository.checkSignature(body, fakeSignature));
}

private String createSHA1Signature(String secret, String body) throws UnsupportedEncodingException, NoSuchAlgorithmException, InvalidKeyException {
String algorithm = "HmacSHA1";
SecretKeySpec keySpec = new SecretKeySpec(secret.getBytes(), algorithm);
Mac mac = Mac.getInstance(algorithm);
mac.init(keySpec);

byte[] signatureBytes = mac.doFinal(body.getBytes("UTF-8"));
String signature = new String(Hex.encodeHexString(signatureBytes));
return "sha1=" + signature;
}

private void initGHPRWithTestData() throws IOException {
/** Mock PR data */
given(ghPullRequest.getUser()).willReturn(ghUser);
Expand Down
Loading