Account for Tumbleweed sshd config changes #2501

[phillxnet]

Newer systems now establish default sshd config in /usr/etc/ssh/sshd_config; and include overrides via drop-in files in /etc/ssh/sshd_config.d/*.conf.
Accommodate these changes: initially instantiated in Tumbleweed.

Fixes #2501

Includes

• Establish dict of named tuple to abstract distro aware sshd config files and their paths.
• Account for distro 1.7.0 onwards re sshd config.
• Add PermitRootLogin mechanism by way of flag file. We use AllowUsers from initrock onwards. Enable the exclusion of the 'root' user via a flag file deletion mechanism.
• Establish system constants.py file. Helps to avoid circular dependencies and normalises our system variables.
• Re-factor sshd code to be centralised in ssh.py. Removes sshd knowledge requirements from services.py and initrock.py.
• Improve readability via minor renaming refactoring.
• Fix bug blocking creation of non existent sshd config files. Required as for Tumbleweed, on first build, we establish drop-in files rather than just edit the default os sshd files.
• Abstract Subsystem sftp /path/sftp-server disablement.
• Add TODO re abstraction of AllowRootLogin configuration.
• Transition existing test_sftp.py to properly use fixtures.
• Provide standardised instructions to create new fixtures, and run tests (test_sftp.py).
• Black formatted.
• Add additional mocks to avoid fs/sshd interaction. The prior sftp API tests had insufficient mocking casing unintended sshd config interactions. Fix by adding missing mocks. Facilitates the re-enablement of previously remarked out API test entries.

[phillxnet]

@FroggyFlox & @Hooverdan96

If either of you fancy casting a casual eye across this pr I think it would benefit. Fairly well tested functionally and we have a return of more API tests previously remarked out but we also have a moderate amount of refactoring - but mainly to address better form (the constants.py abstraction that I think we should repeat elsewhere) and readability and to remove potential growing circular dependency (ssh.py knows of sshd so use where such knowledge is required).

Caveat on "Fairly well tested functionally" the new Flag mechanism does not include adding any new files à la the TODO referenced abstraction of PermitRootLogin.conf. But we can address that later and the consequence in the interim is likely no root login by default as per upstream. But at least we don't then add "root" to AllowUsers entries when instructed not to do so: via flag file deletion. But we also don't retro fit this configuration: I had intended only to introduce the beginnings of this mechanism to maintain focus on our sshd config file distro awareness as per referenced issue.

Before squash (no known code changes during that process) rpm build was successful across the Leaps and on Tumbleweed on x86_64 and aarch64.

[phillxnet]

Caveats

On a side channel discussion, concerning another pending change with @FroggyFlox we may have a permissions issue a-foot here.

For existing files such as the Tumbleweed OS default we maintain the default permissions when we edit the sftp-server out i.e.:

rtumbleweed:~ # ls -la /usr/etc/ssh/sshd_config
-rw------- 1 root root 3744 May 23 17:05 /usr/etc/ssh/sshd_config

But for our drop-in sshd config files that we create we have the following:

rtumbleweed:~ # ls -la /etc/ssh/sshd_config.d/rockstor-sftp.conf 
-rw-r--r-- 1 root root 108 May 23 17:13 /etc/ssh/sshd_config.d/rockstor-sftp.conf

It may be we have to be more purposeful here?
Parent directory allows all access (via execution bit for other), i.e. others can change into this directory.

rtumbleweed:~ # ls -la /etc/ssh/ | grep sshd_config.d
drwxr-xr-x 1 root root   76 May 23 17:16 sshd_config.d

[Hooverdan96]

for any other of these types of service directories (*.d) for config files is there a best known practice to "default" to?

Though I would think that considering that the "off-loaded" parameters that we are using in separate files would usually (or historically be under the sshd_config file, it makes the most sense to also assign them the 0600 and same root ownership ...
though it's not explicitly mentioned here I would assume it's in the same "spirit" for any extensions of the base config file:

http://static.open-scap.org/ssg-guides/ssg-sle15-guide-cis.html#xccdf_org.ssgproject.content_rule_file_permissions_sshd_config

Unfortunately, this hardening guide got me to read way too many rules that one could implement :) thanks for the inspiration during the code review comments of the poetry path adjustments ...

[phillxnet]

@Hooverdan96 Re:

Though I would think that considering that the "off-loaded" parameters that we are using in separate files would usually (or historically be under the sshd_config file, it makes the most sense to also assign them the 0600 and same root ownership ...

Agreed. I'll look to adding a commit here to enforce this.

[phillxnet]

mkstemp() in turn is a wrapper that calls shared code under:

def _mkstemp_inner(dir, pre, suf, flags):
"""Code common to mkstemp, TemporaryFile, and NamedTemporaryFile."""

which in turn tries to opens by default a 600 file:

        try:
            fd = _os.open(file, flags, 0600)
            _set_cloexec(fd)
            return (fd, _os.path.abspath(file))

So our source of group + other read is from move.

[FroggyFlox]

So our source of group + other read is from move.

Interesting... and very confusing for me :-
I tried the following, for instance, as an attempt at reproducing what we do most of the time (run in Python console using Rockstor's interpreter):

>>> from tempfile import mkstemp
>>> import os
>>> import shutil

>>> fh, npath = mkstemp()
>>> oct(os.stat(npath).st_mode)
'0100600'

>>> with open(npath, "w") as sfo:
>>>     print("Mask after new open(): {}".format(oct(os.stat(npath).st_mode)))
>>>     sfo.write("this is a test line")
>>> print("Mask after closing the file: {}".format(oct(os.stat(npath).st_mode)))
Mask after new open(): 0100600
Mask after closing the file: 0100600

>>> shutil.move(npath, "/opt/testfile.txt")
>>> oct(os.stat("/opt/testfile.txt").st_mode)
'0100600'

It seems to remain at 600 the whole time.

[phillxnet]

@FroggyFlox Re:

It seems to remain at 600 the whole time.

Yes, it seems my assumption earlier was just plain wrong, edited accordingly. The actual creation of the file (with unwanted g and o permissions comes from the following, when the target doesn't exist I think:

with open(sshd_config, "a+") as sfo:

Which was co-oped in from a prior use I think - Just looking now at this.

[phillxnet]

@FroggyFlox & @Hooverdan96 OK, I think that last commit has sorted our outstanding caveat in this pull request.

I had co-oped an older open(file "a+") that was never called upon to create files, as we need to do with our newer Tumbleweed, but our older python open() can't handle custom openers, ergo resourcing the os.openfd(os.open(), "a+") instead to enable a more optimal permission when we do now need this open to create a file. Side effect is that it will enforce the same permissions when editing files: i.e. when run on Leap for example.

If either/both of you could check this that would be great. Looks to leave new & existing files as intended at 600 root.root. All our other ssh operations use mkstemp with consequent move which, as demonstrated nicely by @FroggyFlox in an earlier commit, gives us the desired 600 rights also as shutil.move uses copy2 / copystat and so we end up inheriting our secure temp file permissions used for edits as a by-product.

I'll leave this additional commit as explicit as would be good to have as a reference if we have a similar situation elsewhere.

[FroggyFlox]

Thanks for all of this fixing and re-work, @phillxnet !

I know you've tested extensively in TW so I've only tested on Leap 15.4:

• can turn ON and OFF the SFTP service without problem
• can login as root via ssh
• can create a SFTP export and access it as described in our docs
• can write to this SFTP export
• can delete this SFTP export
• permissions on sshd_config remained at 600

[Hooverdan96]

I haven't had enough time yet to spin up another instance for this and play around with it. But don't wait for me at this time.

[phillxnet]

@FroggyFlox @Hooverdan96 I will pop this in now as @FroggyFlox 's pending pr has to be re-based due to a common line change. And we looks to be doing what we set out to do. My apologies for this issue/pr taking so long, and potentially holding up progress. However I think the new Tumbleweed compatibility it brings is super important so hopefully it was worth the wait.

[phillxnet]

@FroggyFlox Thanks for the review. Much appreciated. We are publishing first to testing channel so I think we are OK on this one. Always a worry when changing sshd stuff but there-we-go.