net: Ensure a tmp with exec permissions for dhcp #1690
Conversation
cloudinit/net/dhcp.py
Outdated
| @@ -41,7 +41,9 @@ class NoDHCPLeaseMissingDhclientError(NoDHCPLeaseError): | |||
| """Raised when unable to find dhclient.""" | |||
|
|
|||
|
|
|||
| def maybe_perform_dhcp_discovery(nic=None, dhcp_log_func=None): | |||
| def maybe_perform_dhcp_discovery( | |||
| nic=None, dhcp_log_func=None, alt_exe_dir=None | |||
There was a problem hiding this comment.
Perhaps needs_exe=True to temp_utils.tmpdir() should just do the right thing and avoid requiring alt_exe_dir passed around?
There was a problem hiding this comment.
I was wondering about that too (or some other way to reduce the number of repeated changes), however doing that would couple temp_utils to distro. Is maybe that why @aciba90 didn't do this originally? I don't know off the top of my head if such a change would cause circular import issues, but it might.
There was a problem hiding this comment.
Refactored to expose the target tmp_dir and let callers (datasources, modules) determine if the dir is mounted in a noexec point and give an exec dir via the distro in use.
|
Thank you for fixing this!! :D |
In the case cloudinit.temp_utils points to a fs mounted as noexec and needs_exe=True, fallback to use os.join.path(Distro.usr_lib_exec, "cloud-init/clouddir) that will be mounted with exec perms.
It exposes the target tmp_dir and let callers (datasources, modules) determine if the dir is mounted in a noexec point and give a exec dir via the distro in use.
|
Thanks, @cjp256 and @holmanb, for the constructive comments! I have addressed your comments by exposing a function to let the callers of This is ready to be reviewed after CI passes. Please, let me know if there are more concerns. |
aciba90
requested review from
cjp256 and
holmanb
and removed request for
cjp256
There was a problem hiding this comment.
I really like the integration test addition.
I assume fstab failing to mount on boot would cause the test to fail, but do you think we should add an assertion that ensures the test can't pass if not mounted noexec? If you think this is completely redundant, feel free to push back on this.
Thanks, @holmanb, for the review and suggestion. I think it is a great idea to assert test preconditions. I have done so and retested it. |
| args = [tuple()] | ||
| _args = list(args) | ||
| if len(_args) == 0 and "args" not in kwargs: | ||
| _args.append(tuple()) |
There was a problem hiding this comment.
If this branch is taken, then _args will be set to [()] and later on line 315 we will be concatenating a list and a tuple, which will cause a typerror.
_args[0] = [tmpf] + _args[0]and a traceback like:
Traceback (most recent call last):
File "/home/holmanb/cloud-init-a/tmp.py", line 24, in <module>
subp_blob_in_tempfile(None, None)
File "/home/holmanb/cloud-init-a/tmp.py", line 22, in subp_blob_in_tempfile
_args[0] = [tmpf] + _args[0]
TypeError: can only concatenate list (not "tuple") to list
There was a problem hiding this comment.
I just noticed one last nit. It's not something you caused, but since we're modifying this chunk of code I think we should fix it. Pyright picks up the error on main, but not on your branch. The problem still exists, however, it just gets hidden from type analyzers in the changes here. I suspect a fix should be as simple as s/tuple()/list()/g on line highlighted in the comment.
I'll merge once that issue is fixed (I'd merge now but I want a second opinion on my suggestion).
|
Thanks Alberto @aciba90! |
In the case cloudinit.temp_utils points to a fs mounted as noexec and needs_exe=True, fallback to use os.join.path(Distro.usr_lib_exec, "cloud-init/clouddir) that will be mounted with exec perms. LP: #1962343
Proposed Commit Message
Additional Context
https://bugs.launchpad.net/cloud-init/+bug/1962343
https://warthogs.atlassian.net/browse/SC-958
Test Steps
Execute
tests/integration_tests/datasources/test_tmp_noexec.pyChecklist: