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.

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

Resolve hosts when checking against host deny list #496

Merged
merged 7 commits into from Aug 5, 2022

Conversation

qreshi
Copy link
Contributor

@qreshi qreshi commented Aug 4, 2022

Signed-off-by: Mohammad Qureshi 47198598+qreshi@users.noreply.github.com

Description

The Notifications deny list setting (opensearch.notifications.core.http.host_deny_list) can take in a list of IPs or IP ranges to be blocked so that messages can't be sent against them. Example:

PUT /_cluster/settings
{
  "persistent": {
    "opensearch":{
      "notifications": {
        "core": {
          "http": {
            "host_deny_list": ["127.0.0.1", "10.0.0.0/8"]
          }
        }
      }
    }
  }
}

This PR adds hostname resolution before checking against the deny list so that a hostname which is not necessarily an IP (ex. example.com) does not bypass the denied IPs.

Issues Resolved

[List any issues this PR will resolve]

Check List

  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com>
@qreshi qreshi requested a review from a team August 4, 2022 20:15
@lezzago
Copy link
Member

lezzago commented Aug 4, 2022

LGTM, just need to figure out how to fix the tests

@qreshi
Copy link
Contributor Author

qreshi commented Aug 4, 2022

Dashboards is going to fail because of the reason mentioned here.

I'll work on fixing the unit tests though.

Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com>
Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com>
Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com>
@qreshi
Copy link
Contributor Author

qreshi commented Aug 4, 2022

It looks like the unit tests are still failing even after adding the stubbing, possible becasue I'm using mockk whereas the rest of those tests are using EasyMock. The problem is that EasyMock can't mock static functions which those utility functions are.

We should be changing to use mockk anyway outlined in #237 but that's out of scope for this PR.

…of EasyMock

Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com>
Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com>
@codecov-commenter
Copy link

codecov-commenter commented Aug 5, 2022

Codecov Report

Merging #496 (ac9cc63) into main (1dd7dbf) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##               main     #496   +/-   ##
=========================================
  Coverage     61.77%   61.77%           
  Complexity      112      112           
=========================================
  Files            73       73           
  Lines          2451     2451           
  Branches        264      264           
=========================================
  Hits           1514     1514           
  Misses          761      761           
  Partials        176      176           
Flag Coverage Δ
opensearch-notifications 61.77% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...earch/notifications/spi/utils/ValidationHelpers.kt 43.33% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

…BeforeEach as well for consistency

Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com>
Comment on lines +57 to +61
"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",
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.

@lezzago lezzago merged commit 82a926e into opensearch-project:main Aug 5, 2022
@opensearch-trigger-bot
Copy link

The backport to 2.0 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.0 2.0
# Navigate to the new working tree
cd .worktrees/backport-2.0
# Create a new branch
git switch --create backport/backport-496-to-2.0
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 82a926e1a77fa4deb388aac6f31338bbf9580acb
# Push it to GitHub
git push --set-upstream origin backport/backport-496-to-2.0
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.0

Then, create a pull request where the base branch is 2.0 and the compare/head branch is backport/backport-496-to-2.0.

opensearch-trigger-bot bot pushed a commit that referenced this pull request Aug 5, 2022
* 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>
(cherry picked from commit 82a926e)
@opensearch-trigger-bot
Copy link

The backport to 2.1 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.1 2.1
# Navigate to the new working tree
cd .worktrees/backport-2.1
# Create a new branch
git switch --create backport/backport-496-to-2.1
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 82a926e1a77fa4deb388aac6f31338bbf9580acb
# Push it to GitHub
git push --set-upstream origin backport/backport-496-to-2.1
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.1

Then, create a pull request where the base branch is 2.1 and the compare/head branch is backport/backport-496-to-2.1.

qreshi added a commit that referenced this pull request Aug 5, 2022
* 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>
(cherry picked from commit 82a926e)

Co-authored-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com>
qreshi added a commit to qreshi/notifications that referenced this pull request Aug 5, 2022
…t#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>
qreshi added a commit to qreshi/notifications that referenced this pull request Aug 5, 2022
…t#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>
qreshi added a commit that referenced this pull request Aug 5, 2022
* 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>
qreshi added a commit that referenced this pull request Aug 5, 2022
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants