Ca certs api v2 #4635

fmarco76 · 2023-12-07T18:18:09Z

Implement v2 REST end-point for /ca/v2/certs

Implement the REST operations:

GET of /ca/v2/certs/
GET of /ca/v2/certs
POST to /ca/v2/certs/search.

These implement the behaviour of the current REST operations (see [1, 2 and 3]). The new version is not based on RESTeasy framework for the REST operation and the DB searches have replaced VLV with paged queries.

The only differences with the previous implementation are:

the total value now show the number of returned item;
the number of returned entry is limited (this is hard-coded for now).

This replicate the current behaviour imeplemented in /ca/rest/certs/<id>.

Implement the REST operations: - GET of /ca/v2/certs - POST to /ca/v2/certs/search. These implement the behaviour of the current REST operations (see [1, 2]). The new version is not based on RESTeasy framework for the REST operation and the DB searches have replaced VLV with paged queries. The only differences with the previous implementation are: - the total value now show the number of returned item; - the number of returned entry is limited (this is hard-coded for now). 1. https://github.com/dogtagpki/pki/wiki/CA-List-Certificates-REST-API 2. https://github.com/dogtagpki/pki/wiki/CA-Search-Certificates-REST-API

fmarco76 · 2023-12-07T18:22:49Z

@edewata The total can be fixed to provide the exact number if needed but has to be retrieved from the first query and reported to the servlet so it will require some changes. Is this needed.
For the limit on the returned entry I think it is not a real limitation in the real usage but I wondering if it should be configurable and if we should have different limits for search and returned entries.

edewata

I have some comments/questions, but I think overall it's good. Feel free to update or merge as is, then we can address the comments in a new PR.

edewata · 2023-12-11T06:45:11Z

base/ca/src/main/java/com/netscape/cmscore/dbs/CertificateRepository.java

+     * {@Code (&(certRecordId=5)(x509Cert.notBefore=934398398))}
+     *
+     * @param filter search filter
+     * @param maxSize max size to return


The @param should match the actual method params.

edewata · 2023-12-11T06:46:31Z

base/ca/src/main/java/com/netscape/cmscore/dbs/CertificateRepository.java

+     * @return a list of certificates
+     * @exception EBaseException failed to search
+     */
+    public Iterator<CertRecord> searchCertificates(String filter, int timeLimit, int start, int size)


The timeLimit doesn't seem to be used. Should we keep it?

It is implemented in the pagedSeearch. I will add to the call.

edewata · 2023-12-11T06:55:36Z

base/ca/src/main/java/org/dogtagpki/server/ca/v2/CertServlet.java

+        String base64 = CertUtil.toPEM(cert);
+        certData.setEncoded(base64);


This is probably an opportunity to fix APIv1. Here we could just return the base64-encoded value without the PEM header/footer. I think that would be easier to parse and more consistent with other binary data in JSON in general. What do you think?

I do not know in other places yet but here we have the chain in base64 the certificate in pem. It is better to have all base64.

edewata · 2023-12-11T06:58:03Z

base/ca/src/main/java/org/dogtagpki/server/ca/v2/CertServlet.java

+        CertPrettyPrint print = new CertPrettyPrint(cert);
+        certData.setPrettyPrint(print.toString(loc));


The pretty-print probably should be generated on the client side because the client already has the cert binaries, and the output would be dependent on client's locale anyway, so I think we can skip this field.

edewata · 2023-12-11T07:00:34Z

base/ca/src/main/java/org/dogtagpki/server/ca/v2/CertServlet.java

+        Date notBefore = cert.getNotBefore();
+        if (notBefore != null) certData.setNotBefore(notBefore.toString());
+
+        Date notAfter = cert.getNotAfter();
+        if (notAfter != null) certData.setNotAfter(notAfter.toString());


The notBefore and notAfter probably should be generated on the client side too since it's dependent on client's locale and the client already has the cert binaries.

I would leave this for the moment. PrettyPrint is better in the client but the validity could be useful without the need to read the binary. What we could change is the format to be more platform independent (e.g. yyyy-MM-dd HH:mm:ss Z). The client can read or generate from binary

Currently we have two different format of data, in the certificate list we have time in milliseconds while in a single certificate we have the string. This because the list uses the class CertDataInfo while the single certificate uses the class CertData. These classes are in the same package and have almost the same fields but with different format (e.g. date is Date in one and String in the other) . Not clear why we need to have both. We could merge them. To investigate in a separate PR

edewata · 2023-12-11T07:04:04Z

base/ca/src/main/java/org/dogtagpki/server/ca/v2/CertServlet.java

+
+            int total = results.size();
+            logger.info("Search results: " + total);
+            infos.setTotal(total);


We probably can skip the total since it can be obtained from infos.getEntries().size().

