Make SCEP webhook validation look better

authority/provisioner/webhook.go

@@ -107,6 +107,13 @@ type Webhook struct {
 }
 
 func (w *Webhook) Do(client *http.Client, reqBody *webhook.RequestBody, data any) (*webhook.ResponseBody, error) {
+	ctx, cancel := context.WithTimeout(context.Background(), time.Second*10)
+	defer cancel()
+
+	return w.DoWithContext(ctx, client, reqBody, data)
+}
+
+func (w *Webhook) DoWithContext(ctx context.Context, client *http.Client, reqBody *webhook.RequestBody, data any) (*webhook.ResponseBody, error) {
 	tmpl, err := template.New("url").Funcs(templates.StepFuncMap()).Parse(w.URL)
 	if err != nil {
 		return nil, err
@@ -129,8 +136,6 @@ func (w *Webhook) Do(client *http.Client, reqBody *webhook.RequestBody, data any
 			reqBody.Token = tmpl[sshutil.TokenKey]
 		}
 	*/
-	ctx, cancel := context.WithTimeout(context.Background(), time.Second*10)
-	defer cancel()
 
 	reqBody.Timestamp = time.Now()
 

go.mod

@@ -30,7 +30,7 @@ require (
 	go.mozilla.org/pkcs7 v0.0.0-20210826202110-33d05740a352
 	go.step.sm/cli-utils v0.7.6
 	go.step.sm/crypto v0.29.3
-	go.step.sm/linkedca v0.19.1-0.20230428150007-f95d2903af82
+	go.step.sm/linkedca v0.19.1
 	golang.org/x/crypto v0.8.0
 	golang.org/x/exp v0.0.0-20230310171629-522b1b587ee0
 	golang.org/x/net v0.9.0

go.sum

@@ -1034,6 +1034,8 @@ go.step.sm/crypto v0.29.3 h1:lFCsFQQGic1VZIa0B/87iMCDy67+LW8eEl119GTyeWI=
 go.step.sm/crypto v0.29.3/go.mod h1:0lYeIyQMJbFJ27L4BOGaq2gnuTgOShf+Ju/cTsMULq4=
 go.step.sm/linkedca v0.19.1-0.20230428150007-f95d2903af82 h1:oQtwNr4cxHxyrJaqYlI6DfhtVfkoVjsRZlUm0XYhec8=
 go.step.sm/linkedca v0.19.1-0.20230428150007-f95d2903af82/go.mod h1:vPV2ad3LFQJmV7XWt87VlnJSs6UOqgsbVGVWe3veEmI=
+go.step.sm/linkedca v0.19.1 h1:uY0ByT/uB3FCQ8zIo9mU7MWG7HKf5sDXNEBeN94MuP8=
+go.step.sm/linkedca v0.19.1/go.mod h1:vPV2ad3LFQJmV7XWt87VlnJSs6UOqgsbVGVWe3veEmI=
 go.uber.org/atomic v1.3.2/go.mod h1:gD2HeocX3+yG+ygLZcrzQJaqmWj9AIm7n08wl/qW/PE=
 go.uber.org/atomic v1.4.0/go.mod h1:gD2HeocX3+yG+ygLZcrzQJaqmWj9AIm7n08wl/qW/PE=
 go.uber.org/atomic v1.5.0/go.mod h1:sABNBOSYdrvTF6hTgEIbc7YasKWGhgEQZyfxyTvoXHQ=

scep/api/api.go

@@ -308,22 +308,11 @@ func PKIOperation(ctx context.Context, req request) (Response, error) {
 	// NOTE: at this point we have sufficient information for returning nicely signed CertReps
 	csr := msg.CSRReqMessage.CSR
 
-	prov, err := scep.ProvisionerFromContext(ctx) // TODO(hs): should this be retrieved in the API?
+	prov, err := scep.ProvisionerFromContext(ctx)
 	if err != nil {
 		return Response{}, err
 	}
 
-	const checkMethodWebhook string = "webhook"
-	checkMethod := ""
-	for _, wh := range prov.GetOptions().GetWebhooks() {
-		// if there's at least one webhook for validating SCEP challenges, the
-		// webhook will be used to perform challenge validation.
-		if wh.Kind == linkedca.Webhook_SCEPCHALLENGE.String() {
-			checkMethod = checkMethodWebhook
-			break
-		}
-	}
-
 	// NOTE: we're blocking the RenewalReq if the challenge does not match, because otherwise we don't have any authentication.
 	// The macOS SCEP client performs renewals using PKCSreq. The CertNanny SCEP client will use PKCSreq with challenge too, it seems,
 	// even if using the renewal flow as described in the README.md. MicroMDM SCEP client also only does PKCSreq by default, unless
@@ -334,13 +323,16 @@ func PKIOperation(ctx context.Context, req request) (Response, error) {
 		// auth.MatchChallengePassword interface/method. Will need to think about methods
 		// that don't just check the password, but do different things on success and
 		// failure too.
-		switch checkMethod {
+		switch selectValidationMethod(prov) {
-		case checkMethodWebhook:
+		case validationMethodWebhook:
 			c, err := webhook.New(prov.GetOptions().GetWebhooks())
 			if err != nil {
 				return createFailureResponse(ctx, csr, msg, microscep.BadRequest, errors.New("failed creating SCEP validation webhook controller"))
 			}
-			if err := c.Validate(msg.CSRReqMessage.ChallengePassword); err != nil {
+			if err := c.Validate(ctx, msg.CSRReqMessage.ChallengePassword); 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:
@@ -350,7 +342,7 @@ func PKIOperation(ctx context.Context, req request) (Response, error) {
 			}
 			if !challengeMatches {
 				// TODO: can this be returned safely to the client? In the end, if the password was correct, that gains a bit of info too.
-				return createFailureResponse(ctx, csr, msg, microscep.BadRequest, errors.New("wrong chalenge password provided"))
+				return createFailureResponse(ctx, csr, msg, microscep.BadRequest, errors.New("invalid challenge password provided"))
 			}
 		}
 	}
@@ -377,6 +369,28 @@ func PKIOperation(ctx context.Context, req request) (Response, error) {
 	return res, nil
 }
 
+type validationMethod string
+
+const (
+	validationMethodStatic  validationMethod = "static"
+	validationMethodWebhook validationMethod = "webhook"
+)
+
+// selectValidationMethod returns the method to validate SCEP
+// challenges. If a webhook is configured with kind `SCEPCHALLENGE`,
+// the webhook will be used. Otherwise it will default to the
+// static challenge value.
+func selectValidationMethod(p scep.Provisioner) validationMethod {
+	for _, wh := range p.GetOptions().GetWebhooks() {
+		// if there's at least one webhook for validating SCEP challenges, the
+		// webhook will be used to perform challenge validation.
+		if wh.Kind == linkedca.Webhook_SCEPCHALLENGE.String() {
+			return validationMethodWebhook
+		}
+	}
+	return validationMethodStatic
+}
+
 func formatCapabilities(caps []string) []byte {
 	return []byte(strings.Join(caps, "\r\n"))
 }

scep/api/webhook/webhook.go

@@ -1,6 +1,8 @@
 package webhook
 
 import (
+	"context"
+	"fmt"
 	"net/http"
 
 	"go.step.sm/linkedca"
@@ -9,11 +11,13 @@ import (
 	"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,
@@ -21,10 +25,13 @@ func New(webhooks []*provisioner.Webhook) (*Controller, error) {
 	}, nil
 }
 
-func (c *Controller) Validate(challenge string) error {
+// Validate executes zero or more configured webhooks to
-	if c == nil {
+// validate the SCEP challenge. If at least one of indicates
-		return nil
+// the challenge value is accepted, validation succeeds. Other
-	}
+// webhooks will not be executed. If none of the webhooks
+// indicates the challenge is accepted, an error is
+// returned.
+func (c *Controller) Validate(ctx context.Context, challenge string) error {
 	for _, wh := range c.webhooks {
 		if wh.Kind != linkedca.Webhook_SCEPCHALLENGE.String() {
 			continue
@@ -35,17 +42,20 @@ func (c *Controller) Validate(challenge string) error {
 		req := &webhook.RequestBody{
 			SCEPChallenge: challenge,
 		}
-		resp, err := wh.Do(c.client, req, nil) // TODO(hs): support templated URL?
+		resp, err := wh.DoWithContext(ctx, c.client, req, nil) // TODO(hs): support templated URL? Requires some refactoring
 		if err != nil {
-			return err
+			return fmt.Errorf("failed executing webhook request: %w", err)
 		}
-		if !resp.Allow {
+		if resp.Allow {
-			return provisioner.ErrWebhookDenied
+			return nil // return early when response is positive
 		}
 	}
-	return nil
+
+	return provisioner.ErrWebhookDenied
 }
 
+// isCertTypeOK returns whether or not the webhook is for X.509
+// certificates.
 func (c *Controller) isCertTypeOK(wh *provisioner.Webhook) bool {
 	return linkedca.Webhook_X509.String() == wh.CertType
 }

scep/authority.go

@@ -284,7 +284,6 @@ func (a *Authority) SignCSR(ctx context.Context, csr *x509.CertificateRequest, m
 	// Unlike most of the provisioners, scep's AuthorizeSign method doesn't
 	// define the templates, and the template data used in WebHooks is not
 	// available.
-	// TODO(hs): pass in challenge password to this webhook controller too?
 	for _, signOp := range signOps {
 		if wc, ok := signOp.(*provisioner.WebhookController); ok {
 			wc.TemplateData = data