[ResponseOps] Allow users authenticated with an API keys to manage alerting rules #154189

doakalexi · 2023-03-31T17:34:24Z

Summary

Updates the following functions in the Rules Client to re-use the API key in context and avoid having the system invalidate them when no longer in use:

bulk_delete
bulk_edit
clone
create
delete
update
update_api_key

Also adds a new field to the rule SO to help determine when whether an api key was created by a user or created by us.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

To verify

Follow these instructions to create an api key. Make sure to copy your api key
Run the following

curl -X POST "http://localhost:5601/api/alerting/rule/" -H 'Authorization: ApiKey ${API_KEY}' -H 'kbn-xsrf: true' -H 'Content-Type: application/json' -d'
{
  "rule_type_id": "example.pattern",
  "name": "pattern",
  "schedule": {
    "interval": "5s"
  },
  "actions": [
  ],
  "consumer": "alerts",
  "tags": [],
  "notify_when": "onActionGroupChange",
  "params": {
    "patterns": {
      "instA": " a - - a "
    }
  }
}'

Verify that the request returns a rule with"api_key_created_by_user":true
Try this with the other rules clients functions listed above to verify that you can manage alerting rules when authenticated with an api key
Verify that "api_key_created_by_user":false when you remove the api key header and add -u ${USERNAME}:${PASSWORD} to authenticate

elasticmachine · 2023-04-03T12:46:01Z

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

afharo

SO changes LGTM. If not needed for search/aggregations, I'd avoid adding it to the mappings.

x-pack/plugins/alerting/server/saved_objects/mappings.ts

…nto alerting/auth-api-key

pmuellr · 2023-04-05T16:54:33Z

Trying out the _update_api_key API, via the following, doesn't seem to work. Am I holding it wrong?

$ curl -X POST "http://localhost:5601/internal/alerting/rule/4f0a4ec0-d3cf-11ed-8173-855da63578db/_update_api_key" -H "Authorization: ApiKey ${API_KEY}" -H 'kbn-xsrf: true'
{"statusCode":400,"error":"Bad Request","message":"Error updating API key for rule: could not create API key - Unsupported scheme \"ApiKey\" for granting API Key"}

Seems like it should not have tried to create an API key in this case, but didn't look in the code to see what was going on ...

doakalexi · 2023-04-05T17:55:10Z

Trying out the _update_api_key API, via the following, doesn't seem to work. Am I holding it wrong?
$ curl -X POST "http://localhost:5601/internal/alerting/rule/4f0a4ec0-d3cf-11ed-8173-855da63578db/_update_api_key" -H "Authorization: ApiKey ${API_KEY}" -H 'kbn-xsrf: true'
{"statusCode":400,"error":"Bad Request","message":"Error updating API key for rule: could not create API key - Unsupported scheme \"ApiKey\" for granting API Key"}
Seems like it should not have tried to create an API key in this case, but didn't look in the code to see what was going on ...

Resolved in this commit 47d83e6

pmuellr · 2023-04-05T20:49:38Z

Still reviewing, no blockers yet, however there is one potentially big thing I'd like added. Doesn't neccessarily need to be in this PR, but I think we'd want to do it before 8.8.

More function tests. :-).

What I'd like to see is a bunch of the following scenarios:

create a rule
make sure it's running (whatever's easiest, maybe the get event log function)
modify the rule with one of the methods changed in this PR
make sure the shape is correct after the modification, and check if it's still running
ideally, check to make sure any API keys in the scenario are queued for invalidation / still exist / etc, as expected - this might be hard, not sure where we store those queued-for-invalidation keys

I'd like to see this where we run ever combination of methods changed, where when we create the rule, and run the method, we use every combination of api-key / login. So, something like this:

for (const createType of ['api-key', 'login']) {
  for (const mutateType of ['api-key', 'login']) {
    for (const method of ['bulk_delete', 'bulk_edit', ... ]) {
      test(createType, mutateType, method);
    }
  }
}

doakalexi · 2023-04-06T17:25:59Z

Still reviewing, no blockers yet, however there is one potentially big thing I'd like added. Doesn't neccessarily need to be in this PR, but I think we'd want to do it before 8.8.

More function tests. :-).

I created an issue to add functional tests #154584

pmuellr

Lots of comments, generally LGTM. I think the most critical comment is on transform_rule_for_export.test.ts, to add some more tests. If anything else makes sense and is easy, cool. I'm interested in getting this merged soon-ish so we can start hammering on it, so if anything is "too big", I think we can do in a follow-up PR.

pmuellr · 2023-04-03T21:02:15Z

x-pack/plugins/alerting/server/rules_client/methods/bulk_edit.ts

    createdAPIKey = shouldUpdateApiKey
-      ? await context.createAPIKey(generateAPIKeyName(ruleType.id, attributes.name))
+      ? isAuthTypeApiKey
+        ? await context.getAuthenticationAPIKey(name)


Looks like the name here is just added to the return object of getAuthenticationAPIKey(), not otherwise used. AFAIK, we don't display this name anywhere in alerting UX - I think it's primary use is when you are scrolling through the API Keys UX, for the keys we generate, so you can tell what rules they were for.

