Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Resolve hosts when checking against host deny list #496

Merged
merged 7 commits into from
Aug 5, 2022
Merged

Resolve hosts when checking against host deny list #496

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6ff99c0
Resolve hosts when checking against host deny list
qreshi Aug 1, 2022
62a63d1
Stub isHostInDenyList() for notification core unit tests
qreshi Aug 4, 2022
73de828
Stub the correct isHostInDenylist function
qreshi Aug 4, 2022
0fad996
Use Before annotation instead for mocking setup
qreshi Aug 4, 2022
fa1ab61
Test switching one of the ChimeDestinationTests to use mockk instead …
qreshi Aug 4, 2022
5827c3c
Switch to BeforeEach for setup and remove unneeded unit test
qreshi Aug 5, 2022
ac9cc63
Change CustomWebhookDestinationTest and SlackDestinationTests to use …
qreshi Aug 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions notifications/core-spi/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Comment on lines +57 to +61
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need the new mock libraries. Lets have a followup to see if we can fix this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This subproject wasn't using mockk before. I had to add them similar to what the other subproject did. We can however try to refactor the common stuff into the main build.gradle so it doesn't get redeclared. That has some implications though, we can discuss this more as a follow-up.

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

Expand Down
3 changes: 2 additions & 1 deletion ...ions/core-spi/src/main/kotlin/org/opensearch/notifications/spi/utils/ValidationHelpers.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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)) {
Expand Down
21 changes: 14 additions & 7 deletions ...core-spi/src/test/kotlin/org/opensearch/notifications/spi/utils/ValidationHelpersTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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",
Expand All @@ -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
Expand Down
28 changes: 18 additions & 10 deletions ...e/src/test/kotlin/org/opensearch/notifications/core/destinations/ChimeDestinationTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand Down
10 changes: 10 additions & 0 deletions ...st/kotlin/org/opensearch/notifications/core/destinations/CustomWebhookDestinationTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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>) {
Expand Down
10 changes: 10 additions & 0 deletions ...e/src/test/kotlin/org/opensearch/notifications/core/destinations/SlackDestinationTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand Down