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

[Logs UI] Return 403s rather than 500s for ML privilege errors #74506

Merged
merged 3 commits into from
Aug 12, 2020

Conversation

Kerry350
Copy link
Contributor

@Kerry350 Kerry350 commented Aug 6, 2020

Summary

This ticket fixes #70414. The changes here ensure that our routes that integrate with ML return a 403 status code, rather than 500, when there is a recognised ML privilege / capabilities based error.

Testing

Overview

⚠️ Please note that the changes here are based on the changes in #70069, as noted in the description there is a caveat in that: Note, due to limitations in alerting, the functions called by SIEM (jobsSummary and mlAnomalySearch) currently have their capabilities checks disabled., some of our ML integrated routes only call mlAnomalySearch, so those routes currently have these checks disabled. However, I have added the changes to those routes regardless, so they're ready when the alerting changes take effect.

Prerequisites

  • You'll need a user who doesn't have ML app privileges.

Testing changes

There are two ways you could test this, I have used the latter method.

  • You could disable the upfront privilege checks we do in the UI, and ensure the requests from that point fail as expected.

  • Testing the API directly. Below are all of the routes that are affected, and some curl requests that should help. Where I've included not working it's because of the caveat noted in the summary. These routes should still be checked though, as they have changes.

(The UI should, of course, also be tested to ensure it still works as expected - with a user who does and does not have ML privileges).

/api/infra/log_analysis/results/log_entry_anomalies_datasets

curl -X POST "<KIBANA_URL>/api/infra/log_analysis/results/log_entry_anomalies_datasets" --user user_without_ml:changeme -H 'Content-Type: application/json' -H 'kbn-xsrf: professionally-crafted-string-of-text' -d'
{
  "data": {
    "timeRange":{"startTime":1595425114104,"endTime":1596634714104},
    "sourceId": "default"
  }
}
'

/api/infra/log_analysis/results/log_entry_anomalies

curl -X POST "<KIBANA_URL>/api/infra/log_analysis/results/log_entry_anomalies" --user user_without_ml:changeme -H 'Content-Type: application/json' -H 'kbn-xsrf: professionally-crafted-string-of-text' -d'
{
  "data": {
    "timeRange":{"startTime":1595425114104,"endTime":1596634714104},
    "sourceId": "default"
  }
}
'

/api/infra/log_analysis/results/log_entry_categories (not working yet)

curl -X POST "<KIBANA_URL>/api/infra/log_analysis/results/log_entry_categories" --user user_without_ml:changeme -H 'Content-Type: application/json' -H 'kbn-xsrf: professionally-crafted-string-of-text' -d'
{
  "data": {
    "categoryCount": 5,
    "timeRange":{"startTime":1595425114104,"endTime":1596634714104},
    "sourceId": "default",
    "histograms":[{"bucketCount":10,"id":"history","timeRange":{"endTime":1596712908010,"startTime":1594293708008}},    {"bucketCount":1,"id":"reference","timeRange":{"endTime":1595503308009,"startTime":1594293708008}}]
  }
}
'

/api/infra/log_analysis/results/log_entry_category_datasets (not working yet)

curl -X POST "<KIBANA_URL>/api/infra/log_analysis/results/log_entry_category_datasets" --user user_without_ml:changeme -H 'Content-Type: application/json' -H 'kbn-xsrf: professionally-crafted-string-of-text' -d'
{
  "data": {
    "timeRange":{"startTime":1595425114104,"endTime":1596634714104},
    "sourceId": "default"
  }
}
'

/api/infra/log_analysis/results/log_entry_category_examples

curl -X POST "<KIBANA_URL>/api/infra/log_analysis/results/log_entry_category_examples" --user user_without_ml:changeme -H 'Content-Type: application/json' -H 'kbn-xsrf: professionally-crafted-string-of-text' -d'
{
  "data": {
    "timeRange":{"startTime":1595425114104,"endTime":1596634714104},
    "sourceId": "default",
    "categoryId": 1,
    "exampleCount": 5
  }
}
'

/api/infra/log_analysis/results/log_entry_examples

curl -X POST "<KIBANA_URL>/api/infra/log_analysis/results/log_entry_examples" --user user_without_ml:changeme -H 'Content-Type: application/json' -H 'kbn-xsrf: professionally-crafted-string-of-text' -d'
{
  "data": {
    "timeRange":{"startTime":1595425114104,"endTime":1596634714104},
    "sourceId": "default",
    "dataset": "nginx.error",
    "exampleCount": 5
  }
}
'

/api/infra/log_analysis/results/log_entry_rate (not working)

curl -X POST "<KIBANA_URL>/api/infra/log_analysis/results/log_entry_rate" --user user_without_ml:changeme -H 'Content-Type: application/json' -H 'kbn-xsrf: professionally-crafted-string-of-text' -d'
{
  "data": {
    "timeRange":{"startTime":1595425114104,"endTime":1596634714104},
    "sourceId": "default",
    "bucketDuration": 15000
  }
}
'

@Kerry350 Kerry350 added release_note:fix v8.0.0 Feature:Logs UI Logs UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.10.0 labels Aug 6, 2020
@Kerry350 Kerry350 added this to the Logs UI 7.10 milestone Aug 6, 2020
@Kerry350 Kerry350 requested a review from a team August 6, 2020 12:10
@Kerry350 Kerry350 self-assigned this Aug 6, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

@Kerry350
Copy link
Contributor Author

@elasticmachine merge upstream

1 similar comment
@weltenwort
Copy link
Member

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

👀 Reviewing on behalf of @elastic/logs-metrics-ui...

Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

Seems to work as described 👍

Testing via the UI proved difficult because the jobs_summary requests fail earlier such that the routes changed here are never called.

Comment on lines +78 to +83
return response.customError({
statusCode: 403,
body: {
message: error.message,
},
});
Copy link
Member

Choose a reason for hiding this comment

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

Even though it's internally equivalent, did you consider using response.forbidden for the sake of discoverability?

Suggested change
return response.customError({
statusCode: 403,
body: {
message: error.message,
},
});
return response.forbidden({
body: {
message: error.message,
},
});

@weltenwort
Copy link
Member

And thank you for the detailed commentary and helpful testing instructions!

@Kerry350
Copy link
Contributor Author

@weltenwort

Testing via the UI proved difficult because the jobs_summary requests fail earlier such that the routes changed here are never called.

Yeah, UI testing was a bit gnarly 😬

Thanks for the review. I'm going to merge this now as there's a green build; I will do a mini-followup with the response.forbidden changes as that would be nice for discoverability like you say.

@Kerry350 Kerry350 merged commit 2e38f5a into elastic:master Aug 12, 2020
Kerry350 added a commit to Kerry350/kibana that referenced this pull request Aug 12, 2020
Kerry350 added a commit that referenced this pull request Aug 13, 2020
… (#74859)

* Add ML privileges error checks to all routes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Logs UI Logs UI feature release_note:fix Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Logs UI] Return status code 403 instead of 500 for log analysis routes when missing ml capabilities
4 participants