Semgrep: Working with multiple policies

[msorens]

Is your feature request related to a problem? Please describe.

_While my PRs are set up to do differential runs, I wanted to set up a periodic scan of the entire code base as well. This is the outcome of that exploration._

My differential runs (from the makefile perspective) look like this--this effectively runs via CI with every PR. (Here SEMGREP_REPO is "chef/automate", my real repository, and that uses a semgrep policy designated Chef-01.)

docker run $(DOCKER_PARAMS) $(SEMGREP_REPO) $(SEMGREP_CONTAINER) python $(SEMGREP_COMMON_PARAMS) --baseline-ref master

I am now running this next line in our once-nightly CI, where SEMGREP_NIGHTLY_REPO is "chef/automate-nightly".

docker run $(DOCKER_PARAMS) $(SEMGREP_NIGHTLY_REPO) $(SEMGREP_CONTAINER) python $(SEMGREP_COMMON_PARAMS)

The key point here is that "chef/automate-nightly" is fictitious! It is simply a string that is different from "chef/automate", which allows me to use a different policy (Chef-nightly). Using the fictitious repo name, though, does have a minor problem--it causes invalid links to be generated both on the dashboard and in slack notifications. But that is a small price to pay for the benefit.

What is the benefit? Compare the two policies here:

For ongoing work (new PRs) using Chef-01, I want more stringent checks--notice there are three less overrides there. But for a full scan of the existing code -- using Chef-nightly -- I do not want the three highlighted items to add noise to the semgrep output. Those items are "nice to have" for new work, but they are not really of any significance and do not need to be pointed out in the existing code base.

So this leads me to several feature requests that I am lumping all together in this single issue. I leave it as an exercise for the reader to subdivide this as you see fit. 😁

(1) To get rid of the invalid links problem: would be neat if the system would use the whole repo name for creating a project name but ignore anything after a certain meta-character when generating links. Then I could provide, e.g., "chef/automatenightly" as my repo name to get a distinct project called "chef/automatenightly", but generated links would ignore the *nightly portion, using only "chef/automate".

(2) Would like an easy way to copy a policy. Would have been very handy for this exercise. As is, I had to manually duplicate all the rulesets and exceptions one at a time into my new policy. Maintenance problem: if original policy is updated later, chances are want to update the copied policy, too, requiring double the work.

(3) Would like an easy way to "subclass a policy". Would have been even nicer than (2); this would eliminate maintenance work moving forward. That is, my Chef-nightly policy is a strict superset of Chef-01. So if I could just say the contents of Chef-nightly is equal to Chef-01 plus two more overrides on ruleset x and one more override on ruleset y, then it is future change-proof.

(4) Would like an optional comment/note/description for each rule override in a policy; as it is, I have to keep a separate note file to remember why I wanted to exclude certain rules.

(5) Speaking of why I wanted to exclude certain rules, there were a couple that do not quite make sense to me and a couple others that are not working correctly. Consider adding a way for interested users to upvote/downvote each rule in a ruleset so other users could get "crowdsourced opinion" on rules that are generally considered good or considered bad.

(6) Would like to see the policy page tightened up a bit. The top figure shows my policy when I land on the page. As shown, upon expanding the overrides on one, one sees the bottom figure--but the pagination navigation is off the bottom of the screen! When the page is displaying so few items like this, then the navigation controls should always be visible. Alternately, if you change the page to display a longer list, then it is OK to let the navigation controls be off-screen. Also, there seems to be a lot of wasted white space between override rows. Notice in my earlier figure how I tightened up the line spacing. 😁 (My personal preference is for seeing a long list on one page so as not to have to keep jumping back and forth between pages, but consider making the number-per-page configurable to make avoid unhappiness one way or the other.)

[chmccreery]

Thanks for creating this!

1) feels like working around a bigger problem in our workflow, and I would rather fix the big problem instead of patching the holes in a workaround. But feel free to ping me again later if this is really bothering you and we haven't provided a better way to do this.

I just submitted a PR for 2) because it has been on my to-do list for awhile and I just needed a little encouragement 👍 I agree that it isn't good from a maintenance burden perspective, but it is a super easy thing to make people's lives easier while we figure out the optimal solution. I hope to get the PR out to prod in the next day or two.

3) I could see sub-classing working well here, but also we could accomplish this with the many-to-many policy-to-project mapping, which would allow high overlap without forcing either to be a strict subset of the other.

4) I love this idea and had not thought of it before. Made a ticket in semgrep-app repository.

5) I also love this idea and made a ticket here: https://github.com/returntocorp/semgrep/issues/2339

6) I am bringing this up with other people to see if we can prioritize a re-design of the policy page soon

[dlukeomalley]

For ongoing work (new PRs) using Chef-01, I want more stringent checks--notice there are three less overrides there. But for a full scan of the existing code -- using Chef-nightly -- I do not want the three highlighted items to add noise to the semgrep output. Those items are "nice to have" for new work, but they are not really of any significance and do not need to be pointed out in the existing code base.

Because Semgrep.dev deduplicates results and will only email/Slack/alert on new findings, is it necessary to use different policies for your nightly and PR scans? The time where this would be problematic is the first nightly scan you do, in which case Semgrep.dev may believe all (or most) of the results are new since they haven't been registered in its database before.

Would like an optional comment/note/description for each rule override in a policy; as it is, I have to keep a separate note file to remember why I wanted to exclude certain rules.

A related request I've gotten is to provide a policy/project "audit log" where any changes to these are logged, with the author denoted, and a "commit" message provided. Do you think this captures or solves your use case as well?

(5) Speaking of why I wanted to exclude certain rules, there were a couple that do not quite make sense to me and a couple others that are not working correctly. Consider adding a way for interested users to upvote/downvote each rule in a ruleset so other users could get "crowdsourced opinion" on rules that are generally considered good or considered bad.

BIG +1! I see a world where there is both manual upvote/downvote feedback and also aggregated stats covering how often rules fire, how frequently they're fixed when they do, etc. Is the aggregate information something that would help you make decisions about the rules you use?

[msorens]

@dlukeomalley
(1) So if I understand you correctly, semgrep will still report the findings if I look in the log, but it will only emit notifications about them the first time. Then, yes, I think that would obviate the need for the shenanigans with my fictitious project.

(2) On the "audit log" suggestion, yes, that sounds like it would cover my use case sufficiently.

[dlukeomalley]

So if I understand you correctly, semgrep will still report the findings if I look in the log, but it will only emit notifications about them the first time. Then, yes, I think that would obviate the need for the shenanigans with my fictitious project.

That is my understanding from chatting with @nbrahms (CCed so he can correct me if I misunderstand).

@chmccreery @msorens - Given all of the other tickets we've filed based on this one, should we close this ticket?

[msorens]

Update: Working further on this--actually wiring up the nightly run, that is--I do still find a need for the fictitious project, because I really do not want the extra noise in the log/semgrep output. I want all of the reported issues there to be considered actionable. Right now, my nightly run is reporting 18 such findings. If I had used my "regular" policy, the one I use for differential scans on PRs, it would have reported many more that I do not feel warrant any attention (they are legacy ones that are fine to stay there) so it would be harmful for them to appear.

I dug further on the consequences of using a fictitious project; if I got it right, there is only one bad link -- please see the annotated illustrations in this PR.