Page MenuHomePhabricator

Scap should be aware of security patches
Closed, ResolvedPublic

Description

It should not be possible to deploy or scap and remove security patches in the process. This happens from time to time though accidentally. Scap should know about security (or other) patches and ensure they're applied before sync'ing anything.

Revisions and Commits

rMSCA Scap
Restricted Differential Revision

Event Timeline

demon raised the priority of this task from to Needs Triage.
demon updated the task description. (Show Details)
demon added subscribers: demon, 20after4, hashar and 2 others.
thcipriani added a revision: Restricted Differential Revision.Dec 15 2015, 12:41 AM

git apply trials

D80 is straightforward. It attempts to dry-run revert the security patches on the branch and will error out whenever it can not (an indication the patch is not applied). Since the code is around, it is probably good enough for now.

Tyler and I had just had a discussion regarding using a git branch to hold the security patches. That would ease rebasing them by reusing the git merge assistance. But on second thought I am not sure how it is going to work with submodules. But maybe git submodule update --rebase would handle it.

Regardless, for a single repo we talked about two additional solutions:

git cherry

Checkout a new local branch against the tip of what is in Gerrit: security
Apply the patch files to it. That gives you a reference about what should be applied
Use git-cherry to verify that all patches in securityare effectively in the local wmf branch:

git cherry wmf security

By considering wmf to be the upstream, git cherry will report for any security patches that have been merged upstream. Ie they are going to be deployed.

But why bother with .patch files at this point...

hold security patches in local branch

Each repo has a security branch. When updating the repo one scap would attempt to rebase security on the new tip and complains asking for rebasing. That would make it easy to handle rebasing instead of playing with patch files.

* Merge sec patches in origin/wmf (HEAD)
|\
| * Sec 3 (security)
| * Sec 2
| * Sec 1
|/
* Tip of wmf (origin/wmf)
* rest of history

When branch is updated in Gerrit:

* New tip of wmf (origin/wmf)
| * Merge sec patches in origin/wmf (wmf)
| |\
| | * Sec 3 (security)
| | * Sec 2
| | * Sec 1
| |/
* Old tip of wmf (HEAD)
* rest of history

To reapply the patches one would git rebase --no-ff and ends up with:

* Merge sec patches in origin/wmf (HEAD)
|\
| * Sec 3 (security)
| * Sec 2
| * Sec 1
|/
* New tip of wmf (origin/wmf)
* Old tip of wmf
* rest of history

But again I have no idea how it would work with submodules.

An advantage is that when security patches are disclosed, rebase discard them for us magically.

A con is if the patch do not apply cleanly on both branches. We would either need different security branches or rely on recording the patch conflict resolution with git rerere, but that tool is definitely going to confuse everyone.

Conclusion

The git apply --check --reverse is straightforward and easy to understand. Bonus point D80 is already implementing it. A drawback is that on conflict that involves a bunch of manual operations and doesn't help removing patches that have been merged.

Using local branches / cherry should be fine. A risk is loosing the security branch and have to figure out how it is going to work with submodule

So D80 looks fine to me :-)

If you could find a way to record a patch has been published in Gerrit, it would be helpful to propose to get rid of the associated .patch file. That would ease tracking of patch files.