From 668ff9b515411dadc3ef6e50196a294ef96b2945 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Mon, 1 May 2023 11:55:05 +0200 Subject: [PATCH] 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()})) }