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

[Patch] Graphite SSRF patch #392

Merged
merged 8 commits into from
Jun 4, 2021
Merged

Conversation

ananzh
Copy link
Member

@ananzh ananzh commented Jun 1, 2021

Signed-off-by: Anan Zhuang [email protected]

Description

This PR is an implementation of ssrf patch. This patch allows customers to choose allowlist (vis_type_timeline.graphiteUrls) or blocklist (vis_type_timeline.blocklist) or both to verify its users' graphite url inputs. Customers can simply enable or disable these settings in the opensearch_dashboards.yml file to control what method they would like to apply for the safety check.

Only allowlist

  • Advanced setting: timeline:graphite.url: the first url in the allowlist is shown as the default setting in the text box. For example, vis_type_timeline.graphiteUrls: ['https://www.hostedgraphite.com/UID/ACCESS_KEY/graphite'] Screen Shot 2021-05-31 at 11 01 39 PM
  • User enter url: The input url is checked by allowlist
    • url is in the allowlist: okay
    • url is not in the allowlist: error message Screen Shot 2021-05-31 at 11 05 21 PM

Only blocklist

  • Advanced setting: timeline:graphite.url: empty text box. For example, vis_type_timeline.blocklist: ['127.0.0.0/8'] Screen Shot 2021-05-31 at 11 06 16 PM
  • User enter url: The input url is checked by blocklist
    • url is not in the blocklist: okay
    • url is in the allowlist: error message

Both blocklist and allowlist

  • Advanced setting: timeline:graphite.url: the first url in the allowlist is shown as the default setting in the text box. For example, vis_type_timeline.graphiteUrls: ['https://www.hostedgraphite.com/UID/ACCESS_KEY/graphite'] and vis_type_timeline.blocklist: ['127.0.0.0/8']
  • User enter url: The input url is checked by blocklist
    • url is not in the blocklist: okay
    • url is in the allowlist: error message

Neither blocklist nor allowlist

  • Advanced setting: timeline:graphite.url: empty text box
  • User enter url: error message asking user to configure Screen Shot 2021-05-31 at 11 09 53 PM

Notice

This PR doesn't implement the proposed combined UI component which is a text box that allows for a drop down menu as well due to the following considerations:

  • First, customer's allowlist might contain several users accessible urls. Advanced setting timeline:graphite.url should be a place for user to enter his/her own url to get access check. It should not expose other users' urls.
  • Second, if customer has users with similar but long graphite url, dropdown list could not display the complete url which will cause usage pain: Screen Shot 2021-05-31 at 11 31 24 PM
  • Third, if customer has many users, drop down will be too long and users might take a while to search the drop menu.

Therefore, a simple text box is enough for user to input his/her own graphite url. If this part needs further discussion, a separate PR could be opened since this would not affect security and would just be a UI component change.

Issues Resolved

#331

Instruction to recreate the issue

  • code base configuration: config the settings vis_type_timeline.graphiteAllowedUrls and vis_type_timeline.graphiteBlockedIPs (pick one or both) in opensearch_dashboards.yml in config folder
  • open dashboards and go to stack Management
  • click Advanced Settings and find Timeline section
  • configure the url in Graphite URL text box
  • back to Home menu and go to visualize
  • click create visualization then click timeline
  • at Timeline expression use graphite function: .graphite(1) (1 is the passed parameter. just an example)

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff

@ananzh ananzh added the patch label Jun 1, 2021
@ananzh ananzh self-assigned this Jun 1, 2021
@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 63598eb

Copy link
Member

@seraphjiang seraphjiang left a comment

Choose a reason for hiding this comment

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

Do we have readme or doc for all supported config/settings. If not, could we add the example config for (blocklist) into opensearch_dashboards.yml

@ananzh ananzh closed this Jun 1, 2021
@ananzh
Copy link
Member Author

ananzh commented Jun 1, 2021

Do we have readme or doc for all supported config/settings. If not, could we add the example config for (blocklist) into opensearch_dashboards.yml

will do

@ananzh ananzh reopened this Jun 1, 2021
@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 63598eb

@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 39f183f

@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 36a6a1c

@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 68b0db0

@boktorbb
Copy link
Contributor

boktorbb commented Jun 3, 2021

Do we have readme or doc for all supported config/settings. If not, could we add the example config for (blocklist) into opensearch_dashboards.yml

I agree. We should have a description in the opensearch_dashboards.yml file but we should have more detailed documentation covering settings combinations and usage in the README.md in the plugin directory itself

@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 461ef9a

@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed ec28338

@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 504c96c

@boktorbb boktorbb linked an issue Jun 3, 2021 that may be closed by this pull request
2 tasks
ananzh added 6 commits June 3, 2021 22:36
Signed-off-by: Anan Zhuang <[email protected]>
Signed-off-by: Anan Zhuang <[email protected]>
Signed-off-by: Anan Zhuang <[email protected]>
Signed-off-by: Anan Zhuang <[email protected]>
Signed-off-by: Anan Zhuang <[email protected]>
seraphjiang
seraphjiang previously approved these changes Jun 3, 2021
Copy link
Member

@seraphjiang seraphjiang left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

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

Only comment related to typo needs to be addressed, some nitpicks but works nicely!

I specifically ensured the backwards compatibility was still working as expected. Nothing to worry about there!

Thanks!

Signed-off-by: Anan Zhuang <[email protected]>
@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed def813d

@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 1036bf2

Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@boktorbb boktorbb 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. Thanks!

@ananzh ananzh merged commit 433165a into opensearch-project:main Jun 4, 2021
kavilla pushed a commit that referenced this pull request Jun 4, 2021
* ssrf patch

Signed-off-by: Anan Zhuang <[email protected]>

* revise based on PR comments

Signed-off-by: Anan Zhuang <[email protected]>

* revise unit test and comments

Signed-off-by: Anan Zhuang <[email protected]>

* fix lint issue

Signed-off-by: Anan Zhuang <[email protected]>

* add helper

Signed-off-by: Anan Zhuang <[email protected]>

* fix bug

Signed-off-by: Anan Zhuang <[email protected]>

* fix comments

Signed-off-by: Anan Zhuang <[email protected]>

* fix frontend display. helper shouldnot show in visualize

Signed-off-by: Anan Zhuang <[email protected]>
kavilla pushed a commit that referenced this pull request Jun 4, 2021
* ssrf patch

Signed-off-by: Anan Zhuang <[email protected]>

* revise based on PR comments

Signed-off-by: Anan Zhuang <[email protected]>

* revise unit test and comments

Signed-off-by: Anan Zhuang <[email protected]>

* fix lint issue

Signed-off-by: Anan Zhuang <[email protected]>

* add helper

Signed-off-by: Anan Zhuang <[email protected]>

* fix bug

Signed-off-by: Anan Zhuang <[email protected]>

* fix comments

Signed-off-by: Anan Zhuang <[email protected]>

* fix frontend display. helper shouldnot show in visualize

Signed-off-by: Anan Zhuang <[email protected]>
@ananzh ananzh deleted the patch-ssrf branch February 23, 2022 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Patch] Graphite SSRF patch
6 participants