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

Ehsan Akhgari
On 2017-03-13 9:32 AM, Boris Zbarsky wrote:

> 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...

I'm really struggling to imagine what kind of an actor we are trying to
protect against.  There is a cost to going through the pain of
compromising a trusted developer's account, authoring a patch without
their knowledge, getting it landed, correctly getting it approved for an
uplift and actually uplifted to our release branches and wait for the
vulnerability to get deployed and profit.  This puts a price tag on this
attack vector, in terms of financials, and logistics, and it risks
getting detected by breaching in to a machine used by a Mozilla developer.

What kind of an actor would choose to actually go through this, versus,
let's say, buying a Firefox 0-day from a vulnerability vendor (would
perhaps cost a bit more in financials but a lot less in logistics), or
just look through our oceans of unsafe C++ code fishing for fresh
0-days?  (Honest question, not just rhetorical.)

Also, in all honesty, I feel like our review process is pretty
ineffective against this attach vector at any rate.  I don't know about
other reviewers, but I'm going to admit that most of the time when I'm
reviewing code coming from trusted Mozilla colleagues who I have worked
with for many years, I implicitly trust the code as being non-malicious,
whereas when I review code coming from contributors who I don't
recognize, I usually try to treat the code as potentially containing
disastrous security sensitive mistakes.  :/
_______________________________________________
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

David Burns-9
In reply to this post by Mike Connor-4
As the manager of the sheriffs, I am in favour of this proposal.

The reasons why are as follow (and to note there are only 3 paid sheriffs
to try cover the world):

* A number of r+ with nits land up in the sheriffs queue for
checkin-needed. This then puts the onus on the sheriffs, not the reviewer
that the right thing has been done. The sheriffs do no have the context
knowledge of the patch, never mind the knowledge of the system being
changed.

* The throughput of patches into the trees is only going to increase. If
there are failures, and the sheriffs need to back out, this can be a long
process depending on the failure leading to pile ups of broken patches.

A number of people have complained that using autoland doesn't allow us to
fail forward on patches. While that is true, there is the ability to do
T-shaped try pushes to make sure that you at least compile on all
platforms. This can easily done from Mozreview (Note: I am not suggesting
we move to mozreview).

Regarding burden on reviewers, the comments in this thread just highlight
how broken our current process is by having to flag individual people for
reviews. This leads to the a handful of people doing 50%+ of reviews on the
code. We, at least visibly, don't do enough to encourage new committers
that would lighten the load to allow the re-review if there are nits. Also,
we need to do work to automate the removal of nits to limit the amount of
re-reviews that reviewer can do.

We should try mitigate the security problem and fix our nit problem instead
of bashing that we can't handle re-reviews because of nits.

David

On 9 March 2017 at 21:53, 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
>
> _______________________________________________
> firefox-dev mailing list
> [hidden email]
> https://mail.mozilla.org/listinfo/firefox-dev
>
>
_______________________________________________
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

Byron Jones-5
David Burns wrote:
> We should try mitigate the security problem and fix our nit problem
> instead of bashing that we can't handle re-reviews because of nits.
one way tooling could help here is to allow the reviewer to make minor
changes to the patch before it lands.
ie.  "r+, fix typo in comment before landing" would become "r+, i fixed
the comment typo"

--
glob — engineering productivity — moz://a

_______________________________________________
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

Steve Fink-4
In reply to this post by SmauG-2
Count me as another one skeptical about adding a review step. (I work
most closely with someone 8 hours off of my timezone; I'd bet there's a
correlation between opinions on final review vs timezone offsets.)

The most promising option in my mind would be to (1) improve tooling on
interdiffs, and then (2) automatically post an interdiff to the bug
*after* it lands. Then we could choose whether that final interdiff
requires review or whether it's just an FYI. It doesn't prevent someone
from sneaking in some huge security hole, but it makes it a lot more
visible and obvious when they do -- which provides some amount of
discouragement from it happening in the first place. For slightly more
visibility, you could post all of these ex post facto interdiffs to some
easily-skimmable place.

IIUC, we're trying to scale here. One problem with scaling is that there
are more cracks to sneak security flaws into. Another is scaling the
rate of changes landing in the tree. More process helps the first and
hurts the second, so we need to find a balance. I submit that ex post
facto interdiffs are a reasonable point of tradeoff.

Obviously, anything we could do to help *both* sides of the equation
would be great. That includes better interdiffs and the autoposted nit
interdiffs, both of which would be useful even without process changes.
(I'm guessing that scanning a stream of nit interdiffs would point
towards some tooling improvements that could eliminate the need for a
fair number of final touch-ups.)

If we *do* end up requiring a review on the final version, whether
retroactive or blocking, we could also reduce the friction slightly via
tooling that allows comment changes through. Maybe even variable renames
or formatting changes. But I doubt that changes the overall picture
significantly.

_______________________________________________
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
In reply to this post by Boris Zbarsky
On Mon, Mar 13, 2017 at 6:36 AM, Boris Zbarsky <[hidden email]> wrote:

> 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.
>

Actually, I wish I had written this differently. Say I get an r+ w/o nits,
I suspect
that the sheriffs will accept an updated patch (e.g., ostensibly with a
comment
fix) that is marked r=<foo>.


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.  ;)


