Re: [Pki-devel] [PATCH] 278 - handle external certs

Found an error .. lets try again .. Ade On Tue, 2016-03-01 at 17:20 -0500, Ade Lee wrote: > Based on feedback, revamped the internals to make it look much > nicer. > > Ade > On Tue, 2016-03-01 at 13:19 -0500, Ade Lee wrote: > > On Mon, 2016-02-29 at 20:36 -0500, John Magne wrote: > > > Review Notes: > > > > > > Looks like good stuff. > > > > > > Just a couple of small picky questions. > > > > > > We can delay the final ACK to after we either decide that the > > > questions > > > are not that important or we decide to make any minor changes. > > > > > > > > > thanks, > > > jack > > > > > > > > > This method: > > > > > > in __init__.py : > > > > > > > > > + def load_external_certs(self, conf_file): > > > + # load external certs data > > > + if os.path.exists(conf_file): > > > + lines = open(conf_file).read().splitlines() > > > + for line in lines: > > > + parts = line.split('=', 1) > > > + name = parts[0] > > > + value = parts[1] > > > + self.external_certs[name] = value > > > + else: > > > + self.external_certs['num_certs'] = 0 > > > + > > > > > > I see if there is no file, the value of num_certs is 0. > > > > > > What about in the loop that reads the file when it does exist? > > > What > > > if it's empty? > > > Also when doing the splitting for the values, do we need some > > > simple > > > sanity checking? > > > > > How about this -- > > > > + def load_external_certs(self, conf_file): > > + # load external certs data > > + if os.path.exists(conf_file) and > > os.stat(conf_file).st_size > > > 0: > > + lines = open(conf_file).read().splitlines() > > + for line in lines: > > + parts = line.split('=', 1) > > if len(parts) != 2: > > // throw some exception > > + name = parts[0] > > + value = parts[1] > > + self.external_certs[name] = value > > + else: > > + self.external_certs['num_certs'] = 0 > > > > + if 'num_certs' not in self.external_certs: > > // throw some exception > > > > > Same Q's for method: def update_external_cert_conf(self, > > > external_path, deployer): > > > > > > > > > def delete_external_cert(self, nickname, token): > > > + match_found = False > > > + num_certs = int(self.external_certs['num_certs']) > > > + if num_certs > 0: > > > + for num in range(0, num_certs): > > > + current_nick = self.external_certs[str(num) + > > > ".nickname"] > > > + current_token = self.external_certs[str(num) + > > > ".token"] > > > + if current_nick == nickname and current_token > > > == > > > token: > > > + del self.external_certs[str(num) + > > > ".nickname"] > > > + del self.external_certs[str(num) + > > > ".token"] > > > + match_found = True > > > + continue > > > + if match_found: > > > + self.external_certs[str(num - 1) + > > > ".nickname"] = current_nick > > > + self.external_certs[str(num - 1) + > > > ".token"] > > > = > > > current_token > > > + > > > + self.external_certs['num_certs'] = num_certs - 1 > > > + self.save_external_cert_data() > > > > > > I may have this wrong, but if you find a match and then > > > continue? > > > Will the "match_found if, ever get executed? > > > > > It will get executed on the next -- and every subsequent value. > > The purpose is to reorder subsequent values. > > > > So lets say we have values 0,1,2,3,4, and we have a match on 1. > > Then > > we will remove 1 and set 2-> 1, 3-> 2, 4->3. So the result will > > be > > 0,1,2,3 > > > > > Also you could if you wanted pre-calculate those hash indexes > > > into > > > a > > > var and use them. > > > > > yeah - but it doesn't really help as much -- see comment above. > > > > > > Question: Here: > > > > > > nicks = self.import_certs( > > > + instance, cert_file, nickname, token, trust_args) > > > + self.update_instance_config(instance, nicks, token) > > > + > > > + self.print_message('Certificate imported for instance > > > %s.' > > > % > > > + instance_name) > > > > > > Question is is there somewhere in that call chain that throws > > > an > > > exception if something goes wrong? > > > > yes. I'm tested a number of cases where that does happen. The > > exception comes all the way from nss out. > > > > > Just checking to see if those late to lines get called (along > > > with > > > the print, even if there is an error importing certs) ?? > > > > > No they don't. The exceptions cause the code to break out. > > > > > > Method: > > > > > > class InstanceExternalCertDeleteCLI(pki.cli.CLI): > > > > > > Would there be any value in allowing multiple nicknames to be > > > deleted? > > > > > perhaps - we can add as a refinement later if needed. > > > > > Also same at above here: > > > > > > instance = pki.server.PKIInstance(instance_name) > > > + instance.load() > > > + > > > + self.remove_cert(instance, nickname, token) > > > + instance.delete_external_cert(nickname, token) > > > + > > > + self.print_message('Certificate removed from instance > > > %s.' > > > % > > > + instance_name) > > > > > > > > > Do we get to the end if instance.load does not work? > > > > > No we don't. Exceptions propagate up. > > > > > > > > > > > > > > > > > > > > ----- Original Message ----- > > > From: "Ade Lee" <alee(a)redhat.com&gt; > > > To: pki-devel(a)redhat.com > > > Sent: Monday, February 29, 2016 9:25:30 AM > > > Subject: [Pki-devel] [PATCH] 278 - handle external certs > > > > > > This is to resolve ticket 1742. > > > > > > For this ticket, we need a mechanism to import third party > > > certs > > > to > > > clones. This patch provides a general mechanism to do this. > > > > > > A follow-on patch with documentation on how this all works is > > > forthcoming. > > > > > > Ade > > > _______________________________________________ _______________________________________________ Pki-devel mailing list Pki-devel(a)redhat.com https://www.redhat.com/mailman/listinfo/pki-devel