Icinga / icinga-core

Icinga 1.x, the old core (EOL 31.12.2018)
GNU General Public License v2.0
45 stars 27 forks source link

[dev.icinga.com #2291] unknown macros are not replaced, and misleading to single dollar signs #855

Closed icinga-migration closed 12 years ago

icinga-migration commented 12 years ago

This issue has been migrated from Redmine: https://dev.icinga.com/issues/2291

Created by mfriedrich on 2012-01-29 00:21:22 +00:00

Assignee: mfriedrich Status: Resolved (closed on 2012-08-31 11:10:53 +00:00) Target Version: 1.8 Last Update: 2012-08-31 11:10:53 +00:00 (in Redmine)

Icinga Version: 1.7.1
OS Version: Debian

if you are using macros which are not valid in the current situation, such as using $CONTACTEMAIL$ within a check command, or a service only macro while executing a host check/notification, this will put the macro as is on the shell.

this is then being interpreted as $CONTACTEMAIL (empty) and $ ... and then confusing the user that there was only a dollar sign as macro replacement.

-------- Original Message --------
Subject:    [Nagios-devel] Non existent macros are not replaced
Date:   Thu, 3 Mar 2011 17:39:48 +0100
From:   Matthieu Kermagoret 
Reply-To:   Nagios Developers List 
To:     Nagios Developers List 

Hi list,

When using a non-existent macros in a command (like for example
$SERVICEDESC$ in a host notification command), such macro is not
replaced. It is instead left as-is on the command line.

Such construct is interpreted by the shell as a variable (usually
empty) followed by a dollar sign which (usually) expands to a single
dollar and lead users to believe that Nagios replace non-existent
macros with a single dollar (see ticket #34
http://tracker.nagios.org/view.php?id=34).

I believe this behavior is not intuitive and should therefore be
modified to simply remove non-existent macros, as the attached patch
does.

What do you think about it ?

Best regards,

-- 
Matthieu KERMAGORET | Développeur

mkermagoret@merethis.com

MERETHIS est éditeur du logiciel Centreon.

http://tracker.nagios.org/view.php?id=34

Attachments

Changesets

2012-08-22 17:46:32 +00:00 by mfriedrich 02341c6222e603d3357016a11714e28a92e33169

core: unknown macros are not replaced, and misleading to single dollar signs #2291 - MF

people report bugs, that check_bla -c $foo$ will lead into errors, like
the shell interpreting $foo as variable, but $ throws an error then.
others tell to put quotes around, especially having a password.

the correct way of putting dollar signs into command lines is to
properly escape them with another dollar sign (see the docs). all other
methods are not valid, and only circumvent the real problem.

other than that, the config could contain macros which are not valid for
the object executing the command. those macros were not replaced either
and therefore causing the commands to fail, leading to the infamous bug
reports too.

the best way to enforce this being fixed is logging a warning instead of
just leaving that on the shell. default will now remove the unknown
macro string from the macro buffer, leading to safety.

since this will possibly break all other existing setups, having their
own tricks and workarounds applied, we'll add the new icinga.cfg option
keep_unknown_macros, which can be re-enabled in order to revert to the
old, buggy behaviour.

it's up to each user which to chose, in favor of resolving a long
lasting bug, the default will change with 1.8.

refs #2291

2012-10-08 13:28:14 +00:00 by mfriedrich d5f9efdfefe20e68e49259b93405c08957d6c64d

tests: update #2291 with user macro test refs #2291

Relations:

icinga-migration commented 12 years ago

Updated by mfriedrich on 2012-01-29 00:39:04 +00:00

one major disadvantage removing the output - strings containing two dollar signs could be accidently interpreted as macro, i.e. passwords. so this must be evaluated if to be resolved differently, or just updating the documentation on possible errors.

another reason for removing it might be the case that all dollar signs need to be escaped by a second one, i.e. "$$foo$$bar" becomes "$foo$bar" afterwards.

-------- Original Message --------
Subject:    Re: [Nagios-devel] Non existent macros are not replaced
Date:   Thu, 03 Mar 2011 19:33:47 +0100
From:   Andreas Ericsson 
Reply-To:   Nagios Developers List 
To:     Nagios Developers List 
CC:     Jochen Bern 

On 03/03/2011 06:37 PM, Jochen Bern wrote:
> On 03/03/2011 05:54 PM, Andreas Ericsson wrote:
>> I think that would be quite stupid, as it would prevent users from
>> using shell variables in their commands, or multiple dollar-signs
>> for that matter.
> 
> Not quite, the handling of escaped dollar signs ("$$") is in a separate
> branch immediately preceding the "unknown macro name" branch. Hence, the
> properly escaped notation ("$$FOO$$BAR" in Nagios, becomes "$FOO$BAR" on
> shell level) would still be available.
> 

Yes, but how about a command_line such as this:

       $USER1$/check_something '$pas$word'

$pas$ is not a valid macro, so by the reasoning of the original poster
we should remove it, but that would mean there can *never* be two
dollar-signs that aren't part of macros inside anything where macros
get interpreted, and that would be a real limitation put in place to
prevent the minor discomfort of having to redo a botched job.

I just flat out refuse to accept patches with those effects.

> (I admit I wasn't aware that it's the *shell* which reduces "$FOO$" to
> "$", rather than Nagios.)
> 
> On one hand, trusting Nagios to leave "$FOO$" be (rather than properly
> configuring "$$FOO$$") is not supported.

It is. If the dollar-signs are within single-quotes the shell won't even
touch them and '$FOO$' gets passed verbatim to whatever command one's
trying to run.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Considering the successes of the wars on alcohol, poverty, drugs and
terror, I think we should give some serious thought to declaring war
on peace.

-------- Original Message --------
Subject:    Re: [Nagios-devel] Non existent macros are not replaced
Date:   Thu, 03 Mar 2011 20:08:16 +0100
From:   Jochen Bern 
Reply-To:   Nagios Developers List 
To:     Nagios Developers List 

On 03/03/2011 07:33 PM, Andreas Ericsson wrote:
> On 03/03/2011 06:37 PM, Jochen Bern wrote:
>> Not quite, the handling of escaped dollar signs ("$$") is in a separate
>> branch immediately preceding the "unknown macro name" branch. Hence, the
>> properly escaped notation ("$$FOO$$BAR" in Nagios, becomes "$FOO$BAR" on
>> shell level) would still be available.
> 
> Yes, but how about a command_line such as this:
>        $USER1$/check_something '$pas$word'
> $pas$ is not a valid macro, so by the reasoning of the original poster
> we should remove it, but that would mean there can *never* be two
> dollar-signs that aren't part of macros inside anything where macros
> get interpreted

I'm not saying that you should follow his reasoning, but why you keep
ignoring the official escaping method?

"Also, if you want to pass a dollar sign ($) on the command line, you
have to escape it with another dollar sign."
-- http://nagios.sourceforge.net/docs/3_0/objectdefinitions.html#command

# grep -n 'pas.*word' ../var/objects.cache
612:    command_name    test_password
613:    command_line    /bin/echo '$$pas$$word'
35520:  check_command   test_password
# grep -n 'pas.*word' ../var/spool/status.dat
54898:  check_command=test_password
54924:  plugin_output=$pas$word

>> On one hand, trusting Nagios to leave "$FOO$" be (rather than properly
>> configuring "$$FOO$$") is not supported.
> 
> It is. If the dollar-signs are within single-quotes the shell won't even
> touch them and '$FOO$' gets passed verbatim to whatever command one's
> trying to run.

The above quote from the docs says it isn't ("you HAVE to"). But as I
said, *enforcing* that out of the blue sky is problematic, to say the least.

Kind regards,
                                J. Bern
icinga-migration commented 12 years ago

Updated by mfriedrich on 2012-04-03 07:14:32 +00:00

i'm still undecided if it makes sense to clean non-existant macros away, or maybe allow re-enabling with a config flag?

icinga-migration commented 12 years ago

Updated by mfriedrich on 2012-04-19 14:48:00 +00:00

postpone it, and leave the discussion open.

icinga-migration commented 12 years ago

Updated by mfriedrich on 2012-07-06 17:45:11 +00:00

hm, that could be made a config option, allowing users to restore compatibility again (if they know what they do, as most the users having these errors, actually don't).

icinga-migration commented 12 years ago

Updated by idl0r on 2012-07-30 08:05:23 +00:00

A config option to let the user decide whether to strip away the non-existant macros would be perfect. +1 on that :P

icinga-migration commented 12 years ago

Updated by mfriedrich on 2012-08-09 15:02:38 +00:00

dev decision:

icinga-migration commented 12 years ago

Updated by mfriedrich on 2012-08-22 17:37:09 +00:00

Aug 22 19:32:01 icinga-dev icinga: Warning: Skipping unknown macro '$UNKNOWNHOSTMACRO1$', removing it from output! Fix your config, or set keep_unknown_macros accordingly...

Aug 22 19:36:45 icinga-dev icinga: Warning: Skipping unknown macro '$UNKNOWSERVICENMACRO1$', removing it from output! Fix your config, or set keep_unknown_macros accordingly...
icinga-migration commented 12 years ago

Updated by mfriedrich on 2012-08-22 17:38:10 +00:00

# KEEP UNKNOWN MACROS
# This option can be used to keep unknown macros within the output.
# e.g. check_proc -C $foo$ will remain.
# This was the default in versions older than Icinga 1.8, but now
# the default is to remove those macros from the output, causing
# the shell to interpret $foo and leaving a single $ there. See
# #2291 for further information.
# Make sure to escape single dollar signs with another '$', as the
# docs describe. Other than that, enable this setting to revert to
# the old behaviour.

keep_unknown_macros=0
icinga-migration commented 12 years ago

Updated by mfriedrich on 2012-08-22 17:42:38 +00:00

this is a behavioural change, which should be marked in CHANGES section. people wanting the old behavior can revert that by setting

keep_unknown_macros=1

where the macro outputbuffer is kept as is. the default case will be to warn the user on unkbown macros.

that can be

it possibly not work with the shell tricks for passwords, such as

'$mypassisquoted$fortheshell'

the correct way of defining that one will be

'$$mypassisquoted$$fortheshell'

in order to let the core know that there's actually a valid dolar sign. that method is described in the docs, but i believe people have probably circumvented that in various ways, so prepare for the clusterfuck either way.

but, as a matter of fact, leaving that on the shell for interpreting $foo and erroring out on $ this should be fixed either way.

icinga-migration commented 12 years ago

Updated by mfriedrich on 2012-08-22 17:43:35 +00:00

icinga-migration commented 12 years ago

Updated by mfriedrich on 2012-08-22 17:54:10 +00:00

in dev/core - please test!

icinga-migration commented 12 years ago

Updated by mfriedrich on 2012-08-31 11:10:53 +00:00