Me too. And I think "trust" in this case at least arguably should be
defined as
"trusted by Mozilla" (e.g., L3 committer). So, one possibility would be
have a
policy like the following.

- Every CL must either be written by someone trusted OR r+ed by someone
trusted.
- If a patch is r+ with nits, then the final patch must be posted by someone
trusted.

This would ensure that every CL that lands was signed off on in its final
form
by a someone trusted.

Does this seem crazy?

-Ekr


>
>
> -Boris
>
>
> _______________________________________________
> 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

Bobby Holley-2
On Mon, Mar 13, 2017 at 10:33 AM, Eric Rescorla <[hidden email]> wrote:

> On Mon, Mar 13, 2017 at 6:36 AM, Boris Zbarsky <[hidden email]> wrote:
>
> > 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.
> >
>
> Actually, I wish I had written this differently. Say I get an r+ w/o nits,
> I suspect
> that the sheriffs will accept an updated patch (e.g., ostensibly with a
> comment
> fix) that is marked r=<foo>.
>
>
> 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.  ;)
>
>
> Me too. And I think "trust" in this case at least arguably should be
> defined as
> "trusted by Mozilla" (e.g., L3 committer). So, one possibility would be
> have a
> policy like the following.
>
> - Every CL must either be written by someone trusted OR r+ed by someone
> trusted.
> - If a patch is r+ with nits, then the final patch must be posted by
> someone
> trusted.
>
> This would ensure that every CL that lands was signed off on in its final
> form
> by a someone trusted.
>
> Does this seem crazy?
>

This seems like a very reasonable compromise to me. It gets the overhead
out of the way in the hot paths but still gives us some organization-wide
guarantees.


>
> -Ekr
>
>
> >
> >
> > -Boris
> >
> >
> > _______________________________________________
> > 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
>
_______________________________________________
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
On 2017-03-13 1:40 PM, Bobby Holley wrote:

> On Mon, Mar 13, 2017 at 10:33 AM, Eric Rescorla <[hidden email]> wrote:
>
>> On Mon, Mar 13, 2017 at 6:36 AM, Boris Zbarsky <[hidden email]> wrote:
>>
>>> 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.
>>>
>>
>> Actually, I wish I had written this differently. Say I get an r+ w/o nits,
>> I suspect
>> that the sheriffs will accept an updated patch (e.g., ostensibly with a
>> comment
>> fix) that is marked r=<foo>.
>>
>>
>> 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.  ;)
>>
>>
>> Me too. And I think "trust" in this case at least arguably should be
>> defined as
>> "trusted by Mozilla" (e.g., L3 committer). So, one possibility would be
>> have a
>> policy like the following.
>>
>> - Every CL must either be written by someone trusted OR r+ed by someone
>> trusted.
>> - If a patch is r+ with nits, then the final patch must be posted by
>> someone
>> trusted.
>>
>> This would ensure that every CL that lands was signed off on in its final
>> form
>> by a someone trusted.
>>
>> Does this seem crazy?
>>
>
> This seems like a very reasonable compromise to me. It gets the overhead
> out of the way in the hot paths but still gives us some organization-wide
> guarantees.

I still think this has mostly the same issues as has been raised in the
thread so far, even though at first glance it may seem a bit nicer than
the original proposal: for example, there is still the issue of whether
reviewers actually treat code coming from someone named Ehsan Akhgari as
potentially malicious code, what this means for rebases, who will be
responsible for the final sign off on the patch in the r+-with-nits
situation, etc.

Note that it seems now we have modified our goal slightly.  Originally
it seems we were trying to ensure we don't get incoming security bugs by
ensuring that any commit that ultimately gets checked in gets an r+.
Now we are talking about every commit needs to be r+ed or authored by
someone with L3 access.  Are we still protecting against incoming
security vulnerabilities?
_______________________________________________
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

Bobby Holley-2
On Mon, Mar 13, 2017 at 11:04 AM, Ehsan Akhgari <[hidden email]>
wrote:

> On 2017-03-13 1:40 PM, Bobby Holley wrote:
> > On Mon, Mar 13, 2017 at 10:33 AM, Eric Rescorla <[hidden email]> wrote:
> >
> >> On Mon, Mar 13, 2017 at 6:36 AM, Boris Zbarsky <[hidden email]>
> wrote:
> >>
> >>> 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.
> >>>
> >>
> >> Actually, I wish I had written this differently. Say I get an r+ w/o
> nits,
> >> I suspect
> >> that the sheriffs will accept an updated patch (e.g., ostensibly with a
> >> comment
> >> fix) that is marked r=<foo>.
> >>
> >>
> >> 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.  ;)
> >>
> >>
> >> Me too. And I think "trust" in this case at least arguably should be
> >> defined as
> >> "trusted by Mozilla" (e.g., L3 committer). So, one possibility would be
> >> have a
> >> policy like the following.
> >>
> >> - Every CL must either be written by someone trusted OR r+ed by someone
> >> trusted.
> >> - If a patch is r+ with nits, then the final patch must be posted by
> >> someone
> >> trusted.
> >>
> >> This would ensure that every CL that lands was signed off on in its
> final
> >> form
> >> by a someone trusted.
> >>
> >> Does this seem crazy?
> >>
> >
> > This seems like a very reasonable compromise to me. It gets the overhead
> > out of the way in the hot paths but still gives us some organization-wide
> > guarantees.
>
> I still think this has mostly the same issues as has been raised in the
> thread so far, even though at first glance it may seem a bit nicer than
> the original proposal:


