From e8c1e8719d35aeedebd8ca19a407afc797a8f663 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Mon, 1 May 2023 22:09:42 +0200 Subject: [PATCH] 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 }