Block landing a security patch that doesn't have a sec-rating or is sec-high without sec-approval
Categories
(Conduit :: Lando, enhancement, P3)
Tracking
(Not tracked)
People
(Reporter: dmeehan, Unassigned)
Details
Current Behavior:
When landing a patch for a sec bug the following checkbox is displayed regardless of sec rating or sec approval:
Is a secure revision and should follow the Security Bug Approval Process.
Desired Behavior:
If a bug does not have a sec-rating (i.e. sec-critical, sec-high, sec-moderate, sec-low, sec-other) then the patch should be blocked from landing.
If a bug has a sec-high rating and the patch does not have sec-approval then the patch should be blocked from landing.
The following message could be displayed
Is a secure revision without a security rating or sec-high without security approval. See Security Bug Approval Process.
|
Reporter | |
Comment 1•2 years ago
|
||
Tom/Dan - could you take a look at the description in this enhancement request? If it aligns with your expectations
|
||
Comment 2•2 years ago
|
||
This is implied, but to be pedantic: If a bug with a security group applied to it does not have a sec-rating (i.e. sec-critical, sec-high, sec-moderate, sec-low, sec-other) then the patch should be blocked from landing.
But looks good! Unless Dan, should we have something for sec-moderates that's not a blocker suggesting they get sec-approval if they think it might be warranted?
|
||
Comment 3•2 years ago
|
||
Further adventures in pedantry 😀 :
- the specific check would be "group contains the string
core-security"; that is, a substring match - the "i.e." in "(i.e. sec-critical, sec-high, ..." should be "e.g.": those are examples, not a complete list. Check for "keyword starts with
sec-". "starts with" is necessary to ignore misused "wsec-" keywords (for "web" security bugs) which I've sometimes seen on client bugs. [avoiding this search issue is why client bugs use the more awkwardcsectype-for that use] - both sec-high and sec-critical require sec-approval. sec-critical is rare these days so it's easy to leave off as a short-hand, but if we're implementing it in code it's important not to miss those.
You're right that some sec-moderate bugs deserve caution in landing, but I'm not sure what we could say that could make a difference. People who are concerned might have already sought out advice (via Slack, for example). The rest aren't likely to switch to "concerned" by reading that they can ask for approval if they are what they aren't.
The following message could be displayed [if landing is blocked]
Is that in place of the current checkbox, or in addition to? I interpret this RFE as "remove the honor-system checkbox and enforce rules instead". Does that mean that the only time this message appears is if landing is blocked? In that case maybe we do need to ask something from sec-moderate people as Tom suggested. Would it work if there's a 3rd state? If a security-bug has a sec-moderate rating then use the current behavior of showing the checkbox step but not blocking the landing.
|
|
||
Comment 4•2 years ago
|
||
The query string for my first point is "core-security". No spaces around it despite how it looks with the markdown code styling. You won't match any bugs if you include the spaces. The groups we use are core-security, xxx-core-security (for various "xxx"), and core-security-release.
|
|
||
Comment 5•2 years ago
|
||
For the moderate bug idea we would not show the checkbox if the patch has sec-approval. If it doesn't the checkbox string could be something like
This security vulnerability has no more than moderate impact; it is not a high-impact bug downgraded to moderate because it affects a limited population.
Wording needs work, but I'll stop here in case you think this is a terrible concept.
|
|
Reporter | |
Comment 6•2 years ago
|
||
Is that in place of the current checkbox, or in addition to? I interpret this RFE as "remove the honor-system checkbox and enforce rules instead". Does that mean that the only time this message appears is if landing is blocked?
I suggest this change is instead of the current checkbox - "remove the honor-system checkbox and enforce rules instead" if you're ok with that (That was my understanding in the discussion we had around this - there are enough people with access to change the flag in Bugzilla to unblock if ever there was anything timely and warranted a deviation)
The current behavior should still exist for sec bugs with a rating and are not sec-high. The users would see the same checkbox as they currently do, anything below sec-high falls under the "honor system"
Though if that message needs a change after this proposal, then we should track that in a separate bug as a dependant on Bug 1848027.
In that case maybe we do need to ask something from sec-moderate people as Tom suggested. Would it work if there's a 3rd state? If a security-bug has a sec-moderate rating then use the current behavior of showing the checkbox step but not blocking the landing.
I'm not sure how complicated it is to add/maintain this for Lando. I suggest maybe once this enhancement request has been triaged and is reviewed by the devs, we could track anything needed for sec-moderate in a different bug?
|
|
||
Comment 7•2 years ago
|
||
I suggest this change is instead of the current checkbox - "remove the honor-system checkbox and enforce rules instead" if you're ok with that
Yes, no need to make people see it twice
Another edge case I thought of: we don't require sec-approval for sec-high+ bugs if it's fixing a regression in the currently nightly. This mostly comes up from fuzzer-found regressions, especially in the JS engine. Is that something Lando could check? The rule might be "if cf_status_firefox_nightly==affected and cf_status_firefox_beta==unaffected then sec-high bugs don't need sec-approval". the "beta" condition might be "unaffected or disabled". Un-set does not count! we need people to positively assert this has not escaped out of nightly.
Tom: please check me on the above: we include disabled as well, right? Also, if it's a recent regression, do we need to force a rating before letting them fix it? I think we could let the un-rated ones land in the interest of not slowing folks down.
|
|
||
Comment 8•2 years ago
|
||
Yeah, if a feature is disabled in Beta then we don't need to require sec-approval.
|
||
Comment 9•2 years ago
|
||
This request seems reasonable to me, and shouldn't be too difficult to implement. We added a module for Lando to query limited information about sec bugs while implementing the new uplift workflow. We would need to expand this to also return information about the sec-rating and then block secure revisions from landing an invalid sec-approval state as described above.
Marking as P3 for now and I'll bring this up in our team meeting next week.
Description
•