From e403039d2dc8c94adea77926d20a964fdf3a7d76 Mon Sep 17 00:00:00 2001 From: mohamedlajmileanix Date: Thu, 8 Aug 2024 16:40:32 +0200 Subject: [PATCH 1/7] CID-2777: Secure webhook listener endpoint - Initial implementation --- config/detekt/detekt.yml | 2 + .../config/GitHubEnterpriseProperties.kt | 1 + .../controllers/GitHubWebhookController.kt | 18 ++----- .../advice/GlobalExceptionHandler.kt | 31 ++++++++++++ .../githubagent/exceptions/Exceptions.kt | 2 + .../services/GitHubWebhookService.kt | 50 +++++++++++++++++++ .../shared/GitHubWebHookEventHelper.kt | 24 +++++++++ src/main/resources/application.yaml | 1 + 8 files changed, 116 insertions(+), 13 deletions(-) create mode 100644 src/main/kotlin/net/leanix/githubagent/controllers/advice/GlobalExceptionHandler.kt create mode 100644 src/main/kotlin/net/leanix/githubagent/services/GitHubWebhookService.kt create mode 100644 src/main/kotlin/net/leanix/githubagent/shared/GitHubWebHookEventHelper.kt diff --git a/config/detekt/detekt.yml b/config/detekt/detekt.yml index 72506d0..403977c 100644 --- a/config/detekt/detekt.yml +++ b/config/detekt/detekt.yml @@ -28,6 +28,8 @@ style: active: false UnusedPrivateMember: active: false + ThrowsCount: + active: false empty-blocks: EmptyFunctionBlock: diff --git a/src/main/kotlin/net/leanix/githubagent/config/GitHubEnterpriseProperties.kt b/src/main/kotlin/net/leanix/githubagent/config/GitHubEnterpriseProperties.kt index 00681ff..bf93f3b 100644 --- a/src/main/kotlin/net/leanix/githubagent/config/GitHubEnterpriseProperties.kt +++ b/src/main/kotlin/net/leanix/githubagent/config/GitHubEnterpriseProperties.kt @@ -8,4 +8,5 @@ data class GitHubEnterpriseProperties( val gitHubAppId: String, val pemFile: String, val manifestFileDirectory: String, + val webhookSecret: String ) diff --git a/src/main/kotlin/net/leanix/githubagent/controllers/GitHubWebhookController.kt b/src/main/kotlin/net/leanix/githubagent/controllers/GitHubWebhookController.kt index ca1c3fe..460d6ca 100644 --- a/src/main/kotlin/net/leanix/githubagent/controllers/GitHubWebhookController.kt +++ b/src/main/kotlin/net/leanix/githubagent/controllers/GitHubWebhookController.kt @@ -1,8 +1,6 @@ package net.leanix.githubagent.controllers -import net.leanix.githubagent.services.WebhookService -import net.leanix.githubagent.shared.SUPPORTED_EVENT_TYPES -import org.slf4j.LoggerFactory +import net.leanix.githubagent.services.GitHubWebhookService import org.springframework.http.HttpStatus import org.springframework.web.bind.annotation.PostMapping import org.springframework.web.bind.annotation.RequestBody @@ -13,22 +11,16 @@ import org.springframework.web.bind.annotation.RestController @RestController @RequestMapping("github") -class GitHubWebhookController(private val webhookService: WebhookService) { - - private val logger = LoggerFactory.getLogger(GitHubWebhookController::class.java) +class GitHubWebhookController(private val gitHubWebhookService: GitHubWebhookService) { @PostMapping("/webhook") @ResponseStatus(HttpStatus.ACCEPTED) fun hook( @RequestHeader("X-Github-Event") eventType: String, + @RequestHeader("X-GitHub-Enterprise-Host") host: String, + @RequestHeader("X-Hub-Signature-256", required = false) signature256: String?, @RequestBody payload: String ) { - runCatching { - if (SUPPORTED_EVENT_TYPES.contains(eventType.uppercase())) { - webhookService.consumeWebhookEvent(eventType, payload) - } else { - logger.warn("Received an unsupported event of type: $eventType") - } - } + gitHubWebhookService.processWebhookEvent(eventType, host, signature256, payload) } } diff --git a/src/main/kotlin/net/leanix/githubagent/controllers/advice/GlobalExceptionHandler.kt b/src/main/kotlin/net/leanix/githubagent/controllers/advice/GlobalExceptionHandler.kt new file mode 100644 index 0000000..83dd6b5 --- /dev/null +++ b/src/main/kotlin/net/leanix/githubagent/controllers/advice/GlobalExceptionHandler.kt @@ -0,0 +1,31 @@ +package net.leanix.githubagent.controllers.advice + +import net.leanix.githubagent.exceptions.InvalidEventSignatureException +import net.leanix.githubagent.exceptions.WebhookSecretNotSetException +import org.slf4j.Logger +import org.slf4j.LoggerFactory +import org.springframework.http.HttpStatus +import org.springframework.http.ProblemDetail +import org.springframework.web.bind.annotation.ControllerAdvice +import org.springframework.web.bind.annotation.ExceptionHandler + +@ControllerAdvice +class GlobalExceptionHandler { + + val exceptionLogger: Logger = LoggerFactory.getLogger(GlobalExceptionHandler::class.java) + + @ExceptionHandler(InvalidEventSignatureException::class) + fun handleInvalidEventSignatureException(exception: InvalidEventSignatureException): ProblemDetail { + val problemDetail = ProblemDetail.forStatusAndDetail(HttpStatus.UNAUTHORIZED, "Invalid event signature") + problemDetail.title = exception.message + exceptionLogger.warn(exception.message) + return problemDetail + } + + @ExceptionHandler(WebhookSecretNotSetException::class) + fun handleWebhookSecretNotSetException(exception: WebhookSecretNotSetException): ProblemDetail { + val problemDetail = ProblemDetail.forStatusAndDetail(HttpStatus.BAD_REQUEST, "Webhook secret not set") + problemDetail.title = exception.message + return problemDetail + } +} diff --git a/src/main/kotlin/net/leanix/githubagent/exceptions/Exceptions.kt b/src/main/kotlin/net/leanix/githubagent/exceptions/Exceptions.kt index a5aa9b7..04bca83 100644 --- a/src/main/kotlin/net/leanix/githubagent/exceptions/Exceptions.kt +++ b/src/main/kotlin/net/leanix/githubagent/exceptions/Exceptions.kt @@ -11,3 +11,5 @@ class UnableToConnectToGitHubEnterpriseException(message: String) : RuntimeExcep class JwtTokenNotFound : RuntimeException("JWT token not found") class GraphQLApiException(errors: List) : RuntimeException("Errors: ${errors.joinToString(separator = "\n") { it.message }}") +class WebhookSecretNotSetException : RuntimeException("Webhook secret not set") +class InvalidEventSignatureException : RuntimeException("Invalid event signature") diff --git a/src/main/kotlin/net/leanix/githubagent/services/GitHubWebhookService.kt b/src/main/kotlin/net/leanix/githubagent/services/GitHubWebhookService.kt new file mode 100644 index 0000000..b082933 --- /dev/null +++ b/src/main/kotlin/net/leanix/githubagent/services/GitHubWebhookService.kt @@ -0,0 +1,50 @@ +package net.leanix.githubagent.services + +import net.leanix.githubagent.config.GitHubEnterpriseProperties +import net.leanix.githubagent.exceptions.InvalidEventSignatureException +import net.leanix.githubagent.exceptions.WebhookSecretNotSetException +import net.leanix.githubagent.shared.SUPPORTED_EVENT_TYPES +import net.leanix.githubagent.shared.hmacSHA256 +import net.leanix.githubagent.shared.timingSafeEqual +import org.slf4j.LoggerFactory +import org.springframework.stereotype.Service + +@Service +class GitHubWebhookService( + private val webhookService: WebhookService, + private val gitHubEnterpriseProperties: GitHubEnterpriseProperties +) { + + private val logger = LoggerFactory.getLogger(GitHubWebhookService::class.java) + private var isWebhookProcessingEnabled = true + + fun processWebhookEvent(eventType: String, host: String, signature256: String?, payload: String) { + if (!isWebhookProcessingEnabled) { + throw WebhookSecretNotSetException() + } + if (!gitHubEnterpriseProperties.baseUrl.contains(host)) { + logger.error("Received a webhook event from an unknown host: $host") + return + } + if (gitHubEnterpriseProperties.webhookSecret == "" && !signature256.isNullOrEmpty()) { + logger.error( + "Event signature is present but webhook secret is not set, " + + "please restart the agent with a valid secret" + ) + isWebhookProcessingEnabled = false + throw WebhookSecretNotSetException() + } + if (gitHubEnterpriseProperties.webhookSecret != "" && !signature256.isNullOrEmpty()) { + val hashedSecret = hmacSHA256(gitHubEnterpriseProperties.webhookSecret, payload) + val isEqual = timingSafeEqual(signature256.removePrefix("sha256="), hashedSecret) + if (!isEqual) throw InvalidEventSignatureException() + } else { + logger.warn("Webhook secret is not set, Skipping signature verification") + } + if (SUPPORTED_EVENT_TYPES.contains(eventType.uppercase())) { + webhookService.consumeWebhookEvent(eventType, payload) + } else { + logger.warn("Received an unsupported event of type: $eventType") + } + } +} diff --git a/src/main/kotlin/net/leanix/githubagent/shared/GitHubWebHookEventHelper.kt b/src/main/kotlin/net/leanix/githubagent/shared/GitHubWebHookEventHelper.kt new file mode 100644 index 0000000..6d03bff --- /dev/null +++ b/src/main/kotlin/net/leanix/githubagent/shared/GitHubWebHookEventHelper.kt @@ -0,0 +1,24 @@ +package net.leanix.githubagent.shared + +import javax.crypto.Mac +import javax.crypto.spec.SecretKeySpec + +fun hmacSHA256(secret: String, data: String): String { + val secretKey = SecretKeySpec(secret.toByteArray(), "HmacSHA256") + val mac = Mac.getInstance("HmacSHA256") + mac.init(secretKey) + val hmacData = mac.doFinal(data.toByteArray()) + return hmacData.joinToString("") { "%02x".format(it) } +} + +fun timingSafeEqual(a: String, b: String): Boolean { + val aBytes = a.toByteArray() + val bBytes = b.toByteArray() + if (aBytes.size != bBytes.size) return false + + var diff = 0 + for (i in aBytes.indices) { + diff = diff or (aBytes[i].toInt() xor bBytes[i].toInt()) + } + return diff == 0 +} diff --git a/src/main/resources/application.yaml b/src/main/resources/application.yaml index 105da24..97f5a43 100644 --- a/src/main/resources/application.yaml +++ b/src/main/resources/application.yaml @@ -3,6 +3,7 @@ github-enterprise: githubAppId: ${GITHUB_APP_ID:} pemFile: ${PEM_FILE:} manifestFileDirectory: ${MANIFEST_FILE_DIRECTORY:} + webhookSecret: ${WEBHOOK_SECRET:} leanix: base-url: https://${LEANIX_DOMAIN}/services ws-base-url: wss://${LEANIX_DOMAIN}/services/git-integrations/v1/git-ws From df14987b23abe82eda63b96a145be086ed39f38c Mon Sep 17 00:00:00 2001 From: mohamedlajmileanix Date: Fri, 9 Aug 2024 10:17:55 +0200 Subject: [PATCH 2/7] CID-2776: fix tests --- .../controllers/GitHubWebhookControllerTest.kt | 12 ++++++++++++ src/test/resources/application.yaml | 1 + 2 files changed, 13 insertions(+) diff --git a/src/test/kotlin/net/leanix/githubagent/controllers/GitHubWebhookControllerTest.kt b/src/test/kotlin/net/leanix/githubagent/controllers/GitHubWebhookControllerTest.kt index 7da7b9e..93193f7 100644 --- a/src/test/kotlin/net/leanix/githubagent/controllers/GitHubWebhookControllerTest.kt +++ b/src/test/kotlin/net/leanix/githubagent/controllers/GitHubWebhookControllerTest.kt @@ -1,8 +1,11 @@ package net.leanix.githubagent.controllers import com.ninjasquad.springmockk.MockkBean +import io.mockk.every import io.mockk.verify +import net.leanix.githubagent.services.GitHubWebhookService import net.leanix.githubagent.services.WebhookService +import org.junit.jupiter.api.BeforeEach import org.junit.jupiter.api.Test import org.mockito.ArgumentMatchers.anyString import org.springframework.beans.factory.annotation.Autowired @@ -20,6 +23,14 @@ class GitHubWebhookControllerTest { @MockkBean private lateinit var webhookService: WebhookService + @MockkBean + private lateinit var gitHubWebhookService: GitHubWebhookService + + @BeforeEach + fun setUp() { + every { gitHubWebhookService.processWebhookEvent(any(), any(), any(), any()) } returns Unit + } + @Test fun `should not process not supported events`() { val eventType = "UNSUPPORTED_EVENT" @@ -28,6 +39,7 @@ class GitHubWebhookControllerTest { mockMvc.perform( MockMvcRequestBuilders.post("/github/webhook") .header("X-GitHub-Event", eventType) + .header("X-GitHub-Enterprise-Host", "host") .content(payload) ) .andExpect(MockMvcResultMatchers.status().isAccepted) diff --git a/src/test/resources/application.yaml b/src/test/resources/application.yaml index 74a4afd..c836586 100644 --- a/src/test/resources/application.yaml +++ b/src/test/resources/application.yaml @@ -3,6 +3,7 @@ github-enterprise: githubAppId: ${GITHUB_APP_ID:dummy} pemFile: ${PEM_FILE:dummy} manifestFileDirectory: ${MANIFEST_FILE_DIRECTORY:} + webhookSecret: ${WEBHOOK_SECRET:} leanix: base-url: https://${LEANIX_DOMAIN:dummy}/services ws-base-url: wss://${LEANIX_DOMAIN:dummy}/services From bd2df7cd4693e575e1f663243b786f382592eb6e Mon Sep 17 00:00:00 2001 From: mohamedlajmileanix Date: Fri, 9 Aug 2024 11:00:21 +0200 Subject: [PATCH 3/7] CID-2776: adapt controller tests and add GitHubWebhookService tests --- .../GitHubWebhookControllerTest.kt | 41 ++++++--- .../services/GitHubWebhookServiceTest.kt | 88 +++++++++++++++++++ 2 files changed, 115 insertions(+), 14 deletions(-) create mode 100644 src/test/kotlin/net/leanix/githubagent/services/GitHubWebhookServiceTest.kt diff --git a/src/test/kotlin/net/leanix/githubagent/controllers/GitHubWebhookControllerTest.kt b/src/test/kotlin/net/leanix/githubagent/controllers/GitHubWebhookControllerTest.kt index 93193f7..91a6955 100644 --- a/src/test/kotlin/net/leanix/githubagent/controllers/GitHubWebhookControllerTest.kt +++ b/src/test/kotlin/net/leanix/githubagent/controllers/GitHubWebhookControllerTest.kt @@ -2,12 +2,9 @@ package net.leanix.githubagent.controllers import com.ninjasquad.springmockk.MockkBean import io.mockk.every -import io.mockk.verify +import net.leanix.githubagent.exceptions.WebhookSecretNotSetException import net.leanix.githubagent.services.GitHubWebhookService -import net.leanix.githubagent.services.WebhookService -import org.junit.jupiter.api.BeforeEach import org.junit.jupiter.api.Test -import org.mockito.ArgumentMatchers.anyString import org.springframework.beans.factory.annotation.Autowired import org.springframework.boot.test.autoconfigure.web.servlet.WebMvcTest import org.springframework.test.web.servlet.MockMvc @@ -20,30 +17,46 @@ class GitHubWebhookControllerTest { @Autowired private lateinit var mockMvc: MockMvc - @MockkBean - private lateinit var webhookService: WebhookService - @MockkBean private lateinit var gitHubWebhookService: GitHubWebhookService - @BeforeEach - fun setUp() { + @Test + fun `should return 202 if webhook event is processed successfully`() { + val eventType = "PUSH" + val payload = "{}" + val host = "valid.host" + every { gitHubWebhookService.processWebhookEvent(any(), any(), any(), any()) } returns Unit + + mockMvc.perform( + MockMvcRequestBuilders.post("/github/webhook") + .header("X-GitHub-Event", eventType) + .header("X-GitHub-Enterprise-Host", host) + .content(payload) + ) + .andExpect(MockMvcResultMatchers.status().isAccepted) } @Test - fun `should not process not supported events`() { + fun `should return 400 if missing webhook secret when event had signature`() { val eventType = "UNSUPPORTED_EVENT" val payload = "{}" + val host = "host" + val signature256 = "sha256=invalidsignature" + + every { + gitHubWebhookService.processWebhookEvent( + eventType, host, signature256, payload + ) + } throws WebhookSecretNotSetException() mockMvc.perform( MockMvcRequestBuilders.post("/github/webhook") .header("X-GitHub-Event", eventType) - .header("X-GitHub-Enterprise-Host", "host") + .header("X-GitHub-Enterprise-Host", host) + .header("X-Hub-Signature-256", signature256) .content(payload) ) - .andExpect(MockMvcResultMatchers.status().isAccepted) - - verify(exactly = 0) { webhookService.consumeWebhookEvent(anyString(), anyString()) } + .andExpect(MockMvcResultMatchers.status().isBadRequest) } } diff --git a/src/test/kotlin/net/leanix/githubagent/services/GitHubWebhookServiceTest.kt b/src/test/kotlin/net/leanix/githubagent/services/GitHubWebhookServiceTest.kt new file mode 100644 index 0000000..d0d289f --- /dev/null +++ b/src/test/kotlin/net/leanix/githubagent/services/GitHubWebhookServiceTest.kt @@ -0,0 +1,88 @@ +package net.leanix.githubagent.services + +import io.mockk.every +import io.mockk.mockk +import io.mockk.verify +import net.leanix.githubagent.config.GitHubEnterpriseProperties +import net.leanix.githubagent.exceptions.InvalidEventSignatureException +import net.leanix.githubagent.exceptions.WebhookSecretNotSetException +import org.junit.jupiter.api.BeforeEach +import org.junit.jupiter.api.Test +import org.junit.jupiter.api.assertThrows +import java.lang.reflect.Field + +class GitHubWebhookServiceTest { + + private val webhookService = mockk() + private val gitHubEnterpriseProperties = mockk() + private val gitHubWebhookService = GitHubWebhookService(webhookService, gitHubEnterpriseProperties) + + @BeforeEach + fun setUp() { + } + + @Test + fun `should throw WebhookSecretNotSetException when webhook processing is disabled`() { + setPrivateField(gitHubWebhookService, "isWebhookProcessingEnabled", false) + + assertThrows { + gitHubWebhookService.processWebhookEvent("PUSH", "host", null, "{}") + } + } + + @Test + fun `should not process event if unknown host`() { + every { gitHubEnterpriseProperties.baseUrl } returns "known.host" + + gitHubWebhookService.processWebhookEvent("PUSH", "unknown.host", null, "{}") + + verify(exactly = 0) { webhookService.consumeWebhookEvent(any(), any()) } + } + + @Test + fun `should throw WebhookSecretNotSetException when signature is present but secret is not set`() { + every { gitHubEnterpriseProperties.baseUrl } returns "known.host" + every { gitHubEnterpriseProperties.webhookSecret } returns "" + + assertThrows { + gitHubWebhookService.processWebhookEvent("PUSH", "known.host", "sha256=signature", "{}") + } + } + + @Test + fun `should throw InvalidEventSignatureException for invalid signature`() { + every { gitHubEnterpriseProperties.baseUrl } returns "known.host" + every { gitHubEnterpriseProperties.webhookSecret } returns "secret" + + assertThrows { + gitHubWebhookService.processWebhookEvent("PUSH", "known.host", "sha256=signature", "{}") + } + } + + @Test + fun `should not process unsupported event type`() { + every { gitHubEnterpriseProperties.baseUrl } returns "known.host" + every { gitHubEnterpriseProperties.webhookSecret } returns "" + + gitHubWebhookService.processWebhookEvent("UNSUPPORTED_EVENT", "known.host", null, "{}") + + verify(exactly = 0) { webhookService.consumeWebhookEvent(any(), any()) } + } + + @Test + fun `should process supported event type successfully`() { + every { gitHubEnterpriseProperties.baseUrl } returns "host" + every { gitHubEnterpriseProperties.webhookSecret } returns "" + every { webhookService.consumeWebhookEvent(any(), any()) } returns Unit + + gitHubWebhookService.processWebhookEvent("PUSH", "host", null, "{}") + + verify { webhookService.consumeWebhookEvent("PUSH", "{}") } + } + + private fun setPrivateField(target: Any, fieldName: String, value: Any) { + val field: Field = target::class.java.getDeclaredField(fieldName) + field.isAccessible = true + field.set(target, value) + } +} From d488c3cca7448d3046e189768f2bfbac5f36969e Mon Sep 17 00:00:00 2001 From: mohamedlajmileanix Date: Fri, 9 Aug 2024 11:10:30 +0200 Subject: [PATCH 4/7] CID-2776: rename classes --- .../controllers/GitHubWebhookController.kt | 6 ++-- ...hookService.kt => GitHubWebhookHandler.kt} | 10 +++---- ...bhookService.kt => WebhookEventService.kt} | 4 +-- .../GitHubWebhookControllerTest.kt | 8 +++--- ...iceTest.kt => GitHubWebhookHandlerTest.kt} | 28 +++++++++---------- ...viceTest.kt => WebhookEventServiceTest.kt} | 10 +++---- 6 files changed, 33 insertions(+), 33 deletions(-) rename src/main/kotlin/net/leanix/githubagent/services/{GitHubWebhookService.kt => GitHubWebhookHandler.kt} (84%) rename src/main/kotlin/net/leanix/githubagent/services/{WebhookService.kt => WebhookEventService.kt} (97%) rename src/test/kotlin/net/leanix/githubagent/services/{GitHubWebhookServiceTest.kt => GitHubWebhookHandlerTest.kt} (66%) rename src/test/kotlin/net/leanix/githubagent/services/{WebhookServiceTest.kt => WebhookEventServiceTest.kt} (92%) diff --git a/src/main/kotlin/net/leanix/githubagent/controllers/GitHubWebhookController.kt b/src/main/kotlin/net/leanix/githubagent/controllers/GitHubWebhookController.kt index 460d6ca..984531f 100644 --- a/src/main/kotlin/net/leanix/githubagent/controllers/GitHubWebhookController.kt +++ b/src/main/kotlin/net/leanix/githubagent/controllers/GitHubWebhookController.kt @@ -1,6 +1,6 @@ package net.leanix.githubagent.controllers -import net.leanix.githubagent.services.GitHubWebhookService +import net.leanix.githubagent.services.GitHubWebhookHandler import org.springframework.http.HttpStatus import org.springframework.web.bind.annotation.PostMapping import org.springframework.web.bind.annotation.RequestBody @@ -11,7 +11,7 @@ import org.springframework.web.bind.annotation.RestController @RestController @RequestMapping("github") -class GitHubWebhookController(private val gitHubWebhookService: GitHubWebhookService) { +class GitHubWebhookController(private val gitHubWebhookHandler: GitHubWebhookHandler) { @PostMapping("/webhook") @ResponseStatus(HttpStatus.ACCEPTED) @@ -21,6 +21,6 @@ class GitHubWebhookController(private val gitHubWebhookService: GitHubWebhookSer @RequestHeader("X-Hub-Signature-256", required = false) signature256: String?, @RequestBody payload: String ) { - gitHubWebhookService.processWebhookEvent(eventType, host, signature256, payload) + gitHubWebhookHandler.handleWebhookEvent(eventType, host, signature256, payload) } } diff --git a/src/main/kotlin/net/leanix/githubagent/services/GitHubWebhookService.kt b/src/main/kotlin/net/leanix/githubagent/services/GitHubWebhookHandler.kt similarity index 84% rename from src/main/kotlin/net/leanix/githubagent/services/GitHubWebhookService.kt rename to src/main/kotlin/net/leanix/githubagent/services/GitHubWebhookHandler.kt index b082933..8f990af 100644 --- a/src/main/kotlin/net/leanix/githubagent/services/GitHubWebhookService.kt +++ b/src/main/kotlin/net/leanix/githubagent/services/GitHubWebhookHandler.kt @@ -10,15 +10,15 @@ import org.slf4j.LoggerFactory import org.springframework.stereotype.Service @Service -class GitHubWebhookService( - private val webhookService: WebhookService, +class GitHubWebhookHandler( + private val webhookEventService: WebhookEventService, private val gitHubEnterpriseProperties: GitHubEnterpriseProperties ) { - private val logger = LoggerFactory.getLogger(GitHubWebhookService::class.java) + private val logger = LoggerFactory.getLogger(GitHubWebhookHandler::class.java) private var isWebhookProcessingEnabled = true - fun processWebhookEvent(eventType: String, host: String, signature256: String?, payload: String) { + fun handleWebhookEvent(eventType: String, host: String, signature256: String?, payload: String) { if (!isWebhookProcessingEnabled) { throw WebhookSecretNotSetException() } @@ -42,7 +42,7 @@ class GitHubWebhookService( logger.warn("Webhook secret is not set, Skipping signature verification") } if (SUPPORTED_EVENT_TYPES.contains(eventType.uppercase())) { - webhookService.consumeWebhookEvent(eventType, payload) + webhookEventService.consumeWebhookEvent(eventType, payload) } else { logger.warn("Received an unsupported event of type: $eventType") } diff --git a/src/main/kotlin/net/leanix/githubagent/services/WebhookService.kt b/src/main/kotlin/net/leanix/githubagent/services/WebhookEventService.kt similarity index 97% rename from src/main/kotlin/net/leanix/githubagent/services/WebhookService.kt rename to src/main/kotlin/net/leanix/githubagent/services/WebhookEventService.kt index 7966e23..8144153 100644 --- a/src/main/kotlin/net/leanix/githubagent/services/WebhookService.kt +++ b/src/main/kotlin/net/leanix/githubagent/services/WebhookEventService.kt @@ -11,7 +11,7 @@ import org.slf4j.LoggerFactory import org.springframework.stereotype.Service @Service -class WebhookService( +class WebhookEventService( private val webSocketService: WebSocketService, private val gitHubGraphQLService: GitHubGraphQLService, private val gitHubEnterpriseProperties: GitHubEnterpriseProperties, @@ -19,7 +19,7 @@ class WebhookService( private val gitHubAuthenticationService: GitHubAuthenticationService ) { - private val logger = LoggerFactory.getLogger(WebhookService::class.java) + private val logger = LoggerFactory.getLogger(WebhookEventService::class.java) private val objectMapper = jacksonObjectMapper() fun consumeWebhookEvent(eventType: String, payload: String) { diff --git a/src/test/kotlin/net/leanix/githubagent/controllers/GitHubWebhookControllerTest.kt b/src/test/kotlin/net/leanix/githubagent/controllers/GitHubWebhookControllerTest.kt index 91a6955..b68320d 100644 --- a/src/test/kotlin/net/leanix/githubagent/controllers/GitHubWebhookControllerTest.kt +++ b/src/test/kotlin/net/leanix/githubagent/controllers/GitHubWebhookControllerTest.kt @@ -3,7 +3,7 @@ package net.leanix.githubagent.controllers import com.ninjasquad.springmockk.MockkBean import io.mockk.every import net.leanix.githubagent.exceptions.WebhookSecretNotSetException -import net.leanix.githubagent.services.GitHubWebhookService +import net.leanix.githubagent.services.GitHubWebhookHandler import org.junit.jupiter.api.Test import org.springframework.beans.factory.annotation.Autowired import org.springframework.boot.test.autoconfigure.web.servlet.WebMvcTest @@ -18,7 +18,7 @@ class GitHubWebhookControllerTest { private lateinit var mockMvc: MockMvc @MockkBean - private lateinit var gitHubWebhookService: GitHubWebhookService + private lateinit var gitHubWebhookHandler: GitHubWebhookHandler @Test fun `should return 202 if webhook event is processed successfully`() { @@ -26,7 +26,7 @@ class GitHubWebhookControllerTest { val payload = "{}" val host = "valid.host" - every { gitHubWebhookService.processWebhookEvent(any(), any(), any(), any()) } returns Unit + every { gitHubWebhookHandler.handleWebhookEvent(any(), any(), any(), any()) } returns Unit mockMvc.perform( MockMvcRequestBuilders.post("/github/webhook") @@ -45,7 +45,7 @@ class GitHubWebhookControllerTest { val signature256 = "sha256=invalidsignature" every { - gitHubWebhookService.processWebhookEvent( + gitHubWebhookHandler.handleWebhookEvent( eventType, host, signature256, payload ) } throws WebhookSecretNotSetException() diff --git a/src/test/kotlin/net/leanix/githubagent/services/GitHubWebhookServiceTest.kt b/src/test/kotlin/net/leanix/githubagent/services/GitHubWebhookHandlerTest.kt similarity index 66% rename from src/test/kotlin/net/leanix/githubagent/services/GitHubWebhookServiceTest.kt rename to src/test/kotlin/net/leanix/githubagent/services/GitHubWebhookHandlerTest.kt index d0d289f..410e2d5 100644 --- a/src/test/kotlin/net/leanix/githubagent/services/GitHubWebhookServiceTest.kt +++ b/src/test/kotlin/net/leanix/githubagent/services/GitHubWebhookHandlerTest.kt @@ -11,11 +11,11 @@ import org.junit.jupiter.api.Test import org.junit.jupiter.api.assertThrows import java.lang.reflect.Field -class GitHubWebhookServiceTest { +class GitHubWebhookHandlerTest { - private val webhookService = mockk() + private val webhookEventService = mockk() private val gitHubEnterpriseProperties = mockk() - private val gitHubWebhookService = GitHubWebhookService(webhookService, gitHubEnterpriseProperties) + private val gitHubWebhookHandler = GitHubWebhookHandler(webhookEventService, gitHubEnterpriseProperties) @BeforeEach fun setUp() { @@ -23,10 +23,10 @@ class GitHubWebhookServiceTest { @Test fun `should throw WebhookSecretNotSetException when webhook processing is disabled`() { - setPrivateField(gitHubWebhookService, "isWebhookProcessingEnabled", false) + setPrivateField(gitHubWebhookHandler, "isWebhookProcessingEnabled", false) assertThrows { - gitHubWebhookService.processWebhookEvent("PUSH", "host", null, "{}") + gitHubWebhookHandler.handleWebhookEvent("PUSH", "host", null, "{}") } } @@ -34,9 +34,9 @@ class GitHubWebhookServiceTest { fun `should not process event if unknown host`() { every { gitHubEnterpriseProperties.baseUrl } returns "known.host" - gitHubWebhookService.processWebhookEvent("PUSH", "unknown.host", null, "{}") + gitHubWebhookHandler.handleWebhookEvent("PUSH", "unknown.host", null, "{}") - verify(exactly = 0) { webhookService.consumeWebhookEvent(any(), any()) } + verify(exactly = 0) { webhookEventService.consumeWebhookEvent(any(), any()) } } @Test @@ -45,7 +45,7 @@ class GitHubWebhookServiceTest { every { gitHubEnterpriseProperties.webhookSecret } returns "" assertThrows { - gitHubWebhookService.processWebhookEvent("PUSH", "known.host", "sha256=signature", "{}") + gitHubWebhookHandler.handleWebhookEvent("PUSH", "known.host", "sha256=signature", "{}") } } @@ -55,7 +55,7 @@ class GitHubWebhookServiceTest { every { gitHubEnterpriseProperties.webhookSecret } returns "secret" assertThrows { - gitHubWebhookService.processWebhookEvent("PUSH", "known.host", "sha256=signature", "{}") + gitHubWebhookHandler.handleWebhookEvent("PUSH", "known.host", "sha256=signature", "{}") } } @@ -64,20 +64,20 @@ class GitHubWebhookServiceTest { every { gitHubEnterpriseProperties.baseUrl } returns "known.host" every { gitHubEnterpriseProperties.webhookSecret } returns "" - gitHubWebhookService.processWebhookEvent("UNSUPPORTED_EVENT", "known.host", null, "{}") + gitHubWebhookHandler.handleWebhookEvent("UNSUPPORTED_EVENT", "known.host", null, "{}") - verify(exactly = 0) { webhookService.consumeWebhookEvent(any(), any()) } + verify(exactly = 0) { webhookEventService.consumeWebhookEvent(any(), any()) } } @Test fun `should process supported event type successfully`() { every { gitHubEnterpriseProperties.baseUrl } returns "host" every { gitHubEnterpriseProperties.webhookSecret } returns "" - every { webhookService.consumeWebhookEvent(any(), any()) } returns Unit + every { webhookEventService.consumeWebhookEvent(any(), any()) } returns Unit - gitHubWebhookService.processWebhookEvent("PUSH", "host", null, "{}") + gitHubWebhookHandler.handleWebhookEvent("PUSH", "host", null, "{}") - verify { webhookService.consumeWebhookEvent("PUSH", "{}") } + verify { webhookEventService.consumeWebhookEvent("PUSH", "{}") } } private fun setPrivateField(target: Any, fieldName: String, value: Any) { diff --git a/src/test/kotlin/net/leanix/githubagent/services/WebhookServiceTest.kt b/src/test/kotlin/net/leanix/githubagent/services/WebhookEventServiceTest.kt similarity index 92% rename from src/test/kotlin/net/leanix/githubagent/services/WebhookServiceTest.kt rename to src/test/kotlin/net/leanix/githubagent/services/WebhookEventServiceTest.kt index 1537294..fe41c46 100644 --- a/src/test/kotlin/net/leanix/githubagent/services/WebhookServiceTest.kt +++ b/src/test/kotlin/net/leanix/githubagent/services/WebhookEventServiceTest.kt @@ -13,7 +13,7 @@ import org.springframework.test.context.ActiveProfiles @SpringBootTest @ActiveProfiles("test") -class WebhookServiceTest { +class WebhookEventServiceTest { @MockkBean private lateinit var webSocketService: WebSocketService @@ -28,7 +28,7 @@ class WebhookServiceTest { private lateinit var gitHubAuthenticationService: GitHubAuthenticationService @Autowired - private lateinit var webhookService: WebhookService + private lateinit var webhookEventService: WebhookEventService @BeforeEach fun setUp() { @@ -56,7 +56,7 @@ class WebhookServiceTest { every { cachingService.get("installationToken:1") } returns null andThen "token" - webhookService.consumeWebhookEvent("PUSH", payload) + webhookEventService.consumeWebhookEvent("PUSH", payload) verify(exactly = 1) { gitHubAuthenticationService.refreshTokens() } } @@ -82,7 +82,7 @@ class WebhookServiceTest { "ref": "refs/heads/main" }""" - webhookService.consumeWebhookEvent("PUSH", payload) + webhookEventService.consumeWebhookEvent("PUSH", payload) verify(exactly = 1) { webSocketService.sendMessage( @@ -114,7 +114,7 @@ class WebhookServiceTest { "ref": "refs/heads/main" }""" - webhookService.consumeWebhookEvent("OTHER", payload) + webhookEventService.consumeWebhookEvent("OTHER", payload) verify(exactly = 1) { webSocketService.sendMessage("/events/other", payload) } } From 68162df7a4fb02d13ca73716130638fd92acbdcf Mon Sep 17 00:00:00 2001 From: mohamedlajmileanix Date: Fri, 9 Aug 2024 13:12:56 +0200 Subject: [PATCH 5/7] CID-2776: addressing PR comments --- .../net/leanix/githubagent/services/GitHubWebhookHandler.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/kotlin/net/leanix/githubagent/services/GitHubWebhookHandler.kt b/src/main/kotlin/net/leanix/githubagent/services/GitHubWebhookHandler.kt index 8f990af..862e00a 100644 --- a/src/main/kotlin/net/leanix/githubagent/services/GitHubWebhookHandler.kt +++ b/src/main/kotlin/net/leanix/githubagent/services/GitHubWebhookHandler.kt @@ -26,7 +26,7 @@ class GitHubWebhookHandler( logger.error("Received a webhook event from an unknown host: $host") return } - if (gitHubEnterpriseProperties.webhookSecret == "" && !signature256.isNullOrEmpty()) { + if (gitHubEnterpriseProperties.webhookSecret.isBlank() && signature256 != null) { logger.error( "Event signature is present but webhook secret is not set, " + "please restart the agent with a valid secret" @@ -34,7 +34,7 @@ class GitHubWebhookHandler( isWebhookProcessingEnabled = false throw WebhookSecretNotSetException() } - if (gitHubEnterpriseProperties.webhookSecret != "" && !signature256.isNullOrEmpty()) { + if (gitHubEnterpriseProperties.webhookSecret.isNotBlank() && signature256 != null) { val hashedSecret = hmacSHA256(gitHubEnterpriseProperties.webhookSecret, payload) val isEqual = timingSafeEqual(signature256.removePrefix("sha256="), hashedSecret) if (!isEqual) throw InvalidEventSignatureException() From c50106da497401cb72edfc01f20ad67c74b9bcac Mon Sep 17 00:00:00 2001 From: mohamedlajmileanix Date: Fri, 9 Aug 2024 13:33:54 +0200 Subject: [PATCH 6/7] CID-2776: removing logic for disabling webhook listener endpoint --- .../services/GitHubWebhookHandler.kt | 42 +++++++++---------- .../services/GitHubWebhookHandlerTest.kt | 16 ------- 2 files changed, 19 insertions(+), 39 deletions(-) diff --git a/src/main/kotlin/net/leanix/githubagent/services/GitHubWebhookHandler.kt b/src/main/kotlin/net/leanix/githubagent/services/GitHubWebhookHandler.kt index 862e00a..d042407 100644 --- a/src/main/kotlin/net/leanix/githubagent/services/GitHubWebhookHandler.kt +++ b/src/main/kotlin/net/leanix/githubagent/services/GitHubWebhookHandler.kt @@ -16,32 +16,28 @@ class GitHubWebhookHandler( ) { private val logger = LoggerFactory.getLogger(GitHubWebhookHandler::class.java) - private var isWebhookProcessingEnabled = true fun handleWebhookEvent(eventType: String, host: String, signature256: String?, payload: String) { - if (!isWebhookProcessingEnabled) { - throw WebhookSecretNotSetException() - } - if (!gitHubEnterpriseProperties.baseUrl.contains(host)) { - logger.error("Received a webhook event from an unknown host: $host") - return - } - if (gitHubEnterpriseProperties.webhookSecret.isBlank() && signature256 != null) { - logger.error( - "Event signature is present but webhook secret is not set, " + - "please restart the agent with a valid secret" - ) - isWebhookProcessingEnabled = false - throw WebhookSecretNotSetException() - } - if (gitHubEnterpriseProperties.webhookSecret.isNotBlank() && signature256 != null) { - val hashedSecret = hmacSHA256(gitHubEnterpriseProperties.webhookSecret, payload) - val isEqual = timingSafeEqual(signature256.removePrefix("sha256="), hashedSecret) - if (!isEqual) throw InvalidEventSignatureException() - } else { - logger.warn("Webhook secret is not set, Skipping signature verification") - } if (SUPPORTED_EVENT_TYPES.contains(eventType.uppercase())) { + if (!gitHubEnterpriseProperties.baseUrl.contains(host)) { + logger.error("Received a webhook event from an unknown host: $host") + return + } + if (gitHubEnterpriseProperties.webhookSecret.isBlank() && signature256 != null) { + logger.error( + "Event signature is present but webhook secret is not set, " + + "please restart the agent with a valid secret, " + + "or remove the secret from the GitHub App settings." + ) + throw WebhookSecretNotSetException() + } + if (gitHubEnterpriseProperties.webhookSecret.isNotBlank() && signature256 != null) { + val hashedSecret = hmacSHA256(gitHubEnterpriseProperties.webhookSecret, payload) + val isEqual = timingSafeEqual(signature256.removePrefix("sha256="), hashedSecret) + if (!isEqual) throw InvalidEventSignatureException() + } else { + logger.warn("Webhook secret is not set, Skipping signature verification") + } webhookEventService.consumeWebhookEvent(eventType, payload) } else { logger.warn("Received an unsupported event of type: $eventType") diff --git a/src/test/kotlin/net/leanix/githubagent/services/GitHubWebhookHandlerTest.kt b/src/test/kotlin/net/leanix/githubagent/services/GitHubWebhookHandlerTest.kt index 410e2d5..7b24c48 100644 --- a/src/test/kotlin/net/leanix/githubagent/services/GitHubWebhookHandlerTest.kt +++ b/src/test/kotlin/net/leanix/githubagent/services/GitHubWebhookHandlerTest.kt @@ -9,7 +9,6 @@ import net.leanix.githubagent.exceptions.WebhookSecretNotSetException import org.junit.jupiter.api.BeforeEach import org.junit.jupiter.api.Test import org.junit.jupiter.api.assertThrows -import java.lang.reflect.Field class GitHubWebhookHandlerTest { @@ -21,15 +20,6 @@ class GitHubWebhookHandlerTest { fun setUp() { } - @Test - fun `should throw WebhookSecretNotSetException when webhook processing is disabled`() { - setPrivateField(gitHubWebhookHandler, "isWebhookProcessingEnabled", false) - - assertThrows { - gitHubWebhookHandler.handleWebhookEvent("PUSH", "host", null, "{}") - } - } - @Test fun `should not process event if unknown host`() { every { gitHubEnterpriseProperties.baseUrl } returns "known.host" @@ -79,10 +69,4 @@ class GitHubWebhookHandlerTest { verify { webhookEventService.consumeWebhookEvent("PUSH", "{}") } } - - private fun setPrivateField(target: Any, fieldName: String, value: Any) { - val field: Field = target::class.java.getDeclaredField(fieldName) - field.isAccessible = true - field.set(target, value) - } } From 011646185567944c6fd047b4f2cdbd90d8b0ffba Mon Sep 17 00:00:00 2001 From: mohamedlajmileanix Date: Fri, 9 Aug 2024 13:42:51 +0200 Subject: [PATCH 7/7] CID-2776: update README.md --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index 7df230a..91abfa3 100644 --- a/README.md +++ b/README.md @@ -28,6 +28,7 @@ The SAP LeanIX agent discovers self-built software in self-hosted GitHub Enterpr - `GITHUB_APP_ID`: The ID of your GitHub App. - `PEM_FILE`: The path to your GitHub App's PEM file inside the Docker container. - `MANIFEST_FILE_DIRECTORY`: The directory path where the manifest files are stored in each repository. Manifest files are crucial for microservice discovery as they provide essential information about the service. For more information, see [Microservice Discovery Through a Manifest File](https://docs-eam.leanix.net/reference/microservice-discovery-manifest-file) in our documentation. + - `WEBHOOK_SECRET`: The secret used to validate incoming webhook events from GitHub. (Optional, but recommended. [Needs to be set in the GitHub App settings first](https://docs.github.com/en/enterprise-server@3.8/webhooks/using-webhooks/validating-webhook-deliveries).) 5. **Start the agent**: To start the agent, run the following Docker command. Replace the variables in angle brackets with your actual values. @@ -38,6 +39,7 @@ The SAP LeanIX agent discovers self-built software in self-hosted GitHub Enterpr -e GITHUB_APP_ID= \ -e PEM_FILE=/privateKey.pem \ -e MANIFEST_FILE_DIRECTORY= \ + -e WEBHOOK_SECRET= \ leanix-github-agent ```