ssh: use correct ld-linux for chroot on arm64

[mcbridematt]

The ld-linux filename is architecture dependent so we need to use
the correct one for the system.

Fixes #2183

One possible improvement could be to find the dependencies for sftp-server,rsync and bash at runtime by invoking ldd on each binary, the ldd results combined match up with the hardcoded list:

 ldd /usr/bin/rsync
        linux-vdso.so.1 (0x0000ffff9a787000)
        libacl.so.1 => /lib64/libacl.so.1 (0x0000ffff9a691000)
        libz.so.1 => /lib64/libz.so.1 (0x0000ffff9a660000)
        libpopt.so.0 => /usr/lib64/libpopt.so.0 (0x0000ffff9a63f000)
        libslp.so.1 => /usr/lib64/libslp.so.1 (0x0000ffff9a60e000)
        libc.so.6 => /lib64/libc.so.6 (0x0000ffff9a488000)
        /lib/ld-linux-aarch64.so.1 (0x0000ffff9a759000)
        libattr.so.1 => /lib64/libattr.so.1 (0x0000ffff9a467000)
        libcrypto.so.1.1 => /usr/lib64/libcrypto.so.1.1 (0x0000ffff9a1e8000)
        libpthread.so.0 => /lib64/libpthread.so.0 (0x0000ffff9a1b3000)
        libdl.so.2 => /lib64/libdl.so.2 (0x0000ffff9a192000)

“opensuse-leap” list	sftp	rsync	bash
libacl		x
libz	x	x
libpopt		x
libslp		x
libc	x	x	x
libattr		x
libcrypto	x	x
libpthread	x	x
Ld-linux	x	x
libdl	x		x
libreadline			x
libtinfo			x

[phillxnet]

@mcbridematt Thanks for the pull request and providing the context of our prior discussion on your Rockstor fork for AArch64 work. Much appreciated, and I'm super eager to have full AArch64 support.

The future improvement idea you put forward in the main intro text was also proposed by @FroggyFlox who made the most recent changes to this area of the code, in part, in the following pr:

"Update libraries needed for Rsync over SFTP" #2167

We could dynamically fetch those by writing some sort of wrapper around ldd for instance, but that would also add another system call each time we need to refresh our chroot dir setup. @phillxnet , do you think that would be worth the cost? Parsing all these libs and their respective paths might end up cost a little in terms of resources, and thus I'm not sure it would be worth it. Thanks for letting me know if you think otherwise and I'll try to work on that.

But for now I think what we have works and we do have bigger fish to fry. But as for your pull request code changes themselves, continuing our keeping it simple (at least for now) idea of hard-wiring and simply substituting the appropriate lib where appropriate according to the architecture awareness provided by

import platform

looks to be good.

It is also in-keeping with our prior use of this same library in our rpm file name lookup in:

"[openSUSE] replace legacy yum with dnf-yum. Fixes #2092" #2097

However there I used:

machine_arch = platform.machine()

With the expectation that the result would yield "aarch64".

Where as in this pull request you have used:

platform.processor() == "aarch64"

I don't have any real AArch64 machines on which to test this, but from: https://docs.python.org/2/library/platform.html

We have:

platform.machine()
Returns the machine type, e.g. 'i386'. An empty string is returned if the value cannot be determined.
...
platform.processor()
Returns the (real) processor name, e.g. 'amdk6'.
An empty string is returned if the value cannot be determined. Note that many platforms do not
provide this information or simply return the same value as for machine(). NetBSD does this.

So it may be that in your setup these are the same, but my reading was that we needed the platform.machine() ideally as we are after the basic type rather than the specific CPU name.

Also, before we merge, can you indicate if this works for your? As per more recent changes the rsync capability will actually be disabled by default however:

"Configure SFTP server at buildtime and update customization settings. Fixes #2168" #2173

As per the prior changes upon which this pr builds, in the linked pull requests, the resulting code was tested on the intended target distros / platforms. But since we have some python 2 technical debt a Tumbleweed build/test is now rather challenging, and I believe you also have some limitations within the AArch64 platform for your particular hardware of interest so it would at least be good to see some report of this having at least worked for you on say your current Leap 15.1 builds and ideally Leap 15.2 RC. If nothing else we can then confidentially report that this fixes the issue it pertains to on the tested distros. So if in the future we have reports on one of the other distros it should help to narrow down what may have happened.

On the issue note. As per our docs: http://rockstor.com/docs/contribute_section.html in the
http://rockstor.com/docs/contribute.html#developers section we have a requirement that each pull request links back to at least one issue.

This is not being pedantic and I'm happy to create this if need be but we use the associated issue links in our display within the Rockstor Web-UI which reads the rpm changelog and parses it thus:

And as such it also serves to surface to our users proper attribution and if you could create the issue then we have the appropriate link. But I'm happy to create this if need be.

So in short if you could:

1. Double check the platform.processor() vs platform.machine() use.
2. Confirm this pr works in reality and on what distro targets you tested it.
3. Create an issue so we can surface the attribution within the Web-UI.

Thanks again for taking the time to present this pull request and apologies for so many words. Chuffed to have our first AArch64 specialist contribution. Very welcome.

[mcbridematt]

Good point about the platform.machine() and platform.processor() - I've updated the pull to use .machine()

It turns out they are only equivalent on openSuSE due to openSuSE stuffing the uname string with extra info, here it is on Debian:

# uname -a
Linux debian23ed 5.5.0-rc5-00041-gb07f636fca1c #1 SMP PREEMPT Thu Jan 9 01:18:27 UTC 2020 aarch64 GNU/Linux

# python3
Python 3.7.3 (default, Dec 20 2019, 18:57:59)
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import platform
>>> platform.processor()
''
>>> platform.machine()
'aarch64'

On openSuSE:

# uname -a
Linux rockstortest 4.12.14-lp151.28.52-default #1 SMP Wed Jun 10 15:32:08 UTC 2020 (464fb5f) aarch64 aarch64 aarch64 GNU/Linux
# python3
import platform
>>> platform.machine()
'aarch64'
>>> platform.processor()
'aarch64'

Presumably the extra 'aarch64 aarch64' is there to fix apps which look for the processor model - for various reasons Linux doesn't explicitly identify CPU core or processor models on arm64 but one can decode it from the implementor/implementation IDs in cpuinfo.

Re the two pull requests: I'll spin up a clean instance and check what happens.

[mcbridematt]

Confirming the #2168 / #2173 change works on my system - that the SFTP server is "operational" after install (ON in the web-ui) and users can log into the sftp once a share has been exported.

[phillxnet]

@mcbridematt Thanks for the confirmation of function. From our other communications I'll note there that this looks to be known working on Leap 15.1 AArch64.

And thanks for double checking that platform library result and making the change. Interesting and good to know. I'll merge this soon ready for our next rpm release and will update the pull request accordingly.

Thanks again for submitting this. Much appreciated and sorry to have to ask for the issue etc. But I think we are all set now. I've just got a few odds and ends to see to and should then be able to do another merge and final testing session before the next rpm release and git tag.

[phillxnet]

@StephenBrown2 Nice find. I'd completely missed that.
We actually have a new project 'norm' of using black formatting, and I had noticed the 'non black' brackets but as this was @mcbridematt first pull request here, and I had already requested a number of hoops, I was reluctant to mention a stylistic issue re the black formatting.

@mcbridematt If you do fancy running this file through black, which should sort all formatting issues auto-style then that would be great. Or just commit @StephenBrown2 suggestion for now. But don't make any more functional changes if you do push any more to this branch as I'm currently working towards the next release which this pr is part of. No worries otherwise as we can always follow up with another pr to fix formatting issue, or tend to them the next time the file receives attention of course.

Black formatting checker

We need to setup a GitHub action really and inform folks of our more recent black policy, so a doc pr I'm thinking in the developers section. I'm afraid I just haven't gotten around to such things. Or even know how currently on the GitHub actions bit. If anyone wants to point me in the right direction re a 'black' pass/fail or change-suggester GitHub task on all .py files or the like four our pull requests that would be great as then folks will get immediate feedback and will likely then be more inclined to tend to such things immediately via further fixing commits, or applying suggested changes or the like.

Sorry to have let the side done on this black notice/GitHub-task folks, busy times.

[phillxnet]

@mcbridematt That's great. Thanks for applying @StephenBrown2 suggestion. I'll get to final testing on this now.

[mcbridematt]

I'll go one better - took me a couple of minutes to get black up and running.

I authored the original patch in vi on my test instance so the formatting got mixed up. I'm surprised Python didn't complain as other editors (e.g Atom) more clearly show a difference.

[phillxnet]

@mcbridematt Thanks for the black formatting. Much better, and much appreciated. Hopefully we can get that added to the developer docs soon. Python can deal with both but we have had an instances of a dead code line and for a while had lots of mixed tab and spaces (still do in non python files). And given indentation is functionally critical in Python we have moved to black to 'make it so'. Plus it does a byte code pre/post comparison to ensure it makes no functional changes which is nice.

Thanks for going the little extra on this one. We are black formatting all python files that are changed in pull requests as we go. But on larger prs it can be very difficult to review them with potentially massive formatting changes included so we are trying to keep the black formatting to it's own commit in that case.

Getting there.

It would be nice if GitHub highlighted tabs vs spaces actually.
Thanks again @StephenBrown2. Much appreciated.

[phillxnet]

I have now repeated the sftp test as per:
#2167 (comment)
and a Leap15.2 x86_64 build is as-was and we have the identical function on a Leap15.2 aarch64 build:

sftp [email protected]

where sft-test was the user setup to own the sftp-test exported share. And further to this the resulting exported share and it's contents were displayed as expected:

sftp> ls
bin        lib64      sftp-test  usr 
sftp> cd sftp-test/
sftp> ls
testfile  
sftp> exit

Where "testfile" was the result of:

touch /mnt2/sftp-test/testfile

Noting test procedure here to avoid a long tail of having to dig through prior pull request to know how we tested this capability prior to merging.

[phillxnet]

@mcbridematt Thanks again for this. I eventually got around to doing a functional confirmation prior to merging and this, unsurprisingly, is exactly as presented. @StephenBrown2 Thanks for having our back on the tab / spaces thing and contributing to this pull request by way of a review.

@mcbridematt I have edited your original post to add the following line:
Fixes #2183
As that we we get auto closure of the indicated issue.

Merging ready for the next testing release.