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

Tue May 8 01:58:30 UTC 2012

Thanks.

Pushed to master based on suggested changes.

----- Original Message -----
From: "Ade Lee" <alee at redhat.com>
To: "John Magne" <jmagne at redhat.com>
Cc: "Endi Sukma Dewata" <edewata at redhat.com>, pki-devel at redhat.com
Sent: Monday, May 7, 2012 1:30:58 PM
Subject: Re: [Pki-devel] 0001-Provide-CA-EE-Restful-interface-and-test-client.patch

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 at redhat.com>
> To: "John Magne" <jmagne at redhat.com>
> Cc: "Endi Sukma Dewata" <edewata at redhat.com>, pki-devel at 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 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.
> > 
> > _______________________________________________
> > Pki-devel mailing list
> > Pki-devel at redhat.com
> > https://www.redhat.com/mailman/listinfo/pki-devel
> 
> 