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

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)

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<String> request) {
+ void verifySignature(RequestEntity<String> 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.")
  }

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<String> 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}")

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

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<String> 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: "[email protected]")
@@ -80,7 +80,7 @@ class SlackInteractiveNotificationServiceSpec extends Specification {
  URLEncoder.encode(getClass().getResource("/slack/callbackRequestBody.txt").text)
 
  RequestEntity<String> 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<String> 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<String> 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<String> 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(*_) >> { }

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<String> actualUrl
  @Subject BlockingVariable<String> actualPayload
  SlackLegacyProperties configProperties
+ SlackAppProperties appProperties
 
  def setup() {
  actualUrl = new BlockingVariable<String>()
@@ -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<String> 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<NameValuePair> params, String fieldName) {
  params.find({ it -> it.name == fieldName })