Re-enable automatic approval of launch command if configured by admin #47
Conversation
| } | ||
|
|
||
| private Object readResolve() { | ||
| ScriptApproval.get().configuring(command, SystemCommandLanguage.get(), ApprovalContext.create()); | ||
| ScriptApproval.get().configuring(command, SystemCommandLanguage.get(), ApprovalContext.create(), true); |
There was a problem hiding this comment.
I don't think we would ever have an active user in this context, so maybe false makes more sense? Not sure.
There was a problem hiding this comment.
Yeah, I thought about this too
There was a problem hiding this comment.
Seems this one has to stay as true because of submission via REST and CLI
There was a problem hiding this comment.
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.
| @@ -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); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
* command now gets implicitly approved only if manually edited by admin
|
|
||
| @Override | ||
| public ComputerLauncher newInstance(@Nullable StaplerRequest req, @NonNull JSONObject formData) throws FormException { | ||
| CommandLauncher instance = (CommandLauncher) super.newInstance(req, formData); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Checked CommandLauncher, it gets called. Haven't verified if CommandConnector override gets called though, but the two are very similar.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Works with ec2-fleet, newInstance was called ✔️
There was a problem hiding this comment.
Looks good to me assuming that https://github.com/jenkinsci/command-launcher-plugin/pull/47/files#r879581449 is not a problem.
| } | ||
|
|
||
| private Object readResolve() { | ||
| ScriptApproval.get().configuring(command, SystemCommandLanguage.get(), ApprovalContext.create()); | ||
| ScriptApproval.get().configuring(command, SystemCommandLanguage.get(), ApprovalContext.create(), true); |
There was a problem hiding this comment.
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.
|
Could this please be released to facilitate BOM/PCT testing? |
It is released :) |
|
Sorry for not noticing, and thank you! |
Re-enable automatic approval of launch command if configured by administrator user. Related to the script-security hardening (jenkinsci/script-security-plugin@f4c0bb9)