From a959f4fbd823624d2f55741033cc3d74a5e203b4 Mon Sep 17 00:00:00 2001 From: Luis Fernando Pollo <1323478+luispollo@users.noreply.github.com> Date: Wed, 12 Feb 2020 19:06:57 -0800 Subject: [PATCH] fix(slack): Left-over fixes from interactive notifications (#774) * fix(slack): Fix and enable signature verification for Slack callbacks * fix(slack): Remove token verification in favor of signature verification * fix(slack): Fix left-over refactored method call * chore(log): More debug logs * fix(test): Fix interactive notification tests --- .../SlackNotificationAgent.groovy | 2 +- .../echo/slack/SlackAppService.groovy | 24 ++++++++----- ...SlackInteractiveNotificationService.groovy | 5 ++- .../slack/SlackNotificationService.groovy | 1 + ...kInteractiveNotificationServiceSpec.groovy | 14 ++++---- .../echo/slack/SlackServiceSpec.groovy | 34 +++++++++++++++++++ 6 files changed, 60 insertions(+), 20 deletions(-) diff --git a/echo-notifications/src/main/groovy/com/netflix/spinnaker/echo/notification/SlackNotificationAgent.groovy b/echo-notifications/src/main/groovy/com/netflix/spinnaker/echo/notification/SlackNotificationAgent.groovy index f15dace9e..5b6bfc220 100644 --- a/echo-notifications/src/main/groovy/com/netflix/spinnaker/echo/notification/SlackNotificationAgent.groovy +++ b/echo-notifications/src/main/groovy/com/netflix/spinnaker/echo/notification/SlackNotificationAgent.groovy @@ -103,7 +103,7 @@ class SlackNotificationAgent extends AbstractEventNotificationAgent { Response response if (sendCompactMessages) { - response = slackService.sendCompactMessage(token, new CompactSlackMessage(body, color), address, true) + response = slackService.sendCompactMessage(new CompactSlackMessage(body, color), address, true) } else { String title = getNotificationTitle(config.type, application, status) response = slackService.sendMessage(new SlackAttachment(title, body, color), address, true) diff --git a/echo-notifications/src/main/groovy/com/netflix/spinnaker/echo/slack/SlackAppService.groovy b/echo-notifications/src/main/groovy/com/netflix/spinnaker/echo/slack/SlackAppService.groovy index 22f4276e0..4181922b7 100644 --- a/echo-notifications/src/main/groovy/com/netflix/spinnaker/echo/slack/SlackAppService.groovy +++ b/echo-notifications/src/main/groovy/com/netflix/spinnaker/echo/slack/SlackAppService.groovy @@ -22,7 +22,7 @@ import groovy.transform.Canonical import groovy.util.logging.Slf4j import org.springframework.http.HttpHeaders import org.springframework.http.RequestEntity -import org.springframework.security.crypto.codec.Hex +import org.apache.commons.codec.binary.Hex import javax.crypto.Mac import javax.crypto.spec.SecretKeySpec @@ -42,29 +42,35 @@ class SlackAppService extends SlackService { // FIXME (lfp): this algorithm works as I've validated it against the sample data provided in the Slack documentation, // but it doesn't work with our requests and signing secret for some reason. I've reached out to Slack support but // have not received any definitive answers yet. - void verifySignature(RequestEntity request) { + void verifySignature(RequestEntity request, boolean preventReplays = true) { HttpHeaders headers = request.getHeaders() String body = request.getBody() String timestamp = headers['X-Slack-Request-Timestamp'].first() String signature = headers['X-Slack-Signature'].first() - if ((Instant.ofEpochSecond(Long.valueOf(timestamp)) + Duration.ofMinutes(5)).isBefore(Instant.now())) { + if (preventReplays && + (Instant.ofEpochSecond(Long.valueOf(timestamp)) + Duration.ofMinutes(5)).isBefore(Instant.now())) { // The request timestamp is more than five minutes from local time. It could be a replay attack. throw new InvalidRequestException("Slack request timestamp is older than 5 minutes. Replay attack?") } - String signatureBaseString = 'v0:' + timestamp + ':' + body + String calculatedSignature = calculateSignature(timestamp, body) + if (calculatedSignature != signature) { + throw new InvalidRequestException("Invalid Slack signature header.") + } + } + + String calculateSignature(String timestamp, String body, String version = "v0") { try { + // For some reason, Spring URL-decodes asterisks in the body (but not other URL-encoded characters :-P) + body = body.replaceAll(/\*/, "%2A") + String signatureBaseString = "$version:$timestamp:$body" Mac mac = Mac.getInstance("HmacSHA256") SecretKeySpec secretKeySpec = new SecretKeySpec(config.signingSecret.getBytes(), "HmacSHA256") mac.init(secretKeySpec) byte[] digest = mac.doFinal(signatureBaseString.getBytes()) - String calculatedSignature = "v0=" + Hex.encode(digest).toString() - - if (calculatedSignature != signature) { - throw new InvalidRequestException("Invalid Slack signature header.") - } + return "$version=${Hex.encodeHex(digest).toString()}" } catch (InvalidKeyException e) { throw new InvalidRequestException("Invalid key exception verifying Slack request signature.") } diff --git a/echo-notifications/src/main/groovy/com/netflix/spinnaker/echo/slack/SlackInteractiveNotificationService.groovy b/echo-notifications/src/main/groovy/com/netflix/spinnaker/echo/slack/SlackInteractiveNotificationService.groovy index ccb7f4d82..693d2067f 100644 --- a/echo-notifications/src/main/groovy/com/netflix/spinnaker/echo/slack/SlackInteractiveNotificationService.groovy +++ b/echo-notifications/src/main/groovy/com/netflix/spinnaker/echo/slack/SlackInteractiveNotificationService.groovy @@ -99,12 +99,11 @@ class SlackInteractiveNotificationService extends SlackNotificationService imple @Override Notification.InteractiveActionCallback parseInteractionCallback(RequestEntity request) { - // TODO(lfp): This currently doesn't work -- troubleshooting with Slack support. - //slack.verifySignature(request) + // Before anything else, verify the signature on the request + slackAppService.verifySignature(request) Map payload = parseSlackPayload(request.getBody()) log.debug("Received callback event from Slack of type ${payload.type}") - slackAppService.verifyToken(payload.token) if (payload.actions.size > 1) { log.warn("Expected a single selected action from Slack, but received ${payload.actions.size}") diff --git a/echo-notifications/src/main/groovy/com/netflix/spinnaker/echo/slack/SlackNotificationService.groovy b/echo-notifications/src/main/groovy/com/netflix/spinnaker/echo/slack/SlackNotificationService.groovy index 731482ff9..406b4e683 100644 --- a/echo-notifications/src/main/groovy/com/netflix/spinnaker/echo/slack/SlackNotificationService.groovy +++ b/echo-notifications/src/main/groovy/com/netflix/spinnaker/echo/slack/SlackNotificationService.groovy @@ -50,6 +50,7 @@ class SlackNotificationService implements NotificationService { @Override EchoResponse.Void handle(Notification notification) { + log.debug("Handling Slack notification to ${notification.to}") def text = notificationTemplateEngine.build(notification, NotificationTemplateEngine.Type.BODY) notification.to.each { def response diff --git a/echo-notifications/src/test/groovy/com/netflix/spinnaker/echo/slack/SlackInteractiveNotificationServiceSpec.groovy b/echo-notifications/src/test/groovy/com/netflix/spinnaker/echo/slack/SlackInteractiveNotificationServiceSpec.groovy index 53a840ce3..275c626af 100644 --- a/echo-notifications/src/test/groovy/com/netflix/spinnaker/echo/slack/SlackInteractiveNotificationServiceSpec.groovy +++ b/echo-notifications/src/test/groovy/com/netflix/spinnaker/echo/slack/SlackInteractiveNotificationServiceSpec.groovy @@ -52,7 +52,7 @@ class SlackInteractiveNotificationServiceSpec extends Specification { URLEncoder.encode(getClass().getResource("/slack/callbackRequestBody.txt").text) RequestEntity request = new RequestEntity<>( - slackRequestBody, new HttpHeaders(), HttpMethod.POST, new URI("/notifications/callbaks")) + slackRequestBody, new HttpHeaders(), HttpMethod.POST, new URI("/notifications/callbacks")) slackAppService.verifyToken(*_) >> { } slackAppService.getUserInfo(*_) >> new SlackService.SlackUserInfo(email: "john@doe.com") @@ -80,7 +80,7 @@ class SlackInteractiveNotificationServiceSpec extends Specification { URLEncoder.encode(getClass().getResource("/slack/callbackRequestBody.txt").text) RequestEntity request = new RequestEntity<>( - slackRequestBody, new HttpHeaders(), HttpMethod.POST, new URI("/notifications/callbaks")) + slackRequestBody, new HttpHeaders(), HttpMethod.POST, new URI("/notifications/callbacks")) slackAppService.verifyToken(*_) >> { } slackAppService.getUserInfo(*_) >> { throw new Exception("oops!") } @@ -106,7 +106,7 @@ class SlackInteractiveNotificationServiceSpec extends Specification { given: String slackRequestBody = "content=suspicious" RequestEntity request = new RequestEntity<>( - slackRequestBody, new HttpHeaders(), HttpMethod.POST, new URI("/notifications/callbaks")) + slackRequestBody, new HttpHeaders(), HttpMethod.POST, new URI("/notifications/callbacks")) when: service.parseInteractionCallback(request) @@ -115,15 +115,15 @@ class SlackInteractiveNotificationServiceSpec extends Specification { thrown(InvalidRequestException) } - def "failing to verify the token from Slack throws an exception"() { + def "failing to verify the signature from Slack throws an exception"() { given: String slackRequestBody = "payload=" + URLEncoder.encode(getClass().getResource("/slack/callbackRequestBody.txt").text) RequestEntity request = new RequestEntity<>( - slackRequestBody, new HttpHeaders(), HttpMethod.POST, new URI("/notifications/callbaks")) + slackRequestBody, new HttpHeaders(), HttpMethod.POST, new URI("/notifications/callbacks")) - slackAppService.verifyToken(*_) >> { throw new InvalidRequestException() } + slackAppService.verifySignature(*_) >> { throw new InvalidRequestException() } slackAppService.getUserInfo(*_) >> { } when: @@ -139,7 +139,7 @@ class SlackInteractiveNotificationServiceSpec extends Specification { String slackRequestBody = "payload=" + URLEncoder.encode(payload, "UTF-8") RequestEntity request = new RequestEntity<>( - slackRequestBody, new HttpHeaders(), HttpMethod.POST, new URI("/notifications/callbaks")) + slackRequestBody, new HttpHeaders(), HttpMethod.POST, new URI("/notifications/callbacks")) slackAppService.verifyToken(*_) >> { } slackAppService.getUserInfo(*_) >> { } diff --git a/echo-notifications/src/test/groovy/com/netflix/spinnaker/echo/slack/SlackServiceSpec.groovy b/echo-notifications/src/test/groovy/com/netflix/spinnaker/echo/slack/SlackServiceSpec.groovy index 36daadf16..a8bb3ecbd 100644 --- a/echo-notifications/src/test/groovy/com/netflix/spinnaker/echo/slack/SlackServiceSpec.groovy +++ b/echo-notifications/src/test/groovy/com/netflix/spinnaker/echo/slack/SlackServiceSpec.groovy @@ -1,11 +1,16 @@ package com.netflix.spinnaker.echo.slack import com.netflix.spinnaker.echo.api.Notification +import com.netflix.spinnaker.echo.config.SlackAppProperties import com.netflix.spinnaker.echo.config.SlackConfig import com.netflix.spinnaker.echo.config.SlackLegacyProperties +import com.netflix.spinnaker.kork.web.exceptions.InvalidRequestException import groovy.json.JsonSlurper import org.apache.http.NameValuePair import org.apache.http.client.utils.URLEncodedUtils +import org.springframework.http.HttpHeaders +import org.springframework.http.HttpMethod +import org.springframework.http.RequestEntity import retrofit.client.Client import retrofit.client.Request import retrofit.client.Response @@ -26,6 +31,7 @@ class SlackServiceSpec extends Specification { @Subject BlockingVariable actualUrl @Subject BlockingVariable actualPayload SlackLegacyProperties configProperties + SlackAppProperties appProperties def setup() { actualUrl = new BlockingVariable() @@ -40,6 +46,7 @@ class SlackServiceSpec extends Specification { } configProperties = new SlackLegacyProperties() + appProperties = new SlackAppProperties() } def 'test sending Slack notification using incoming web hook'() { @@ -143,6 +150,33 @@ class SlackServiceSpec extends Specification { ] } + def "verifying a Slack notification callback"() { + given: "a SlackAppService configured with a signing secret and an incoming callback" + appProperties.signingSecret = "d41090bb6ec741bb9f68f4d77d34fa0ad897c5af" + + def slackAppService = slackConfig.slackAppService(appProperties, mockHttpClient, LogLevel.FULL) + + String timestamp = "1581528126" + String payload = getClass().getResource("/slack/callbackRequestBody.txt").text + String slackRequestBody = "payload=" + URLEncoder.encode(payload, "UTF-8") + String signature = slackAppService.calculateSignature(timestamp, slackRequestBody) + + RequestEntity request = new RequestEntity<>( + slackRequestBody, + new HttpHeaders( + "X-Slack-Signature": signature, + "X-Slack-Request-Timestamp": timestamp + ), + HttpMethod.POST, + new URI("/notifications/callbacks")) + + when: "the verifySignature method is called" + slackAppService.verifySignature(request, false) + + then: "the calculated signature matches the received signature" + notThrown(InvalidRequestException) + } + def static getField(Collection params, String fieldName) { params.find({ it -> it.name == fieldName })