Hi Christian, Thanks for starting this work. I haven't started looking at the code, but wanted to ask questions and comment about the proposed classes.
On Sat, Nov 15, 2014 at 11:43:51PM +0100, Christian Boltz wrote: > This patch adds four classes - two "base" classes and two specific for > capabilities: > > utils/apparmor/rule/__init__.py: > > class base_rule(object): > Base class to handle and store a single rule > > class base_rules(object): > Base class to handle and store a collection of rules > > utils/apparmor/rule/capability.py: > > class capability_rule(base_rule): > Class to handle and store a single capability rule > > class capability_rules(base_rules): > Class to handle and store a collection of capability rules > > > Most of the code is in the base classes (in __init__.py) - that's code > that will be reused by each rule class. > > capability.py contains only code that is specific to capabilities. That all seems sensible. > The layout of the base_rules and capability_rules class is not too > different from the proposal I sent some weeks ago. > > The biggest difference is that the class doesn't store the full profile > and include list. When I tried to do this, aa-logprof started to eat > *lots of* memory before I Ctrl-C'd it in a deepcopy() operation. > Either we find a way to implement this in a sane way that doesn't eat > memory, or we just keep it out ;-) Do you have a code snippet of how you went about this? Keeping references to things should be relatively lightweight. Perhaps it's an artifact of the specific implementation of the global hasher thingy. > I didn't implement the functions to propose rules yet - that's something > for the second round. The interesting question is if we can do this > inside the class without having the list of include files inside the > class. > > The storage for single rules is more different - initially I planned to > use a dict at least for the simple rule types like capability, but it > turned out that using a class has some advantages. Yeah. Even if we did something 'clever' with capabilities, given that its a small finite set of permissions, it would not be a good model for the other rule types. > Here's the list of functions in each class: > > utils/apparmor/rule/__init__.py: > > class base_rule(object): > '''Base class to handle and store a single rule''' Are the fields that you expect to store in each rule? 'audit' and 'deny' seem like obvious candidates. > def get_raw(self, depth): > '''return raw rule (with original formatting, and leading whitespace > in the depth parameter)''' > > def audit_allow_str(self): > '''return the allow/deny and audit keyword as string, including > whitespace''' > > def parse_audit_allow(self, matches): > '''returns audit, deny, allow_keyword and comment from the matches > object > > class base_rules(object): > '''Base class to handle and store a collection of rules''' > > def __init__(self): > self.delete_all_rules() > > def delete_all_rules(self): > self.rules = [] Why is delete_all_rules() necessary? __init__() should probably just do the initialization of self.rules directly, and we really shouldn't be re-using an object to encapsulate a different set of rules. And realistically, if you just declare 'rules = []' as a field in the class, then neither function is necessary (though it wouldn't surprise me if we needed __init__() for other reasons). > def add_raw(self, rawrule): > '''parse rawrule (from profile file) and store it in a structured > way''' Is rawrule a string or a complex data structure that contains back-reference information to the file/line it came from? > def get_raw(self, depth): > '''return all raw rules (if possible/not modified in their original > formatting). > > def get_clean(self, depth): > '''return all rules (in clean/default formatting) > > def covered_obj(self, rule_obj, check_allow_deny = True, check_audit = > False): > '''return True if rule_obj is covered by existing rules, otherwise > False''' > > def covered_log(self, parsed_log_event, check_allow_deny = True, > check_audit = False): > '''return True if parsed_log_event is covered by existing rules, > otherwise False''' > > def covered_raw(self, rawrule, check_allow_deny = True, check_audit = > False): > '''return True if rawrule is covered by existing rules, otherwise > False''' > > def delete_obj(self, rule_obj): > '''Delete rule_obj from rules''' > > def delete_raw(self, rawrule): > '''Delete rawrule from rules''' Some of these feel like duplication, or overlapping of purpose. > def delete_duplicates(self, inccaps): > '''Delete duplicate rules. What is the inccaps argument here? > def get_glob_ext(self, path_or_rule): > '''returns the next possible glob with extension (for file rules > only). When we come around to dbus and some of the other rule types, it's likely they will also have meaningful results. What's the return type/value for if there's no meaningful glob? Would 'capability,' (*shudder*) be a valid glob for capabilities? > utils/apparmor/rule/capability.py: > > class capability_rule(base_rule): > '''Class to handle and store a single capability rule''' > > def __init__(self): > self.capability = [] Why is it a list, for a single capability? (And again, just declare the field with the initial value.) > def get_clean(self, depth): > '''return rule (in clean/default formatting)''' > > def set_raw(self, rawrule): > '''parse and store rawrule''' > > def set_log(self, parsed_log_event): > '''parse and store log event''' > > def covered(self, rule_obj, check_allow_deny = True, check_audit = False): > '''check if rule_obj is covered by this rule object''' > > class capability_rules(base_rules): > '''Class to handle and store a collection of capability rules''' > > def new_rule(self): > '''tiny helper function that allows to keep several functions to > parent class''' I... don't quite parse that, but perhaps I need to look at code to see the purpose. > def get_glob(self, path_or_rule): > '''Return the next possible glob. For capability rules, that's always > "capability," (all capabilities)''' Seems like perhaps there should be just a get_globs() function, which returns a list of globs/glob extensions, etc.? Thanks again. -- Steve Beattie <[email protected]> http://NxNW.org/~steve/
signature.asc
Description: Digital signature
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
