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.

Sign up for GitHub

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

Jump to bottom

Ignore external accounts in /etc/passwd #958

Merged
merged 1 commit into from
Jan 18, 2023
Merged

Ignore external accounts in /etc/passwd #958

merged 1 commit into from
Jan 18, 2023

Conversation

PeterMocary
Copy link
Member

The /etc/passwd can contain special entries to selectively incorporate entries
from another service source such as NIS or LDAP. These entries don't need to
contain all the fields that are normally present in the /etc/passwd entry and
would cause the upgrade failure in facts phase.

JIRA ref: OAMG-6279
RHBZ: 2031098

@github-actions
Copy link

Thank you for contributing to the Leapp project!

Please note that every PR needs to comply with the Leapp Guidelines and must pass all tests in order to be mergeable.
If you want to request a review or rebuild a package in copr, you can use following commands as a comment:

  • review please to notify leapp developers of review request
  • /packit copr-build to submit a public copr build using packit

To launch regression testing public members of oamg organization can leave the following comment:

  • /rerun to schedule basic regression tests using this pr build and leapp*master* as artifacts
  • /rerun 42 to schedule basic regression tests using this pr build and leapp*PR42* as artifacts
  • /rerun-all to schedule all tests (including sst) using this pr build and leapp*master* as artifacts
  • /rerun-all 42 to schedule all tests (including sst) using this pr build and leapp*PR42* as artifacts

Please open ticket in case you experience technical problem with the CI. (RH internal only)

Note: In case there are problems with tests not being triggered automatically on new PR/commit or pending for a long time, please consider rerunning the CI by commenting leapp-ci build (might require several comments). If the problem persists, contact leapp-infra.

@PeterMocary
Copy link
Member Author

Should we note somewhere what entries have been skipped or don't skip them at all?

@MichalHe MichalHe self-assigned this Aug 29, 2022
@PeterMocary PeterMocary marked this pull request as ready for review August 29, 2022 11:15
@MichalHe
Copy link
Member

@PeterMocary Ad your question regarding that some users have been skipped - please add a debug log message informing about the skipped entries.

@PeterMocary
Copy link
Member Author

/rerun

@github-actions
Copy link

Copr build succeeded: https://copr.fedorainfracloud.org/coprs/build/4785375

@github-actions
Copy link

Testing Farm request for RHEL-8.6-rhui/4785375 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

Testing Farm request for RHEL-7.9-rhui/4785375 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

Testing Farm request for RHEL-8.7.0-Nightly/4785375 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

Testing Farm request for RHEL-8.6.0-Nightly/4785375 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

Testing Farm request for RHEL-7.9-ZStream/4785375 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

Testing Farm request for RHEL-7.9-ZStream/4785375 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@MichalHe
Copy link
Member

MichalHe commented Sep 8, 2022

@PeterMocary Hi, I have started reading about NIS-style changes to the usual format of /etc/passwd and your assumption that only pwd might be empty is not correct:

MichalHe
MichalHe requested changes Sep 8, 2022
View reviewed changes
Copy link
Member

@MichalHe MichalHe left a comment

Choose a reason for hiding this comment

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

See my previous comment. Please, make sure that we also handle (ignore) a standalone +, which is a valid syntax - I am not sure how such entry will be parsed.

@PeterMocary
Copy link
Member Author

PeterMocary commented Sep 12, 2022 •
edited

Hi, I have started reading about NIS-style changes to the usual format of /etc/passwd and your assumption that only pwd might be empty is not correct.

Even though the /etc/passwd can contain entries which have empty fields, the pwd library, used for the extraction of these entries, assigns default values to some of the fields when they are empty in /etc/passwd. Based on behavior of this library I decided to limit the check to only to pw_dir or that the home field of our User model isn't filled with None. I tested the behavior by hand however, which isn't the best way thus I missed some cases where the pw_name can also be empty and the entry is extracted by pwd library. This means that pw_name should be checked too.

This patch should handle all possible entries introduced by the compact mode, therefore, basically ignore anything starting with +

The issue here is about the fact that the entries in User model aren't nullable and therefore their values need to be checked before assignment. This was just pointed out by the specific + or - entries mentioned in this issue, which will be skipped from now on whenever necessary fields don't contain a value or aren't assigned one by the pwd library. Do you think that all of these entries should be skipped?

