Add SARIF support #40

shaopeng-gh · 2022-03-23T04:21:54Z

Add support for output in SARIF format.

Hello! We are interested in adding support for output in the open-standard SARIF format to puppet-lint. SARIF support is required to integrate it in GitHub code scanning. Doing so would make it available to every repo in GitHub and could result in increase in user base.

More about SARIF here:
What is SARIF?
Why SARIF?

After the support for SARIF output is added to the tool and published a new version, we will work on creating a starter workflow to make it available as a GitHub code scanner.

CLAassistant · 2022-03-23T04:21:58Z

All committers have signed the CLA.

shaopeng-gh · 2022-03-23T04:28:14Z

@yongyan-gh
@eddynaka

ekohl

I've been wondering if puppet-lint shouldn't properly implement a --format option. In #34 I already added a start with this.

lib/puppet-lint/bin.rb

shaopeng-gh · 2022-03-24T20:42:36Z

@ekohl

SARIF format need to have a description of the rule (beside the message that is very specific the the error found which we already have now),
and a help uri if it is available.

I can see in each rule it do have this info but in the comments.
I will go ahead and modify each rule and also expose them in the object properties so I can get in the code,

let me know if that sounds good?

the above example is not a good example, the message do not have any extra data so it looks like it can be directly used as description as well, but we have rules like this:
:message => "indentation of => is not properly aligned (expected in column #{arrow_column[level_idx]}, but found it in column #{arrow_tok.column})",

the rule description we are looking for, is static for a rule.

In reply to: 1078242810

shaopeng-gh · 2022-03-30T02:33:52Z

@ekohl

SARIF format need to have a description of the rule (beside the message that is very specific the the error found which we already have now), and a help uri if it is available.

I can see in each rule it do have this info but in the comments. I will go ahead and modify each rule and also expose them in the object properties so I can get in the code,

let me know if that sounds good?

the above example is not a good example, the message do not have any extra data so it looks like it can be directly used as description as well, but we have rules like this: :message => "indentation of => is not properly aligned (expected in column #{arrow_column[level_idx]}, but found it in column #{arrow_tok.column})",

the rule description we are looking for, is static for a rule.

I have done making this change and updated the PR.

fyi the new data will be visible in SARIF viewer UI like below:

In reply to: 1082556441

ekohl

What about plugins which haven't added a description yet? Will that work well? Should it also document something for plugin writers?

ekohl · 2022-04-04T09:56:03Z

lib/puppet-lint.rb

@@ -180,9 +180,8 @@ def report(problems)
        print_github_annotation(message) if configuration.github_actions
      end
    end
-    puts JSON.pretty_generate(json) if configuration.json


This could affect usage via Rake. I don't know if people do this, but there are effectively 2 entry points: the bin file which you already found and Rake:
https://github.com/puppetlabs/puppet-lint/blob/020143b705b023946739eb44e7c7d99fcd087527/lib/puppet-lint/tasks/puppet-lint.rb#L88-L107=

thanks for all the inputs! I will set the PR as draft for now and look into them.

I have followed this instruction:
https://github.com/puppetlabs/puppet-lint/blob/master/README.md#testing-with-puppet-lint-as-a-rake-task

then running rake lint seems not having issue and output result normally.
I think running in rake currently only have normal
output does not support json option in the configuration, so the line highlighted will not be run in rake lint command, so should be good.

ekohl · 2022-04-04T09:57:30Z

lib/puppet-lint/bin.rb

+      report_sarif(all_problems, full_base_path, full_base_path_uri) if PuppetLint.configuration.sarif
+
+      if PuppetLint.configuration.json


Should these 2 be mutually exclusive? Either JSON or sarif?

they should be mutually exclusive, added the if else.

ekohl · 2022-04-04T09:58:43Z

lib/puppet-lint/bin.rb

+          problems.each do |problem|
+            [:description, :help_uri].each { |key| problem.delete(key) }
+          end


There's also delete_if which is probably cleaner. Or you could be explicit and use Hash.slice and select the entries that are relevant.

yes, changed to use delete_if

This reverts commit 427333f.

shaopeng-gh · 2022-04-12T23:26:23Z

I have verified running with plugins don't have those 2 new properties, will not throw any error and will just have those 2 sarif properties not populated.
I found this file lib/puppet-lint/checkplugin.rb has some description, I added the 2 new properties.

In reply to: 930112225

ekohl

I think my concerns have been addressed. However, I'm not a maintainer on this repository.

@binford2k you may want to give some visibility on this. It could be interesting for developers.

ekohl · 2022-04-19T16:39:18Z

