Refactor the SCEP authority initialization

Instead of relying on an intermediate `scep.Service` struct,
initialize the `scep.Authority` directly. This removes one redundant
layer of indirection.

authority/authority.go

@@ -62,7 +62,7 @@ type Authority struct {
 	x509Enforcers         []provisioner.CertificateEnforcer
 
 	// SCEP CA
-	scepService *scep.Service
+	scepAuthority *scep.Authority
 
 	// SSH CA
 	sshHostPassword         []byte
@@ -263,11 +263,12 @@ func (a *Authority) ReloadAdminResources(ctx context.Context) error {
 	a.admins = adminClxn
 
 	// update the SCEP service with the currently active SCEP
-	// provisioner names.
+	// provisioner names and revalidate the configuration.
-	// TODO(hs): trigger SCEP authority (re)validation using
+	if a.scepAuthority != nil {
-	// the current set of SCEP provisioners.
+		a.scepAuthority.UpdateProvisioners(a.getSCEPProvisionerNames())
-	if a.scepService != nil {
+		if err := a.scepAuthority.Validate(); err != nil {
-		a.scepService.UpdateProvisioners(a.getSCEPProvisionerNames())
+			log.Printf("failed validating SCEP authority: %v\n", err)
+		}
 	}
 
 	return nil
@@ -696,10 +697,16 @@ func (a *Authority) init() error {
 		// can be validated when the CA is started.
 		options.SCEPProvisionerNames = a.getSCEPProvisionerNames()
 
-		a.scepService, err = scep.NewService(ctx, options)
+		// create a new SCEP authority
+		a.scepAuthority, err = scep.New(a, options)
 		if err != nil {
 			return err
 		}
+
+		// validate the SCEP authority
+		if err := a.scepAuthority.Validate(); err != nil {
+			a.initLogf("failed validating SCEP authority: %v", err)
+		}
 	}
 
 	// Load X509 constraints engine.
@@ -871,9 +878,9 @@ func (a *Authority) getSCEPProvisionerNames() (names []string) {
 	return
 }
 
-// GetSCEP returns the configured SCEP Service.
+// GetSCEP returns the configured SCEP Authority
-func (a *Authority) GetSCEP() *scep.Service {
+func (a *Authority) GetSCEP() *scep.Authority {
-	return a.scepService
+	return a.scepAuthority
 }
 
 func (a *Authority) startCRLGenerator() error {

ca/ca.go

@@ -250,19 +250,14 @@ func (ca *CA) Init(cfg *config.Config) (*CA, error) {
 
 	var scepAuthority *scep.Authority
 	if ca.shouldServeSCEPEndpoints() {
-		if scepAuthority, err = scep.New(auth, scep.AuthorityOptions{
-			Service: auth.GetSCEP(),
-		}); err != nil {
-			return nil, errors.Wrap(err, "failed creating SCEP authority")
-		}
-
 		// validate the SCEP authority configuration. Currently this
 		// will not result in a failure to start if one or more SCEP
 		// provisioners are not correctly configured. Only a log will
 		// be emitted.
-		shouldFail := false
+		scepAuthority = auth.GetSCEP()
 		if err := scepAuthority.Validate(); err != nil {
 			err = errors.Wrap(err, "failed validating SCEP authority")
+			shouldFail := false
 			if shouldFail {
 				return nil, err
 			}

scep/authority.go

@@ -18,8 +18,13 @@ import (
 
 // Authority is the layer that handles all SCEP interactions.
 type Authority struct {
-	service  *Service // TODO: refactor, so that this is not required
+	signAuth             SignAuthority
-	signAuth SignAuthority
+	roots                []*x509.Certificate
+	intermediates        []*x509.Certificate
+	signerCertificate    *x509.Certificate
+	signer               crypto.Signer
+	defaultDecrypter     crypto.Decrypter
+	scepProvisionerNames []string
 }
 
 type authorityKey struct{}
@@ -45,13 +50,6 @@ func MustFromContext(ctx context.Context) *Authority {
 	}
 }
 
-// AuthorityOptions required to create a new SCEP Authority.
-type AuthorityOptions struct {
-	// Service provides the roots, intermediates, the signer and the (default)
-	// decrypter to the SCEP Authority.
-	Service *Service
-}
-
 // SignAuthority is the interface for a signing authority
 type SignAuthority interface {
 	Sign(cr *x509.CertificateRequest, opts provisioner.SignOptions, signOpts ...provisioner.SignOption) ([]*x509.Certificate, error)
@@ -59,10 +57,18 @@ type SignAuthority interface {
 }
 
 // New returns a new Authority that implements the SCEP interface.
-func New(signAuth SignAuthority, ops AuthorityOptions) (*Authority, error) {
+func New(signAuth SignAuthority, opts Options) (*Authority, error) {
+	if err := opts.Validate(); err != nil {
+		return nil, err
+	}
 	authority := &Authority{
-		signAuth: signAuth,
+		signAuth:             signAuth, // TODO: provide signAuth through context instead?
-		service:  ops.Service,
+		roots:                opts.Roots,
+		intermediates:        opts.Intermediates,
+		signerCertificate:    opts.SignerCert,
+		signer:               opts.Signer,
+		defaultDecrypter:     opts.Decrypter,
+		scepProvisionerNames: opts.SCEPProvisionerNames,
 	}
 	return authority, nil
 }
@@ -71,15 +77,15 @@ func New(signAuth SignAuthority, ops AuthorityOptions) (*Authority, error) {
 // The validation includes a check if a decrypter is available, either
 // an authority wide decrypter, or a provisioner specific decrypter.
 func (a *Authority) Validate() error {
-	noDefaultDecrypterAvailable := a.service.defaultDecrypter == nil
+	noDefaultDecrypterAvailable := a.defaultDecrypter == nil
-	for _, name := range a.service.scepProvisionerNames {
+	for _, name := range a.scepProvisionerNames {
 		p, err := a.LoadProvisionerByName(name)
 		if err != nil {
 			return fmt.Errorf("failed loading provisioner %q: %w", name, err)
 		}
 		if scepProv, ok := p.(*provisioner.SCEP); ok {
 			cert, decrypter := scepProv.GetDecrypter()
-			// TODO: return sentinel/typed error, to be able to ignore/log these cases during init?
+			// TODO(hs): return sentinel/typed error, to be able to ignore/log these cases during init?
 			if cert == nil && noDefaultDecrypterAvailable {
 				return fmt.Errorf("SCEP provisioner %q does not have a decrypter certificate", name)
 			}
@@ -92,6 +98,13 @@ func (a *Authority) Validate() error {
 	return nil
 }
 
+// UpdateProvisioners updates the SCEP Authority with the new, and hopefully
+// current SCEP provisioners configured. This allows the Authority to be
+// validated with the latest data.
+func (a *Authority) UpdateProvisioners(scepProvisionerNames []string) {
+	a.scepProvisionerNames = scepProvisionerNames
+}
+
 var (
 	// TODO: check the default capabilities; https://tools.ietf.org/html/rfc8894#section-3.5.2
 	defaultCapabilities = []string{
@@ -134,15 +147,14 @@ func (a *Authority) GetCACertificates(ctx context.Context) (certs []*x509.Certif
 		certs = append(certs, decrypterCertificate)
 	}
 
-	// TODO: ensure logic, so that signer is first intermediate and that
+	// TODO(hs): ensure logic is in place that checks the signer is the first
-	// there are no doubles certificates.
+	// intermediate and that there are no double certificates.
-	//certs = append(certs, a.service.signerCertificate)
+	certs = append(certs, a.intermediates...)
-	certs = append(certs, a.service.intermediates...)
 
-	// the CA roots are added for completeness. Clients are responsible
+	// the CA roots are added for completeness when configured to do so. Clients
-	// to select the right cert(s) to store and use.
+	// are responsible to select the right cert(s) to store and use.
 	if p.ShouldIncludeRootInChain() {
-		certs = append(certs, a.service.roots...)
+		certs = append(certs, a.roots...)
 	}
 
 	return certs, nil
@@ -213,8 +225,8 @@ func (a *Authority) selectDecrypter(ctx context.Context) (cert *x509.Certificate
 	}
 
 	// fallback to the CA wide decrypter
-	cert = a.service.signerCertificate
+	cert = a.signerCertificate
-	pkey = a.service.defaultDecrypter
+	pkey = a.defaultDecrypter
 
 	return
 }
@@ -351,8 +363,8 @@ func (a *Authority) SignCSR(ctx context.Context, csr *x509.CertificateRequest, m
 	// as the first certificate in the array
 	signedData.AddCertificate(cert)
 
-	authCert := a.service.signerCertificate
+	authCert := a.signerCertificate
-	signer := a.service.signer
+	signer := a.signer
 
 	// sign the attributes
 	if err := signedData.AddSigner(authCert, signer, config); err != nil {
@@ -423,7 +435,7 @@ func (a *Authority) CreateFailureResponse(_ context.Context, _ *x509.Certificate
 	}
 
 	// sign the attributes
-	if err := signedData.AddSigner(a.service.signerCertificate, a.service.signer, config); err != nil {
+	if err := signedData.AddSigner(a.signerCertificate, a.signer, config); err != nil {
 		return nil, err
 	}
 

scep/common.go

@@ -1,29 +0,0 @@
-package scep
-
-import (
-	"context"
-	"errors"
-)
-
-// ContextKey is the key type for storing and searching for SCEP request
-// essentials in the context of a request.
-type ContextKey string
-
-const (
-	// ProvisionerContextKey provisioner key
-	ProvisionerContextKey = ContextKey("provisioner")
-)
-
-// provisionerFromContext searches the context for a SCEP provisioner.
-// Returns the provisioner or an error.
-func provisionerFromContext(ctx context.Context) (Provisioner, error) {
-	val := ctx.Value(ProvisionerContextKey)
-	if val == nil {
-		return nil, errors.New("provisioner expected in request context")
-	}
-	p, ok := val.(Provisioner)
-	if !ok || p == nil {
-		return nil, errors.New("provisioner in context is not a SCEP provisioner")
-	}
-	return p, nil
-}

scep/database.go

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

scep/provisioner.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"crypto"
 	"crypto/x509"
+	"errors"
 	"time"
 
 	"github.com/smallstep/certificates/authority/provisioner"
@@ -22,3 +23,26 @@ type Provisioner interface {
 	GetContentEncryptionAlgorithm() int
 	ValidateChallenge(ctx context.Context, challenge, transactionID string) error
 }
+
+// ContextKey is the key type for storing and searching for SCEP request
+// essentials in the context of a request.
+type ContextKey string
+
+const (
+	// ProvisionerContextKey provisioner key
+	ProvisionerContextKey = ContextKey("provisioner")
+)
+
+// provisionerFromContext searches the context for a SCEP provisioner.
+// Returns the provisioner or an error.
+func provisionerFromContext(ctx context.Context) (Provisioner, error) {
+	val := ctx.Value(ProvisionerContextKey)
+	if val == nil {
+		return nil, errors.New("provisioner expected in request context")
+	}
+	p, ok := val.(Provisioner)
+	if !ok || p == nil {
+		return nil, errors.New("provisioner in context is not a SCEP provisioner")
+	}
+	return p, nil
+}

scep/service.go

@@ -1,38 +0,0 @@
-package scep
-
-import (
-	"context"
-	"crypto"
-	"crypto/x509"
-)
-
-// Service is a wrapper for a crypto.Decrypter and crypto.Signer for
-// decrypting SCEP requests and signing certificates in response to
-// SCEP certificate requests.
-type Service struct {
-	roots                []*x509.Certificate
-	intermediates        []*x509.Certificate
-	signerCertificate    *x509.Certificate
-	signer               crypto.Signer
-	defaultDecrypter     crypto.Decrypter
-	scepProvisionerNames []string
-}
-
-// NewService returns a new Service type.
-func NewService(_ context.Context, opts Options) (*Service, error) {
-	if err := opts.Validate(); err != nil {
-		return nil, err
-	}
-	return &Service{
-		roots:                opts.Roots,
-		intermediates:        opts.Intermediates,
-		signerCertificate:    opts.SignerCert,
-		signer:               opts.Signer,
-		defaultDecrypter:     opts.Decrypter,
-		scepProvisionerNames: opts.SCEPProvisionerNames,
-	}, nil
-}
-
-func (s *Service) UpdateProvisioners(scepProvisionerNames []string) {
-	s.scepProvisionerNames = scepProvisionerNames
-}