Hm, maybe I'm missing something obvious, but I don't follow.


> for example, there is still the issue of whether
> reviewers actually treat code coming from someone named Ehsan Akhgari as
> potentially malicious code,


I assumed they wouldn't.


> what this means for rebases, who will be
> responsible for the final sign off on the patch in the r+-with-nits
> situation, etc.
>

The point is that r+-with-nits is only allowed in the case where the person
doing the nit fixup is trusted.


>
> Note that it seems now we have modified our goal slightly.  Originally
> it seems we were trying to ensure we don't get incoming security bugs by
> ensuring that any commit that ultimately gets checked in gets an r+.
> Now we are talking about every commit needs to be r+ed or authored by
> someone with L3 access.  Are we still protecting against incoming
> security vulnerabilities?
>
_______________________________________________
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 Eric Rescorla
This is something I've been thinking about a bunch over the weekend.  I
think trusted needs a smaller scope than "one of the hundreds of people
with L3 access."  As Bobby noted, the primary issue is the hot paths, so we
should look at how to explicitly define those paths.

While there's some fuzzy lines between modules (and the pan-tree rewrite
case needs a bit more discussion), I think we can quickly to get to a point
where module owners and peers are defined in-tree, in a machine-readable
format that we could use to a) validate that the reviewer(s) of a patch are
the appropriate people and b) suggest potential reviewers.  b) is
especially crucial in terms of better balancing review load, which is
something David Burns noted earlier.  I'm almost certain that we could do a
much better job of balancing review loads.

If we had that, we could reframe this idea as:

* every patch must be reviewed by an appropriate reviewer (verifiable)
* patch authors who are *also* owners/peers or otherwise marked as trusted
for the code in question (membership in SR group, or explicitly trusted by
the module owner) are permitted to carry over a review.
* carrying over a review requires the original patch be marked explicitly
(i.e. review+ and review-with-nits+)

This does require explicitly marking people as trusted, but I don't think
that's a huge burden, since that list of people doesn't change often.

I think this tightens things up, without creating a significant amount of
pain.

-- Mike

On Mon, Mar 13, 2017 at 1:33 PM, Eric Rescorla <[hidden email]> wrote:

> On Mon, Mar 13, 2017 at 6:36 AM, Boris Zbarsky <[hidden email]> wrote:
>
> > 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.
> >
>
> Actually, I wish I had written this differently. Say I get an r+ w/o nits,
> I suspect
> that the sheriffs will accept an updated patch (e.g., ostensibly with a
> comment
> fix) that is marked r=<foo>.
>
>
> 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.  ;)
>
>
> Me too. And I think "trust" in this case at least arguably should be
> defined as
> "trusted by Mozilla" (e.g., L3 committer). So, one possibility would be
> have a
> policy like the following.
>
> - Every CL must either be written by someone trusted OR r+ed by someone
> trusted.
> - If a patch is r+ with nits, then the final patch must be posted by
> someone
> trusted.
>
> This would ensure that every CL that lands was signed off on in its final
> form
> by a someone trusted.
>
> Does this seem crazy?
>
> -Ekr
>
>
> >
> >
> > -Boris
> >
> >
> > _______________________________________________
> > 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
>
_______________________________________________
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
In reply to this post by Ehsan Akhgari
On Mon, Mar 13, 2017 at 11:04 AM, Ehsan Akhgari <[hidden email]>
wrote:

> On 2017-03-13 1:40 PM, Bobby Holley wrote:
> > On Mon, Mar 13, 2017 at 10:33 AM, Eric Rescorla <[hidden email]> wrote:
> >
> >> On Mon, Mar 13, 2017 at 6:36 AM, Boris Zbarsky <[hidden email]>
> wrote:
> >>
> >>> 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.
> >>>
> >>
> >> Actually, I wish I had written this differently. Say I get an r+ w/o
> nits,
> >> I suspect
> >> that the sheriffs will accept an updated patch (e.g., ostensibly with a
> >> comment
> >> fix) that is marked r=<foo>.
> >>
> >>
> >> 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.  ;)
> >>
> >>
> >> Me too. And I think "trust" in this case at least arguably should be
> >> defined as
> >> "trusted by Mozilla" (e.g., L3 committer). So, one possibility would be
> >> have a
> >> policy like the following.
> >>
> >> - Every CL must either be written by someone trusted OR r+ed by someone
> >> trusted.
> >> - If a patch is r+ with nits, then the final patch must be posted by
> >> someone
> >> trusted.
> >>
> >> This would ensure that every CL that lands was signed off on in its
> final
> >> form
> >> by a someone trusted.
> >>
> >> Does this seem crazy?
> >>
> >
> > This seems like a very reasonable compromise to me. It gets the overhead
> > out of the way in the hot paths but still gives us some organization-wide
> > guarantees.
>
> I still think this has mostly the same issues as has been raised in the
> thread so far, even though at first glance it may seem a bit nicer than
> the original proposal: for example, there is still the issue of whether
> reviewers actually treat code coming from someone named Ehsan Akhgari as
> potentially malicious code


