From 05f7ab979f2aafe61489aae08cf8edb8934fa351 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Fri, 28 Apr 2023 15:47:22 +0200 Subject: [PATCH 1/9] Create basic webhook for SCEP challenge validation --- go.mod | 4 + go.sum | 7 ++ scep/api/api.go | 58 +++++++++++-- scep/api/webhook/options.go | 24 ++++++ scep/api/webhook/webhook.go | 161 ++++++++++++++++++++++++++++++++++++ scep/authority.go | 8 +- scep/common.go | 4 +- 7 files changed, 253 insertions(+), 13 deletions(-) create mode 100644 scep/api/webhook/options.go create mode 100644 scep/api/webhook/webhook.go diff --git a/go.mod b/go.mod index 0b59f165..a469dcb6 100644 --- a/go.mod +++ b/go.mod @@ -106,6 +106,8 @@ require ( github.com/jackc/pgx/v4 v4.18.0 // indirect github.com/jmespath/go-jmespath v0.4.0 // indirect github.com/klauspost/compress v1.15.11 // indirect + github.com/kr/pretty v0.3.1 // indirect + github.com/kr/text v0.2.0 // indirect github.com/kylelemons/godebug v1.1.0 // indirect github.com/manifoldco/promptui v0.9.0 // indirect github.com/mattn/go-colorable v0.1.8 // indirect @@ -119,8 +121,10 @@ require ( github.com/peterbourgon/diskv/v3 v3.0.1 // indirect github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect + github.com/rogpeppe/go-internal v1.9.0 // indirect github.com/russross/blackfriday/v2 v2.1.0 // indirect github.com/ryanuber/go-glob v1.0.0 // indirect + github.com/ryboe/q v1.0.19 // indirect github.com/schollz/jsonstore v1.1.0 // indirect github.com/shopspring/decimal v1.2.0 // indirect github.com/shurcooL/sanitized_anchor_name v1.0.0 // indirect diff --git a/go.sum b/go.sum index 7f417b36..f91d5a9c 100644 --- a/go.sum +++ b/go.sum @@ -657,6 +657,8 @@ github.com/kr/logfmt v0.0.0-20140226030751-b84e30acd515/go.mod h1:+0opPa2QZZtGFB github.com/kr/pretty v0.1.0/go.mod h1:dAy3ld7l9f0ibDNOQOHHMYYIIbhfbHSm3C4ZsoJORNo= github.com/kr/pretty v0.2.0/go.mod h1:ipq/a2n7PKx3OHsz4KJII5eveXtPO4qwEXGdVfWzfnI= github.com/kr/pretty v0.2.1/go.mod h1:ipq/a2n7PKx3OHsz4KJII5eveXtPO4qwEXGdVfWzfnI= +github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE= +github.com/kr/pretty v0.3.1/go.mod h1:hoEshYVHaxMs3cyo3Yncou5ZscifuDolrwPKZanG3xk= github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ= github.com/kr/pty v1.1.8/go.mod h1:O1sed60cT9XZ5uDucP5qwvh+TE3NnUj51EiZO/lmSfw= github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= @@ -794,6 +796,7 @@ github.com/pierrec/lz4 v1.0.2-0.20190131084431-473cd7ce01a1/go.mod h1:3/3N9NVKO0 github.com/pierrec/lz4 v2.0.5+incompatible/go.mod h1:pdkljMzZIN41W+lC3N2tnIh5sFi+IEE17M5jbnwPHcY= github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8 h1:KoWmjvw+nsYOo29YJK9vDA65RGE3NrOnUtO7a+RF9HU= github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8/go.mod h1:HKlIX3XHQyzLZPlr7++PzdhaXEj94dEiJgZDTsxEqUI= +github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e/go.mod h1:pJLUxLENpZxwdsKMEsNbx1VGcRFpLqf3715MtcvvzbA= github.com/pkg/errors v0.8.0/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4= @@ -844,6 +847,8 @@ github.com/rogpeppe/fastuuid v0.0.0-20150106093220-6724a57986af/go.mod h1:XWv6So github.com/rogpeppe/fastuuid v1.1.0/go.mod h1:jVj6XXZzXRy/MSR5jhDC/2q6DgLz+nrA6LYCDYWNEvQ= github.com/rogpeppe/fastuuid v1.2.0/go.mod h1:jVj6XXZzXRy/MSR5jhDC/2q6DgLz+nrA6LYCDYWNEvQ= github.com/rogpeppe/go-internal v1.3.0/go.mod h1:M8bDsm7K2OlrFYOpmOWEs/qY81heoFRclV5y23lUDJ4= +github.com/rogpeppe/go-internal v1.9.0 h1:73kH8U+JUqXU8lRuOHeVHaa/SZPifC7BkcraZVejAe8= +github.com/rogpeppe/go-internal v1.9.0/go.mod h1:WtVeX8xhTBvf0smdhujwtBcq4Qrzq/fJaraNFVN+nFs= github.com/rs/cors v1.7.0/go.mod h1:gFx+x8UowdsKA9AchylcLynDq+nNFfI8FkUZdN/jGCU= github.com/rs/cors v1.8.0/go.mod h1:EBwu+T5AvHOcXwvZIkQFjUN6s8Czyqw12GL/Y0tUyRM= github.com/rs/xid v1.2.1/go.mod h1:+uKXf+4Djp6Md1KODXJxgGQPKngRmWyn10oCKFzNHOQ= @@ -859,6 +864,8 @@ github.com/ryanuber/columnize v0.0.0-20160712163229-9b3edd62028f/go.mod h1:sm1tb github.com/ryanuber/columnize v2.1.0+incompatible/go.mod h1:sm1tb6uqfes/u+d4ooFouqFdy9/2g9QGwK3SQygK0Ts= github.com/ryanuber/go-glob v1.0.0 h1:iQh3xXAumdQ+4Ufa5b25cRpC5TYKlno6hsv6Cb3pkBk= github.com/ryanuber/go-glob v1.0.0/go.mod h1:807d1WSdnB0XRJzKNil9Om6lcp/3a0v4qIHxIXzX/Yc= +github.com/ryboe/q v1.0.19 h1:1dO1anK4gorZRpXBD/edBZkMxIC1tFIwN03nfyOV13A= +github.com/ryboe/q v1.0.19/go.mod h1:IoEB3Q2/p6n1qbhIQVuNyakxtnV4rNJ/XJPK+jsEa0M= github.com/samuel/go-zookeeper v0.0.0-20190923202752-2cc03de413da/go.mod h1:gi+0XIa01GRL2eRQVjQkKGqKF3SF9vZR/HnPullcV2E= github.com/sassoftware/go-rpmutils v0.0.0-20190420191620-a8f1baeba37b/go.mod h1:am+Fp8Bt506lA3Rk3QCmSqmYmLMnPDhdDUcosQCAx+I= github.com/satori/go.uuid v1.2.0/go.mod h1:dA0hQrYB0VpLJoorglMZABFdXlWrHn1NEOzdhQKdks0= diff --git a/scep/api/api.go b/scep/api/api.go index 346b9c75..66118388 100644 --- a/scep/api/api.go +++ b/scep/api/api.go @@ -14,12 +14,14 @@ import ( "github.com/go-chi/chi" microscep "github.com/micromdm/scep/v2/scep" + "github.com/ryboe/q" "go.mozilla.org/pkcs7" "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 ( @@ -306,19 +308,61 @@ 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? + if err != nil { + return Response{}, err + } + + _ = prov + q.Q(prov) + + // TODO(hs): set the checking method based on what's configured in provisioner. Or try something dynamic. + const checkMethodWebhook string = "webhook" + checkMethod := checkMethodWebhook + // 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 // 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 { - challengeMatches, err := auth.MatchChallengePassword(ctx, msg.CSRReqMessage.ChallengePassword) - if err != nil { - return createFailureResponse(ctx, csr, msg, microscep.BadRequest, errors.New("error when checking password")) - } - 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 password provided")) + // TODO(hs): might be nice use strategy pattern implementation; maybe behind the + // 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: + // TODO(hs): implement webhook call: needs endpoint, auth, request body + // TODO(hs): integrate this with the existing webhook implementation by extending it + fmt.Println("test") + q.Q("HERE") + q.Q(msg.CSRReqMessage) + opts := []webhook.ControllerOption{ + webhook.WithURL("http://127.0.0.1:8081/scepvalidate"), + webhook.WithBearerToken("fake-token"), + } + c, err := webhook.New(opts...) + if err != nil { + return createFailureResponse(ctx, csr, msg, microscep.BadRequest, errors.New("failed creating SCEP validation webhook controller")) + } + q.Q(c) + ok, err := c.Validate(msg.CSRReqMessage.ChallengePassword) + if err != nil { + q.Q(err) + return createFailureResponse(ctx, csr, msg, microscep.BadRequest, errors.New("failed validating challenge password")) + } + if !ok { + return createFailureResponse(ctx, csr, msg, microscep.BadRequest, errors.New("wrong challenge password provided")) + } + default: + challengeMatches, err := auth.MatchChallengePassword(ctx, msg.CSRReqMessage.ChallengePassword) + if err != nil { + return createFailureResponse(ctx, csr, msg, microscep.BadRequest, errors.New("error when checking password")) + } + 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")) + } } } diff --git a/scep/api/webhook/options.go b/scep/api/webhook/options.go new file mode 100644 index 00000000..ce809cb4 --- /dev/null +++ b/scep/api/webhook/options.go @@ -0,0 +1,24 @@ +package webhook + +type ControllerOption func(*Controller) error + +func WithURL(url string) ControllerOption { + return func(c *Controller) error { + c.webhook.URL = url + return nil + } +} + +func WithBearerToken(token string) ControllerOption { + return func(c *Controller) error { + c.webhook.BearerToken = token + return nil + } +} + +func WithDisableTLSClientAuth(enabled bool) ControllerOption { + return func(c *Controller) error { + c.webhook.DisableTLSClientAuth = enabled + return nil + } +} diff --git a/scep/api/webhook/webhook.go b/scep/api/webhook/webhook.go new file mode 100644 index 00000000..d3474c14 --- /dev/null +++ b/scep/api/webhook/webhook.go @@ -0,0 +1,161 @@ +package webhook + +import ( + "bytes" + "context" + "crypto/hmac" + "crypto/sha256" + "encoding/base64" + "encoding/hex" + "encoding/json" + "errors" + "fmt" + "log" + "net/http" + "time" + + "github.com/ryboe/q" +) + +type Controller struct { + client *http.Client + webhook *Webhook +} + +func New(options ...ControllerOption) (*Controller, error) { + c := &Controller{ + client: http.DefaultClient, + webhook: &Webhook{}, + } + for _, apply := range options { + if err := apply(c); err != nil { + return nil, err + } + } + return c, nil +} + +func (c *Controller) Validate(challenge string) (bool, error) { + req := &Request{ + Challenge: challenge, + } + client := c.client + if client == nil { + client = http.DefaultClient + } + resp, err := c.webhook.Do(client, req) + if err != nil { + q.Q(err) + return false, fmt.Errorf("failed performing webhook request: %w", err) + } + + if resp == nil { + return false, nil + } + + return true, nil +} + +type Webhook struct { + URL string + DisableTLSClientAuth bool + Secret string + BearerToken string + BasicAuth struct { + Username string + Password string + } +} + +func (w *Webhook) Do(client *http.Client, req *Request) (*Response, error) { + + ctx, cancel := context.WithTimeout(context.Background(), time.Second*10) + defer cancel() + + reqBytes, err := json.Marshal(req) + if err != nil { + return nil, err + } + + retries := 1 +retry: + + r, err := http.NewRequestWithContext(ctx, "POST", w.URL, bytes.NewReader(reqBytes)) + if err != nil { + return nil, err + } + + if w.Secret != "" { + secret, err := base64.StdEncoding.DecodeString(w.Secret) + if err != nil { + return nil, err + } + sig := hmac.New(sha256.New, secret).Sum(reqBytes) + r.Header.Set("X-Smallstep-Signature", hex.EncodeToString(sig)) + //req.Header.Set("X-Smallstep-Webhook-ID", w.ID) + } + + if w.BearerToken != "" { + r.Header.Set("Authorization", fmt.Sprintf("Bearer %s", w.BearerToken)) + } else if w.BasicAuth.Username != "" || w.BasicAuth.Password != "" { + r.SetBasicAuth(w.BasicAuth.Username, w.BasicAuth.Password) + } + if w.DisableTLSClientAuth { + transport, ok := client.Transport.(*http.Transport) + if !ok { + return nil, errors.New("client transport is not a *http.Transport") + } + transport = transport.Clone() + tlsConfig := transport.TLSClientConfig.Clone() + tlsConfig.GetClientCertificate = nil + tlsConfig.Certificates = nil + transport.TLSClientConfig = tlsConfig + client = &http.Client{ + Transport: transport, + } + } + + resp, err := client.Do(r) + if err != nil { + if errors.Is(err, context.DeadlineExceeded) { + return nil, err + } else if retries > 0 { + retries-- + time.Sleep(time.Second) + goto retry + } + return nil, err + } + defer func() { + if err := resp.Body.Close(); err != nil { + // TODO: return this error instead of (just) logging? + log.Printf("failed to close body of response from %s", w.URL) + } + }() + + if resp.StatusCode >= 500 && retries > 0 { + retries-- + time.Sleep(time.Second) + goto retry + } + if resp.StatusCode >= 400 { + return nil, fmt.Errorf("webhook server responded with %d", resp.StatusCode) + } + + respBody := &Response{} + // TODO: decide on the JSON structure for the response (if any); HTTP status code may be enough. + // if err := json.NewDecoder(resp.Body).Decode(respBody); err != nil { + // return nil, err + // } + + return respBody, nil +} + +type Request struct { + Challenge string `json:"challenge"` +} + +type Response struct { + // TODO: define expected response format? Or do we consider 200 OK enough? + Allow bool `json:"allow"` +} diff --git a/scep/authority.go b/scep/authority.go index 585b937e..9bfa20b8 100644 --- a/scep/authority.go +++ b/scep/authority.go @@ -161,7 +161,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 +235,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 } @@ -458,7 +458,7 @@ func (a *Authority) CreateFailureResponse(ctx context.Context, csr *x509.Certifi // MatchChallengePassword verifies a SCEP challenge password func (a *Authority) MatchChallengePassword(ctx context.Context, password string) (bool, error) { - p, err := provisionerFromContext(ctx) + p, err := ProvisionerFromContext(ctx) if err != nil { return false, err } @@ -476,7 +476,7 @@ func (a *Authority) MatchChallengePassword(ctx context.Context, password string) // 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 } diff --git a/scep/common.go b/scep/common.go index 73b16ed4..ca87841f 100644 --- a/scep/common.go +++ b/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") From 27cdcaf5ee293b3692944590db5f9009abfcc8a0 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Fri, 28 Apr 2023 17:15:05 +0200 Subject: [PATCH 2/9] Integrate the SCEP webhook with the existing webhook logic --- go.mod | 6 +- go.sum | 11 +-- scep/api/api.go | 35 +++----- scep/api/webhook/options.go | 24 ----- scep/api/webhook/webhook.go | 174 +++++++----------------------------- scep/authority.go | 1 + webhook/types.go | 2 + 7 files changed, 50 insertions(+), 203 deletions(-) delete mode 100644 scep/api/webhook/options.go diff --git a/go.mod b/go.mod index a469dcb6..17fcec58 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.0 + go.step.sm/linkedca v0.19.1-0.20230428150007-f95d2903af82 golang.org/x/crypto v0.8.0 golang.org/x/exp v0.0.0-20230310171629-522b1b587ee0 golang.org/x/net v0.9.0 @@ -106,8 +106,6 @@ require ( github.com/jackc/pgx/v4 v4.18.0 // indirect github.com/jmespath/go-jmespath v0.4.0 // indirect github.com/klauspost/compress v1.15.11 // indirect - github.com/kr/pretty v0.3.1 // indirect - github.com/kr/text v0.2.0 // indirect github.com/kylelemons/godebug v1.1.0 // indirect github.com/manifoldco/promptui v0.9.0 // indirect github.com/mattn/go-colorable v0.1.8 // indirect @@ -121,10 +119,8 @@ require ( github.com/peterbourgon/diskv/v3 v3.0.1 // indirect github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect - github.com/rogpeppe/go-internal v1.9.0 // indirect github.com/russross/blackfriday/v2 v2.1.0 // indirect github.com/ryanuber/go-glob v1.0.0 // indirect - github.com/ryboe/q v1.0.19 // indirect github.com/schollz/jsonstore v1.1.0 // indirect github.com/shopspring/decimal v1.2.0 // indirect github.com/shurcooL/sanitized_anchor_name v1.0.0 // indirect diff --git a/go.sum b/go.sum index f91d5a9c..1aa1170d 100644 --- a/go.sum +++ b/go.sum @@ -657,8 +657,6 @@ github.com/kr/logfmt v0.0.0-20140226030751-b84e30acd515/go.mod h1:+0opPa2QZZtGFB github.com/kr/pretty v0.1.0/go.mod h1:dAy3ld7l9f0ibDNOQOHHMYYIIbhfbHSm3C4ZsoJORNo= github.com/kr/pretty v0.2.0/go.mod h1:ipq/a2n7PKx3OHsz4KJII5eveXtPO4qwEXGdVfWzfnI= github.com/kr/pretty v0.2.1/go.mod h1:ipq/a2n7PKx3OHsz4KJII5eveXtPO4qwEXGdVfWzfnI= -github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE= -github.com/kr/pretty v0.3.1/go.mod h1:hoEshYVHaxMs3cyo3Yncou5ZscifuDolrwPKZanG3xk= github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ= github.com/kr/pty v1.1.8/go.mod h1:O1sed60cT9XZ5uDucP5qwvh+TE3NnUj51EiZO/lmSfw= github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= @@ -796,7 +794,6 @@ github.com/pierrec/lz4 v1.0.2-0.20190131084431-473cd7ce01a1/go.mod h1:3/3N9NVKO0 github.com/pierrec/lz4 v2.0.5+incompatible/go.mod h1:pdkljMzZIN41W+lC3N2tnIh5sFi+IEE17M5jbnwPHcY= github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8 h1:KoWmjvw+nsYOo29YJK9vDA65RGE3NrOnUtO7a+RF9HU= github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8/go.mod h1:HKlIX3XHQyzLZPlr7++PzdhaXEj94dEiJgZDTsxEqUI= -github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e/go.mod h1:pJLUxLENpZxwdsKMEsNbx1VGcRFpLqf3715MtcvvzbA= github.com/pkg/errors v0.8.0/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4= @@ -847,8 +844,6 @@ github.com/rogpeppe/fastuuid v0.0.0-20150106093220-6724a57986af/go.mod h1:XWv6So github.com/rogpeppe/fastuuid v1.1.0/go.mod h1:jVj6XXZzXRy/MSR5jhDC/2q6DgLz+nrA6LYCDYWNEvQ= github.com/rogpeppe/fastuuid v1.2.0/go.mod h1:jVj6XXZzXRy/MSR5jhDC/2q6DgLz+nrA6LYCDYWNEvQ= github.com/rogpeppe/go-internal v1.3.0/go.mod h1:M8bDsm7K2OlrFYOpmOWEs/qY81heoFRclV5y23lUDJ4= -github.com/rogpeppe/go-internal v1.9.0 h1:73kH8U+JUqXU8lRuOHeVHaa/SZPifC7BkcraZVejAe8= -github.com/rogpeppe/go-internal v1.9.0/go.mod h1:WtVeX8xhTBvf0smdhujwtBcq4Qrzq/fJaraNFVN+nFs= github.com/rs/cors v1.7.0/go.mod h1:gFx+x8UowdsKA9AchylcLynDq+nNFfI8FkUZdN/jGCU= github.com/rs/cors v1.8.0/go.mod h1:EBwu+T5AvHOcXwvZIkQFjUN6s8Czyqw12GL/Y0tUyRM= github.com/rs/xid v1.2.1/go.mod h1:+uKXf+4Djp6Md1KODXJxgGQPKngRmWyn10oCKFzNHOQ= @@ -864,8 +859,6 @@ github.com/ryanuber/columnize v0.0.0-20160712163229-9b3edd62028f/go.mod h1:sm1tb github.com/ryanuber/columnize v2.1.0+incompatible/go.mod h1:sm1tb6uqfes/u+d4ooFouqFdy9/2g9QGwK3SQygK0Ts= github.com/ryanuber/go-glob v1.0.0 h1:iQh3xXAumdQ+4Ufa5b25cRpC5TYKlno6hsv6Cb3pkBk= github.com/ryanuber/go-glob v1.0.0/go.mod h1:807d1WSdnB0XRJzKNil9Om6lcp/3a0v4qIHxIXzX/Yc= -github.com/ryboe/q v1.0.19 h1:1dO1anK4gorZRpXBD/edBZkMxIC1tFIwN03nfyOV13A= -github.com/ryboe/q v1.0.19/go.mod h1:IoEB3Q2/p6n1qbhIQVuNyakxtnV4rNJ/XJPK+jsEa0M= github.com/samuel/go-zookeeper v0.0.0-20190923202752-2cc03de413da/go.mod h1:gi+0XIa01GRL2eRQVjQkKGqKF3SF9vZR/HnPullcV2E= github.com/sassoftware/go-rpmutils v0.0.0-20190420191620-a8f1baeba37b/go.mod h1:am+Fp8Bt506lA3Rk3QCmSqmYmLMnPDhdDUcosQCAx+I= github.com/satori/go.uuid v1.2.0/go.mod h1:dA0hQrYB0VpLJoorglMZABFdXlWrHn1NEOzdhQKdks0= @@ -1039,8 +1032,8 @@ go.step.sm/cli-utils v0.7.6 h1:YkpLVrepmy2c5+eaz/wduiGxlgrRx3YdAStE37if25g= go.step.sm/cli-utils v0.7.6/go.mod h1:j+FxFZ2gbWkAJl0eded/rksuxmNqWpmyxbkXcukGJaY= 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.0 h1:xuagkR35wrJI2gnu6FAM+q3VmjwsHScvGcJsfZ0GdsI= -go.step.sm/linkedca v0.19.0/go.mod h1:b7vWPrHfYLEOTSUZitFEcztVCpTc+ileIN85CwEAluM= +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.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 66118388..9e659887 100644 --- a/scep/api/api.go +++ b/scep/api/api.go @@ -14,8 +14,8 @@ import ( "github.com/go-chi/chi" microscep "github.com/micromdm/scep/v2/scep" - "github.com/ryboe/q" "go.mozilla.org/pkcs7" + "go.step.sm/linkedca" "github.com/smallstep/certificates/api" "github.com/smallstep/certificates/api/log" @@ -313,12 +313,16 @@ func PKIOperation(ctx context.Context, req request) (Response, error) { return Response{}, err } - _ = prov - q.Q(prov) - - // TODO(hs): set the checking method based on what's configured in provisioner. Or try something dynamic. const checkMethodWebhook string = "webhook" - checkMethod := checkMethodWebhook + 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, @@ -332,28 +336,13 @@ func PKIOperation(ctx context.Context, req request) (Response, error) { // failure too. switch checkMethod { case checkMethodWebhook: - // TODO(hs): implement webhook call: needs endpoint, auth, request body - // TODO(hs): integrate this with the existing webhook implementation by extending it - fmt.Println("test") - q.Q("HERE") - q.Q(msg.CSRReqMessage) - opts := []webhook.ControllerOption{ - webhook.WithURL("http://127.0.0.1:8081/scepvalidate"), - webhook.WithBearerToken("fake-token"), - } - c, err := webhook.New(opts...) + 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")) } - q.Q(c) - ok, err := c.Validate(msg.CSRReqMessage.ChallengePassword) - if err != nil { - q.Q(err) + if err := c.Validate(msg.CSRReqMessage.ChallengePassword); err != nil { return createFailureResponse(ctx, csr, msg, microscep.BadRequest, errors.New("failed validating challenge password")) } - if !ok { - return createFailureResponse(ctx, csr, msg, microscep.BadRequest, errors.New("wrong challenge password provided")) - } default: challengeMatches, err := auth.MatchChallengePassword(ctx, msg.CSRReqMessage.ChallengePassword) if err != nil { diff --git a/scep/api/webhook/options.go b/scep/api/webhook/options.go deleted file mode 100644 index ce809cb4..00000000 --- a/scep/api/webhook/options.go +++ /dev/null @@ -1,24 +0,0 @@ -package webhook - -type ControllerOption func(*Controller) error - -func WithURL(url string) ControllerOption { - return func(c *Controller) error { - c.webhook.URL = url - return nil - } -} - -func WithBearerToken(token string) ControllerOption { - return func(c *Controller) error { - c.webhook.BearerToken = token - return nil - } -} - -func WithDisableTLSClientAuth(enabled bool) ControllerOption { - return func(c *Controller) error { - c.webhook.DisableTLSClientAuth = enabled - return nil - } -} diff --git a/scep/api/webhook/webhook.go b/scep/api/webhook/webhook.go index d3474c14..63fdd533 100644 --- a/scep/api/webhook/webhook.go +++ b/scep/api/webhook/webhook.go @@ -1,161 +1,51 @@ package webhook import ( - "bytes" - "context" - "crypto/hmac" - "crypto/sha256" - "encoding/base64" - "encoding/hex" - "encoding/json" - "errors" - "fmt" - "log" "net/http" - "time" - "github.com/ryboe/q" + "go.step.sm/linkedca" + + "github.com/smallstep/certificates/authority/provisioner" + "github.com/smallstep/certificates/webhook" ) type Controller struct { - client *http.Client - webhook *Webhook + client *http.Client + webhooks []*provisioner.Webhook } -func New(options ...ControllerOption) (*Controller, error) { - c := &Controller{ - client: http.DefaultClient, - webhook: &Webhook{}, +func New(webhooks []*provisioner.Webhook) (*Controller, error) { + return &Controller{ + client: http.DefaultClient, + webhooks: webhooks, + }, nil +} + +func (c *Controller) Validate(challenge string) error { + if c == nil { + return nil } - for _, apply := range options { - if err := apply(c); err != nil { - return nil, err + for _, wh := range c.webhooks { + if wh.Kind != linkedca.Webhook_SCEPCHALLENGE.String() { + continue } - } - return c, nil -} - -func (c *Controller) Validate(challenge string) (bool, error) { - req := &Request{ - Challenge: challenge, - } - client := c.client - if client == nil { - client = http.DefaultClient - } - resp, err := c.webhook.Do(client, req) - if err != nil { - q.Q(err) - return false, fmt.Errorf("failed performing webhook request: %w", err) - } - - if resp == nil { - return false, nil - } - - return true, nil -} - -type Webhook struct { - URL string - DisableTLSClientAuth bool - Secret string - BearerToken string - BasicAuth struct { - Username string - Password string - } -} - -func (w *Webhook) Do(client *http.Client, req *Request) (*Response, error) { - - ctx, cancel := context.WithTimeout(context.Background(), time.Second*10) - defer cancel() - - reqBytes, err := json.Marshal(req) - if err != nil { - return nil, err - } - - retries := 1 -retry: - - r, err := http.NewRequestWithContext(ctx, "POST", w.URL, bytes.NewReader(reqBytes)) - if err != nil { - return nil, err - } - - if w.Secret != "" { - secret, err := base64.StdEncoding.DecodeString(w.Secret) + if !c.isCertTypeOK(wh) { + continue + } + req := &webhook.RequestBody{ + SCEPChallenge: challenge, + } + resp, err := wh.Do(c.client, req, nil) // TODO(hs): support templated URL? if err != nil { - return nil, err + return err } - sig := hmac.New(sha256.New, secret).Sum(reqBytes) - r.Header.Set("X-Smallstep-Signature", hex.EncodeToString(sig)) - //req.Header.Set("X-Smallstep-Webhook-ID", w.ID) - } - - if w.BearerToken != "" { - r.Header.Set("Authorization", fmt.Sprintf("Bearer %s", w.BearerToken)) - } else if w.BasicAuth.Username != "" || w.BasicAuth.Password != "" { - r.SetBasicAuth(w.BasicAuth.Username, w.BasicAuth.Password) - } - if w.DisableTLSClientAuth { - transport, ok := client.Transport.(*http.Transport) - if !ok { - return nil, errors.New("client transport is not a *http.Transport") - } - transport = transport.Clone() - tlsConfig := transport.TLSClientConfig.Clone() - tlsConfig.GetClientCertificate = nil - tlsConfig.Certificates = nil - transport.TLSClientConfig = tlsConfig - client = &http.Client{ - Transport: transport, + if !resp.Allow { + return provisioner.ErrWebhookDenied } } - - resp, err := client.Do(r) - if err != nil { - if errors.Is(err, context.DeadlineExceeded) { - return nil, err - } else if retries > 0 { - retries-- - time.Sleep(time.Second) - goto retry - } - return nil, err - } - defer func() { - if err := resp.Body.Close(); err != nil { - // TODO: return this error instead of (just) logging? - log.Printf("failed to close body of response from %s", w.URL) - } - }() - - if resp.StatusCode >= 500 && retries > 0 { - retries-- - time.Sleep(time.Second) - goto retry - } - if resp.StatusCode >= 400 { - return nil, fmt.Errorf("webhook server responded with %d", resp.StatusCode) - } - - respBody := &Response{} - // TODO: decide on the JSON structure for the response (if any); HTTP status code may be enough. - // if err := json.NewDecoder(resp.Body).Decode(respBody); err != nil { - // return nil, err - // } - - return respBody, nil + return nil } -type Request struct { - Challenge string `json:"challenge"` -} - -type Response struct { - // TODO: define expected response format? Or do we consider 200 OK enough? - Allow bool `json:"allow"` +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 9bfa20b8..c1304bb7 100644 --- a/scep/authority.go +++ b/scep/authority.go @@ -284,6 +284,7 @@ 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 diff --git a/webhook/types.go b/webhook/types.go index 19624f5c..a1e10efe 100644 --- a/webhook/types.go +++ b/webhook/types.go @@ -68,4 +68,6 @@ type RequestBody struct { X509Certificate *X509Certificate `json:"x509Certificate,omitempty"` SSHCertificateRequest *SSHCertificateRequest `json:"sshCertificateRequest,omitempty"` SSHCertificate *SSHCertificate `json:"sshCertificate,omitempty"` + // Only set for SCEP requests + SCEPChallenge string `json:"scepChallenge,omitempty"` } From 419478d1e563cb6e24d7b1f65b01cd105a96e0ae Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Sat, 29 Apr 2023 01:15:39 +0200 Subject: [PATCH 3/9] 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 From ad4d8e6c68949202e794fa90b81e893fa216c2fb Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Sat, 29 Apr 2023 01:40:03 +0200 Subject: [PATCH 4/9] Add `SCEPCHALLENGE` as valid webhook type in admin API --- authority/admin/api/webhook.go | 4 ++-- authority/admin/api/webhook_test.go | 20 ++++++++++++++++++++ scep/api/webhook/webhook.go | 7 +++++-- 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/authority/admin/api/webhook.go b/authority/admin/api/webhook.go index f73f6806..3939d55e 100644 --- a/authority/admin/api/webhook.go +++ b/authority/admin/api/webhook.go @@ -57,9 +57,9 @@ func validateWebhook(webhook *linkedca.Webhook) error { // kind switch webhook.Kind { - case linkedca.Webhook_ENRICHING, linkedca.Webhook_AUTHORIZING: + case linkedca.Webhook_ENRICHING, linkedca.Webhook_AUTHORIZING, linkedca.Webhook_SCEPCHALLENGE: default: - return admin.NewError(admin.ErrorBadRequestType, "webhook kind is invalid") + return admin.NewError(admin.ErrorBadRequestType, "webhook kind %q is invalid", webhook.Kind) } return nil diff --git a/authority/admin/api/webhook_test.go b/authority/admin/api/webhook_test.go index baac2c11..0fb199f0 100644 --- a/authority/admin/api/webhook_test.go +++ b/authority/admin/api/webhook_test.go @@ -180,6 +180,26 @@ func TestWebhookAdminResponder_CreateProvisionerWebhook(t *testing.T) { statusCode: 400, } }, + "fail/unsupported-webhook-kind": func(t *testing.T) test { + prov := &linkedca.Provisioner{ + Name: "provName", + } + ctx := linkedca.NewContextWithProvisioner(context.Background(), prov) + adminErr := admin.NewError(admin.ErrorBadRequestType, `(line 5:13): invalid value for enum type: "UNSUPPORTED"`) + adminErr.Message = `(line 5:13): invalid value for enum type: "UNSUPPORTED"` + body := []byte(` + { + "name": "metadata", + "url": "https://example.com", + "kind": "UNSUPPORTED", + }`) + return test{ + ctx: ctx, + body: body, + err: adminErr, + statusCode: 400, + } + }, "fail/auth.UpdateProvisioner-error": func(t *testing.T) test { adm := &linkedca.Admin{ Subject: "step", diff --git a/scep/api/webhook/webhook.go b/scep/api/webhook/webhook.go index 07dafd78..b191c426 100644 --- a/scep/api/webhook/webhook.go +++ b/scep/api/webhook/webhook.go @@ -54,8 +54,11 @@ func (c *Controller) Validate(ctx context.Context, challenge string) error { return provisioner.ErrWebhookDenied } -// isCertTypeOK returns whether or not the webhook is for X.509 -// certificates. +// isCertTypeOK returns whether or not the webhook can be used +// with the SCEP challenge validation webhook controller. func (c *Controller) isCertTypeOK(wh *provisioner.Webhook) bool { + if wh.CertType == linkedca.Webhook_ALL.String() || wh.CertType == "" { + return true + } return linkedca.Webhook_X509.String() == wh.CertType } From 5f0f0f4bccf429ef45a0c6c52fafe86c2b5abc7e Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Mon, 1 May 2023 11:14:50 +0200 Subject: [PATCH 5/9] Add SCEP webhook validation tests --- scep/api/api.go | 36 ++++--- scep/api/api_test.go | 50 +++++++++ scep/api/webhook/webhook.go | 5 +- scep/api/webhook/webhook_test.go | 176 +++++++++++++++++++++++++++++++ webhook/types.go | 5 +- 5 files changed, 256 insertions(+), 16 deletions(-) create mode 100644 scep/api/webhook/webhook_test.go diff --git a/scep/api/api.go b/scep/api/api.go index 96e25104..f6e1b1ce 100644 --- a/scep/api/api.go +++ b/scep/api/api.go @@ -305,14 +305,21 @@ func PKIOperation(ctx context.Context, req request) (Response, error) { return Response{}, err } - // NOTE: at this point we have sufficient information for returning nicely signed CertReps - csr := msg.CSRReqMessage.CSR - 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) + challengePassword := msg.CSRReqMessage.ChallengePassword + // 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 @@ -323,22 +330,22 @@ 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 selectValidationMethod(prov) { + switch selectValidationMethod(scepProv) { case validationMethodWebhook: - c, err := webhook.New(prov.GetOptions().GetWebhooks()) + 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, msg.CSRReqMessage.ChallengePassword); err != nil { + 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, msg.CSRReqMessage.ChallengePassword) + challengeMatches, err := auth.MatchChallengePassword(ctx, challengePassword) if err != nil { - return createFailureResponse(ctx, csr, msg, microscep.BadRequest, errors.New("error when checking password")) + return createFailureResponse(ctx, csr, msg, microscep.BadRequest, errors.New("failed checking password")) } 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. @@ -372,6 +379,7 @@ func PKIOperation(ctx context.Context, req request) (Response, error) { type validationMethod string const ( + validationMethodNone validationMethod = "none" validationMethodStatic validationMethod = "static" validationMethodWebhook validationMethod = "webhook" ) @@ -380,15 +388,19 @@ const ( // 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 { +func selectValidationMethod(p *provisioner.SCEP) 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 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 } } - return validationMethodStatic + if challenge := p.GetChallengePassword(); challenge != "" { + return validationMethodStatic + } + return validationMethodNone } func formatCapabilities(caps []string) []byte { diff --git a/scep/api/api_test.go b/scep/api/api_test.go index bdb51594..ee53d25e 100644 --- a/scep/api/api_test.go +++ b/scep/api/api_test.go @@ -9,6 +9,12 @@ 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) { @@ -111,3 +117,47 @@ 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(), + }, + }, + }, + Claims: &provisioner.Claims{}, + }, "webhook"}, + {"challenge", &provisioner.SCEP{ + Name: "SCEP", + Type: "SCEP", + ChallengePassword: "pass", + Options: &provisioner.Options{}, + Claims: &provisioner.Claims{}, + }, "static"}, + {"none", &provisioner.SCEP{ + Name: "SCEP", + Type: "SCEP", + Options: &provisioner.Options{}, + Claims: &provisioner.Claims{}, + }, "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) + }) + } +} diff --git a/scep/api/webhook/webhook.go b/scep/api/webhook/webhook.go index b191c426..dbaa5749 100644 --- a/scep/api/webhook/webhook.go +++ b/scep/api/webhook/webhook.go @@ -31,7 +31,7 @@ func New(webhooks []*provisioner.Webhook) (*Controller, error) { // 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 { +func (c *Controller) Validate(ctx context.Context, challenge, transactionID string) error { for _, wh := range c.webhooks { if wh.Kind != linkedca.Webhook_SCEPCHALLENGE.String() { continue @@ -40,7 +40,8 @@ func (c *Controller) Validate(ctx context.Context, challenge string) error { continue } req := &webhook.RequestBody{ - SCEPChallenge: challenge, + SCEPChallenge: challenge, + SCEPTransactionID: transactionID, } resp, err := wh.DoWithContext(ctx, c.client, req, nil) // TODO(hs): support templated URL? Requires some refactoring if err != nil { diff --git a/scep/api/webhook/webhook_test.go b/scep/api/webhook/webhook_test.go new file mode 100644 index 00000000..5d8012ac --- /dev/null +++ b/scep/api/webhook/webhook_test.go @@ -0,0 +1,176 @@ +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) { + c := &Controller{} + assert.True(t, c.isCertTypeOK(&provisioner.Webhook{CertType: linkedca.Webhook_X509.String()})) + assert.True(t, c.isCertTypeOK(&provisioner.Webhook{CertType: linkedca.Webhook_ALL.String()})) + assert.True(t, c.isCertTypeOK(&provisioner.Webhook{CertType: ""})) + assert.False(t, c.isCertTypeOK(&provisioner.Webhook{CertType: linkedca.Webhook_SSH.String()})) +} diff --git a/webhook/types.go b/webhook/types.go index a1e10efe..9605742a 100644 --- a/webhook/types.go +++ b/webhook/types.go @@ -68,6 +68,7 @@ type RequestBody struct { X509Certificate *X509Certificate `json:"x509Certificate,omitempty"` SSHCertificateRequest *SSHCertificateRequest `json:"sshCertificateRequest,omitempty"` SSHCertificate *SSHCertificate `json:"sshCertificate,omitempty"` - // Only set for SCEP requests - SCEPChallenge string `json:"scepChallenge,omitempty"` + // Only set for SCEP challenge validation requests + SCEPChallenge string `json:"scepChallenge,omitempty"` + SCEPTransactionID string `json:"scepTransactionID,omitempty"` } From 668ff9b515411dadc3ef6e50196a294ef96b2945 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Mon, 1 May 2023 11:55:05 +0200 Subject: [PATCH 6/9] Cleanup some comments and tests --- scep/api/api.go | 7 +++---- scep/api/api_test.go | 32 +++++++++++++++++++++++++------- scep/api/webhook/webhook.go | 14 +++++++------- scep/api/webhook/webhook_test.go | 9 ++++----- 4 files changed, 39 insertions(+), 23 deletions(-) diff --git a/scep/api/api.go b/scep/api/api.go index f6e1b1ce..1375b630 100644 --- a/scep/api/api.go +++ b/scep/api/api.go @@ -326,7 +326,7 @@ 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 use strategy pattern implementation; maybe behind the + // TODO(hs): might be nice to use strategy pattern implementation; maybe behind the // 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. @@ -348,7 +348,6 @@ func PKIOperation(ctx context.Context, req request) (Response, error) { return createFailureResponse(ctx, csr, msg, microscep.BadRequest, errors.New("failed checking password")) } 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("invalid challenge password provided")) } } @@ -386,8 +385,8 @@ const ( // 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. +// 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 diff --git a/scep/api/api_test.go b/scep/api/api_test.go index ee53d25e..63b76b3e 100644 --- a/scep/api/api_test.go +++ b/scep/api/api_test.go @@ -134,20 +134,38 @@ func Test_selectValidationMethod(t *testing.T) { }, }, }, - Claims: &provisioner.Claims{}, }, "webhook"}, {"challenge", &provisioner.SCEP{ Name: "SCEP", Type: "SCEP", ChallengePassword: "pass", - Options: &provisioner.Options{}, - Claims: &provisioner.Claims{}, + }, "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", - Options: &provisioner.Options{}, - Claims: &provisioner.Claims{}, + 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 { diff --git a/scep/api/webhook/webhook.go b/scep/api/webhook/webhook.go index dbaa5749..1e622c92 100644 --- a/scep/api/webhook/webhook.go +++ b/scep/api/webhook/webhook.go @@ -26,17 +26,17 @@ func New(webhooks []*provisioner.Webhook) (*Controller, error) { } // 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. +// 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 !c.isCertTypeOK(wh) { + if !isCertTypeOK(wh) { continue } req := &webhook.RequestBody{ @@ -57,7 +57,7 @@ func (c *Controller) Validate(ctx context.Context, challenge, transactionID stri // isCertTypeOK returns whether or not the webhook can be used // with the SCEP challenge validation webhook controller. -func (c *Controller) isCertTypeOK(wh *provisioner.Webhook) bool { +func isCertTypeOK(wh *provisioner.Webhook) bool { if wh.CertType == linkedca.Webhook_ALL.String() || wh.CertType == "" { return true } diff --git a/scep/api/webhook/webhook_test.go b/scep/api/webhook/webhook_test.go index 5d8012ac..3520d216 100644 --- a/scep/api/webhook/webhook_test.go +++ b/scep/api/webhook/webhook_test.go @@ -168,9 +168,8 @@ func TestController_Validate(t *testing.T) { } func TestController_isCertTypeOK(t *testing.T) { - c := &Controller{} - assert.True(t, c.isCertTypeOK(&provisioner.Webhook{CertType: linkedca.Webhook_X509.String()})) - assert.True(t, c.isCertTypeOK(&provisioner.Webhook{CertType: linkedca.Webhook_ALL.String()})) - assert.True(t, c.isCertTypeOK(&provisioner.Webhook{CertType: ""})) - assert.False(t, c.isCertTypeOK(&provisioner.Webhook{CertType: linkedca.Webhook_SSH.String()})) + 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()})) } From e8c1e8719d35aeedebd8ca19a407afc797a8f663 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Mon, 1 May 2023 22:09:42 +0200 Subject: [PATCH 7/9] Refactor SCEP webhook validation --- authority/provisioner/scep.go | 115 ++++++++++ authority/provisioner/scep_test.go | 343 +++++++++++++++++++++++++++++ scep/api/api.go | 66 +----- scep/api/api_test.go | 68 ------ scep/api/webhook/webhook.go | 65 ------ scep/api/webhook/webhook_test.go | 175 --------------- scep/authority.go | 33 +-- scep/common.go | 4 +- scep/provisioner.go | 2 +- 9 files changed, 476 insertions(+), 395 deletions(-) create mode 100644 authority/provisioner/scep_test.go delete mode 100644 scep/api/webhook/webhook.go delete mode 100644 scep/api/webhook/webhook_test.go diff --git a/authority/provisioner/scep.go b/authority/provisioner/scep.go index 0f27b206..0d71df58 100644 --- a/authority/provisioner/scep.go +++ b/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 +} diff --git a/authority/provisioner/scep_test.go b/authority/provisioner/scep_test.go new file mode 100644 index 00000000..906ad986 --- /dev/null +++ b/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) + }) + } +} diff --git a/scep/api/api.go b/scep/api/api.go index 1375b630..98da818b 100644 --- a/scep/api/api.go +++ b/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,30 +314,11 @@ 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 - // 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 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")) + if err := auth.ValidateChallenge(ctx, challengePassword, transactionID); err != nil { + if errors.Is(err, provisioner.ErrSCEPChallengeInvalid) { + return createFailureResponse(ctx, csr, msg, microscep.BadRequest, err) } + return createFailureResponse(ctx, csr, msg, microscep.BadRequest, errors.New("failed validating challenge password")) } } @@ -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")) } diff --git a/scep/api/api_test.go b/scep/api/api_test.go index 63b76b3e..bdb51594 100644 --- a/scep/api/api_test.go +++ b/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) - }) - } -} diff --git a/scep/api/webhook/webhook.go b/scep/api/webhook/webhook.go deleted file mode 100644 index 1e622c92..00000000 --- a/scep/api/webhook/webhook.go +++ /dev/null @@ -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 -} diff --git a/scep/api/webhook/webhook_test.go b/scep/api/webhook/webhook_test.go deleted file mode 100644 index 3520d216..00000000 --- a/scep/api/webhook/webhook_test.go +++ /dev/null @@ -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()})) -} diff --git a/scep/authority.go b/scep/authority.go index 9bfa20b8..8ba9c9c9 100644 --- a/scep/authority.go +++ b/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) +} diff --git a/scep/common.go b/scep/common.go index ca87841f..73b16ed4 100644 --- a/scep/common.go +++ b/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") diff --git a/scep/provisioner.go b/scep/provisioner.go index 679c6353..8120057e 100644 --- a/scep/provisioner.go +++ b/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 } From 4bb88adf63fe03b134dfb310aa6b725297137a8f Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Mon, 1 May 2023 23:59:48 +0200 Subject: [PATCH 8/9] Move SCEP checks after reload of provisioners in CA initialization --- authority/authority.go | 88 +++++++++++++++++++++--------------------- 1 file changed, 44 insertions(+), 44 deletions(-) diff --git a/authority/authority.go b/authority/authority.go index 7904a7ea..ae85c018 100644 --- a/authority/authority.go +++ b/authority/authority.go @@ -545,50 +545,6 @@ func (a *Authority) init() error { tmplVars.SSH.UserFederatedKeys = append(tmplVars.SSH.UserFederatedKeys, a.sshCAUserFederatedCerts...) } - // Check if a KMS with decryption capability is required and available - if a.requiresDecrypter() { - if _, ok := a.keyManager.(kmsapi.Decrypter); !ok { - return errors.New("keymanager doesn't provide crypto.Decrypter") - } - } - - // TODO: decide if this is a good approach for providing the SCEP functionality - // It currently mirrors the logic for the x509CAService - if a.requiresSCEPService() && a.scepService == nil { - var options scep.Options - - // Read intermediate and create X509 signer and decrypter for default CAS. - options.CertificateChain, err = pemutil.ReadCertificateBundle(a.config.IntermediateCert) - if err != nil { - return err - } - options.CertificateChain = append(options.CertificateChain, a.rootX509Certs...) - options.Signer, err = a.keyManager.CreateSigner(&kmsapi.CreateSignerRequest{ - SigningKey: a.config.IntermediateKey, - Password: a.password, - }) - if err != nil { - return err - } - - if km, ok := a.keyManager.(kmsapi.Decrypter); ok { - options.Decrypter, err = km.CreateDecrypter(&kmsapi.CreateDecrypterRequest{ - DecryptionKey: a.config.IntermediateKey, - Password: a.password, - }) - if err != nil { - return err - } - } - - a.scepService, err = scep.NewService(ctx, options) - if err != nil { - return err - } - - // TODO: mimick the x509CAService GetCertificateAuthority here too? - } - if a.config.AuthorityConfig.EnableAdmin { // Initialize step-ca Admin Database if it's not already initialized using // WithAdminDB. @@ -684,6 +640,50 @@ func (a *Authority) init() error { return err } + // Check if a KMS with decryption capability is required and available + if a.requiresDecrypter() { + if _, ok := a.keyManager.(kmsapi.Decrypter); !ok { + return errors.New("keymanager doesn't provide crypto.Decrypter") + } + } + + // TODO: decide if this is a good approach for providing the SCEP functionality + // It currently mirrors the logic for the x509CAService + if a.requiresSCEPService() && a.scepService == nil { + var options scep.Options + + // Read intermediate and create X509 signer and decrypter for default CAS. + options.CertificateChain, err = pemutil.ReadCertificateBundle(a.config.IntermediateCert) + if err != nil { + return err + } + options.CertificateChain = append(options.CertificateChain, a.rootX509Certs...) + options.Signer, err = a.keyManager.CreateSigner(&kmsapi.CreateSignerRequest{ + SigningKey: a.config.IntermediateKey, + Password: a.password, + }) + if err != nil { + return err + } + + if km, ok := a.keyManager.(kmsapi.Decrypter); ok { + options.Decrypter, err = km.CreateDecrypter(&kmsapi.CreateDecrypterRequest{ + DecryptionKey: a.config.IntermediateKey, + Password: a.password, + }) + if err != nil { + return err + } + } + + a.scepService, err = scep.NewService(ctx, options) + if err != nil { + return err + } + + // TODO: mimick the x509CAService GetCertificateAuthority here too? + } + // Load X509 constraints engine. // // This is currently only available in CA mode. From c73f157ea487376c1829e315a2f4c473740d393b Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Tue, 2 May 2023 00:52:11 +0200 Subject: [PATCH 9/9] Remove unused error from challenge validation controller creator --- authority/provisioner/scep.go | 10 ++++------ authority/provisioner/scep_test.go | 5 ++--- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/authority/provisioner/scep.go b/authority/provisioner/scep.go index 0d71df58..c20f9bf1 100644 --- a/authority/provisioner/scep.go +++ b/authority/provisioner/scep.go @@ -96,7 +96,7 @@ type challengeValidationController struct { // newChallengeValidationController creates a new challengeValidationController // that performs challenge validation through webhooks. -func newChallengeValidationController(client *http.Client, webhooks []*Webhook) (*challengeValidationController, error) { +func newChallengeValidationController(client *http.Client, webhooks []*Webhook) *challengeValidationController { scepHooks := []*Webhook{} for _, wh := range webhooks { if wh.Kind != linkedca.Webhook_SCEPCHALLENGE.String() { @@ -110,7 +110,7 @@ func newChallengeValidationController(client *http.Client, webhooks []*Webhook) return &challengeValidationController{ client: client, webhooks: scepHooks, - }, nil + } } var ( @@ -177,12 +177,10 @@ 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( + s.challengeValidationController = newChallengeValidationController( config.WebhookClient, s.GetOptions().GetWebhooks(), - ); err != nil { - return fmt.Errorf("failed creating challenge validation controller: %w", err) - } + ) // TODO: add other, SCEP specific, options? diff --git a/authority/provisioner/scep_test.go b/authority/provisioner/scep_test.go index 906ad986..acf047fb 100644 --- a/authority/provisioner/scep_test.go +++ b/authority/provisioner/scep_test.go @@ -134,15 +134,14 @@ func Test_challengeValidationController_Validate(t *testing.T) { } 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) + c := newChallengeValidationController(tt.fields.client, tt.fields.webhooks) if tt.server != nil { defer tt.server.Close() } ctx := context.Background() - err = c.Validate(ctx, tt.args.challenge, tt.args.transactionID) + err := c.Validate(ctx, tt.args.challenge, tt.args.transactionID) if tt.expErr != nil { assert.EqualError(t, err, tt.expErr.Error())