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

[Alerting] action validators should be passed allow-list config utils #64659

Closed
pmuellr opened this issue Apr 28, 2020 · 2 comments · Fixed by #139438
Closed

[Alerting] action validators should be passed allow-list config utils #64659

pmuellr opened this issue Apr 28, 2020 · 2 comments · Fixed by #139438
Assignees
Labels
estimate:small Small Estimated Level of Effort Feature:Actions/Framework Issues related to the Actions Framework Feature:Actions Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) technical debt Improvement of the software architecture and operational architecture

Comments

@pmuellr
Copy link
Member

pmuellr commented Apr 28, 2020

While reviewing PR #63450, and thinking about moving the code that PR adds to the actions plugin to a case-specific plugin, it seems like we need to expose our whitelisting validating checks, which are currently private to the actions plugin.

Here's an example usage:

function valdiateActionTypeConfig(
configurationUtilities: ActionsConfigurationUtilities,
configObject: ActionTypeConfigType
) {
try {
configurationUtilities.ensureWhitelistedUri(getPagerDutyApiUrl(configObject));
} catch (whitelistError) {
return i18n.translate('xpack.actions.builtin.pagerduty.pagerdutyConfigurationError', {
defaultMessage: 'error configuring pagerduty action: {message}',
values: {
message: whitelistError.message,
},
});
}
}

See the code above that to see how configObject is curried onto the validator params.

I think what we should probably do is add an optional validatorServices parameter object to all the validators - config, secrets, params (params not technically needed at this point, but might as well be consistent, and maybe needed in the future). Validators that need to whitelist will then just get the APIs needed to do the check in that object, without having to curry. Those that don't, won't need to change.

@pmuellr pmuellr added Feature:Alerting Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) labels Apr 28, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

@pmuellr pmuellr self-assigned this May 8, 2020
pmuellr added a commit to pmuellr/kibana that referenced this issue May 9, 2020
resolves elastic#64659

Previously, for action validators that needed to perform whitelisting
checks, they would need to curry in the config utilties providing the
whitelisting functions, into their validators.  This is unwieldy -
the config utilities should be passed explicitly in.
pmuellr added a commit to pmuellr/kibana that referenced this issue May 26, 2020
elastic#64659

In this PR, the action whitelisting utilities have been refactored to allow
them to (eventually) be used in plugins other than the actions plugin.  Prior
to this, actions required deeper integration with the internal of the
actions plugin.

This also adds some generic typing to the actionType config, secrets, and
params properties, since they are now referred to in multiple places within
the actionType.
@mikecote mikecote changed the title [Alerting] action validators should be passed whitelist config utils [Alerting] action validators should be passed allow-list config utils Feb 10, 2021
@pmuellr
Copy link
Member Author

pmuellr commented Feb 10, 2021

At this point, the scope has moved beyond the allow list. It should also take into account the proxy and http/https agent in general, and probably the axios utilities.

@YulNaumenko YulNaumenko added the technical debt Improvement of the software architecture and operational architecture label Mar 11, 2021
@gmmorris gmmorris added Feature:Actions Feature:Actions/Framework Issues related to the Actions Framework and removed Feature:Alerting labels Jul 1, 2021
@gmmorris gmmorris added the loe:medium Medium Level of Effort label Jul 15, 2021
@gmmorris gmmorris added the estimate:small Small Estimated Level of Effort label Aug 18, 2021
@gmmorris gmmorris removed the loe:medium Medium Level of Effort label Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
estimate:small Small Estimated Level of Effort Feature:Actions/Framework Issues related to the Actions Framework Feature:Actions Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) technical debt Improvement of the software architecture and operational architecture
Projects
No open projects
6 participants