On 09/17/2014 09:17 AM, Ricardo Kirkner wrote: > > > On Wed, Sep 17, 2014 at 10:44 AM, Jamie Strandboge <[email protected] > <mailto:[email protected]>> 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/
signature.asc
Description: OpenPGP digital signature
-- Mailing list: https://launchpad.net/~ubuntu-appstore-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~ubuntu-appstore-developers More help : https://help.launchpad.net/ListHelp
