Resolve hosts when checking against host deny list (#496) (#498)

* Resolve hosts when checking against host deny list

Signed-off-by: Mohammad Qureshi <[email protected]>

* Stub isHostInDenyList() for notification core unit tests

Signed-off-by: Mohammad Qureshi <[email protected]>

* Stub the correct isHostInDenylist function

Signed-off-by: Mohammad Qureshi <[email protected]>

* Use Before annotation instead for mocking setup

Signed-off-by: Mohammad Qureshi <[email protected]>

* Test switching one of the ChimeDestinationTests to use mockk instead of EasyMock

Signed-off-by: Mohammad Qureshi <[email protected]>

* Switch to BeforeEach for setup and remove unneeded unit test

Signed-off-by: Mohammad Qureshi <[email protected]>

* Change CustomWebhookDestinationTest and SlackDestinationTests to use BeforeEach as well for consistency

Signed-off-by: Mohammad Qureshi <[email protected]>
(cherry picked from commit 82a926e)

Co-authored-by: Mohammad Qureshi <[email protected]>

Author: opensearch-trigger-bot[bot]

notifications/core-spi/build.gradle

@@ -51,15 +51,24 @@ dependencies {
     implementation "org.apache.httpcomponents:httpcore:${versions.httpcore}"
     implementation "org.apache.httpcomponents:httpclient:${versions.httpclient}"
     implementation "com.github.seancfoley:ipaddress:5.3.3"
-    implementation("commons-validator:commons-validator:1.7")
+    implementation "commons-validator:commons-validator:1.7"
 
     testImplementation(
-            'org.junit.jupiter:junit-jupiter-api:5.6.2',
+            "io.mockk:mockk:1.11.0",
+            "io.mockk:mockk-common:1.11.0",
+            "io.mockk:mockk-dsl:1.11.0",
+            "io.mockk:mockk-dsl-jvm:1.11.0",
+            "io.mockk:mockk-agent-api:1.11.0",
+            "io.mockk:mockk-agent-common:1.11.0",
+            "io.mockk:mockk-agent-jvm:1.11.0",
+            "org.junit.jupiter:junit-jupiter-api:5.6.2",
             "org.junit.jupiter:junit-jupiter-params:5.6.2",
-            'org.mockito:mockito-junit-jupiter:3.10.0',
+            "org.mockito:mockito-junit-jupiter:3.10.0",
     )
     testRuntimeOnly('org.junit.jupiter:junit-jupiter-engine:5.6.2')
     testImplementation "org.jetbrains.kotlin:kotlin-test:${kotlin_version}"
+    testImplementation "org.jetbrains.kotlin:kotlin-reflect:${kotlin_version}" // required by mockk
+    testImplementation "net.bytebuddy:byte-buddy-agent:1.12.7"
     testImplementation "org.opensearch.test:framework:${opensearch_version}"
 }
 

notifications/core-spi/src/main/kotlin/org/opensearch/notifications/spi/utils/ValidationHelpers.kt

@@ -12,6 +12,7 @@ import org.apache.http.client.methods.HttpPost
 import org.apache.http.client.methods.HttpPut
 import org.opensearch.common.Strings
 import org.opensearch.notifications.spi.utils.ValidationHelpers.FQDN_REGEX
+import java.net.InetAddress
 import java.net.URL
 
 private object ValidationHelpers {
@@ -51,7 +52,7 @@ fun isValidUrl(urlString: String): Boolean {
 fun isHostInDenylist(urlString: String, hostDenyList: List<String>): Boolean {
     val url = URL(urlString)
     if (url.host != null) {
-        val ipStr = IPAddressString(url.host)
+        val ipStr = IPAddressString(InetAddress.getByName(url.host).hostAddress)
         for (network in hostDenyList) {
             val netStr = IPAddressString(network)
             if (netStr.contains(ipStr)) {

notifications/core-spi/src/test/kotlin/org/opensearch/notifications/spi/utils/ValidationHelpersTests.kt

@@ -5,9 +5,12 @@
 
 package org.opensearch.notifications.spi.utils
 
+import io.mockk.every
+import io.mockk.mockkStatic
 import org.junit.jupiter.api.Assertions.assertEquals
 import org.junit.jupiter.api.Test
 import org.junit.jupiter.api.assertThrows
+import java.net.InetAddress
 import java.net.MalformedURLException
 
 internal class ValidationHelpersTests {
@@ -20,7 +23,7 @@ internal class ValidationHelpersTests {
     private val WEBHOOK_URL = "https://test-webhook.com:1234/subdirectory?param1=value1&param2=&param3=value3"
     private val CHIME_URL = "https://domain.com/sample_chime_url#1234567890"
 
-    private val hostDentyList = listOf(
+    private val hostDenyList = listOf(
         "127.0.0.0/8",
         "10.0.0.0/8",
         "172.16.0.0/12",
@@ -41,16 +44,20 @@ internal class ValidationHelpersTests {
             "9.9.9.9"
         )
         for (ip in ips) {
-            assertEquals(true, isHostInDenylist("https://$ip", hostDentyList))
+            assertEquals(true, isHostInDenylist("https://$ip", hostDenyList))
         }
     }
 
     @Test
-    fun `test url in denylist`() {
-        val urls = listOf("https://www.amazon.com", "https://mytest.com", "https://mytest.com")
-        for (url in urls) {
-            assertEquals(false, isHostInDenylist(url, hostDentyList))
-        }
+    fun `test hostname gets resolved to ip for denylist`() {
+        val invalidHost = "invalid.com"
+        mockkStatic(InetAddress::class)
+        every { InetAddress.getByName(invalidHost).hostAddress } returns "10.0.0.1" // 10.0.0.0/8
+        assertEquals(true, isHostInDenylist("https://$invalidHost", hostDenyList))
+
+        val validHost = "valid.com"
+        every { InetAddress.getByName(validHost).hostAddress } returns "174.12.0.0"
+        assertEquals(false, isHostInDenylist("https://$validHost", hostDenyList))
     }
 
     @Test

notifications/core/src/test/kotlin/org/opensearch/notifications/core/destinations/ChimeDestinationTests.kt

@@ -5,6 +5,9 @@
 
 package org.opensearch.notifications.core.destinations
 
+import io.mockk.every
+import io.mockk.mockk
+import io.mockk.mockkStatic
 import org.apache.http.client.methods.CloseableHttpResponse
 import org.apache.http.client.methods.HttpPost
 import org.apache.http.entity.StringEntity
@@ -13,6 +16,7 @@ import org.apache.http.message.BasicStatusLine
 import org.easymock.EasyMock
 import org.junit.jupiter.api.Assertions
 import org.junit.jupiter.api.Assertions.assertEquals
+import org.junit.jupiter.api.BeforeEach
 import org.junit.jupiter.api.Test
 import org.junit.jupiter.api.assertThrows
 import org.junit.jupiter.params.ParameterizedTest
@@ -43,23 +47,27 @@ internal class ChimeDestinationTests {
             )
     }
 
+    @BeforeEach
+    fun setup() {
+        // Stubbing isHostInDenylist() so it doesn't attempt to resolve hosts that don't exist in the unit tests
+        mockkStatic("org.opensearch.notifications.spi.utils.ValidationHelpersKt")
+        every { org.opensearch.notifications.spi.utils.isHostInDenylist(any(), any()) } returns false
+    }
+
     @Test
     fun `test chime message null entity response`() {
-        val mockHttpClient: CloseableHttpClient = EasyMock.createMock(CloseableHttpClient::class.java)
+        val mockHttpClient = mockk<CloseableHttpClient>()
 
         // The DestinationHttpClient replaces a null entity with "{}".
         val expectedWebhookResponse = DestinationMessageResponse(RestStatus.OK.status, "{}")
         // TODO replace EasyMock in all UTs with mockk which fits Kotlin better
-        val httpResponse: CloseableHttpResponse = EasyMock.createMock(CloseableHttpResponse::class.java)
-        EasyMock.expect(mockHttpClient.execute(EasyMock.anyObject(HttpPost::class.java))).andReturn(httpResponse)
+        val httpResponse = mockk<CloseableHttpResponse>()
+        every { mockHttpClient.execute(any<HttpPost>()) } returns httpResponse
 
-        val mockStatusLine: BasicStatusLine = EasyMock.createMock(BasicStatusLine::class.java)
-        EasyMock.expect(httpResponse.statusLine).andReturn(mockStatusLine)
-        EasyMock.expect(httpResponse.entity).andReturn(null).anyTimes()
-        EasyMock.expect(mockStatusLine.statusCode).andReturn(RestStatus.OK.status)
-        EasyMock.replay(mockHttpClient)
-        EasyMock.replay(httpResponse)
-        EasyMock.replay(mockStatusLine)
+        val mockStatusLine = mockk<BasicStatusLine>()
+        every { httpResponse.statusLine } returns mockStatusLine
+        every { httpResponse.entity } returns null
+        every { mockStatusLine.statusCode } returns RestStatus.OK.status
 
         val httpClient = DestinationHttpClient(mockHttpClient)
         val webhookDestinationTransport = WebhookDestinationTransport(httpClient)

notifications/core/src/test/kotlin/org/opensearch/notifications/core/destinations/CustomWebhookDestinationTests.kt

@@ -5,6 +5,8 @@
 
 package org.opensearch.notifications.core.destinations
 
+import io.mockk.every
+import io.mockk.mockkStatic
 import org.apache.http.client.methods.CloseableHttpResponse
 import org.apache.http.client.methods.HttpPatch
 import org.apache.http.client.methods.HttpPost
@@ -16,6 +18,7 @@ import org.apache.http.message.BasicStatusLine
 import org.easymock.EasyMock
 import org.junit.jupiter.api.Assertions
 import org.junit.jupiter.api.Assertions.assertEquals
+import org.junit.jupiter.api.BeforeEach
 import org.junit.jupiter.api.Test
 import org.junit.jupiter.api.assertThrows
 import org.junit.jupiter.params.ParameterizedTest
@@ -53,6 +56,13 @@ internal class CustomWebhookDestinationTests {
             )
     }
 
+    @BeforeEach
+    fun setup() {
+        // Stubbing isHostInDenylist() so it doesn't attempt to resolve hosts that don't exist in the unit tests
+        mockkStatic("org.opensearch.notifications.spi.utils.ValidationHelpersKt")
+        every { org.opensearch.notifications.spi.utils.isHostInDenylist(any(), any()) } returns false
+    }
+
     @ParameterizedTest(name = "method {0} should return corresponding type of Http request object {1}")
     @MethodSource("methodToHttpRequestType")
     fun `test custom webhook message null entity response`(method: String, expectedHttpClass: Class<HttpUriRequest>) {

notifications/core/src/test/kotlin/org/opensearch/notifications/core/destinations/SlackDestinationTests.kt

@@ -5,6 +5,8 @@
 
 package org.opensearch.notifications.core.destinations
 
+import io.mockk.every
+import io.mockk.mockkStatic
 import org.apache.http.client.methods.CloseableHttpResponse
 import org.apache.http.client.methods.HttpPost
 import org.apache.http.entity.StringEntity
@@ -13,6 +15,7 @@ import org.apache.http.message.BasicStatusLine
 import org.easymock.EasyMock
 import org.junit.jupiter.api.Assertions
 import org.junit.jupiter.api.Assertions.assertEquals
+import org.junit.jupiter.api.BeforeEach
 import org.junit.jupiter.api.Test
 import org.junit.jupiter.api.assertThrows
 import org.junit.jupiter.params.ParameterizedTest
@@ -44,6 +47,13 @@ internal class SlackDestinationTests {
             )
     }
 
+    @BeforeEach
+    fun setup() {
+        // Stubbing isHostInDenylist() so it doesn't attempt to resolve hosts that don't exist in the unit tests
+        mockkStatic("org.opensearch.notifications.spi.utils.ValidationHelpersKt")
+        every { org.opensearch.notifications.spi.utils.isHostInDenylist(any(), any()) } returns false
+    }
+
     @Test
     fun `test Slack message null entity response`() {
         val mockHttpClient: CloseableHttpClient = EasyMock.createMock(CloseableHttpClient::class.java)