1490796 - Improvements to lando support for sec bug landings

Status: NEW

(Conduit :: Lando, enhancement, P2)

Description

[:Gijs (he/him)]

Lando is getting support to land confidential patches (e.g. for sec bugs) very soon now (set to be available next week). Yay! No more manual landing on inbound!

Per talking with smacleod, though, from a "what happens to the thing you put in phab to put it into autoland" perspective, it's much the same as what happens for non-security bugs. I think we can do slightly better.

Based on https://wiki.mozilla.org/Security/Bug_Approval_Process , here are some suggestions:

- refuse to land if the bug has the `sec-high` or `sec-critical` keyword, or if no other sec-severity (sec-want, sec-audit, sec-low, sec-moderate) keyword is present UNLESS the `sec-approval` flag has been set on the mirrored bugzilla attachment.
--> alternatively, ensure that someone from the sec-approvers group has reviewed the commit, effectively giving approval that way. That's probably something you want to discuss with the people in that group - I know dveditz/abillings are, I dunno who else.

- scrub commit messages; As the doc points out, we want to avoid 0-day'ing ourselves, like by including a full description of what we're fixing in the thing we land. I'm wondering if we should have lando automatically (is it hard to have opt-in/opt-out for this on the lando page?) scrub the commit message down to "Bug N, r=foo,bar,baz" (and nothing else). Descriptions are useful when writing and reviewing the patch, and because phab now keeps the patch confidential, it seems useful to encourage people to submit to phab *with* the details, but to land *without*.

- display obvious warnings (that can be acknowledged and bypassed) if it's landing a confidential revision that includes any test changes. Again, see wikipage for why we generally don't land tests at the same time as the original patch.

Effectively, I'm hoping we can use lando to avoid some cases where people accidentally land things that should have had sec-approval first, or land them with stuff that shouldn't be there, like a detailed description of the security issue. These things happen today because most people don't work on sec bugs all the time so they sometimes don't know.

Comment 1

[Daniel Veditz [:dveditz]]

Sometimes a generic check-in comment is better than no comment. Could that be part of the requirement for sec-approval? I mean it currently is informally:

 sec-approval+ to check-in after <date closer to release>
 sec-approval+ to check-in without tests
 sec-approval+ but change the check-in comment

If we're using lando we'd have to withhold the sec-approval until after the required changes were made. This isn't different than the review situation for non-secure bugs, but we should formalize it in our process description.

Not sure what to do about test landings with lando. In a normal bug we'd check in without tests and set an "in-testsuite?" flag. We'd probably want to formally spin off a "land tests for bug <xxx>" security bug and put the tests there. On clarity grounds I'd prefer that to the in-testsuite flag anyway, but it is a bit of extra work.

Comment 2

[Tom Prince [:tomprince]]

Is there a need for a separate bug for tests, or just a separate patch on the same bug, that lands later?

Comment 3

[Steven MacLeod [:smacleod]]

Morphing this to be a meta bug tracking specific incremental improvements to the process including moving sec-approval into Phabricator itself. A plan has been formulated and work has started on the implementation. Dependent bugs to follow.