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 all commits
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
29 changes: 25 additions & 4 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 @@ -23,19 +23,26 @@
*/
package hudson.slaves;

import edu.umd.cs.findbugs.annotations.NonNull;
import edu.umd.cs.findbugs.annotations.Nullable;
import hudson.EnvVars;
import hudson.Extension;
import hudson.Util;
import hudson.model.TaskListener;
import hudson.util.FormValidation;
import java.io.IOException;

import net.sf.json.JSONObject;
import org.apache.commons.lang.StringUtils;
import org.jenkinsci.Symbol;
import org.jenkinsci.plugins.command_launcher.Messages;
import org.jenkinsci.plugins.scriptsecurity.scripts.ApprovalContext;
import org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval;
import org.jenkinsci.plugins.scriptsecurity.scripts.languages.SystemCommandLanguage;
import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.QueryParameter;
import org.kohsuke.stapler.Stapler;
import org.kohsuke.stapler.StaplerRequest;

/**
* Executes a program on the controller and expect that script to connect.
Expand All @@ -49,11 +56,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(), Stapler.getCurrentRequest() == null);
}

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 @@ -65,16 +72,30 @@ public CommandLauncher launch(String host, TaskListener listener) throws IOExcep

@Extension @Symbol("command")
public static class DescriptorImpl extends ComputerConnectorDescriptor {
@Override
public ComputerConnector newInstance(@Nullable StaplerRequest req, @NonNull JSONObject formData) throws FormException {
CommandConnector instance = (CommandConnector) super.newInstance(req, formData);
if (formData.get("oldCommand") != null) {
String oldCommand = formData.getString("oldCommand");
boolean approveIfAdmin = !StringUtils.equals(oldCommand, instance.command);
if (approveIfAdmin) {
ScriptApproval.get().configuring(instance.command, SystemCommandLanguage.get(),
ApprovalContext.create().withCurrentUser(), true);
}
}
return instance;
}

@Override
public String getDisplayName() {
return Messages.CommandLauncher_displayName();
}

public FormValidation doCheckCommand(@QueryParameter String value) {
public FormValidation doCheckCommand(@QueryParameter String value, @QueryParameter String oldCommand) {
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(), !StringUtils.equals(value, oldCommand));
}
}

Expand Down
27 changes: 23 additions & 4 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 @@ -23,6 +23,8 @@
*/
package hudson.slaves;

import edu.umd.cs.findbugs.annotations.NonNull;
import edu.umd.cs.findbugs.annotations.Nullable;
import hudson.AbortException;
import hudson.EnvVars;
import hudson.Extension;
Expand All @@ -40,6 +42,7 @@
import java.util.logging.Level;
import java.util.logging.Logger;
import jenkins.model.Jenkins;
import net.sf.json.JSONObject;
import org.apache.commons.lang.StringUtils;
import org.jenkinsci.Symbol;
import org.jenkinsci.plugins.scriptsecurity.scripts.ApprovalContext;
Expand All @@ -48,6 +51,8 @@
import org.jenkinsci.plugins.scriptsecurity.scripts.languages.SystemCommandLanguage;
import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.QueryParameter;
import org.kohsuke.stapler.Stapler;
import org.kohsuke.stapler.StaplerRequest;

/**
* {@link ComputerLauncher} through a remote login mechanism like ssh/rsh.
Expand Down Expand Up @@ -79,7 +84,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(), Stapler.getCurrentRequest() == null);
}

/** Constructor for programmatic use. Always approves the script.
Expand All @@ -102,7 +107,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 @@ -220,15 +225,29 @@ private static void reportProcessTerminated(Process proc, TaskListener listener)

@Extension @Symbol("command")
public static class DescriptorImpl extends Descriptor<ComputerLauncher> {

@Override
public ComputerLauncher newInstance(@Nullable StaplerRequest req, @NonNull JSONObject formData) throws FormException {
CommandLauncher instance = (CommandLauncher) super.newInstance(req, formData);
Copy link
Member

@dwnusbaum dwnusbaum May 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Did you check that both of the newInstance overrides actually get invoked when the objects are constructed? Just in case something else needs to be changed like with workflow-cps and workflow-job.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checked CommandLauncher, it gets called. Haven't verified if CommandConnector override gets called though, but the two are very similar.

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 entirely sure, but I think CommandConnector is never used. I've looked at ssh-slaves-plugin, there's SSHLauncher/config.jelly which in turn includes the corresponding connector template SSHConnector/config.jelly. And that one specifies the details for ssh connection.

Here it's CommandLauncher's config.xml that provides us an input field to specify a launch command.

Copy link
Member

@dwnusbaum dwnusbaum May 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think that cloud plugins may use it, for example ec2-fleet, delta-cloud, spotinst, and I think one proprietary plugin I have access to. Some other clouds appear to only use a cloud-specific subclass of ComputerConnector, in which case this class is not used. So either way, this definitely does not seem to be a commonly used class, but it might be possible to test it with one of those plugins.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works with ec2-fleet, newInstance was called ✔️

if (formData.get("oldCommand") != null) {
String oldCommand = formData.getString("oldCommand");
boolean approveIfAdmin = !StringUtils.equals(oldCommand, instance.agentCommand);
if (approveIfAdmin) {
ScriptApproval.get().configuring(instance.agentCommand, SystemCommandLanguage.get(),
ApprovalContext.create().withCurrentUser(), true);
}
}
return instance;
}
public String getDisplayName() {
return org.jenkinsci.plugins.command_launcher.Messages.CommandLauncher_displayName();
}

public FormValidation doCheckCommand(@QueryParameter String value) {
public FormValidation doCheckCommand(@QueryParameter String value, @QueryParameter String oldCommand) {
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(), !StringUtils.equals(value, oldCommand));
}
}
}
1 change: 1 addition & 0 deletions src/main/resources/hudson/slaves/CommandConnector/config.jelly
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ THE SOFTWARE.

<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form">
<input type="hidden" name="oldCommand" value="${instance.command}"/>
<f:entry title="${%Launch command}" field="command">
<f:textbox />
</f:entry>
Expand Down
1 change: 1 addition & 0 deletions src/main/resources/hudson/slaves/CommandLauncher/config.jelly
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ THE SOFTWARE.

<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form">
<input type="hidden" name="oldCommand" value="${instance.command}"/>
<f:entry title="${%Launch command}" field="command">
<f:textbox />
</f:entry>
Expand Down