Merge pull request #838 from smallstep/max/validate-provisioner-before-store

Validate provisioner configuration before storing in DB

CHANGELOG.md

@@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 ### Deprecated
 ### Removed
 ### Fixed
+- During provisioner add - validate provisioner configuration before storing to DB.
 ### Security
 
 ## [0.18.1] - 2022-02-03

authority/authority.go

@@ -175,7 +175,7 @@ func (a *Authority) reloadAdminResources(ctx context.Context) error {
 	// Create provisioner collection.
 	provClxn := provisioner.NewCollection(provisionerConfig.Audiences)
 	for _, p := range provList {
-		if err := p.Init(*provisionerConfig); err != nil {
+		if err := p.Init(provisionerConfig); err != nil {
 			return err
 		}
 		if err := provClxn.Store(p); err != nil {

authority/provisioners.go

@@ -87,20 +87,20 @@ func (a *Authority) LoadProvisionerByName(name string) (provisioner.Interface, e
 	return p, nil
 }
 
-func (a *Authority) generateProvisionerConfig(ctx context.Context) (*provisioner.Config, error) {
+func (a *Authority) generateProvisionerConfig(ctx context.Context) (provisioner.Config, error) {
 	// Merge global and configuration claims
 	claimer, err := provisioner.NewClaimer(a.config.AuthorityConfig.Claims, config.GlobalProvisionerClaims)
 	if err != nil {
-		return nil, err
+		return provisioner.Config{}, err
 	}
 	// TODO: should we also be combining the ssh federated roots here?
 	// If we rotate ssh roots keys, sshpop provisioner will lose ability to
 	// validate old SSH certificates, unless they are added as federated certs.
 	sshKeys, err := a.GetSSHRoots(ctx)
 	if err != nil {
-		return nil, err
+		return provisioner.Config{}, err
 	}
-	return &provisioner.Config{
+	return provisioner.Config{
 		Claims:    claimer.Claims(),
 		Audiences: a.config.GetAudiences(),
 		DB:        a.db,
@@ -133,9 +133,18 @@ func (a *Authority) StoreProvisioner(ctx context.Context, prov *linkedca.Provisi
 			"provisioner with token ID %s already exists", certProv.GetIDForToken())
 	}
 
+	provisionerConfig, err := a.generateProvisionerConfig(ctx)
+	if err != nil {
+		return admin.WrapErrorISE(err, "error generating provisioner config")
+	}
+
+	if err := certProv.Init(provisionerConfig); err != nil {
+		return admin.WrapError(admin.ErrorBadRequestType, err, "error validating configuration for provisioner %s", prov.Name)
+	}
+
 	// Store to database -- this will set the ID.
 	if err := a.adminDB.CreateProvisioner(ctx, prov); err != nil {
-		return admin.WrapErrorISE(err, "error creating admin")
+		return admin.WrapErrorISE(err, "error creating provisioner")
 	}
 
 	// We need a new conversion that has the newly set ID.
@@ -145,12 +154,7 @@ func (a *Authority) StoreProvisioner(ctx context.Context, prov *linkedca.Provisi
 			"error converting to certificates provisioner from linkedca provisioner")
 	}
 
-	provisionerConfig, err := a.generateProvisionerConfig(ctx)
-	if err != nil {
-		return admin.WrapErrorISE(err, "error generating provisioner config")
-	}
-
-	if err := certProv.Init(*provisionerConfig); err != nil {
+	if err := certProv.Init(provisionerConfig); err != nil {
 		return admin.WrapErrorISE(err, "error initializing provisioner %s", prov.Name)
 	}
 
@@ -179,7 +183,7 @@ func (a *Authority) UpdateProvisioner(ctx context.Context, nu *linkedca.Provisio
 		return admin.WrapErrorISE(err, "error generating provisioner config")
 	}
 
-	if err := certProv.Init(*provisionerConfig); err != nil {
+	if err := certProv.Init(provisionerConfig); err != nil {
 		return admin.WrapErrorISE(err, "error initializing provisioner %s", nu.Name)
 	}