From 9147615921ad36f46fa7e752130e850d504e6e03 Mon Sep 17 00:00:00 2001 From: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com> Date: Thu, 4 Aug 2022 18:22:45 -0700 Subject: [PATCH] Resolve hosts when checking against host deny list (#496) * Resolve hosts when checking against host deny list Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com> * Stub isHostInDenyList() for notification core unit tests Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com> * Stub the correct isHostInDenylist function Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com> * Use Before annotation instead for mocking setup Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com> * Test switching one of the ChimeDestinationTests to use mockk instead of EasyMock Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com> * Switch to BeforeEach for setup and remove unneeded unit test Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com> * Change CustomWebhookDestinationTest and SlackDestinationTests to use BeforeEach as well for consistency Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com> --- notifications/core-spi/build.gradle | 13 +++++++-- .../spi/utils/ValidationHelpers.kt | 3 +- .../spi/utils/ValidationHelpersTests.kt | 21 +++++++++----- .../destinations/ChimeDestinationTests.kt | 28 ++++++++++++------- .../CustomWebhookDestinationTests.kt | 10 +++++++ .../destinations/SlackDestinationTests.kt | 10 +++++++ 6 files changed, 65 insertions(+), 20 deletions(-) diff --git a/notifications/core-spi/build.gradle b/notifications/core-spi/build.gradle index 32145fb9..9071e339 100644 --- a/notifications/core-spi/build.gradle +++ b/notifications/core-spi/build.gradle @@ -53,12 +53,21 @@ dependencies { implementation "com.github.seancfoley:ipaddress:5.3.3" 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}" } diff --git a/notifications/core-spi/src/main/kotlin/org/opensearch/notifications/spi/utils/ValidationHelpers.kt b/notifications/core-spi/src/main/kotlin/org/opensearch/notifications/spi/utils/ValidationHelpers.kt index ff4fd991..0dcca54e 100644 --- a/notifications/core-spi/src/main/kotlin/org/opensearch/notifications/spi/utils/ValidationHelpers.kt +++ b/notifications/core-spi/src/main/kotlin/org/opensearch/notifications/spi/utils/ValidationHelpers.kt @@ -10,6 +10,7 @@ import org.apache.http.client.methods.HttpPatch import org.apache.http.client.methods.HttpPost import org.apache.http.client.methods.HttpPut import org.opensearch.common.Strings +import java.net.InetAddress import java.net.URL fun validateUrl(urlString: String) { @@ -30,7 +31,7 @@ fun isValidUrl(urlString: String): Boolean { fun isHostInDenylist(urlString: String, hostDenyList: List): 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)) { diff --git a/notifications/core-spi/src/test/kotlin/org/opensearch/notifications/spi/utils/ValidationHelpersTests.kt b/notifications/core-spi/src/test/kotlin/org/opensearch/notifications/spi/utils/ValidationHelpersTests.kt index ba09ebe9..b633ad08 100644 --- a/notifications/core-spi/src/test/kotlin/org/opensearch/notifications/spi/utils/ValidationHelpersTests.kt +++ b/notifications/core-spi/src/test/kotlin/org/opensearch/notifications/spi/utils/ValidationHelpersTests.kt @@ -5,12 +5,15 @@ 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 java.net.InetAddress internal class ValidationHelpersTests { - private val hostDentyList = listOf( + private val hostDenyList = listOf( "127.0.0.0/8", "10.0.0.0/8", "172.16.0.0/12", @@ -31,15 +34,19 @@ 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)) } } diff --git a/notifications/core/src/test/kotlin/org/opensearch/notifications/core/destinations/ChimeDestinationTests.kt b/notifications/core/src/test/kotlin/org/opensearch/notifications/core/destinations/ChimeDestinationTests.kt index e3a0a1ae..4944b823 100644 --- a/notifications/core/src/test/kotlin/org/opensearch/notifications/core/destinations/ChimeDestinationTests.kt +++ b/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() // 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() + every { mockHttpClient.execute(any()) } 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() + 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) diff --git a/notifications/core/src/test/kotlin/org/opensearch/notifications/core/destinations/CustomWebhookDestinationTests.kt b/notifications/core/src/test/kotlin/org/opensearch/notifications/core/destinations/CustomWebhookDestinationTests.kt index 5c91586d..0325b0a8 100644 --- a/notifications/core/src/test/kotlin/org/opensearch/notifications/core/destinations/CustomWebhookDestinationTests.kt +++ b/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) { diff --git a/notifications/core/src/test/kotlin/org/opensearch/notifications/core/destinations/SlackDestinationTests.kt b/notifications/core/src/test/kotlin/org/opensearch/notifications/core/destinations/SlackDestinationTests.kt index 91f7427d..65345014 100644 --- a/notifications/core/src/test/kotlin/org/opensearch/notifications/core/destinations/SlackDestinationTests.kt +++ b/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)