Project

General

Profile

Actions

Bug #19173

closed

Won't fix: Incoherent escaping between Technique Editor and Rudder Technique, and unexpected reports or behaviour in 6.X

Added by Nicolas CHARLES almost 3 years ago. Updated about 2 years ago.

Status:
Resolved
Priority:
N/A
Assignee:
-
Category:
Web - Compliance & node report
Target version:
Severity:
Critical - prevents main use of Rudder | no workaround | data loss | security
UX impact:
User visibility:
Getting started - demo | first install | Technique editor and level 1 Techniques
Effort required:
Priority:
82
Name check:
To do
Fix check:
To do
Regression:

Description

This issue won't be fixed in Rudder 6.x, as it would break existing installation of Rudder
However, it is fixed in Rudder 7.0, meaning that upgrades from 6.X to 7.0 may break values by escaping string escaped by hand

Using multiple "\" in a class parameter leads to inconsistent behaviour

Technique editor

Number of \ Value File edited State of edition Report State of report
1 /tmp/escapedfilebyfileonlyone\s /tmp/escapedfilebyfileonlyone\s OK /tmp/escapedfilebyfileonlyone\s OK
2 /tmp/escapedfilebyfiletwice\\s /tmp/escapedfilebyfiletwice\s NOK /tmp/escapedfilebyfiletwice\s NOK
3 /tmp/escapedfilebyfile\\\s /tmp/escapedfilebyfile\\s NOK /tmp/escapedfilebyfile\\s NOK

and you get the idea for the rest
There is no escaping here, causing the \\ to be replaced by \
Content in file, or commands is also unescaped, so \\ are replaced by \

Rudder built-in techniques

Used with Technique Check generic file content

Number of \ Value File edited State of edition Report State of report
1 /tmp/file\sonce /tmp/file\sonce OK /tmp/file\sonce OK
2 /tmp/file\\stwice /tmp/file\\stwice OK /tmp/file\\stwice OK
3 /tmp/file\\\sthrice 'file\\\sthrice' OK /tmp/escapedfilebyfile\\\s OK

There is escaping here, with \\ being replaced by \\\\, and so it ends up correct both in the file name and in the report


Subtasks 2 (0 open2 closed)

Bug #19899: Breaking change: correct escaping for technique editor in 7.0ReleasedFrançois ARMANDActions
Bug #19919: Document that escaping is incoherent with the rest in Technique Editor in 6.X, that it is corrected in 7.0, and that's a breaking changeReleasedAlexis MoussetActions
Actions #2

Updated by Nicolas CHARLES almost 3 years ago

database entry looks surprising, as the \ are doubled

"Command execution","values":["sed -i 's#next if .*issuer_cn.*Let.*Encrypt.*#next if ($info->{\\x27issuer_o\\x27} !~ /Let\\x27s\\s\\\\\\s+Encrypt/i);#g' /tmp/test"],"unexpanded":["sed -i 's#next if .*issuer_cn.*Let.*Encrypt.*#next if ($info->{\\x27issuer_o\\x27} !~ /Let\\x27s\\s\\\\\\s+Encrypt/i);#g' /tmp/test"]}]}]},

report is not ok, as a \ is missing

 282792118 | 2021-04-21 10:30:42.413451+02 | root   | b94d4f41-10ea-4467-ada9-1acb62f73719 | be2f3386-0404-4a83-aff6-9ce76a92b070 |      0 | Command execution                       | sed -i 's#next if .*issuer_cn.*Let.*Encrypt.*#next if ($info->{\x27issuer_o\x27} !~ /Let\x27s\s\\s+Encrypt/i);#g' /tmp/test | 2021-04-21 10:30:38+02 | result_repaired | Command execution                       | Execute the command sed -i 's#next if .*issuer_cn.*Let.*Encrypt.*#next if ($info->{\x27issuer_o\x27} !~ /Let\x27s\s\\s+Encrypt/i);#g' /tmp/test was repaired

output from agent has the missing \

rudder     info: Executing 'no timeout' ... 'sed -i 's#Let.*Encrypt.*#Lets\x27s\s\\sEncrypt/i);#g

but generated code is

command_execution("sed -i 's#Let.*Encrypt.*#Lets\\x27s\\s\\\sEncrypt/i);#g' /tmp/test")

Actions #3

Updated by Nicolas CHARLES almost 3 years ago

  • Description updated (diff)
Actions #4

Updated by Nicolas CHARLES almost 3 years ago

  • Description updated (diff)
Actions #5

Updated by Nicolas CHARLES almost 3 years ago

  • Description updated (diff)
Actions #6

Updated by François ARMAND almost 3 years ago

  • Description updated (diff)
Actions #7

Updated by François ARMAND almost 3 years ago

  • Description updated (diff)
Actions #8

Updated by Nicolas CHARLES almost 3 years ago

  • Subject changed from Unexpected report when using complex sed command to Unexpected report when using command execution with \\
  • Description updated (diff)
Actions #9

Updated by François ARMAND almost 3 years ago

  • Severity set to Minor - inconvenience | misleading | easy workaround
  • User visibility set to Operational - other Techniques | Rudder settings | Plugins
  • Priority changed from 0 to 32

