Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add findsecbugs plugin to spotbugs #61

Open
wants to merge 2 commits into
base: master
from

Conversation

@StefanSpieker
Copy link

StefanSpieker commented Nov 17, 2019

I added find-sec-bugs plugin to spotbugs. I don't think that it should be merged right away, because there are a lot of findigs in Jenkins. This is here for discussion, if this is useful at all.

But I found for example:
jenkinsci/jenkins#4363

@oleg-nenashev oleg-nenashev requested review from jeffret-b and Wadeck Dec 10, 2019
@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Dec 10, 2019

@Wadeck @jeffret-b Do we want to enable it in all components? I see you submitted your PRs in Core and Remoting, so your feedback would be very useful

@oleg-nenashev oleg-nenashev changed the title added findsecbugs plugin to spotbugs Add findsecbugs plugin to spotbugs Dec 10, 2019
@jeffret-b

This comment has been minimized.

Copy link

jeffret-b commented Dec 10, 2019

I would like to get this change merged. I'm working to propose the equivalent for the plugin-pom.

The big question I see is how we get there. I'm going to start a discussion on the dev mailing list, to try and solicit input on how we proceed.

@jeffret-b

This comment has been minimized.

Copy link

jeffret-b commented Dec 10, 2019

I sent a message to the Jenkins Dev list to start getting feedback about turning on findsecbugs in the Jenkins developer ecosystem. See https://groups.google.com/forum/#!topic/jenkinsci-dev/exd3fc9NUAg

I think we should accept this PR and make findsecbugs a standard part of the build process for components using this parent pom. Developers updating these components will need to fix or suppress any findings when updating the pom before the next release.

When we merge this, we should remove the pieces about findsecbugs I'm currently adding directly into the Jenkins and Remoting poms.

(I'm not so sure about the equivalent change for the plugin parent pom. Does making it the default become too burdensome?)

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Jan 6, 2020

This is still blocked by the dev list discussion, right?

@jeffret-b

This comment has been minimized.

Copy link

jeffret-b commented Jan 6, 2020

There isn't really anything blocking in the dev list discussion. But, I don't think we want to proceed here until after we've got agreement to proceed on jenkinsci/jenkins#4381. We seem to be getting close there. I'd like to get @Wadeck to comment there again but James seems to be okay with where it's going.

Anybody have any idea what different projects would be impacted by this pom change?

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Jan 18, 2020

Anybody have any idea what different projects would be impacted by this pom change?

Well, there are dozens of repos depending on this POM. some of them will be likely full by false-positives. Personally I would be fine with merging if there was a way to opt-out from FindSecBugs via a Maven property

@jeffret-b

This comment has been minimized.

Copy link

jeffret-b commented Jan 20, 2020

I've been wondering about whether to pursue opt-in or opt-out. Given your comment, I propose this PR be adapted to have an opt-out via a maven property and then we proceed with merging it. I would merge after we get jenkinsci/jenkins#4381 in then I'll back out the pom modifications there.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Jan 21, 2020

opt-out is fine with me

@Wadeck
Wadeck approved these changes Feb 17, 2020
Copy link

Wadeck left a comment

jenkinsci/jenkins#4381 is now merged, let's move forward :)

Copy link
Member

oleg-nenashev left a comment

I am not against it, but FTR it is going to cause a huge number of warnings down the line.
Jenkins core has a high threshold, and there was still a lot of warnings. Imagine what's going to happen with modules on the standard medium level. Even if we enable it by default, I kindly ask for a documented way to opt-out

@jeffret-b

This comment has been minimized.

Copy link

jeffret-b commented Feb 17, 2020

I agree that we should get this moving forward and that we need a clear way of opting out.

@jeffret-b

This comment has been minimized.

Copy link

jeffret-b commented Feb 17, 2020

There's already an easy way of opting out. Just add this to a spotbugs excludes file, which is already well-supported in our environment:

<FindBugsFilter>
    <Match>
        <Bug category="SECURITY"/>
    </Match>
</FindBugsFilter>

What we're going to need is more communication, so that people understand what is going on, how to handle the findings, and how to opt out like this.

I've had a few more examples of adding this to plugins sitting around. I've been waiting around to make sure we can get some movement in Jenkins before pushing on those areas. I'll work on getting those proposed.

After I've got those up as some examples, I'd be happy to submit additional comments on the email lists, blog posts, or even some demos. Whatever would be helpful for getting the word out. Once we can disseminate that information, we should go ahead with this PR and see about an equivalent one in the plugin parent pom.

@MarkEWaite

This comment has been minimized.

Copy link
Contributor

MarkEWaite commented Feb 18, 2020

I consistently encounter spotbugs/spotbugs#527 when I attempt to enable findsecbugs on the platformlabeler-plugin. That bug makes it impossible to enable findsecbugs on platformlabeler-plugin.

So long as there is a clear way to opt out and the opt out technique will avoid spotbugs/spotbugs#527, then I'm fine with the change.

I fear that the "opt out" to avoid that bug will likely require that I insert the spotbugs plugin definition into the pom and then declare a configuration which does not include findsecbugs.

@jeffret-b

This comment has been minimized.

Copy link

jeffret-b commented Feb 18, 2020

@MarkEWaite, I don't see spotbugs issue 527 as being a significant problem. Sure it adds a few unnecessary lines to the build log, but it doesn't cause any real issues. As the maintainers mention there, they should "make sure we don't raise stupid warnings" but the stupid warnings don't really cause problems.

Take a look at jenkinsci/platformlabeler-plugin#165 for an example of how the plugin would behave. Of particular note, is that findsecbugs doesn't find anything in platformlabeler, but could potentially help avoid things that might creep in sometime in the future. That PR also demonstrates opting-out of some or all checks related to findsecbugs.

@MarkEWaite

This comment has been minimized.

Copy link
Contributor

MarkEWaite commented Feb 18, 2020

I don't intend to block this change. If others feel it will help the project and the community, I'm willing to adapt my use to it.

@jeffret-b

This comment has been minimized.

Copy link

jeffret-b commented Mar 25, 2020

It's time to get this one moving forward. The change looks fine. I've double-checked it locally and seen that it does what we want.

I have provided information about my experiences and observations, including recommendations for developers on the Jenkins blog Findsecbugs for Developers and Jenkins Online Meetup 2020 03 11 Developer Meetup find sec bugs for core and plugins.

Opt-out is the best way to go, for two reasons, 1) we really want people to adopt findsecbugs, and 2) given the way maven configuration works, opt-out is much simpler. I describe how to do it in the resources referenced above.

@jeffret-b jeffret-b requested review from oleg-nenashev and MarkEWaite Mar 25, 2020
@jvz
jvz approved these changes Mar 25, 2020
Copy link
Member

jvz left a comment

Would be a great addition.

Copy link
Contributor

MarkEWaite left a comment

No objections from me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.