Refactor SCEP webhook validation

authority/provisioner/scep.go

@@ -2,10 +2,16 @@ package provisioner
 
 import (
 	"context"
+	"crypto/subtle"
+	"fmt"
+	"net/http"
 	"time"
 
 	"github.com/pkg/errors"
+
 	"go.step.sm/linkedca"
+
+	"github.com/smallstep/certificates/webhook"
 )
 
 // SCEP is the SCEP provisioner type, an entity that can authorize the
@@ -35,6 +41,7 @@ type SCEP struct {
 	ctl                           *Controller
 	secretChallengePassword       string
 	encryptionAlgorithm           int
+	challengeValidationController *challengeValidationController
 }
 
 // GetID returns the provisioner unique identifier.
@@ -82,6 +89,67 @@ func (s *SCEP) DefaultTLSCertDuration() time.Duration {
 	return s.ctl.Claimer.DefaultTLSCertDuration()
 }
 
+type challengeValidationController struct {
+	client   *http.Client
+	webhooks []*Webhook
+}
+
+// newChallengeValidationController creates a new challengeValidationController
+// that performs challenge validation through webhooks.
+func newChallengeValidationController(client *http.Client, webhooks []*Webhook) (*challengeValidationController, error) {
+	scepHooks := []*Webhook{}
+	for _, wh := range webhooks {
+		if wh.Kind != linkedca.Webhook_SCEPCHALLENGE.String() {
+			continue
+		}
+		if !isCertTypeOK(wh) {
+			continue
+		}
+		scepHooks = append(scepHooks, wh)
+	}
+	return &challengeValidationController{
+		client:   client,
+		webhooks: scepHooks,
+	}, nil
+}
+
+var (
+	ErrSCEPChallengeInvalid = errors.New("webhook server did not allow request")
+)
+
+// Validate executes zero or more configured webhooks to
+// validate the SCEP challenge. If at least one of them indicates
+// the challenge value is accepted, validation succeeds. In
+// that case, the other webhooks will be skipped. If none of
+// the webhooks indicates the value of the challenge was accepted,
+// an error is returned.
+func (c *challengeValidationController) Validate(ctx context.Context, challenge, transactionID string) error {
+	for _, wh := range c.webhooks {
+		req := &webhook.RequestBody{
+			SCEPChallenge:     challenge,
+			SCEPTransactionID: transactionID,
+		}
+		resp, err := wh.DoWithContext(ctx, c.client, req, nil) // TODO(hs): support templated URL? Requires some refactoring
+		if err != nil {
+			return fmt.Errorf("failed executing webhook request: %w", err)
+		}
+		if resp.Allow {
+			return nil // return early when response is positive
+		}
+	}
+
+	return ErrSCEPChallengeInvalid
+}
+
+// isCertTypeOK returns whether or not the webhook can be used
+// with the SCEP challenge validation webhook controller.
+func isCertTypeOK(wh *Webhook) bool {
+	if wh.CertType == linkedca.Webhook_ALL.String() || wh.CertType == "" {
+		return true
+	}
+	return linkedca.Webhook_X509.String() == wh.CertType
+}
+
 // Init initializes and validates the fields of a SCEP type.
 func (s *SCEP) Init(config Config) (err error) {
 	switch {
@@ -109,6 +177,13 @@ func (s *SCEP) Init(config Config) (err error) {
 		return errors.New("only encryption algorithm identifiers from 0 to 4 are valid")
 	}
 
+	if s.challengeValidationController, err = newChallengeValidationController(
+		config.WebhookClient,
+		s.GetOptions().GetWebhooks(),
+	); err != nil {
+		return fmt.Errorf("failed creating challenge validation controller: %w", err)
+	}
+
 	// TODO: add other, SCEP specific, options?
 
 	s.ctl, err = NewController(s, s.Claims, config, s.Options)
@@ -156,3 +231,43 @@ func (s *SCEP) ShouldIncludeRootInChain() bool {
 func (s *SCEP) GetContentEncryptionAlgorithm() int {
 	return s.encryptionAlgorithm
 }
+
+// ValidateChallenge validates the provided challenge. It starts by
+// selecting the validation method to use, then performs validation
+// according to that method.
+func (s *SCEP) ValidateChallenge(ctx context.Context, challenge, transactionID string) error {
+	if s.challengeValidationController == nil {
+		return fmt.Errorf("provisioner %q wasn't initialized", s.Name)
+	}
+	switch s.selectValidationMethod() {
+	case validationMethodWebhook:
+		return s.challengeValidationController.Validate(ctx, challenge, transactionID)
+	default:
+		if subtle.ConstantTimeCompare([]byte(s.secretChallengePassword), []byte(challenge)) == 0 {
+			return errors.New("invalid challenge password provided")
+		}
+		return nil
+	}
+}
+
+type validationMethod string
+
+const (
+	validationMethodNone    validationMethod = "none"
+	validationMethodStatic  validationMethod = "static"
+	validationMethodWebhook validationMethod = "webhook"
+)
+
+// selectValidationMethod returns the method to validate SCEP
+// challenges. If a webhook is configured with kind `SCEPCHALLENGE`,
+// the webhook method will be used. If a challenge password is set,
+// the static method is used. It will default to the `none` method.
+func (s *SCEP) selectValidationMethod() validationMethod {
+	if len(s.challengeValidationController.webhooks) > 0 {
+		return validationMethodWebhook
+	}
+	if s.secretChallengePassword != "" {
+		return validationMethodStatic
+	}
+	return validationMethodNone
+}

authority/provisioner/scep_test.go

@@ -0,0 +1,343 @@
+package provisioner
+
+import (
+	"context"
+	"encoding/json"
+	"errors"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"go.step.sm/linkedca"
+)
+
+func Test_challengeValidationController_Validate(t *testing.T) {
+	type request struct {
+		Challenge     string `json:"scepChallenge"`
+		TransactionID string `json:"scepTransactionID"`
+	}
+	type response struct {
+		Allow bool `json:"allow"`
+	}
+	nokServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		req := &request{}
+		err := json.NewDecoder(r.Body).Decode(req)
+		require.NoError(t, err)
+		assert.Equal(t, "not-allowed", req.Challenge)
+		assert.Equal(t, "transaction-1", req.TransactionID)
+		b, err := json.Marshal(response{Allow: false})
+		require.NoError(t, err)
+		w.WriteHeader(200)
+		w.Write(b)
+	}))
+	okServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		req := &request{}
+		err := json.NewDecoder(r.Body).Decode(req)
+		require.NoError(t, err)
+		assert.Equal(t, "challenge", req.Challenge)
+		assert.Equal(t, "transaction-1", req.TransactionID)
+		b, err := json.Marshal(response{Allow: true})
+		require.NoError(t, err)
+		w.WriteHeader(200)
+		w.Write(b)
+	}))
+	type fields struct {
+		client   *http.Client
+		webhooks []*Webhook
+	}
+	type args struct {
+		challenge     string
+		transactionID string
+	}
+	tests := []struct {
+		name   string
+		fields fields
+		args   args
+		server *httptest.Server
+		expErr error
+	}{
+		{
+			name:   "fail/no-webhook",
+			fields: fields{http.DefaultClient, nil},
+			args:   args{"no-webhook", "transaction-1"},
+			expErr: errors.New("webhook server did not allow request"),
+		},
+		{
+			name: "fail/wrong-cert-type",
+			fields: fields{http.DefaultClient, []*Webhook{
+				{
+					Kind:     linkedca.Webhook_SCEPCHALLENGE.String(),
+					CertType: linkedca.Webhook_SSH.String(),
+				},
+			}},
+			args:   args{"wrong-cert-type", "transaction-1"},
+			expErr: errors.New("webhook server did not allow request"),
+		},
+		{
+			name: "fail/wrong-secret-value",
+			fields: fields{http.DefaultClient, []*Webhook{
+				{
+					ID:       "webhook-id-1",
+					Name:     "webhook-name-1",
+					Secret:   "{{}}",
+					Kind:     linkedca.Webhook_SCEPCHALLENGE.String(),
+					CertType: linkedca.Webhook_X509.String(),
+					URL:      okServer.URL,
+				},
+			}},
+			args: args{
+				challenge:     "wrong-secret-value",
+				transactionID: "transaction-1",
+			},
+			expErr: errors.New("failed executing webhook request: illegal base64 data at input byte 0"),
+		},
+		{
+			name: "fail/not-allowed",
+			fields: fields{http.DefaultClient, []*Webhook{
+				{
+					ID:       "webhook-id-1",
+					Name:     "webhook-name-1",
+					Secret:   "MTIzNAo=",
+					Kind:     linkedca.Webhook_SCEPCHALLENGE.String(),
+					CertType: linkedca.Webhook_X509.String(),
+					URL:      nokServer.URL,
+				},
+			}},
+			args: args{
+				challenge:     "not-allowed",
+				transactionID: "transaction-1",
+			},
+			server: nokServer,
+			expErr: errors.New("webhook server did not allow request"),
+		},
+		{
+			name: "ok",
+			fields: fields{http.DefaultClient, []*Webhook{
+				{
+					ID:       "webhook-id-1",
+					Name:     "webhook-name-1",
+					Secret:   "MTIzNAo=",
+					Kind:     linkedca.Webhook_SCEPCHALLENGE.String(),
+					CertType: linkedca.Webhook_X509.String(),
+					URL:      okServer.URL,
+				},
+			}},
+			args: args{
+				challenge:     "challenge",
+				transactionID: "transaction-1",
+			},
+			server: okServer,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			c, err := newChallengeValidationController(tt.fields.client, tt.fields.webhooks)
+			require.NoError(t, err)
+
+			if tt.server != nil {
+				defer tt.server.Close()
+			}
+
+			ctx := context.Background()
+			err = c.Validate(ctx, tt.args.challenge, tt.args.transactionID)
+
+			if tt.expErr != nil {
+				assert.EqualError(t, err, tt.expErr.Error())
+				return
+			}
+
+			assert.NoError(t, err)
+		})
+	}
+}
+
+func TestController_isCertTypeOK(t *testing.T) {
+	assert.True(t, isCertTypeOK(&Webhook{CertType: linkedca.Webhook_X509.String()}))
+	assert.True(t, isCertTypeOK(&Webhook{CertType: linkedca.Webhook_ALL.String()}))
+	assert.True(t, isCertTypeOK(&Webhook{CertType: ""}))
+	assert.False(t, isCertTypeOK(&Webhook{CertType: linkedca.Webhook_SSH.String()}))
+}
+
+func Test_selectValidationMethod(t *testing.T) {
+	tests := []struct {
+		name string
+		p    *SCEP
+		want validationMethod
+	}{
+		{"webhooks", &SCEP{
+			Name: "SCEP",
+			Type: "SCEP",
+			Options: &Options{
+				Webhooks: []*Webhook{
+					{
+						Kind: linkedca.Webhook_SCEPCHALLENGE.String(),
+					},
+				},
+			},
+		}, "webhook"},
+		{"challenge", &SCEP{
+			Name:              "SCEP",
+			Type:              "SCEP",
+			ChallengePassword: "pass",
+		}, "static"},
+		{"challenge-with-different-webhook", &SCEP{
+			Name: "SCEP",
+			Type: "SCEP",
+			Options: &Options{
+				Webhooks: []*Webhook{
+					{
+						Kind: linkedca.Webhook_AUTHORIZING.String(),
+					},
+				},
+			},
+			ChallengePassword: "pass",
+		}, "static"},
+		{"none", &SCEP{
+			Name: "SCEP",
+			Type: "SCEP",
+		}, "none"},
+		{"none-with-different-webhook", &SCEP{
+			Name: "SCEP",
+			Type: "SCEP",
+			Options: &Options{
+				Webhooks: []*Webhook{
+					{
+						Kind: linkedca.Webhook_AUTHORIZING.String(),
+					},
+				},
+			},
+		}, "none"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := tt.p.Init(Config{Claims: globalProvisionerClaims})
+			require.NoError(t, err)
+			got := tt.p.selectValidationMethod()
+			assert.Equal(t, tt.want, got)
+		})
+	}
+}
+
+func TestSCEP_ValidateChallenge(t *testing.T) {
+	type request struct {
+		Challenge     string `json:"scepChallenge"`
+		TransactionID string `json:"scepTransactionID"`
+	}
+	type response struct {
+		Allow bool `json:"allow"`
+	}
+	okServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		req := &request{}
+		err := json.NewDecoder(r.Body).Decode(req)
+		require.NoError(t, err)
+		assert.Equal(t, "webhook-challenge", req.Challenge)
+		assert.Equal(t, "webhook-transaction-1", req.TransactionID)
+		b, err := json.Marshal(response{Allow: true})
+		require.NoError(t, err)
+		w.WriteHeader(200)
+		w.Write(b)
+	}))
+	type args struct {
+		challenge     string
+		transactionID string
+	}
+	tests := []struct {
+		name   string
+		p      *SCEP
+		server *httptest.Server
+		args   args
+		expErr error
+	}{
+		{"ok/webhooks", &SCEP{
+			Name: "SCEP",
+			Type: "SCEP",
+			Options: &Options{
+				Webhooks: []*Webhook{
+					{
+						ID:       "webhook-id-1",
+						Name:     "webhook-name-1",
+						Secret:   "MTIzNAo=",
+						Kind:     linkedca.Webhook_SCEPCHALLENGE.String(),
+						CertType: linkedca.Webhook_X509.String(),
+						URL:      okServer.URL,
+					},
+				},
+			},
+		}, okServer, args{"webhook-challenge", "webhook-transaction-1"},
+			nil,
+		},
+		{"fail/webhooks-secret-configuration", &SCEP{
+			Name: "SCEP",
+			Type: "SCEP",
+			Options: &Options{
+				Webhooks: []*Webhook{
+					{
+						ID:       "webhook-id-1",
+						Name:     "webhook-name-1",
+						Secret:   "{{}}",
+						Kind:     linkedca.Webhook_SCEPCHALLENGE.String(),
+						CertType: linkedca.Webhook_X509.String(),
+						URL:      okServer.URL,
+					},
+				},
+			},
+		}, nil, args{"webhook-challenge", "webhook-transaction-1"},
+			errors.New("failed executing webhook request: illegal base64 data at input byte 0"),
+		},
+		{"ok/static-challenge", &SCEP{
+			Name:              "SCEP",
+			Type:              "SCEP",
+			Options:           &Options{},
+			ChallengePassword: "secret-static-challenge",
+		}, nil, args{"secret-static-challenge", "static-transaction-1"},
+			nil,
+		},
+		{"fail/wrong-static-challenge", &SCEP{
+			Name:              "SCEP",
+			Type:              "SCEP",
+			Options:           &Options{},
+			ChallengePassword: "secret-static-challenge",
+		}, nil, args{"the-wrong-challenge-secret", "static-transaction-1"},
+			errors.New("invalid challenge password provided"),
+		},
+		{"ok/no-challenge", &SCEP{
+			Name:              "SCEP",
+			Type:              "SCEP",
+			Options:           &Options{},
+			ChallengePassword: "",
+		}, nil, args{"", "static-transaction-1"},
+			nil,
+		},
+		{"fail/no-challenge-but-provided", &SCEP{
+			Name:              "SCEP",
+			Type:              "SCEP",
+			Options:           &Options{},
+			ChallengePassword: "",
+		}, nil, args{"a-challenge-value", "static-transaction-1"},
+			errors.New("invalid challenge password provided"),
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+
+			if tt.server != nil {
+				defer tt.server.Close()
+			}
+
+			err := tt.p.Init(Config{Claims: globalProvisionerClaims, WebhookClient: http.DefaultClient})
+			require.NoError(t, err)
+			ctx := context.Background()
+
+			err = tt.p.ValidateChallenge(ctx, tt.args.challenge, tt.args.transactionID)
+			if tt.expErr != nil {
+				assert.EqualError(t, err, tt.expErr.Error())
+				return
+			}
+
+			assert.NoError(t, err)
+		})
+	}
+}

