[Pki-devel] Request for Review: TPS Revocation Routing

[John Magne]

Christina:

Just some comments below:


1. 

TPS_PUBLIC int CertEnroll::revokeFromOtherCA( CERTCertificate *cert,
+        bool revoke,
+        const char*serialno, char *&o_status,
+        const char *reason) {
+
+    const char *caList = NULL;
+    const char *nick = NULL;
+    char configname[256];
+    char configname_nick[256];
+    ConfigStore *store = RA::GetConfigStore();


Initialize the char arrays like :    char configname_nick[256] = {0};

Check that store is not NULL ?

2. Same method:

Assert that 

char *caList_x = PL_strdup(caList); is not NULL?

3.
TOKENDB_PUBLIC int CertEnroll::UnrevokeCertificate(CERTCertificate *cert, const char *serialno, const char *connid,
+  char *&o_status)
+{
+    char configname[5000];
+    CERTCertDBHandle *certdb = CERT_GetDefaultCertDB();
+    CERTCertificate *caCert = NULL;
+    char * caNickname = NULL;
+
+    PR_ASSERT(certdb != NULL);
+    if ((cert == NULL) ||
+        (serialno == NULL) || (connid == NULL)) {
+        RA::Debug("CertEnroll::UnrevokeCertificate", "missing info in call");
+        return 1;
+    }
+    if (cert != NULL) {
+        /* For debugging; 

If cert was NULL, we would have already bombed out of the func.

Same thing here:

+TOKENDB_PUBLIC int CertEnroll::RevokeCertificate(CERTCertificate *cert, const char *reason, const char *serialno, const char *connid, char *&o_status)

4.
 Higher level question:  Why do we have this special function revokeFromOtherCA that is called from Unrevoke and Revoke. Why can't we just have one Revoke that first tries the regular CA and then rifles through the list of others to contact? That way we wouldn't need Unrevoke and revokeFromOtherCA??


5.

 caSKI_x = BTOA_ConvertItemToAscii(&ca_ski);


Looks like caSKI_x gets set in two different ways, here and earlier. At the end we have a free, is this free method appropriate for both cases?


Also in same method:

char error_msg[512];
+                int status = 0;
+                status = store->Commit(true, error_msg, 512);


Init the error_msg?

Also, is Commit expecting a char * ? or do we need the address of error_msg? This comes up in the other revoke related methods as well.


6.

Same method:



RevokeCertificate looks almost line for line identical to this one. I suppose there must be some way to conveniently merge these into one?

7.

Method: TPS_PUBLIC int CertEnroll::revokeFromOtherCA( CERTCertificate *cert,
+        bool revoke,
+        const char*serialno, char *&o_status,
+        const char *reason) {
+

/* store it in config */
+            caSKI_x = BTOA_ConvertItemToAscii(&ca_ski);
+            if (caSKI_x == NULL) {
+                if (caCert != NULL) {
+                    CERT_DestroyCertificate(cert);
+                }
+                continue;
+            }

Looks like you are Destroying the wrong cert, cert instead of caCert.

Also, I think the same thing is happening further down here:

if (ret == 0) {
+                if (caList_x != NULL) {
+                    PL_strfree(caList_x);
+                }
+                if (caSKI_x != NULL) {
+                    PL_strfree(caSKI_x);
+                }
+                if (caCert != NULL) {
+                    CERT_DestroyCertificate(cert);
+                }

And at the end of the function.
Maybe this is on purpose, but should we be destroying something that was sent in as a param? Also, there seems to be some duplication in the
freeing code, maybe create a cleanup label and jump down?

8. One more general question:

Inside each ca connection entry in the cfg, we have a list of other ca connections, which I suppose point to other ca connections. I was just wondering if this list should be at a higher level? If I get this right, would every ca connection have a list of pointers to all the other ca connections?

Actually , it now looks like maybe that is what you are doing, having the list as conn.ca.list=ca1,ca2, but you appear to have this guy listed under the comments for the block of conn.ca1, which represents a specific connection.


9. Looks like we have some more uninitialized static char arrays sprinkled in the code. Just a minor thing.


----- Original Message -----
From: "Christina Fu" <cfu at redhat.com>
To: pki-devel at redhat.com
Sent: Wednesday, March 20, 2013 10:33:34 AM
Subject: Re: [Pki-devel] Request for Review: TPS Revocation Routing


Here is a newer revision that stores AKI in the CS.cfg the first time it is figured out so it does not have to be figured out for every revocation or unrevocation operation. 

https://bugzilla.redhat.com/attachment.cgi?id=713376&action=diff&context=patch&collapsed=&headers=1&format=raw 

Christina 

On 03/19/2013 10:35 AM, Christina Fu wrote: 

Please note that the "Issuance routing" part in this bug is not included in this design/patch. It will be filed as a separate bug to be addressed. 
This design/patch is for TPS revocation routing only. 

regards, 
Christina 

On 03/18/2013 08:19 PM, Christina Fu wrote: 

Hi, 
Please review the code for the following design: 
http://pki.fedoraproject.org/wiki/TPS_Revocation_Routing 
for Bug 902952 - RFE: Cert System 8.1 - Issuance/Revocation routing with TPS and multiple non-cloned CAs 

code: 
https://bugzilla.redhat.com/attachment.cgi?id=712351&action=diff&context=patch&collapsed=&headers=1&format=raw 

thanks! 
Christina 
_______________________________________________
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 

_______________________________________________
Pki-devel mailing list
Pki-devel at redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel