Please review: Patches listed in reverse order (306 -> 303) Ade commit e3d47aabee97773832d2f8ac7ff138314b44f646 Author: Ade Lee <alee@redhat.com> Date: Thu May 19 11:56:26 2016 -0400 Add revocation information to pki CLI output. The date on which the certificate is revoked and the agent that revoked it is displayed now in cert-find and cert-show output. Ticket 1055 commit fb7707dbf7148387075fc21d803e2ecb12c66ab6 Author: Ade Lee <alee@redhat.com> Date: Thu May 19 10:49:59 2016 -0400 Allow cert-find using revocation reasons The REST API expects the integer revocation code to be passed in a certificate search. We have modified the client to allow the user to provide either a revocation code or a revocation reason as a search parameter. Ticket 1053 commit 443b3676302e7861180802784d8a1ebc43d07ea3 Author: Ade Lee <alee@redhat.com> Date: Thu May 19 00:08:20 2016 -0400 Add parameters to purge old published files Ticket 2254 commit 31342868aa4468fd7c2818727930932fd1e2d23e Author: Ade Lee <alee@redhat.com> Date: Wed May 18 15:33:36 2016 -0400 Add parameters to disable cert or crl publishing Right now, if publishing is enabled, both CRLs and Cert publishing is enabled. This causes a bunch of spurious error messages on IPA servers as cert publishing is not configured. As it is impossible to determine if cert publishing is not desired or simply misconfigured, we provide options to explicitly disable either cert or crl publishing. Specifically: * to enable/disable both cert and crl publishing: ca.publishing.enable = True/False This is the legacy behavior. * to enable CRL publishing only: ca.publishing.enable = True ca.publishing.cert_enable = False * to enable cert publishing only: ca.publishing.enable = True ca.publishing.crl_enable = False Ticket 2275
On 5/20/2016 2:20 PM, Ade Lee wrote:
Please review:
Patches listed in reverse order (306 -> 303)
Ade
Some comments/questions: Patch #303: 1. Instead of using underscores (i.e. ca.publishing.cert_enable and ca.publishing.crl_enable) it would be more consistent to use dots (i.e. ca.publishing.cert.enable and ca.publishing.crl.enable) in the parameter names. 2. The PublisherProcessor.isCertPublishingEnabled() and isCRLPublishingEnabled() currently swallow the exception thrown by getBoolean() and interpret it as disabled. I think since "this should never happen" the exception should (if not too much additional work) be allowed to bubble up. Patch #304: 1. I think the default maxAge and maxFiles should not be unlimited because most people probably will use the default values until something goes wrong (e.g. disk full), and we want to avoid that. It would be better to pick something reasonable, for example 1 year and 100 files, respectively. 2. Currently the unit for maxAge is hour. How long do people usually want to retain old files? Should we use day instead? 3. In purgeExcessFiles() the files are sorted by last modified timestamp. It's kind of risky since someone could accidentally do something that updates the timestamp, then code will be deleting a file that's not supposed to be purged yet. Can the files be sorted by their names? In Tomcat the log files can be sorted by their names since they contain a timestamp or sequence number. 4. Also in purgeExcessFiles() the last loop calls dir.listFiles() in each iteration. For efficiency it might be better to use a counter since the number of excess files can be computed before the loop. 5. The exception thrown by getInteger() should not be swallowed either. If there's a configuration problem we want to know that. Patch #305: 1. It might be better to check for invalid revocation reason in the RevocationReason.valueOf() itself so any code using it is guaranteed to get a valid value. Patch #306 will follow later. -- Endi S. Dewata
Patches 303, 305 and 306 have been modified as discussed and checked in. Patch 304 has been revised as discussed on IRC. Please review. Ade On Fri, 2016-05-20 at 17:00 -0500, Endi Sukma Dewata wrote:
On 5/20/2016 2:20 PM, Ade Lee wrote:
Please review:
Patches listed in reverse order (306 -> 303)
Ade
Some comments/questions:
Patch #303: 1. Instead of using underscores (i.e. ca.publishing.cert_enable and ca.publishing.crl_enable) it would be more consistent to use dots (i.e. ca.publishing.cert.enable and ca.publishing.crl.enable) in the parameter names.
2. The PublisherProcessor.isCertPublishingEnabled() and isCRLPublishingEnabled() currently swallow the exception thrown by getBoolean() and interpret it as disabled. I think since "this should never happen" the exception should (if not too much additional work) be allowed to bubble up.
Patch #304: 1. I think the default maxAge and maxFiles should not be unlimited because most people probably will use the default values until something goes wrong (e.g. disk full), and we want to avoid that. It would be better to pick something reasonable, for example 1 year and 100 files, respectively.
2. Currently the unit for maxAge is hour. How long do people usually want to retain old files? Should we use day instead?
3. In purgeExcessFiles() the files are sorted by last modified timestamp. It's kind of risky since someone could accidentally do something that updates the timestamp, then code will be deleting a file that's not supposed to be purged yet. Can the files be sorted by their names? In Tomcat the log files can be sorted by their names since they contain a timestamp or sequence number.
4. Also in purgeExcessFiles() the last loop calls dir.listFiles() in each iteration. For efficiency it might be better to use a counter since the number of excess files can be computed before the loop.
5. The exception thrown by getInteger() should not be swallowed either. If there's a configuration problem we want to know that.
Patch #305: 1. It might be better to check for invalid revocation reason in the RevocationReason.valueOf() itself so any code using it is guaranteed to get a valid value.
Patch #306 will follow later.
On 5/24/2016 10:32 PM, Ade Lee wrote:
Patches 303, 305 and 306 have been modified as discussed and checked in.
Patch 304 has been revised as discussed on IRC. Please review.
Ade
Just one thing, the maxAge unit is still hours. I'm not sure anybody wants to purge CRLs less than a day old. Considering in the future we're going to provide a default maxAge of 1 year, it might be better to specify it in days instead. Other than that it's ACKed. -- Endi S. Dewata
participants (2)
-
Ade Lee -
Endi Sukma Dewata