config_ini duplicating lines due to tab instead of space (e.g. Subsystem)

[OrangeDog]

After the latest update 3e01ad8 the config_ini state added an extra Subsystem sftp /usr/lib/openssh/sftp-server to all my hosts, resulting in invalid config and sshd failing to start.

I had to manually remove the earlier line (that had the comment # override default of no subsystems) to fix it.

[myii]

@OrangeDog Possibly something in the latest merge: 3e01ad8#diff-9f4ce7590a183f3029c8f1febe1e4205L1? Can you confirm if you're using that or not? And the version of Salt being used.

[OrangeDog]

Sorry, I was looking at the wrong file.
Indeed, this is caused by me switching to 3e01ad8
Salt 2018.3.4,

[myii]

I've compared the map before and after and I don't detect anything that significant that changed there as such:

   "generate_ed25519_keys": false,
   "generate_rsa_keys": false,
   "generate_rsa_size": 4096,
+  "host_key_algos": "ecdsa,ed25519,rsa",
   "known_hosts": {
     "aliases": [
       "cname-to-minion.example.org",

That's an expected change since it was introduced recently in 4b84dea. So we're looking at something in the formula itself.

CC: @alxwr.

[myii]

Not seeing any doubling up of Subsystem in the debug output when running openssh.config_ini. Do you mind sharing all references to Subsystem in your pillar, @OrangeDog?

[OrangeDog]

all references to Subsystem in your pillar

I have none.

sshd_config:
  DenyUsers: localadmin
  PermitRootLogin: 'no'

It added the extra Subsystem to the end of the file, after DenyUsers.

[myii]

Historically, Subsystem used to be provided by default directly in map.jinja. A few months back, it's been moved to osfamilymap.yaml. Either way, it's been part of the final map for a while now. The most recent default was added for Debian-based systems. What OS are your minions, by the way? And do you know what commit you updated from?

List of the most recent commits involving Subsystem:

• 6fbef86 -- introduced for Debian here.
• 9845b1f -- introduced for Solaris here.
• 54dde36 -- switched to osfamilymap.yaml.

https://docs.saltstack.com/en/2018.3/ref/states/all/salt.states.ini_manage.html#salt.states.ini_manage.options_present

options present in file and not specified in sections dict will be untouched, unless strict: True flag is used

• The formula isn't using strict: True, so it shouldn't have added it if already present.

Do you mind showing us the how the duplicate lines appear in the file?

[OrangeDog]

OS is Ubuntu 18.04.2
I updated from 463ad69 to 3e01ad8
https://gist.github.com/OrangeDog/8ad607c0dcd7e5f72076ab9e3a49763a

I'm guessing it's because the existing line uses tabs (that the gist doesn't preserve), while the formula added one with spaces.

[myii]

I'm guessing it's because the existing line uses tabs (that the gist doesn't preserve), while the formula added one with spaces.

Sounds like you've identified the problem there. config_ini was introduced in dcb70e5 via. #124 and has always used a space as the separator. So was this an issue of an existing configuration file with tabs?

[OrangeDog]

That file has always used tabs on that line, because that's what Ubuntu ships for the default config.
I've been using config_ini for months, and it hasn't been a problem until now.

[myii]

Strange, there's no reason why 3e01ad8 should have had any effect on that. If anything, did you upgrade Salt in the meantime (to 2018.3.4)? Perhaps there was a change in the way ini.options_present handles whitespace (regex perhaps?). In any case, it would be worth investigating if 463ad69 still works without causing the issue. Can you do that @OrangeDog?

[myii]

OK, got a moment to run some tests and there are some interesting findings. This goes further than Subsystem but it only started affecting Subsystem after 3e01ad8; 463ad69 doesn't affect Subsytem for some as-yet unidentified reason.

---

Setup

Using the basic pillar supplied above:

sshd_config:
  DenyUsers: localadmin
  PermitRootLogin: 'no'

Running each time directly with:

# salt -Cv 'minion' state.sls_id sshd_config-with-ini openssh.config_ini

The Ubuntu-based minion in question started out with no existing /etc/ssh/sshd_config file.

---

Separate error in first run

Since the file doesn't exist, ended up with an error:

          ID: sshd_config-with-ini
    Function: ini.options_present
        Name: /etc/ssh/sshd_config
      Result: False
     Comment: An exception occurred in this state: Traceback (most recent call last):
                File "/usr/lib/python2.7/dist-packages/salt/state.py", line 1933, in call
                  **cdata['kwargs'])
                File "/usr/lib/python2.7/dist-packages/salt/loader.py", line 1939, in wrapper
                  return f(*args, **kwargs)
                File "/usr/lib/python2.7/dist-packages/salt/states/ini_manage.py", line 66, in options_present
                  cur_ini = __salt__['ini.get_ini'](name, separator)
                File "/usr/lib/python2.7/dist-packages/salt/modules/ini_manage.py", line 242, in get_ini
                  inifile = _Ini.get_ini_file(file_name, separator=separator)
                File "/usr/lib/python2.7/dist-packages/salt/modules/ini_manage.py", line 462, in get_ini_file
                  inifile.refresh()
                File "/usr/lib/python2.7/dist-packages/salt/modules/ini_manage.py", line 425, in refresh
                  "Exception: {1}".format(self.name, exc)
              CommandExecutionError: Unable to open file '/etc/ssh/sshd_config'. Exception: [Errno 2] No such file or directory: u'/etc/ssh/sshd_config'

• This is an unrelated issue.
• While this may not be encountered regularly, safeguards should be put in place to prevent this occurring.
• Resolved simply by creating an empty file on the minion with touch /etc/ssh/sshd_config.

---

Normal behaviour

Running the state populates the file as expected. Subsequent runs identify no change is required, returning with Comment: No anomaly detected.

File contains:

Subsystem sftp /usr/lib/openssh/sftp-server
PermitRootLogin no
DenyUsers localadmin

---

Separating by tabs instead

Simply replaced each space with a tab and then ran the state again.

File contains (before):

Subsystem       sftp /usr/lib/openssh/sftp-server
PermitRootLogin no
DenyUsers       localadmin

Based on the latest commit (3e01ad8)

     Changes:   
              ----------
              DenyUsers:
                  ----------
                  after:
                      localadmin
                  before:
                      None
              PermitRootLogin:
                  ----------
                  after:
                      no
                  before:
                      None
              Subsystem:
                  ----------
                  after:
                      sftp /usr/lib/openssh/sftp-server
                  before:
                      None

File contains (after):

Subsystem       sftp /usr/lib/openssh/sftp-server
PermitRootLogin no
DenyUsers       localadmin
Subsystem sftp /usr/lib/openssh/sftp-server
PermitRootLogin no
DenyUsers localadmin

• Every line with tab separation is "replaced".

Based on the previous commit (463ad69)

     Changes:   
              ----------
              DenyUsers:
                  ----------
                  after:
                      localadmin
                  before:
                      None
              PermitRootLogin:
                  ----------
                  after:
                      no
                  before:
                      None

File contains (after):

Subsystem       sftp /usr/lib/openssh/sftp-server
PermitRootLogin no
DenyUsers       localadmin
PermitRootLogin no
DenyUsers localadmin

• Every line with tab separation is "replaced", except for Subsystem.
• I went back to the commit where openssh.config_ini was introduced and got the same result with that as well (i.e. dcb70e5 via. [openssh/config_ini] initial version of config_ini which uses ini state #124).
• So to be clear: this has been present from the beginning and only started affecting the Subsystem line after the last commit.

---

Testing further with values from pillar.example

Didn't spend long on this but it looks like most, if not all, other values are duplicated as well.

One thing I did come across is that removing values from the pillar don't change the file. Another separate issue: do we need an ini.options_absent state as well?

---

Conclusion

Firstly, this issue is confirmed as a bug. Subsystem is definitely being duplicated after the latest commit. Yet, it appears to be masking a deeper issue where the implementation is doing that for many others lines already, where tab has been used. So the resolution needs to consider the whole problem, not just specific to Subsystem.

• @OrangeDog Would you like to rename this issue to something more appropriate?
• @arthurlogilab @chenmen Pinging you since you both have been involved with this implementation and may have useful suggestions.
• @alxwr Already pinged you above but once again since your input would be useful.

[myii]

As a passing note, it would be great to have tests for this in the future.

[OrangeDog]

I could change the title, but it's still the case it only just started happening to Subsystem.
As that appears to be the only one that uses tabs by default (on Ubuntu at least) it's still significant.

[myii]

I've tested and proposed a global fix in #163, so we don't rely on some "magical" way of getting Subsystem working again. Simply replace the tab(s) with a single space and let ini_options.present work the way it was intended to. I'd appreciate if some of you could test it and comment on the PR with your results.