I was assuming that they did not.


, what this means for rebases, who will be
> responsible for the final sign off on the patch in the r+-with-nits
> situation, etc.
>

This would depend on the original patch author. Specifically:

- If the original patch author (or really the person posting the final
version)
  was trusted, then they would be able to (once they had r+) make final
  changes, rebase, etc.

- If the original patch author was untrusted, the they would need final
signoff
  on rebases, nits, etc. from someone trusted.


Note that it seems now we have modified our goal slightly.  Originally
> it seems we were trying to ensure we don't get incoming security bugs by
> ensuring that any commit that ultimately gets checked in gets an r+.
> Now we are talking about every commit needs to be r+ed or authored by
> someone with L3 access.  Are we still protecting against incoming
> security vulnerabilities?
>

That's certainly the intent of what I'm suggesting. Note that this is
intended
to enforce a weaker security condition than the one that Mike's proposal
claimed to be trying to enforce (specifically that compromise of a single
trusted person's credentials would not allow them to land malicious code).
However, as I noted originally, I do not believe that that proposal actually
achieved those objectives. The intent of this proposal is, rather, that at
least one trusted person must sign off on any code before it lands.

-Ekr
_______________________________________________
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

Nathan Froyd-5
In reply to this post by Mike Connor-4
On Mon, Mar 13, 2017 at 2:12 PM, Mike Connor <[hidden email]> wrote:
> While there's some fuzzy lines between modules (and the pan-tree rewrite
> case needs a bit more discussion), I think we can quickly to get to a point
> where module owners and peers are defined in-tree, in a machine-readable
> format that we could use to a) validate that the reviewer(s) of a patch are
> the appropriate people and b) suggest potential reviewers.

I think you're going to want to handle delegation of reviews, e.g.

1) I own/peer a module and I would like somebody to review my patches
so I can get a sense of how thoroughly they review code and/or to help
provide feedback on their review skills.

2) Similar to the above, maybe I get tagged for review, but I would
like somebody else who doesn't own/peer the module to review the code
because I judge them capable enough.  This and the above case are
mostly about growing new reviewers.

2a) Perhaps I would just like a second set of eyes on the patch (e.g.
threading issues) and am comfortable handing off review to somebody
who is more expert in such matters and whose judgement I trust to
handle any other issues capably.

3) Sometimes I get asked about reviewing particular things and I ask
the person who I know wrote the original code to review instead
because they are more familiar than I am.  Maybe this can be addressed
with ever-finer lists of code owners.

4) Would just like to reiterate that the rewriting code case needs
some thought, as somebody who has done such rewrites (and appreciated
getting a single review) and as somebody who has reviewed code for
such rewrites.

If we require reviewers to appear in a trusted-list in-tree, then
actually growing new reviewers is going to be a real pain.

> * patch authors who are *also* owners/peers or otherwise marked as trusted
> for the code in question (membership in SR group, or explicitly trusted by
> the module owner) are permitted to carry over a review.

This idea could also be expanded to handle the delegation cases above,
of course.

-Nathan
_______________________________________________
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 Eric Rescorla
On 2017-03-13 2:19 PM, Eric Rescorla wrote:

>
>
> On Mon, Mar 13, 2017 at 11:04 AM, Ehsan Akhgari <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     On 2017-03-13 1:40 PM, Bobby Holley wrote:
>     > On Mon, Mar 13, 2017 at 10:33 AM, Eric Rescorla <[hidden email]
>     <mailto:[hidden email]>> wrote:
>     >
>     >> On Mon, Mar 13, 2017 at 6:36 AM, Boris Zbarsky <[hidden email]
>     <mailto:[hidden email]>> wrote:
>     >>
>     >>> 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.
>     >>>
>     >>
>     >> Actually, I wish I had written this differently. Say I get an r+
>     w/o nits,
>     >> I suspect
>     >> that the sheriffs will accept an updated patch (e.g., ostensibly
>     with a
>     >> comment
>     >> fix) that is marked r=<foo>.
>     >>
>     >>
>     >> 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.  ;)
>     >>
>     >>
>     >> Me too. And I think "trust" in this case at least arguably should be
>     >> defined as
>     >> "trusted by Mozilla" (e.g., L3 committer). So, one possibility
>     would be
>     >> have a
>     >> policy like the following.
>     >>
>     >> - Every CL must either be written by someone trusted OR r+ed by
>     someone
>     >> trusted.
>     >> - If a patch is r+ with nits, then the final patch must be posted by
>     >> someone
>     >> trusted.
>     >>
>     >> This would ensure that every CL that lands was signed off on in
>     its final
>     >> form
>     >> by a someone trusted.
>     >>
>     >> Does this seem crazy?
>     >>
>     >
>     > This seems like a very reasonable compromise to me. It gets the
>     overhead
>     > out of the way in the hot paths but still gives us some
>     organization-wide
>     > guarantees.
>
>     I still think this has mostly the same issues as has been raised in the
>     thread so far, even though at first glance it may seem a bit nicer than
>     the original proposal: for example, there is still the issue of whether
>     reviewers actually treat code coming from someone named Ehsan Akhgari as
>     potentially malicious code
>
>
> I was assuming that they did not.

