Re: [Pki-devel] [pki-devel][Patch] 0014-Initial-enrollment-progress.patch

From: "Christina Fu" <cfu(a)redhat.com&gt; To: pki-devel(a)redhat.com Sent: Thursday, June 5, 2014 4:07:20 PM Subject: Re: [Pki-devel] [pki-devel][Patch] 0014-Initial-enrollment-progress.patch ACK if format is tested to work. I'm not drilling down on the enrollment part since we are still not quite there yet. Christina On 06/04/2014 01:54 PM, John Magne wrote: > CFU: > > Review comments addressed. > > Plus a little bit more progress on enrollment, > up until the point we need to parse the public key blob returned > by the token. Also, rebased and merged to to the latest trunk as of an hour > ago. > > > > > > ----- Original Message ----- >> From: "Christina Fu" <cfu(a)redhat.com&gt; >> To: pki-devel(a)redhat.com >> Sent: Tuesday, June 3, 2014 9:28:46 AM >> Subject: Re: [Pki-devel] [pki-devel][Patch] >> 0013-Initial-enrollment-progress.patch >> >> Jack, >> >> Overall, thanks for the refactoring. It does make things easier to read. I >> understand there are still code to be filled. Here are my comments. >> >> 1. getConfiguredKeyType() >> - The original code goes to loser if the keytype is undefined. You >> assigned >> "signing" as default. I think the original code is probably better >> - The original code verifies if the keytype gotten from config is one of >> the >> TokenKeyType defined. You probably should add such check as well. >> >> 2. I noticed that the format() method does not take the skipAuth boolean >> like >> before when enroll() has already done authentication. Wouldn't that mean >> format() could result authentication being done twice if format is forced >> (required) during enrollment? >> >> 3. a few typos >> - "TPSProcessor.enroll:" in debug should be "TPSEnrollProcessor.enroll:" >> - "TPSEnrollProcessor>enroll:" in debug should be >> "TPSEnrollProcessor.enroll:" >> - "selelect" should be "select" >> >> 4. generateCertificates() >> - I see that EnrolledCertsInfo serves both as input and output, so passing >> it >> in as a parameter is reasonable. But why do you need to return it? And you >> didn't' really return it. Or you can just do void instead. It already >> throws >> TPSException in case of errors. >> >> 5. I think there are a few missed "TODO". If you happen to see them, you >> might want to add to comment so we don't miss them. >> >> 6. I'm sorry I checked in my patch before you, so there will be conflicts >> to >> be resolved. Let's take one more look after your merge. >> >> Christina >> >> >> On 05/28/2014 07:47 PM, John Magne wrote: >> >> >> >> Initial enrollment operation progress. >> >> 1. Changed the names of some message classes for convenience. >> 2. Did some minor refactoring of methods needed by both the enroll >> and >> tps processor. >> 3. Created classes to handle the parsing and archival of PKCS#11 >> token >> data. >> 4. Created prep code for enrollment that reads in a bunch of config >> params and creates >> convenience objects to carry the data instead of the lengthy >> parameter >> lists we have had before. >> >> >> _______________________________________________ >> Pki-devel mailing list Pki-devel(a)redhat.com >> https://www.redhat.com/mailman/listinfo/pki-devel >> >> >> _______________________________________________ >> Pki-devel mailing list >> Pki-devel(a)redhat.com >> https://www.redhat.com/mailman/listinfo/pki-devel _______________________________________________ Pki-devel mailing list Pki-devel(a)redhat.com https://www.redhat.com/mailman/listinfo/pki-devel