scep/api/api.go

@@ -15,13 +15,11 @@ import (
 	"github.com/go-chi/chi"
 	microscep "github.com/micromdm/scep/v2/scep"
 	"go.mozilla.org/pkcs7"
-	"go.step.sm/linkedca"
 
 	"github.com/smallstep/certificates/api"
 	"github.com/smallstep/certificates/api/log"
 	"github.com/smallstep/certificates/authority/provisioner"
 	"github.com/smallstep/certificates/scep"
-	"github.com/smallstep/certificates/scep/api/webhook"
 )
 
 const (
@@ -305,16 +303,6 @@ func PKIOperation(ctx context.Context, req request) (Response, error) {
 		return Response{}, err
 	}
 
-	prov, err := scep.ProvisionerFromContext(ctx)
-	if err != nil {
-		return Response{}, err
-	}
-
-	scepProv, ok := prov.(*provisioner.SCEP)
-	if !ok {
-		return Response{}, errors.New("wrong type of provisioner in context")
-	}
-
 	// NOTE: at this point we have sufficient information for returning nicely signed CertReps
 	csr := msg.CSRReqMessage.CSR
 	transactionID := string(msg.TransactionID)
@@ -326,31 +314,12 @@ func PKIOperation(ctx context.Context, req request) (Response, error) {
 	// a certificate exists; then it will use RenewalReq. Adding the challenge check here may be a small breaking change for clients.
 	// We'll have to see how it works out.
 	if msg.MessageType == microscep.PKCSReq || msg.MessageType == microscep.RenewalReq {
-		// TODO(hs): might be nice to use strategy pattern implementation; maybe behind the
+		if err := auth.ValidateChallenge(ctx, challengePassword, transactionID); err != nil {
-		// auth.MatchChallengePassword interface/method. Will need to think about methods
+			if errors.Is(err, provisioner.ErrSCEPChallengeInvalid) {
-		// that don't just check the password, but do different things on success and
+				return createFailureResponse(ctx, csr, msg, microscep.BadRequest, err)
-		// failure too.
-		switch selectValidationMethod(scepProv) {
-		case validationMethodWebhook:
-			c, err := webhook.New(scepProv.GetOptions().GetWebhooks())
-			if err != nil {
-				return createFailureResponse(ctx, csr, msg, microscep.BadRequest, errors.New("failed creating SCEP validation webhook controller"))
-			}
-			if err := c.Validate(ctx, challengePassword, transactionID); err != nil {
-				if errors.Is(err, provisioner.ErrWebhookDenied) {
-					return createFailureResponse(ctx, csr, msg, microscep.BadRequest, errors.New("invalid challenge password provided"))
 			}
 			return createFailureResponse(ctx, csr, msg, microscep.BadRequest, errors.New("failed validating challenge password"))
 		}
-		default:
-			challengeMatches, err := auth.MatchChallengePassword(ctx, challengePassword)
-			if err != nil {
-				return createFailureResponse(ctx, csr, msg, microscep.BadRequest, errors.New("failed checking password"))
-			}
-			if !challengeMatches {
-				return createFailureResponse(ctx, csr, msg, microscep.BadRequest, errors.New("invalid challenge password provided"))
-			}
-		}
 	}
 
 	// TODO: authorize renewal: we can authorize renewals with the challenge password (if reusable secrets are used).
@@ -375,33 +344,6 @@ func PKIOperation(ctx context.Context, req request) (Response, error) {
 	return res, nil
 }
 
-type validationMethod string
-
-const (
-	validationMethodNone    validationMethod = "none"
-	validationMethodStatic  validationMethod = "static"
-	validationMethodWebhook validationMethod = "webhook"
-)
-
-// selectValidationMethod returns the method to validate SCEP
-// challenges. If a webhook is configured with kind `SCEPCHALLENGE`,
-// the webhook method will be used. If a challenge password is set,
-// the static method is used. It will default to the `none` method.
-func selectValidationMethod(p *provisioner.SCEP) validationMethod {
-	for _, wh := range p.GetOptions().GetWebhooks() {
-		// if at least one webhook for validating SCEP challenges has
-		// been configured, that will be used to perform challenge
-		// validation.
-		if wh.Kind == linkedca.Webhook_SCEPCHALLENGE.String() {
-			return validationMethodWebhook
-		}
-	}
-	if challenge := p.GetChallengePassword(); challenge != "" {
-		return validationMethodStatic
-	}
-	return validationMethodNone
-}
-
 func formatCapabilities(caps []string) []byte {
 	return []byte(strings.Join(caps, "\r\n"))
 }

scep/api/api_test.go

@@ -9,12 +9,6 @@ import (
 	"reflect"
 	"testing"
 	"testing/iotest"
-
-	"github.com/smallstep/certificates/authority/config"
-	"github.com/smallstep/certificates/authority/provisioner"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"go.step.sm/linkedca"
 )
 
 func Test_decodeRequest(t *testing.T) {
@@ -117,65 +111,3 @@ func Test_decodeRequest(t *testing.T) {
 		})
 	}
 }
-
-func Test_selectValidationMethod(t *testing.T) {
-	tests := []struct {
-		name string
-		p    *provisioner.SCEP
-		want validationMethod
-	}{
-		{"webhooks", &provisioner.SCEP{
-			Name: "SCEP",
-			Type: "SCEP",
-			Options: &provisioner.Options{
-				Webhooks: []*provisioner.Webhook{
-					{
-						Kind: linkedca.Webhook_SCEPCHALLENGE.String(),
-					},
-				},
-			},
-		}, "webhook"},
-		{"challenge", &provisioner.SCEP{
-			Name:              "SCEP",
-			Type:              "SCEP",
-			ChallengePassword: "pass",
-		}, "static"},
-		{"challenge-with-different-webhook", &provisioner.SCEP{
-			Name:              "SCEP",
-			Type:              "SCEP",
-			ChallengePassword: "pass",
-			Options: &provisioner.Options{
-				Webhooks: []*provisioner.Webhook{
-					{
-						Kind: linkedca.Webhook_AUTHORIZING.String(),
-					},
-				},
-			},
-		}, "static"},
-		{"none", &provisioner.SCEP{
-			Name: "SCEP",
-			Type: "SCEP",
-		}, "none"},
-		{"none-with-different-webhook", &provisioner.SCEP{
-			Name: "SCEP",
-			Type: "SCEP",
-			Options: &provisioner.Options{
-				Webhooks: []*provisioner.Webhook{
-					{
-						Kind: linkedca.Webhook_AUTHORIZING.String(),
-					},
-				},
-			},
-		}, "none"},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			err := tt.p.Init(provisioner.Config{
-				Claims: config.GlobalProvisionerClaims,
-			})
-			require.NoError(t, err)
-			got := selectValidationMethod(tt.p)
-			assert.Equal(t, tt.want, got)
-		})
-	}
-}

