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

Resovle host to all ips and check against the deny list #964

Merged
merged 3 commits into from
Sep 25, 2024

Conversation

amsiglan
Copy link
Collaborator

Description

Currently there are two issues with the deny list check in custom webhooks

  • We are not resolving the host to the IP address before matching
  • We match only for the first IP the host resolves to

This PR fixes both the issues

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

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.

@amsiglan amsiglan force-pushed the fix-deny-list-check branch 2 times, most recently from 9eeeaf6 to 3c88de0 Compare September 25, 2024 05:49
Copy link
Collaborator

@Hailong-am Hailong-am left a comment

Choose a reason for hiding this comment

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

looks good to me, please make sure CI passed on linux which will run integration test

@Hailong-am
Copy link
Collaborator

@amsiglan
Copy link
Collaborator Author

@amsiglan please include https://github.com/opensearch-project/notifications/pull/959/files#diff-02b4072a5be0696ea2c2519f53324d5264dd0d7b9ce8d17745331604016220aeR10-R12 in your PR that will resolve GLIBC_2.27 issue.

So, you mean we need to use v4 instead of v3?

@Hailong-am
Copy link
Collaborator

@amsiglan please include https://github.com/opensearch-project/notifications/pull/959/files#diff-02b4072a5be0696ea2c2519f53324d5264dd0d7b9ce8d17745331604016220aeR10-R12 in your PR that will resolve GLIBC_2.27 issue.

So, you mean we need to use v4 instead of v3?

these two lines that will allow to use legacy node 16 instead of 20

env:
  ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSION: true

@amsiglan
Copy link
Collaborator Author

@amsiglan please include https://github.com/opensearch-project/notifications/pull/959/files#diff-02b4072a5be0696ea2c2519f53324d5264dd0d7b9ce8d17745331604016220aeR10-R12 in your PR that will resolve GLIBC_2.27 issue.

So, you mean we need to use v4 instead of v3?

these two lines that will allow to use legacy node 16 instead of 20

env:
  ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSION: true

Created this PR to address the CIs - #965

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

fixed build

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

added test

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

added try catch

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

refactored logic to validate host first

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

fixed ktlint

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>
Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>
sbcd90
sbcd90 previously approved these changes Sep 25, 2024
Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>
@amsiglan
Copy link
Collaborator Author

The CI failures are unrelated to the changes in this PR

Copy link
Contributor

@riysaxen-amzn riysaxen-amzn left a comment

Choose a reason for hiding this comment

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

lgtm

@amsiglan amsiglan merged commit f0668a7 into opensearch-project:main Sep 25, 2024
13 of 16 checks passed
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

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

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/notifications/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/notifications/backport-2.x
# Create a new branch
git switch --create backport/backport-964-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 f0668a7d50be271f05423f7f007cf7d5efda1fc5
# Push it to GitHub
git push --set-upstream origin backport/backport-964-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/notifications/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-964-to-2.x.

@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.17 failed:

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

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/notifications/backport-2.17 2.17
# Navigate to the new working tree
pushd ../.worktrees/notifications/backport-2.17
# Create a new branch
git switch --create backport/backport-964-to-2.17
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 f0668a7d50be271f05423f7f007cf7d5efda1fc5
# Push it to GitHub
git push --set-upstream origin backport/backport-964-to-2.17
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/notifications/backport-2.17

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

@Divyaasm
Copy link

Hey @zhongnansu @amsiglan , can you backport the change to 2.17

riysaxen-amzn pushed a commit to riysaxen-amzn/notifications that referenced this pull request Sep 25, 2024
…roject#964)

* resovle host to all ips and check against the deny list

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

fixed build

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

added test

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

added try catch

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

refactored logic to validate host first

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

fixed ktlint

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

* fixed integ tests

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

* throw unknownhostexception instead of illegal argument exception

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

---------

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>
(cherry picked from commit f0668a7)
riysaxen-amzn pushed a commit to riysaxen-amzn/notifications that referenced this pull request Sep 25, 2024
…roject#964)

* resovle host to all ips and check against the deny list

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

fixed build

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

added test

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

added try catch

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

refactored logic to validate host first

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

fixed ktlint

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

* fixed integ tests

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

* throw unknownhostexception instead of illegal argument exception

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

---------

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>
(cherry picked from commit f0668a7)
zhongnansu pushed a commit that referenced this pull request Sep 25, 2024
* resovle host to all ips and check against the deny list

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

fixed build

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

added test

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

added try catch

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

refactored logic to validate host first

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

fixed ktlint

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

* fixed integ tests

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

* throw unknownhostexception instead of illegal argument exception

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

---------

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>
(cherry picked from commit f0668a7)

Co-authored-by: Amardeepsingh Siglani <amardeep7194@gmail.com>
amsiglan added a commit that referenced this pull request Sep 25, 2024
* resovle host to all ips and check against the deny list

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

fixed build

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

added test

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

added try catch

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

refactored logic to validate host first

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

fixed ktlint

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

* fixed integ tests

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

* throw unknownhostexception instead of illegal argument exception

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>

---------

Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>
(cherry picked from commit f0668a7)

Co-authored-by: Amardeepsingh Siglani <amardeep7194@gmail.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.

6 participants