The future of commit access policy for core Firefox

classic Classic list List threaded Threaded
77 messages Options
1234
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

Boris Zbarsky
On 3/9/17 4:53 PM, Mike Connor wrote:
>    - Compromising a single individual's credentials must not be sufficient
>    to land malicious code into our products.

This is a good goal in general.  Devil is in the details, of course...

>    - Two-factor auth must be a requirement for all users approving or
>    pushing a change.

This seems reasonable if we're willing to help contributors with this
(e.g. hand out yubikeys as needed).

>    - The change that gets pushed must be the same change that was approved.

This sounds great at first glance, but I suspect is hellish in practice,
as others have pointed out in the thread.  Given the need to rebase,
this is completely impossible to achieve as stated.  We should figure
out what the _real_ goal is here.  It's possible it's actually more than
one goal.

>    - Broken commits must be rejected automatically as a part of the commit
>    process.

This looks good.

I think there are some critical goals missing.  Some of these may be
something we're assuming everyone shares as a goal, but I'd rather make
them explicit, because I think some of these are not in fact being
considered in the specific changes being proposed.  I think some of
these are more contentious than others....

- Pushing is possible.
- Pushing is possible for non-MoCo-employees.
- Pushing is not a serious time drain on the patch author.  The term
   "serious" needs definition.
- Pushing is not a serious time drain on the patch reviewer.  The
   term "serious" needs definition.
- There should be little incentive to do one push instead of
   multiple pushes for work that is conceptually separate.

I may be missing some, honestly.  I have tried to think about edge cases
here, but having a failure of imagination.

We should have a clear concept of workflows that we consider "in scope"
for this system in terms of evaluating the "not a time drain" goals.  I
have explicitly not put any specific workflows in my list of critical
goals, though I consider some workflows critical.  I mean workflows in
terms of how work is organized into commits/pushes/bugs, not workflows
like branch-based vs mq vs git vs hg vs whatever.  As a concrete
example, pushing multiple things that depend on each other in practice
but not conceptually should probably not be considered an edge case.
That is, I'm talking about changes to the same code (hence need to be
pushed in the right order) but belonging to separate bugs and needing to
be tracked separately and whatnot. [1]

Basically, the current set of goals is more or less all defined from a
security perspective, and seems to be treating usability as a non-goal.
I think approaching the problem from that perspective will create an
unusable system [2], and we need to have specific usability goals in
mind as we design this.  The above are my suggestions for high-level
usability goals, but I would like to see what suggestions others have.

I am explicitly not writing down specific proposals that I think would
help achieve the "not a serious time drain" goals, but I do have some
thoughts here and am happy to drill down into them if we agree on those
goals in general.

-Boris

[1] The current mozreview+autoland workflow falls down badly on this,
for example.  See https://bugzilla.mozilla.org/show_bug.cgi?id=1344432

[2] Citation needed, but I think this is a commonly understood thing
about software security.  Examples like encrypted email, our old sync
service, etc abound.  And those are examples where people _were_
considering usability!

_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

Steven MacLeod
I think "r+ with nits" would be much less of an issue if the
trust/permissions
model was less coarse, both in level of permission and breadth of code it
encompasses. It seems a little bit insane to me that a developer who
primarily
works on area X of the codebase and is given scm_level_3 due to trust in
that
area can then land to any other part of the tree.

Attempts to fix this for parts of mozilla-central have been implemented with
push hooks[1]. Having a generic way to assign this ownership/trust for all
parts
of the codebase (and actually making it enforceable, rather than just
checking
a user providable string in a commit message) would give automation the
information it needs to relax certain requirements in explicitly defined
cases.

For example, it probably makes sense to allow the owner of a particular file
to land there when given an r+ on a previous revision of the change,
especially
if that review came from another owner or peer of that file. If you'd like
to
keep an audit trail, automation could even send out an interdiff of what was
reviewed and what was landed, after the fact.

It seems to me like it's time that an actual machine readable code ownership
system is implemented (Like most large orgs have). Allowing some level of
trust
to keep people productive makes sense to me, but I don't think the current
scm_level system provides enough control to grant reasonable trust we can
enforce with automation.

As Gijs mentioned, you could also allow reviewers to just fix the nits
themselves. Have tools support the reviewer pushing up an alternative fix
(or
editing directly in a review interface) and suggesting it as a change which
the
author could accept and land after viewing the differences.


[1]
https://hg.mozilla.org/hgcustom/version-control-tools/file/tip/hghooks/mozhghooks/prevent_webidl_changes.py
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

