ubuntu-appstore-developers team mailing list archive

[Jamie Strandboge <jamie@xxxxxxxxxxxxx>]

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.

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


-- 
Jamie Strandboge                 http://www.ubuntu.com/

Attachment: signature.asc
Description: OpenPGP digital signature