[PATCH] pki-cfu-0027-ticket-882-tokendb-policy-handling-revocation-and-re.patch
This is continuation of token policy work from ticket #882 This patch provides 1. the 2nd part (and not last) of the token policy work - determining policies for Enrollment, Re-enrollment, Renewal, Recovery, and Revocation (note: Recovery decision needs more refinement in later patch. The generateCertsAfterRenewalRecoveryPolicy method is currently just a close translation from the original TPS and is extremely complex. It deserves some close inspection and improvement at later time, in next round) 2. revocation for certificates due to token operations (note: administrator initiated will be handled in later patch) thanks, Christina
Comments: -This method: public TPSTokenPolicy (TPSSubsystem tps) { + this.tps = tps; + // init from config first + String policySetString = getDefaultPolicySetString(); + parsePolicySetString(policySetString); Maybe just check for a null tps in the constructor and throw an exception. Then we don't have to worry about tps being invalid again. -This method: public Hashtable<String, TokenRecord> tdbFindTokenRecords(String uid) + throws Exception { OK, there are a bunch of places where we are passing back and passing around these hashbables of tokens or certs or whatever. First we have no idea what is in the the hash table unless inspecting the code closely. Second. It looks like this method is takin an Iterator returned by the DB code and then making this hash table. I propose that we create classes for this such as "TPSTokenRecordList" and then provide convenient methods to return the entries to the caller. We could even just use the Iterator returned before. Then when we pass this stuff to other functions , we pass the convenient class. -This method: + private TPSStatus generateCertsAfterRenewalRecoveryPolicy(EnrolledCertsInfo certsInfo, SecureChannel channel, AppletInfo aInfo) + throws TPSException { Just restating what you said earlier in that this thing is way to complex and confusing to digest. It needs simplification. - This method: protected void revokeCertificates(String cuid) throws TPSException { Here we wait until we are like 70% of the way through the processing to see if we are even configured to revoke certificates. I see that in the beginning we try to cull out already revoked certs, but this could be done in another method or just wait for the next time we actually need to revoke anything. ----- Original Message ----- From: "Christina Fu" <cfu@redhat.com> To: pki-devel@redhat.com Sent: Monday, August 18, 2014 6:56:17 PM Subject: [Pki-devel] [PATCH] pki-cfu-0027-ticket-882-tokendb-policy-handling-revocation-and-re.patch This is continuation of token policy work from ticket #882 This patch provides 1. the 2nd part (and not last) of the token policy work - determining policies for Enrollment, Re-enrollment, Renewal, Recovery, and Revocation (note: Recovery decision needs more refinement in later patch. The generateCertsAfterRenewalRecoveryPolicy method is currently just a close translation from the original TPS and is extremely complex. It deserves some close inspection and improvement at later time, in next round) 2. revocation for certificates due to token operations (note: administrator initiated will be handled in later patch) thanks, Christina _______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel
Thanks for the review, jack. Please find the new patch that: 1. checks for tps null value in TPSTokenPolicy constructor 2. for the methods with Hashtables passing around, I added comments to explain what they are, and renamed the methods to be more clear on what they do. However, I need time to think about creating classes for them as I'm not very certain we want to do that at the moment. I will add the discussion to the next patch, if it's okay with you,. 3. I moved the check with revokeCert config to the top of the method. Finally, as I stated, there are improvements to come for the recovery decision method. thanks, Christina On 08/18/2014 07:16 PM, John Magne wrote:
Comments:
-This method:
public TPSTokenPolicy (TPSSubsystem tps) { + this.tps = tps; + // init from config first + String policySetString = getDefaultPolicySetString(); + parsePolicySetString(policySetString);
Maybe just check for a null tps in the constructor and throw an exception. Then we don't have to worry about tps being invalid again.
-This method:
public Hashtable<String, TokenRecord> tdbFindTokenRecords(String uid) + throws Exception {
OK, there are a bunch of places where we are passing back and passing around these hashbables of tokens or certs or whatever.
First we have no idea what is in the the hash table unless inspecting the code closely. Second. It looks like this method is takin an Iterator returned by the DB code and then making this hash table.
I propose that we create classes for this such as "TPSTokenRecordList" and then provide convenient methods to return the entries to the caller. We could even just use the Iterator returned before. Then when we pass this stuff to other functions , we pass the convenient class.
-This method:
+ private TPSStatus generateCertsAfterRenewalRecoveryPolicy(EnrolledCertsInfo certsInfo, SecureChannel channel, AppletInfo aInfo) + throws TPSException {
Just restating what you said earlier in that this thing is way to complex and confusing to digest. It needs simplification.
- This method:
protected void revokeCertificates(String cuid) throws TPSException {
Here we wait until we are like 70% of the way through the processing to see if we are even configured to revoke certificates. I see that in the beginning we try to cull out already revoked certs, but this could be done in another method or just wait for the next time we actually need to revoke anything.
----- Original Message ----- From: "Christina Fu" <cfu@redhat.com> To: pki-devel@redhat.com Sent: Monday, August 18, 2014 6:56:17 PM Subject: [Pki-devel] [PATCH] pki-cfu-0027-ticket-882-tokendb-policy-handling-revocation-and-re.patch
This is continuation of token policy work from ticket #882
This patch provides 1. the 2nd part (and not last) of the token policy work - determining policies for Enrollment, Re-enrollment, Renewal, Recovery, and Revocation (note: Recovery decision needs more refinement in later patch. The generateCertsAfterRenewalRecoveryPolicy method is currently just a close translation from the original TPS and is extremely complex. It deserves some close inspection and improvement at later time, in next round) 2. revocation for certificates due to token operations (note: administrator initiated will be handled in later patch)
thanks, Christina
_______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel
Agreed to table the structure discussion (#2) to next patch - Note: I have converted Hashtables to ArrayList since. Verbal ACK received from Jack. Pushed to master: commit f90798b725430ac2ec44d1e29ea9fbd53abc4c64 thanks, Christina On 08/18/2014 07:56 PM, Christina Fu wrote:
Thanks for the review, jack. Please find the new patch that: 1. checks for tps null value in TPSTokenPolicy constructor 2. for the methods with Hashtables passing around, I added comments to explain what they are, and renamed the methods to be more clear on what they do. However, I need time to think about creating classes for them as I'm not very certain we want to do that at the moment. I will add the discussion to the next patch, if it's okay with you,. 3. I moved the check with revokeCert config to the top of the method.
Finally, as I stated, there are improvements to come for the recovery decision method.
thanks, Christina
On 08/18/2014 07:16 PM, John Magne wrote:
Comments:
-This method:
public TPSTokenPolicy (TPSSubsystem tps) { + this.tps = tps; + // init from config first + String policySetString = getDefaultPolicySetString(); + parsePolicySetString(policySetString);
Maybe just check for a null tps in the constructor and throw an exception. Then we don't have to worry about tps being invalid again.
-This method:
public Hashtable<String, TokenRecord> tdbFindTokenRecords(String uid) + throws Exception {
OK, there are a bunch of places where we are passing back and passing around these hashbables of tokens or certs or whatever.
First we have no idea what is in the the hash table unless inspecting the code closely. Second. It looks like this method is takin an Iterator returned by the DB code and then making this hash table.
I propose that we create classes for this such as "TPSTokenRecordList" and then provide convenient methods to return the entries to the caller. We could even just use the Iterator returned before. Then when we pass this stuff to other functions , we pass the convenient class.
-This method:
+ private TPSStatus generateCertsAfterRenewalRecoveryPolicy(EnrolledCertsInfo certsInfo, SecureChannel channel, AppletInfo aInfo) + throws TPSException {
Just restating what you said earlier in that this thing is way to complex and confusing to digest. It needs simplification.
- This method:
protected void revokeCertificates(String cuid) throws TPSException {
Here we wait until we are like 70% of the way through the processing to see if we are even configured to revoke certificates. I see that in the beginning we try to cull out already revoked certs, but this could be done in another method or just wait for the next time we actually need to revoke anything.
----- Original Message ----- From: "Christina Fu" <cfu@redhat.com> To: pki-devel@redhat.com Sent: Monday, August 18, 2014 6:56:17 PM Subject: [Pki-devel] [PATCH] pki-cfu-0027-ticket-882-tokendb-policy-handling-revocation-and-re.patch
This is continuation of token policy work from ticket #882
This patch provides 1. the 2nd part (and not last) of the token policy work - determining policies for Enrollment, Re-enrollment, Renewal, Recovery, and Revocation (note: Recovery decision needs more refinement in later patch. The generateCertsAfterRenewalRecoveryPolicy method is currently just a close translation from the original TPS and is extremely complex. It deserves some close inspection and improvement at later time, in next round) 2. revocation for certificates due to token operations (note: administrator initiated will be handled in later patch)
thanks, Christina
_______________________________________________ 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