Did you see my response here?
<https://groups.google.com/d/msg/mozilla.dev.planning/nUP-j72I9bE/7OFmSMuqDgAJ>
 I don't understand how any of this is useful without this assumption.

>     , what this means for rebases, who will be
>     responsible for the final sign off on the patch in the r+-with-nits
>     situation, etc.
>
>
> This would depend on the original patch author. Specifically:
>
> - If the original patch author (or really the person posting the final
> version)
>   was trusted, then they would be able to (once they had r+) make final
>   changes, rebase, etc.
>
> - If the original patch author was untrusted, the they would need final
> signoff
>   on rebases, nits, etc. from someone trusted.

OK, to me, this essentially reads as if an L3 committer is malicious,
we're screwed, which is basically the status quo.  Am I understanding
the pre-and post- picture correctly under your suggestion?

(I agree on the other part of this proposal about *non-L3 committers*
being malicious, this proposal would inject a hook into the review
process where we can scan those commits for malicious code, but we would
need to do a good job somehow at highlighting people's commit access
rights in the UI of the tool we use to do code reviews so that the
reviewer can quickly assess whether they're supposed to treat the path
author as potentially malicious.)

>     Note that it seems now we have modified our goal slightly.  Originally
>     it seems we were trying to ensure we don't get incoming security bugs by
>     ensuring that any commit that ultimately gets checked in gets an r+.
>     Now we are talking about every commit needs to be r+ed or authored by
>     someone with L3 access.  Are we still protecting against incoming
>     security vulnerabilities?
>
>
> That's certainly the intent of what I'm suggesting. Note that this is
> intended
> to enforce a weaker security condition than the one that Mike's proposal
> claimed to be trying to enforce (specifically that compromise of a single
> trusted person's credentials would not allow them to land malicious code).
> However, as I noted originally, I do not believe that that proposal actually
> achieved those objectives.

Yes, this much we can agree on.  :-)

> The intent of this proposal is, rather, that at
> least one trusted person must sign off on any code before it lands.

I still don't think this proposal achieves any meaningful security
benefits where code is coming from L3 committers.  With this proposal,
we're as screwed in case an L3 committer starts to act maliciously as we
are today.

(FWIW, if I may interject some personal opinions here, if I were to redo
our commit access policy I would probably approach things from a
different angle.  I don't think there is anything we can do to remove
the requirement around trust in our L3 committers without basically
removing commit access and hence the need to trust humans which IMHO
would be the wrong end of the security/usability spectrum.  Instead I
would probably focus a bit on verification measures, for example around
whether the L3 committers actually keep acting on good faith and
investigations on our disclosed security vulnerabilities, if we aren't
already.)
_______________________________________________
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
On Mon, Mar 13, 2017 at 11:35 AM, Ehsan Akhgari <[hidden email]>
wrote:

> On 2017-03-13 2:19 PM, Eric Rescorla wrote:
> >
> >
> > On Mon, Mar 13, 2017 at 11:04 AM, Ehsan Akhgari <[hidden email]
> > <mailto:[hidden email]>> wrote:
> >
> >     On 2017-03-13 1:40 PM, Bobby Holley wrote:
> >     > On Mon, Mar 13, 2017 at 10:33 AM, Eric Rescorla <[hidden email]
> >     <mailto:[hidden email]>> wrote:
> >     >
> >     >> On Mon, Mar 13, 2017 at 6:36 AM, Boris Zbarsky <[hidden email]
> >     <mailto:[hidden email]>> wrote:
> >     >>
> >     >>> 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.
> >     >>>
> >     >>
> >     >> Actually, I wish I had written this differently. Say I get an r+
> >     w/o nits,
> >     >> I suspect
> >     >> that the sheriffs will accept an updated patch (e.g., ostensibly
> >     with a
> >     >> comment
> >     >> fix) that is marked r=<foo>.
> >     >>
> >     >>
> >     >> 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.  ;)
> >     >>
> >     >>
> >     >> Me too. And I think "trust" in this case at least arguably should
> be
> >     >> defined as
> >     >> "trusted by Mozilla" (e.g., L3 committer). So, one possibility
> >     would be
> >     >> have a
> >     >> policy like the following.
> >     >>
> >     >> - Every CL must either be written by someone trusted OR r+ed by
> >     someone
> >     >> trusted.
> >     >> - If a patch is r+ with nits, then the final patch must be posted
> by
> >     >> someone
> >     >> trusted.
> >     >>
> >     >> This would ensure that every CL that lands was signed off on in
> >     its final
> >     >> form
> >     >> by a someone trusted.
> >     >>
> >     >> Does this seem crazy?
> >     >>
> >     >
> >     > This seems like a very reasonable compromise to me. It gets the
> >     overhead
> >     > out of the way in the hot paths but still gives us some
> >     organization-wide
> >     > guarantees.
> >
> >     I still think this has mostly the same issues as has been raised in
> the
> >     thread so far, even though at first glance it may seem a bit nicer
> than
> >     the original proposal: for example, there is still the issue of
> whether
> >     reviewers actually treat code coming from someone named Ehsan
> Akhgari as
> >     potentially malicious code
> >
> >
> > I was assuming that they did not.
>
> Did you see my response here?
> <https://groups.google.com/d/msg/mozilla.dev.planning/nUP-
> j72I9bE/7OFmSMuqDgAJ>
>

