ManageIQ / manageiq-appliance

System files for the ManageIQ appliance.
Apache License 2.0
7 stars 50 forks source link

fix quoted strings in properties files #345

Closed kbrock closed 2 years ago

kbrock commented 2 years ago

Issue

We want the ability to have quoted strings in our /etc/defalt/manageiq.properties files. Unfortunately

# manageiq.properties
A='a'

# produces
echo $A
'a'

# desired
echo $A
a

This was introduced in #329

Solution

eval properly sets the property value, and export exposes the variable outside the shell to other processes

test run

[root@localhost ~]# cat /etc/default/manageiq-test.properties
A=a
B="b b"
C='c' # this is c

[root@localhost ~]# . /etc/default/evm        # <== before
-bash: export: `b"': not a valid identifier
-bash: export: `#': not a valid identifier

[root@localhost ~]# . /etc/default/evm        # <== after
[root@localhost ~]# echo "'$A' '$B' '$C'"
'a' 'b b' 'c'

[root@localhost ~]# time . /etc/default/evm

real    0m0.002s
user    0m0.002s
sys 0m0.000s

Other changes

As seen for example c above, trailing comments on a line of a file are now ignored.

Fixes #335

Fryguy commented 2 years ago

I really want to avoid an eval, because we will get dinged in security scans for sure. Curious if there is a better way.

miq-bot commented 2 years ago

Checked commit https://github.com/kbrock/manageiq-appliance/commit/50a0cbeb41d007c68f71251ead79ba7fad2dc373 with ruby 2.6.7, rubocop 1.19.1, haml-lint 0.35.0, and yamllint 0 files checked, 0 offenses detected Everything looks fine. :+1:

Fryguy commented 2 years ago

Nice! Not necessarily for this PR, but I'm wondering if we can maybe add a proper spec for this? Perhaps with bats? https://github.com/bats-core/bats-core

kbrock commented 2 years ago

update:

It runs on the following test file:

#good
SPACE="sp ace"
ESPACE = value
NQUOTE=no quote
QUOTES='quotes'
DQUOTES="dquotes"
COMMENT="value"     # comment
COMMENT2=value      # comment
COMMENT3="value" # comment
COMMENT4=value # comment
COMMENT5=value_with_#_in_it
PLAIN=plain
EQUAL=NAME=VALUE
# dangerous
DOLLAR="$dollar"
DOLLAR2="${dollar}"
SHELL="$(ls)"
TICKS=`ls`
# fails
COMMENT6="quoted # value"
COMMENT7="slash \# value"
# bad
$x=y
$(ls)
Fryguy commented 2 years ago

not sure how to test since the path of the input file is hardcoded in the script.

hmmm ok.

Fryguy commented 2 years ago

Does this need to go back to najdorf?

kbrock commented 2 years ago

Does this need to go back to najdorf?

Yes. It protects us from many more bad configuration file formats

Fryguy commented 2 years ago

Backported to najdorf in commit b604e80b91832cac3049fd0c8c52b2e16e315f23.

commit b604e80b91832cac3049fd0c8c52b2e16e315f23
Author: Jason Frey <fryguy9@gmail.com>
Date:   Fri Feb 25 17:37:23 2022 -0500

    Merge pull request #345 from kbrock/env_vars_3

    fix quoted strings in properties files

    (cherry picked from commit 05cc4498b5d54a891114bd1af121d816c94a1d61)