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

[Ade Lee]

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 at redhat.com>
> To: pki-devel at 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel