[FAB-7464] Don't use RevokedBefore if not set

If 'RevokedBefore' if not specified in the CRL request then don't use it in the database query that is used to retrieve the revoked certificates. If RevokedBefore is not specified then all certificates that were revoked after 'RevokedAfter' attribute are returned. Change-Id: If1b76d9c0eef6314729064185f3985b9c012b7ef Signed-off-by: Anil Ambati <aambati@us.ibm.com>
hyperledger · Jan 17, 2018 · e39b3e4 · e39b3e4
1 parent 673faef
commit e39b3e4
Show file tree

Hide file tree

Showing 3 changed files with 41 additions and 35 deletions.
diff --git a/cmd/fabric-ca-client/main_test.go b/cmd/fabric-ca-client/main_test.go
@@ -421,6 +421,19 @@ func TestGenCRL(t *testing.T) {
 	assert.NoError(t, err, "gencrl failed")
 	checkCRL(t, admin.GetClient(), revokedCertSerials)
 
+	// success case 6: gencrl invoked with --expireafter, --revokedbefore and --revokedafter args
+	err = RunMain([]string{cmdName, "gencrl", "--expireafter", time.Now().UTC().Format(time.RFC3339),
+		"--revokedafter", pastTime, "--revokedbefore", futureTime})
+	assert.NoError(t, err, "gencrl failed")
+	checkCRL(t, admin.GetClient(), revokedCertSerials)
+
+	// success case 6: gencrl invoked with all args
+	err = RunMain([]string{cmdName, "gencrl", "--expireafter", time.Now().UTC().Format(time.RFC3339),
+		"--expirebefore", time.Now().Add(time.Hour * 24 * 365 * 2).UTC().Format(time.RFC3339),
+		"--revokedafter", pastTime, "--revokedbefore", futureTime})
+	assert.NoError(t, err, "gencrl failed")
+	checkCRL(t, admin.GetClient(), revokedCertSerials)
+
 	// Error cases
 	// Error case 2: should fail when invoked with invalid --revokedafter arg
 	err = RunMain([]string{cmdName, "gencrl", "--revokedafter", "foo"})

diff --git a/lib/certdbaccessor.go b/lib/certdbaccessor.go
@@ -51,14 +51,6 @@ UPDATE certificates
 SET status='revoked', revoked_at=CURRENT_TIMESTAMP, reason=:reason
 WHERE (id = :id AND status != 'revoked');`
 
-	selectRevokedSQL = `
-SELECT %s FROM certificates
-	WHERE (expiry > ? AND status='revoked' AND revoked_at > ? AND revoked_at < ?);`
-
-	selectRevokedSQL1 = `
-SELECT %s FROM certificates
-	WHERE (expiry > ? AND expiry < ? AND status='revoked' AND revoked_at > ? AND revoked_at < ?);`
-
 	deleteCertificatebyID = `
 DELETE FROM certificates
 		WHERE (ID = ?);`
@@ -214,20 +206,24 @@ func (d *CertDBAccessor) GetRevokedCertificates(expiredAfter, expiredBefore, rev
 		return nil, err
 	}
 	var crs []certdb.CertificateRecord
-	if expiredBefore.IsZero() {
-		err = d.db.Select(&crs, fmt.Sprintf(d.db.Rebind(selectRevokedSQL),
-			sqlstruct.Columns(certdb.CertificateRecord{})), expiredAfter, revokedAfter, revokedBefore)
-		if err != nil {
-			return crs, getError(err, "Certificate")
-		}
-	} else {
-		err = d.db.Select(&crs, fmt.Sprintf(d.db.Rebind(selectRevokedSQL1),
-			sqlstruct.Columns(certdb.CertificateRecord{})), expiredAfter, expiredBefore, revokedAfter, revokedBefore)
-		if err != nil {
-			return crs, getError(err, "Certificate")
-		}
+	revokedSQL := "SELECT %s FROM certificates WHERE (WHERE_CLAUSE);"
+	whereConds := []string{"status='revoked' AND expiry > ? AND revoked_at > ?"}
+	args := []interface{}{expiredAfter, revokedAfter}
+	if !expiredBefore.IsZero() {
+		whereConds = append(whereConds, "expiry < ?")
+		args = append(args, expiredBefore)
+	}
+	if !revokedBefore.IsZero() {
+		whereConds = append(whereConds, "revoked_at < ?")
+		args = append(args, revokedBefore)
+	}
+	whereClause := strings.Join(whereConds, " AND ")
+	revokedSQL = strings.Replace(revokedSQL, "WHERE_CLAUSE", whereClause, 1)
+	err = d.db.Select(&crs, fmt.Sprintf(d.db.Rebind(revokedSQL),
+		sqlstruct.Columns(certdb.CertificateRecord{})), args...)
+	if err != nil {
+		return crs, getError(err, "Certificate")
 	}
-
 	return crs, nil
 }
 

diff --git a/lib/servergencrl.go b/lib/servergencrl.go
@@ -93,31 +93,27 @@ func genCRLHandler(ctx *serverRequestContext) (interface{}, error) {
 // GenCRL will generate CRL
 func genCRL(ca *CA, req api.GenCRLRequest) ([]byte, error) {
 	var err error
-	revokedBefore := req.RevokedBefore
-	if revokedBefore.IsZero() {
-		revokedBefore = time.Now().UTC()
-	}
-	if req.RevokedAfter.After(revokedBefore) {
+	if !req.RevokedBefore.IsZero() && req.RevokedAfter.After(req.RevokedBefore) {
 		return nil, newHTTPErr(400, ErrInvalidRevokedAfter,
 			"Invalid 'revokedafter' value. It must not be a timestamp greater than 'revokedbefore'")
 	}
 
-	if req.ExpireAfter.After(req.ExpireBefore) {
+	if !req.ExpireBefore.IsZero() && req.ExpireAfter.After(req.ExpireBefore) {
 		return nil, newHTTPErr(400, ErrInvalidExpiredAfter,
 			"Invalid 'expireafter' value. It must not be a timestamp greater than 'expirebefore'")
 	}
 
 	// Get revoked certificates from the database
-	certs, err := ca.certDBAccessor.GetRevokedCertificates(req.ExpireAfter, req.ExpireBefore, req.RevokedAfter, revokedBefore)
+	certs, err := ca.certDBAccessor.GetRevokedCertificates(req.ExpireAfter, req.ExpireBefore, req.RevokedAfter, req.RevokedBefore)
 	if err != nil {
-		return nil, newHTTPErr(500, ErrRevokedCertsFromDB,
-			"Failed to get revoked certificates from the database: %s", err)
+		log.Errorf("Failed to get revoked certificates from the database: %s", err)
+		return nil, newHTTPErr(500, ErrRevokedCertsFromDB, "Failed to get revoked certificates")
 	}
 
 	caCert, err := getCACert(ca)
 	if err != nil {
-		return nil, newHTTPErr(500, ErrGetCACert,
-			"Failed to get certficate for the CA '%s': %s", ca.HomeDir, err)
+		log.Errorf("Failed to get certficate for CA '%s': %s", ca.HomeDir, err)
+		return nil, newHTTPErr(500, ErrGetCACert, "Failed to get certficate for CA '%s'", ca.HomeDir)
 	}
 
 	if !canSignCRL(caCert) {
@@ -128,8 +124,8 @@ func genCRL(ca *CA, req api.GenCRLRequest) ([]byte, error) {
 	// Get the signer for the CA
 	_, signer, err := util.GetSignerFromCert(caCert, ca.csp)
 	if err != nil {
-		return nil, newHTTPErr(500, ErrGetCASigner,
-			"Failed to get signer for the CA '%s': %s", ca.HomeDir, err)
+		log.Errorf("Failed to get signer for CA '%s': %s", ca.HomeDir, err)
+		return nil, newHTTPErr(500, ErrGetCASigner, "Failed to get signer for CA '%s'", ca.HomeDir)
 	}
 
 	expiry := time.Now().UTC().Add(ca.Config.CRL.Expiry)
@@ -148,7 +144,8 @@ func genCRL(ca *CA, req api.GenCRLRequest) ([]byte, error) {
 
 	crl, err := crl.CreateGenericCRL(revokedCerts, signer, caCert, expiry)
 	if err != nil {
-		return nil, newHTTPErr(500, ErrGenCRL, "Failed to generate the CRL for the CA '%s': %s", ca.HomeDir, err)
+		log.Errorf("Failed to generate CRL for CA '%s': %s", ca.HomeDir, err)
+		return nil, newHTTPErr(500, ErrGenCRL, "Failed to generate CRL for CA '%s'", ca.HomeDir)
 	}
 	blk := &pem.Block{Bytes: crl, Type: crlPemType}
 	return pem.EncodeToMemory(blk), nil