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

[Christina Fu]

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