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

Improving check-profile #773

Merged
merged 4 commits into from Jul 5, 2021
Merged

Conversation

joseivanlopez
Copy link
Contributor

@joseivanlopez joseivanlopez commented Jul 2, 2021

Problem

A new check-profile command was recently added. This command is intended to check an AutoYaST profile without applying changes to the system. Generally, such a check could be done without root permissions, but there are some situations where root permissions are required:

  • With option run-scripts=true, if the script code requires root permissions.
  • With an ERB profile, if the ERB code requires root permissions.
  • With option import-all=true, if any of the imported modules requires probing.

Running scripts/ERB code as root could lead to security problems, see:

Solution

It is difficult (or almost impossible) to know in advance if the command will require root access. The only way to ensure that the check will no fail because the permissions would be:

  • By requiring root when run-scripts=true or import-all=true or the profile is an ERB. But this can be overkill in situations where the script/erb/probing does not require root.
  • By executing YaST in a sandboxed environment (docker, namespaces) in order to avoid changes in the system. But it could have several implications and, at this moment, YaST is not well prepared for it.

It was agreed with security team to go with the following solution:

  • Extend the command help with the implications of executing it as root.
  • Add a new run-erb option in order to explicitly indicate that you want to execute the ERB. This option is mandatory if the profile is an ERB file and the check is executed as root.

Testing

  • Added unit tests
  • Manually tested

@coveralls
Copy link

coveralls commented Jul 2, 2021

Coverage Status

Coverage increased (+0.2%) to 65.78% when pulling 979ef7b on joseivanlopez:root_warning into b43c13b on yast:master.

@joseivanlopez joseivanlopez marked this pull request as ready for review July 5, 2021 11:24
Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

It looks (almost) good. Just a comment about the option description.

src/lib/autoinstall/clients/autoyast.rb Outdated Show resolved Hide resolved
@joseivanlopez joseivanlopez merged commit 1704175 into yast:master Jul 5, 2021
@yast-bot
Copy link
Contributor

yast-bot commented Jul 5, 2021

❌ Internal Jenkins job #111 failed

@yast-bot
Copy link
Contributor

yast-bot commented Jul 5, 2021

✔️ Public Jenkins job #197 successfully finished

@joseivanlopez joseivanlopez mentioned this pull request Jul 6, 2021
@mvidner
Copy link
Member

mvidner commented Jul 12, 2021

@mvidner
Copy link
Member

mvidner commented Jul 12, 2021

My plan is to extend the doc-sle section Checking a control file (source)

  1. adding a note about run-erb
  2. removing the sudo and explaining when it is needed

But now while testing it on SP3 I have found that (2.) is currently pointless as it never works without root, because get_file_from_url wants to mkdir -p /run/YaST2/mount but a user cannot write in /run. It works if you first run it as root, because the dirs are not removed.

/usr/sbin/yast autoyast check-profile filename=/vagrant/opensuse_minimal.xml output=check-profile-output.xml

test file: https://github.com/os-autoinst/os-autoinst-distri-opensuse/blob/master/data/autoyast_opensuse/opensuse_minimal.xml

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants