[JENKINS-48012] Require a X-Hub-Signature header when receiving a hoo…

…k payload and if… (#188) * Require a X-Hub-Signature header when receiving a hook payload and if a secret is configured * Make it clear that the hook signature is only validated if a hook secret is specified in the GitHub plugin config
jenkinsci · Jan 16, 2018 · 8bb18cd · 8bb18cd
1 parent 3c5ad4a
commit 8bb18cd
Show file tree

Hide file tree

Showing 3 changed files with 34 additions and 4 deletions.
diff --git a/src/main/java/org/jenkinsci/plugins/github/webhook/RequirePostWithGHHookPayload.java b/src/main/java/org/jenkinsci/plugins/github/webhook/RequirePostWithGHHookPayload.java
@@ -132,17 +132,20 @@ protected void shouldContainParseablePayload(Object[] arguments) throws Invocati
         }
 
         /**
-         * Checks that an incoming request has a valid signature, if there is specified a signature in the config.
+         * Checks that an incoming request has a valid signature, if a hook secret is specified in the GitHub plugin config.
+         * If no hook secret is configured, then the signature is ignored.
          *
          * @param req Incoming request.
          *
          * @throws InvocationTargetException if any of preconditions is not satisfied
          */
         protected void shouldProvideValidSignature(StaplerRequest req, Object[] args) throws InvocationTargetException {
-            Optional<String> signHeader = Optional.fromNullable(req.getHeader(SIGNATURE_HEADER));
             Secret secret = GitHubPlugin.configuration().getHookSecretConfig().getHookSecret();
 
-            if (signHeader.isPresent() && Optional.fromNullable(secret).isPresent()) {
+            if (Optional.fromNullable(secret).isPresent()) {
+                Optional<String> signHeader = Optional.fromNullable(req.getHeader(SIGNATURE_HEADER));
+                isTrue(signHeader.isPresent(), "Signature was expected, but not provided");
+
                 String digest = substringAfter(signHeader.get(), SHA1_PREFIX);
                 LOGGER.trace("Trying to verify sign from header {}", signHeader.get());
                 isTrue(

diff --git a/src/test/java/org/jenkinsci/plugins/github/test/HookSecretHelper.java b/src/test/java/org/jenkinsci/plugins/github/test/HookSecretHelper.java
@@ -62,4 +62,20 @@ public void run() {
     public static void storeSecret(final String secretText) {
         storeSecretIn(Jenkins.getInstance().getDescriptorByType(GitHubPluginConfig.class), secretText);
     }
+
+    /**
+     * Unsets the current hook secret.
+     *
+     * @param config where to remove
+     */
+    public static void removeSecretIn(GitHubPluginConfig config) {
+        config.getHookSecretConfig().setCredentialsId(null);
+    }
+
+    /**
+     * Unsets the current hook secret.
+     */
+    public static void removeSecret() {
+        removeSecretIn(Jenkins.getInstance().getDescriptorByType(GitHubPluginConfig.class));
+    }
 }
diff --git a/src/test/java/org/jenkinsci/plugins/github/webhook/RequirePostWithGHHookPayloadTest.java b/src/test/java/org/jenkinsci/plugins/github/webhook/RequirePostWithGHHookPayloadTest.java
@@ -19,6 +19,7 @@
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.equalTo;
 import static org.jenkinsci.plugins.github.test.HookSecretHelper.storeSecret;
+import static org.jenkinsci.plugins.github.test.HookSecretHelper.removeSecret;
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.when;
 
@@ -96,7 +97,17 @@ public void shouldNotPassOnLessCountOfArgs() throws Exception {
     }
 
     @Test
-    public void shouldPassOnAbsentSignatureInRequest() throws Exception {
+    @Issue("JENKINS-37481")
+    public void shouldPassOnAbsentSignatureInRequestIfSecretIsNotConfigured() throws Exception {
+        doReturn(PAYLOAD).when(processor).payloadFrom(req, null);
+        removeSecret();
+
+        processor.shouldProvideValidSignature(req, null);
+    }
+
+    @Test(expected = InvocationTargetException.class)
+    @Issue("JENKINS-48012")
+    public void shouldNotPassOnAbsentSignatureInRequest() throws Exception {
         doReturn(PAYLOAD).when(processor).payloadFrom(req, null);
 
         processor.shouldProvideValidSignature(req, null);