Fix case - cow_on_read_path #6724
Conversation
WalkthroughThe changes add an unprivileged user configuration to the nbdkit test variants and implement a non-root execution path in the test runner. In v2v/tests/cfg/nbdkit/nbdkit.cfg the Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
v2v/tests/src/nbdkit/nbdkit.py (1)
3-3: Guard against missingunprivileged_userin params
unprivileged_user = params.get('unprivileged_user')will beNoneoutside the new variant or if the cfg is misconfigured, which then breaks later formatting and system calls. Consider failing early when theregular_user_sudocheckpoint is selected andunprivileged_useris empty (for example via a simple check andtest.error(...)) to make the failure mode explicit instead of getting harder‑to‑debug exceptions down in the branch.Also applies to: 26-26
v2v/tests/cfg/nbdkit/nbdkit.cfg (1)
43-46: Nestedregular_user_sudovariant overridescow_on_read_pathcheckpointBy setting
checkpoint = 'regular_user_sudo'in this nested variant, the final params for this path will havecheckpointequal toregular_user_sudo, so the dispatcher innbdkit.pywill not callcow_on_read_path()for this variant—it will go into the newregular_user_sudobranch instead.If the intent is to still exercise the
cow_on_read_pathtest while adding the unprivileged‑user/sudo setup, consider either:
- keeping
checkpoint = 'cow_on_read_path'here and driving the behavior via the newunprivileged_userparam, or- leaving this checkpoint as
regular_user_sudobut having that branch callcow_on_read_path()after it finishes the user/key setup.Please confirm which behavior you want and adjust either the cfg or the Python branch accordingly so the test actually covers the cow_on_read_path scenario under this new mode.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
v2v/tests/cfg/nbdkit/nbdkit.cfg(1 hunks)v2v/tests/src/nbdkit/nbdkit.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.14.8)
v2v/tests/src/nbdkit/nbdkit.py
789-789: Local variable pub_key is assigned to but never used
Remove assignment to unused variable pub_key
(F841)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.11
- GitHub Check: Python 3.12
- GitHub Check: Python 3.9
- GitHub Check: Python 3.8
v2v/tests/src/nbdkit/nbdkit.py
Outdated
| elif checkpoint == 'regular_user_sudo': | ||
| LOG.info("I am in ") | ||
| regular_sudo_config = '/etc/sudoers.d/v2v_test' | ||
| with open(regular_sudo_config, 'w') as fd: | ||
| fd.write('%s ALL=(ALL) NOPASSWD: ALL' % unprivileged_user) | ||
|
|
||
| # create user | ||
| try: | ||
| pwd.getpwnam(unprivileged_user) | ||
| except KeyError: | ||
| process.system("useradd %s" % unprivileged_user) | ||
|
|
||
| # generate ssh-key | ||
| rsa_private_key_path = '/home/%s/.ssh/id_rsa' % unprivileged_user | ||
| rsa_public_key_path = '/home/%s/.ssh/id_rsa.pub' % unprivileged_user | ||
| process.system( | ||
| 'su - %s -c \'ssh-keygen -t rsa -q -N "" -f %s\'' % | ||
| (unprivileged_user, rsa_private_key_path)) | ||
|
|
||
| with open(rsa_public_key_path) as fd: | ||
| pub_key = fd.read() |
There was a problem hiding this comment.
regular_user_sudo checkpoint: hard failures, sudoers robustness, and unused pub_key
A few issues here:
-
Likely failure on first run for a new user (no
~/.sshdir).
For a freshly created user,/home/<user>/.sshusually doesn’t exist.ssh-keygen -f /home/<user>/.ssh/id_rsawill fail if the parent directory is missing, causingprocess.system(...)to raise and the checkpoint to fail. You should ensure the.sshdirectory exists (and has 0700 perms) before callingssh-keygen, ideally using the user’s home frompwd.getpwnam(unprivileged_user).pw_dirinstead of hard‑coding/home/.... -
Sudoers file permissions and safety.
/etc/sudoers.dentries are expected to be owned by root and typically0440; writing with default permissions may cause sudo to ignore or reject the file. After writingregular_sudo_config, it’s safer to explicitly set mode (and, if needed, ownership), e.g.os.chmod(regular_sudo_config, 0o440). Also consider adding a newline at the end of the line for readability in that file. -
Checkpoint doesn’t exercise
cow_on_read_path()at all.
With the cfg change, theregular_user_sudovariant overridescheckpointto'regular_user_sudo', so this branch runs instead ofcow_on_read_path(). Right now it only sets up a user/keys and then returns. If the intent is to run the existingcow_on_read_pathtest under this unprivileged sudo scenario, you probably want to callcow_on_read_path()at the end of this branch (or avoid overridingcheckpointand drive the behavior via an extra param). Please double‑check the intended flow. -
Unused
pub_key.
pub_keyis read but never used, which Ruff correctly flags. Either use it (for example, to propagate into other test steps) or drop the read/variable to keep the code clean.
One possible direction for the ssh‑related fixes:
- elif checkpoint == 'regular_user_sudo':
- LOG.info("I am in ")
- regular_sudo_config = '/etc/sudoers.d/v2v_test'
- with open(regular_sudo_config, 'w') as fd:
- fd.write('%s ALL=(ALL) NOPASSWD: ALL' % unprivileged_user)
-
- # create user
- try:
- pwd.getpwnam(unprivileged_user)
- except KeyError:
- process.system("useradd %s" % unprivileged_user)
-
- # generate ssh-key
- rsa_private_key_path = '/home/%s/.ssh/id_rsa' % unprivileged_user
- rsa_public_key_path = '/home/%s/.ssh/id_rsa.pub' % unprivileged_user
- process.system(
- 'su - %s -c \'ssh-keygen -t rsa -q -N "" -f %s\'' %
- (unprivileged_user, rsa_private_key_path))
-
- with open(rsa_public_key_path) as fd:
- pub_key = fd.read()
+ elif checkpoint == 'regular_user_sudo':
+ regular_sudo_config = '/etc/sudoers.d/v2v_test'
+ with open(regular_sudo_config, 'w') as fd:
+ fd.write('%s ALL=(ALL) NOPASSWD: ALL\n' % unprivileged_user)
+ os.chmod(regular_sudo_config, 0o440)
+
+ # ensure user exists
+ try:
+ pw = pwd.getpwnam(unprivileged_user)
+ except KeyError:
+ process.system('useradd %s' % unprivileged_user)
+ pw = pwd.getpwnam(unprivileged_user)
+
+ # generate ssh key under user's home
+ ssh_dir = os.path.join(pw.pw_dir, '.ssh')
+ os.makedirs(ssh_dir, mode=0o700, exist_ok=True)
+ rsa_private_key_path = os.path.join(ssh_dir, 'id_rsa')
+ rsa_public_key_path = rsa_private_key_path + '.pub'
+ process.system(
+ 'su - %s -c \'ssh-keygen -t rsa -q -N "" -f "%s"\'' %
+ (unprivileged_user, rsa_private_key_path))
+
+ # If needed later, read or export pub_key here; otherwise drop this.
+ # with open(rsa_public_key_path) as fd:
+ # pub_key = fd.read()
+
+ # Optionally run the actual test if this checkpoint is meant to
+ # extend cow_on_read_path():
+ # cow_on_read_path()Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.8)
789-789: Local variable pub_key is assigned to but never used
Remove assignment to unused variable pub_key
(F841)
🤖 Prompt for AI Agents
In v2v/tests/src/nbdkit/nbdkit.py around lines 769-789, the regular_user_sudo
branch hard-codes /home paths, can fail when the .ssh directory is missing,
writes a sudoers.d file without safe permissions/ownership, never exercises
cow_on_read_path(), and reads pub_key without using it; fix by resolving the
unprivileged user’s home via pwd.getpwnam(unprivileged_user).pw_dir, ensure the
~/.ssh directory exists and is owned by that user with mode 0o700 before running
ssh-keygen, write the sudoers fragment then set its ownership to root and mode
to 0o440 and ensure it ends with a newline, either invoke cow_on_read_path() at
the end of this branch (if the intent is to run the test under the sudoed user)
or remove the branch override, and remove the unused pub_key read or use it
where needed (e.g., install into authorized_keys) so the variable is not unused.
Signed-off-by: Ganesh Hubale <ghubale@redhat.com>
ganeshhubale
force-pushed
the
fix-read-path
branch
from
6da7f55 to
ecb86a8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
v2v/tests/src/nbdkit/nbdkit.py (3)
786-787: Remove unusedpub_keyvariable.The public key is read but never used. Either use it (e.g., install into
authorized_keys) or remove the read operation to keep the code clean.- with open(rsa_public_key_path) as fd: - pub_key = fd.read() -
779-784: Hard-coded paths will fail for newly created users.The code hard-codes
/home/{user}/.ssh/id_rsa, which has two problems:
- User home directories aren't always in
/home(e.g., could be/var/lib/{user})- For newly created users, the
.sshdirectory won't exist, causingssh-keygento fail# generate ssh-key - rsa_private_key_path = '/home/%s/.ssh/id_rsa' % unprivileged_user - rsa_public_key_path = '/home/%s/.ssh/id_rsa.pub' % unprivileged_user + pw = pwd.getpwnam(unprivileged_user) + ssh_dir = os.path.join(pw.pw_dir, '.ssh') + os.makedirs(ssh_dir, mode=0o700, exist_ok=True) + process.run('chown %s:%s %s' % (unprivileged_user, unprivileged_user, ssh_dir), shell=True) + rsa_private_key_path = os.path.join(ssh_dir, 'id_rsa') + rsa_public_key_path = rsa_private_key_path + '.pub' process.system( 'su - %s -c \'ssh-keygen -t rsa -q -N "" -f %s\'' % (unprivileged_user, rsa_private_key_path))
768-771: Sudoers file needs proper permissions and formatting.The sudoers file is created with default permissions, but
/etc/sudoers.dentries must be owned by root and typically set to mode 0440. Without proper permissions, sudo may ignore or reject the file.regular_sudo_config = '/etc/sudoers.d/v2v_test' try: with open(regular_sudo_config, 'w') as fd: - fd.write('%s ALL=(ALL) NOPASSWD: ALL' % unprivileged_user) + fd.write('%s ALL=(ALL) NOPASSWD: ALL\n' % unprivileged_user) + os.chmod(regular_sudo_config, 0o440)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
v2v/tests/cfg/nbdkit/nbdkit.cfg(1 hunks)v2v/tests/src/nbdkit/nbdkit.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v2v/tests/cfg/nbdkit/nbdkit.cfg
🧰 Additional context used
🪛 Ruff (0.14.8)
v2v/tests/src/nbdkit/nbdkit.py
787-787: Local variable pub_key is assigned to but never used
Remove assignment to unused variable pub_key
(F841)
797-797: Function call with shell=True parameter identified, security issue
(S604)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.8
- GitHub Check: Python 3.12
- GitHub Check: Python 3.9
- GitHub Check: Python 3.11
🔇 Additional comments (2)
v2v/tests/src/nbdkit/nbdkit.py (2)
3-3: LGTM!The
pwdmodule import is necessary for checking user existence viapwd.getpwnam().
26-26: LGTM!Parameter retrieval is appropriate for the non-root execution flow.
| regular_sudo_config = '/etc/sudoers.d/v2v_test' | ||
| try: | ||
| with open(regular_sudo_config, 'w') as fd: | ||
| fd.write('%s ALL=(ALL) NOPASSWD: ALL' % unprivileged_user) | ||
|
|
||
| # create user | ||
| try: | ||
| pwd.getpwnam(unprivileged_user) | ||
| except KeyError: | ||
| process.system("useradd %s" % unprivileged_user) | ||
|
|
||
| # generate ssh-key | ||
| rsa_private_key_path = '/home/%s/.ssh/id_rsa' % unprivileged_user | ||
| rsa_public_key_path = '/home/%s/.ssh/id_rsa.pub' % unprivileged_user | ||
| process.system( | ||
| 'su - %s -c \'ssh-keygen -t rsa -q -N "" -f %s\'' % | ||
| (unprivileged_user, rsa_private_key_path)) | ||
|
|
||
| with open(rsa_public_key_path) as fd: | ||
| pub_key = fd.read() | ||
|
|
||
| # Run the cow_on_read_path test as the non-root user | ||
| LOG.info("Running cow_on_read_path test as non-root user: %s" % unprivileged_user) | ||
| cow_on_read_path() | ||
| finally: | ||
| # Cleanup: remove sudoers config and user | ||
| if os.path.exists(regular_sudo_config): | ||
| os.remove(regular_sudo_config) | ||
| if unprivileged_user: | ||
| process.run("userdel -fr %s" % unprivileged_user, shell=True, ignore_status=True) |
There was a problem hiding this comment.
Critical: Test doesn't actually run as the unprivileged user.
The code sets up sudo access, creates a user, and generates SSH keys, but then calls cow_on_read_path() directly on line 791, which runs as root—not as the unprivileged user. This defeats the purpose of the PR.
To run the test as the unprivileged user, you need to wrap the cow_on_read_path() call with su:
- # Run the cow_on_read_path test as the non-root user
- LOG.info("Running cow_on_read_path test as non-root user: %s" % unprivileged_user)
- cow_on_read_path()
+ # Run the cow_on_read_path test as the non-root user
+ LOG.info("Running cow_on_read_path test as non-root user: %s" % unprivileged_user)
+ process.run('su - %s -c "python3 -c \'from %s import cow_on_read_path; cow_on_read_path()\'"' %
+ (unprivileged_user, __name__), shell=True)However, this approach may not work directly since cow_on_read_path() is a nested function. Consider either:
- Refactoring to make the test runnable via a command-line entry point, or
- Using
sudo -uto run the specific nbdkit commands withincow_on_read_path()as the unprivileged user, or - Verifying if the intent is just to ensure sudo access works (in which case the SSH key generation logic should be removed as unnecessary).
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.8)
787-787: Local variable pub_key is assigned to but never used
Remove assignment to unused variable pub_key
(F841)
797-797: Function call with shell=True parameter identified, security issue
(S604)
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.