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

[RAC][Alerts] - Addition of RBAC to unified alerts index #96014

Closed
wants to merge 19 commits into from

Conversation

yctercero
Copy link
Contributor

@yctercero yctercero commented Apr 1, 2021

Summary

++ in collaboration with @dhurley14

This PR adds RBAC (Role based access-control) for the .alerts index that rules from numerous features/plugins will be writing into.

Initial issue outlining work 95721.

The below diagram is a very rough outline of what we plan to do, based largely off of RBAC work being done by Cases and existing RBAC code done by Alerting.

alerts as data role based access control authorization request flow diagram

The alerts RBAC will be based off of the alerting plugin's existing RBAC model. We will be using the same security plugin Actions and FeaturePrivilegeBuilder. What does all that mean? Actions in the security plugin are used to build out URIs that are then associated to users to determine what they have access to. So for alerting, a user who has access to get a rule from the security solution may have a URI like alerting:DetectionEngine/securitySolution/get. What we want to do is reuse this, but add an additional piece of information in there to denote whether the action is referring to a rule or an alert. The following URIs associated with alerts would be:

Get an alert

alerting:<rule-alert-type>/<producer>/alert/get

Search alerts

alerting:<rule-alert-type>/<producer>/alert/find

Update an alert

alerting:<rule-alert-type>/<producer>/alert/updateStatus

In addition to reusing the Actions and FeaturePrivilegeBuilder, the RAC auth class will extend the alerting auth class to reuse the following methods: ensureAuthorized and getFindAuthorizationFilter. The ensureAuthorized is reused nearly 1 for 1, however the getFindAuthorizationFilter is modified slightly to allow us to ask for an ES filter, as opposed to a kuery built around the SO structure. This is because the alerts are ES entities and rules are SO/Kibana entities. Each is searched differently, so the filter and what is appended to it varies slightly.

Kibana Feature Privilege

Within the feature privilege object, these privileges would be displayed as follows (note that this may differ depending on how open we want to leave it to users to have varying combinations of read/write between rules and alerts):

plugins.features.registerKibanaFeature({
      id: MY_APP_ID,
      name: 'My App',
      alerting:['siem.signals'],
      privileges: {
        all: {
          alerting: {
            all:['siem.signals'],
          },
        },
        read: {
          alerting: {
            read:['siem.signals'],
          },
        },
      },
    subFeatures: [
      {
        name:  'My App',
        privilegeGroups: [
          {
            groupType: 'independent',
            privileges: [
              {
                id: 'allowAlertStatusUpdate',
                name: 'Alert status update',
                includeIn: 'all',
                alerting: {
                  all: ['alertUpdate'],
                },
              },
            ],
          },
        ],
      },
    ],
});

This means that a user who has a Kibana role with Security Solution set to All would get all privileges for rules and alerts, a role set to Read would get read privileges for rules and alerts, and if they would like, they can select to modify the subfeature to allow a user to be read on rules and write on alerts.

image

Testing stuff out

Pull down this branch and run ES locally using the following steps..

Running ES locally

  1. Edit buildSrc/src/main/groovy/elasticsearch.run.gradle and add setting 'xpack.security.authc.api_key.enabled', 'true' after line 24
  2. ./gradlew run # this runs with a trial license
  3. Execute below curl script to post kibana_elastic user
curl -u elastic:password -X POST "http://127.0.01:9200/_security/user/kibana_elastic?pretty" -H 'Content-Type: application/json' -d '{"password":"changeme","roles":["superuser"],"full_name":"kibana","email":"jacknich@example.com"}'
  1. Set kibana.dev.yml to use kibana_elastic as the user
elasticsearch:
  username: 'kibana_elastic'
  password: 'changeme'
  hosts: 'http://127.0.0.1:9200'

Start up kibana

You should now be able to create a rule which generates alerts and then "find" those alerts (using the kibana system user) using the scripts located in x-pack/plugins/rule_registry/server/scripts/get_security_solution_alert.sh

Test data
POST myfakeindex-1/_doc
{
  "message": "hello world 1"
}