Yes.


 I don't understand how any of this is useful without this assumption.


I'm not following. The reason that this is not necessary is because (by
hypothesis)
you are trusted, and this proposal is not intended to prevent against
attack by
trusted people. In order to do that, we would need to ensure signoff by
*two*
trusted people.


>     , what this means for rebases, who will be
> >     responsible for the final sign off on the patch in the r+-with-nits
> >     situation, etc.
> >
> >
> > This would depend on the original patch author. Specifically:
> >
> > - If the original patch author (or really the person posting the final
> > version)
> >   was trusted, then they would be able to (once they had r+) make final
> >   changes, rebase, etc.
> >
> > - If the original patch author was untrusted, the they would need final
> > signoff
> >   on rebases, nits, etc. from someone trusted.
>
> OK, to me, this essentially reads as if an L3 committer is malicious,
> we're screwed, which is basically the status quo.  Am I understanding
> the pre-and post- picture correctly under your suggestion?
>

Yes. As noted above, we would need to do something more aggressive
than mconnor is proposing to address this threat.


(I agree on the other part of this proposal about *non-L3 committers*
> being malicious, this proposal would inject a hook into the review
> process where we can scan those commits for malicious code, but we would
> need to do a good job somehow at highlighting people's commit access
> rights in the UI of the tool we use to do code reviews so that the
> reviewer can quickly assess whether they're supposed to treat the path
> author as potentially malicious.)
>

Yes.


> The intent of this proposal is, rather, that at
> > least one trusted person must sign off on any code before it lands.
>
> I still don't think this proposal achieves any meaningful security
> benefits where code is coming from L3 committers.  With this proposal,
> we're as screwed in case an L3 committer starts to act maliciously as we
> are today.
>

I agree that it does not. It merely attempts to ensure that the code comes
from
L3 committers.

-Ekr
_______________________________________________
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

Botond Ballo
In reply to this post by Daniel Veditz-2
On Sun, Mar 12, 2017 at 8:44 PM, Daniel Veditz <[hidden email]> wrote:
> 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 don't see how that's the case. To actually land something on
Autoland, you need to have L3 commit access. So, if this hypothetical
evil person does not themselves have L3 commit access, after getting
an "r+ with nits" and adding malicious code along with the nits, it's
still their reviewer who needs to submit to Autoland, and thus has the
opportunity to review their changes before submitting them.

Cheers,
Botond
_______________________________________________
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

Aki Sasaki-2
In reply to this post by Eric Rescorla
On Mon, Mar 13, 2017 at 11:41 AM, Eric Rescorla <[hidden email]> wrote:

> On Mon, Mar 13, 2017 at 11:35 AM, Ehsan Akhgari <[hidden email]>
> wrote:
>
> > On 2017-03-13 2:19 PM, Eric Rescorla wrote:
> > >
> > >
> > > On Mon, Mar 13, 2017 at 11:04 AM, Ehsan Akhgari <
> [hidden email]
> > > <mailto:[hidden email]>> wrote:
> > >
> > >     On 2017-03-13 1:40 PM, Bobby Holley wrote:
> > >     > On Mon, Mar 13, 2017 at 10:33 AM, Eric Rescorla <[hidden email]
> > >     <mailto:[hidden email]>> wrote:
> > >     >
> > >     >> On Mon, Mar 13, 2017 at 6:36 AM, Boris Zbarsky <
> [hidden email]
> > >     <mailto:[hidden email]>> wrote:
> > >     >>
> > >     >>> 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.
> > >     >>>
> > >     >>
> > >     >> Actually, I wish I had written this differently. Say I get an r+
> > >     w/o nits,
> > >     >> I suspect
> > >     >> that the sheriffs will accept an updated patch (e.g., ostensibly
> > >     with a
> > >     >> comment
> > >     >> fix) that is marked r=<foo>.
> > >     >>
> > >     >>
> > >     >> 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.  ;)
> > >     >>
> > >     >>
> > >     >> Me too. And I think "trust" in this case at least arguably
> should
> > be
> > >     >> defined as
> > >     >> "trusted by Mozilla" (e.g., L3 committer). So, one possibility
> > >     would be
> > >     >> have a
> > >     >> policy like the following.
> > >     >>
> > >     >> - Every CL must either be written by someone trusted OR r+ed by
> > >     someone
> > >     >> trusted.
> > >     >> - If a patch is r+ with nits, then the final patch must be
> posted
> > by
> > >     >> someone
> > >     >> trusted.
> > >     >>
> > >     >> This would ensure that every CL that lands was signed off on in
> > >     its final
> > >     >> form
> > >     >> by a someone trusted.
> > >     >>
> > >     >> Does this seem crazy?
> > >     >>
> > >     >
> > >     > This seems like a very reasonable compromise to me. It gets the
> > >     overhead
> > >     > out of the way in the hot paths but still gives us some
> > >     organization-wide
> > >     > guarantees.
> > >
> > >     I still think this has mostly the same issues as has been raised in
> > the
> > >     thread so far, even though at first glance it may seem a bit nicer
> > than
> > >     the original proposal: for example, there is still the issue of
> > whether
> > >     reviewers actually treat code coming from someone named Ehsan
> > Akhgari as
> > >     potentially malicious code
> > >
> > >
> > > I was assuming that they did not.
> >
> > Did you see my response here?
> > <https://groups.google.com/d/msg/mozilla.dev.planning/nUP-
> > j72I9bE/7OFmSMuqDgAJ>
> >
>
> Yes.
>
>
>  I don't understand how any of this is useful without this assumption.
>
>
> I'm not following. The reason that this is not necessary is because (by
> hypothesis)
> you are trusted, and this proposal is not intended to prevent against
> attack by
> trusted people. In order to do that, we would need to ensure signoff by
> *two*
> trusted people.
>
>
> >     , what this means for rebases, who will be
> > >     responsible for the final sign off on the patch in the r+-with-nits
> > >     situation, etc.
> > >
> > >
> > > This would depend on the original patch author. Specifically:
> > >
> > > - If the original patch author (or really the person posting the final
> > > version)
> > >   was trusted, then they would be able to (once they had r+) make final
> > >   changes, rebase, etc.
> > >
> > > - If the original patch author was untrusted, the they would need final
> > > signoff
> > >   on rebases, nits, etc. from someone trusted.
> >
> > OK, to me, this essentially reads as if an L3 committer is malicious,
> > we're screwed, which is basically the status quo.  Am I understanding
> > the pre-and post- picture correctly under your suggestion?
> >
>
> Yes. As noted above, we would need to do something more aggressive
> than mconnor is proposing to address this threat.
>


