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

Re-enable automatic approval of launch command if configured by admin #47

Merged
merged 2 commits into from
May 25, 2022
Merged

Re-enable automatic approval of launch command if configured by admin #47

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
86de048
Auto approve launch command if configured by admin
yaroslavafenkin May 9, 2022
c158b7b
Adjust approval rules
yaroslavafenkin May 18, 2022
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
7 changes: 4 additions & 3 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
<properties>
<changelist>999999-SNAPSHOT</changelist>
<gitHubRepo>jenkinsci/${project.artifactId}-plugin</gitHubRepo>
<jenkins.version>2.277.1</jenkins.version>
<jenkins.version>2.332.1</jenkins.version>
</properties>
<name>Command Agent Launcher Plugin</name>
<description>Allows agents to be launched using a specified command.</description>
Expand Down Expand Up @@ -46,8 +46,8 @@
<dependencies>
<dependency>
<groupId>io.jenkins.tools.bom</groupId>
<artifactId>bom-2.277.x</artifactId>
<version>984.vb5eaac999a7e</version>
<artifactId>bom-2.332.x</artifactId>
<version>1382.v7d694476f340</version>
<type>pom</type>
<scope>import</scope>
</dependency>
Expand All @@ -57,6 +57,7 @@
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>script-security</artifactId>
<version>1172.v35f6a_0b_8207e</version><!-- TODO: Remove when version in BOM is at least that -->
</dependency>
<dependency>
<groupId>io.jenkins</groupId>
Expand Down
7 changes: 4 additions & 3 deletions src/main/java/hudson/slaves/CommandConnector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import hudson.model.TaskListener;
import hudson.util.FormValidation;
import java.io.IOException;
import jenkins.model.Jenkins;
import org.jenkinsci.Symbol;
import org.jenkinsci.plugins.command_launcher.Messages;
import org.jenkinsci.plugins.scriptsecurity.scripts.ApprovalContext;
Expand All @@ -49,11 +50,11 @@ public class CommandConnector extends ComputerConnector {
public CommandConnector(String command) {
this.command = command;
// TODO add withKey if we can determine the Cloud.name being configured
ScriptApproval.get().configuring(command, SystemCommandLanguage.get(), ApprovalContext.create().withCurrentUser());
ScriptApproval.get().configuring(command, SystemCommandLanguage.get(), ApprovalContext.create().withCurrentUser(), true);
Copy link
Member

Choose a reason for hiding this comment

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

Is this safe? I was thinking we would need to have oldCommand get passed through the UI like with CpsFlowDefinition, but I am not familiar with this plugin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if anyone other than admins can configure that, need to check.
If it's only admins who have permissions then I assume we're safe with true.

Copy link
Member

@dwnusbaum dwnusbaum May 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I think the relevant config page might only require Computer/Configure permission, but I'm not entirely sure. I guess if it always required administer permission then there would be no need for the plugin to integrate with script-security at all.

Copy link
Member

Choose a reason for hiding this comment

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

IIUC the discussion: Computer/Configure is enough to access the page, that's why this plugin and the security fix preceding it were needed in the first place.

Copy link
Member

Choose a reason for hiding this comment

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

Done in c158b7b.

}

private Object readResolve() {
ScriptApproval.get().configuring(command, SystemCommandLanguage.get(), ApprovalContext.create());
ScriptApproval.get().configuring(command, SystemCommandLanguage.get(), ApprovalContext.create(), true);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we would ever have an active user in this context, so maybe false makes more sense? Not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I thought about this too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems this one has to stay as true because of submission via REST and CLI

Copy link
Member

Choose a reason for hiding this comment

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

Ok, and it looks like both of those mechanisms of updating the configuration fully replace the existing content, so it seems ok to assume that admins always approve scripts in those cases.

return this;
}

Expand All @@ -74,7 +75,7 @@ public FormValidation doCheckCommand(@QueryParameter String value) {
if (Util.fixEmptyAndTrim(value) == null) {
return FormValidation.error(Messages.CommandLauncher_NoLaunchCommand());
} else {
return ScriptApproval.get().checking(value, SystemCommandLanguage.get());
return ScriptApproval.get().checking(value, SystemCommandLanguage.get(), Jenkins.get().hasPermission(Jenkins.ADMINISTER));
}
}

Expand Down
6 changes: 3 additions & 3 deletions src/main/java/hudson/slaves/CommandLauncher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ public CommandLauncher(String command) {
agentCommand = command;
env = null;
// TODO add withKey if we can determine the Slave.nodeName being configured
ScriptApproval.get().configuring(command, SystemCommandLanguage.get(), ApprovalContext.create().withCurrentUser());
ScriptApproval.get().configuring(command, SystemCommandLanguage.get(), ApprovalContext.create().withCurrentUser(), true);
}

/** Constructor for programmatic use. Always approves the script.
Expand All @@ -102,7 +102,7 @@ public CommandLauncher(String command, EnvVars env) {
}

private Object readResolve() {
ScriptApproval.get().configuring(agentCommand, SystemCommandLanguage.get(), ApprovalContext.create());
ScriptApproval.get().configuring(agentCommand, SystemCommandLanguage.get(), ApprovalContext.create(), true);
return this;
}

Expand Down Expand Up @@ -228,7 +228,7 @@ public FormValidation doCheckCommand(@QueryParameter String value) {
if(Util.fixEmptyAndTrim(value)==null)
return FormValidation.error(org.jenkinsci.plugins.command_launcher.Messages.CommandLauncher_NoLaunchCommand());
else
return ScriptApproval.get().checking(value, SystemCommandLanguage.get());
return ScriptApproval.get().checking(value, SystemCommandLanguage.get(), Jenkins.get().hasPermission(Jenkins.ADMINISTER));
}
}
}