Bug 1798922

Summary: Systemd: improve parsing of quoted variables of Environment
Product: Red Hat Enterprise Linux 8 Reporter: Pino Toscano <ptoscano>
Component: augeasAssignee: Pino Toscano <ptoscano>
Status: CLOSED ERRATA QA Contact: YongkuiGuo <yoguo>
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: 8.1CC: rjones, yoguo
Target Milestone: rcFlags: pm-rhel: mirror+
Target Release: 8.2   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: augeas-1.12.0-5.el8 Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2020-04-28 16:49:27 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Pino Toscano 2020-02-06 10:27:08 UTC
Description of problem:
The Systemd lens does not parse some of the possible ways to specify environment variables for a service in the Environment= key.

In particular:
Environment=VAR=\"with some spaces\"
Environment=VAR='with some spaces'
are not parsed correctly, even though systemd supports them.

Additional info:
I sent a fix for this upstream:
https://github.com/hercules-team/augeas/pull/659
which was accepted and merged as commit
d438f523de4e7ed012897de53cd9340d6728cf98
which is in augeas > 0.12.0.

Comment 2 YongkuiGuo 2020-02-11 12:49:34 UTC
Have a try to verify with package:
augeas-1.12.0-5.el8.86_64

Steps:

Senario 1:

1. 
#cat /lib/systemd/system/dnf-makecache.service
...
Environment="ABRT_IGNORE_PYTHON=1  "   --- add spaces at the end

2.
# augtool print /files/lib/systemd/system/dnf-makecache.service
/files/lib/systemd/system/dnf-makecache.service/Service/Environment
/files/lib/systemd/system/dnf-makecache.service/Service/Environment/ABRT_IGNORE_PYTHON = "1  "

It can be parsed correctly.


Senario 2:

1.
#cat /lib/systemd/system/dnf-makecache.service
...
Environment="  ABRT_IGNORE_PYTHON=1"   --- add spaces at the beginning

2.
# augtool print /files/lib/systemd/system/dnf-makecache.service  --no output

It can not be parsed.


pino, Is there any problem with the above steps?