I think we're going the right direction in this thread: moving from less
secure to more secure.  But if the threat is compromised L3 creds, what
sort of solutions might help mitigate that?  (Assume one full set of
compromised L3 creds.  Prevent, or mitigate, shipping malicious code.)  Can
that solution be designed in such a way that it can coexist with at least
close-to-current levels of developer productivity?

I would guess addressing review and commit policy, as we are discussing in
this thread, would be at least part of that solution.  Requiring 2 L3
committers to sign off on certain types of commits may be a solution.
Pre-merge-day tree-review may be a solution that requires less day-to-day
overhead, but adds a significant task every N weeks, and doesn't protect
Nightly users.  A pre-Nightly branch (or set of branches) where changes can
accumulate until they are given another round of cumulative review may be a
solution.  Any solution that leverages automation to lessen manual overhead
would be worth investigating, as long as we have some trust in that
automation.

aki



>
>
> (I agree on the other part of this proposal about *non-L3 committers*
> > being malicious, this proposal would inject a hook into the review
> > process where we can scan those commits for malicious code, but we would
> > need to do a good job somehow at highlighting people's commit access
> > rights in the UI of the tool we use to do code reviews so that the
> > reviewer can quickly assess whether they're supposed to treat the path
> > author as potentially malicious.)
> >
>
> Yes.
>
>
> > The intent of this proposal is, rather, that at
> > > least one trusted person must sign off on any code before it lands.
> >
> > I still don't think this proposal achieves any meaningful security
> > benefits where code is coming from L3 committers.  With this proposal,
> > we're as screwed in case an L3 committer starts to act maliciously as we
> > are today.
> >
>
> I agree that it does not. It merely attempts to ensure that the code comes
> from
> L3 committers.
>
> -Ekr
> _______________________________________________
> 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

Boris Zbarsky
In reply to this post by Boris Zbarsky
On 3/13/17 1:33 PM, Eric Rescorla wrote:
> Actually, I wish I had written this differently. Say I get an r+ w/o nits,
> I suspect
> that the sheriffs will accept an updated patch (e.g., ostensibly with a
> comment fix) that is marked r=<foo>.

