From 419478d1e563cb6e24d7b1f65b01cd105a96e0ae Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Sat, 29 Apr 2023 01:15:39 +0200 Subject: [PATCH] Make SCEP webhook validation look better --- authority/provisioner/webhook.go | 9 +++++-- go.mod | 2 +- go.sum | 2 ++ scep/api/api.go | 46 +++++++++++++++++++++----------- scep/api/webhook/webhook.go | 28 ++++++++++++------- scep/authority.go | 1 - 6 files changed, 59 insertions(+), 29 deletions(-) diff --git a/authority/provisioner/webhook.go b/authority/provisioner/webhook.go index ea02da35..cb15547d 100644 --- a/authority/provisioner/webhook.go +++ b/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() diff --git a/go.mod b/go.mod index 17fcec58..a30c2389 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index 1aa1170d..d5aca405 100644 --- a/go.sum +++ b/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= diff --git a/scep/api/api.go b/scep/api/api.go index 9e659887..96e25104 100644 --- a/scep/api/api.go +++ b/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 { - case checkMethodWebhook: + switch selectValidationMethod(prov) { + 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")) } diff --git a/scep/api/webhook/webhook.go b/scep/api/webhook/webhook.go index 63fdd533..07dafd78 100644 --- a/scep/api/webhook/webhook.go +++ b/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 { - if c == nil { - return nil - } +// Validate executes zero or more configured webhooks to +// validate the SCEP challenge. If at least one of indicates +// 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 { - return provisioner.ErrWebhookDenied + if resp.Allow { + 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 } diff --git a/scep/authority.go b/scep/authority.go index c1304bb7..9bfa20b8 100644 --- a/scep/authority.go +++ b/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