Bug 1369380 - NetworkManager.service ignores commented 'ONBOOT=no # comment' lines in ifcfg
Summary: NetworkManager.service ignores commented 'ONBOOT=no # comment' lines in ifcfg
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Enterprise Linux 7
Classification: Red Hat
Component: NetworkManager
Version: 7.0
Hardware: Unspecified
OS: Unspecified
medium
medium
Target Milestone: rc
: ---
Assignee: Thomas Haller
QA Contact: Desktop QE
URL:
Whiteboard:
Depends On:
Blocks: OpenVswitch_Support
TreeView+ depends on / blocked
 
Reported: 2016-08-23 09:12 UTC by Petr Horáček
Modified: 2017-08-01 09:17 UTC (History)
13 users (show)

Fixed In Version: NetworkManager-1.8.0-0.2.git20170215.1d40c5f4.el7
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-08-01 09:17:07 UTC


Attachments (Terms of Use)


Links
System ID Priority Status Summary Last Updated
Red Hat Product Errata RHSA-2017:2299 normal SHIPPED_LIVE Moderate: NetworkManager and libnl3 security, bug fix and enhancement update 2017-08-01 12:40:28 UTC

Description Petr Horáček 2016-08-23 09:12:27 UTC
Copy of an upstream bug https://bugzilla.gnome.org/show_bug.cgi?id=769862

NetworkManager.service ignores ifcfg lines which contains comments (key=value  # comment) while network.service handles them as well.

DEVICE=ens7
BOOTPROTO=dhcp
ONBOOT=no  # foo

This config keeps the NIC down when network.service in enabled. When NetworkManager.service is enabled, it turns it up on boot.

NetworkManager 1.0.12
Fedora 23

Comment 2 Thomas Haller 2016-08-30 10:25:19 UTC
Note, that NetworkManager only supports a subset of full-blown shell scripts. You can always create a ifcfg-rh file that NM will treat differently from initscripts (e.g. not shell expansion).

But yeah, for certain cases like here, we should aim to better parse shell.

Comment 3 Blueowl 2016-09-07 16:17:32 UTC
As Thomas said there is a difference between initscripts and NM ifcfg reader. The scripts just source the file as a bash script. NM reads that as key/values and makes some escaping and other tweaks as appropriate.

However, commenting variables after their values seems useful, so lets allow it.

See NM upstream repository branch jk/ifcfg-comments-rh1369380.

Comment 4 Thomas Haller 2016-09-07 17:39:57 UTC
+    if (s[0] == '"' || s[0] == '\'') {
+         do {
+              quote_end = strchr (quote_end + 1, s[0]);
+         } while (quote_end && *(quote_end-1) == '\\');

if the value is quoted with '', escaping is not supported. Should be like:
+         } while (quote_end &&
                   (s[0] == '\'' && *(quote_end-1) == '\\'));

Otherwise that looks good to me.

Comment 5 Blueowl 2016-09-13 15:11:22 UTC
(In reply to Thomas Haller from comment #4)
> +    if (s[0] == '"' || s[0] == '\'') {
> +         do {
> +              quote_end = strchr (quote_end + 1, s[0]);
> +         } while (quote_end && *(quote_end-1) == '\\');
> 
> if the value is quoted with '', escaping is not supported. Should be like:
> +         } while (quote_end &&
>                    (s[0] == '\'' && *(quote_end-1) == '\\'));
> 
> Otherwise that looks good to me.

Actually svUnescape() supports escaping both inside "" and ''.

And I want to support something like this:
NAME='that\'s it  # part of value ' # comment
NAME="this is double quote \" # part of value " # comment

i.e. ' inside '' and " inside "" should be escaped with \. And thus checking whether '#' should be regarded as a comment or not, checks for \.
Thus I think the original code is the way to go.

Comment 6 Thomas Haller 2016-09-13 15:39:48 UTC
this is not how shell treats single quotes. Backslash-escaping inside '' is not possible:

$ NAME='that\'s it  # part of value ' # comment
bash: it: command not found
$ NAME='that'\''s it  # part of value ' # comment
$ echo ">$NAME<"
>that's it  # part of value <


similarly, ifcfg-rh parser should aim to behave the same.


If svUnescape treats \ in '' special, that is another bug.

Comment 7 Blueowl 2016-09-15 10:13:10 UTC
I am aware that bash doesn't allow escaping inside single quotes.
However, NM ifcfg parser treats quoted values and escaping a bit differently. Basically, svUnescape() just strips quotes (either '' or "") as the first and last character and then goes through the string and unespaces.
So, for example
ZONE='that\'s it'
accepts |that's it| string the same as 
ZONE='that's it' does.

And the new code for comments tries to preserve current behaviour and producing
|that's it| string for
ZONE='that\'s it' # comment

So I think this is better for the comments.
But if the escaping should be changed, it is definitely another topic and should be done outside of this bug. (I am not sure what compatibility consequences it would have.)

However, if you prefer, I added your suggestion as a separate commit to the branch. Thus the second ' closes the string.

Comment 8 Thomas Haller 2016-09-16 17:08:04 UTC
maybe this should be just fixed altogether.

It's not that hard to walk through the string and support quoting and escaping like shell does.

For example: https://git.gnome.org/browse/network-manager-openvpn/tree/properties/import-export.c?id=bd7130b4e30046c7b12e5c2e518c95014452e789#n410
(note, that nm-openvpn parses the values like openvpn, which is not exactly the same as shell does).


Difficult is, if we see any ${VAR} or $(cmd) because of possibly nested braces. For that, I would treat $ as a normal character. Or maybe just ignore the value entire value as invalid and return NULL.

Comment 9 Thomas Haller 2016-09-16 17:10:06 UTC
ups, last paragraph ... :)
I mean, in face of a $ or `, just error out and return NULL for that value. It's not supported.

Comment 10 Thomas Haller 2016-11-01 10:00:09 UTC
I did a large rework of the shell parsing code:

th/ifcfg-rh-shell-parsing-rh1369380


It can support everything except:

 - no line continuations, like

      FOO1='
      '
      FOO2=a\
      b

  - no shell expansions via ``, $, !

  - no commands, like

      ls -1
      LANG=C ls -1

   - no multiple statements per line, like

      FOO1=a;FOO2=b


It now uses bash-syntax to escape \r and \n.

Comment 11 Thomas Haller 2016-11-03 11:52:41 UTC
note that the branch fixes previous bugs, which unfortunately leads to certain configurations to be interpreted differently after update.


E.g. with previous versions of NM:

  $ nmcli connection modify t-ifcfg connection.stable-id "aa'bb"

result in

  STABLE_ID="aa\'bb"


After update, this is read back with the additional backslash -- like shell would do.


I don't see how that can be avoided. It was a bug, fixing it means to interpret previously written connections differently.

Note that this only matters for a few characters, like ' or ~. So, I'd hope the impact of this is acceptable.

Comment 12 Beniamino Galvani 2016-11-07 09:30:02 UTC
> ifcfg-rh: unescape ifcfg value for CIPHER_GROUP/CIPHER_PAIRWISE

    It's not clear why we would read the CIPHER_GROUP/CIPHER_PAIRWISE
    verbatim=FALSE (without shell unescaping).

You meant 'verbatim=TRUE' ?

Branch th/ifcfg-rh-shell-parsing-rh1369380 LGTM.

Comment 13 Thomas Haller 2016-11-07 10:54:57 UTC
(In reply to Beniamino Galvani from comment #12)

thanks. fixed and re-pushed.

Comment 16 errata-xmlrpc 2017-08-01 09:17:07 UTC
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHSA-2017:2299


Note You need to log in before you can comment on or make changes to this bug.