[pki-devel][Patch] 0013-Initial-enrollment-progress.patch
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.
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@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel
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@redhat.com> To: pki-devel@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@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel
_______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel
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@redhat.com> To: pki-devel@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@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel
_______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel
Based on cfu ACK, pushed to master. Created brand new instance locally with this code and format and the current progress of enrollment worked as expected. ----- Original Message -----
From: "Christina Fu" <cfu@redhat.com> To: pki-devel@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@redhat.com> To: pki-devel@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@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel
_______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel
_______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel
participants (2)
-
Christina Fu -
John Magne