ubuntu-appstore-developers team mailing list archive

[Ricardo Kirkner <ricardo.kirkner@xxxxxxxxxxxxx>]

Jamie,


On Wed, Sep 17, 2014 at 11:31 AM, Jamie Strandboge <jamie@xxxxxxxxxxxxx>
wrote:

> On 09/17/2014 09:17 AM, Ricardo Kirkner wrote:
> >
> >
> > On Wed, Sep 17, 2014 at 10:44 AM, Jamie Strandboge <jamie@xxxxxxxxxxxxx
> > <mailto:jamie@xxxxxxxxxxxxx>> wrote:
> >
> >     On 08/29/2014 09:11 AM, Daniel Holbach wrote:
> >     > Hello everybody,
> >     >
> >     > it'd make Alan and my lives easier (until there's automatic
> >     > reviews/rejections/etc.), if we could somehow track current
> overrides.
> >     > Currently there's mainly the core apps and things such as the PayUI
> >     > which override some of the errors and warnings which would normally
> >     > block apps from going through.
> >     >
> >     > Is there a way you would like something like this to be
> implemented?
> >     >
> >     > My idea would have been to change the output of click-review to
> simply show:
> >     >
> >     >  - Known overrides
> >     >  - new errors/warnings
> >     >  - general info messages
> >     >
> >     > If there are no new errors or warnings, it would simply say "pass"
> and
> >     > let us approve the app.
> >     >
> >     > To track known overrides, we could maybe just store a piece of
> json like
> >     > this somewhere:
> >     >
> >     > {
> >     >   "com.canonical.payui":
> >     >   {
> >     >     "security_template_valid (payui.json)":
> >     >       "(MANUAL REVIEW) 'unconfined' not allowed",
> >     >     "lint_hooks_redflag_payui":
> >     >       "(MANUAL REVIEW) 'pay-ui' not allowed"
> >     >   }
> >     > }
> >     >
> >     > What do you think?
> >     >
> >
> >     Sorry for not responding earlier-- as you'll see, I have a lot to
> say on the
> >     manner. :)
> >
> >     In general, I am not opposed to something like this, but we need to
> be very
> >     careful. What I am most concerned about is the malicious developer
> that games
> >     the system. Eg, the developer uploads an otherwise legitimate app
> that is
> >     reviewed, requires a manual review (eg unconfined, reserved policy
> groups, etc)
> >     and it gets the manual review. We update the json to remember for
> next time, and
> >     then the next upload, the upload contains malicious code that takes
> advantage of
> >     the overrides and the app sails through. Another example is my app
> permy--
> >     currently it uses "read_path" in its security policy but
> (intentionally) does
> >     not include the "networking" policy group. If permy were to be
> overridden, we
> >     would want to prompt for manual review if the networking policy
> group is added
> >     in the future.
> >
> >     In addition to that, we need to make sure the overrides are
> maintained. Eg, if
> >     an app uses a reserved policy group, has an override added, then
> drops the
> >     policy group, the override should be removed such that if the app
> starts to use
> >     the policy group again, it triggers a manual review (policy group is
> but one
> >     example).
> >
> >     To spark the conversation, I'll toss out some ideas that might be
> worth
> >     considering:
> >      - including the reviewer who added the override to the json
> >      - including the date the override was added
> >      - including a note justifying the override
> >      - including an optional vendor tag
> >      - including an optional requires_new_review tag
> >      - only allow 'pass'es on apps with only warning for overrides
> >      - require manual review for errors, but display the json data to the
> >        reviewer
> >      - new errors and warnings should always prompt a new review (eg,
> the override
> >        can't mask new warnings)
> >      - if there is a security policy override and if security policy
> changes in any
> >        way (including policy_version), prompt for a new review
> >
> >     The idea behind the vendor tag is that we can perhaps do different
> things
> >     depending on the vendor or if it is omitted. Eg, perhaps if the
> vendor is:
> >      - "Canonical" (ie, official Canonical apps), both errors and
> warnings get a
> >        pass
> >      - "BQ" (ie, official apps from a trusted vendor), both errors and
> warning get
> >        a pass
> >      - "Ubuntu" (eg, not really official Canonical apps, but not apps
> from
> >        developers either), warnings get a pass, errors are displayed
> >      - omitted (ie, normal app without special consideration), warnings
> get a pass,
> >        errors are displayed
> >
> >     The idea behind the 'requires_new_review' tag is similar, we can do
> different
> >     things depending on the value. If it is present and true, we will
> always trigger
> >     for manual review. The idea here is that we can add the override
> data so it can
> >     be shown to the reviewer, but it will never get an automatic pass
> for what is
> >     being overridden. This might be useful for apps that need a full
> code review to
> >     verify if their use of a particular policy group is safe.
> >
> >     Process wise, I think it is important that overrides are the
> exception and their
> >     justification must be sound. We should have guidelines for adding
> overrides (eg,
> >     "apps with Canonical in the vendor field of the override should use
> the
> >     'com.canonical' namespace") and ee should not add overrides for
> things just to
> >     push them through. Furthermore, addition of overrides must be peer
> reviewed.
> >     Using a bzr branch seems an easy way to achieve this (it can include
> the
> >     guidelines as well).
> >
> >     Possible json using examples from above:
> >     {
> >       "com.canonical.payui":
> >       {
> >         "reviewer": "dholbach",
> >         "date": "2014-09-07",
> >         "note": "Canonical supported. unconfined ok per jdstrand, pay-ui
> redflagged
> >     ok per conversation with ted, dholbach and jdstrand",
> >         "vendor": "Canonical",
> >         "overrides": {
> >           "error": {
> >             "security_template_valid (payui.json)":
> >               "(MANUAL REVIEW) 'unconfined' not allowed",
> >             "lint_hooks_redflag_payui":
> >               "(MANUAL REVIEW) 'pay-ui' not allowed"
> >           },
> >           "warn": {}
> >         }
> >       },
> >       "com.bq.super-contacts":
> >       {
> >         "reviewer": "dholbach",
> >         "date": "2014-09-07",
> >         "note": "Special (official) contacts App from BQ. Necessarily
> needs access
> >     to global contacts",
> >         "vendor": "BQ",
> >         "overrides": {
> >           "error": {
> >             "security_policy_groups_safe_contacts (super-contacts.json)":
> >               "(MANUAL REVIEW) reserved policy group 'contacts': vetted
> applications
> >     only"
> >           },
> >           "warn": {}
> >         }
> >       },
> >       "com.ubuntu.music":
> >       {
> >         "reviewer": "popey",
> >         "date": "2014-09-07",
> >         "note": "Unconfined ok per jdstrand, but should move to confined
> with
> >     music-files-read policy group in the future",
> >         "overrides": {
> >           "error": {
> >             "security_template_valid (apparmor.json)":
> >               "(MANUAL REVIEW) 'unconfined' not allowed"
> >           },
> >           "warn": {
> >             "lint_click_local_extensions":
> >               "found unofficial extensions: x-source, x-test"
> >             }
> >           }
> >         }
> >       },
> >       "com.ubuntu.developer.jdstrand.permy":
> >       {
> >         "reviewer": "popey",
> >         "date": "2014-09-07",
> >         "note": "App permissions viewer-- requires access to click
> security json.
> >     Should not include networking",
> >         "overrides": {
> >           "error": {
> >             "security_redflag_fields (permy.json)":
> >               "found redflagged fields (needs human review): read_path"
> >           },
> >           "warn": {}
> >         }
> >       },
> >       "com.ubuntu.developer.foo.bar":
> >       {
> >         "reviewer": "jdstrand",
> >         "date": "2014-09-07",
> >         "note": "Uses global contacts list in read only. Code review
> shown to abuse
> >     access. New code should be verified this is still true.",
> >         "requires_new_review": true,
> >         "overrides": {
> >           "error": {
> >             "security_policy_groups_safe_contacts (apparmor.json)":
> >               "(MANUAL REVIEW) reserved policy group 'contacts': vetted
> applications
> >     only"
> >           },
> >           "warn": {}
> >         }
> >       }
> >     }
> >
> >
> > All of this sounds pretty reasonable, however I'm now somewhat confused.
> I think
> > we're talking about different things here. So to disambiguate,
> >
> > 1. the process is to override errors so to not automatically reject, but
> force a
> > manual review, and *not* to override errors to allow an automatic
> approval, right?
>
> What you have implemented thus far is perfect. What Daniel is wanting to
> do is
> further reduce the number of manual reviews for apps where the app was
> already
> manually reviewed, so it doesn't have to be next time. What I have
> discussed
> allows for the overrides to allow warnings and errors that would normally
> prompt
> a manual review to be ignored. I also discussed that we could have logic
> around
> this so that sometimes errors could get a pass, but other times not and if
> not,
> there is extra information that is available for display to the reviewer.
>

thanks for the clarification. Makes absolute sense now.

>
> > 2. the json data proposed in this thread, where does that live? does
> this belong
> > in each click package metadata? is this the json returned by the
> > click-reviewers-tools scripts?
> >
>
> I was thinking about this and I think that the click-reviewers-tools
> should be
> what it applying the overrides, so the server doesn't have to have any more
> complicated logic. I envision the scripts could add the applicable
> override json
> to its json output and return "pass" when overrides are applied (assuming
> no
> other warning or errors). The json data could live anywhere and the scripts
> would fetch it just like they do now for frameworks and apparmor json
> data. The
> only requirement I have is that adjusting the overrides needs to always
> have
> peer review.
>

Regarding this json data... I'm currently working on moving the frameworks
json out of the click-reviewers-tools branch. Specifically, the idea is
that the frameworks will be maintained on the server, with an admin ui for
certain people to add/edit frameworks and an API to query all frameworks
and their status (the API will provide the same json data that is currently
on the frameworks.json file, but dynamically). I'll be updating
click-reviewers-tools later to fetch the frameworks from the server and
remove the frameworks.json file.

Given that we're moving the frameworks out of click-reviewers-tools and
into the server, we should probably do the same with this new
manual-review-overrides json data.


>
>
> --
> Jamie Strandboge                 http://www.ubuntu.com/
>
>
> --
> Mailing list: https://launchpad.net/~ubuntu-appstore-developers
> Post to     : ubuntu-appstore-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~ubuntu-appstore-developers
> More help   : https://help.launchpad.net/ListHelp
>
>