Page MenuHomePhabricator

Fix unquoted URL parameters in Icinga health checks
Closed, ResolvedPublic

Description

In T304237#7790867 it was discovered that most http-related checks in our Icinga configuration don't quote the URL argument, and as such might lead to unwanted results.

In particular if an URL contains an ampersend (&), the rest of the URL/check parameters are truncated and the check is executed in background, always returning 0 (OK) to Icinga even in case of failure.
This gives the false security of checking something that will not be actually checked.

This is the full list of check commands in modules/nagios_common/templates/checkcommands.cfg.erb that use the URL parameter without quotes:

check_https_port_status
check_http_unauthorized
check_https_unauthorized
check_https_client_auth_puppet
check_https_client_auth_puppet_post
check_http_redirect
check_https_redirect
check_https_sso_redirect
check_http_url_at_address_for_string_with_timeout
check_http_url_at_address_for_string
check_https_url_at_address_for_string
check_https_url_at_address_for_minsize
check_http_url_for_string
check_https_url_for_string
check_http_url_for_regexp_on_port
check_http_url
check_https_url
check_https_url_custom_ip

I didn't grep all of them into the puppet repo or the Icinga configuration to extract the current amount of affected checks.
Keep into account that this is also related to T304321, and many of the above checks are both using and URL and the -Cparameter, so the URL is actually ignored.
I think this one too should be fixed ASAP.

Event Timeline

Volans triaged this task as High priority.

Change 772448 had a related patch set uploaded (by Filippo Giunchedi; author: Filippo Giunchedi):

[operations/puppet@production] nagios: quote check_http url/string parameters

https://gerrit.wikimedia.org/r/772448

Change 772448 merged by Filippo Giunchedi:

[operations/puppet@production] nagios: quote check_http url/string parameters

https://gerrit.wikimedia.org/r/772448

Change 772802 had a related patch set uploaded (by Volans; author: Volans):

[operations/puppet@production] icinga: remove quotes from ereg parameter

https://gerrit.wikimedia.org/r/772802

Change 772802 merged by Volans:

[operations/puppet@production] icinga: remove quotes from ereg parameter

https://gerrit.wikimedia.org/r/772802

Change 772806 had a related patch set uploaded (by Volans; author: Volans):

[operations/puppet@production] icinga: avoid double quoting for the URL

https://gerrit.wikimedia.org/r/772806

Change 772806 merged by Volans:

[operations/puppet@production] icinga: avoid double quoting for the URL

https://gerrit.wikimedia.org/r/772806

Unfortunately this had some follow up alert (some expected) due to double quoting, done both in the caller and the command definition. I think we should check all the calls to make sure they are not double quoted.
git grep "check_http" | grep "'" and git grep "check_http" | grep '"' seems to be a good place to catch them all.

When fixing the calls to check_http_url_for_regexp_on_port make sure to re-add the quotes removed in https://gerrit.wikimedia.org/r/c/operations/puppet/+/772802/

Change 774377 had a related patch set uploaded (by Filippo Giunchedi; author: Filippo Giunchedi):

[operations/puppet@production] icinga: quote check_http_url_for_regexp_on_port regex argument

https://gerrit.wikimedia.org/r/774377

Change 774407 had a related patch set uploaded (by Filippo Giunchedi; author: Filippo Giunchedi):

[operations/puppet@production] icinga: remove double quoting for gerrit health check

https://gerrit.wikimedia.org/r/774407

Change 774408 had a related patch set uploaded (by Filippo Giunchedi; author: Filippo Giunchedi):

[operations/puppet@production] lists: remove double quoting for http check

https://gerrit.wikimedia.org/r/774408

looks like icinga mailman checks are affected by this T304886

Change 774540 had a related patch set uploaded (by Dzahn; author: Dzahn):

[operations/puppet@production] icinga/lists: fix double quoted mailman monitoring check commands

https://gerrit.wikimedia.org/r/774540

Change 774540 abandoned by Dzahn:

[operations/puppet@production] icinga/lists: fix double quoted mailman monitoring check commands

Reason:

duplicate of https://gerrit.wikimedia.org/r/c/operations/puppet/+/774408

https://gerrit.wikimedia.org/r/774540

Change 774408 merged by Filippo Giunchedi:

[operations/puppet@production] lists: remove double quoting for http check

https://gerrit.wikimedia.org/r/774408

Change 774407 merged by Filippo Giunchedi:

[operations/puppet@production] icinga: remove double quoting for gerrit health check

https://gerrit.wikimedia.org/r/774407

Change 774377 merged by Filippo Giunchedi:

[operations/puppet@production] icinga: quote check_http_url_for_regexp_on_port regex argument

https://gerrit.wikimedia.org/r/774377

fgiunchedi claimed this task.

I believe with the latest patches merged all check_http urls are quoted now, tentatively resolving!

Krinkle renamed this task from Unquoted URL parameter to Fix unquoted URL parameters in Icgina health checks.Apr 21 2022, 6:45 PM
colewhite renamed this task from Fix unquoted URL parameters in Icgina health checks to Fix unquoted URL parameters in Icinga health checks.Apr 21 2022, 7:38 PM