[Pki-devel] 0001-Provide-CA-EE-Restful-interface-and-test-client.patch

[John Magne]

Great stuff, thanks.

I will look into the ones you had questions about.

----- Original Message -----
From: "Endi Sukma Dewata" <edewata at redhat.com>
To: pki-devel at redhat.com, "John Magne" <jmagne at 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.

-- 
Endi S. Dewata