scep/api/webhook/webhook.go

@@ -1,65 +0,0 @@
-package webhook
-
-import (
-	"context"
-	"fmt"
-	"net/http"
-
-	"go.step.sm/linkedca"
-
-	"github.com/smallstep/certificates/authority/provisioner"
-	"github.com/smallstep/certificates/webhook"
-)
-
-// Controller controls webhook execution
-type Controller struct {
-	client   *http.Client
-	webhooks []*provisioner.Webhook
-}
-
-// New returns a new SCEP webhook Controller
-func New(webhooks []*provisioner.Webhook) (*Controller, error) {
-	return &Controller{
-		client:   http.DefaultClient,
-		webhooks: webhooks,
-	}, nil
-}
-
-// Validate executes zero or more configured webhooks to
-// validate the SCEP challenge. If at least one of them indicates
-// the challenge value is accepted, validation succeeds. In
-// that case, the other webhooks will be skipped. If none of
-// the webhooks indicates the value of the challenge was accepted,
-// an error is returned.
-func (c *Controller) Validate(ctx context.Context, challenge, transactionID string) error {
-	for _, wh := range c.webhooks {
-		if wh.Kind != linkedca.Webhook_SCEPCHALLENGE.String() {
-			continue
-		}
-		if !isCertTypeOK(wh) {
-			continue
-		}
-		req := &webhook.RequestBody{
-			SCEPChallenge:     challenge,
-			SCEPTransactionID: transactionID,
-		}
-		resp, err := wh.DoWithContext(ctx, c.client, req, nil) // TODO(hs): support templated URL? Requires some refactoring
-		if err != nil {
-			return fmt.Errorf("failed executing webhook request: %w", err)
-		}
-		if resp.Allow {
-			return nil // return early when response is positive
-		}
-	}
-
-	return provisioner.ErrWebhookDenied
-}
-
-// isCertTypeOK returns whether or not the webhook can be used
-// with the SCEP challenge validation webhook controller.
-func isCertTypeOK(wh *provisioner.Webhook) bool {
-	if wh.CertType == linkedca.Webhook_ALL.String() || wh.CertType == "" {
-		return true
-	}
-	return linkedca.Webhook_X509.String() == wh.CertType
-}