It is OK to remove for now. The pageSearch could provide the total number of entries in the DB but the information is not returned from the LDAPSession. If needed we could handle that information to the client. We will consider this in future.

I have done a couple of test and it is not enough to skip the setTotal(), it is reported as total=0 in the final json. I am leaving for now and evaluate in separate PR.

edewata · 2023-12-11T08:41:55Z

base/server/src/main/java/com/netscape/cmscore/dbs/LDAPSession.java

+
+            LDAPSearchConstraints cons = new LDAPSearchConstraints();
+            if (timeLimit > 0) {
+                cons.setServerTimeLimit(timeLimit);


The LDAPSearchConstraints also has a setMaxResults(). I was wondering whether we should just ignore it, set it to size, or add another param for it (e.g. sizeLimit).

Another consideration is, IIUC the size is only used internally for paging, but the number of results the client will see won't actually be affected by the size. On the other hand, the sizeLimit will (if it's used to call setMaxResults()).

So maybe we should replace size with a constant or a server-side configurable param, and add a sizeLimit not for paging but for setting the max results.

Actually, size elements are returned from the search. The size is always used in the last query and the elements are returned . If start is provided, than one or more pages are requested until start - 1 elements are retrieved and then a page with size is requested.
If size is not provided there is a limit to 500. I am leaving the limit now and we can decide if this has to be customisable in a separate PR

Not sure about the MaxResults. We could discuss better approach with DS folk.

edewata · 2023-12-11T08:47:51Z

base/ca/src/main/java/org/dogtagpki/server/ca/v2/CertServlet.java

+        byte[] p7Bytes = bos.toByteArray();
+
+        String p7Str = Utils.base64encode(p7Bytes, true);
+        certData.setPkcs7CertChain(p7Str);


I was wondering if we should skip this field since not all clients needs to get the PKCS #7 cert chain, so maybe it can be provided by a different API.

We can remove this field when the new API is created in a separate PR.

edewata · 2023-12-11T08:48:47Z

base/ca/src/main/java/org/dogtagpki/server/ca/v2/CertServlet.java

+                        revExts.get(CRLReasonExtension.NAME);
+                    certData.setRevocationReason(ext.getReason().getCode());
+                } catch (X509ExtensionException e) {
+                    // nothing to do


Let's log an error so at least we know if something happens.

sonarqubecloud · 2023-12-11T15:40:28Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
48 Code Smells

0.0% Coverage
2.6% Duplication

fmarco76 · 2023-12-11T16:09:56Z

@edewata Thanks!

I have update following some of your comments. The remaining comments need additional work in separate PRs.

fmarco76 added 2 commits December 7, 2023 19:11

Implement GET verb for /ca/v2/certs/<id> REST end-point

0493625

This replicate the current behaviour imeplemented in /ca/rest/certs/<id>.

fmarco76 requested a review from edewata December 7, 2023 18:18

fmarco76 changed the title ~~Ca certs ap iv2~~ Ca certs api v2 Dec 7, 2023

Tidy up CertServlet

63ca1e8

edewata approved these changes Dec 11, 2023

View reviewed changes

Fix Cert REST api v2 output format

477390f

fmarco76 merged commit 1ea0985 into dogtagpki:master Dec 11, 2023
131 checks passed

fmarco76 deleted the CACertsAPIv2 branch December 11, 2023 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ca certs api v2 #4635

Ca certs api v2 #4635

fmarco76 commented Dec 7, 2023

fmarco76 commented Dec 7, 2023

edewata left a comment

edewata Dec 11, 2023

edewata Dec 11, 2023

fmarco76 Dec 11, 2023

edewata Dec 11, 2023

fmarco76 Dec 11, 2023

edewata Dec 11, 2023

edewata Dec 11, 2023

fmarco76 Dec 11, 2023

fmarco76 Dec 11, 2023 •

edited

Loading

edewata Dec 11, 2023

fmarco76 Dec 11, 2023

fmarco76 Dec 11, 2023

edewata Dec 11, 2023

edewata Dec 11, 2023

fmarco76 Dec 11, 2023

fmarco76 Dec 11, 2023

edewata Dec 11, 2023

fmarco76 Dec 11, 2023

edewata Dec 11, 2023

sonarqubecloud bot commented Dec 11, 2023

fmarco76 commented Dec 11, 2023

		String base64 = CertUtil.toPEM(cert);
		certData.setEncoded(base64);

		CertPrettyPrint print = new CertPrettyPrint(cert);
		certData.setPrettyPrint(print.toString(loc));

Ca certs api v2 #4635

Ca certs api v2 #4635

Conversation

fmarco76 commented Dec 7, 2023

fmarco76 commented Dec 7, 2023

edewata left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmarco76 Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Dec 11, 2023

fmarco76 commented Dec 11, 2023

fmarco76 Dec 11, 2023 •

edited

Loading