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.

Sign up for GitHub

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

Jump to bottom

Settings should be validated at parse time #47711

Closed
danhermann opened this issue Oct 7, 2019 · 5 comments
Closed

Settings should be validated at parse time #47711

danhermann opened this issue Oct 7, 2019 · 5 comments
Assignees
@danhermann
Labels
:Core/Infra/Settings Settings infrastructure and APIs :Data Management/Monitoring Meta

Comments

@danhermann
Copy link
Contributor

danhermann commented Oct 7, 2019 •
edited
Loading

The monitoring settings below are currently validated at usage time meaning that they can be accepted into the cluster state when invalid thereby poisoning the cluster state with the invalid setting. All of the following should be validated as part of the settings validation framework which will eliminate this problem.

This should resolve all of the instances of the problem described in #47038 for settings related to monitoring.
@danhermann danhermann added WIP :Core/Infra/Settings Settings infrastructure and APIs Meta labels Oct 7, 2019
@danhermann danhermann self-assigned this Oct 7, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (:Core/Infra/Settings)

@rjernst
Copy link
Member

rjernst commented Oct 7, 2019

I think the validation of these should look similar to that in #47246. For example, AUTH_USERNAME_SETTING must be set in conjunction with AUTH_PASSWORD_SETTING, and should be checked with a validator for both settings which cross checks the other is set.

@danhermann danhermann mentioned this issue Oct 8, 2019
Merged
@danhermann
Copy link
Contributor Author

@rjernst, there is currently a check that AUTH_PASSWORD_SETTING cannot be set without a corresponding AUTH_USERNAME_SETTING but a username can be set without a password. Should I add a new check that prevents that latter scenario?

@rjernst
Copy link
Member

rjernst commented Oct 8, 2019

Yes, I think both must always be set together? At least the existing check implies that with the error message, and I'm not sure how one could have a username with no password.