scep/api/webhook/webhook_test.go

@@ -1,175 +0,0 @@
-package webhook
-
-import (
-	"context"
-	"encoding/json"
-	"errors"
-	"net/http"
-	"net/http/httptest"
-	"testing"
-
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-
-	"go.step.sm/linkedca"
-
-	"github.com/smallstep/certificates/authority/provisioner"
-)
-
-func TestController_Validate(t *testing.T) {
-	type request struct {
-		Challenge     string `json:"scepChallenge"`
-		TransactionID string `json:"scepTransactionID"`
-	}
-	type response struct {
-		Allow bool `json:"allow"`
-	}
-	nokServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		req := &request{}
-		err := json.NewDecoder(r.Body).Decode(req)
-		require.NoError(t, err)
-		assert.Equal(t, "not-allowed", req.Challenge)
-		assert.Equal(t, "transaction-1", req.TransactionID)
-		b, err := json.Marshal(response{Allow: false})
-		require.NoError(t, err)
-		w.WriteHeader(200)
-		w.Write(b)
-	}))
-	okServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		req := &request{}
-		err := json.NewDecoder(r.Body).Decode(req)
-		require.NoError(t, err)
-		assert.Equal(t, "challenge", req.Challenge)
-		assert.Equal(t, "transaction-1", req.TransactionID)
-		b, err := json.Marshal(response{Allow: true})
-		require.NoError(t, err)
-		w.WriteHeader(200)
-		w.Write(b)
-	}))
-	type fields struct {
-		client   *http.Client
-		webhooks []*provisioner.Webhook
-	}
-	type args struct {
-		challenge     string
-		transactionID string
-	}
-	tests := []struct {
-		name   string
-		fields fields
-		args   args
-		server *httptest.Server
-		expErr error
-	}{
-		{
-			name:   "fail/no-webhook",
-			fields: fields{http.DefaultClient, nil},
-			args:   args{"no-webhook", "transaction-1"},
-			expErr: errors.New("webhook server did not allow request"),
-		},
-		{
-			name: "fail/no-scep-webhook",
-			fields: fields{http.DefaultClient, []*provisioner.Webhook{
-				{
-					Kind: linkedca.Webhook_AUTHORIZING.String(),
-				},
-			}},
-			args:   args{"no-scep-webhook", "transaction-1"},
-			expErr: errors.New("webhook server did not allow request"),
-		},
-		{
-			name: "fail/wrong-cert-type",
-			fields: fields{http.DefaultClient, []*provisioner.Webhook{
-				{
-					Kind:     linkedca.Webhook_SCEPCHALLENGE.String(),
-					CertType: linkedca.Webhook_SSH.String(),
-				},
-			}},
-			args:   args{"wrong-cert-type", "transaction-1"},
-			expErr: errors.New("webhook server did not allow request"),
-		},
-		{
-			name: "fail/wrong-secret-value",
-			fields: fields{http.DefaultClient, []*provisioner.Webhook{
-				{
-					ID:       "webhook-id-1",
-					Name:     "webhook-name-1",
-					Secret:   "{{}}",
-					Kind:     linkedca.Webhook_SCEPCHALLENGE.String(),
-					CertType: linkedca.Webhook_X509.String(),
-					URL:      okServer.URL,
-				},
-			}},
-			args: args{
-				challenge:     "wrong-secret-value",
-				transactionID: "transaction-1",
-			},
-			expErr: errors.New("failed executing webhook request: illegal base64 data at input byte 0"),
-		},
-		{
-			name: "fail/not-allowed",
-			fields: fields{http.DefaultClient, []*provisioner.Webhook{
-				{
-					ID:       "webhook-id-1",
-					Name:     "webhook-name-1",
-					Secret:   "MTIzNAo=",
-					Kind:     linkedca.Webhook_SCEPCHALLENGE.String(),
-					CertType: linkedca.Webhook_X509.String(),
-					URL:      nokServer.URL,
-				},
-			}},
-			args: args{
-				challenge:     "not-allowed",
-				transactionID: "transaction-1",
-			},
-			server: nokServer,
-			expErr: errors.New("webhook server did not allow request"),
-		},
-		{
-			name: "ok",
-			fields: fields{http.DefaultClient, []*provisioner.Webhook{
-				{
-					ID:       "webhook-id-1",
-					Name:     "webhook-name-1",
-					Secret:   "MTIzNAo=",
-					Kind:     linkedca.Webhook_SCEPCHALLENGE.String(),
-					CertType: linkedca.Webhook_X509.String(),
-					URL:      okServer.URL,
-				},
-			}},
-			args: args{
-				challenge:     "challenge",
-				transactionID: "transaction-1",
-			},
-			server: okServer,
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			c := &Controller{
-				client:   tt.fields.client,
-				webhooks: tt.fields.webhooks,
-			}
-
-			if tt.server != nil {
-				defer tt.server.Close()
-			}
-
-			ctx := context.Background()
-			err := c.Validate(ctx, tt.args.challenge, tt.args.transactionID)
-			if tt.expErr != nil {
-				assert.EqualError(t, err, tt.expErr.Error())
-				return
-			}
-
-			assert.NoError(t, err)
-		})
-	}
-}
-
-func TestController_isCertTypeOK(t *testing.T) {
-	assert.True(t, isCertTypeOK(&provisioner.Webhook{CertType: linkedca.Webhook_X509.String()}))
-	assert.True(t, isCertTypeOK(&provisioner.Webhook{CertType: linkedca.Webhook_ALL.String()}))
-	assert.True(t, isCertTypeOK(&provisioner.Webhook{CertType: ""}))
-	assert.False(t, isCertTypeOK(&provisioner.Webhook{CertType: linkedca.Webhook_SSH.String()}))
-}

