-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Change action config / params schema from joi to @kbn/config-schema #40694
Conversation
fixes elastic#40177 Prior to this, the `actionTypeConfig` was not excluded from AAD when using encrypted saved objects in actions. https://github.com/elastic/kibana/blob/d0da71c2b4b154fe2efe86b44869c06709c15d14/x-pack/legacy/plugins/actions/server/init.ts#L31-L35 This caused a problem when updating values in the `actionTypeConfig`, as per issue elastic#40177 Also added `x-pack/test/functional/es_archives/actions/README.md` to explain how to get the id and encrypted value string, if this needs to be done again later, since it's a little tricky.
Pinging @elastic/kibana-stack-services |
💔 Build Failed |
Alert happened to reuse the archived action, so it's reference to the action also had to be updated.
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think excluding actionTypeConfig
(which contains host, post, username, etc) from AAD goes against what it was developed for. We want the encryption to change when the configuration changes.
This change could allow vulnerabilities where you could discover the encrypted password by changing host. I'm not 100% certain on how this works, maybe @azasypkin or @kobelb can chime in?
I think if we fix the config cleanup on update (comment below), it will make the original issue go away without needing to worry about AAD. I'm happy to help investigate this as it may not be as easy to follow.
}, | ||
}) | ||
.expect(200); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I add the following here, I can see the update still didn't clear the old values. The attributes host
and port
are still returned. I think there's something wrong with the mapping for actionTypeConfig
in elasticsearch that is causing this.
const { body: fetchedAction } = await supertest
.get(`/api/action/${emailActionId}`)
.expect(200);
expect(fetchedAction).to.eql({
attributes: {
description: 'a test email action 2',
actionTypeId: '.email',
actionTypeConfig: {
from: 'email-from@example.com',
service: '__json',
},
},
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I probably should have mentioned that I view the "partial update" issue as separate. My assumption is that the old values are merged with the new, somewhere in the update processing, before writing the entry to ES, and so needs to be fixed there. In the case above, I would expect host
and port
to NOT be in a subsequent GET, but they are there now.
I have a card for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want the encryption to change when the configuration changes.
Do we? I assumed if you made a change to a non-encrypted config property, you wouldn't need to change the encryption. Guess we need to figure out what we want to happen. And this is how it used to work, but then for whatever reason, we couldn't decrypt the entries when non-encrypted properties were changed, so we'd need to figure out HOW to make it work :-)
This change could allow vulnerabilities where you could discover the encrypted password by changing host.
More info on this, please! I assume you mean the discovery would be via ES queries of the saved objects directly, somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the problem where it merges old values with new is the cause of the Unsupported state or unable to authenticate data
errors. It's a really strange issue by itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we?
I'll let @kobelb or @azasypkin jump in for this one as I'm relaying the information I've been told. Something along the lines from #40022 (comment).
More info on this, please!
Let's use this scenario:
- Load an email action, capture
host
attribute - Change that action's
host
to my custom SMTP server that will capture passwords - Fire the action
- Find what password was attempted to authenticate with
- You now have the password of the original SMTP
host
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mikecote has done a good job of capturing the primary concerns. We want to use the AAD to "tie" the encrypted attributes to anything that can be controlled to where we send the encrypted attributes like "host", "type", etc. The AAD essentially ensures that the credentials are only used for the original purpose that the author specified and you can't do things like take the credentials that someone configure for slack, change them to be sent to another service which I have control of and capture the credentials in that manner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an attack that can occur even if the secrets are re-encrypted when non-secrets change, right?
We DO need to figure out the security story here. My assumption is that there will be some kind of "action-admin" permission that will allow users to create/update/delete actions (mutate the config secrets and non-secrets), and that permission would also return config secrets on read (without that permission, you can get config non-secrets, but not config secrets).
In that world, only admins can change host
, and those users can do anything anyway, so ... not clear this is an issue, in my mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can't do things like take the credentials that someone configure for slack, change them to be sent to another service which I have control of and capture the credentials in that manner.
This seems to imply there would be a permission story that would let you change non-encrypted config properties but not encrypted config properties. Sounds complicated. If you imagine there is a single mutation permission that covers all config properties, is this still a problem? That's the assumption I was working off of ...
💔 Build Failed |
💔 Build Failed |
Current status is:
Side-effects of using
|
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I do think we should open an issue for the partial update issue with ESOs, as others will run into the object corrupted by partial updates issue. I noted one style nit, but feel free to ignore.
// Based on data here: | ||
// - https://github.com/nodemailer/nodemailer/blob/master/lib/well-known/services.json | ||
function getSecureValue(secure: boolean | null | undefined, port: number): boolean { | ||
if (secure != null) return secure; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Kind of surprised this passes linting, I thought the one-line if style was not how we did things in Kibana. Anyway, I definitely prefer the (admittedly unnecessary) {}
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can live with no one-line if's if the linter complains, but for short simple functions I actually prefer it. The problem for me with one-line's is that you often can't set a breakpoint on the statement, just the if, so can be a pain to debug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one nit but the changes LGTM!
import nodemailerServices from 'nodemailer/lib/well-known/services.json'; | ||
|
||
import { sendEmail, JSON_TRANSPORT_SERVICE } from './lib/send_email'; | ||
import { ActionType, ActionTypeExecutorOptions } from '../types'; | ||
|
||
const PORT_MAX = 256 * 256 - 1; | ||
|
||
export type ActionTypeConfigType = TypeOf<typeof ActionTypeConfig.schema>; | ||
function nullableType<V>(type: Type<V>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we'll probably re-use this in the future. I guess this will move somewhere common (types.ts or w/e) when a second action could use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second time we need it, I'll see if we can't add it to config-schema proper. Seems generally useful.
password: 'email-password', | ||
from: 'email-from@example.com', | ||
host: 'host-is-ignored-here.example.com', | ||
port: 666, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
666 🔥
💚 Build Succeeded |
…lastic#40694) * Adds actionTypeConfig to AAD exclusion for action ESOs fixes elastic#40177 Prior to this, the `actionTypeConfig` was not excluded from AAD when using encrypted saved objects in actions. https://github.com/elastic/kibana/blob/d0da71c2b4b154fe2efe86b44869c06709c15d14/x-pack/legacy/plugins/actions/server/init.ts#L31-L35 This caused a problem when updating values in the `actionTypeConfig`, as per issue elastic#40177 Also added `x-pack/test/functional/es_archives/actions/README.md` to explain how to get the id and encrypted value string, if this needs to be done again later, since it's a little tricky. * change alertings reference to actions archived action Alert happened to reuse the archived action, so it's reference to the action also had to be updated.
…40694) (#41161) * Adds actionTypeConfig to AAD exclusion for action ESOs fixes #40177 Prior to this, the `actionTypeConfig` was not excluded from AAD when using encrypted saved objects in actions. https://github.com/elastic/kibana/blob/d0da71c2b4b154fe2efe86b44869c06709c15d14/x-pack/legacy/plugins/actions/server/init.ts#L31-L35 This caused a problem when updating values in the `actionTypeConfig`, as per issue #40177 Also added `x-pack/test/functional/es_archives/actions/README.md` to explain how to get the id and encrypted value string, if this needs to be done again later, since it's a little tricky. * change alertings reference to actions archived action Alert happened to reuse the archived action, so it's reference to the action also had to be updated.
fixes #40177
Prior to this, the
actionTypeConfig
was not excluded from AAD when usingencrypted saved objects in actions.
kibana/x-pack/legacy/plugins/actions/server/init.ts
Lines 31 to 35 in d0da71c
This caused a problem when updating values in the
actionTypeConfig
, as perissue #40177
Also added
x-pack/test/functional/es_archives/actions/README.md
to explainhow to get the id and encrypted value string, if this needs to be done again
later, since it's a little tricky.