Address (most) PR comments

authority/authority.go

@@ -466,34 +466,29 @@ func (a *Authority) init() error {
 	// TODO: decide if this is a good approach for providing the SCEP functionality
 	// It currently mirrors the logic for the x509CAService
 	if a.requiresSCEPService() && a.scepService == nil {
-		var options casapi.Options
+		var options scep.Options
-		if a.config.AuthorityConfig.Options != nil {
-			options = *a.config.AuthorityConfig.Options
-		}
 
 		// Read intermediate and create X509 signer and decrypter for default CAS.
-		if options.Is(casapi.SoftCAS) {
+		options.CertificateChain, err = pemutil.ReadCertificateBundle(a.config.IntermediateCert)
-			options.CertificateChain, err = pemutil.ReadCertificateBundle(a.config.IntermediateCert)
+		if err != nil {
-			if err != nil {
+			return err
-				return err
+		}
-			}
+		options.Signer, err = a.keyManager.CreateSigner(&kmsapi.CreateSignerRequest{
-			options.Signer, err = a.keyManager.CreateSigner(&kmsapi.CreateSignerRequest{
+			SigningKey: a.config.IntermediateKey,
-				SigningKey: a.config.IntermediateKey,
+			Password:   []byte(a.config.Password),
-				Password:   []byte(a.config.Password),
+		})
+		if err != nil {
+			return err
+		}
+
+		if km, ok := a.keyManager.(kmsapi.Decrypter); ok {
+			options.Decrypter, err = km.CreateDecrypter(&kmsapi.CreateDecrypterRequest{
+				DecryptionKey: a.config.IntermediateKey,
+				Password:      []byte(a.config.Password),
 			})
 			if err != nil {
 				return err
 			}
-
-			if km, ok := a.keyManager.(kmsapi.Decrypter); ok {
-				options.Decrypter, err = km.CreateDecrypter(&kmsapi.CreateDecrypterRequest{
-					DecryptionKey: a.config.IntermediateKey,
-					Password:      []byte(a.config.Password),
-				})
-				if err != nil {
-					return err
-				}
-			}
 		}
 
 		a.scepService, err = scep.NewService(context.Background(), options)

authority/provisioner/scep.go

@@ -102,9 +102,3 @@ func (s *SCEP) GetChallengePassword() string {
 func (s *SCEP) GetCapabilities() []string {
 	return s.Capabilities
 }
-
-// Interface guards
-var (
-	_ Interface = (*SCEP)(nil)
-	//_ scep.Provisioner = (*SCEP)(nil)
-)

ca/ca.go

@@ -235,6 +235,15 @@ func (ca *CA) Init(config *config.Config) (*CA, error) {
 
 	ca.auth = auth
 	ca.srv = server.New(config.Address, handler, tlsConfig)
+
+	// TODO: instead opt for having a single server.Server but two
+	// http.Servers handling the HTTP and HTTPS handler? The latter
+	// will probably introduce more complexity in terms of graceful
+	// reload.
+	if config.InsecureAddress != "" {
+		ca.insecureSrv = server.New(config.InsecureAddress, insecureHandler, nil)
+	}
+
 	return ca, nil
 }
 

kms/apiv1/options.go

@@ -16,6 +16,8 @@ type KeyManager interface {
 	Close() error
 }
 
+// Decrypter is an interface implemented by KMSes that are used
+// in operations that require decryption
 type Decrypter interface {
 	CreateDecrypter(req *CreateDecrypterRequest) (crypto.Decrypter, error)
 }

kms/apiv1/requests.go

@@ -138,9 +138,6 @@ type CreateDecrypterRequest struct {
 	Decrypter        crypto.Decrypter
 	DecryptionKey    string
 	DecryptionKeyPEM []byte
-	TokenLabel       string
-	PublicKey        string
-	PublicKeyPEM     []byte
 	Password         []byte
 }
 

scep/api/api.go

@@ -4,8 +4,6 @@ import (
 	"context"
 	"crypto/x509"
 	"encoding/base64"
-	"errors"
-	"fmt"
 	"io"
 	"io/ioutil"
 	"net/http"
@@ -18,6 +16,8 @@ import (
 	"github.com/smallstep/certificates/scep"
 	"go.mozilla.org/pkcs7"
 
+	"github.com/pkg/errors"
+
 	microscep "github.com/micromdm/scep/scep"
 )
 
@@ -75,7 +75,7 @@ func (h *Handler) Get(w http.ResponseWriter, r *http.Request) {
 
 	request, err := decodeSCEPRequest(r)
 	if err != nil {
-		writeError(w, fmt.Errorf("not a scep get request: %w", err))
+		writeError(w, errors.Wrap(err, "not a scep get request"))
 		return
 	}
 
@@ -90,11 +90,11 @@ func (h *Handler) Get(w http.ResponseWriter, r *http.Request) {
 	case opnPKIOperation:
 		// TODO: implement the GET for PKI operation? Default CACAPS doesn't specify this is in use, though
 	default:
-		err = fmt.Errorf("unknown operation: %s", request.Operation)
+		err = errors.Errorf("unknown operation: %s", request.Operation)
 	}
 
 	if err != nil {
-		writeError(w, fmt.Errorf("get request failed: %w", err))
+		writeError(w, errors.Wrap(err, "get request failed"))
 		return
 	}
 
@@ -106,7 +106,7 @@ func (h *Handler) Post(w http.ResponseWriter, r *http.Request) {
 
 	request, err := decodeSCEPRequest(r)
 	if err != nil {
-		writeError(w, fmt.Errorf("not a scep post request: %w", err))
+		writeError(w, errors.Wrap(err, "not a scep post request"))
 		return
 	}
 
@@ -117,11 +117,11 @@ func (h *Handler) Post(w http.ResponseWriter, r *http.Request) {
 	case opnPKIOperation:
 		response, err = h.PKIOperation(ctx, request)
 	default:
-		err = fmt.Errorf("unknown operation: %s", request.Operation)
+		err = errors.Errorf("unknown operation: %s", request.Operation)
 	}
 
 	if err != nil {
-		writeError(w, fmt.Errorf("post request failed: %w", err))
+		writeError(w, errors.Wrap(err, "post request failed"))
 		return
 	}
 
@@ -163,7 +163,7 @@ func decodeSCEPRequest(r *http.Request) (SCEPRequest, error) {
 				Message:   decodedMessage,
 			}, nil
 		default:
-			return SCEPRequest{}, fmt.Errorf("unsupported operation: %s", operation)
+			return SCEPRequest{}, errors.Errorf("unsupported operation: %s", operation)
 		}
 	case http.MethodPost:
 		body, err := ioutil.ReadAll(io.LimitReader(r.Body, maxPayloadSize))
@@ -175,7 +175,7 @@ func decodeSCEPRequest(r *http.Request) (SCEPRequest, error) {
 			Message:   body,
 		}, nil
 	default:
-		return SCEPRequest{}, fmt.Errorf("unsupported method: %s", method)
+		return SCEPRequest{}, errors.Errorf("unsupported method: %s", method)
 	}
 }
 
@@ -187,7 +187,7 @@ func (h *Handler) lookupProvisioner(next nextHTTP) nextHTTP {
 		name := chi.URLParam(r, "provisionerID")
 		provisionerID, err := url.PathUnescape(name)
 		if err != nil {
-			api.WriteError(w, fmt.Errorf("error url unescaping provisioner id '%s'", name))
+			api.WriteError(w, errors.Errorf("error url unescaping provisioner id '%s'", name))
 			return
 		}
 
@@ -229,6 +229,9 @@ func (h *Handler) GetCACert(ctx context.Context) (SCEPResponse, error) {
 	if len(certs) == 1 {
 		response.Data = certs[0].Raw
 	} else {
+		// create degenerate pkcs7 certificate structure, according to
+		// https://tools.ietf.org/html/rfc8894#section-4.2.1.2, because
+		// not signed or encrypted data has to be returned.
 		data, err := microscep.DegenerateCertificates(certs)
 		if err != nil {
 			return SCEPResponse{}, err
@@ -329,12 +332,11 @@ func writeSCEPResponse(w http.ResponseWriter, response SCEPResponse) {
 	w.Header().Set("Content-Type", contentHeader(response))
 	_, err := w.Write(response.Data)
 	if err != nil {
-		writeError(w, fmt.Errorf("error when writing scep response: %w", err)) // This could end up as an error again
+		writeError(w, errors.Wrap(err, "error when writing scep response")) // This could end up as an error again
 	}
 }
 
 func writeError(w http.ResponseWriter, err error) {
-	// TODO: this probably needs to use SCEP specific errors (i.e. failInfo)
 	scepError := &scep.Error{
 		Message: err.Error(),
 		Status:  http.StatusInternalServerError, // TODO: make this a param?

scep/authority.go

@@ -1,32 +1,24 @@
 package scep
 
 import (
-	"bytes"
 	"context"
+	"crypto/subtle"
 	"crypto/x509"
-	"errors"
 	"fmt"
 	"net/url"
 
 	"github.com/smallstep/certificates/authority/provisioner"
-	database "github.com/smallstep/certificates/db"
-
-	"github.com/smallstep/nosql"
 
 	microx509util "github.com/micromdm/scep/crypto/x509util"
 	microscep "github.com/micromdm/scep/scep"
 
-	//"github.com/smallstep/certificates/scep/pkcs7"
+	"github.com/pkg/errors"
 
 	"go.mozilla.org/pkcs7"
 
 	"go.step.sm/crypto/x509util"
 )
 
-var (
-	certTable = []byte("scep_certs")
-)
-
 // Interface is the SCEP authority interface.
 type Interface interface {
 	LoadProvisionerByID(string) (provisioner.Interface, error)
@@ -42,28 +34,21 @@ type Interface interface {
 
 // Authority is the layer that handles all SCEP interactions.
 type Authority struct {
-	backdate provisioner.Duration
+	db                      DB
-	db       nosql.DB
+	prefix                  string
-	prefix   string
+	dns                     string
-	dns      string
-
-	// dir      *directory
-
 	intermediateCertificate *x509.Certificate
-
+	service                 *Service
-	service  *Service
+	signAuth                SignAuthority
-	signAuth SignAuthority
 }
 
 // AuthorityOptions required to create a new SCEP Authority.
 type AuthorityOptions struct {
-	// Service provides the SCEP functions to Authority
+	// Service provides the certificate chain, the signer and the decrypter to the Authority
 	Service *Service
-	// Backdate
+	// DB is the database used by SCEP
-	Backdate provisioner.Duration
+	DB DB
-	// DB is the database used by nosql.
+	// DNS is the host used to generate accurate SCEP links. By default the authority
-	DB nosql.DB
-	// DNS the host used to generate accurate SCEP links. By default the authority
 	// will use the Host from the request, so this value will only be used if
 	// request.Host is empty.
 	DNS string
@@ -81,19 +66,7 @@ type SignAuthority interface {
 // New returns a new Authority that implements the SCEP interface.
 func New(signAuth SignAuthority, ops AuthorityOptions) (*Authority, error) {
 
-	if _, ok := ops.DB.(*database.SimpleDB); !ok {
-		// If it's not a SimpleDB then go ahead and bootstrap the DB with the
-		// necessary SCEP tables. SimpleDB should ONLY be used for testing.
-		tables := [][]byte{certTable}
-		for _, b := range tables {
-			if err := ops.DB.CreateTable(b); err != nil {
-				return nil, fmt.Errorf("%w: error creating table %s", err, string(b))
-			}
-		}
-	}
-
 	authority := &Authority{
-		backdate: ops.Backdate,
 		db:       ops.DB,
 		prefix:   ops.Prefix,
 		dns:      ops.DNS,
@@ -102,7 +75,7 @@ func New(signAuth SignAuthority, ops AuthorityOptions) (*Authority, error) {
 
 	// TODO: this is not really nice to do; the Service should be removed
 	// in its entirety to make this more interoperable with the rest of
-	// step-ca.
+	// step-ca, I think.
 	if ops.Service != nil {
 		authority.intermediateCertificate = ops.Service.certificateChain[0]
 		authority.service = ops.Service
@@ -200,12 +173,12 @@ func (a *Authority) DecryptPKIEnvelope(ctx context.Context, msg *PKIMessage) err
 
 	p7c, err := pkcs7.Parse(msg.P7.Content)
 	if err != nil {
-		return err
+		return errors.Wrap(err, "error parsing pkcs7 content")
 	}
 
 	envelope, err := p7c.Decrypt(a.intermediateCertificate, a.service.decrypter)
 	if err != nil {
-		return err
+		return errors.Wrap(err, "error decrypting encrypted pkcs7 content")
 	}
 
 	msg.pkiEnvelope = envelope
@@ -214,19 +187,19 @@ func (a *Authority) DecryptPKIEnvelope(ctx context.Context, msg *PKIMessage) err
 	case microscep.CertRep:
 		certs, err := microscep.CACerts(msg.pkiEnvelope)
 		if err != nil {
-			return err
+			return errors.Wrap(err, "error extracting CA certs from pkcs7 degenerate data")
 		}
-		msg.CertRepMessage.Certificate = certs[0] // TODO: check correctness of this
+		msg.CertRepMessage.Certificate = certs[0]
 		return nil
 	case microscep.PKCSReq, microscep.UpdateReq, microscep.RenewalReq:
 		csr, err := x509.ParseCertificateRequest(msg.pkiEnvelope)
 		if err != nil {
-			return fmt.Errorf("parse CSR from pkiEnvelope: %w", err)
+			return errors.Wrap(err, "parse CSR from pkiEnvelope")
 		}
 		// check for challengePassword
 		cp, err := microx509util.ParseChallengePassword(msg.pkiEnvelope)
 		if err != nil {
-			return fmt.Errorf("scep: parse challenge password in pkiEnvelope: %w", err)
+			return errors.Wrap(err, "parse challenge password in pkiEnvelope")
 		}
 		msg.CSRReqMessage = &microscep.CSRReqMessage{
 			RawDecrypted:      msg.pkiEnvelope,
@@ -235,7 +208,7 @@ func (a *Authority) DecryptPKIEnvelope(ctx context.Context, msg *PKIMessage) err
 		}
 		return nil
 	case microscep.GetCRL, microscep.GetCert, microscep.CertPoll:
-		return fmt.Errorf("not implemented") //errNotImplemented
+		return errors.Errorf("not implemented")
 	}
 
 	return nil
@@ -266,16 +239,13 @@ func (a *Authority) SignCSR(ctx context.Context, csr *x509.CertificateRequest, m
 	}
 
 	// Template data
-	data := x509util.NewTemplateData()
+	data := x509util.CreateTemplateData(csr.Subject.CommonName, csr.DNSNames)
-	data.SetCommonName(csr.Subject.CommonName)
-	data.SetSANs(csr.DNSNames)
-	data.SetCertificateRequest(csr)
 
 	// Get authorizations from the SCEP provisioner.
 	ctx = provisioner.NewContextWithMethod(ctx, provisioner.SignMethod)
 	signOps, err := p.AuthorizeSign(ctx, "")
 	if err != nil {
-		return nil, fmt.Errorf("error retrieving authorization options from SCEP provisioner: %w", err)
+		return nil, errors.Wrap(err, "error retrieving authorization options from SCEP provisioner")
 	}
 
 	opts := provisioner.SignOptions{
@@ -285,19 +255,20 @@ func (a *Authority) SignCSR(ctx context.Context, csr *x509.CertificateRequest, m
 
 	templateOptions, err := provisioner.TemplateOptions(p.GetOptions(), data)
 	if err != nil {
-		return nil, fmt.Errorf("error creating template options from SCEP provisioner: %w", err)
+		return nil, errors.Wrap(err, "error creating template options from SCEP provisioner")
 	}
 	signOps = append(signOps, templateOptions)
 
 	certChain, err := a.signAuth.Sign(csr, opts, signOps...)
 	if err != nil {
-		return nil, fmt.Errorf("error generating certificate for order %w", err)
+		return nil, errors.Wrap(err, "error generating certificate for order")
 	}
 
+	// take the issued certificate (only); https://tools.ietf.org/html/rfc8894#section-3.3.2
 	cert := certChain[0]
 
-	// create a degenerate cert structure
+	// and create a degenerate cert structure
-	deg, err := degenerateCertificates([]*x509.Certificate{cert})
+	deg, err := microscep.DegenerateCertificates([]*x509.Certificate{cert})
 	if err != nil {
 		return nil, err
 	}
@@ -370,13 +341,12 @@ func (a *Authority) SignCSR(ctx context.Context, csr *x509.CertificateRequest, m
 		CertRepMessage: cr,
 	}
 
-	// TODO: save more data?
+	// store the newly created certificate
-	_, err = newCert(a.db, CertOptions{
+	err = newCert(a.db, CertOptions{
 		Leaf:          certChain[0],
 		Intermediates: certChain[1:],
 	})
 	if err != nil {
-		fmt.Println(err)
 		return nil, err
 	}
 
@@ -459,7 +429,7 @@ func (a *Authority) MatchChallengePassword(ctx context.Context, password string)
 		return false, err
 	}
 
-	if p.GetChallengePassword() == password {
+	if subtle.ConstantTimeCompare([]byte(p.GetChallengePassword()), []byte(password)) == 1 {
 		return true, nil
 	}
 
@@ -491,21 +461,3 @@ func (a *Authority) GetCACaps(ctx context.Context) []string {
 
 	return caps
 }
-
-// degenerateCertificates creates degenerate certificates pkcs#7 type
-func degenerateCertificates(certs []*x509.Certificate) ([]byte, error) {
-	var buf bytes.Buffer
-	for _, cert := range certs {
-		buf.Write(cert.Raw)
-	}
-	degenerate, err := pkcs7.DegenerateCertificate(buf.Bytes())
-	if err != nil {
-		return nil, err
-	}
-	return degenerate, nil
-}
-
-// Interface guards
-var (
-	_ Interface = (*Authority)(nil)
-)

scep/certificate.go

@@ -2,79 +2,20 @@ package scep
 
 import (
 	"crypto/x509"
-	"encoding/json"
-	"encoding/pem"
-	"fmt"
-	"time"
 
-	"github.com/smallstep/nosql"
+	"github.com/pkg/errors"
 )
 
-type certificate struct {
-	ID            string    `json:"id"`
-	Created       time.Time `json:"created"`
-	Leaf          []byte    `json:"leaf"`
-	Intermediates []byte    `json:"intermediates"`
-}
-
 // CertOptions options with which to create and store a cert object.
 type CertOptions struct {
 	Leaf          *x509.Certificate
 	Intermediates []*x509.Certificate
 }
 
-func newCert(db nosql.DB, ops CertOptions) (*certificate, error) {
+func newCert(db DB, ops CertOptions) error {
-
+	err := db.StoreCertificate(ops.Leaf)
-	// TODO: according to the RFC this should be IssuerAndSerialNumber,
-	// but sscep seems to use just the serial number for getcert
-
-	id := ops.Leaf.SerialNumber.String()
-
-	leaf := pem.EncodeToMemory(&pem.Block{
-		Type:  "CERTIFICATE",
-		Bytes: ops.Leaf.Raw,
-	})
-	var intermediates []byte
-	for _, cert := range ops.Intermediates {
-		intermediates = append(intermediates, pem.EncodeToMemory(&pem.Block{
-			Type:  "CERTIFICATE",
-			Bytes: cert.Raw,
-		})...)
-	}
-
-	cert := &certificate{
-		ID:            id,
-		Leaf:          leaf,
-		Intermediates: intermediates,
-		Created:       time.Now().UTC(),
-	}
-	certB, err := json.Marshal(cert)
 	if err != nil {
-		return nil, fmt.Errorf("%w: error marshaling certificate", err)
+		errors.Wrap(err, "error while storing certificate")
-	}
-
-	_, swapped, err := db.CmpAndSwap(certTable, []byte(id), nil, certB)
-	switch {
-	case err != nil:
-		return nil, fmt.Errorf("%w: error storing certificate", err)
-	case !swapped:
-		return nil, fmt.Errorf("error storing certificate; " +
-			"value has changed since last read")
-	default:
-		return cert, nil
 	}
+	return nil
 }
-
-// func getCert(db nosql.DB, id string) (*certificate, error) {
-// 	b, err := db.Get(certTable, []byte(id))
-// 	if nosql.IsErrNotFound(err) {
-// 		return nil, fmt.Errorf("certificate %s not found", id)
-// 	} else if err != nil {
-// 		return nil, fmt.Errorf("error loading certificate")
-// 	}
-// 	var cert certificate
-// 	if err := json.Unmarshal(b, &cert); err != nil {
-// 		return nil, fmt.Errorf("%w: error unmarshaling certificate", err)
-// 	}
-// 	return &cert, nil
-// }

scep/database.go

@@ -0,0 +1,7 @@
+package scep
+
+import "crypto/x509"
+
+type DB interface {
+	StoreCertificate(crt *x509.Certificate) error
+}

scep/options.go

@@ -0,0 +1,31 @@
+package scep
+
+import (
+	"crypto"
+	"crypto/x509"
+)
+
+type Options struct {
+	// CertificateChain is the issuer certificate, along with any other bundled certificates
+	// to be returned in the chain for consumers. Configured in ca.json crt property.
+	CertificateChain []*x509.Certificate
+	// Signer signs CSRs in SCEP. Configured in ca.json key property.
+	Signer crypto.Signer `json:"-"`
+	// Decrypter decrypts encrypted SCEP messages. Configured in ca.json key property.
+	Decrypter crypto.Decrypter `json:"-"`
+}
+
+// Validate checks the fields in Options.
+func (o *Options) Validate() error {
+	// var typ Type
+	// if o == nil {
+	// 	typ = Type(SoftCAS)
+	// } else {
+	// 	typ = Type(o.Type)
+	// }
+	// // Check that the type can be loaded.
+	// if _, ok := LoadCertificateAuthorityServiceNewFunc(typ); !ok {
+	// 	return errors.Errorf("unsupported cas type %s", typ)
+	// }
+	return nil
+}

scep/service.go

@@ -4,9 +4,6 @@ import (
 	"context"
 	"crypto"
 	"crypto/x509"
-	"strings"
-
-	"github.com/smallstep/certificates/cas/apiv1"
 )
 
 // Service is a wrapper for crypto.Signer and crypto.Decrypter
@@ -16,20 +13,12 @@ type Service struct {
 	decrypter        crypto.Decrypter
 }
 
-func NewService(ctx context.Context, opts apiv1.Options) (*Service, error) {
+func NewService(ctx context.Context, opts Options) (*Service, error) {
 
 	if err := opts.Validate(); err != nil {
 		return nil, err
 	}
 
-	t := apiv1.Type(strings.ToLower(opts.Type))
-	if t == apiv1.DefaultCAS {
-		t = apiv1.SoftCAS
-	}
-
-	// TODO: silence the linter (temporarily)
-	_ = t
-
 	// TODO: should this become similar to the New CertificateAuthorityService as in x509CAService?
 	return &Service{
 		certificateChain: opts.CertificateChain,