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

[ResponseOps][Alerting] Fix stackAlerts plugin missing rac API auth scope #193948

Merged

Conversation

umbopepato
Copy link
Member

@umbopepato umbopepato commented Sep 25, 2024

Summary

Adds the ['rac'] API access scope to the Stack Alerts feature to correctly authenticate alerts API endpoints with the stackAlerts permission.
Also adds a dedicated API integration test for the impacted endpoint and permission set.

Release note

Fix Stack Alerts feature API access control

To verify

  1. Create rules that fire alerts in Stack management
  2. Wait for alerts to be created
  3. Create a role with only Stack Management > Rules : Read privilege
  4. Create a user with that role
  5. In another window, open Kibana with the newly created user
  6. Check that the Stack Management > Alerts page renders correctly, not showing any missing 403 error toasts

@umbopepato umbopepato added release_note:fix Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Sep 25, 2024
@umbopepato umbopepato force-pushed the fix-stack-alerts-endpoints-authentication branch from 6a3603f to 37fdc74 Compare October 1, 2024 14:10
};

const getSecuritySolutionIndexName = async (
const getIndexName = async (
Copy link
Member Author

Choose a reason for hiding this comment

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

Reduced similar functions to a single one parametrized based on featureIds

@umbopepato umbopepato marked this pull request as ready for review October 1, 2024 14:13
@umbopepato umbopepato requested a review from a team as a code owner October 1, 2024 14:13
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

LGTM! I tested and is working as expected. I think is unrelated to your PR but I tested without ES permission on the alert's index and I got the following error:

Screenshot 2024-10-02 at 12 39 56 PM

@@ -126,6 +126,14 @@ export default async function ({ readConfigFile }: FtrConfigProviderContext) {
spaces: ['*'],
},
],
elasticsearch: {
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason for adding this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

But before it was working without it, why does this role (alerts_and_actions_role) need extra permission? Did some tests start failing because we added the rac API privilege?

Copy link
Member

Choose a reason for hiding this comment

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

But before it was working without it, why does this role (alerts_and_actions_role) need extra permission? Did some tests start failing because we added the rac API privilege?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just added it to simulate a correct scenario (currently the user needs access to the index to be able to see the controls bar) but if we don't mind seeing the error toast in the functional test I can remove it, the other tests work just fine

Copy link
Member

Choose a reason for hiding this comment

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

I think is fine what I do not get is why the alerts_and_actions_role role needs it and not the stackAlertsOnlyReadSpacesAll which is used by the tests. I am sure I am missing something 🙂.

@umbopepato
Copy link
Member Author

LGTM! I tested and is working as expected. I think is unrelated to your PR but I tested without ES permission on the alert's index and I got the following error:

Screenshot 2024-10-02 at 12 39 56 PM

That's the broader issue I was trying to solve with the other PR we decided to split. If you don't have direct (at least read) access to the alerting index the controls embeddable cannot work correctly. 🙂

@cnasikas
Copy link
Member

cnasikas commented Oct 2, 2024

That's the broader issue I was trying to solve with the other PR we decided to split. If you don't have direct (at least read) access to the alerting index the controls embeddable cannot work correctly. 🙂

Make sense, thanks for that!

Copy link
Contributor

@js-jankisalvi js-jankisalvi left a comment

Choose a reason for hiding this comment

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

May be I am not understanding it correctly, but I assumed user with stack rules read role should at least be able to see the alerts, right?

But I don't see any alerts:

Elastic user Screenshot 2024-10-02 at 13 18 07
User with stack rules read Screenshot 2024-10-02 at 13 18 52

@umbopepato
Copy link
Member Author

May be I am not understanding it correctly, but I assumed user with stack rules read role should at least be able to see the alerts, right?

But I don't see any alerts:

Elastic user
Screenshot 2024-10-02 at 13 18 07
User with stack rules read
Screenshot 2024-10-02 at 13 18 52

Hey! Seems like those alerts have been created with the Example featureId which is different from stackAlerts. If you try to fire alerts from Stack rules you should be able to see them 🙂

Copy link
Contributor

@ersin-erdal ersin-erdal left a comment

Choose a reason for hiding this comment

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

LGTM
Tested locally, works as expected

@js-jankisalvi
Copy link
Contributor

Hey! Seems like those alerts have been created with the Example featureId which is different from stackAlerts. If you try to fire alerts from Stack rules you should be able to see them 🙂

Oops! My bad 🤦‍♀️

@umbopepato umbopepato enabled auto-merge (squash) October 7, 2024 12:59
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #11 / Serverless Index Management APIs Inference endpoints create inference endpoint

Metrics [docs]

✅ unchanged

History

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

@umbopepato umbopepato merged commit 17fcaa5 into elastic:main Oct 7, 2024
23 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11218654541

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 7, 2024
…cope (elastic#193948)

## Summary

Adds the `['rac']` API access scope to the Stack Alerts feature to
correctly authenticate alerts API endpoints with the `stackAlerts`
permission.
Also adds a dedicated API integration test for the impacted endpoint and
permission set.

## Release note

Fix Stack Alerts feature API access control

## To verify

1. Create rules that fire alerts in Stack management
2. Wait for alerts to be created
3. Create a role with only `Stack Management > Rules : Read` privilege
4. Create a user with that role
5. In another window, open Kibana with the newly created user
6. Check that the Stack Management > Alerts page renders correctly, not
showing any missing 403 error toasts

(cherry picked from commit 17fcaa5)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Oct 7, 2024
…auth scope (#193948) (#195279)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[ResponseOps][Alerting] Fix stackAlerts plugin missing rac API auth
scope (#193948)](#193948)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Umberto
Pepato","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-07T15:17:31Z","message":"[ResponseOps][Alerting]
Fix stackAlerts plugin missing rac API auth scope (#193948)\n\n##
Summary\r\n\r\nAdds the `['rac']` API access scope to the Stack Alerts
feature to\r\ncorrectly authenticate alerts API endpoints with the
`stackAlerts`\r\npermission.\r\nAlso adds a dedicated API integration
test for the impacted endpoint and\r\npermission set.\r\n\r\n## Release
note\r\n\r\nFix Stack Alerts feature API access control\r\n\r\n## To
verify\r\n\r\n1. Create rules that fire alerts in Stack management\r\n2.
Wait for alerts to be created\r\n3. Create a role with only `Stack
Management > Rules : Read` privilege\r\n4. Create a user with that
role\r\n5. In another window, open Kibana with the newly created
user\r\n6. Check that the Stack Management > Alerts page renders
correctly, not\r\nshowing any missing 403 error
toasts","sha":"17fcaa5c8eb6cdff5f89a2fa28a20f42d020381f","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Team:ResponseOps","v9.0.0","backport:prev-minor"],"title":"[ResponseOps][Alerting]
Fix stackAlerts plugin missing rac API auth
scope","number":193948,"url":"https://github.com/elastic/kibana/pull/193948","mergeCommit":{"message":"[ResponseOps][Alerting]
Fix stackAlerts plugin missing rac API auth scope (#193948)\n\n##
Summary\r\n\r\nAdds the `['rac']` API access scope to the Stack Alerts
feature to\r\ncorrectly authenticate alerts API endpoints with the
`stackAlerts`\r\npermission.\r\nAlso adds a dedicated API integration
test for the impacted endpoint and\r\npermission set.\r\n\r\n## Release
note\r\n\r\nFix Stack Alerts feature API access control\r\n\r\n## To
verify\r\n\r\n1. Create rules that fire alerts in Stack management\r\n2.
Wait for alerts to be created\r\n3. Create a role with only `Stack
Management > Rules : Read` privilege\r\n4. Create a user with that
role\r\n5. In another window, open Kibana with the newly created
user\r\n6. Check that the Stack Management > Alerts page renders
correctly, not\r\nshowing any missing 403 error
toasts","sha":"17fcaa5c8eb6cdff5f89a2fa28a20f42d020381f"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/193948","number":193948,"mergeCommit":{"message":"[ResponseOps][Alerting]
Fix stackAlerts plugin missing rac API auth scope (#193948)\n\n##
Summary\r\n\r\nAdds the `['rac']` API access scope to the Stack Alerts
feature to\r\ncorrectly authenticate alerts API endpoints with the
`stackAlerts`\r\npermission.\r\nAlso adds a dedicated API integration
test for the impacted endpoint and\r\npermission set.\r\n\r\n## Release
note\r\n\r\nFix Stack Alerts feature API access control\r\n\r\n## To
verify\r\n\r\n1. Create rules that fire alerts in Stack management\r\n2.
Wait for alerts to be created\r\n3. Create a role with only `Stack
Management > Rules : Read` privilege\r\n4. Create a user with that
role\r\n5. In another window, open Kibana with the newly created
user\r\n6. Check that the Stack Management > Alerts page renders
correctly, not\r\nshowing any missing 403 error
toasts","sha":"17fcaa5c8eb6cdff5f89a2fa28a20f42d020381f"}}]}]
BACKPORT-->

Co-authored-by: Umberto Pepato <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) release_note:fix Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.16.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants