1:53 p.m.
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.
8:05 p.m.
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
12:24 p.m.
Great stuff, thanks.
I will look into the ones you had questions about.
----- Original Message -----
From: "Endi Sukma Dewata" <edewata(a)redhat.com>
To: pki-devel(a)redhat.com, "John Magne" <jmagne(a)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
7:05 p.m.
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(a)redhat.com>
To: pki-devel(a)redhat.com, "John Magne" <jmagne(a)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
9:26 a.m.
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(a)redhat.com>
To: pki-devel(a)redhat.com, "John Magne" <jmagne(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
11:38 a.m.
On 5/2/2012 7:05 PM, John Magne wrote:
Revised patch as per the suggestions below:
All the suggestions made sense and I implemented them as suggested.
Tests ran fine.
Thanks for the changes.
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.
The code also moves the cert into the beginning of the array. Here's the
scenario I was asking, suppose the cert chain is:
c0 c1 c2 c3 c4
Suppose c4 is the cert being requested. The code will generate this
result without any problem:
c4 c0 c1 c2 c3
However, suppose the requested cert is c2. The code will move c2 to
first and shift the rest back by one position:
c2 c0 c1 null c3
c4 can't be moved into the result since there isn't enough space.
If the scenario is possible, the code would include a null in the result
and possibly generate an index-out-of-bound error when moving c4.
If the scenario is not possible, the code could be optimized to check
the requested cert only against the last cert in the CA chain.
--
Endi S. Dewata
8:29 p.m.
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(a)redhat.com>
To: "John Magne" <jmagne(a)redhat.com>
Cc: "Endi Sukma Dewata" <edewata(a)redhat.com>, pki-devel(a)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(a)redhat.com>
To: pki-devel(a)redhat.com, "John Magne" <jmagne(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
3:30 p.m.
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(a)redhat.com>
To: "John Magne" <jmagne(a)redhat.com>
Cc: "Endi Sukma Dewata" <edewata(a)redhat.com>, pki-devel(a)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(a)redhat.com>
> To: pki-devel(a)redhat.com, "John Magne" <jmagne(a)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(a)redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel
8:58 p.m.
Thanks.
Pushed to master based on suggested changes.
----- Original Message -----
From: "Ade Lee" <alee(a)redhat.com>
To: "John Magne" <jmagne(a)redhat.com>
Cc: "Endi Sukma Dewata" <edewata(a)redhat.com>, pki-devel(a)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(a)redhat.com>
To: "John Magne" <jmagne(a)redhat.com>
Cc: "Endi Sukma Dewata" <edewata(a)redhat.com>, pki-devel(a)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(a)redhat.com>
> To: pki-devel(a)redhat.com, "John Magne" <jmagne(a)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(a)redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel
4381
days inactive
4389
days old
8 comments
3 participants
participants (3)
-
Ade Lee -
Endi Sukma Dewata -
John Magne