diff --git a/pom.xml b/pom.xml index d8a5170c3..13bcf4108 100644 --- a/pom.xml +++ b/pom.xml @@ -14,7 +14,7 @@ hpi - 1.5.36 + 1.5.37 -SNAPSHOT 2.346.3 High diff --git a/src/main/java/com/dabsquared/gitlabjenkins/webhook/build/BuildWebHookAction.java b/src/main/java/com/dabsquared/gitlabjenkins/webhook/build/BuildWebHookAction.java index e39ef5cba..253f7ec18 100644 --- a/src/main/java/com/dabsquared/gitlabjenkins/webhook/build/BuildWebHookAction.java +++ b/src/main/java/com/dabsquared/gitlabjenkins/webhook/build/BuildWebHookAction.java @@ -1,10 +1,14 @@ package com.dabsquared.gitlabjenkins.webhook.build; +import java.nio.charset.StandardCharsets; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; import java.util.logging.Logger; +import edu.umd.cs.findbugs.annotations.NonNull; + import hudson.model.Item; import hudson.model.Job; -import hudson.security.Messages; import hudson.security.Permission; import hudson.util.HttpResponses; import jenkins.model.Jenkins; @@ -34,21 +38,51 @@ public final void execute(StaplerResponse response) { protected abstract static class TriggerNotifier implements Runnable { private final Item project; - private final String secretToken; + private final byte[] hashedSecretToken; private final Authentication authentication; public TriggerNotifier(Item project, String secretToken, Authentication authentication) { this.project = project; - this.secretToken = secretToken; + /* secretToken may be null, but we want constant time comparison of tokens */ + /* Remember secretToken was passed as null, then handle it as non-matchinng later */ + this.hashedSecretToken = secretToken != null ? hashedBytes(secretToken) : null; this.authentication = authentication; } + @NonNull + private static byte[] hashedBytes(@NonNull String token) { + final String HASH_ALGORITHM = "SHA-256"; + try { + MessageDigest digest = MessageDigest.getInstance(HASH_ALGORITHM); + return digest.digest(token.getBytes(StandardCharsets.UTF_8)); + } catch (NoSuchAlgorithmException e) { + throw new AssertionError("Hash algorithm " + HASH_ALGORITHM + " not found", e); + } + } + + /* Constant time comparison of token argument and secretToken that was + * passed to the constructor. If a null secretToken was passed to the + * constructor, this method must still perform constant time comparison. + */ + private boolean tokenMatches(@NonNull String token) { + byte[] tokenBytes = hashedBytes(token); + if (hashedSecretToken != null) { + return MessageDigest.isEqual(tokenBytes, hashedSecretToken); + } + + // assure the isEqual comparison compares same number of bytes + byte [] secretTokenBytes = tokenBytes.clone(); + // change last byte to assure the isEqual comparison will not match + secretTokenBytes[secretTokenBytes.length - 1] ^= 1 << 3; + return MessageDigest.isEqual(tokenBytes, secretTokenBytes); + } + public void run() { GitLabPushTrigger trigger = GitLabPushTrigger.getFromJob((Job) project); if (trigger != null) { if (StringUtils.isEmpty(trigger.getSecretToken())) { checkPermission(Item.BUILD, project); - } else if (!StringUtils.equals(trigger.getSecretToken(), secretToken)) { + } else if (!tokenMatches(trigger.getSecretToken())) { throw HttpResponses.errorWithoutStack(401, "Invalid token"); } performOnPost(trigger); diff --git a/src/test/java/com/dabsquared/gitlabjenkins/webhook/build/BuildWebHookActionTest.java b/src/test/java/com/dabsquared/gitlabjenkins/webhook/build/BuildWebHookActionTest.java new file mode 100644 index 000000000..f0f6bcfd9 --- /dev/null +++ b/src/test/java/com/dabsquared/gitlabjenkins/webhook/build/BuildWebHookActionTest.java @@ -0,0 +1,149 @@ +/* + * Test tokenMatches in the BuildWebHookAction class + * Author: Mark Waite + */ +package com.dabsquared.gitlabjenkins.webhook.build; + +import com.dabsquared.gitlabjenkins.connection.GitLabConnectionConfig; +import com.dabsquared.gitlabjenkins.GitLabPushTrigger; + +import edu.umd.cs.findbugs.annotations.NonNull; + +import hudson.model.FreeStyleProject; +import hudson.model.Item; +import hudson.model.Project; +import hudson.security.ACL; + +import org.acegisecurity.Authentication; + +import org.junit.Before; +import org.junit.Test; +import org.junit.Rule; +import org.jvnet.hudson.test.JenkinsRule; + +import org.kohsuke.stapler.HttpResponses.HttpResponseException; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; + +/** + * Test the BuildWebHookAction class + * + * @author Mark Waite + */ +public class BuildWebHookActionTest { + + @Rule + public JenkinsRule j = new JenkinsRule(); + + private FreeStyleProject project; + private GitLabPushTrigger trigger; + + public BuildWebHookActionTest() { + } + + @Before + public void confgureGitLabConnection() throws Exception { + j.get(GitLabConnectionConfig.class).setUseAuthenticatedEndpoint(true); + } + + @Before + public void createFreeStyleProjectWithGitLabTrigger() throws Exception { + project = j.createFreeStyleProject(); + trigger = new GitLabPushTrigger(); + project.addTrigger(trigger); + } + + // trigger token == action token, expected to succeed + @Test + public void testNotifierTokenMatches() throws Exception { + String triggerToken = "testNotifierTokenMatches-token"; + trigger.setSecretToken(triggerToken); + String actionToken = triggerToken; + BuildWebHookActionImpl action = new BuildWebHookActionImpl(project, actionToken); + action.runNotifier(); + assertTrue("performOnPost not called, token did not match?", action.performOnPostCalled); + } + + // trigger token != action token, expected to throw an exception + @Test + public void testNotifierTokenDoesNotMatchString() throws Exception { + String triggerToken = "testNotifierTokenDoesNotMatchString-token"; + trigger.setSecretToken(triggerToken); + String actionToken = triggerToken + "-no-match"; // Won't match + BuildWebHookActionImpl action = new BuildWebHookActionImpl(project, actionToken); + assertThrows(HttpResponseException.class, + () -> { + action.runNotifier(); + } + ); + assertFalse("performOnPost was called, unexpected token match?", action.performOnPostCalled); + } + + // trigger token != null action token, expected to throw an exception + @Test + public void testNotifierTokenDoesNotMatchNull() throws Exception { + String triggerToken = "testNotifierTokenDoesNotMatchNull-token"; + trigger.setSecretToken(triggerToken); + String actionToken = null; + BuildWebHookActionImpl action = new BuildWebHookActionImpl(project, actionToken); + assertThrows(HttpResponseException.class, + () -> { + action.runNotifier(); + } + ); + assertFalse("performOnPost was called, unexpected token match?", action.performOnPostCalled); + } + + // null trigger token != action token, expected to succeed + @Test + public void testNullNotifierTokenAllowsAccess() throws Exception { + // String triggerToken = null; + // trigger.setSecretToken(triggerToken); + String actionToken = "testNullNotifierTokenAllowsAccess-token"; + BuildWebHookActionImpl action = new BuildWebHookActionImpl(project, actionToken); + action.runNotifier(); + assertTrue("performOnPost not called, token did not match?", action.performOnPostCalled); + } + + public class BuildWebHookActionImpl extends BuildWebHookAction { + + // Used for the assertion that tokenMatches() returned true + public boolean performOnPostCalled = false; + + private final MyTriggerNotifier myNotifier; + + public BuildWebHookActionImpl() { + myNotifier = new MyTriggerNotifier(null, null, null); + } + + public BuildWebHookActionImpl(@NonNull Project project, @NonNull String token) { + myNotifier = new MyTriggerNotifier(project, token, ACL.SYSTEM); + } + + public void runNotifier() { + myNotifier.run(); + } + + public class MyTriggerNotifier extends TriggerNotifier { + + public MyTriggerNotifier(Item project, String secretToken, Authentication authentication) { + super(project, secretToken, authentication); + } + + @Override + protected void performOnPost(GitLabPushTrigger trigger) { + performOnPostCalled = true; + } + } + + @Override + public void processForCompatibility() { + } + + @Override + public void execute() { + } + } +}