@danhermann danhermann removed the WIP label Oct 9, 2019
This was referenced Oct 9, 2019
Merged
Merged
Merged
Merged
This was referenced Oct 31, 2019
Merged
Merged
Merged
This was referenced Nov 14, 2019
Merged
Merged
@jasontedor jasontedor mentioned this issue Dec 9, 2019
Merged
This was referenced Feb 3, 2020
Closed
Closed
@albertzaharovits albertzaharovits mentioned this issue Feb 19, 2020
Merged
albertzaharovits added a commit that referenced this issue Feb 24, 2020
@albertzaharovits
Logfile audit settings validation (#52537)
aa01487 
Add validation for the following logfile audit settings:

    xpack.security.audit.logfile.events.include
    xpack.security.audit.logfile.events.exclude
    xpack.security.audit.logfile.events.ignore_filters.*.users
    xpack.security.audit.logfile.events.ignore_filters.*.realms
    xpack.security.audit.logfile.events.ignore_filters.*.roles
    xpack.security.audit.logfile.events.ignore_filters.*.indices

Closes #52357
Relates #47711 #47038
Follows the example from #47246
@albertzaharovits albertzaharovits mentioned this issue Feb 24, 2020
Merged
albertzaharovits added a commit to albertzaharovits/elasticsearch that referenced this issue Feb 24, 2020
@albertzaharovits
Logfile audit settings validation (elastic#52537)
ec255ec 
Add validation for the following logfile audit settings:

    xpack.security.audit.logfile.events.include
    xpack.security.audit.logfile.events.exclude
    xpack.security.audit.logfile.events.ignore_filters.*.users
    xpack.security.audit.logfile.events.ignore_filters.*.realms
    xpack.security.audit.logfile.events.ignore_filters.*.roles
    xpack.security.audit.logfile.events.ignore_filters.*.indices

Closes elastic#52357
Relates elastic#47711 elastic#47038
Follows the example from elastic#47246
@albertzaharovits albertzaharovits mentioned this issue Feb 24, 2020
Merged
albertzaharovits added a commit to albertzaharovits/elasticsearch that referenced this issue Feb 24, 2020
@albertzaharovits
Logfile audit settings validation (elastic#52537)
a7da064 
Add validation for the following logfile audit settings:

    xpack.security.audit.logfile.events.include
    xpack.security.audit.logfile.events.exclude
    xpack.security.audit.logfile.events.ignore_filters.*.users
    xpack.security.audit.logfile.events.ignore_filters.*.realms
    xpack.security.audit.logfile.events.ignore_filters.*.roles
    xpack.security.audit.logfile.events.ignore_filters.*.indices

Closes elastic#52357
Relates elastic#47711 elastic#47038
Follows the example from elastic#47246
@albertzaharovits albertzaharovits mentioned this issue Feb 24, 2020
Closed
albertzaharovits added a commit that referenced this issue Feb 24, 2020
@albertzaharovits
Logfile audit settings validation (#52537)
c2678ac 
Add validation for the following logfile audit settings:

    xpack.security.audit.logfile.events.include
    xpack.security.audit.logfile.events.exclude
    xpack.security.audit.logfile.events.ignore_filters.*.users
    xpack.security.audit.logfile.events.ignore_filters.*.realms
    xpack.security.audit.logfile.events.ignore_filters.*.roles
    xpack.security.audit.logfile.events.ignore_filters.*.indices

Closes #52357
Relates #47711 #47038
Follows the example from #47246
albertzaharovits added a commit that referenced this issue Feb 24, 2020
@albertzaharovits
Logfile audit settings validation (#52537)
33131e2 
Add validation for the following logfile audit settings:

    xpack.security.audit.logfile.events.include
    xpack.security.audit.logfile.events.exclude
    xpack.security.audit.logfile.events.ignore_filters.*.users
    xpack.security.audit.logfile.events.ignore_filters.*.realms
    xpack.security.audit.logfile.events.ignore_filters.*.roles
    xpack.security.audit.logfile.events.ignore_filters.*.indices

Closes #52357
Relates #47711 #47038
Follows the example from #47246
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features (:Core/Features/Monitoring)

@danhermann danhermann mentioned this issue Mar 12, 2020
Merged
This was referenced Apr 1, 2020
Closed
Closed
@jakelandis jakelandis mentioned this issue May 31, 2020
Merged
jakelandis added a commit that referenced this issue Jun 4, 2020
@jakelandis
Ensure type exists for all monitoring configuration (#57399)
7dde9d4 
#47711 and #47246 helped to validate that monitoring settings are
rejected at time of setting the monitoring settings. Else an invalid
monitoring setting can find it's way into the cluster state and result
in an exception thrown [1] on the cluster state application (there by
causing significant issues). Some additional monitoring settings have
been identified that can result in invalid cluster state that also
result in exceptions thrown on cluster state application.

All settings require a type of either http or local to be
applicable. When a setting is changed, the exporters are automatically
updated with the new settings. However, if the old or new settings lack
of a type setting an exception will be thrown (since exporters are
always of type 'http' or 'local'). Arguably we shouldn't blindly create
and destroy new exporters on each monitoring setting update, but the
lifecycle of the exporters is abit out the scope this PR is trying to
address.

This commit introduces a similar methodology to check for validity as
#47711 and #47246 but this time for ALL (including non-http) settings.
Monitoring settings are not useful unless there an exporter with a type
defined. The type is used as dependent setting, such that it must
exist to set the value. This ensures that when any monitoring settings
changes that they can only get added to cluster state if the type
exists. If the type exists (and the other validations pass) then the
exporters will get re-built and the cluster state remains valid.

Tests have been included to ensure that all dynamic monitoring settings
have the type as dependent settings.

[1]
org.elasticsearch.common.settings.SettingsException: missing exporter type for [found-user-defined] exporter
at org.elasticsearch.xpack.monitoring.exporter.Exporters.initExporters(Exporters.java:126) ~[?:?]
jakelandis added a commit to jakelandis/elasticsearch that referenced this issue Jun 4, 2020
@jakelandis
Ensure type exists for all monitoring configuration (elastic#57399)
b2bbd8e 
elastic#47711 and elastic#47246 helped to validate that monitoring settings are
rejected at time of setting the monitoring settings. Else an invalid
monitoring setting can find it's way into the cluster state and result
in an exception thrown [1] on the cluster state application (there by
causing significant issues). Some additional monitoring settings have
been identified that can result in invalid cluster state that also
result in exceptions thrown on cluster state application.

All settings require a type of either http or local to be
applicable. When a setting is changed, the exporters are automatically
updated with the new settings. However, if the old or new settings lack
of a type setting an exception will be thrown (since exporters are
always of type 'http' or 'local'). Arguably we shouldn't blindly create
and destroy new exporters on each monitoring setting update, but the
lifecycle of the exporters is abit out the scope this PR is trying to
address.

This commit introduces a similar methodology to check for validity as
elastic#47711 and elastic#47246 but this time for ALL (including non-http) settings.
Monitoring settings are not useful unless there an exporter with a type
defined. The type is used as dependent setting, such that it must
exist to set the value. This ensures that when any monitoring settings
changes that they can only get added to cluster state if the type
exists. If the type exists (and the other validations pass) then the
exporters will get re-built and the cluster state remains valid.

Tests have been included to ensure that all dynamic monitoring settings
have the type as dependent settings.

[1]
org.elasticsearch.common.settings.SettingsException: missing exporter type for [found-user-defined] exporter
at org.elasticsearch.xpack.monitoring.exporter.Exporters.initExporters(Exporters.java:126) ~[?:?]
jakelandis added a commit to jakelandis/elasticsearch that referenced this issue Jun 4, 2020
@jakelandis
Ensure type exists for all monitoring configuration (elastic#57399)
71f9a5c 
elastic#47711 and elastic#47246 helped to validate that monitoring settings are
rejected at time of setting the monitoring settings. Else an invalid
monitoring setting can find it's way into the cluster state and result
in an exception thrown [1] on the cluster state application (there by
causing significant issues). Some additional monitoring settings have
been identified that can result in invalid cluster state that also
result in exceptions thrown on cluster state application.

All settings require a type of either http or local to be
applicable. When a setting is changed, the exporters are automatically
updated with the new settings. However, if the old or new settings lack
of a type setting an exception will be thrown (since exporters are
always of type 'http' or 'local'). Arguably we shouldn't blindly create
and destroy new exporters on each monitoring setting update, but the
lifecycle of the exporters is abit out the scope this PR is trying to
address.

This commit introduces a similar methodology to check for validity as
elastic#47711 and elastic#47246 but this time for ALL (including non-http) settings.
Monitoring settings are not useful unless there an exporter with a type
defined. The type is used as dependent setting, such that it must
exist to set the value. This ensures that when any monitoring settings
changes that they can only get added to cluster state if the type
exists. If the type exists (and the other validations pass) then the
exporters will get re-built and the cluster state remains valid.

Tests have been included to ensure that all dynamic monitoring settings
have the type as dependent settings.

[1]
org.elasticsearch.common.settings.SettingsException: missing exporter type for [found-user-defined] exporter
at org.elasticsearch.xpack.monitoring.exporter.Exporters.initExporters(Exporters.java:126) ~[?:?]
jakelandis added a commit that referenced this issue Jun 5, 2020
@jakelandis
[7.x] Ensure type exists for all monitoring configuration (#57399) (#…
459ab9a 
…57704)

#47711 and #47246 helped to validate that monitoring settings are
rejected at time of setting the monitoring settings. Else an invalid
monitoring setting can find it's way into the cluster state and result
in an exception thrown [1] on the cluster state application (there by
causing significant issues). Some additional monitoring settings have
been identified that can result in invalid cluster state that also
result in exceptions thrown on cluster state application.

All settings require a type of either http or local to be
applicable. When a setting is changed, the exporters are automatically
updated with the new settings. However, if the old or new settings lack
of a type setting an exception will be thrown (since exporters are
always of type 'http' or 'local'). Arguably we shouldn't blindly create
and destroy new exporters on each monitoring setting update, but the
lifecycle of the exporters is abit out the scope this PR is trying to
address.

This commit introduces a similar methodology to check for validity as
#47711 and #47246 but this time for ALL (including non-http) settings.
Monitoring settings are not useful unless there an exporter with a type
defined. The type is used as dependent setting, such that it must
exist to set the value. This ensures that when any monitoring settings
changes that they can only get added to cluster state if the type
exists. If the type exists (and the other validations pass) then the
exporters will get re-built and the cluster state remains valid.

Tests have been included to ensure that all dynamic monitoring settings
have the type as dependent settings.

[1]
org.elasticsearch.common.settings.SettingsException: missing exporter type for [found-user-defined] exporter
at org.elasticsearch.xpack.monitoring.exporter.Exporters.initExporters(Exporters.java:126) ~[?:?]
jakelandis added a commit that referenced this issue Jun 5, 2020
@jakelandis
[7.8] Ensure type exists for all monitoring configuration (#57399) (#…
51c79da 
…57703)

#47711 and #47246 helped to validate that monitoring settings are
rejected at time of setting the monitoring settings. Else an invalid
monitoring setting can find it's way into the cluster state and result
in an exception thrown [1] on the cluster state application (there by
causing significant issues). Some additional monitoring settings have
been identified that can result in invalid cluster state that also
result in exceptions thrown on cluster state application.

All settings require a type of either http or local to be
applicable. When a setting is changed, the exporters are automatically
updated with the new settings. However, if the old or new settings lack
of a type setting an exception will be thrown (since exporters are
always of type 'http' or 'local'). Arguably we shouldn't blindly create
and destroy new exporters on each monitoring setting update, but the
lifecycle of the exporters is abit out the scope this PR is trying to
address.

This commit introduces a similar methodology to check for validity as
#47711 and #47246 but this time for ALL (including non-http) settings.
Monitoring settings are not useful unless there an exporter with a type
defined. The type is used as dependent setting, such that it must
exist to set the value. This ensures that when any monitoring settings
changes that they can only get added to cluster state if the type
exists. If the type exists (and the other validations pass) then the
exporters will get re-built and the cluster state remains valid.

Tests have been included to ensure that all dynamic monitoring settings
have the type as dependent settings.

[1]
org.elasticsearch.common.settings.SettingsException: missing exporter type for [found-user-defined] exporter
at org.elasticsearch.xpack.monitoring.exporter.Exporters.initExporters(Exporters.java:126) ~[?:?]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@danhermann danhermann

Labels
:Core/Infra/Settings Settings infrastructure and APIs :Data Management/Monitoring Meta
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@rjernst @elasticmachine @danhermann