.github/workflows/scan.yml

+
+  security:
+
+    strategy:


It feels odd to have a matrix here if there's just a single entry. Is that really needed?

Matrix is not needed, I have removed it.

ekohl · 2022-04-19T16:41:24Z

.github/workflows/scan.yml

@@ -0,0 +1,39 @@
+name: puppet-lint scan


This is an interesting thing. Do you have an example of where this ran so I can see the resulting report?

This is just a sample (or sometimes called a starter workflow) that the user of the puppet-lint can add to their own repo, after we got all things done. User can use it to scan their repo and get code scanning alerts of the .pp files in their repo.
GitHub support this and the format used is SARIF format.

this is a run in my fork as example: (it is not intended to run against the puppet-lint repo itself, but the user's repo)
https://github.com/shaopeng-gh/puppet-lint/security/code-scanning

for people don't have permission:

ekohl · 2022-04-19T16:42:56Z

lib/puppet-lint/bin.rb

+      elsif PuppetLint.configuration.json
+        all_problems.each do |problems|
+          problems.each do |problem|
+            problem.delete_if { |key, _| [:description, :help_uri].include?(key) }


I'm not sure the description must be excluded. The help URI also doesn't hurt IMHO. Perhaps a maintainer can weigh in on that.

Perhaps a maintainer can weigh in on that. ---- yes the only reason for this code block is I am trying to maintain the same JSON output as before, maintainer can advise if also want these 2 new properties in JSON and let me know to change accordingly.

ekohl · 2022-05-21T10:19:58Z

At this point I think this looks good, but someone who actually has access on this module needs to step in and do a review. I can't even approve CI to run.

alexjfisher · 2022-05-21T10:57:31Z

@binford2k Can I nominate @ekohl for trusted contributor access to this repo? :)

chelnak · 2022-05-21T13:04:29Z

I've kicked off the CI run 👍

shaopeng-gh · 2022-05-31T04:34:39Z

Thanks for looking and triggering the CI! let me know.

(The plan is after the support for SARIF output is added to the tool, I will work on creating a starter workflow in the GitHub code scanner starter workflow repo to make it available as a GitHub code scanner. I have added more information in the description of this PR at the top. )

chelnak · 2022-05-31T08:09:43Z

@shaopeng-gh This looks fine and it looks like you have addressed all of the comments from @ekohl.

My only question is do we need the scan.yml in this repo? If it is an example, would it be better to include in in the README?

Thanks!

shaopeng-gh · 2022-05-31T21:39:08Z

@chelnak your are correct, I have removed this file form the pr. It is just an example I will be submitting it to GitHub code scanner starter workflow repo, and user will be able to use it.
After that is done we can add a few lines in the readme mentioning there is a GitHub starter workflows available.

shaopeng-gh · 2022-06-09T19:59:17Z

Thanks, let me know if can be merged.

shaopeng-gh · 2022-06-14T10:29:08Z

A gentle ping, after this is merged and a new version created I will work on the GitHub code scanner starter workflow. Thanks!

david22swan

LGTM
Just need's a squash merge

david22swan · 2022-06-14T10:52:24Z

@shaopeng-gh Thanks for putting in the work :)

chelnak · 2022-06-14T13:09:55Z

@shaopeng-gh I'm really looking forward to seeing what happens with this.

Thank you 🙏

shaopeng-gh · 2022-06-23T00:13:16Z

@chelnak
thanks, not familiar with how Ruby Gems are updated, can a new version to be published so that in the workflow I can use below command to install and have the --sarif support:
gem install puppet-lint
https://rubygems.org/gems/puppet-lint/versions I see last version is 2.5.2 - September 14, 2021

add SARIF support

60e3ac2

shaopeng-gh requested a review from a team as a code owner March 23, 2022 04:21

ekohl reviewed Mar 23, 2022

View reviewed changes

lib/puppet-lint/bin.rb Outdated Show resolved Hide resolved

lib/puppet-lint/bin.rb Outdated Show resolved Hide resolved

lib/puppet-lint/bin.rb Outdated Show resolved Hide resolved

lib/puppet-lint/bin.rb Outdated Show resolved Hide resolved

pretty print the whole json at the end

283382b

shaopeng-gh added 3 commits March 27, 2022 23:01

SARIF base path uri should end with /

8627248

Use better sample data in SARIF template

8d15b84

Add help_uri and description to SARIF output

b2e0dce

update example values in sarif template

40cfdd4

ekohl reviewed Apr 4, 2022

View reviewed changes

shaopeng-gh marked this pull request as draft April 4, 2022 23:51