scep/authority.go

@@ -2,7 +2,6 @@ package scep
 
 import (
 	"context"
-	"crypto/subtle"
 	"crypto/x509"
 	"errors"
 	"fmt"
@@ -161,7 +160,7 @@ func (a *Authority) GetCACertificates(ctx context.Context) ([]*x509.Certificate,
 	// The certificate to use should probably depend on the (configured) provisioner and may
 	// use a distinct certificate, apart from the intermediate.
 
-	p, err := ProvisionerFromContext(ctx)
+	p, err := provisionerFromContext(ctx)
 	if err != nil {
 		return nil, err
 	}
@@ -235,7 +234,7 @@ func (a *Authority) SignCSR(ctx context.Context, csr *x509.CertificateRequest, m
 	// poll for the status. It seems to be similar as what can happen in ACME, so might want to model
 	// the implementation after the one in the ACME authority. Requires storage, etc.
 
-	p, err := ProvisionerFromContext(ctx)
+	p, err := provisionerFromContext(ctx)
 	if err != nil {
 		return nil, err
 	}
@@ -456,27 +455,9 @@ func (a *Authority) CreateFailureResponse(ctx context.Context, csr *x509.Certifi
 	return crepMsg, nil
 }
 
-// MatchChallengePassword verifies a SCEP challenge password
-func (a *Authority) MatchChallengePassword(ctx context.Context, password string) (bool, error) {
-	p, err := ProvisionerFromContext(ctx)
-	if err != nil {
-		return false, err
-	}
-
-	if subtle.ConstantTimeCompare([]byte(p.GetChallengePassword()), []byte(password)) == 1 {
-		return true, nil
-	}
-
-	// TODO: support dynamic challenges, i.e. a list of challenges instead of one?
-	// That's probably a bit harder to configure, though; likely requires some data store
-	// that can be interacted with more easily, via some internal API, for example.
-
-	return false, nil
-}
-
 // GetCACaps returns the CA capabilities
 func (a *Authority) GetCACaps(ctx context.Context) []string {
-	p, err := ProvisionerFromContext(ctx)
+	p, err := provisionerFromContext(ctx)
 	if err != nil {
 		return defaultCapabilities
 	}
@@ -494,3 +475,11 @@ func (a *Authority) GetCACaps(ctx context.Context) []string {
 
 	return caps
 }
+
+func (a *Authority) ValidateChallenge(ctx context.Context, challenge, transactionID string) error {
+	p, err := provisionerFromContext(ctx)
+	if err != nil {
+		return err
+	}
+	return p.ValidateChallenge(ctx, challenge, transactionID)
+}

scep/common.go

@@ -14,9 +14,9 @@ const (
 	ProvisionerContextKey = ContextKey("provisioner")
 )
 
-// ProvisionerFromContext searches the context for a SCEP provisioner.
+// provisionerFromContext searches the context for a SCEP provisioner.
 // Returns the provisioner or an error.
-func ProvisionerFromContext(ctx context.Context) (Provisioner, error) {
+func provisionerFromContext(ctx context.Context) (Provisioner, error) {
 	val := ctx.Value(ProvisionerContextKey)
 	if val == nil {
 		return nil, errors.New("provisioner expected in request context")

scep/provisioner.go

@@ -14,8 +14,8 @@ type Provisioner interface {
 	GetName() string
 	DefaultTLSCertDuration() time.Duration
 	GetOptions() *provisioner.Options
-	GetChallengePassword() string
 	GetCapabilities() []string
 	ShouldIncludeRootInChain() bool
 	GetContentEncryptionAlgorithm() int
+	ValidateChallenge(ctx context.Context, challenge, transactionID string) error
 }