Cleanup some comments and tests

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
+// the webhook method will be used. If a challenge password is set,
-// static challenge value.
+// 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

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{},
+		}, "static"},
-			Claims:            &provisioner.Claims{},
+		{"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",
+			Name: "SCEP",
-			Type:    "SCEP",
+			Type: "SCEP",
-			Options: &provisioner.Options{},
+		}, "none"},
-			Claims:  &provisioner.Claims{},
+		{"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 {

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
+// validate the SCEP challenge. If at least one of them indicates
-// the challenge value is accepted, validation succeeds. Other
+// the challenge value is accepted, validation succeeds. In
-// webhooks will not be executed. If none of the webhooks
+// that case, the other webhooks will be skipped. If none of
-// indicates the challenge is accepted, an error is
+// the webhooks indicates the value of the challenge was accepted,
-// returned.
+// 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
 	}

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, isCertTypeOK(&provisioner.Webhook{CertType: linkedca.Webhook_X509.String()}))
-	assert.True(t, c.isCertTypeOK(&provisioner.Webhook{CertType: linkedca.Webhook_X509.String()}))
+	assert.True(t, isCertTypeOK(&provisioner.Webhook{CertType: linkedca.Webhook_ALL.String()}))
-	assert.True(t, c.isCertTypeOK(&provisioner.Webhook{CertType: linkedca.Webhook_ALL.String()}))
+	assert.True(t, isCertTypeOK(&provisioner.Webhook{CertType: ""}))
-	assert.True(t, c.isCertTypeOK(&provisioner.Webhook{CertType: ""}))
+	assert.False(t, isCertTypeOK(&provisioner.Webhook{CertType: linkedca.Webhook_SSH.String()}))
-	assert.False(t, c.isCertTypeOK(&provisioner.Webhook{CertType: linkedca.Webhook_SSH.String()}))
 }