POST myfakeindex-2/_doc
{
  "message": "hello world 2",
  "event": {
    "ingested": "2021-04-30T15:23:03.520Z" <replace with current ISO date>
  }
}

POST myfakeindex-3/_doc
{
  "message": "hello world 3",
  "@timestamp": "2021-04-30T15:23:03.520Z" <replace with current ISO date>
}

Create a rule to query myfa* and it should generate an alert in the security solution

To get the alert, change directory into x-pack/plugins/rule_registry/server/scripts and execute

term$ ./get_security_solution_alert.sh

This script will post the security role and observer role and execute a find using the new alerts as data client. To test the authz functionality execute the below, expecting a 403 response.

term$ ./get_security_solution_alert.sh observer
term$ {
  "statusCode": 401,
  "error": "Unauthorized",
  "message": "Unauthorized to get \"rac:8.0.0:siem/get\" alert\""
}

Checklist

}

public get(owner: string, operation: string): string {
if (!operation || !isString(operation)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these checks necessary given the type signature of the function? The only case in which this would be true (and pass type check) is if you passed in an empty string.

owner: string,
operation: string
): string {
return `${authorizationResult} to ${operation} "${owner}" alert"`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this meant to be shown to users, including perhaps API users? If so, should this be internationalized?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes server-side internationalization should definitely be happening. I haven't seen examples of this before though. If anyone has any to point to that'd be helpful but for sure would love to have this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think, historically, we haven't applies i18n to Server and Audit logs.... 🤔
I recall that there's a reason, but I can't recall what it is 🤦

(poking old timers with tribal knowledge @kobelb @legrego )

Copy link
Contributor

Choose a reason for hiding this comment

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

The weaknesses of tribal knowledge are apparent here because I too recall us not i18n'ing error messages or audit log entries; however, I don't recall the reasons.

Copy link
Contributor

@mikecote mikecote Apr 19, 2021

Choose a reason for hiding this comment

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

For i18n and server logs, I'm guessing from a supportability / SDH perspective that it would be hard to understand and help?

Copy link
Contributor

Choose a reason for hiding this comment

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

This code is concatenating strings as it is, so I don't think doing that using an i18n library would make it any harder to read. If the message is intended for CLI logs and the such, then I agree that they don't need to be i18n'd. But if the message is being returned in an HTTP response then I think we should consider i18n'ing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dhurley14 @gmmorris @mikecote I opened a discuss issue to talk about this: #97650

/**
* Solutions should specify owners of alerts here which will provide the solution read / write access to those alerts.
*/
rac?: {
Copy link
Contributor

@gmmorris gmmorris Apr 13, 2021

Choose a reason for hiding this comment

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

The name rac suggests this controls access to Rules and Cases as well as Alerts.
I know this isn't true for Rules (as that's controlled by the alerting RBAC).
Not sure about Cases.

I recognise the complication with terminology update, as our privileges would have to stay as alerting for now (as opposed to changing them to be under rules, as that would be a breaking change I believe [cc @legrego ?]), but it feels like naming these privileges rac will add to the confusion.

So, should this be changed to something more specific? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Feature IDs and sub-feature privilege ids are the only entities that cannot be renamed, as these get translated into Elasticsearch Application Privileges, which are then directly assigned to roles. I thought your features were named actions and stackAlerts, unless I'm thinking of something else?

Copy link
Contributor

Choose a reason for hiding this comment

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

naming is hard, what about just SolutionAlerts? I do agree with you @gmmorris

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought your features were named actions and stackAlerts, unless I'm thinking of something else?

Ah, sorry, I'm referring to the key in the privileges object, rather than the feature IDs.
We use alerting:

@XavierM I don't think SolutionAlerts is accurate as it'll be used by Stack Rules too.
Am I right that this will define the RBAC for the alerts as data operations? Feels like this should probably be alertsAsData or alerts, and we should change ours to be rules 🤔

Though... it raises the question: shouldn't the alerts privileges be inferred from rules privileges? Otherwise a user can create a rule but can't read the alerts produced by that rule 😬

Copy link
Contributor

Choose a reason for hiding this comment

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

@mikecote I think we might need to do another terminology change on our end - change our privileges key to rules instead of alerting 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Though... it raises the question: shouldn't the alerts privileges be inferred from rules privileges? Otherwise a user can create a rule but can't read the alerts produced by that rule 😬

We probably have a good reason to have different access controls for alerts, but IMHO inheriting those privileges from the rules make a lot of sense, and is the simplest model I can think of.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dgieselaar for t1 analysts, they're able to update alerts (status) but not able to write on rules.

@gmmorris I think the renaming to rules would be super helpful

Comment on lines +177 to +190
- `get`
- `find`
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why we're starting out restricted rather than permissive? E.g., how do I do aggregations with this client?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tgrid is conducting all the search strategy operations including aggs. The RBAC will be built into tgrid.

@XavierM does this sound accurate?

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 the client itself should be low-level. TGrid (AFAIK) is a UI component, but we need to show alerts in context, e.g. as an annotation in a timeseries graph. Paginating through a list of alerts won't scale, as that could easily be 1000s, so we would need to bucket alerts by date, for instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I got it. We'll create a permissive find route to be used to search alerts.

@dgieselaar
Copy link
Member

dgieselaar commented Apr 24, 2021

A couple of things came up yesterday that might impact how RBAC is implemented. I think @spong would reach out to this group but I'll note them here as well (as I have some additions).

  • Users will want to use the stack rule types to create alerts that will show up in solutions. E.g., a user wants to use the ES Query rule type for APM data, and have its alerts show up in Observability. We would either need to include data from stack alert indices in the Observability UI, or users need to route their alert data into a security alert index. If the former is the case, how does a user that only has access to Observability alerts see the data from the stack rule? If the latter is the case, how do we ensure that the user that owns the rule has the appropriate permissions to write to the Observability alert index (given the fact that these requests happen with the internal user)?
  • Does Rule monitoring data fall under the same umbrella as Alerts when it comes to RBAC? Or does it need a different model? Should it inherit privileges from rules? How does this impact where we write rule monitoring data?
  • Generally, what happens when rules start generating other data than alerts? Do we need a different model for each type of data? It might be rule monitoring, but it also might be things like an SLO or other forms of background processing. We can solve this later, but I think it's the same issue as above, so maybe we should take it into account as well.

yctercero and others added 12 commits May 12, 2021 11:52
… include space id in constructor rather than parameter as a part of the get since the spaceId will be available to us in the start phase of the plugin
… and adds some rac client functions to be implemented (#3)

* wip - ignore

* adds rac client initialization to plugin setup / startup and adds scaffolding for CRUD client functions

Co-authored-by: Yara Tercero <yara.tercero@elastic.co>
…ed to rac feature in plugin feature registry
…ould be able to query for data but getting back 403 for internal user
…e log statements, added security as a required plugin to rule_registry plugin without which, the rac authorization class was receiving an undefined security client so our calls to shouldCheckAuthorization were failing silently. Added some routes and scripts to test authz functionality. To test please see the README in the rule_registry/scripts.
…for security solution, need to work through rule registry changes (#8)
…thz get for security solution, need to work through rule registry changes" (#9)

This reverts commit f59c33c.
…for security solution, need to work through rule registry changes (#10)

* adds an 'owner' field to the siem-signals mapping, working authz get for security solution, need to work through rule registry changes

* minor cleanup

* undo owner change in rule registry, will come in different pr

* enhances user experience of test scripts

* response error
@dhurley14 dhurley14 changed the title [RAC][Alerts][skip-ci] - Addition of RBAC to unified alerts index [RAC][Alerts] - Addition of RBAC to unified alerts index May 14, 2021
@yctercero yctercero closed this Jul 20, 2021
@yctercero yctercero deleted the rac_rbac_poc branch August 4, 2022 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:RAC label obsolete release_note:breaking Team:Detections and Resp Security Detection Response Team Theme: rac label obsolete v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants