Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for AIX (closes #241) #311

Merged
merged 5 commits into from
Feb 13, 2022
Merged

Add support for AIX (closes #241) #311

merged 5 commits into from
Feb 13, 2022

Conversation

HorlogeSkynet
Copy link
Member

No description provided.

@HorlogeSkynet
Copy link
Member Author

Up @nir0s @jokurz 🙇

@HorlogeSkynet HorlogeSkynet added this to the 1.6.1 milestone Oct 9, 2021
@HorlogeSkynet
Copy link
Member Author

Up @nir0s @jokurz 🙏

nir0s
nir0s previously approved these changes Oct 21, 2021
Copy link
Collaborator

@nir0s nir0s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@HorlogeSkynet
Copy link
Member Author

Can we have a quick thought from you on this @sethmlarson if you don't mind ? 🙇

Copy link
Contributor

@hartwork hartwork left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to raise concerns that AIX is (Unix but) not Linux and that hence putting AIX bits into class LinuxDistribution seems like mis-leading modelling. Also I see a mix of host and target system here, e.g. host sys.platform is inspected no matter if .root_dir is /or not, same for the call to oslevel. Just my two cents

@HorlogeSkynet
Copy link
Member Author

HorlogeSkynet commented Nov 1, 2021

I'd like to raise concerns that AIX is (Unix but) not Linux and that hence putting AIX bits into class LinuxDistribution seems like mis-leading modelling.

You're right, this seems related to the same questions raised in #177 about this module architecture. At the moment, despite renaming LinuxDistribution to UnixDistribution (in a backward-compatible way until v2.0.0 of course), I don't have any other idea about how we could add AIX support here (LinuxDistribution() being a distro singleton and so on...).

Also I see a mix of host and target system here, e.g. host sys.platform is inspected no matter if .root_dir is /or not, same for the call to oslevel. Just my two cents

You were absolutely right, please take a look to the updated (and rebased) patch, it aims at improving host and target system information separation (surprisingly it simplified testing and I think this would improve detection rate OOTB).

Bye, thanks for your time Sebastian 🙇

Copy link
Contributor

@hartwork hartwork left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HorlogeSkynet I see some good ideas here, but I'm unsure if this approach will work without use of a chroot-like approach, which I don't see here: adjusting PATH alone will not keep the commands we are calling from reading files outside/upwards .root_dir. Am I missing something?

distro.py Outdated Show resolved Hide resolved
@HorlogeSkynet
Copy link
Member Author

HorlogeSkynet commented Nov 2, 2021

I see some good ideas here, but I'm unsure if this approach will work without use of a chroot-like approach, which I don't see here: adjusting PATH alone will not keep the commands we are calling from reading files outside/upwards .root_dir. Am I missing something?

No you don't, I actually did think about that before adapting this patch, but then forgot 🤔
I'll replace this mess by a proper os.chroot soon. It will also simplify all os.path.join operations performed around root_dir attribute. Furthermore, replacing ":" literal by os.pathsep won't be required anymore.
I'll keep you posted, bye 👋


Note : how could we then support --root-dir option with os.chroot availability limited to UNIX platforms ?

EDIT : I think I got an idea to keep this "BC" for Windows, stay put 😌

@HorlogeSkynet
Copy link
Member Author

So I'm back Sebastian, latest thoughts on this topic :

  • os.chroot needs at least CAP_SYS_CHROOT, and I don't think we can want distro to require any privilege in the future
  • Actually, even chroot-ing wouldn't solve our issue here. The --root-dir option cannot be extended to third binaries execution :
    • What if targeted rootfs architecture is different from the host one ?
    • What if the binary output is based on running kernel information (see UNAME(2) for instance, I don't know how oslevel works) ?

As AIX detection would be based on information gathered from uname, --root-dir usage from a Linux host (for instance) targeting an AIX rootfs would be incompatible.

Where do you think we should go from here ?
I see two options from my PoV :

  1. Reverting my previous changes and accounting for --root-dir limitations, as long as preventing any sub-process execution when this options is used (a.k.a. not trusting include_uname nor include_lsb constructor values)
  2. Opting for a proper os.chroot implementation and warning users if they are not on UNIX or running with insufficient privileges.

Bye 👋

HorlogeSkynet pushed a commit that referenced this pull request Dec 14, 2021
The global idea here is about preferring no data instead of misleading ones if `-r` is set (for instance, by calling a binary returning host information).
This patch additionally introduces a new `include_third_bins` parameter, controlling third-party binaries execution (as AIX' oslevel command).

See #311 for complete thread.
@HorlogeSkynet
Copy link
Member Author

HorlogeSkynet commented Dec 14, 2021

Dear Sebastian @hartwork, dear maintainers,

You will find here an updated branch, with 4 (rebased) commits :

  1. The first one "naively" brings support for AIX (as it was originally proposed here) ;
  2. The second one is actually "breaking", as it removes support for binaries execution when -r/--root-dir (see Add support for detecting a distro from a rootfs even on another OS #161 and Detect distro from arbitrary rootfs root_dir #161 #247) is specified (rationale below) ;
  3. The third one updates documentation about LinuxDistribution exceptions that can be raised (a catch and ignore subprocess.CalledProcessError when running lsb_relsase #261 omission) ;
  4. The last one adds a proper consideration for root_dir public attribute.

Rationale about 2 : According to the above discussion, concerns have been raised about -r/--root-dir argument incompatibility with target rootfs binaries execution.
Bringing support for AIX emphasized this issue, as calling uname might return totally wrong information (running kernel would be probed, not the one from the targeted rootfs).
Moreover, about oslevel (and maybe in the future any interesting binary), we cannot expect it/them from being even executable (architecture, dynamic libraries, specific target system requirement, ...).
The chroot-based approach mentioned above is a dead-end in our case, as, in addition not to fix all of these issues, it requires system privileges that we cannot expect regular users to have/give to.

A new parameter has been introduced (include_third_bins) to control execution of each one of these "third-party" binaries that we might happen to call in the future. I'm not sure about its name though, so any comment would be appreciated.

To conclude, a very quick GitHub search shows(?) that actually no one uses this parameter from API (at least on this forge).

Bye, thanks for your time ! 👋

@HorlogeSkynet HorlogeSkynet requested a review from a team February 6, 2022 10:36
Copy link
Contributor

@hartwork hartwork left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @HorlogeSkynet , good summary, sorry for not getting to a review and reply earlier:

tests/resources/distros/aix72/bin/uname Show resolved Hide resolved
distro.py Outdated Show resolved Hide resolved
distro.py Outdated Show resolved Hide resolved
distro.py Outdated Show resolved Hide resolved
HorlogeSkynet pushed a commit that referenced this pull request Feb 10, 2022
The global idea here is about preferring no data instead of misleading ones if `-r` is set (for instance, by calling a binary returning host information).
This patch additionally introduces a new `include_oslevel` parameter, controlling the execution of (AIX) oslevel command.

See #311 for complete thread.
HorlogeSkynet pushed a commit that referenced this pull request Feb 13, 2022
The global idea here is about preferring no data instead of misleading
ones if `root_dir` parameter is set (as this may result in calling a
binary returning host information).

If developers used to set `root_dir` and purposely included LSB, uname
or oslevel commands, a warning will now be emitted.

See #311 for complete thread and rationale.
src/distro/distro.py Outdated Show resolved Hide resolved
The global idea here is about preferring no data instead of false ones
if `root_dir` parameter is set (as this may result in calling a
binary returning, for instance, host information).

Developers used to set `root_dir` and purposely including LSB, uname
or oslevel commands will now get a `ValueError` exception during class
initialization.

See #311 for complete thread and rationale.
@HorlogeSkynet HorlogeSkynet merged commit 2a89f76 into master Feb 13, 2022
@HorlogeSkynet HorlogeSkynet deleted the feat/aix_support branch February 13, 2022 17:58
bmwiedemann pushed a commit to bmwiedemann/openSUSE that referenced this pull request Feb 17, 2022
https://build.opensuse.org/request/show/955104
by user sebix + dimstar_suse
- remove shebang from distro.py
- update to version 1.7.0:
 - BACKWARD COMPATIBILITY:
 - Dropped support for EOL Pythons 2.7, 3.4 and 3.5 [[#281](python-distro/distro#281)]
 - Dropped support for LSB and `uname` back-ends when `--root-dir` is specified [[#311](python-distro/distro#311)]
 - Moved `distro.py` to `src/distro/distro.py` [[#315](python-distro/distro#315)]
 - ENHANCEMENTS:
 - Documented that `distro.version()` can return an empty string on rolling releases [[#312](python-distro/distro#312)]
 - Documented support for Python 3.10 [[#316](python-distro/distro#316)]
 - Added official support for Rocky Linux distribution [[#318](https://github.com/python-distro/distr
HorlogeSkynet pushed a commit that referenced this pull request Feb 18, 2022
This patch is a followup of #311.

It appeared that we were not resolving paths when reading from files.
This means that a symbolic link present under `root_dir` could be
blindly followed _outside_ of `root_dir`, possibly leading to host own
materials.
HorlogeSkynet pushed a commit that referenced this pull request Feb 18, 2022
This patch is a followup of #311.

It appeared that we were not resolving paths when reading from files.
This means that a symbolic link present under `root_dir` could be
blindly followed _outside_ of `root_dir`, possibly leading to host own
materials.
HorlogeSkynet pushed a commit that referenced this pull request Feb 18, 2022
This patch is a followup of #311.

It appeared that we were not resolving paths when reading from files.
This means that a symbolic link present under `root_dir` could be
blindly followed _outside_ of `root_dir`, possibly leading to host own
materials.
HorlogeSkynet pushed a commit that referenced this pull request Mar 5, 2022
This patch is a followup of #311.

It appeared that we were not resolving paths when reading from files.
This means that a symbolic link present under `root_dir` could be
blindly followed _outside_ of `root_dir`, possibly leading to host own
materials.
HorlogeSkynet pushed a commit that referenced this pull request Mar 6, 2022
This patch is a followup of #311.

It appeared that we were not resolving paths when reading from files.
This means that a symbolic link present under `root_dir` could be
blindly followed _outside_ of `root_dir`, possibly leading to host own
materials.
HorlogeSkynet pushed a commit that referenced this pull request Mar 6, 2022
This patch is a followup of #311.

It appeared that we were not resolving paths when reading from files.
This means that a symbolic link present under `root_dir` could be
blindly followed _outside_ of `root_dir`, possibly leading to host own
materials.
HorlogeSkynet pushed a commit that referenced this pull request Apr 19, 2022
This patch is a followup of #311.

It appeared that we were not resolving paths when reading from files.
This means that a symbolic link present under `root_dir` could be
blindly followed _outside_ of `root_dir`, possibly leading to host own
materials.
HorlogeSkynet pushed a commit that referenced this pull request Apr 19, 2022
This patch is a followup of #311.

It appeared that we were not resolving paths when reading from files.
This means that a symbolic link present under `root_dir` could be
blindly followed _outside_ of `root_dir`, possibly leading to host own
materials.
HorlogeSkynet pushed a commit that referenced this pull request May 20, 2022
This patch is a followup of #311.

It appeared that we were not resolving paths when reading from files.
This means that a symbolic link present under `root_dir` could be
blindly followed _outside_ of `root_dir`, possibly leading to host own
materials.
HorlogeSkynet pushed a commit that referenced this pull request Jun 2, 2022
This patch is a followup of #311.

It appeared that we were not resolving paths when reading from files.
This means that a symbolic link present under `root_dir` could be
blindly followed _outside_ of `root_dir`, possibly leading to host own
materials.
HorlogeSkynet pushed a commit that referenced this pull request Jun 6, 2022
This patch is a followup of #311.

It appeared that we were not resolving paths when reading from files.
This means that a symbolic link present under `root_dir` could be
blindly followed _outside_ of `root_dir`, possibly leading to host own
materials.
HorlogeSkynet pushed a commit that referenced this pull request Oct 10, 2022
This patch is a followup of #311.

It appeared that we were not resolving paths when reading from files.
This means that a symbolic link present under `root_dir` could be
blindly followed _outside_ of `root_dir`, possibly leading to host own
materials.
HorlogeSkynet pushed a commit that referenced this pull request Apr 20, 2024
This patch is a followup of #311.

It appeared that we were not resolving paths when reading from files.
This means that a symbolic link present under `root_dir` could be
blindly followed _outside_ of `root_dir`, possibly leading to host
files.
HorlogeSkynet pushed a commit that referenced this pull request Apr 20, 2024
This patch is a followup of #311.

It appeared that we were not resolving paths when reading from files.
This means that a symbolic link present under `root_dir` could be
blindly followed _outside_ of `root_dir`, possibly leading to host
files.
HorlogeSkynet added a commit that referenced this pull request Apr 20, 2024
This patch is a followup of #311 (2a89f76).

It appeared that we were not resolving paths when reading from files.
This means that a symbolic link present under `root_dir` could be
blindly followed _outside_ of `root_dir`, possibly leading to host
files.

Note : this patch **only** changes `root_dir` behavior.
HorlogeSkynet added a commit that referenced this pull request Apr 27, 2024
This patch is a followup of #311 (2a89f76).

It appeared that we were not resolving paths when reading from files.
This means that a symbolic link present under `root_dir` could be
blindly followed _outside_ of `root_dir`, possibly leading to host
files.

Note : this patch **only** changes `root_dir` behavior.
HorlogeSkynet added a commit that referenced this pull request Apr 27, 2024
This patch is a followup of #311 (2a89f76).

It appeared that we were not resolving paths when reading from files.
This means that a symbolic link present under `root_dir` could be
blindly followed _outside_ of `root_dir`, possibly leading to host
files.

Note : this patch **only** changes `root_dir` behavior.
HorlogeSkynet added a commit that referenced this pull request Apr 27, 2024
This patch is a followup of #311 (2a89f76).

It appeared that we were not resolving paths when reading from files.
This means that a symbolic link present under `root_dir` could be
blindly followed _outside_ of `root_dir`, possibly leading to host
files.

Note : this patch **only** changes `root_dir` behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for AIX
3 participants