L. David Baron
In reply to this post by Boris Zbarsky
On Friday 2017-03-10 11:05 -0500, Boris Zbarsky wrote:
> - Pushing is possible.
> - Pushing is possible for non-MoCo-employees.
> - Pushing is not a serious time drain on the patch author.  The term
>   "serious" needs definition.
> - Pushing is not a serious time drain on the patch reviewer.  The
>   term "serious" needs definition.
> - There should be little incentive to do one push instead of
>   multiple pushes for work that is conceptually separate.

I like the way you're looking at specific usability goals, although
the last one made me think that perhaps some of the goals could be
higher level, e.g.:

 - doesn't provide incentives that reduce our ability to get prompt
   feedback from nightly users (e.g., by delaying code landing)
 - doesn't provide incentives that reduce our ability to find
   regressions via bisection (e.g., by merging landings together)

-David

--
𝄞   L. David Baron                         http://dbaron.org/   𝄂
𝄢   Mozilla                          https://www.mozilla.org/   𝄂
             Before I built a wall I'd ask to know
             What I was walling in or walling out,
             And to whom I was like to give offense.
               - Robert Frost, Mending Wall (1914)

_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning

signature.asc (817 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

L. David Baron
In reply to this post by Steven MacLeod
On Friday 2017-03-10 11:33 -0500, Steven MacLeod wrote:
> It seems to me like it's time that an actual machine readable code ownership
> system is implemented (Like most large orgs have). Allowing some level of
> trust
> to keep people productive makes sense to me, but I don't think the current
> scm_level system provides enough control to grant reasonable trust we can
> enforce with automation.

There's a big tradeoff here, which is that we don't want to
discourage refactoring or cleanup changes that touch large parts of
the codebase.  Such changes are important for the long-term health
of the codebase.  To allow them, we've generally allowed the owners
of an API to review straightforward changes to users of that API
across all parts of the tree, because getting review from every
module touched is a significant amount of work and delay, and
strongly discourages making such refactoring / cleanup changes.

-David

--
𝄞   L. David Baron                         http://dbaron.org/   𝄂
𝄢   Mozilla                          https://www.mozilla.org/   𝄂
             Before I built a wall I'd ask to know
             What I was walling in or walling out,
             And to whom I was like to give offense.
               - Robert Frost, Mending Wall (1914)

_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning

signature.asc (817 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

Steven MacLeod
On Fri, Mar 10, 2017 at 11:58 AM, L. David Baron <[hidden email]> wrote:

> On Friday 2017-03-10 11:33 -0500, Steven MacLeod wrote:
> > It seems to me like it's time that an actual machine readable code
> ownership
> > system is implemented (Like most large orgs have). Allowing some level of
> > trust
> > to keep people productive makes sense to me, but I don't think the
> current
> > scm_level system provides enough control to grant reasonable trust we can
> > enforce with automation.
>
> There's a big tradeoff here, which is that we don't want to
> discourage refactoring or cleanup changes that touch large parts of
> the codebase.  Such changes are important for the long-term health
> of the codebase.  To allow them, we've generally allowed the owners
> of an API to review straightforward changes to users of that API
> across all parts of the tree, because getting review from every
> module touched is a significant amount of work and delay, and
> strongly discourages making such refactoring / cleanup changes.
>

In the model I'm proposing, I think there could still be a place for a very
small
number of very trusted developers who could approve changes repository wide.
Vast refactoring could rely on sign-off from one of these developers. I
assume
the number of changes actually requiring such sign-off would be low enough
that it wouldn't overburden this small group? Even if their review was just
a
rubber-stamp indicating they trust the owner of the API making the
refactoring
and the reviewer who doesn't have full access.
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

Boris Zbarsky
In reply to this post by Boris Zbarsky
On 3/10/17 11:54 AM, L. David Baron wrote:
>  - doesn't provide incentives that reduce our ability to get prompt
>    feedback from nightly users (e.g., by delaying code landing)
>  - doesn't provide incentives that reduce our ability to find
>    regressions via bisection (e.g., by merging landings together)

Yes, I like those very much.

-Boris

_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

Mike Connor-4
In reply to this post by L. David Baron
On Thu, Mar 9, 2017 at 5:14 PM, L. David Baron <[hidden email]> wrote:

> On Thursday 2017-03-09 16:53 -0500, Mike Connor wrote:
> > I've identified the following goals as critical for a responsible commit
> > access policy:
> ...
> >    - The change that gets pushed must be the same change that was
> approved.
>
> I'm curious what this goal means.  In particular, does it mean that
> you're trying to end "review+ if you make the following changes",
> and require that reviewers re-review the revisions no matter what?
>
> (If it does mean that, then that's a substantial increase on
> reviewer load; if it doesn't, then I'm curious what definition of
> "the same" you're using.)


Replying to this specific part, since there's a lot of expected and
understandable concerns here.

That's a potential end state, and one end of the spectrum of strictness of
code review/approval.  That spectrum ranges from "trust approved developers
to make good changes, don't do code review" to "every line of code that
lands should be explicitly reviewed first."  Leaving aside productivity
concerns for the moment, I think we would all agree that code review is a
critical part of our process, because developers are human and humans make
mistakes.  As a result, the closer we get to doing formal review on final
versions, instead of mostly-done versions, the better off we should be.
That will take time, and I think it's the right goal to work toward.

Big picture, I agree that doing this today presents a lot of challenges,
especially for reviewers currently carrying a huge load.  We don't *yet*
have great tooling in place to easily review the differences between
submissions (and ideally recognize when it's just
whitespace/syntax/comments typos getting fixed).  We have a lot of
reviewers barely keeping up with reviews (or worse, always falling
behind).  We don't have an easy enough process for running patches through
automated testing in advance of reviews.  And most difficult of all,
patches are frequently not in a "ready to land" state when submitted for
review, meaning a clean review is relatively rare.  I believe we can and
should work to address all of these issues, especially the last one.

To expand a bit on that last point: if we can trust core/known developers
to competently make necessary changes prior to checkin, we should also be
able to expect them to fix most/all of those issues *before* they submit
those patches for review.  My experience as both author and reviewer has
generally been that doing code review on something 90% finished takes far
more time than something that's been refined thoroughly prior to
submission.  I'd love to see us tighten up the expectations on patch
authors to only request review when they believe something is fully ready
to land.  (If you want early/high-level feedback, there's a flag for that.)

Assuming we can make significant progress on all of those challenges, and
the net reviewer overhead is about the same, are there other reasons we
wouldn't want to move to a much stricter review process?

-- Mike
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

Kartikaya Gupta-6
On Fri, Mar 10, 2017 at 12:49 PM, Mike Connor <[hidden email]> wrote:
> To expand a bit on that last point: if we can trust core/known developers
> to competently make necessary changes prior to checkin, we should also be
> able to expect them to fix most/all of those issues *before* they submit
> those patches for review.

Not necessarily, because oftentimes those issues are specific to the
reviewer's preferences and the area of code being touched, which the
patch author may not be totally familiar with.

> Assuming we can make significant progress on all of those challenges, and
> the net reviewer overhead is about the same, are there other reasons we
> wouldn't want to move to a much stricter review process?

I think the burden of proof is on anybody claiming they can do this
without increasing net reviewer overhead. Without actually showing
that can be done, this whole line of reasoning is kind of moot.

Cheers,
kats
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

Nicholas Nethercote
In reply to this post by Mike Connor-4
On Sat, Mar 11, 2017 at 4:49 AM, Mike Connor <[hidden email]> wrote:

>
> To expand a bit on that last point: if we can trust core/known developers
> to competently make necessary changes prior to checkin, we should also be
> able to expect them to fix most/all of those issues *before* they submit
> those patches for review.  My experience as both author and reviewer has
> generally been that doing code review on something 90% finished takes far
> more time than something that's been refined thoroughly prior to
> submission.  I'd love to see us tighten up the expectations on patch
> authors to only request review when they believe something is fully ready
> to land.  (If you want early/high-level feedback, there's a flag for that.)
>

I frequently makes minor changes to patches after review that haven't been
suggested by reviewers. Often after sleeping on it. These are probably more
often in comments than code. Maybe I'm unusual in this respect.

I'm surprised that Servo and Rust haven't come up yet, because they do
already have a model much like the one suggested -- landing approval must
come from an authorized reviewer.

In my experience it's not a disaster but it is a hassle at times and
motivates against suggesting (as a reviewer) or making (as an author) minor
changes -- it's easy to be lazy and say "close enough". And in the case
where minor changes are requested, the final approval often is a complete
rubber stamp -- I've had Servo patches authorized by people uninvolved with
the PR simply because they were in my timezone and so online at the time.

Nick
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

Cameron McCormack-4
In reply to this post by Mike Connor-4
On Sat, Mar 11, 2017, at 09:02 AM, Nicholas Nethercote wrote:
> I'm surprised that Servo and Rust haven't come up yet, because they do
> already have a model much like the one suggested -- landing approval must
> come from an authorized reviewer.

In Servo, reviewers sometimes do "r+ with changes" by saying "delegate+"
in the PR, which tells the landing bot that the PR submitter can mark
the patch as reviewed themselves (even after pushing some updates).
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

Ehsan Akhgari
In reply to this post by Nicholas Nethercote
On 2017-03-10 8:02 PM, Nicholas Nethercote wrote:

> On Sat, Mar 11, 2017 at 4:49 AM, Mike Connor <[hidden email]> wrote:
>
>>
>> To expand a bit on that last point: if we can trust core/known developers
>> to competently make necessary changes prior to checkin, we should also be
>> able to expect them to fix most/all of those issues *before* they submit
>> those patches for review.  My experience as both author and reviewer has
>> generally been that doing code review on something 90% finished takes far
>> more time than something that's been refined thoroughly prior to
>> submission.  I'd love to see us tighten up the expectations on patch
>> authors to only request review when they believe something is fully ready
>> to land.  (If you want early/high-level feedback, there's a flag for that.)
>>
>
> I frequently makes minor changes to patches after review that haven't been
> suggested by reviewers. Often after sleeping on it. These are probably more
> often in comments than code. Maybe I'm unusual in this respect.

This is part of my usual workflow as well.

> I'm surprised that Servo and Rust haven't come up yet, because they do
> already have a model much like the one suggested -- landing approval must
> come from an authorized reviewer.
>
> In my experience it's not a disaster but it is a hassle at times and
> motivates against suggesting (as a reviewer) or making (as an author) minor
> changes -- it's easy to be lazy and say "close enough". And in the case
> where minor changes are requested, the final approval often is a complete
> rubber stamp -- I've had Servo patches authorized by people uninvolved with
> the PR simply because they were in my timezone and so online at the time.

That is quite sad to hear...  As a reviewer, oftentimes the changes that
I ask for in my r+ with nits are changes I actually care about quite a
lot for various reasons depending on the context...  To me, r+ with nits
means I'm trusting the author to be able to make the changes without
oversight, not that the changes are unimportant.  Even if all such
changes are trivial nits, this is a good way to build up technical debt.
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

Ehsan Akhgari
In reply to this post by Cameron McCormack-4
On Fri, Mar 10, 2017 at 8:17 PM, Cameron McCormack <[hidden email]> wrote:

> On Sat, Mar 11, 2017, at 09:02 AM, Nicholas Nethercote wrote:
> > I'm surprised that Servo and Rust haven't come up yet, because they do
> > already have a model much like the one suggested -- landing approval must
> > come from an authorized reviewer.
>
> In Servo, reviewers sometimes do "r+ with changes" by saying "delegate+"
> in the PR, which tells the landing bot that the PR submitter can mark
> the patch as reviewed themselves (even after pushing some updates).
>

OK, something like this would address the issue of reviewers refraining
from asking for nits.  But the detail of the implementation matter here...

--
Ehsan
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

SmauG-2
In reply to this post by Mike Connor-4
On 03/10/2017 12:59 AM, Bobby Holley wrote:

> At a high level, I think the goals here are good.
>
> However, the tooling here needs to be top-notch for this to work, and the
> standard approach of flipping on an MVP and dealing with the rest in
> followup bugs isn't going to be acceptable for something so critical to our
> productivity as landing code. The only reason more developers aren't
> complaining about the MozReview+autoland workflow right now is that it's
> optional.
>
> The busiest and most-productive developers (ehsan, bz, dbaron, etc) tend
> not to pay attention to new workflows because they have too much else on
> their plate. The onus needs to be on the team deploying this to have the
> highest-volume committers using the new system happily and voluntarily
> before it becomes mandatory. That probably means that the team should not
> have any deadline-oriented incentives to ship it before it's ready.
>
> Also, getting rid of "r+ with comments" is a non-starter.

FWIW, with my reviewer hat on, I think that is not true, _assuming_ there is a reliable interdiff for patches.
And not only MozReview patches but for all the patches. (and MozReview interdiff isn't reliable, it has dataloss issues so it
doesn't really count there.).
I'd be ok to do a quick r+ if interdiff was working well.



-Olli

>
> bholley
>
>
> On Thu, Mar 9, 2017 at 1:53 PM, Mike Connor <[hidden email]> wrote:
>
>> (please direct followups to dev-planning, cross-posting to governance,
>> firefox-dev, dev-platform)
>>
>>
>> Nearly 19 years after the creation of the Mozilla Project, commit access
>> remains essentially the same as it has always been.  We've evolved the
>> vouching process a number of times, CVS has long since been replaced by
>> Mercurial & others, and we've taken some positive steps in terms of
>> securing the commit process.  And yet we've never touched the core idea of
>> granting developers direct commit access to our most important
>> repositories.  After a large number of discussions since taking ownership
>> over commit policy, I believe it is time for Mozilla to change that
>> practice.
>>
>> Before I get into the meat of the current proposal, I would like to outline
>> a set of key goals for any change we make.  These goals have been informed
>> by a set of stakeholders from across the project including the engineering,
>> security, release and QA teams.  It's inevitable that any significant
>> change will disrupt longstanding workflows.  As a result, it is critical
>> that we are all aligned on the goals of the change.
>>
>>
>> I've identified the following goals as critical for a responsible commit
>> access policy:
>>
>>
>>     - Compromising a single individual's credentials must not be sufficient
>>     to land malicious code into our products.
>>     - Two-factor auth must be a requirement for all users approving or
>>     pushing a change.
>>     - The change that gets pushed must be the same change that was approved.
>>     - Broken commits must be rejected automatically as a part of the commit
>>     process.
>>
>>
>> In order to achieve these goals, I propose that we commit to making the
>> following changes to all Firefox product repositories:
>>
>>
>>     - Direct commit access to repositories will be strictly limited to
>>     sheriffs and a subset of release engineering.
>>        - Any direct commits by these individuals will be limited to fixing
>>        bustage that automation misses and handling branch merges.
>>     - All other changes will go through an autoland-based workflow.
>>        - Developers commit to a staging repository, with scripting that
>>        connects the changeset to a Bugzilla attachment, and integrates
>> with review
>>        flags.
>>        - Reviewers and any other approvers interact with the changeset as
>>        today (including ReviewBoard if preferred), with Bugzilla flags as
>> the
>>        canonical source of truth.
>>        - Upon approval, the changeset will be pushed into autoland.
>>        - If the push is successful, the change is merged to mozilla-central,
>>        and the bug updated.
>>
>> I know this is a major change in practice from how we currently operate,
>> and my ask is that we work together to understand the impact and concerns.
>> If you find yourself disagreeing with the goals, let's have that discussion
>> instead of arguing about the solution.  If you agree with the goals, but
>> not the solution, I'd love to hear alternative ideas for how we can achieve
>> the outcomes outlined above.
>>
>> -- Mike
>> _______________________________________________
>> dev-planning mailing list
>> [hidden email]
>> https://lists.mozilla.org/listinfo/dev-planning
>>

_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

SmauG-2
On 03/11/2017 05:23 AM, smaug wrote:

> On 03/10/2017 12:59 AM, Bobby Holley wrote:
>> At a high level, I think the goals here are good.
>>
>> However, the tooling here needs to be top-notch for this to work, and the
>> standard approach of flipping on an MVP and dealing with the rest in
>> followup bugs isn't going to be acceptable for something so critical to our
>> productivity as landing code. The only reason more developers aren't
>> complaining about the MozReview+autoland workflow right now is that it's
>> optional.
>>
>> The busiest and most-productive developers (ehsan, bz, dbaron, etc) tend
>> not to pay attention to new workflows because they have too much else on
>> their plate. The onus needs to be on the team deploying this to have the
>> highest-volume committers using the new system happily and voluntarily
>> before it becomes mandatory. That probably means that the team should not
>> have any deadline-oriented incentives to ship it before it's ready.
>>
>> Also, getting rid of "r+ with comments" is a non-starter.
>
> FWIW, with my reviewer hat on, I think that is not true, _assuming_ there is a reliable interdiff for patches.
> And not only MozReview patches but for all the patches. (and MozReview interdiff isn't reliable, it has dataloss issues so it
> doesn't really count there.).
> I'd be ok to do a quick r+ if interdiff was working well.
>

But I could rephrase this differently. During the last couple of weeks I've been doing pretty much only reviews, again, so increasing reviewing time
significantly is a non-starter for me.  If we can find more reviewers, great, if not, can we please figure out processes which make reviewing less
time consuming. I guess I'd wish positive answer to both these questions. (a quick r+ doesn't affect here much _if_ interdiff was working.)

Though, I do honestly think that our reviewing process would feel quite a bit better if everyone would always prioritize reviews over pretty much
everything else (some deadlines would be valid exceptions of this rule, then review queue could stay closed).
If that was happening, review requests might spread more evenly.


-Olli

>
>
> -Olli
>
>>
>> bholley
>>
>>
>> On Thu, Mar 9, 2017 at 1:53 PM, Mike Connor <[hidden email]> wrote:
>>
>>> (please direct followups to dev-planning, cross-posting to governance,
>>> firefox-dev, dev-platform)
>>>
>>>
>>> Nearly 19 years after the creation of the Mozilla Project, commit access
>>> remains essentially the same as it has always been.  We've evolved the
>>> vouching process a number of times, CVS has long since been replaced by
>>> Mercurial & others, and we've taken some positive steps in terms of
>>> securing the commit process.  And yet we've never touched the core idea of
>>> granting developers direct commit access to our most important
>>> repositories.  After a large number of discussions since taking ownership
>>> over commit policy, I believe it is time for Mozilla to change that
>>> practice.
>>>
>>> Before I get into the meat of the current proposal, I would like to outline
>>> a set of key goals for any change we make.  These goals have been informed
>>> by a set of stakeholders from across the project including the engineering,
>>> security, release and QA teams.  It's inevitable that any significant
>>> change will disrupt longstanding workflows.  As a result, it is critical
>>> that we are all aligned on the goals of the change.
>>>
>>>
>>> I've identified the following goals as critical for a responsible commit
>>> access policy:
>>>
>>>
>>>     - Compromising a single individual's credentials must not be sufficient
>>>     to land malicious code into our products.
>>>     - Two-factor auth must be a requirement for all users approving or
>>>     pushing a change.
>>>     - The change that gets pushed must be the same change that was approved.
>>>     - Broken commits must be rejected automatically as a part of the commit
>>>     process.
>>>
>>>
>>> In order to achieve these goals, I propose that we commit to making the
>>> following changes to all Firefox product repositories:
>>>
>>>
>>>     - Direct commit access to repositories will be strictly limited to
>>>     sheriffs and a subset of release engineering.
>>>        - Any direct commits by these individuals will be limited to fixing
>>>        bustage that automation misses and handling branch merges.
>>>     - All other changes will go through an autoland-based workflow.
>>>        - Developers commit to a staging repository, with scripting that
>>>        connects the changeset to a Bugzilla attachment, and integrates
>>> with review
>>>        flags.
>>>        - Reviewers and any other approvers interact with the changeset as
>>>        today (including ReviewBoard if preferred), with Bugzilla flags as
>>> the
>>>        canonical source of truth.
>>>        - Upon approval, the changeset will be pushed into autoland.
>>>        - If the push is successful, the change is merged to mozilla-central,
>>>        and the bug updated.
>>>
>>> I know this is a major change in practice from how we currently operate,
>>> and my ask is that we work together to understand the impact and concerns.
>>> If you find yourself disagreeing with the goals, let's have that discussion
>>> instead of arguing about the solution.  If you agree with the goals, but
>>> not the solution, I'd love to hear alternative ideas for how we can achieve
>>> the outcomes outlined above.
>>>
>>> -- Mike
>>> _______________________________________________
>>> dev-planning mailing list
>>> [hidden email]
>>> https://lists.mozilla.org/listinfo/dev-planning
>>>
>

_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

Trevor Saunders-2
In reply to this post by Mike Connor-4
On Fri, Mar 10, 2017 at 12:49:50PM -0500, Mike Connor wrote:

> On Thu, Mar 9, 2017 at 5:14 PM, L. David Baron <[hidden email]> wrote:
>
> > On Thursday 2017-03-09 16:53 -0500, Mike Connor wrote:
> > > I've identified the following goals as critical for a responsible commit
> > > access policy:
> > ...
> > >    - The change that gets pushed must be the same change that was
> > approved.
> >
> > I'm curious what this goal means.  In particular, does it mean that
> > you're trying to end "review+ if you make the following changes",
> > and require that reviewers re-review the revisions no matter what?
> >
> > (If it does mean that, then that's a substantial increase on
> > reviewer load; if it doesn't, then I'm curious what definition of
> > "the same" you're using.)
>
>
> Replying to this specific part, since there's a lot of expected and
> understandable concerns here.
>
> That's a potential end state, and one end of the spectrum of strictness of
> code review/approval.  That spectrum ranges from "trust approved developers
> to make good changes, don't do code review" to "every line of code that
> lands should be explicitly reviewed first."  Leaving aside productivity
> concerns for the moment, I think we would all agree that code review is a
> critical part of our process, because developers are human and humans make
> mistakes.  As a result, the closer we get to doing formal review on final
> versions, instead of mostly-done versions, the better off we should be.
> That will take time, and I think it's the right goal to work toward.

If we pretend the goal has no downside then of course it seems like a
good idea.  However its really hard for me to imagine a world where its
worth the time to review patches that amount to

+  MOZ_ASSERT(x);

after I asked you to please assert x there.  The worst downsides would
seem to be forgeting to add the assert which often isn't that big a
deal, or you adding a wrong assert which will show up soon enough.

Even more extreme consider a patch like

-#include <IUnknown.h>
+#include <iunknown.h>

For that to have fixed things on a case sensitive fs it basically has to
have been the correct change.

Finally for
-class X : public Y
+class X final : public Y

which I don't have a way to screw up that a compiler will accept on the
top of my head.

So until we have a world where getting that reviewed takes less time than
dealing with the bug mail for it today I'm inclined to think r+ with
nits for the first and the standing rubber stamps for the latter two
make a lot of sense.

> Big picture, I agree that doing this today presents a lot of challenges,
> especially for reviewers currently carrying a huge load.  We don't *yet*
> have great tooling in place to easily review the differences between
> submissions (and ideally recognize when it's just
> whitespace/syntax/comments typos getting fixed).  We have a lot of
> reviewers barely keeping up with reviews (or worse, always falling
> behind).  We don't have an easy enough process for running patches through
> automated testing in advance of reviews.  And most difficult of all,
> patches are frequently not in a "ready to land" state when submitted for
> review, meaning a clean review is relatively rare.  I believe we can and
> should work to address all of these issues, especially the last one.

I share the experience of seeing plenty of patches that aren't perfectly
ready to land, but I think most of the reasons are pretty unavoidable.
I'd break them up into a couple groups.

First we have patches that are just baddly designed.    As people learn
the area they will get better at avoiding writing those patches, but it
seems likely there will always be some.

Then we have the patches that missed some corner case or race condition.
Idealy we might be able to catch all those with tests, but I'm kind of
skeptical especially when we look at the number of possibly busted tests
we find ourselves disabling.

The third group is patches missing things like asserts, maybe missing a
comment for something tricky, or maybe just a comment typo.  Any of
those might easily slip by the author, so I'd expect we'll keep seeing
them.

> To expand a bit on that last point: if we can trust core/known developers
> to competently make necessary changes prior to checkin, we should also be
> able to expect them to fix most/all of those issues *before* they submit
> those patches for review.  My experience as both author and reviewer has
> generally been that doing code review on something 90% finished takes far
> more time than something that's been refined thoroughly prior to
> submission.  I'd love to see us tighten up the expectations on patch
> authors to only request review when they believe something is fully ready
> to land.  (If you want early/high-level feedback, there's a flag for that.)

Given what I observed above I tend to come up with some different
conclusions.

First I don't think we have a particularly big problem in the people I
work with of people asking for review on particularly half done sloppy
patches.

Second while they could fix the issues before asking for review they very
often reasonably don't know of them.  So the choice is between making
them aware and then rereviewing, or trusting them to get it right /
being ok with them messing it up once in a while.

So to the degreee I review patches that aren't ready to land the
problems in them are not the major time sinks in reviewing the patch.

> Assuming we can make significant progress on all of those challenges, and
> the net reviewer overhead is about the same, are there other reasons we
> wouldn't want to move to a much stricter review process?

I think there are some really big assumptions in there, but its hard to
disagree with wanting to print free money.

Trev

>
> -- Mike
> _______________________________________________
> dev-planning mailing list
> [hidden email]
> https://lists.mozilla.org/listinfo/dev-planning
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

Daniel Veditz-2
On 3/12/17 6:54 AM, Trevor Saunders wrote:
> If we pretend the goal has no downside then of course it seems like a
> good idea.  However its really hard for me to imagine a world where its
> worth the time to review patches that amount to
>
> +  MOZ_ASSERT(x);
>
> after I asked you to please assert x there.  The worst downsides would
> seem to be forgeting to add the assert which often isn't that big a
> deal, or you adding a wrong assert which will show up soon enough.

That assumes the developers you review have good intentions--which is
almost always true. Like most security-inspired restrictions, however,
the worry is about someone with bad intentions. It's similar to
insurance in that you pay every year but really you hope that your house
does NOT burn down, but in that case it feels like a waste.

Hypothetically, an evil person could take a "r+ with nits" and then
check in the patch with bad code that was never seen by another human
and which may not get noticed. When we put autoland into the mix this
essentially gives L3 commit access to anyone who can put together a
helpful "almost right" patch. I think there's not a lot of controversy
about imposing this restriction for patch submitters who are not yet
trusted.

This does add overhead when applied to developers who are, in fact,
trusted. On the surface, clearly noted in this thread, that seems
insane. But at the top of this thread, the worry was "what if a trusted
committer's credentials get compromised?" Currently the answer is "We're
screwed". We can hope the sheriffs would notice an odd patch during
uplift, but the sheriffs are busy and any attacker who was any good
would make sure the patch looked totally normal on the surface.

No easy answers.

-Dan Veditz
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

Eric Rescorla
Top-posting because I'm not really responding to Dan.

I'd like to suggest we take a step back here from the technical mechanisms
and ask
a simpler policy question, namely:

"What is the minimum number of 'trusted' people that should be required to
land
a patch"?

Currently, I suspect the answer is effectively 0: a random person can post
a patch to
Bugzilla, mark it "r=<committer>" (with some claim that it was in IRC) and
then mark
it checkin-needed and (I suspect, but perhaps the Sheriffs will correct me)
it will get
landed. Alternately, you can create a patch which gets r+ with nits, and
then update
with some malicious code and  r=<committer>.

So, I would ask: do people believe that this is an acceptable state of
affairs or
should the minimum number of 'trusted' people required to land a patch be 1
or
more?

-Ekr









On Sun, Mar 12, 2017 at 5:44 PM, Daniel Veditz <[hidden email]> wrote:

> On 3/12/17 6:54 AM, Trevor Saunders wrote:
> > If we pretend the goal has no downside then of course it seems like a
> > good idea.  However its really hard for me to imagine a world where its
> > worth the time to review patches that amount to
> >
> > +  MOZ_ASSERT(x);
> >
> > after I asked you to please assert x there.  The worst downsides would
> > seem to be forgeting to add the assert which often isn't that big a
> > deal, or you adding a wrong assert which will show up soon enough.
>
> That assumes the developers you review have good intentions--which is
> almost always true. Like most security-inspired restrictions, however,
> the worry is about someone with bad intentions. It's similar to
> insurance in that you pay every year but really you hope that your house
> does NOT burn down, but in that case it feels like a waste.
>
> Hypothetically, an evil person could take a "r+ with nits" and then
> check in the patch with bad code that was never seen by another human
> and which may not get noticed. When we put autoland into the mix this
> essentially gives L3 commit access to anyone who can put together a
> helpful "almost right" patch. I think there's not a lot of controversy
> about imposing this restriction for patch submitters who are not yet
> trusted.
>
> This does add overhead when applied to developers who are, in fact,
> trusted. On the surface, clearly noted in this thread, that seems
> insane. But at the top of this thread, the worry was "what if a trusted
> committer's credentials get compromised?" Currently the answer is "We're
> screwed". We can hope the sheriffs would notice an odd patch during
> uplift, but the sheriffs are busy and any attacker who was any good
> would make sure the patch looked totally normal on the surface.
>
> No easy answers.
>
> -Dan Veditz
> _______________________________________________
> dev-planning mailing list
> [hidden email]
> https://lists.mozilla.org/listinfo/dev-planning
>
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

Kartikaya Gupta-6
Here's a thought to potentially mitigate some of the delays associated with
re-reviewing for nits and rebases: post-commit review. That is, person A
authors the patch, puts it up for review as usual, person R reviews, says
"r+ with nits fixed". Person A can fix nits/rebase/etc and land right away,
but the patch then still requires the re-review by R after landing.
Obviously in the case where no changes are required, we can skip the
post-commit review.

This doesn't fix the total reviewer time spent, but it does fix the extra
(potentially multiple rebase) timezone/round-trip latency for person A.
That being said, it would require tooling support to make sure that reviews
don't get lost and buy-in from people to actually do the post-commit
reviews. In my experience with the QuantumRender work where we tried this,
a number of patches got left behind and without somebody following up on
them they would have been missed. So I'm not sure this option is actually a
good one, but I'll throw it in this thread as something to consider.

Another option is that person R implements the nit fixes and does the
landing instead of bouncing it back to A. This has obvious problems as
well, but again, throwing more ideas out there for the sake of discussion.
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

Boris Zbarsky
In reply to this post by Trevor Saunders-2
On 3/12/17 8:44 PM, Daniel Veditz wrote:
> This does add overhead when applied to developers who are, in fact,
> trusted. On the surface, clearly noted in this thread, that seems
> insane. But at the top of this thread, the worry was "what if a trusted
> committer's credentials get compromised?" Currently the answer is "We're
> screwed". We can hope the sheriffs would notice an odd patch during
> uplift, but the sheriffs are busy and any attacker who was any good
> would make sure the patch looked totally normal on the surface.

I feel like there's an important point to be made here.  If I had
compromised a developer's credentials and were trying to get an attack
past a "review right before landing" system, I would do it as follows:

1)  In one patch, introduce some code that relies on an invariant that
currently holds.

2)  In another, non-conflicting, patch, introduce a violation of that
invariant and fix up the other places depending on it, but not the one
from item 1.

3)  Have different people review the two patches.

4)  Make sure I have both reviews before landing either patch, so the
reviewers can't catch the problem.  There are various ways of doing
this, like making both patches depend on yet at third patch that I make
sure gets reviewed slowly.

This is a somewhat higher bar than just "add the malicious code and land
it", of course.  But this is also what I came up with after about 30
seconds' worth of thought...

-Boris
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: The future of commit access policy for core Firefox

Boris Zbarsky
In reply to this post by Daniel Veditz-2
On 3/12/17 9:55 PM, Eric Rescorla wrote:
> Alternately, you can create a patch which gets r+ with nits, and
> then update with some malicious code and  r=<committer>.

Speaking as a reviewer, for people I don't trust I pretty much never
give "r+ with nits", because I don't trust them to address the nits
correctly.

Even in cases when I do (e.g. if it's a typo in a comment), I do exactly
the "last review", sometimes post-landing, that we seem to be talking about.

I certainly have no problem formalizing this for the untrusted patch
author case.

Addressing the "compromised trusted account" case sanely is much harder,
not least because the number of patches involved is suddenly much
larger.  :(

> So, I would ask: do people believe that this is an acceptable state of
> affairs or should the minimum number of 'trusted' people required to land a patch be 1
> or more?

Obviously the latter.  ;)

-Boris

_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
1234