11:25 a.m.
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
7:36 p.m.
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?
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?
Also you could if you wanted pre-calculate those hash indexes into a var and use them.
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?
Just checking to see if those late to lines get called (along with the print, even if
there is an error importing certs) ??
Method:
class InstanceExternalCertDeleteCLI(pki.cli.CLI):
Would there be any value in allowing multiple nicknames to be deleted?
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?
----- Original Message -----
From: "Ade Lee" <alee(a)redhat.com>
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
12:19 p.m.
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>
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
4:20 p.m.
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>
> 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
9:53 p.m.
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>
> > 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
2:42 p.m.
Acked by Jack. Pushed to master.
On Tue, 2016-03-01 at 22:53 -0500, Ade Lee wrote:
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>
> > > 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
2969
days inactive
2972
days old
5 comments
2 participants
participants (2)
-
Ade Lee -
John Magne