Implemented writeYaml step #23

witokondoria · 2017-06-15T16:02:53Z

Delegates to snakeyaml the task of serializing any Object to a yaml file

rsandell · 2017-06-16T09:22:31Z

pom.xml

@@ -28,7 +28,7 @@
    <parent>
        <groupId>org.jenkins-ci.plugins</groupId>
        <artifactId>plugin</artifactId>
-        <version>1.609.1</version>
+        <version>2.19</version>


Why do you need this?
I don't see any reason for it. As I've said in previous PRs; When we bump anything here I want to see a more rigorous cleanup for example moving to more recent workflow.version and cleaning up all @Inject etc. And that should go in a separate PR.

The real reason was to bump surefire plugin up to 2.19.1 so testing could be done with -Dtest=method1#method2. Will revert the change

rsandell · 2017-06-16T09:25:37Z

src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/conf/WriteYamlStep.java

+
+		@Override
+		public Step newInstance(Map<String, Object> arguments) throws Exception {
+			WriteYamlStep step = new WriteYamlStep();


I doubt this is needed for this step, skip it and just put file and data as required params in the @DataBoundConstructor.
The special handling here that you've seen in other steps are because they have an either or relation that can't be expressed in a better way.

rsandell · 2017-06-16T09:26:20Z

src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/conf/WriteYamlStep.java

+
+		@Override
+		protected Void run () throws Exception {
+            FilePath path = null;


Bad formating.

rsandell · 2017-06-16T09:54:12Z

src/test/java/org/jenkinsci/plugins/pipeline/utility/steps/conf/WriteYamlStepTest.java

+                "def l = [] \n" +
+                        "node('slaves') {\n" +
+                        "  writeYaml file: 'test', data: /['a': ]/ \n" +
+                        " sh 'cat test' \n" +


There is a feature coming soon to pipeline which stores the parameters of steps, that might risk printing the step to the log, which means your assert might not assert on the thing it think it does.

It would be more precise if you archived the file and read it back in in the test to verify the contents. But it's just a nit.

Will readYaml in every test

witokondoria · 2017-07-03T07:54:04Z

Requested changes were adressed. @rsandell, could you review them?

rsandell

Sorry for not responding earlier. But I have been waiting for today's security announcement.
Since this has the same security vulnerability as was disclosed in Script Security with groovy.json.JsonOutput.toJson(Object) as it allows arbitrary get methods to be invoked that a script would normally not be allowed to access.
One way we could solve this would be to only allow the step to take a Map. The other would be to somehow either look at the classloader to see if it is an object declared in the script or a library or to inspect the object to see if all get methods are whitelisted before proceeding. Or maybe the yaml library has a filtering option where we could filter only whitelisted getters somehow.

witokondoria · 2017-07-11T13:31:27Z

Completely understandable, better block a bug than fix it at a later stage.
I believe its easier/leaner to limit the classes to dump (ala script-approval). Inspecting the object to check if every getter is whitelisted does not seem really clear.

Have added a recursive (:imp:) isValidObjectType method to check wether the object to dump is "safe". WDYT?

rsandell

Besides my comments it looks good to me.
Just would like someone from the Jenkins cert team to take another look in case I've missed something.

rsandell · 2017-07-11T13:46:50Z

src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/conf/WriteYamlStep.java

+            Boolean ret = true;
+            Set entry = ((Map) obj).entrySet();
+            Iterator iterator = entry.iterator();
+            while (iterator.hasNext() && ret) {


This looks a bit convoluted, something like this would be more readable and simpler I think

for (Map.Entry entry : map.entrySet()) { if (!isValidObject(entry.getKey()) || !isValidObject(entry.getValue())) { return false; } }

It also seems like you are overwriting the ret var every time in the loop, so effectively you are only checking the last entry in the map which my suggestion also solves.

True that, Its result of a late commit. Will fix it.

rsandell · 2017-07-11T13:49:43Z

src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/conf/WriteYamlStep.java

+        } else if (obj instanceof Collection)  {
+            Boolean ret = true;
+            Iterator iterator = ((Collection) obj).iterator();
+            while (iterator.hasNext() && ret) {


Same simplification and issue here as for the Map.
Suggestion:

for (Object o : ((Collection)obj)) { if (!isValidObject(o)) { return false; } }

oleg-nenashev

The feature itself looks good, but I am very aware about the isValidObjectType implementation

oleg-nenashev · 2017-07-11T14:05:22Z

pom.xml

+    <pluginRepositories>
+        <pluginRepository>
+            <id>repo.jenkins-ci.org</id>
+            <url>http://repo.jenkins-ci.org/public/</url>


oleg-nenashev · 2017-07-11T14:06:25Z

src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/conf/WriteYamlStep.java

+ * Writes a yaml file from the workspace.
+ *
+ * @author Javier DELGADO &lt;witokondoria@gmail.com&gt;.
+ */


since TODO would be useful since we make attempts to visualize extension javadocs

not needed. it's not present in any other of the steps.

oleg-nenashev · 2017-07-11T14:07:01Z

src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/conf/WriteYamlStep.java

+
+    @DataBoundConstructor
+    public WriteYamlStep(@Nonnull String file, @Nonnull Object data) {
+        if (file == null) {


StringUtils.isBlank()

oleg-nenashev · 2017-07-11T14:07:11Z

src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/conf/WriteYamlStep.java

+        if (file == null) {
+            throw new IllegalArgumentException("file parameter must be provided to writeYaml");
+        }
+        this.file = file;


truncate spaces?

Does it also make sense to verify the patch and to ensure nobody passes an absolute one or a relative with traversal to upper levels?

want to trim the string? Forcing a path from within the current current workspace seems fine

From the writeFile Step, paths are not verified (so traversal is also possible).

Would you prefer a higer level implementation for this check?

oleg-nenashev · 2017-07-11T14:09:14Z

src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/conf/WriteYamlStep.java

+     *
+     * @param file file name
+     */
+    public void setFile(String file) {


Do you need setters? If no, I would advice to either remove them and make the fields final OR to convert the class to @DataBoundSetters

oleg-nenashev · 2017-07-11T14:10:27Z

src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/conf/WriteYamlStep.java

+            (obj instanceof URL) || (obj instanceof Calendar) ||
+            (obj instanceof Date) || (obj instanceof UUID) ||
+            (obj == null)) {
+            return true;


It is strange, error-prone (E.g. GString, Long, etc.) and ineffective. Why would you need it?

Its not a complete and final list, but a one less strict than plain Maps, with objects already whitelisted when toJson invocations:
Long is covered by Number
GStrings hasnt been even tested :(

My guess is that the GStrings will be evaluated/expanded in CPS/sandbox before entering the java context, but it needs to be tested non the less.

something like:

writeYaml data: "${currentBuild.rawBuild}", file: 'data'

might be enough

will add tests tomorrow!

daniel-beck

Haven't been able to break this in some limited testing as indicated, but that's not exactly a strong endorsement :-/

daniel-beck · 2017-07-11T14:03:36Z

.../resources/org/jenkinsci/plugins/pipeline/utility/steps/conf/WriteYamlStep/config.properties

+file.title=File
+file.description=File to save as YAML
+text.title=Data
+text.description=Object to serialize yo YAML


Yo dawg, I heard yo and yo dawg like yo yos.

LOL, that made my afternoon

daniel-beck · 2017-07-11T14:46:01Z

src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/conf/WriteYamlStep.java

+    private boolean isValidObjectType(Object obj) {
+        if ((obj instanceof Boolean) || (obj instanceof Character) ||
+            (obj instanceof Number) || (obj instanceof String) ||
+            (obj instanceof URL) || (obj instanceof Calendar) ||


I had some concerns about this, but it seems it's safe enough:
https://bitbucket.org/asomov/snakeyaml/src/6c6516c003eb4902a0c4c308100c26b99e2ef99c/src/main/java/org/yaml/snakeyaml/representer/SafeRepresenter.java?at=default&fileviewer=file-view-default#SafeRepresenter.java-331

daniel-beck · 2017-07-11T14:47:09Z

pom.xml

@@ -175,4 +175,11 @@
        <url>https://github.com/jenkinsci/pipeline-utility-steps-plugin</url>
        <tag>HEAD</tag>
    </scm>
+
+    <pluginRepositories>


Time for a new parent pom before trying to fix this.

witokondoria · 2017-07-12T08:17:06Z

Most of the changes were adressed (path check is unimplemented). Im willing to squash the commits in one, but have left all of them to easen reviews.

rsandell requested changes Jun 16, 2017

View reviewed changes

Implemented writeYaml step

ef1a671

witokondoria force-pushed the feature/writeYaml branch from a5746eb to ef1a671 Compare June 16, 2017 13:13

rsandell requested changes Jul 10, 2017

View reviewed changes

just accept basic types for yaml dumps

0fb26fa

rsandell requested changes Jul 11, 2017

View reviewed changes

rsandell requested review from jglick, stephenc, daniel-beck and oleg-nenashev July 11, 2017 13:53

oleg-nenashev requested changes Jul 11, 2017

View reviewed changes

daniel-beck reviewed Jul 11, 2017

View reviewed changes

witokondoria added 6 commits July 12, 2017 09:29

simplifications by @rsandell

a3f1355

ssl FTW

bcc4c34

isBlank check to the Step constructor

12c9c7c

unused setters

fafb31c

tests ensuring GStrings gets expanded before step call

376be02

proper language

15e5fa6

rsandell approved these changes Aug 4, 2017

View reviewed changes

rsandell merged commit 254439b into jenkinsci:master Aug 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented writeYaml step #23

Implemented writeYaml step #23

witokondoria commented Jun 15, 2017 •

edited

Loading

rsandell Jun 16, 2017

witokondoria Jun 16, 2017

rsandell Jun 16, 2017

rsandell Jun 16, 2017

rsandell Jun 16, 2017

witokondoria Jun 16, 2017

witokondoria commented Jul 3, 2017

rsandell left a comment

witokondoria commented Jul 11, 2017

rsandell left a comment

rsandell Jul 11, 2017

witokondoria Jul 11, 2017 •

edited

Loading

rsandell Jul 11, 2017

witokondoria Jul 11, 2017

oleg-nenashev left a comment

oleg-nenashev Jul 11, 2017

oleg-nenashev Jul 11, 2017

rsandell Jul 11, 2017

oleg-nenashev Jul 11, 2017

oleg-nenashev Jul 11, 2017

oleg-nenashev Jul 11, 2017

witokondoria Jul 11, 2017

witokondoria Jul 12, 2017

oleg-nenashev Jul 11, 2017

oleg-nenashev Jul 11, 2017

witokondoria Jul 11, 2017

rsandell Jul 11, 2017

witokondoria Jul 11, 2017

daniel-beck left a comment

daniel-beck Jul 11, 2017

witokondoria Jul 11, 2017 •

edited

Loading

daniel-beck Jul 11, 2017

daniel-beck Jul 11, 2017

witokondoria commented Jul 12, 2017

Implemented writeYaml step #23

Implemented writeYaml step #23

Conversation

witokondoria commented Jun 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

witokondoria commented Jul 3, 2017

rsandell left a comment

Choose a reason for hiding this comment

witokondoria commented Jul 11, 2017

rsandell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

witokondoria Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-beck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

witokondoria Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

witokondoria commented Jul 12, 2017

witokondoria commented Jun 15, 2017 •

edited

Loading

witokondoria Jul 11, 2017 •

edited

Loading

witokondoria Jul 11, 2017 •

edited

Loading