Skip to content

Commit

Permalink
SECURITY-3017
Browse files Browse the repository at this point in the history
  • Loading branch information
@jonesbusy @Kevin-CB
jonesbusy authored and Kevin-CB committed May 15, 2023
1 parent 8191fd5 commit 4cbc486
Show file tree
Hide file tree
Showing 7 changed files with 34 additions and 20 deletions.
2 changes: 1 addition & 1 deletion src/main/java/org/jenkinsci/plugins/ansible/AbstractAnsibleInvocation.java
View file
Expand Up @@ -121,7 +121,7 @@ public ArgumentListBuilder appendExtraVars(ArgumentListBuilder args) {
if (extraVars != null && ! extraVars.isEmpty()) {
for (ExtraVar var : extraVars) {
args.add("-e");
String value = envVars.expand(var.getValue());
String value = envVars.expand(var.getSecretValue().getPlainText());

Check warning on line 124 in src/main/java/org/jenkinsci/plugins/ansible/AbstractAnsibleInvocation.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line 

Line 124 is not covered by tests
if (Pattern.compile("\\s").matcher(value).find()) {
value = Util.singleQuote(value);
}
Expand Down
27 changes: 19 additions & 8 deletions src/main/java/org/jenkinsci/plugins/ansible/ExtraVar.java
View file
Expand Up @@ -18,42 +18,53 @@
import hudson.Extension;
import hudson.model.AbstractDescribableImpl;
import hudson.model.Descriptor;
import hudson.util.Secret;

import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.DataBoundSetter;

public class ExtraVar extends AbstractDescribableImpl<ExtraVar> {

public String key;

public String value;
public transient String value;

public Secret secretValue;

public boolean hidden;
public boolean hidden = true;

@DataBoundConstructor
public ExtraVar() {
}

protected Object readResolve() {
if (value != null) {

Check warning on line 41 in src/main/java/org/jenkinsci/plugins/ansible/ExtraVar.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line 

Line 41 is only partially covered, one branch is missing
this.setSecretValue(Secret.fromString(value));

Check warning on line 42 in src/main/java/org/jenkinsci/plugins/ansible/ExtraVar.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line 

Line 42 is not covered by tests
}
return this;
}

@DataBoundSetter
public void setKey(String key) {
this.key = key;
}

@DataBoundSetter
public void setValue(String value) {
this.value = value;
public void setHidden(boolean hidden) {
this.hidden = hidden;
}

@DataBoundSetter
public void setHidden(boolean hidden) {
this.hidden = hidden;
public void setSecretValue(Secret value) {
this.secretValue = value;
}

public String getKey() {
return key;
}

public String getValue() {
return value;
public Secret getSecretValue() {
return this.secretValue;
}

public boolean isHidden() {
Expand Down
4 changes: 3 additions & 1 deletion src/main/java/org/jenkinsci/plugins/ansible/jobdsl/context/ExtraVarsContext.java
View file
Expand Up @@ -6,6 +6,8 @@
import javaposse.jobdsl.dsl.Context;
import org.jenkinsci.plugins.ansible.ExtraVar;

import hudson.util.Secret;

/**
* @author pawbur (Pawel Burchard)
*/
Expand All @@ -15,7 +17,7 @@ public class ExtraVarsContext implements Context {
public void extraVar(String key, String value, boolean hidden) {
ExtraVar extraVar = new ExtraVar();
extraVar.setKey(key);
extraVar.setValue(value);
extraVar.setSecretValue(Secret.fromString(value));
extraVar.setHidden(hidden);
this.extraVars.add(extraVar);
}
Expand Down
5 changes: 3 additions & 2 deletions src/main/java/org/jenkinsci/plugins/ansible/workflow/AnsiblePlaybookStep.java
View file
Expand Up @@ -38,6 +38,7 @@
import hudson.model.Run;
import hudson.model.TaskListener;
import hudson.util.ListBoxModel;
import hudson.util.Secret;
import jenkins.model.Jenkins;
import org.apache.commons.lang.StringUtils;
import org.jenkinsci.plugins.ansible.AnsibleInstallation;
Expand Down Expand Up @@ -383,10 +384,10 @@ private List<ExtraVar> convertExtraVars(Map<String, Object> extraVars) {
var.setKey(entry.getKey());
Object o = entry.getValue();
if (o instanceof Map) {
var.setValue(((Map)o).get("value").toString());
var.setSecretValue((Secret)((Map)o).get("value"));

Check warning on line 387 in src/main/java/org/jenkinsci/plugins/ansible/workflow/AnsiblePlaybookStep.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line 

Line 387 is not covered by tests
var.setHidden((Boolean)((Map)o).get("hidden"));
} else {
var.setValue(o.toString());
var.setSecretValue((Secret)o);

Check warning on line 390 in src/main/java/org/jenkinsci/plugins/ansible/workflow/AnsiblePlaybookStep.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line 

Line 390 is not covered by tests
var.setHidden(false);
}
extraVarList.add(var);
Expand Down
6 changes: 3 additions & 3 deletions src/main/resources/org/jenkinsci/plugins/ansible/AnsibleAdHocCommandBuilder/config.jelly
View file
Expand Up @@ -71,11 +71,11 @@
<f:entry title="${%Key}" field="key">
<f:textbox clazz="required" />
</f:entry>
<f:entry title="${%Value}" field="value">
<f:textbox clazz="required" />
<f:entry title="${%Value}" field="secretValue">
<f:password clazz="required" />
</f:entry>
<f:entry title="${%Hidden variable in build log}" field="hidden">
<f:checkbox default="false" />
<f:checkbox default="true" />
</f:entry>
<f:entry>
<div align="right" class="repeatable-delete show-if-only">
Expand Down
6 changes: 3 additions & 3 deletions src/main/resources/org/jenkinsci/plugins/ansible/AnsiblePlaybookBuilder/config.jelly
View file
Expand Up @@ -80,11 +80,11 @@
<f:entry title="${%Key}" field="key">
<f:textbox clazz="required" />
</f:entry>
<f:entry title="${%Value}" field="value">
<f:textbox clazz="required" />
<f:entry title="${%Value}" field="secretValue">
<f:password clazz="required" />
</f:entry>
<f:entry title="${%Hidden variable in build log}" field="hidden">
<f:checkbox default="false" />
<f:checkbox default="true" />
</f:entry>
<f:entry>
<div align="right" class="repeatable-delete show-if-only">
Expand Down
4 changes: 2 additions & 2 deletions src/test/java/org/jenkinsci/plugins/ansible/jobdsl/JobDslIntegrationTest.java
View file
Expand Up @@ -64,7 +64,7 @@ public void shouldCreateJobWithPlaybookDsl() throws Exception {
assertThat("disableHostKeyChecking", step.disableHostKeyChecking, is(false));
assertThat("additionalParameters", step.additionalParameters, is("params"));
assertThat("extraVar.key", step.extraVars.get(0).getKey(), is("key"));
assertThat("extraVar.value", step.extraVars.get(0).getValue(), is("value"));
assertThat("extraVar.value", step.extraVars.get(0).getSecretValue().getPlainText(), is("value"));
assertThat("extraVar.hidden", step.extraVars.get(0).isHidden(), is(true));

}
Expand Down Expand Up @@ -93,7 +93,7 @@ public void shouldCreateJobWithLegacyPlaybookDsl() throws Exception {
assertThat("disableHostKeyChecking", step.disableHostKeyChecking, is(true));
assertThat("additionalParameters", step.additionalParameters, is("params"));
assertThat("extraVar.key", step.extraVars.get(0).getKey(), is("key"));
assertThat("extraVar.value", step.extraVars.get(0).getValue(), is("value"));
assertThat("extraVar.value", step.extraVars.get(0).getSecretValue().getPlainText(), is("value"));
assertThat("extraVar.hidden", step.extraVars.get(0).isHidden(), is(true));

}
Expand Down

4 comments on commit 4cbc486

@seanmceligot
Copy link

Choose a reason for hiding this comment

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

This fix breaks extraVars

node {
  stage "test ansible plugin"
  writeFile file: 'hello.yml', text: '''
- name: test
  hosts: 127.0.0.1
  tasks:
    - ansible.builtin.debug:
       var: foo
    - assert:
       that:
        - foo == "bar"
'''
ansiblePlaybook extraVars: [foo:"bar"], playbook: 'hello.yml'
}

org.jenkinsci.plugins.workflow.actions.ErrorAction$ErrorId: 3628ee6a-0282-452b-823b-903b95bef410 java.lang.ClassCastException: class java.lang.String cannot be cast to class hudson.util.Secret (java.lang.String is in module java.base of loader 'bootstrap'; hudson.util.Secret is in unnamed module of loader org.eclipse.jetty.webapp.WebAppClassLoader @1700915) at org.jenkinsci.plugins.ansible.workflow.AnsiblePlaybookStep$AnsiblePlaybookExecution.convertExtraVars(AnsiblePlaybookStep.java:390)

@jonesbusy
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fix breaks extraVars

node {
  stage "test ansible plugin"
  writeFile file: 'hello.yml', text: '''
- name: test
  hosts: 127.0.0.1
  tasks:
    - ansible.builtin.debug:
       var: foo
    - assert:
       that:
        - foo == "bar"
'''
ansiblePlaybook extraVars: [foo:"bar"], playbook: 'hello.yml'
}

org.jenkinsci.plugins.workflow.actions.ErrorAction$ErrorId: 3628ee6a-0282-452b-823b-903b95bef410 java.lang.ClassCastException: class java.lang.String cannot be cast to class hudson.util.Secret (java.lang.String is in module java.base of loader 'bootstrap'; hudson.util.Secret is in unnamed module of loader org.eclipse.jetty.webapp.WebAppClassLoader @1700915) at org.jenkinsci.plugins.ansible.workflow.AnsiblePlaybookStep$AnsiblePlaybookExecution.convertExtraVars(AnsiblePlaybookStep.java:390)

Thanks @seanmceligot

I was able to reproduce. I will do a release today.

Sadly this plugin doesn't have test for pipeline code... (Any PR is welcome).

Regards,

@jonesbusy
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be fixed on this release : https://github.com/jenkinsci/ansible-plugin/releases/tag/215.v1c78933a_b_0d7

@seanmceligot
Copy link

Choose a reason for hiding this comment

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

Sadly this plugin doesn't have test for pipeline code... (Any PR is welcome).

Is it possible to run test jenkins pipeline in a gitlab action?

Please sign in to comment.