Re: [Pki-devel] [PATCH] 303-306 Various issues

2016-05-20 Thread Endi Sukma Dewata

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

___
Pki-devel mailing list
Pki-devel@redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel


Re: [Pki-devel] [PATCH] 751 Added TPS UI for managing user roles.

2016-05-20 Thread Endi Sukma Dewata

On 5/19/2016 9:38 PM, Endi Sukma Dewata wrote:

The TPS UI has been modified to provide an interface to manage
the user roles.

The ErrorDialog was modified to handle both text and JSON error
responses.

https://fedorahosted.org/pki/ticket/2267


New patch attached. Fixed column label.

--
Endi S. Dewata
>From e60e0d0801877ef82c7c49aff36300489937c78e Mon Sep 17 00:00:00 2001
From: "Endi S. Dewata" 
Date: Thu, 19 May 2016 22:26:29 +0200
Subject: [PATCH] Added TPS UI for managing user roles.

The TPS UI has been modified to provide an interface to manage
the user roles.

The ErrorDialog was modified to handle both text and JSON error
responses.

https://fedorahosted.org/pki/ticket/2267
---
 base/server/share/webapps/pki/js/pki-ui.js |  48 +
 base/tps/shared/webapps/tps/js/token.js|  14 ++-
 base/tps/shared/webapps/tps/js/user.js | 111 -
 base/tps/shared/webapps/tps/ui/index.html  |  10 +-
 .../webapps/tps/ui/{user.html => user-roles.html}  |  64 
 base/tps/shared/webapps/tps/ui/user.html   |   1 +
 6 files changed, 168 insertions(+), 80 deletions(-)
 copy base/tps/shared/webapps/tps/ui/{user.html => user-roles.html} (67%)

