Bug 855121

In fact, this seems to be an non-issue for ricci side as lon as
only &, <, >, ' and " are considered.

Then, only client-side work would be needed.

FWIW, a sort of unit test for the discussed XML entities un-/escaping
in ricci si now in upstream [2].  It was used to check that those
5 characters from [comment 1] are treated correctly.

[2] http://git.fedorahosted.org/cgit/conga.git/commit/?id=35c7af4

In fact, there is an issue with proactive skipping of already
present XML entities when performing the escaping as a part of
cooking the externalized XML.  Taking the circumstances into
account, we need to make sure there is not such extra treatment
and initial '&' of these entities is escaped as usual.

I proposed a (roughly tested) patch in (temporary) branch upstream:
http://git.fedorahosted.org/cgit/conga.git/commit/?h=bz855121

The (sort of) unit test is committed separately into the master:
http://git.fedorahosted.org/cgit/conga.git/commit/?id=35c7af4

Using this test (see also the commit message for the fix):

BEFORE:

ricci/test/xml_escape$ make check
./test-xmlescape | sed "s|\(.*\)|\1\n|g" | tee -a /dev/stderr | \
    ./test-xmlescape -i | sed "s|\(.*\)|\1\n|g"
foo&bar
foo&bar
foo&bar

Original information got changed within escape-unescape cycle.

AFTER:

$ make check
./test-xmlescape | sed "s|\(.*\)|\1\n|g" | tee -a /dev/stderr | \
    ./test-xmlescape -i | sed "s|\(.*\)|\1\n|g"
foo&bar
foo&bar
foo&bar

Information preserved.

Re [comment 3]:
This change may not be so important from common user-defined values
standpoint, but it becomes actual when the ricci password is generated
by some kind of PRNG and the resulting string, possibly containg
sequences like "<", is just copied around whenever needed, without
any attention to its content.
This is the situation the patch helps to prevent.

In fact, the things are not got right on multiple levels,
"keen escaping" discussed in [comment 3] being one them.

I did a little experiment taking ricci codebase and playing with
common/XML.cpp and copying resulting ricci, ricci-worker, and modcluster
binaries in place of the ones from ricci-0.16.2-55.el6.x86_64.
Whenever un-/escaped string differed from its original, this was logged
into syslog for debugging purposes (see bellow).

Following conditional macros were used to enable/disable particular
parts of code:

INVERT: whether to use unescaping of the entities (i.e., use invert_chars
function); studying the behavior of xmllib2 tells that this is completely
unnecessary as this is done automatically (at least as far as attribute
values are concerned)

KEENESCAPE: whether to use original code or mimic the fix from [comment 3]

For this purpose, in relation to [comment 3], I put following XML snippet
into cluster.conf on the examined node:
> <fencedevices>
>   <fencedevice agent="fence_apc" ipaddr="localhost" login="root"
>                name="apcpowerswitch" passwd="foo&amp;amp;bar"/>
> </fencedevices>
then from the machine outside the cluster, I ran:
> ccs-overly -d -h mynode1 --getconf
which I used to verify for correctness of password form corresponding
to the snippet above (marked as CCS-RESULT below).

---

RICCI-COMPILE: INVERT=1 KEENESCAPE=1
ricci: ricci-xml: foo&amp;bar -> foo&bar
ricci[660]: Executing '/usr/libexec/ricci/ricci-worker
            -f /var/lib/ricci/queue/1168596506'
modcluster: ricci-xml: foo&amp;bar -> foo&bar
modcluster: ricci-xml: foo&bar => foo&amp;bar
ricci-worker: ricci-xml: foo&bar => foo&amp;bar
ricci: ricci-xml: foo&bar => foo&amp;bar

CCS-RESULT: foo&amp;bar

---

RICCI-COMPILE: INVERT=1 KEENESCAPE=0
ricci: ricci-xml: foo&amp;bar -> foo&bar
ricci[1926]: Executing '/usr/libexec/ricci/ricci-worker
             -f /var/lib/ricci/queue/1778838431'
modcluster: ricci-xml: foo&amp;bar -> foo&bar
modcluster: ricci-xml: foo&bar => foo&amp;bar
ricci-worker: ricci-xml: foo&bar => foo&amp;bar
ricci: ricci-xml: foo&bar => foo&amp;bar

CCS-RESULT: foo&amp;bar

---

RICCI-COMPILE: INVERT=0 KEENESCAPE=1
ricci[3163]: Executing '/usr/libexec/ricci/ricci-worker
             -f /var/lib/ricci/queue/521582756'
ricci-worker: ricci-xml: foo&bar => foo&amp;bar
ricci: ricci-xml: foo&bar => foo&amp;bar

CCS-RESULT: foo&amp;bar

---

RICCI-COMPILE: INVERT=0 KEENESCAPE=0
ricci[4309]: Executing '/usr/libexec/ricci/ricci-worker
             -f /var/lib/ricci/queue/1490636628'
modcluster: ricci-xml: foo&amp;bar => foo&amp;amp;bar
ricci-worker: ricci-xml: foo&amp;bar => foo&amp;amp;bar
ricci: ricci-xml: foo&amp;bar => foo&amp;amp;bar

CCS-RESULT: foo&amp;amp;bar

---

Based on this, the conclusion is:

1/ patch proposed in [comment 3] is half-way to a desired behavior

2/ there is no need for and it is even harmful to unescape entities
   (call invert_chars) as the attribute values provided by xmllib2
   are already unescaped ("raw" as opposed to "mangled for XML well-
   -formedness")

3/ modcluster package (clustermon BZ component) is also affected
   -> [bug 858386]

Action items: 2/ and 3/ (will solve these shortly)

Based on [comment 6], I committed another changeset into referred branch
bz855121.

In addition, the version of top-level protocol was bumped to 1.0 -> 1.1.
Provided that it is likely more doable to update client components then
server one in usual deployments (n.b. very often different machines),
this allows for backward-compatible workaround of buggy versions of
ricci.  These are to be implemented as part of already opened bugs:

- ccs:      [bug 855117]
- ccs_sync: [bug 855120]
- luci:     [bug 855112]

Bumping to 6.5 since we're still waiting on an acked patch.

*** Bug 949566 has been marked as a duplicate of this bug. ***

*** This bug has been marked as a duplicate of bug 855112 ***