updating action config yields Unsupported state or unable to authenticate data

[pmuellr]

Kibana version: master

Elasticsearch version: n/a

Server OS version: n/a

Browser version: n/a

Browser OS version: n/a

Original install method (e.g. download page, yum, from source, etc.): dev

Describe the bug:

The slack action throws a not useful error when the webhook url is invalid.

Steps to reproduce:

1. install kbn-action
2. start the functional test server
3. run

kbn-action create .email "email action" '{"user":"X","password":"X","from":"x@gmail.com","service":"gmail"}'

4. get the action id from the response, to use in the next commands
5. run (changing from using config service to using host and port instead)

kbn-action update ${actionID} "email action" '{"user":"X", "password":"X","from":"x@gmail.com", "host":"example.com", "port": 80}'

6. run

kbn-action fire ${actionID} '{"subject":"testing","message":"hallo","to":["pmuellr@gmail.com"]}'

kbn-action logs:

kbn-action: status code 500
body: {
    "statusCode": 500,
    "error": "Internal Server Error",
    "message": "An internal server error occurred"
}

Expected behavior:

if the various parms had been valid, should send an email

Provide logs and/or server output (if relevant):

   │ proc [kibana] server    log   [12:28:13.401] [error][encrypted_saved_objects] Failed to decrypt "actionTypeConfigSecrets" attribute: Unsupported state or unable to authenticate data
   │ proc [kibana] server   error  [12:28:13.373]  Error: Unable to decrypt attribute "actionTypeConfigSecrets"
   │ proc [kibana]     at EncryptedSavedObjectsService.decryptAttributes (/Users/pmuellr/Projects/elastic/kibana/x-pack/legacy/plugins/encrypted_saved_objects/server/lib/encrypted_saved_objects_service.ts:244:15)

[elasticmachine]

Pinging @elastic/kibana-stack-services

[pmuellr]

wonder if this is somehow related to AAD w/encrypted saved objects, but I was poking through those bits yesterday and I think we've done the right thing there ...

[pmuellr]

Appears to be AAD-related:

kibana/x-pack/legacy/plugins/actions/server/init.ts

Line 34 in 3a1842c

attributesToExcludeFromAAD: new Set(['description']),

[pmuellr]

Also, appears we are doing "partial updates", and the result from an update and subsequent get are not the same.

In the case below, the action has a config property of "service": "gmail" before the update, but we really want that removed and replaced with the new "host" and "port" properties:

$ kbn-action update cfe35848-0304-4374-b919-299ce1bb6b88 "email action" \
    '{"user":"X", "password":"X","from":"x@gmail.com", "host":"example.com", "port": 80}'
{
    "id": "cfe35848-0304-4374-b919-299ce1bb6b88",
    "type": "action",
    "updated_at": "2019-07-02T16:26:56.467Z",
    "version": "WzI4LDFd",
    "references": [],
    "attributes": {
        "description": "email action",
        "actionTypeConfig": {
            "host": "example.com",
            "port": 80,
            "from": "x@gmail.com"
        },
        "actionTypeId": ".email"
    }
}

$ kbn-action get cfe35848-0304-4374-b919-299ce1bb6b88
{
    "id": "cfe35848-0304-4374-b919-299ce1bb6b88",
    "type": "action",
    "updated_at": "2019-07-02T16:26:56.467Z",
    "version": "WzI4LDFd",
    "attributes": {
        "actionTypeId": ".email",
        "description": "email action",
        "actionTypeConfig": {
            "service": "gmail",
            "from": "x@gmail.com",
            "port": 80,
            "host": "example.com"
        }
    },
    "references": []
}

[pmuellr]

So the consensus seems to be that the existing "partial updates" that are happening in master are what's causing the AAD issue, and that if we fix that, the original symptom we're seeing of Unsupported state or unable to authenticate data won't happen.

Also, since we're not exposing AAD to clients via the API directly, clients can't build an action config where non-encrypted config properties are made part of AAD - it's all or nothing, and we need to decide which it will be. Safer to be all, it seems, since there's little downside to this (eg, performance-wise).

Given all that, seems like we'll end up NOT merging this PR. I'm going to start working on a separate PR to fix the partial update problem, which will hopefully resolve the original issue.

[pmuellr]

Rather than close the original PR #40694 that was open to "fix" this issue, since I initially fixed it "wrong", I decided to just do the right thing with more commits - I already had some decent test cases and such.

So current status is:

• existing actions have been converted to use @kbn/schema-config to describe their config and params
• the email config has been set up via config-schema to never use undefined values for properties, but null instead - the undefined values it was allowing was the cause of the "partial updates" we were seeing
• the non-encrypted config properties are now considered for AAD, just as they were before I created this branch; with email config no longer doing partial updates, we shouldn't see the original issue reported at the top

Side-effects of using @kbn-schema-config:

• the validation errors messages provided are MUCH better than the ones we had to scrape out of joi
• we also get TS types from them, and can be used with "extra validation" functions action providers might want to use, like our email one: (note: bit of a hack to a avoid a circular reference):

kibana/x-pack/legacy/plugins/actions/server/builtin_action_types/email.ts

Lines 40 to 64 in 7260dd7

	function validateConfig(configObject: any): string | void {
	// avoids circular reference ...
	const config: ActionTypeConfigType = configObject;
	// Make sure service is set, or if not, both host/port must be set.
	// If service is set, host/port are ignored, when the email is sent.
	if (config.service == null) {
	if (config.host == null && config.port == null) {
	return 'either [service] or [host]/[port] is required';
	}
	if (config.host == null) {
	return '[host] is required if [service] is not provided';
	}
	if (config.port == null) {
	return '[port] is required if [service] is not provided';
	}
	} else {
	// service is not null
	if (!isValidService(config.service)) {
	return `[service] value "${config.service}" is not valid`;
	}
	}
	}