So, the problem is that bash (or shell) replace \\ into \ and so the value reported by cfengine is the one with only one \. There is no simple way to change that appart from having a special case in expected report when we deal with command execution.

This does not breaks anything but impact compliance, so I put it in "misleading", but perhaps it more grave than what I understand now.

Actions #10

Updated by François ARMAND almost 3 years ago

  • Status changed from New to In progress
  • Assignee set to François ARMAND
Actions #12

Updated by François ARMAND almost 3 years ago

  • Status changed from In progress to New
  • Assignee deleted (François ARMAND)
Actions #13

Updated by Nicolas CHARLES almost 3 years ago

  • Subject changed from Unexpected report when using command execution with \\ to Incoherent escaping between Technique Editor and Rudder Technique, and unexpected reports or behaviour
  • Description updated (diff)
Actions #14

Updated by Nicolas CHARLES almost 3 years ago

  • Description updated (diff)
Actions #15

Updated by Nicolas CHARLES almost 3 years ago

A workaround for the reporting part in the technique editor can be to:
  1. change the value of class parameter in _method_reporting_context call before the generic method to escape it
  2. edit the 20/log_rudder.cf log rudder_mode method to replace the use of class_parameter with ${report_data.component_key}

This will fix the reporting, but the content will still be incorrect - people will need to properly escape their content

Actions #16

Updated by François ARMAND almost 3 years ago

  • Severity changed from Minor - inconvenience | misleading | easy workaround to Major - prevents use of part of Rudder | no simple workaround
  • User visibility changed from Operational - other Techniques | Rudder settings | Plugins to Getting started - demo | first install | Technique editor and level 1 Techniques
  • Priority changed from 32 to 70
Actions #17

Updated by François ARMAND almost 3 years ago

  • Severity changed from Major - prevents use of part of Rudder | no simple workaround to Critical - prevents main use of Rudder | no workaround | data loss | security
  • Priority changed from 70 to 94

Setting it to critical, as it may lead to major outage (and executing commands is a main use case for rudder)

Actions #18

Updated by Vincent MEMBRÉ almost 3 years ago

  • Target version changed from 6.1.13 to 6.1.14
Actions #19

Updated by Vincent MEMBRÉ almost 3 years ago

  • Target version changed from 6.1.14 to 6.1.15
  • Priority changed from 94 to 91
Actions #20

Updated by Vincent MEMBRÉ over 2 years ago

  • Target version changed from 6.1.15 to 6.1.16
Actions #21

Updated by Félix DALLIDET over 2 years ago

  • Priority changed from 91 to 89

I had a similar issue recently, in a 6.2.9, if I create a technique via the technique editor using the File lines present method and want to ensure that the line \\"x is present, the technique editor does accept the input, but the generation will break with the following error:

⇨ Policy update error for process '8' at 2021-08-27 09:50:16 
⇨  Cannot write nodes configuration 
⇨  Unexpected: Error when executing hooks:
 Exit code=1 for hook: '/opt/rudder/etc/hooks.d/policy-generation-node-ready/10-cf-promise-check'. [stdout:   error: There are syntax errors in policy files
][stderr:/var/rudder/cfengine-community/inputs.new/testing/1.0/technique.cf:13:109: error: syntax error
    "File lines present_${report_data.directive_id}_0" usebundle => file_lines_present("/tmp/my_file", "\\"x"),                                                                                       ...] (for node(s) root)

The json escape it like such:

{
  "name":"lines",
  "value":"\\\\\"x" 
}

And in the cf file: file_lines_present("/tmp/my_file", "\\"x")

While in 7.0, it works perfectly. The same technique done via the technique editor does not break at generation time, and render the proper string in the target file.
Json generated by the technique editor was:

{
  "name":"lines",
  "value":"\\\\\"x" 
}

And the cf file:
"61986e26-f5d9-429b-8ffa-3cffb9befc72" usebundle => file_lines_present("/tmp/my_file", "\\\\\"x");

While the 7.0 took the exact same string as written in the json, the 6.2 seems to interpret the json while reading it somehow.

Actions #22

Updated by Nicolas CHARLES over 2 years ago

if technique is compiled with rudderc, then \\"x works as expected
but if it falls back to previous method, policy generation fails in 7.0

Actions #23

Updated by Vincent MEMBRÉ over 2 years ago

  • Target version changed from 6.1.16 to 6.1.17
Actions #24

Updated by Nicolas CHARLES over 2 years ago

  • Subject changed from Incoherent escaping between Technique Editor and Rudder Technique, and unexpected reports or behaviour to Won't fix: Incoherent escaping between Technique Editor and Rudder Technique, and unexpected reports or behaviour in 6.X
  • Description updated (diff)
  • Priority changed from 89 to 88
Actions #25

Updated by Vincent MEMBRÉ over 2 years ago

  • Target version changed from 6.1.17 to 6.1.18
  • Priority changed from 88 to 85
Actions #26

Updated by Vincent MEMBRÉ over 2 years ago

  • Target version changed from 6.1.18 to 6.1.19
  • Priority changed from 85 to 84
Actions #27

Updated by François ARMAND about 2 years ago

  • Status changed from New to Resolved
  • Priority changed from 84 to 82

It's corrected in 7.0, and as explained won't be corrected in 6.x.

Actions

Also available in: Atom PDF