diff --git a/base/server/share/webapps/pki/js/pki-ui.js b/base/server/share/webapps/pki/js/pki-ui.js
index 0b5cdb0ddbbc2ecbc5941602ecd6684c8ee626e7..a59a283d8d4e4fd5782fa42197384ad12c4ada5b 100644
--- a/base/server/share/webapps/pki/js/pki-ui.js
+++ b/base/server/share/webapps/pki/js/pki-ui.js
@@ -89,8 +89,9 @@ var Model = Backbone.Model.extend({
 
 var Collection = Backbone.Collection.extend({
 urlRoot: null,
-initialize: function(options) {
+initialize: function(models, options) {
 var self = this;
+Collection.__super__.initialize.call(self, models, options);
 
 self.options = options;
 self.links = {};
@@ -337,8 +338,19 @@ var ErrorDialog = Backbone.View.extend({
 var self = this;
 ErrorDialog.__super__.initialize.call(self, options);
 
-self.title = options.title;
-self.content = options.content;
+var response = options.response;
+if (response && response.responseJSON) {
+self.title = "HTTP Error " + response.responseJSON.Code;
+self.content = response.responseJSON.Message;
+
+} else if (response && response.responseText) {
+self.title = "HTTP Error " + response.status;
+self.content = response.responseText;
+
+} else {
+self.title = options.title;
+self.content = options.content;
+}
 },
 render: function() {
 var self = this;
@@ -830,8 +842,7 @@ var ModelTable = Table.extend({
 error: function(collection, response, options) {
 new ErrorDialog({
 el: $("#error-dialog"),
-title: "HTTP Error " + response.responseJSON.Code,
-content: response.responseJSON.Message
+response: response
 }).open();
 }
 });
@@ -851,6 +862,8 @@ var ModelTable = Table.extend({
 },
 totalEntries: function() {
 var self = this;
+if (!self.collection)
+return 0;
 return self.collection.total;
 },
 open: function(item) {
@@ -889,8 +902,7 @@ var ModelTable = Table.extend({
 }
 new ErrorDialog({
 el: $("#error-dialog"),
-title: "HTTP Error " + response.responseJSON.Code,
-content: response.responseJSON.Message
+response: response
 }).open();
 }
 });
@@ -904,8 +916,7 @@ var ModelTable = Table.extend({
 error: function(model, response, options) {
 new ErrorDialog({
 el: $("#error-dialog"),
-title: "HTTP Error " + response.responseJSON.Code,
-content: response.responseJSON.Message
+response: response
 }).open();
 }
 });
@@ -945,8 +956,7 @@ var ModelTable = Table.extend({
 }
 new ErrorDialog({
 el: $("#error-dialog"),
-title: "HTTP Error " + response.responseJSON.Code,
-content: response.responseJSON.Message
+response: response
 }).open();
 }
 });
@@ -968,8 +978,7 @@ var ModelTable = Table.extend({
 error: function(model, response, options) {
 new ErrorDialog({
 el: $("#error-dialog"),
-title: "HTTP Error " + response.responseJSON.Code,
-content: response.responseJSON.Message
+response: response
   

[Pki-devel] [PATCH] 752 Added TPS UI for managing user certificates.

2016-05-20 Thread Endi Sukma Dewata

The TPS UI has been modified to provide an interface to manage the
user certificates.

The UserService has been modified to provide better error messages.

https://fedorahosted.org/pki/ticket/1434

--
Endi S. Dewata
>From 6176ce70a64999d007dd4f6d91606a304a04278a Mon Sep 17 00:00:00 2001
From: "Endi S. Dewata" 
Date: Fri, 20 May 2016 19:31:45 +0200
Subject: [PATCH] Added TPS UI for managing user certificates.

The TPS UI has been modified to provide an interface to manage the
user certificates.

The UserService has been modified to provide better error messages.

https://fedorahosted.org/pki/ticket/1434
---
 .../src/org/dogtagpki/server/rest/UserService.java |  38 ---
 base/server/share/webapps/pki/js/pki-ui.js |   6 ++
 base/tps/shared/webapps/tps/js/user.js | 113 +
 base/tps/shared/webapps/tps/ui/index.html  |   8 ++
 .../webapps/tps/ui/{user.html => user-certs.html}  |  71 +
 base/tps/shared/webapps/tps/ui/user.html   |   1 +
 6 files changed, 176 insertions(+), 61 deletions(-)
 copy base/tps/shared/webapps/tps/ui/{user.html => user-certs.html} (65%)

diff --git a/base/server/cms/src/org/dogtagpki/server/rest/UserService.java b/base/server/cms/src/org/dogtagpki/server/rest/UserService.java
index 3de7384ee0dc8789e74b619191e7e4c4e739ae6f..0893c4bed36ed1485f5452085ca66953af4d3b53 100644
--- a/base/server/cms/src/org/dogtagpki/server/rest/UserService.java
+++ b/base/server/cms/src/org/dogtagpki/server/rest/UserService.java
@@ -858,6 +858,7 @@ public class UserService extends PKIService implements UserResource {
 cert = new X509CertImpl(binaryCert);
 
 } catch (CertificateException e) {
+CMS.debug("UserService: Submitted data is not an X.509 certificate: " + e);
 // ignore
 }
 
@@ -866,7 +867,7 @@ public class UserService extends PKIService implements UserResource {
 boolean assending = true;
 
 // could it be a pkcs7 blob?
-CMS.debug("UserCertResourceService: " + CMS.getLogMessage("ADMIN_SRVLT_IS_PK_BLOB"));
+CMS.debug("UserService: " + CMS.getLogMessage("ADMIN_SRVLT_IS_PK_BLOB"));
 
 try {
 CryptoManager manager = CryptoManager.getInstance();
@@ -876,7 +877,8 @@ public class UserService extends PKIService implements UserResource {
 X509Certificate p7certs[] = pkcs7.getCertificates();
 
 if (p7certs.length == 0) {
-throw new BadRequestException(getUserMessage("CMS_USRGRP_SRVLT_CERT_ERROR", headers));
+CMS.debug("UserService: PKCS #7 data contains no certificates");
+throw new BadRequestException("PKCS #7 data contains no certificates");
 }
 
 // fix for 370099 - cert ordering can not be assumed
@@ -888,24 +890,24 @@ public class UserService extends PKIService implements UserResource {
 p7certs[0].getIssuerDN().toString()) &&
 (p7certs.length == 1)) {
 cert = p7certs[0];
-CMS.debug("UserCertResourceService: " + CMS.getLogMessage("ADMIN_SRVLT_SINGLE_CERT_IMPORT"));
+CMS.debug("UserService: " + CMS.getLogMessage("ADMIN_SRVLT_SINGLE_CERT_IMPORT"));
 
 } else if (p7certs[0].getIssuerDN().toString().equals(p7certs[1].getSubjectDN().toString())) {
 cert = p7certs[0];
-CMS.debug("UserCertResourceService: " + CMS.getLogMessage("ADMIN_SRVLT_CERT_CHAIN_ACEND_ORD"));
+CMS.debug("UserService: " + CMS.getLogMessage("ADMIN_SRVLT_CERT_CHAIN_ACEND_ORD"));
 
 } else if (p7certs[1].getIssuerDN().toString().equals(p7certs[0].getSubjectDN().toString())) {
 assending = false;
-CMS.debug("UserCertResourceService: " + CMS.getLogMessage("ADMIN_SRVLT_CERT_CHAIN_DESC_ORD"));
+CMS.debug("UserService: " + CMS.getLogMessage("ADMIN_SRVLT_CERT_CHAIN_DESC_ORD"));
 cert = p7certs[p7certs.length - 1];
 
 } else {
 // not a chain, or in random order
-CMS.debug("UserCertResourceService: " + CMS.getLogMessage("ADMIN_SRVLT_CERT_BAD_CHAIN"));
+CMS.debug("UserService: " + CMS.getLogMessage("ADMIN_SRVLT_CERT_BAD_CHAIN"));
 throw new BadRequestException(getUserMessage("CMS_USRGRP_SRVLT_CERT_ERROR", headers));
 }
 
-CMS.debug("UserCertResourceService: "
+CMS.debug("UserService: "
 + CMS.getLogMessage("ADMIN_SRVLT_CHAIN_STORED_DB", String.valueOf(p7certs.length)));
 
 int j = 0;
@@ -922,16 +924,17 @@ public class 

[Pki-devel] [PATCH] 303-306 Various issues

2016-05-20 Thread Ade Lee
Please review:

Patches listed in reverse order (306 -> 303)

Ade

commit e3d47aabee97773832d2f8ac7ff138314b44f646
Author: Ade Lee 
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 
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 
Date:   Thu May 19 00:08:20 2016 -0400

Add parameters to purge old published files

Ticket 2254

commit 31342868aa4468fd7c2818727930932fd1e2d23e
Author: Ade Lee 
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 2275From e3d47aabee97773832d2f8ac7ff138314b44f646 Mon Sep 17 00:00:00 2001
From: Ade Lee 
Date: Thu, 19 May 2016 11:56:26 -0400
Subject: [PATCH 306/306] 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
---
 .../org/dogtagpki/server/ca/rest/CertService.java  | 24 +--
 .../src/com/netscape/certsrv/cert/CertData.java| 36 ++
 .../com/netscape/certsrv/cert/CertDataInfo.java| 33 
 .../src/com/netscape/cmstools/cert/CertCLI.java| 21 +
 4 files changed, 105 insertions(+), 9 deletions(-)

diff --git a/base/ca/src/org/dogtagpki/server/ca/rest/CertService.java b/base/ca/src/org/dogtagpki/server/ca/rest/CertService.java
index 2c5fa52b8e13f8c9bc033b9bc9a850e6220cef33..54a349e2a60c6fd7571c2cb43a0504d96050c11a 100644
--- a/base/ca/src/org/dogtagpki/server/ca/rest/CertService.java
+++ b/base/ca/src/org/dogtagpki/server/ca/rest/CertService.java
@@ -41,15 +41,6 @@ import javax.ws.rs.core.Request;
 import javax.ws.rs.core.Response;
 import javax.ws.rs.core.UriInfo;
 
-import netscape.security.pkcs.ContentInfo;
-import netscape.security.pkcs.PKCS7;
-import netscape.security.pkcs.SignerInfo;
-import netscape.security.provider.RSAPublicKey;
-import netscape.security.x509.AlgorithmId;
-import netscape.security.x509.RevocationReason;
-import netscape.security.x509.X509CertImpl;
-import netscape.security.x509.X509Key;
-
 import org.apache.catalina.realm.GenericPrincipal;
 import org.jboss.resteasy.plugins.providers.atom.Link;
 
@@ -84,6 +75,15 @@ import com.netscape.cms.servlet.processors.CAProcessor;
 import com.netscape.cmsutil.ldap.LDAPUtil;
 import com.netscape.cmsutil.util.Utils;
 
+import netscape.security.pkcs.ContentInfo;
+import netscape.security.pkcs.PKCS7;
+import netscape.security.pkcs.SignerInfo;
+import netscape.security.provider.RSAPublicKey;
+import netscape.security.x509.AlgorithmId;
+import netscape.security.x509.RevocationReason;
+import netscape.security.x509.X509CertImpl;
+import netscape.security.x509.X509Key;
+
 /**
  * @author alee
  *
@@ -527,6 +527,9 @@ public class CertService extends PKIService implements CertResource {
 Date notAfter = cert.getNotAfter();
 if (notAfter != null) certData.setNotAfter(notAfter.toString());
 
+certData.setRevokedOn(record.getRevokedOn());
+certData.setRevokedBy(record.getRevokedBy());
+
 certData.setStatus(record.getStatus());
 
 if (authority.noncesEnabled() && generateNonce) {
@@ -575,6 +578,9 @@ public class CertService extends PKIService implements CertResource {
 info.setIssuedOn(record.getCreateTime());
 info.setIssuedBy(record.getIssuedBy());
 
+info.setRevokedOn(record.getRevokedOn());
+info.setRevokedBy(record.getRevokedBy());
+
 URI uri = uriInfo.getBaseUriBuilder().path(CertResource.class, "getCert").build(id.toHexString());
 info.setLink(new Link("self", uri));
 
diff --git a/base/com