This seems entirely too plausible.  :(

> Me too. And I think "trust" in this case at least arguably should be
> defined as
> "trusted by Mozilla" (e.g., L3 committer). So, one possibility would be
> have a
> policy like the following.

This seems reasonable, if that's the goal.  But this is not the goal
mconnor had in his original post.  I'd love to get to the point where we
agree on the goals.

> - Every CL must either be written by someone trusted OR r+ed by someone
> trusted.
> - If a patch is r+ with nits, then the final patch must be posted by someone
> trusted.

This doesn't quite address your "r+ without nits, then the patch author
updates it anyway" scenario; presumably we would need something to
address that too.

-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

David Major
In reply to this post by Mike Connor-4
How do you plan to handle code in external libraries?

As long as anyone can submit a random patch to libwhatever and get a
free pass into Firefox the next time we pull an update, it's not clear
to me what the proposed restrictions are really buying us.

(And I don't think that requiring an L3 developer to review such merges
would be sufficient. Mozilla may lack the expertise on those libraries,
lack the time to review the details of large merges, or run into the
problems of retro-review that kats mentioned.)

On Tue, Mar 14, 2017, at 07:12 AM, Mike Connor wrote:

> This is something I've been thinking about a bunch over the weekend.  I
> think trusted needs a smaller scope than "one of the hundreds of people
> with L3 access."  As Bobby noted, the primary issue is the hot paths, so
> we
> should look at how to explicitly define those paths.
>
> While there's some fuzzy lines between modules (and the pan-tree rewrite
> case needs a bit more discussion), I think we can quickly to get to a
> point
> where module owners and peers are defined in-tree, in a machine-readable
> format that we could use to a) validate that the reviewer(s) of a patch
> are
> the appropriate people and b) suggest potential reviewers.  b) is
> especially crucial in terms of better balancing review load, which is
> something David Burns noted earlier.  I'm almost certain that we could do
> a
> much better job of balancing review loads.
>
> If we had that, we could reframe this idea as:
>
> * every patch must be reviewed by an appropriate reviewer (verifiable)
> * patch authors who are *also* owners/peers or otherwise marked as
> trusted
> for the code in question (membership in SR group, or explicitly trusted
> by
> the module owner) are permitted to carry over a review.
> * carrying over a review requires the original patch be marked explicitly
> (i.e. review+ and review-with-nits+)
>
> This does require explicitly marking people as trusted, but I don't think
> that's a huge burden, since that list of people doesn't change often.
>
> I think this tightens things up, without creating a significant amount of
> pain.
>
> -- Mike
>
> On Mon, Mar 13, 2017 at 1:33 PM, Eric Rescorla <[hidden email]> wrote:
>
> > On Mon, Mar 13, 2017 at 6:36 AM, Boris Zbarsky <[hidden email]> wrote:
> >
> > > 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.
> > >
> >
> > Actually, I wish I had written this differently. Say I get an r+ w/o nits,
> > I suspect
> > that the sheriffs will accept an updated patch (e.g., ostensibly with a
> > comment
> > fix) that is marked r=<foo>.
> >
> >
> > 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.  ;)
> >
> >
> > Me too. And I think "trust" in this case at least arguably should be
> > defined as
> > "trusted by Mozilla" (e.g., L3 committer). So, one possibility would be
> > have a
> > policy like the following.
> >
> > - Every CL must either be written by someone trusted OR r+ed by someone
> > trusted.
> > - If a patch is r+ with nits, then the final patch must be posted by
> > someone
> > trusted.
> >
> > This would ensure that every CL that lands was signed off on in its final
> > form
> > by a someone trusted.
> >
> > Does this seem crazy?
> >
> > -Ekr
> >
> >
> > >
> > >
> > > -Boris
> > >
> > >
> > > _______________________________________________
> > > 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
> >
> _______________________________________________
> 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

Boris Zbarsky
In reply to this post by Eric Rescorla
On 3/13/17 2:12 PM, Mike Connor wrote:
> If we had that, we could reframe this idea as:

I have a hard time evaluating this proposal without understanding the
threat model we are trying to address.  That is, what the actual "goal"
is.  Can you explain what that is?

-Boris

P.S. I can imagine various goals that might be addressed by this
proposal, most obviously detection of "unusual activity" a la credit
card fraud departments.  But I'm not sure whether those are the goals
we're setting out here, and I think it would be good to be _very_
explicit about our goals, so we can evaluate the specific proposals
against them.


_______________________________________________
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

Justin Dolske-2
In reply to this post by Trevor Saunders-2
On 3/12/17 5:44 PM, Daniel Veditz wrote:

> 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.

Along those lines, it seems worthwhile to note (and I haven't seen this
raised yet?) that at the core this is also a human-factors problem, and
those are notoriously difficult to fix with technology.

Specifically, here: if a reviewer has already decided that a patch is
"r+ with fixes", it's unlikely that the followup patch is going to get a
vigorous, detailed review. Especially if the process is perceived as
pointless overhead, causing delays, and 99.9999% of the time the patch
author is not trying to sneak in malicious code.

So a simple "every patch must be reviewed" requirement which doesn't
address that in some way isn't really going to change much from the
status quo -- you'd get "reviews", but only as a paperwork formality.

To improve that, we'd need to do things like asking what it would take
to have reviewers give "r+ with fixes" less often, understanding if the
risk can be mitigated (by more/less trust in some authors), etc.

> No easy answers.

Indeed.

Justin

_______________________________________________
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
In reply to this post by Boris Zbarsky
On Mon, Mar 13, 2017 at 12:30 PM, Boris Zbarsky <[hidden email]> wrote:

> On 3/13/17 1:33 PM, Eric Rescorla wrote:
>
>> Actually, I wish I had written this differently. Say I get an r+ w/o nits,
>> I suspect
>> that the sheriffs will accept an updated patch (e.g., ostensibly with a
>> comment fix) that is marked r=<foo>.
>>
>
> This seems entirely too plausible.  :(
>
> Me too. And I think "trust" in this case at least arguably should be
>> defined as
>> "trusted by Mozilla" (e.g., L3 committer). So, one possibility would be
>> have a
>> policy like the following.
>>
>
> This seems reasonable, if that's the goal.  But this is not the goal
> mconnor had in his original post.  I'd love to get to the point where we
> agree on the goals.


Me too. I have come to the conclusion that mconnor's goal cannot be achieved
without substantial disruption.


- Every CL must either be written by someone trusted OR r+ed by someone
>> trusted.
>> - If a patch is r+ with nits, then the final patch must be posted by
>> someone
>> trusted.
>>
>
> This doesn't quite address your "r+ without nits, then the patch author
> updates it anyway" scenario; presumably we would need something to address
> that too.


Sorry, I should have clarified this. In any case, the final (landed) patch
must be
either reviewed or posted by someone trusted. The nits thing is a side issue
(which you would think I would have realized from earlier in my own
message!)

-Ekr





>
>
> -Boris
> _______________________________________________
> 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
1234