Comment 3 Pino Toscano 2020-02-11 13:05:27 UTC
(In reply to YongkuiGuo from comment #2)
> Senario 2:
> 
> 1.
> #cat /lib/systemd/system/dnf-makecache.service
> ...
> Environment="  ABRT_IGNORE_PYTHON=1"   --- add spaces at the beginning
> 
> 2.
> # augtool print /files/lib/systemd/system/dnf-makecache.service  --no output
> 
> It can not be parsed.

Never seen this before -- does it work in systemd? That is:
- does systemd parse a .service file with that Environment?
- if so, is the environment variable applied correctly to the service?

> pino, Is there any problem with the above steps?

Scenario #1 is correct, since the spaces are part of the value of the environment variable.

Comment 4 YongkuiGuo 2020-02-12 01:33:41 UTC
(In reply to Pino Toscano from comment #3)
> 
> Never seen this before -- does it work in systemd? That is:
> - does systemd parse a .service file with that Environment?
> - if so, is the environment variable applied correctly to the service?
> 

Yes, the original dnf-makecache.service can be parsed by systemd.aug.

# augtool print /files/lib/systemd/system/dnf-makecache.service
/files/lib/systemd/system/dnf-makecache.service
/files/lib/systemd/system/dnf-makecache.service/Unit
/files/lib/systemd/system/dnf-makecache.service/Unit/Description
/files/lib/systemd/system/dnf-makecache.service/Unit/Description/value = "dnf makecache"
/files/lib/systemd/system/dnf-makecache.service/Unit/#comment[1] = "On systems managed by either rpm-ostree/ostree, dnf is read-only;"
/files/lib/systemd/system/dnf-makecache.service/Unit/#comment[2] = "while someone might theoretically want the cache updated, in practice"
/files/lib/systemd/system/dnf-makecache.service/Unit/#comment[3] = "anyone who wants that could override this via a file in /etc."
/files/lib/systemd/system/dnf-makecache.service/Unit/ConditionPathExists
/files/lib/systemd/system/dnf-makecache.service/Unit/ConditionPathExists/value = "!/run/ostree-booted"
/files/lib/systemd/system/dnf-makecache.service/Unit/After
/files/lib/systemd/system/dnf-makecache.service/Unit/After/value = "network-online.target"
/files/lib/systemd/system/dnf-makecache.service/Service
/files/lib/systemd/system/dnf-makecache.service/Service/Type
/files/lib/systemd/system/dnf-makecache.service/Service/Type/value = "oneshot"
/files/lib/systemd/system/dnf-makecache.service/Service/Nice
/files/lib/systemd/system/dnf-makecache.service/Service/Nice/value = "19"
/files/lib/systemd/system/dnf-makecache.service/Service/IOSchedulingClass
/files/lib/systemd/system/dnf-makecache.service/Service/IOSchedulingClass/value = "2"
/files/lib/systemd/system/dnf-makecache.service/Service/IOSchedulingPriority
/files/lib/systemd/system/dnf-makecache.service/Service/IOSchedulingPriority/value = "7"
/files/lib/systemd/system/dnf-makecache.service/Service/Environment
/files/lib/systemd/system/dnf-makecache.service/Service/Environment/ABRT_IGNORE_PYTHON = "1"
/files/lib/systemd/system/dnf-makecache.service/Service/ExecStart
/files/lib/systemd/system/dnf-makecache.service/Service/ExecStart/command = "/usr/bin/dnf"
/files/lib/systemd/system/dnf-makecache.service/Service/ExecStart/arguments
/files/lib/systemd/system/dnf-makecache.service/Service/ExecStart/arguments/1 = "makecache"
/files/lib/systemd/system/dnf-makecache.service/Service/ExecStart/arguments/2 = "--timer"

Comment 5 Pino Toscano 2020-02-12 09:40:27 UTC
(In reply to YongkuiGuo from comment #4)
> (In reply to Pino Toscano from comment #3)
> > 
> > Never seen this before -- does it work in systemd? That is:
> > - does systemd parse a .service file with that Environment?
> > - if so, is the environment variable applied correctly to the service?
> > 
> 
> Yes, the original dnf-makecache.service can be parsed by systemd.aug.

Oh no, sorry -- I mean if systemd parses and handles correctly a real .service file with your changes.

Comment 6 YongkuiGuo 2020-02-12 10:08:14 UTC
(In reply to Pino Toscano from comment #5)
>
> I mean if systemd parses and handles correctly a real .service file with your changes.

You mean the changed dnf-makecache.service file should be started successfully by running 'systemctl start dnf-makecache.service'?  it failed to execute the 'systemctl start dnf-makecache.service' command even though there are no changes in dnf-makecache.service file.

Comment 7 Pino Toscano 2020-02-12 12:11:20 UTC
(In reply to YongkuiGuo from comment #6)
> (In reply to Pino Toscano from comment #5)
> >
> > I mean if systemd parses and handles correctly a real .service file with your changes.
> 
> You mean the changed dnf-makecache.service file should be started
> successfully by running 'systemctl start dnf-makecache.service'?  it failed
> to execute the 'systemctl start dnf-makecache.service' command even though
> there are no changes in dnf-makecache.service file.

This, or trying a new test service just for this.

For example, I created /etc/systemd/system/test.service as following:
--- BEGIN ---
[Unit]
Description=Test service
Wants=local-fs.target

[Service]
Type=simple
ExecStart=echo "foo=${FOO}"
Environment="FOO=1"

[Install]
WantedBy=basic.target
--- END ---
Running `systemctl daemon-reload` to make systemd pick it.

Then running it shows it works:

# systemctl status test.service 
● test.service - Test service
   Loaded: loaded (/etc/systemd/system/test.service; disabled; vendor preset: disabled)
   Active: inactive (dead)

[...]
Feb 12 13:04:42 rhel8 systemd[1]: Started Test service.
Feb 12 13:04:42 rhel8 echo[32457]: foo=1

Now I change Environment to Environment="  FOO=1", and reload system (daemon-reload): systemd already complains:

# systemctl status test.service 
● test.service - Test service
   Loaded: loaded (/etc/systemd/system/test.service; disabled; vendor preset: disabled)
   Active: inactive (dead)

[...]
Feb 12 13:09:04 rhel8 systemd[1]: /etc/systemd/system/test.service:8: Invalid environment assignment, ignoring:   FOO=1

So I cannot even start it:

# systemctl start test.service 
# systemctl status test.service 
● test.service - Test service
   Loaded: loaded (/etc/systemd/system/test.service; disabled; vendor preset: disabled)
   Active: inactive (dead)

[...]
Feb 12 13:10:01 rhel8 systemd[1]: /etc/systemd/system/test.service:8: Invalid environment assignment, ignoring:   FOO=1
Feb 12 13:10:01 rhel8 systemd[1]: Started Test service.
Feb 12 13:10:01 rhel8 echo[32609]: foo=
Feb 12 13:10:03 rhel8 systemd[1]: /etc/systemd/system/test.service:8: Invalid environment assignment, ignoring:   FOO=1

----

So from what I see systemd does not accept that syntax, so it is correct for augeas to not handle it either.

Comment 8 YongkuiGuo 2020-02-13 07:47:02 UTC
(In reply to Pino Toscano from comment #7)
> (In reply to YongkuiGuo from comment #6)
> > (In reply to Pino Toscano from comment #5)
> > >
> > > I mean if systemd parses and handles correctly a real .service file with your changes.
> > 
> > You mean the changed dnf-makecache.service file should be started
> > successfully by running 'systemctl start dnf-makecache.service'?  it failed
> > to execute the 'systemctl start dnf-makecache.service' command even though
> > there are no changes in dnf-makecache.service file.
> 
> This, or trying a new test service just for this.
> 
> For example, I created /etc/systemd/system/test.service as following:
> --- BEGIN ---
> [Unit]
> Description=Test service
> Wants=local-fs.target
> 
> [Service]
> Type=simple
> ExecStart=echo "foo=${FOO}"
> Environment="FOO=1"
> 
> [Install]
> WantedBy=basic.target
> --- END ---
> Running `systemctl daemon-reload` to make systemd pick it.
> 
> Then running it shows it works:
> 
> # systemctl status test.service 
> ● test.service - Test service
>    Loaded: loaded (/etc/systemd/system/test.service; disabled; vendor
> preset: disabled)
>    Active: inactive (dead)
> 
> [...]
> Feb 12 13:04:42 rhel8 systemd[1]: Started Test service.
> Feb 12 13:04:42 rhel8 echo[32457]: foo=1
> 
> Now I change Environment to Environment="  FOO=1", and reload system
> (daemon-reload): systemd already complains:
> 
> # systemctl status test.service 
> ● test.service - Test service
>    Loaded: loaded (/etc/systemd/system/test.service; disabled; vendor
> preset: disabled)
>    Active: inactive (dead)
> 
> [...]
> Feb 12 13:09:04 rhel8 systemd[1]: /etc/systemd/system/test.service:8:
> Invalid environment assignment, ignoring:   FOO=1
> 
> So I cannot even start it:
> 
> # systemctl start test.service 
> # systemctl status test.service 
> ● test.service - Test service
>    Loaded: loaded (/etc/systemd/system/test.service; disabled; vendor
> preset: disabled)
>    Active: inactive (dead)
> 
> [...]
> Feb 12 13:10:01 rhel8 systemd[1]: /etc/systemd/system/test.service:8:
> Invalid environment assignment, ignoring:   FOO=1
> Feb 12 13:10:01 rhel8 systemd[1]: Started Test service.
> Feb 12 13:10:01 rhel8 echo[32609]: foo=
> Feb 12 13:10:03 rhel8 systemd[1]: /etc/systemd/system/test.service:8:
> Invalid environment assignment, ignoring:   FOO=1
> 
> ----
> 
> So from what I see systemd does not accept that syntax, so it is correct for
> augeas to not handle it either.

Oh, i understood. Thanks so much.

Comment 9 YongkuiGuo 2020-02-13 09:35:33 UTC
Verified with package:
augeas-1.12.0-5.el8.x86_64


Steps:

Scenario 1:

1. 
#cat /lib/systemd/system/dnf-makecache.service
...
Environment="ABRT_IGNORE_PYTHON=  1  "   --- add spaces before and after the value of key

2.
# augtool print /files/lib/systemd/system/dnf-makecache.service
/files/lib/systemd/system/dnf-makecache.service/Service/Environment
/files/lib/systemd/system/dnf-makecache.service/Service/Environment/ABRT_IGNORE_PYTHON = "  1  "

It can be parsed correctly.


Scenario 2:

1.
#cat /lib/systemd/system/lvm2-lvmpolld.service
...
Environment=SD_ACTIVATION="  1  "   --- add double quotation and spaces before and after the value of key

2.
# augtool print /files/lib/systemd/system/lvm2-lvmpolld.service
/files/lib/systemd/system/lvm2-lvmpolld.service/Service/Environment
/files/lib/systemd/system/lvm2-lvmpolld.service/Service/Environment/SD_ACTIVATION = "\"  1  \""

It can be parsed correctly.


Scenario 3:

1.
#cat /lib/systemd/system/lvm2-lvmpolld.service
...
Environment=SD_ACTIVATION='  1  '   --- add single quotation and spaces before and after the value of key

2.
# augtool print /files/lib/systemd/system/lvm2-lvmpolld.service
/files/lib/systemd/system/lvm2-lvmpolld.service/Service/Environment
/files/lib/systemd/system/lvm2-lvmpolld.service/Service/Environment/SD_ACTIVATION = "'  1  '"

It can be parsed correctly.

Comment 11 errata-xmlrpc 2020-04-28 16:49:27 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/RHBA-2020:1824