Known probles:

  • Need to check the pw_name since it can be an empty string in /etc/passwd
  • Another problem that if GID a UID aren't specified pwd assigns 0 to these fields by default, which is a valid value for both fields. Not only that 0 is root UID, setting a value to these fields when it's not defined in /etc/passwd isn't right in my opinion.
    • partially handled by skipping the special +/- entries entirely (these entries can have their fields empty, because "Any fields left blank use the information supplied by the NIS server." so setting the UID or GID to zero might not be valid.
    • when an entry isn't starting with +/-, checking if there is UID 0 already might tell that the field is empty, but this introduces unnecessary logic in my opinion and won't even work for GIDs since there can be multiple users assigned to same group. However adding entries to /etc/password the intended way using useradd or usermod won't allow for empty UID or GID and therefore this case should be users fault.
  • The same problem as in with the User model could happen for the Group model as its fields aren't nullable again and the values assigned to them aren't checked.

@PeterMocary PeterMocary force-pushed the etc-passwd-external-accounts branch 2 times, most recently from 64ad24b to 32d5944 Compare September 21, 2022 14:36
@MichalHe
Copy link
Member

Tested in the following scenarios

/etc/passwd

  • +user
    • preupgrade does not fail
    • These users from /etc/passwd that are special entries for service like NIS, or don't contain all mandatory fields won't be included in UsersFacts: ['+someuser']
  • +@usernetgroup
    • preupgrade does not fail
    • These users from /etc/passwd that are special entries for service like NIS, or don't contain all mandatory fields won't be included in UsersFacts: ['+@usernetgroup']
  • -someuser
    • preupgrade does not fail
    • These users from /etc/passwd that are special entries for service like NIS, or don't contain all mandatory fields won't be included in UsersFacts: ['-someuser']
  • +@someuser::::::/bin/bash
    • preupgrade does not fail
    • corresponding entry in leapp-preupgrade.log: These users from /etc/passwd that are special entries for service like NIS, or don't contain all mandatory fields won't be included in UsersFacts: ['+@someuser']
  • +
    • preupgrade does not fail
    • corresponding entry in leapp-preupgrade.log: These users from /etc/passwd that are special entries for service like NIS, or don't contain all mandatory fields won't be included in UsersFacts: ['+']

/etc/group

  • +nisgroup
    • preupgrade does not fail
    • corresponding entry in leapp-preupgrade.log: These groups from /etc/passwd that are special entries for service like NIS, or don't contain all mandatory fields won't be included in GroupsFacts: ['+nisgroup']
  • -nisgroup
    • preupgrade does not fail
    • corresponding entry in leapp-preupgrade.log: These groups from /etc/passwd that are special entries for service like NIS, or don't contain all mandatory fields won't be included in GroupsFacts: ['-nisgroup']
  • +
    • preupgrade does not fail
    • corresponding entry in leapp-preupgrade.log: These groups from /etc/passwd that are special entries for service like NIS, or don't contain all mandatory fields won't be included in GroupsFacts: ['+']

Testing was successful.

@packit-as-a-service
Copy link

Your git-forge project github.com/oamg/leapp-repository has permissions to build in @oamg/leapp Copr project configured in Packit. However, we migrated to the solution where you can configure the allowed git-forge projects in Copr yourself and will remove the configuration in Packit for the allowed projects soon. Therefore, please, add this git-forge project github.com/oamg/leapp-repository to Packit allowed forge projectsin the Copr project settings.

@MichalHe
Copy link
Member

/rerun

@github-actions
Copy link

Copr build succeeded: https://copr.fedorainfracloud.org/coprs/build/4955044

@github-actions
Copy link

Testing Farm request for RHEL-8.6-rhui/4955044 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

Testing Farm request for RHEL-7.9-rhui/4955044 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

Testing Farm request for RHEL-8.7.0-Nightly/4955044 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

Testing Farm request for RHEL-8.6.0-Nightly/4955044 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

Testing Farm request for RHEL-7.9-ZStream/4955044 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

Testing Farm request for RHEL-7.9-ZStream/4955044 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@pirat89 pirat89 added this to the 8.8/9.2 milestone Nov 1, 2022
fernflower
fernflower reviewed Jan 3, 2023
View reviewed changes
Copy link
Member

@fernflower fernflower left a comment

Choose a reason for hiding this comment

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

LGTM, just one thing - could you add a unit test for _get_system_users, please?

@PeterMocary PeterMocary force-pushed the etc-passwd-external-accounts branch 2 times, most recently from 4aabb43 to c35d519 Compare January 18, 2023 10:40
@PeterMocary
Ignore external accounts in /etc/passwd
770a55f 
The /etc/passwd can contain special entries to selectively incorporate entries
from another service source such as NIS or LDAP. These entries don't need to
contain all the fields that are normally present in the /etc/passwd entry and
would cause the upgrade failure in facts phase.
@PeterMocary
Copy link
Member Author

LGTM, just one thing - could you add a unit test for _get_system_users, please?

Just added the unit test for _get_system_users and also a similar unit test for _get_system_groups.

@fernflower
Copy link
Member

/rerun

fernflower
fernflower approved these changes Jan 18, 2023
View reviewed changes
Copy link
Member

@fernflower fernflower left a comment

Choose a reason for hiding this comment

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

lgtm, will wait for tests and then merge. Great job!

@github-actions
Copy link

Copr build succeeded: https://copr.fedorainfracloud.org/coprs/build/5243806

@github-actions
Copy link

Testing Farm request for RHEL-7.9-rhui/5243806 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

Testing Farm request for RHEL-8.6-rhui/5243806 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

Testing Farm request for RHEL-8.6.0-Nightly/5243806 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@fernflower fernflower merged commit 6ada655 into oamg:master Jan 18, 2023
@github-actions
Copy link

Testing Farm request for RHEL-8.7.0-Nightly/5243806 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

Testing Farm request for RHEL-7.9-ZStream/5243806 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

Testing Farm request for RHEL-7.9-ZStream/5243806 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@pirat89 pirat89 added the changelog-checked The merger/reviewer checked the changelog draft document and updated it when relevant label Jan 20, 2023
pirat89 added a commit to pirat89/leapp-repository that referenced this pull request Feb 21, 2023
@pirat89
# Release 0.18.0
422e3f6 
## Packaging
- Requires cpio (oamg#979)
- Requires python3-gobject-base, NetworkManager-libnm (oamg#969)
- Bump leapp-repository-dependencies to 9 (oamg#969, oamg#979)

## Upgrade handling
### Fixes
- Add leapp RHUI packages to an allowlist to drop confusing reports (oamg#995)
- Check only mounted XFS partitions (oamg#1027)
- Detect the kernel-core RPM instead of kernel to prevent an error during post-upgrade phases (oamg#1024)
- Disable the amazon-id DNF plugin on AWS during the upgrade stage to omit error messages during the upgrade process caused by missing network connection (oamg#990)
- Do not create new *pyc files when running leapp after the DNF upgrade transaction (oamg#1017)
- Enable upgrades on s390x when /boot is part of rootfs (oamg#991)
- Extend the allow list of RHUI clients by azure-sap-apps to omit confusing report (oamg#974)
- Filter out PES events unrelated for the used upgrade path and handle overlapping events (oamg#1008)
- Fix scan of ceph volumes on systems without ceph-osd (oamg#1011)
- Fix scan of ceph volumes when ceph-osd container is not found (oamg#986)
- Fix systemd symlinks that become incorrect during the IPU (oamg#972)
- Fix the check of memory (RAM) limits (oamg#984)
- Fix the upgrade of IBM Z machines configured with ZFCP (oamg#983)
- Ignore external accounts in /etc/passwd (oamg#958)
- Inhibit the upgrade when entries in /etc/fstab cause overshadowing during the upgrade (oamg#1009)
- Prevent leapp failures caused by re-run of leapp in the upgrade initramfs after previous failure, which causes additional confusing error message hiding original bugs (oamg#996)
- Prevent the upgrade with RHSM when a baseos and an appstream target repositories are not discovered (oamg#1001)
- RHUI(Azure) handle correctly various SAP images (oamg#1037)
- Rework the network configuration handling and parse the configuration data properly (oamg#969)
- Set RHSM release for non-ga and non-beta channels (oamg#1033)
- Use the "grub" library to find the GRUB device (oamg#989)
- [IPU 7 -> 8] Detect corrupted grubenv file (oamg#1012)
- [IPU 7 -> 8] Ensure that rsyncd stays enabled if it is enabled prior the upgrade(oamg#1043)
- [IPU 7 -> 8] Ensure that satellite metapackages are installed after the upgrade (oamg#994)
- [IPU 7 -> 8] Ensure the device_cio_free service stays enabled on s390x after the upgrade (oamg#977)
- [IPU 7 -> 8] Fixed checks for RHEL SAP IPU 7.9 -> 8.6 (oamg#978)
- [IPU 7 -> 8] Fixed migration of ntp to chrony when the ntp service is masked (oamg#966)
- [IPU 7 -> 8] Prevent the traceback during migration of sendmail configuration files when the package is not installed (oamg#1041)
- [IPU 7 -> 8] Satellite: reindex all related databases to fix issues due to new locales in RHEL 8 (oamg#1007, oamg#1018)
- [IPU 7 -> 8] Use the correct domain name in SSSD reports (oamg#1040)
- [IPU 8 -> 9] Added checks for RHEL SAP IPU 8.6 -> 9.0 (oamg#978)
- [IPU 8 -> 9] CheckVDO: Ask user for the confirmation only on failures and undetermined devices (oamg#961)
- [IPU 8 -> 9] Fix the kernel detection during initramfs creation for new kernel on RHEL 9.2+ (oamg#1048)
- [IPU 8 -> 9] Fix the upgrade on Azure using RHUI for SAP Apps images (oamg#975)
- [IPU 8 -> 9] Handle correctly firewalld version 0.8 (oamg#963)

### Enhancements
- Set new upgrade paths (oamg#988):
-- RHEL 7.9 -> 8.8, 8.6 (default: 8.8)
-- RHEL 8.6 -> 9.0
-- RHEL 8.8 -> 9.2
- Check that used leapp data are valid and compatible with the installed leapp-repository (oamg#1003)
- Detect a proxy configuration in YUM/DNF and adjust an error msg on issues caused by the configuration (oamg#914)
- Detect and report systemd symlinks that are broken before the upgrade (oamg#972)
- Drop obsoleted upgrade paths (oamg#1047)
- Improve remediation instructions for packages in unknown repositories (oamg#1010)
- Improve the error message to guide users when discovered more space is needed (oamg#956)
- Introduce --nogpgcheck option to skip checking of RPM signatures (oamg#910)
- Introduced an option to use an ISO file as a target RHEL version content source (oamg#979)
- Introduced possibility to specify what systemd services should be enabled/disabled on the upgraded system (oamg#964)
- Map the target repositories also based on the installed content (oamg#967)
- Provide common information about systemd services (oamg#959)
- Register subscribed systems automatically to Red Hat Insights unless --no-insights-register is used (oamg#1000)
- Remove obsoleted GPG keys provided by RH after the upgrade to prevent errors (oamg#1022)
- Run upgrade process with checking RPM signatures by default (oamg#910, oamg#993, oamg#1025)
- Save breadcrumbs results as RHSM facts (oamg#1002)
- Small improvements in various reports (oamg#1038, oamg#1039, oamg#1032)
- [IPU 8 -> 9] Detect CIFS also when upgrading from RHEL8 to RHEL9 (PR1035)
- [IPU 8 -> 9] Detect RoCE on IBM Z machines and check the configuration is safe for the upgrade (oamg#1030)
- [IPU 8 -> 9] Enable upgrades of RHEL 8 for SAP HANA to RHEL 9 on ppc64le (oamg#1042)
- [IPU 8 -> 9] Improve the handling of blocklisted certificates (oamg#992)

## Additional changes interesting for devels
- Started work on bringing up networking inside the upgrade initramfs - currently available for testing and development purposes when LEAPP_DEVEL_INITRAM_NETWORK is set (oamg#960)
- Add leapp debug tools to the upgrade initramfs - dracut upgrade module (oamg#997)
- Enable disabling of DNF plugins in the dnfplugin library (oamg#990)
@pirat89 pirat89 mentioned this pull request Feb 21, 2023
Merged
pirat89 added a commit that referenced this pull request Feb 21, 2023
@pirat89
# Release 0.18.0
21b592a 
## Packaging
- Requires cpio (#979)
- Requires python3-gobject-base, NetworkManager-libnm (#969)
- Bump leapp-repository-dependencies to 9 (#969, #979)

## Upgrade handling
### Fixes
- Add leapp RHUI packages to an allowlist to drop confusing reports (#995)
- Check only mounted XFS partitions (#1027)
- Detect the kernel-core RPM instead of kernel to prevent an error during post-upgrade phases (#1024)
- Disable the amazon-id DNF plugin on AWS during the upgrade stage to omit error messages during the upgrade process caused by missing network connection (#990)
- Do not create new *pyc files when running leapp after the DNF upgrade transaction (#1017)
- Enable upgrades on s390x when /boot is part of rootfs (#991)
- Extend the allow list of RHUI clients by azure-sap-apps to omit confusing report (#974)
- Filter out PES events unrelated for the used upgrade path and handle overlapping events (#1008)
- Fix scan of ceph volumes on systems without ceph-osd (#1011)
- Fix scan of ceph volumes when ceph-osd container is not found (#986)
- Fix systemd symlinks that become incorrect during the IPU (#972)
- Fix the check of memory (RAM) limits (#984)
- Fix the upgrade of IBM Z machines configured with ZFCP (#983)
- Ignore external accounts in /etc/passwd (#958)
- Inhibit the upgrade when entries in /etc/fstab cause overshadowing during the upgrade (#1009)
- Prevent leapp failures caused by re-run of leapp in the upgrade initramfs after previous failure, which causes additional confusing error message hiding original bugs (#996)
- Prevent the upgrade with RHSM when a baseos and an appstream target repositories are not discovered (#1001)
- RHUI(Azure) handle correctly various SAP images (#1037)
- Rework the network configuration handling and parse the configuration data properly (#969)
- Set RHSM release for non-ga and non-beta channels (#1033)
- Use the "grub" library to find the GRUB device (#989)
- [IPU 7 -> 8] Detect corrupted grubenv file (#1012)
- [IPU 7 -> 8] Ensure that rsyncd stays enabled if it is enabled prior the upgrade(#1043)
- [IPU 7 -> 8] Ensure that satellite metapackages are installed after the upgrade (#994)
- [IPU 7 -> 8] Ensure the device_cio_free service stays enabled on s390x after the upgrade (#977)
- [IPU 7 -> 8] Fixed checks for RHEL SAP IPU 7.9 -> 8.6 (#978)
- [IPU 7 -> 8] Fixed migration of ntp to chrony when the ntp service is masked (#966)
- [IPU 7 -> 8] Prevent the traceback during migration of sendmail configuration files when the package is not installed (#1041)
- [IPU 7 -> 8] Satellite: reindex all related databases to fix issues due to new locales in RHEL 8 (#1007, #1018)
- [IPU 7 -> 8] Use the correct domain name in SSSD reports (#1040)
- [IPU 8 -> 9] Added checks for RHEL SAP IPU 8.6 -> 9.0 (#978)
- [IPU 8 -> 9] CheckVDO: Ask user for the confirmation only on failures and undetermined devices (#961)
- [IPU 8 -> 9] Fix the kernel detection during initramfs creation for new kernel on RHEL 9.2+ (#1048)
- [IPU 8 -> 9] Fix the upgrade on Azure using RHUI for SAP Apps images (#975)
- [IPU 8 -> 9] Handle correctly firewalld version 0.8 (#963)

### Enhancements
- Set new upgrade paths (#988):
-- RHEL 7.9 -> 8.8, 8.6 (default: 8.8)
-- RHEL 8.6 -> 9.0
-- RHEL 8.8 -> 9.2
- Check that used leapp data are valid and compatible with the installed leapp-repository (#1003)
- Detect a proxy configuration in YUM/DNF and adjust an error msg on issues caused by the configuration (#914)
- Detect and report systemd symlinks that are broken before the upgrade (#972)
- Drop obsoleted upgrade paths (#1047)
- Improve remediation instructions for packages in unknown repositories (#1010)
- Improve the error message to guide users when discovered more space is needed (#956)
- Introduce --nogpgcheck option to skip checking of RPM signatures (#910)
- Introduced an option to use an ISO file as a target RHEL version content source (#979)
- Introduced possibility to specify what systemd services should be enabled/disabled on the upgraded system (#964)
- Map the target repositories also based on the installed content (#967)
- Provide common information about systemd services (#959)
- Register subscribed systems automatically to Red Hat Insights unless --no-insights-register is used (#1000)
- Remove obsoleted GPG keys provided by RH after the upgrade to prevent errors (#1022)
- Run upgrade process with checking RPM signatures by default (#910, #993, #1025)
- Save breadcrumbs results as RHSM facts (#1002)
- Small improvements in various reports (#1038, #1039, #1032)
- [IPU 8 -> 9] Detect CIFS also when upgrading from RHEL8 to RHEL9 (PR1035)
- [IPU 8 -> 9] Detect RoCE on IBM Z machines and check the configuration is safe for the upgrade (#1030)
- [IPU 8 -> 9] Enable upgrades of RHEL 8 for SAP HANA to RHEL 9 on ppc64le (#1042)
- [IPU 8 -> 9] Improve the handling of blocklisted certificates (#992)

## Additional changes interesting for devels
- Started work on bringing up networking inside the upgrade initramfs - currently available for testing and development purposes when LEAPP_DEVEL_INITRAM_NETWORK is set (#960)
- Add leapp debug tools to the upgrade initramfs - dracut upgrade module (#997)
- Enable disabling of DNF plugins in the dnfplugin library (#990)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fernflower fernflower fernflower approved these changes

@MichalHe MichalHe MichalHe left review comments

Assignees

@MichalHe MichalHe

Labels
changelog-checked The merger/reviewer checked the changelog draft document and updated it when relevant
Projects
None yet
Milestone
8.8/9.2
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@PeterMocary @MichalHe @fernflower @pirat89