Skip to content

Fixes #39132 - Do not rely on ACLs where not strictly necessary #131

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

Merged
adamruzicka merged 1 commit into from
Mar 11, 2026
+7 −2
Merged

Fixes #39132 - Do not rely on ACLs where not strictly necessary #131

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions lib/smart_proxy_remote_execution_ssh/runners/script_runner.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,8 +420,13 @@ def check_expecting_disconnect
end

def ensure_effective_user_access(*paths, mode: 'rx')
unless @user_method.is_a? NoopUserMethod
ensure_remote_command("setfacl -m u:#{@user_method.effective_user}:#{mode} #{paths.join(' ')}")
return if @user_method.is_a?(NoopUserMethod) || @user_method.effective_user == 'root'
Copy link
Copy Markdown
Contributor

@adamruzicka adamruzicka Mar 6, 2026

Choose a reason for hiding this comment

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

In case where @user_method.effective_user == 'root' && @user_method.ssh_user != 'root', does the ssh user have permissions to clean up all the working directories around L256 when the job is done?

Copy link
Copy Markdown
Contributor Author

@pondrejk pondrejk Mar 9, 2026

Choose a reason for hiding this comment

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

hm, well using the scenario from the downstream tests it does, but I suppose that's because it is in wheel, so I guess it would depend on the ssh_user rights. But this was the case before this change as well.

Copy link
Copy Markdown
Contributor

@adamruzicka adamruzicka Mar 9, 2026

Choose a reason for hiding this comment

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

that's because it is in wheel

Ohh, so the ownership transfers to the effective user, but the group ownership is left to the connection user's primary group, meaning the connection user retains write permissions through the group, cool. So not necessarily because of wheel, but because of a group


paths_str = paths.join(' ')
if @user_method.ssh_user == 'root'
ensure_remote_command("chown #{@user_method.effective_user} #{paths_str} && chmod u=#{mode} #{paths_str}")
else
ensure_remote_command("setfacl -m u:#{@user_method.effective_user}:#{mode} #{paths_str}")
end
end
end
Expand Down