Talk:GitLab/Workflows/Security patches

Hello,

reading through the guide at the talk page, I'm worried about having more and more security issues/patches that do not receive enough attention.

With the current Phabricator-based process, everyone with access to security issues can see pending patches and offer their help as-needed. If we move to the GitLab-based process, we'd be in a weird situation where I (a developer authorized to access security tasks) would be able to know the vulnerability (including any PoC submitted by the reporter), but I wouldn't be able to see if there is any patch pending, and if there is, help to review it. To me, that makes no sense – the fix doesn't provide any information that I wouldn't already have from the task itself. I often volunteer to help with various security tasks (and I've helped to review and/or deploy many of them), and I think this new process would make it significantly harder for me to continue doing so. Of course, the new process might say between-the-lines "people outside the SecTeam should not help with security tickets/patches", but I don't think that's a good move either.

Looking at the new process from not-so-active security patch authors (I also saw few of them), it will likely be tricky to guess who "might assist with code review". With regular patches, it doesn't matter very much – the patch is public, and others might easily add more reviewers to facilitate a fast review. With the security patches, that's not the case for obvious reasons (even if we only consider people who we trust to see the actual reports).

CC @SBassett (WMF), who I often interact with when helping in this area.

Hey @Martin Urbanec - AFAIK, Phabricator will still be our primary bug tracker, as we WILL NOT be enabling Gitlab's issue-tracking features, possibly indefinitely. So much of the security bug / review / deploy process would still happen within Phabricator. Users can still work on patches and post them to the relevant, protected Phabricator task. Where things will differ is that, we will then be able to further review patches within protected Gitlab forks of a repository and allow various CI to run against them prior to any deployment, the last piece being a big advantage of Gitlab over Gerrit/Jenkins, at least in my opinion. And it might make sense to eventually create some kind of useful Gitlab security patch bot that could provide useful reporting to the Phabricator task once a merge request was created at Gitlab. I imagine there will still be some workflow glitches to work through once we migrate to Gitlab, but overall I think the migration should still help foster collaboration on security issues.

Hello @SBassett (WMF), thanks for the reply. Does that mean patches will appear both in the private GitLab fork and in Phabricator? That'd resolve most of my concerns, but the steps under "For code which canonically exists within GitLab, please follow the steps below" do not sound like that's what should happen (I don't see any mention of Phabricator there).

@Martin Urbanec - So the documentation, as of right now, does point to the workflow of the patch being written and pushed up directly to Gitlab under a protected fork. I think that can certainly be modified to either allow for the posting of the patch to a protected Phab task first (not really disallowed even at this time) or to have a simple bot that shuffles Gitlab MR events back to Phab as comments, like gerritbot right now. Thoughts on those options? For the first one, we'd just need to clarify the documentation to state that patches can be posted to protected Phab tasks first, and after human CR, will then follow the MR workflow under a protected fork at Gitlab. The bot would obviously take a little more time to spec and implement.

Hi @SBassett (WMF), I think the first solution (Phab review first, then tests run via the MR and a deployer deploys) is more realistic (although I'm not sure if the MR would be an improvement in that case; do secpatches rountinely fail tests when you try to backport them?).

I foresee a bunch of possible issues with the bot approach:

• just as volunteers, the bot would also need access to the MRs; we can include it as a guidance in the documentation, but as I know people, it will be forgotten from time to time,
• the bot will naturally need to be in the security ACL.

Since Wikimedia Cloud is not an appropriate environment to put secrets in (I wouldn't dare to put anything relating to an active vulnerability there), the bot will likely need to run within production. I'm not sure if production-running bots will be liked by people managing the prod infra.

@Martin Urbanec - Ok, I've just published a new version of the doc, if you'd like to review it. I've tried to make the various steps a bit more clear and note the Phabricator-related steps first. I'll have to double-check a few assumptions at gitlab.wikimedia.org, but I believe this process should work for now. Re: the bot, yes, it would be a more challenging approach for sure, but I believe that it's a solvable problem that would likely add benefit to the current security issue workflow.

See also https://gitlab.com/gitlab-org/gitlab/-/issues/334157

Thanks for the heads up. We hadn't planned on using that feature right away, so this is a good issue to keep track of as we assess future workflow changes. I've subscribed to it (turned on notifications).

Yes, until/unless we enable issue-tracking within Gitlab (which may or may not ever happen AIUI), we cannot use confidential merge requests. Hence the somewhat clunky, less-than-ideal-but-still-workable process described within this article for manually creating and deploying security patches to Wikimedia production.