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

[Stack monitoring] Do not expose monitoring.ui.elasticsearch configuration to the browser #123269

Closed
2 tasks
Tracked by #127224
smith opened this issue Jan 18, 2022 · 5 comments · Fixed by #128938
Closed
2 tasks
Tracked by #127224
Assignees
Labels
Feature:Stack Monitoring Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v8.2.0

Comments

@smith
Copy link
Contributor

smith commented Jan 18, 2022

The monitoring.ui.elasticsearch configuration object is exposed to the browser (the whole ui object is exposed in servcer/index.ts.)

The data in this object is needed to connect to the monitoring cluster, but is it necessary to expose this entire object to the browser? It could contain sensitive data about the cluster credentials.

  • Determine which keys of browsermonitoring.ui.elasticsearch, if any, are needed in the browser
  • Only expose the necessary keys to the browser
@smith smith added Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services Feature:Stack Monitoring labels Jan 18, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)

@klacabane
Copy link
Contributor

klacabane commented Mar 23, 2022

Quick lookup shows that we're reading the following values from monitoring.ui in public/plugin.ts:

  • enabled
  • min_interval_seconds
  • show_license_expiration
  • container.elasticsearch.enabled
  • container.logstash.enabled
  • ccs.enabled - ccs_utils.ts

Ideally we'd define a schema with only the nested keys that we want to expose but exposeToBrowser only supports root keys. Implementing nested key support could be an option, I'll look for potential blockers and otherwise alternatives

@smith
Copy link
Contributor Author

smith commented Mar 24, 2022

Could we also look at flattening the ones we need and only exposing those as separate config items?

@klacabane
Copy link
Contributor

klacabane commented Mar 24, 2022

Yup, since we can update that config at runtime we can create a root object out of the properties required by the UI. We'll have to update the paths where we directly access monitoring.ui.{prop} in the UI code and we should be good, or maybe a proxy could abstract this.
Ability to only define a tree in exposeToBrowser looks more intuitive to me though but let's consider that as a future improvement

@klacabane
Copy link
Contributor

I've opened a feature request to handle recursive definition of the configuration which was taken care of instantly, I'll hold on this PR until the feature is merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Stack Monitoring Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v8.2.0
Projects
None yet
4 participants