1. In CertResourceService, you throw InternalServerException(). Its not clear to me how this is different from CMSException (which takes as its default 500 - Internal Server error). Is InternalServerException superfluous then? 2. In CertResourceService, you throw CertNotFoundException in the case of EBaseException. This is too broad -- CertNotFoundException should be thrown only in the case of EDBRecordNotFoundException 3. In CertsResourceService, in searchCerts(data), we call dao.listCerts() with 0 for the maxResults and maxTime. This could lead to unbounded searches. We need to pass in some limits. Other than that, it looks pretty good! Ade On Fri, 2012-05-04 at 21:29 -0400, John Magne wrote:
Attached is the full diff from the current state of master.
Contains Endi's fixes as well as his concern about the certificate chain code.
Contains alee's suggestions including the class refactoring involving the CertRequests vs the KeyRequests.
----- Original Message ----- From: "Ade Lee" <alee@redhat.com> To: "John Magne" <jmagne@redhat.com> Cc: "Endi Sukma Dewata" <edewata@redhat.com>, pki-devel@redhat.com Sent: Thursday, May 3, 2012 7:26:22 AM Subject: Re: [Pki-devel] 0001-Provide-CA-EE-Restful-interface-and-test-client.patch
Some more initial feedback:
1. You create the method retrieveCert(CertRetrievalRequestData data) which is a POST method. This method is unnecessary and violates general Restful principles. That is - we should be using a GET method - which you have provided. The reason we created a POST method for the KeyResource is because we needed to pass in large data blobs as part of the request (a trans wrapped session key for example).
Is there a scenario where we would want to return the Certificate given say, the certificate request or some other large blob? If not, we should remove these methods.
2. The same point for the retrieveProfile (and retrievProfile (spelling?)) methods. The ProfileRetrievalRequest object contains only the profileID. These methods and objects are superfluous and should be removed. The GET method is sufficient.
3. There are a bunch of places where things are ToDo -- please use TODO instead - as it shows up in eclipse as a task to be done.
4. There are a bunch of places where WebApplicationException is thrown - those should be replaced by CMSException with the appropriate response code.
5. CertificateData has serialNumber as a string. Isn't there a class we were using for serial numbers?
6. enrollCert() returns a CertRequestInfo object, which does not contain any reference to a certificate (if issued). How would the user determine the serial number/ url of the certificate given a request? It makes sense to put in fields for the certificate url and possibly also serial number in the CertRequestInfo object.
7. EnrollmentRequestData has field renewal - that should be replaced by a boolean isRenewal.
8. The KeyRequest and CertRequest code is so similar - it makes sense to look now into using inheritance to leverage this.
Ade
On Wed, 2012-05-02 at 20:05 -0400, John Magne wrote:
Revised patch as per the suggestions below:
All the suggestions made sense and I implemented them as suggested. Tests ran fine.
Questions from below:
5. Also in CertDAO.getCertChainData() after the initialization loop it looks like the certsInChain may contain a null value if x509cert exists in mCACerts but not the last element. Is that case possible?
I could not see this scenario. What the code is doing is checking to see if you are trying to get the cert chain of a cert that is already a member of the CA's cert chain. In that case, the size of the array will be the size of the CA's cert chain. If this is not the case, the size of the array will be that value plus one.
9. Could you try creating some certs with this common name "E*Corp, Inc." and "E-Corp, Inc." and see if it can find an exact match. The escapeFilter() should escape the '*'.
I tried this after the fixes and was able to get an exact match on "E*Corp, Inc".
----- Original Message ----- From: "Endi Sukma Dewata" <edewata@redhat.com> To: pki-devel@redhat.com, "John Magne" <jmagne@redhat.com> Sent: Tuesday, May 1, 2012 6:05:28 PM Subject: Re: [Pki-devel] 0001-Provide-CA-EE-Restful-interface-and-test-client.patch
On 4/30/2012 1:53 PM, John Magne wrote:
Provide CA EE Restful interface and test client.
Tickets #144 and #145 Providing the following:
1. Simple EE restful interface for certificates, printing, listing and searching. 2. Simple EE restful interface for certificate enrollment requests. 3. Simple EE restful interface for profiles and profile properties. 4. Simple Test client to exercise the functionality. 5. Created restful client base class inherited by CARestClient and DRMRestClient. 6. Provide simple restful implementations of new interfaces added.
ToDO: Need some more refactoring to base classes for some of the new classes which are similar to classes in the DRM restful area. ToDO: Actual certificate enrollment code that will be refactored from existing ProfileSubmitServlet.
Some feedback:
1. The CMSRestClient could be moved into base/common/src/com/netscape/cms/servlet/csadmin, which can be used by any clients including the test tools.
2. The CAErrorInterceptor is identical to DRMErrorInterceptor. We can merge these classes into CMSErrorInterceptor and place it in the same package as CMSRestClient. The interceptor can be added in the CMSRestClient, so the subclasses don't need to do that.
3. In CertsResourceService.createSearchFilter() the 'matches' cannot be more than 1, so the code could be simplified.
4. In CertDAO.getCertChainData() the certsInChain array is allocated multiple times. It should be possible to use the loop to determine the length first, then allocate the array just once after that.
5. Also in CertDAO.getCertChainData() after the initialization loop it looks like the certsInChain may contain a null value if x509cert exists in mCACerts but not the last element. Is that case possible?
6. The CertSearchData.escapeValueRfc1779() probably can be moved into LDAPUtil.escapeDN(). We can rename the existing LDAPUtil.escape() into LDAPUtil.escapeFilter().
7. In CertSearchData.build*Filter() it's not necessary to use "this." when calling other methods of the same object. It's possible to use the fields directly too.
8. In CertSearchData.build*Filter() the values should be escaped to avoid injection attack:
filter.append("(certRecordId>=" + LDAPUtil.escapeFilter(serialFrom) + ")");
9. In CertSearchData.buildAVAFilter() the code does a double escape with the escape DN method. I think it should use 2 different escape methods, one for the DN and the other for the filter. So the code would look like this:
lf.append(LDAPUtil.escapeFilter(LDAPUtil.escapeDN(param)));
Could you try creating some certs with this common name "E*Corp, Inc." and "E-Corp, Inc." and see if it can find an exact match. The escapeFilter() should escape the '*'.
10. There are some formatting issues and trailing whitespaces. Auto-formatting probably will fix this.
_______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel