[FABC-459] Optimize GetCertificate request

Created a new function that simplifies the
get certificate request by doing error checking
in a single function. Callers of this function
just have to check for error, rather having to check
to see the correct number of certificate were returned.

Also, improved the modularity of the code by creating
an independent package for the certificate request code.

Change-Id: Ibca3fb78207bb5f441ce6c32ff00467464ee8448
Signed-off-by: Saad Karim <[email protected]>

lib/ca.go

@@ -22,6 +22,7 @@ import (
 	"sync"
 	"time"
 
+	"github.com/cloudflare/cfssl/certdb"
 	"github.com/cloudflare/cfssl/config"
 	cfcsr "github.com/cloudflare/cfssl/csr"
 	"github.com/cloudflare/cfssl/initca"
@@ -870,6 +871,22 @@ func (ca *CA) GetDB() *dbutil.DB {
 	return ca.db
 }
 
+// GetCertificate returns a single certificate matching serial and aki, if multiple certificates
+// found for serial and aki an error is returned
+func (ca *CA) GetCertificate(serial, aki string) (*certdb.CertificateRecord, error) {
+	certs, err := ca.CertDBAccessor().GetCertificate(serial, aki)
+	if err != nil {
+		return nil, caerrors.NewHTTPErr(500, caerrors.ErrCertNotFound, "Failed searching certificates: %s", err)
+	}
+	if len(certs) == 0 {
+		return nil, caerrors.NewAuthenticationErr(caerrors.ErrCertNotFound, "Certificate not found with AKI '%s' and serial '%s'", aki, serial)
+	}
+	if len(certs) > 1 {
+		return nil, caerrors.NewAuthenticationErr(caerrors.ErrCertNotFound, "Multiple certificates found, when only should exist with AKI '%s' and serial '%s' combination", aki, serial)
+	}
+	return &certs[0], nil
+}
+
 // Make all file names in the CA config absolute
 func (ca *CA) makeFileNamesAbsolute() error {
 	log.Debug("Making CA filenames absolute")

lib/certdbaccessor.go

@@ -1,17 +1,7 @@
 /*
-Copyright IBM Corp. 2016 All Rights Reserved.
+Copyright IBM Corp. All Rights Reserved.
 
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-                 http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
+SPDX-License-Identifier: Apache-2.0
 */
 
 package lib
@@ -26,7 +16,7 @@ import (
 	certsql "github.com/cloudflare/cfssl/certdb/sql"
 	"github.com/cloudflare/cfssl/log"
 	"github.com/hyperledger/fabric-ca/lib/dbutil"
-	"github.com/hyperledger/fabric-ca/lib/server"
+	cr "github.com/hyperledger/fabric-ca/lib/server/certificaterequest"
 	"github.com/hyperledger/fabric-ca/util"
 	"github.com/jmoiron/sqlx"
 	"github.com/kisielk/sqlstruct"
@@ -306,7 +296,7 @@ func (d *CertDBAccessor) UpsertOCSP(serial, aki, body string, expiry time.Time)
 }
 
 // GetCertificates returns based on filter parameters certificates
-func (d *CertDBAccessor) GetCertificates(req server.CertificateRequest, callersAffiliation string) (*sqlx.Rows, error) {
+func (d *CertDBAccessor) GetCertificates(req cr.CertificateRequest, callersAffiliation string) (*sqlx.Rows, error) {
 	log.Debugf("DB: Get Certificates")
 
 	err := d.checkDB()

lib/certdbaccessor_test.go

@@ -1,17 +1,7 @@
 /*
-Copyright IBM Corp. 2018 All Rights Reserved.
+Copyright IBM Corp. All Rights Reserved.
 
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-                 http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
+SPDX-License-Identifier: Apache-2.0
 */
 
 package lib
@@ -31,7 +21,7 @@ import (
 	"github.com/cloudflare/cfssl/certdb"
 	"github.com/cloudflare/cfssl/log"
 	"github.com/hyperledger/fabric-ca/lib/dbutil"
-	"github.com/hyperledger/fabric-ca/lib/server"
+	"github.com/hyperledger/fabric-ca/lib/server/certificaterequest"
 	"github.com/hyperledger/fabric-ca/lib/spi"
 	"github.com/hyperledger/fabric-ca/util"
 	"github.com/jmoiron/sqlx"
@@ -303,8 +293,8 @@ func testInsertCertificate(req *certdb.CertificateRecord, id string, ca *CA) err
 	return err
 }
 
-func getCertReq(id, serial, aki string, notrevoked, notexpired bool, revokedTimeStart, revokedTimeEnd, expiredTimeStart, expiredTimeEnd *time.Time) *server.CertificateRequestImpl {
-	return &server.CertificateRequestImpl{
+func getCertReq(id, serial, aki string, notrevoked, notexpired bool, revokedTimeStart, revokedTimeEnd, expiredTimeStart, expiredTimeEnd *time.Time) *certificaterequest.Impl {
+	return &certificaterequest.Impl{
 		ID:               id,
 		SerialNumber:     serial,
 		Aki:              aki,

lib/server/certificaterequest.go → lib/server/certificaterequest/certificaterequest.go

@@ -1,20 +1,10 @@
 /*
-Copyright IBM Corp. 2017 All Rights Reserved.
+Copyright IBM Corp. All Rights Reserved.
 
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-                 http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
+SPDX-License-Identifier: Apache-2.0
 */
 
-package server
+package certificaterequest
 
 import (
 	"regexp"
@@ -46,8 +36,8 @@ type RequestContext interface {
 	GetBoolQueryParm(string) (bool, error)
 }
 
-// CertificateRequestImpl defines the properties of a certificate request
-type CertificateRequestImpl struct {
+// Impl defines the properties of a certificate request
+type Impl struct {
 	ID               string
 	SerialNumber     string
 	Aki              string
@@ -68,7 +58,7 @@ type TimeFilters struct {
 }
 
 // NewCertificateRequest returns a certificate request object
-func NewCertificateRequest(ctx RequestContext) (*CertificateRequestImpl, error) {
+func NewCertificateRequest(ctx RequestContext) (*Impl, error) {
 
 	// Convert time string to time type
 	times, err := getTimes(ctx)
@@ -88,7 +78,7 @@ func NewCertificateRequest(ctx RequestContext) (*CertificateRequestImpl, error)
 		return nil, err
 	}
 
-	return &CertificateRequestImpl{
+	return &Impl{
 		ID:               req.ID,
 		SerialNumber:     req.Serial,
 		Aki:              req.AKI,
@@ -102,47 +92,47 @@ func NewCertificateRequest(ctx RequestContext) (*CertificateRequestImpl, error)
 }
 
 // GetID returns the enrollment id filter value
-func (c *CertificateRequestImpl) GetID() string {
+func (c *Impl) GetID() string {
 	return c.ID
 }
 
 // GetSerial returns the serial number filter value
-func (c *CertificateRequestImpl) GetSerial() string {
+func (c *Impl) GetSerial() string {
 	return c.SerialNumber
 }
 
 // GetAKI returns the AKI filter value
-func (c *CertificateRequestImpl) GetAKI() string {
+func (c *Impl) GetAKI() string {
 	return c.Aki
 }
 
 // GetNotExpired returns the notexpired bool value
-func (c *CertificateRequestImpl) GetNotExpired() bool {
+func (c *Impl) GetNotExpired() bool {
 	return c.Notexpired
 }
 
 // GetNotRevoked returns the notrevoked bool value
-func (c *CertificateRequestImpl) GetNotRevoked() bool {
+func (c *Impl) GetNotRevoked() bool {
 	return c.Notrevoked
 }
 
 // GetExpiredTimeStart returns the starting expiration time filter value
-func (c *CertificateRequestImpl) GetExpiredTimeStart() *time.Time {
+func (c *Impl) GetExpiredTimeStart() *time.Time {
 	return c.ExpiredTimeStart
 }
 
 // GetExpiredTimeEnd returns the ending expiration time filter value
-func (c *CertificateRequestImpl) GetExpiredTimeEnd() *time.Time {
+func (c *Impl) GetExpiredTimeEnd() *time.Time {
 	return c.ExpiredTimeEnd
 }
 
 // GetRevokedTimeStart returns the starting revoked time filter value
-func (c *CertificateRequestImpl) GetRevokedTimeStart() *time.Time {
+func (c *Impl) GetRevokedTimeStart() *time.Time {
 	return c.RevokedTimeStart
 }
 
 // GetRevokedTimeEnd returns the ending revoked time filter value
-func (c *CertificateRequestImpl) GetRevokedTimeEnd() *time.Time {
+func (c *Impl) GetRevokedTimeEnd() *time.Time {
 	return c.RevokedTimeEnd
 }
 

lib/server/certificaterequest_test.go → lib/server/certificaterequest/certificaterequest_test.go

@@ -1,20 +1,10 @@
 /*
-Copyright IBM Corp. 2017 All Rights Reserved.
+Copyright IBM Corp. All Rights Reserved.
 
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-                 http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
+SPDX-License-Identifier: Apache-2.0
 */
 
-package server
+package certificaterequest
 
 import (
 	//	"bytes"
@@ -24,7 +14,7 @@ import (
 	"time"
 
 	"github.com/hyperledger/fabric-ca/api"
-	"github.com/hyperledger/fabric-ca/lib/server/mocks"
+	"github.com/hyperledger/fabric-ca/lib/server/certificaterequest/mocks"
 	"github.com/hyperledger/fabric-ca/util"
 	"github.com/stretchr/testify/assert"
 )

lib/servercertificates.go

@@ -13,7 +13,7 @@ import (
 
 	"github.com/cloudflare/cfssl/log"
 	"github.com/hyperledger/fabric-ca/lib/caerrors"
-	"github.com/hyperledger/fabric-ca/lib/server"
+	"github.com/hyperledger/fabric-ca/lib/server/certificaterequest"
 	"github.com/hyperledger/fabric-ca/util"
 	"github.com/pkg/errors"
 )
@@ -95,7 +95,7 @@ func processGetCertificateRequest(ctx ServerRequestContext) error {
 	log.Debug("Processing GET certificate request")
 	var err error
 
-	req, err := server.NewCertificateRequest(ctx)
+	req, err := certificaterequest.NewCertificateRequest(ctx)
 	if err != nil {
 		return caerrors.NewHTTPErr(400, caerrors.ErrGettingCert, "Invalid Request: %s", err)
 	}
@@ -110,7 +110,7 @@ func processGetCertificateRequest(ctx ServerRequestContext) error {
 }
 
 // getCertificates executes the DB query and streams the results to client
-func getCertificates(ctx ServerRequestContext, req *server.CertificateRequestImpl) error {
+func getCertificates(ctx ServerRequestContext, req *certificaterequest.Impl) error {
 	w := ctx.GetResp()
 	flusher, _ := w.(http.Flusher)
 

lib/servercertificates_test.go

@@ -22,7 +22,7 @@ import (
 	"github.com/hyperledger/fabric-ca/lib/caerrors"
 	"github.com/hyperledger/fabric-ca/lib/dbutil"
 	"github.com/hyperledger/fabric-ca/lib/mocks"
-	"github.com/hyperledger/fabric-ca/lib/server"
+	"github.com/hyperledger/fabric-ca/lib/server/certificaterequest"
 	"github.com/hyperledger/fabric-ca/util"
 	"github.com/pkg/errors"
 	"github.com/stretchr/testify/assert"
@@ -232,7 +232,7 @@ func TestServerGetCertificates(t *testing.T) {
 	}, "testCertificate", ca)
 	util.FatalError(t, err, "Failed to insert certificate with serial/AKI")
 
-	err = getCertificates(ctx, &server.CertificateRequestImpl{})
+	err = getCertificates(ctx, &certificaterequest.Impl{})
 	assert.NoError(t, err, "Should not have returned error, failed to process GET certificate request")
 
 	mockCtx := new(mocks.ServerRequestContext)
@@ -245,7 +245,7 @@ func TestServerGetCertificates(t *testing.T) {
 	mockCtx = new(mocks.ServerRequestContext)
 	mockCtx.On("GetResp").Return(nil)
 	mockCtx.On("GetCaller").Return(testUser, nil)
-	mockCtx.On("GetCertificates", (*server.CertificateRequestImpl)(nil), "").Return(nil, errors.New("failed to get certificates"))
+	mockCtx.On("GetCertificates", (*certificaterequest.Impl)(nil), "").Return(nil, errors.New("failed to get certificates"))
 	err = getCertificates(mockCtx, nil)
 	util.ErrorContains(t, err, "failed to get certificates", "did not get correct error response")
 }

lib/serverrequestcontext.go

@@ -24,7 +24,7 @@ import (
 	"github.com/hyperledger/fabric-ca/api"
 	"github.com/hyperledger/fabric-ca/lib/attr"
 	"github.com/hyperledger/fabric-ca/lib/caerrors"
-	"github.com/hyperledger/fabric-ca/lib/server"
+	cr "github.com/hyperledger/fabric-ca/lib/server/certificaterequest"
 	"github.com/hyperledger/fabric-ca/lib/server/idemix"
 	"github.com/hyperledger/fabric-ca/lib/spi"
 	"github.com/hyperledger/fabric-ca/util"
@@ -44,7 +44,7 @@ type ServerRequestContext interface {
 	GetQueryParm(name string) string
 	GetBoolQueryParm(name string) (bool, error)
 	GetResp() http.ResponseWriter
-	GetCertificates(server.CertificateRequest, string) (*sqlx.Rows, error)
+	GetCertificates(cr.CertificateRequest, string) (*sqlx.Rows, error)
 	IsLDAPEnabled() bool
 	ReadBody(interface{}) error
 	ContainsAffiliation(string) error
@@ -203,18 +203,15 @@ func (ctx *serverRequestContextImpl) verifyX509Token(ca *CA, authHdr string, bod
 	serial := util.GetSerialAsHex(cert.SerialNumber)
 	aki = strings.ToLower(strings.TrimLeft(aki, "0"))
 	serial = strings.ToLower(strings.TrimLeft(serial, "0"))
-	certs, err := ca.CertDBAccessor().GetCertificate(serial, aki)
+
+	certificate, err := ca.GetCertificate(serial, aki)
 	if err != nil {
-		return "", caerrors.NewHTTPErr(500, caerrors.ErrCertNotFound, "Failed searching certificates: %s", err)
+		return "", err
 	}
-	if len(certs) == 0 {
-		return "", caerrors.NewAuthenticationErr(caerrors.ErrCertNotFound, "Certificate not found with AKI '%s' and serial '%s'", aki, serial)
-	}
-	for _, certificate := range certs {
-		if certificate.Status == "revoked" {
-			return "", caerrors.NewAuthenticationErr(caerrors.ErrCertRevoked, "The certificate in the authorization header is a revoked certificate")
-		}
+	if certificate.Status == "revoked" {
+		return "", caerrors.NewAuthenticationErr(caerrors.ErrCertRevoked, "The certificate in the authorization header is a revoked certificate")
 	}
+
 	ctx.enrollmentID = id
 	ctx.enrollmentCert = cert
 	ctx.caller, err = ctx.GetCaller()
@@ -680,7 +677,7 @@ func (ctx *serverRequestContextImpl) GetResp() http.ResponseWriter {
 }
 
 // GetCertificates executes the DB query to get back certificates based on the filters passed in
-func (ctx *serverRequestContextImpl) GetCertificates(req server.CertificateRequest, callerAff string) (*sqlx.Rows, error) {
+func (ctx *serverRequestContextImpl) GetCertificates(req cr.CertificateRequest, callerAff string) (*sqlx.Rows, error) {
 	return ctx.ca.certDBAccessor.GetCertificates(req, callerAff)
 }