Hello, (with >34 °C outside, I decided to spend some hours in the office ;-)
Am Montag, 17. Juni 2013 schrieb nore...@launchpad.net: > ------------------------------------------------------------ > revno: 7 > committer: Kshitij Gupta <kgupta8...@gmail.com > branch nick: apparmor-profile-tools > timestamp: Tue 2013-06-18 03:49:05 +0530 > message: > added severity.py with tested convert_regex and the old and new > config added: > lib/configbkp.py > lib/severity.py > modified: > lib/config.py Your code looks quite good, but let me add some random thoughts about it nevertheless ;-) === lib/config.py === > def write_config(filename, config): > """Writes the given configparser to the specified file""" > filepath = confdir + '/' + filename > try: > with open(filepath, 'w') as config_file: > config.write(config_file) > except IOError: > raise IOError("Unable to write to %s"%filename) > else: > permission_600 = stat.S_IRUSR | stat.S_IWUSR # Owner read and write > # Set file permissions as 0600 > os.chmod(filepath, permission_600) This code has two small bugs, at least for nitpickers ;-) - it should write to a tempfile, check for errors and then replace the config_file atomically (to avoid breaking the configfile when your disk is full ;-) - the correct order is: create file, set permissions, write the content (your code leaves the file readable for others for a short moment - except if the python default permissions are 600 of course ;-) >From users' POV, it might be better to keep the previous permissions instead of using hardcoded 600 - but 600 is more secure. @John: what is your opinion about this? === severity.py === > class Severity: > def __init__(self, dbname=None, default_rank=10): > [...] > if line.startswith('/'): > try: > path, read, write, execute = line.split() > except ValueError: > raise("Insufficient values for permissions") Very helpful error message ;-) Please include the filename and the line (line number or content). A good error message would be: Error in /etc/apparmor/severity.db: Insufficient values for permissions in line /foo/bar 2 You should also check if read, write, execute contain valid severity numbers (0-10). > ptr[regexp] = {'SD_RANK': {'r': read, 'w': > write, 'x': execute}} The "SD_" prefix probably dates back to the times when AppArmor was named "SubDomain". Feel free to use another prefix, maybe "AA_" ;-) > elif line.startswith('CAP'): > resource, severity = line.split() > self.severity['CAPABILITIES'][resource] = severity Nitpicking again: I'd check for "CAP_" ;-) Besides that, the error check is missing - I'm quite sure a line CAP_FOOBAR (without a number) will break something ;-) You should check that a) the line consists of two parts (+ optional comment?) b) the second part (the severity) is a valid number (0-10) > else: > print("unexpected database line: %s"%line) This error message is better than the one I quoted above, but adding the filename won't hurt ;-) > def convert_regexp(self, path): > [...] > regex = regex.replace('**', '.SDPROF_INTERNAL_GLOB') > regex = regex.replace('*', '[^/]SDPROF_INTERNAL_GLOB') > [...] > # Restore the * in the final regex > regex = regex.replace('SDPROF_INTERNAL_GLOB', '*') I might be paranoid, but - what happens if access to a file called /foo/barSDPROF_INTERNAL_GLOB is requested? ;-) This is highly theoretical for severity.db, but please keep it in mind if you use similar code for logprof/genprof. (You probably won't be able to avoid such conflicts, but you can reduce their chance by choosing a longer/more "cryptical" string.) === configbkp.py === Looks like a copy of the old code in config.py. That's why bzr has a version history - no need to checkin backups of old code ;-) Regards, Christian Boltz -- Sach ma, siggst du alles von mir? ;) [David Haller in fontlinge-devel] -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor