The Security-Team currently does not take any action on these reports, but they do provide a decent amount of spam to our security-admin-feed@lists email. At some point within the future, I would hope that our appsec pipeline ci includes would be able to address these issues and inform the relevant code maintainers.
Description
Description
Details
Details
Related Changes in Gerrit:
| Subject | Author | Repo | Branch | Lines +/- | |
|---|---|---|---|---|---|
| Remove php-security scanning | Hashar | integration/config | master | +0 -366 | |
| Remove security-related PHP dependency-checking jobs | SBassett | integration/config | master | +0 -145 |
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Open | None | T383067 EPIC: Migrate jobs from Zuulv2 to Zuulv3+ | |||
| Open | None | T383134 Delete unused Jenkins jobs | |||
| Resolved | hashar | T309655 Disable php-composer-security-docker emails to security-admin-feed@lists |
Event Timeline
Comment Actions
Change 801808 had a related patch set uploaded (by SBassett; author: SBassett):
[integration/config@master] Disable email reporting to security admin feed
Comment Actions
That's probably fine to do as opposed to what my patch is doing. I've never fully disabled a jjb job though, so I don't know what all is involved with that. Ideally, alerts like these would be going to various app maintainers to address, but for many of these, there aren't clear maintainers at this time. And the Security-Team isn't really staffed to manage security issues for numerous codebases.
Comment Actions
Getting rid of these seems like the wrong solution - if the security team doesn't want to receive these anymore, the alerts should go to the maintainers of projects (please sign me up for shellbox). It's possible we also need a little more configuration than our very naive job to allow disabling alerts we don't care about.
Comment Actions
That's fine, but yes, as it currently stands, the matrix job only allows for one email for all sub-jobs, so that would need to change. As for these jobs going away, I don't really have an opinion one way or the other, but I would note that the AppSec folks do check for vulnerable php dependencies during manual reviews and would like to include such checks as part of the AppSec pipeline we've been designing at gitlab.wikimedia.org. Neither of these would currently replace the likely need for these jobs today, but could at some point in the future, when (conceivably) most Wikimedia projects have migrated to Gitlab.
Comment Actions
If nobody watch those security notifications we should remove the jobs entirely.
That's probably fine to do as opposed to what my patch is doing. I've never fully disabled a jjb job though, so I don't know what all is involved with that.
There is a way to flag a job as disabled which will prevent it from running again, but in most case we remove the definition from the jjb file and delete the jobs on deployment :]
Ideally, alerts like these would be going to various app maintainers to address, but for many of these, there aren't clear maintainers at this time. And the Security-Team isn't really staffed to manage security issues for numerous codebases.
Identifying maintainers is always a struggle with old code. In a lot of cases the burden end up to fall on the good will of a few people which doesn't not work well indeed.
That's fine, but yes, as it currently stands, the matrix job only allows for one email for all sub-jobs, so that would need to change.
The default is indeed to send them to security-admin-feed. There is one job per group of repositories (such as eventmetrics + svgtranslate which are on GitHub) or wikimania-scholarships / iegreview /slimapp being for some reason all in the same group.
The groups can be split as needed if the repository are maintained by different persons, the default being to send to security-admin-feeds.
So I guess we could list the repositories identify their maintainers and add them as recipients instead of the catch all security team. If there is no maintainer I am inclined to drop the jenkins job / remove the repository from the scan. My reasoning is even if the dependency bump is probably easy to craft, it is unlikely anyone will be able to review and approve the code change since well there is no maintainer.
Comment Actions
Thanks @hashar. The affected repos/projects appear to be:
- mediawiki - no obvious, single maintainer for these
- mediawiki/core
- mediawiki/vendor
- shellbox - @Legoktm has already volunteered for this one
- mediawiki/libs/Shellbox
- various stand-alone php apps - no clear owner outside of a git shortlog -s -n
- wikimedia/wikimania-scholarships
- wikimedia/iegreview
- wikimedia/slimapp
- wikimedia/eventmetrics
- wikimedia/svgtranslate
- fundraising apps - can likely just go to fr-tech-failmail@wikimedia.org and remove security-admin-feed@lists
- wikimedia/fundraising/crm
- wikimedia/fundraising/SmashPig
- mediawiki/core
- mediawiki/extensions/DonationInterface
- wikimedia/fundraising/tools
So that just leaves two groups above - mediawiki and various stand-alone php apps - without clear maintainers, unless we use the often unreliable maintainers page (or similar), picking the top n number of contributors from git shortlog -s -n or what might be mentioned within the contributors section of a README file. I feel the best approach for now might be:
- Update the two groups that we can - shellbox and fundraising apps
- Disable the other two groups, for now: mediawiki and various stand-alone php apps unless someone speaks up as having a strong desire to become a maintainer (which could perhaps be done within a separate task directed at relevant Foundation teams and other contributors)
Comment Actions
Change 801808 abandoned by SBassett:
[integration/config@master] Remove security-related PHP dependency-checking jobs
Reason:
No forward momentum on this for now
Comment Actions
Declining for now. These emails go to a mostly-ignored Slack channel these days. This process eventually needs to be revisited within the context of a more comprehensive vulnerability management program, replete with a better understanding of code and risk ownership.
Comment Actions
I am reopening this task cause it as some valuable history.
Summary
Those jobs run a scan of the composer dependencies, which was initiative done I think from T180278. The job emits a notification that nobody reads and this task was filed to disable the notification. I argued at the time that instead they should be handled and some process held to address those dependencies issue and have the libraries upgraded. Then I guess LibUp magically does it for us as well.
What
As part of upgrading our CI system (Continuous-Integration-Infrastructure (Zuul upgrade) , I am interested in deleting the php-security jobs. They are Matrix jobs and I'd like to avoid having to migrate them if at all possible.
I am thus proposing to delete the jobs given nobody have a use for them (extra point for declining T180278).
Comment Actions
I support this plan. LibUp does handle some of these repos, I believe, but it can obviously fail for certain non-trivial upgrade attempts, when the tests do not pass. The repos mentioned in T309655#7995614 can likely be better-supported via the work happening in T371814, as we plan to introduce SCA tooling (osv-scanner et al) as a part of that stand-alone system.
Comment Actions
Change #1109036 had a related patch set uploaded (by Hashar; author: Hashar):
[integration/config@master] Remove php-security scanning
Comment Actions
Change #1109036 merged by jenkins-bot:
[integration/config@master] Remove php-security scanning
Comment Actions
Eventually I have found the first initiative dates from 2014: T74193: Have a check for reported security issues in dependencies.
I have deleted the docker image (docker-registry.wikimedia.org/releng/composer-security-check) and the five associated jobs:
- fundraising-php-security
- github-php-security
- mediawiki-php-security
- php-composer-security
- shellbox-php-security
Thank you @sbassett for the quick reply yesterday and my apologize for the back and forth given you suggested at some point to delete those jobs :)