If so, I think this is fine.

I just don't want to confuse customers by suggesting there is an API key with that name. Maybe we should add something to the name though anyway, in this case. Could be useful while diagnosing problems ... like prefix/postfix with "(user-created)" or similar.

Oh okay that makes sense, I can add something like that

Resolved in this commit bc37396

pmuellr · 2023-04-05T16:16:55Z

x-pack/plugins/alerting/server/saved_objects/transform_rule_for_export.test.ts

@@ -91,6 +91,7 @@ describe('transform rule for export', () => {
          enabled: false,
          apiKey: null,
          apiKeyOwner: null,
+          apiKeyCreatedByUser: null,


Feels like we should have a test for apiKeyCreatedByUser: true in here, to make sure it's getting exported as null (like the other api key-related properties).

Resolved in this commit bc37396

pmuellr · 2023-04-05T16:19:23Z

x-pack/plugins/alerting/server/saved_objects/mappings.ts

@@ -8,6 +8,7 @@
 import { SavedObjectsTypeMappingDefinition } from '@kbn/core/server';

 export const alertMappings: SavedObjectsTypeMappingDefinition = {
+  dynamic: false,


@XavierM I'm guessing you're adding this in your SO refactor PR; heads up that we're doing it here to add a new property that doesn't need to be indexed.

pmuellr · 2023-04-05T17:51:11Z

x-pack/plugins/alerting/server/rules_client/methods/clone.ts

@@ -96,17 +96,22 @@ export async function clone<Params extends RuleTypeParams = never>(
  const lastRunTimestamp = new Date();
  const legacyId = Semver.lt(context.kibanaVersion, '8.0.0') ? id : null;
  let createdAPIKey = null;
+  let isAuthTypeApiKey = false;


These blocks of code are very similar between the various client methods - wonder if there's anyway we could arrange to do most of this work in a common function, to cut down on the boilerplate. Even it it was just useful for most of the methods, would probably be useful ...

Resolved in this commit bc37396

The only client method I didn't update was create.ts

pmuellr · 2023-04-05T20:27:20Z

x-pack/plugins/alerting/server/rules_client/methods/update_api_key.ts

      context.logger,
      context.unsecuredSavedObjectsClient
    );
    throw e;
  }

-  if (apiKeyToInvalidate) {
+  if (apiKeyToInvalidate && !apiKeyCreatedByUser) {


can we rename these to have a "previous" or "old" prefix. My first thought on seeing these was - which "keyCreatedByUser"? It's not the newly created one, right? No, it's not, but I had to scroll to the top to see where these were assigned from the original rule. I think putting a prefix on them will keep me from wondering in the future :-)

Resolved in this commit bc37396

pmuellr · 2023-04-06T20:26:26Z

x-pack/plugins/alerting/server/rules_client/tests/bulk_delete.test.ts

@@ -313,6 +353,21 @@ describe('bulkDelete', () => {
    );
  });

+  test('should not mark API keys for invalidation if the user is authenticated using an api key', async () => {
+    unsecuredSavedObjectsClient.bulkDelete.mockResolvedValue({
+      statuses: [{ id: 'id3', type: 'alert', success: true }],


can we add id1 and id2 here as well, ensure that they get passed to the bulkKeyInvalidation function

Resolved in this commit bc37396

pmuellr · 2023-04-06T20:47:04Z

x-pack/plugins/alerting/server/rules_client_factory.ts

+        const user = await securityPluginStart.authc.getCurrentUser(request);
+        return user && user.authentication_type ? user.authentication_type === 'api_key' : false;
+      },
+      async getAuthenticationAPIKey(name: string) {


I don't believe either of these methods needs to be async, technically. I'm seeing a squiggle under the await for securityPluginStart.authc.getCurrentUser(request) indicating 'await' has no effect on the type of this expression.

Oh my bad, I will fix this!

Resolved in this commit bc37396

mohamedhamed-ahmed

Infra changes LGTM!

kibana-ci · 2023-04-11T14:29:57Z

💚 Build Succeeded

Buildkite Build
Commit: fca96c9

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`alerting`	543	544	+1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`alerting`	44.5KB	44.6KB	+67.0B

Unknown metric groups

API count

id	before	after	diff
`alerting`	564	565	+1

ESLint disabled line counts

id	before	after	diff
`securitySolution`	433	436	+3

Total ESLint disabled count

id	before	after	diff
`securitySolution`	513	516	+3

History

💔 Build #118718 failed 0f046d6
💔 Build #118659 failed 9168b69
💚 Build #118094 succeeded 47d83e6
💚 Build #117850 succeeded 7ba7360
💔 Build #117779 failed 19085c8

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

doakalexi added 9 commits March 31, 2023 10:30

Updating rules client to allow users with api key auth to manage rules

fbb4059

Updating test

5e9e25c

Renaming to match create function

6006e0c

Fixing some of the functional tests

762f461

Fixing more functional tests

7f5fee5

Fixing more failing tests

547fd88

Updating functional tests

f0c11f4

Updating so test

0e5a5d2

Fixing test

fbdd6be

doakalexi changed the title ~~Alerting/auth api key~~ [ResponseOps] Allow users authenticated with an API keys to manage alerting rules Mar 31, 2023

Fixing failing tests after merge

ad13298

doakalexi added Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.8.0 release_note:feature Makes this part of the condensed release notes labels Mar 31, 2023

doakalexi marked this pull request as ready for review April 3, 2023 12:45

doakalexi requested review from a team as code owners April 3, 2023 12:45

doakalexi added 2 commits April 3, 2023 05:46

Merge branch 'main' into alerting/auth-api-key

a60698b

Merge branch 'main' into alerting/auth-api-key

1f64989

afharo approved these changes Apr 4, 2023

View reviewed changes

x-pack/plugins/alerting/server/saved_objects/mappings.ts Outdated Show resolved Hide resolved

doakalexi added 2 commits April 4, 2023 13:43

Remove from mapping

844c2be

Merge branch 'alerting/auth-api-key' of github.com:doakalexi/kibana i…

19085c8

…nto alerting/auth-api-key

shahzad31 approved these changes Apr 4, 2023

View reviewed changes

doakalexi and others added 2 commits April 4, 2023 14:59

Merge branch 'main' into alerting/auth-api-key

74cb639

Adding dynamic false

7ba7360

Updating api key function

47d83e6

This was referenced Apr 6, 2023

[ResponseOps] add docs for new API key functionality #154580

Closed

[ResponseOps] do we need to display info for rules with user-managed API keys? #154581

Closed

doakalexi mentioned this pull request Apr 6, 2023

[ResponseOps] Add functional tests for managing alerting rules when authenticated with an API key #154584

Closed

pmuellr approved these changes Apr 6, 2023

View reviewed changes

doakalexi and others added 4 commits April 10, 2023 09:55

Addressing pr comments

bc37396

Merge branch 'main' into alerting/auth-api-key

9168b69

Merge branch 'main' into alerting/auth-api-key

0f046d6

Merge branch 'main' into alerting/auth-api-key

fca96c9

mohamedhamed-ahmed approved these changes Apr 11, 2023

View reviewed changes

doakalexi enabled auto-merge (squash) April 11, 2023 13:18

doakalexi merged commit 331eb60 into elastic:main Apr 11, 2023

kibanamachine added the backport:skip This commit does not require backporting label Apr 11, 2023

pmuellr mentioned this pull request Jun 22, 2023

[Security Solution] Detections linked to user API keys lifecycle when using API #159445

Open

vitaliidm mentioned this pull request Sep 4, 2023

[Rules management] updating rule through API key elastic/security-docs#3479

Closed

pmuellr mentioned this pull request Nov 22, 2023

[ResponseOps] docs reference APIs as "This API supports Token-based authentication only" #171761

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ResponseOps] Allow users authenticated with an API keys to manage alerting rules #154189

[ResponseOps] Allow users authenticated with an API keys to manage alerting rules #154189

doakalexi commented Mar 31, 2023 •

edited

Loading

elasticmachine commented Apr 3, 2023

afharo left a comment

pmuellr commented Apr 5, 2023

doakalexi commented Apr 5, 2023 •

edited

Loading

pmuellr commented Apr 5, 2023

doakalexi commented Apr 6, 2023

pmuellr left a comment

pmuellr Apr 3, 2023

doakalexi Apr 10, 2023

doakalexi Apr 10, 2023

pmuellr Apr 5, 2023

doakalexi Apr 10, 2023

pmuellr Apr 5, 2023

pmuellr Apr 5, 2023

doakalexi Apr 10, 2023

pmuellr Apr 5, 2023

doakalexi Apr 10, 2023

pmuellr Apr 6, 2023

doakalexi Apr 10, 2023

pmuellr Apr 6, 2023

doakalexi Apr 10, 2023

doakalexi Apr 10, 2023

mohamedhamed-ahmed left a comment

kibana-ci commented Apr 11, 2023

API count

ESLint disabled line counts

Total ESLint disabled count

[ResponseOps] Allow users authenticated with an API keys to manage alerting rules #154189

[ResponseOps] Allow users authenticated with an API keys to manage alerting rules #154189

Conversation

doakalexi commented Mar 31, 2023 • edited Loading

Summary

Checklist

To verify

elasticmachine commented Apr 3, 2023

afharo left a comment

Choose a reason for hiding this comment

pmuellr commented Apr 5, 2023

doakalexi commented Apr 5, 2023 • edited Loading

pmuellr commented Apr 5, 2023

doakalexi commented Apr 6, 2023

pmuellr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohamedhamed-ahmed left a comment

Choose a reason for hiding this comment

kibana-ci commented Apr 11, 2023

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Page load bundle

API count

ESLint disabled line counts

Total ESLint disabled count

History

doakalexi commented Mar 31, 2023 •

edited

Loading

doakalexi commented Apr 5, 2023 •

edited

Loading