shaopeng-gh added 16 commits April 4, 2022 17:09

test workflow

d916b98

test puppet-lint-action

db93036

test workflow

6e42257

test

f9555df

test path

40c367a

continue-on-error: true

99c91a3

use .

8af4705

change order

427333f

Revert "change order"

369fadd

This reverts commit 427333f.

change order

36e1fbd

remove warning lines

747505a

clean up

d145c7f

use STDERR.put for warnings

0123103

Merge branch 'master' of https://github.com/shaopeng-gh/puppet-lint

ef1b694

remove clean step

16d625d

new line

7fcf324

shaopeng-gh added 4 commits April 7, 2022 21:32

fix test with stderr

1b2da02

use Pathname.new

01f8bd8

use delete_if

17ad7a8

Fix comments

119f16f

shaopeng-gh marked this pull request as ready for review April 12, 2022 23:40

ekohl reviewed Apr 19, 2022

View reviewed changes

shaopeng-gh added 2 commits April 19, 2022 15:56

remove not needed strategy matrix

57c7af8

remove not needed strategy matrix

7232414

shaopeng-gh mentioned this pull request Apr 20, 2022

Adding SARIF support #43

Closed

shaopeng-gh added 2 commits May 10, 2022 00:12

fix rule index

74b39e6

upgrade to use upload-sarif@v2 instead of upload-sarif@v1

9d91452

removed example scan.yml

eca7fe0

david22swan approved these changes Jun 14, 2022

View reviewed changes

david22swan merged commit 97bd8da into puppetlabs:master Jun 14, 2022

chelnak added the enhancement New feature or request label Oct 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SARIF support #40

Add SARIF support #40

shaopeng-gh commented Mar 23, 2022 •

edited

Loading

CLAassistant commented Mar 23, 2022 •

edited

Loading

shaopeng-gh commented Mar 23, 2022

ekohl left a comment

shaopeng-gh commented Mar 24, 2022 •

edited

Loading

shaopeng-gh commented Mar 30, 2022 •

edited

Loading

ekohl left a comment

ekohl Apr 4, 2022

shaopeng-gh Apr 4, 2022

shaopeng-gh Apr 12, 2022

ekohl Apr 4, 2022

shaopeng-gh Apr 12, 2022

ekohl Apr 4, 2022

shaopeng-gh Apr 12, 2022

shaopeng-gh commented Apr 12, 2022

ekohl left a comment

ekohl Apr 19, 2022

shaopeng-gh Apr 19, 2022

ekohl Apr 19, 2022

shaopeng-gh Apr 19, 2022

ekohl Apr 19, 2022

shaopeng-gh Apr 19, 2022

ekohl commented May 21, 2022

alexjfisher commented May 21, 2022

chelnak commented May 21, 2022

shaopeng-gh commented May 31, 2022

chelnak commented May 31, 2022

shaopeng-gh commented May 31, 2022

shaopeng-gh commented Jun 9, 2022

shaopeng-gh commented Jun 14, 2022

david22swan left a comment

david22swan commented Jun 14, 2022

chelnak commented Jun 14, 2022

shaopeng-gh commented Jun 23, 2022

		report_sarif(all_problems, full_base_path, full_base_path_uri) if PuppetLint.configuration.sarif

		if PuppetLint.configuration.json

Add SARIF support #40

Add SARIF support #40

Conversation

shaopeng-gh commented Mar 23, 2022 • edited Loading

CLAassistant commented Mar 23, 2022 • edited Loading

shaopeng-gh commented Mar 23, 2022

ekohl left a comment

Choose a reason for hiding this comment

shaopeng-gh commented Mar 24, 2022 • edited Loading

shaopeng-gh commented Mar 30, 2022 • edited Loading

ekohl 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

shaopeng-gh commented Apr 12, 2022

ekohl 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

ekohl commented May 21, 2022

alexjfisher commented May 21, 2022

chelnak commented May 21, 2022

shaopeng-gh commented May 31, 2022

chelnak commented May 31, 2022

shaopeng-gh commented May 31, 2022

shaopeng-gh commented Jun 9, 2022

shaopeng-gh commented Jun 14, 2022

david22swan left a comment

Choose a reason for hiding this comment

david22swan commented Jun 14, 2022

chelnak commented Jun 14, 2022

shaopeng-gh commented Jun 23, 2022

shaopeng-gh commented Mar 23, 2022 •

edited

Loading

CLAassistant commented Mar 23, 2022 •

edited

Loading

shaopeng-gh commented Mar 24, 2022 •

edited

Loading

shaopeng-gh commented Mar 30, 2022 •

edited

Loading