Proposal: Validate security merge requests before they're merged
Problem to solve:
During a Security Release, the security merge requests are reviewed from Application Security point of view in two different stages:
- During the code review: We have a step on the security merge request template that requires an Application Security engineer to review the security fix.
- On staging: Once the security fixes are deployed to staging, Application Security team validates each security vulnerability to ensure it's fixed.
This approach has been problematic so far:
- Security fixes are validated twice: Before the merge requests are merged and after they're deployed to staging.
- Non-critical security releases normally involve numerous security fixes, making the validation of all of them in staging time-consuming.
- Often, when a security fix is deployed to staging, we encounter that it does not fix the security vulnerability.
The main problem lies in the last point, once a merge request is merged into master
there's no straightforward way of reverting it without exposing the vulnerability, so we need to wait for another security fix to be created, approved, merged and deployed to staging to start the validation again and to continue with the Security Release process. Commonly this has added at least another business day to the security release process.
Having to wait for a new security fix produces undesirable consequences:
- Security release process is blocked - Until a new fix is created and deployed to staging, the Security Release process is halted.
- GitLab.com deployments are also paused - Auto-deploy tasks are paused while performing Security Releases. This implies that no deployments to GitLab.com are executed during this period.
- Waiting for a new security fix to be submitted is also bothersome: We depend on the availability of a Backend Engineer to produce the fix; given timezone differences this person might not be available, which delays the security release even more.
Proposal: Validate security merge requests before they're merged
I'd like to propose performing the validation of security merge requests before they're merged by:
- Triggering a Package and QA job inside security merge requests: This one executes end-to-end tests again the security merge request and builds and Omnibus package from the merge request changes. - https://gitlab.com/gitlab-org/security/gitlab/-/issues/67
- Using the Omnibus package to validate the security fix.
Workflow of merging a security merge request will change to:
- Backend Engineer creates a security merge request on GitLab Security.
- Backend Engineer asks an Application Security Engineer to validate the security fix.
- Application Security Engineer triggers a
package-and-qa
job - Application Security Engineer verifies if the security vulnerability has been fixed and approves if that's the case.
Security merge request is ready to be merged if it has been approved according to our approval guidelines and if it has been approved by an Application Security Engineer.
Workflow of performing the security release tasks will change to:
- Release Manager merges the security fixes.
- Release Manager deploys the security fixes to staging.
- If the deploy is successful and our QA tasks are green, we proceed to promote to Canary.
- The rest of the process stays as-is.
Benefits
Ensuring more reliable testing of the security fixes in an earlier stage brings notable benefits:
- The Security Release process is speed up, which has a direct impact on the GitLab Release Velocity.
- Help minimize the blocking nature of the Security Release process.
- Help prevent production incidents caused by security fixes deployed to GitLab.com
- GitLab.com receives security fixes at a faster pace.
Apart from the aforementioned gains, the stakeholders involved in the Security Release process are also positively impacted:
-
Release Manager:
- By simplifying the security release process we reduce the time Release Managers spent on such tasks.
-
Application Security Engineer
- Allow them to validate the security fixes at their own pace, instead of validating all of them at the same time in the last steps of the Security Release process.
- Eliminates the extra workload of verifying the security fixes twice: At the code review stage and at the staging stage.
-
Backend Engineer
- Removes the extra burden of creating a security fix with urgency, which can lead to not fully tested fixes
-
Quality Engineer
- The test coverage is expanded on security merge requests.
To do
%13.2
Requirements --
Allow package and QA to be executed on security merge requests - https://gitlab.com/gitlab-org/security/gitlab/-/issues/67. -
Test the execution of Package and QA builds on security merge requests - https://gitlab.com/gitlab-org/security/gitlab/-/merge_requests/643 -
Add a validation into our merge requests validator: A merge request is only valid and ready to be merged if it has been approved by an AppSec member. - https://gitlab.com/gitlab-org/release-tools/-/issues/457
After the requirements have been completed, proceed with the following steps
On GitLab and Omnibus GitLab
-
GitLab: Modify the security issue and merge requests templates to state the MR targeting master
needs approval from AppSec gitlab-org/gitlab!36076 (merged)
Update security release documentation
-
Update the Developer documentation - gitlab-org/release/docs!245 (merged) -
Update the Release Manager documentation - gitlab-org/release/docs!246 (merged) -
Update the Security Enginner documentation - gitlab-org/release/docs!244 (merged)
release-tools
:
Update -
Update the security_patch
template https://gitlab.com/gitlab-org/release-tools/-/blob/master/templates/security_patch.md.erb - gitlab-org/release-tools!1078 (merged)
Communicate the Security Release process change
-
Prepare an Announcements issue - #1006 (closed) -
Post the announcement on #releases
,#backend
,#development
,#quality
,#security
,#app-sec
-
Add an item on the Engineering Week in Review.