Merge pull request #2066 from smallstep/mariano/webhook-errors

maraino · web-flow · commit fdaa6657e167 · 2024-11-15T09:48:07.000-08:00
diff --git a/acme/order.go b/acme/order.go
@@ -7,6 +7,7 @@ import (
 	"crypto/x509"
 	"encoding/asn1"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"net"
 	"net/url"
@@ -19,6 +20,7 @@ import (
 
 	"github.com/smallstep/certificates/acme/wire"
 	"github.com/smallstep/certificates/authority/provisioner"
+	"github.com/smallstep/certificates/webhook"
 )
 
 type IdentifierType string
@@ -304,6 +306,17 @@ func (o *Order) Finalize(ctx context.Context, db DB, csr *x509.CertificateReques
 		NotAfter:  provisioner.NewTimeDuration(o.NotAfter),
 	}, signOps...)
 	if err != nil {
+		// Add subproblem for webhook errors, others can be added later.
+		var webhookErr *webhook.Error
+		if errors.As(err, &webhookErr) {
+			acmeError := NewDetailedError(ErrorUnauthorizedType, webhookErr.Error())
+			acmeError.AddSubproblems(Subproblem{
+				Type:   fmt.Sprintf("urn:smallstep:webhook:error:%s", webhookErr.Code),
+				Detail: webhookErr.Message,
+			})
+			return acmeError
+		}
+
 		return WrapErrorISE(err, "error signing certificate for order %s", o.ID)
 	}
 
diff --git a/acme/order_test.go b/acme/order_test.go
@@ -18,6 +18,8 @@ import (
 	"github.com/smallstep/assert"
 	"github.com/smallstep/certificates/authority"
 	"github.com/smallstep/certificates/authority/provisioner"
+	"github.com/smallstep/certificates/errs"
+	"github.com/smallstep/certificates/webhook"
 	"go.step.sm/crypto/keyutil"
 	"go.step.sm/crypto/x509util"
 )
@@ -590,6 +592,55 @@ func TestOrder_Finalize(t *testing.T) {
 				err: NewErrorISE("error signing certificate for order oID: force"),
 			}
 		},
+		"fail/webhook-error": func(t *testing.T) test {
+			now := clock.Now()
+			o := &Order{
+				ID:               "oID",
+				AccountID:        "accID",
+				Status:           StatusReady,
+				ExpiresAt:        now.Add(5 * time.Minute),
+				AuthorizationIDs: []string{"a", "b"},
+				Identifiers: []Identifier{
+					{Type: "dns", Value: "foo.internal"},
+					{Type: "dns", Value: "bar.internal"},
+				},
+			}
+			csr := &x509.CertificateRequest{
+				Subject: pkix.Name{
+					CommonName: "foo.internal",
+				},
+				DNSNames: []string{"bar.internal"},
+			}
+
+			return test{
+				o:   o,
+				csr: csr,
+				prov: &MockProvisioner{
+					MauthorizeSign: func(ctx context.Context, token string) ([]provisioner.SignOption, error) {
+						assert.Equals(t, token, "")
+						return nil, nil
+					},
+					MgetOptions: func() *provisioner.Options {
+						return nil
+					},
+				},
+				ca: &mockSignAuth{
+					signWithContext: func(_ context.Context, _csr *x509.CertificateRequest, signOpts provisioner.SignOptions, extraOpts ...provisioner.SignOption) ([]*x509.Certificate, error) {
+						assert.Equals(t, _csr, csr)
+						return nil, errs.ForbiddenErr(&webhook.Error{Code: "theCode", Message: "The message"}, "forbidden error")
+					},
+				},
+				db: &MockDB{
+					MockGetAuthorization: func(ctx context.Context, id string) (*Authorization, error) {
+						return &Authorization{ID: id, Status: StatusValid}, nil
+					},
+				},
+				err: NewDetailedError(ErrorUnauthorizedType, "The message (theCode)").AddSubproblems(Subproblem{
+					Type:   "urn:smallstep:webhook:error:theCode",
+					Detail: "The message",
+				}),
+			}
+		},
 		"fail/error-db.CreateCertificate": func(t *testing.T) test {
 			now := clock.Now()
 			o := &Order{
@@ -1217,6 +1268,7 @@ func TestOrder_Finalize(t *testing.T) {
 						assert.Equals(t, k.Status, tc.err.Status)
 						assert.Equals(t, k.Err.Error(), tc.err.Err.Error())
 						assert.Equals(t, k.Detail, tc.err.Detail)
+						assert.Equals(t, k.Subproblems, tc.err.Subproblems)
 					} else {
 						assert.FatalError(t, errors.New("unexpected error type"))
 					}
diff --git a/authority/provisioner/webhook.go b/authority/provisioner/webhook.go
@@ -65,6 +65,9 @@ func (wc *WebhookController) Enrich(ctx context.Context, req *webhook.RequestBod
 			return err
 		}
 		if !resp.Allow {
+			if resp.Error != nil {
+				return resp.Error
+			}
 			return ErrWebhookDenied
 		}
 		wc.TemplateData.SetWebhook(wh.Name, resp.Data)
@@ -101,6 +104,9 @@ func (wc *WebhookController) Authorize(ctx context.Context, req *webhook.Request
 			return err
 		}
 		if !resp.Allow {
+			if resp.Error != nil {
+				return resp.Error
+			}
 			return ErrWebhookDenied
 		}
 	}
diff --git a/authority/provisioner/webhook_test.go b/authority/provisioner/webhook_test.go
@@ -122,6 +122,7 @@ func TestWebhookController_Enrich(t *testing.T) {
 		expectErr          bool
 		expectTemplateData any
 		assertRequest      func(t *testing.T, req *webhook.RequestBody)
+		assertError        func(t *testing.T, err error)
 	}
 	tests := map[string]test{
 		"ok/no enriching webhooks": {
@@ -228,6 +229,28 @@ func TestWebhookController_Enrich(t *testing.T) {
 			responses:          []*webhook.ResponseBody{{Allow: false}},
 			expectErr:          true,
 			expectTemplateData: x509util.TemplateData{},
+			assertError: func(t *testing.T, err error) {
+				assert.Equal(t, ErrWebhookDenied, err)
+			},
+		},
+		"deny/with error": {
+			ctl: &WebhookController{
+				client:       http.DefaultClient,
+				webhooks:     []*Webhook{{Name: "people", Kind: "ENRICHING"}},
+				TemplateData: x509util.TemplateData{},
+			},
+			ctx: withRequestID(t, context.Background(), "reqID"),
+			req: &webhook.RequestBody{},
+			responses: []*webhook.ResponseBody{{Allow: false, Error: &webhook.Error{
+				Code: "theCode", Message: "Some message",
+			}}},
+			expectErr:          true,
+			expectTemplateData: x509util.TemplateData{},
+			assertError: func(t *testing.T, err error) {
+				assert.Equal(t, &webhook.Error{
+					Code: "theCode", Message: "Some message",
+				}, err)
+			},
 		},
 		"fail/with options": {
 			ctl: &WebhookController{
@@ -268,6 +291,9 @@ func TestWebhookController_Enrich(t *testing.T) {
 			if test.assertRequest != nil {
 				test.assertRequest(t, test.req)
 			}
+			if test.assertError != nil {
+				test.assertError(t, err)
+			}
 		})
 	}
 }
@@ -283,6 +309,7 @@ func TestWebhookController_Authorize(t *testing.T) {
 		responses     []*webhook.ResponseBody
 		expectErr     bool
 		assertRequest func(t *testing.T, req *webhook.RequestBody)
+		assertError   func(t *testing.T, err error)
 	}
 	tests := map[string]test{
 		"ok/no enriching webhooks": {
@@ -346,6 +373,26 @@ func TestWebhookController_Authorize(t *testing.T) {
 			req:       &webhook.RequestBody{},
 			responses: []*webhook.ResponseBody{{Allow: false}},
 			expectErr: true,
+			assertError: func(t *testing.T, err error) {
+				assert.Equal(t, ErrWebhookDenied, err)
+			},
+		},
+		"deny/withError": {
+			ctl: &WebhookController{
+				client:   http.DefaultClient,
+				webhooks: []*Webhook{{Name: "people", Kind: "AUTHORIZING"}},
+			},
+			ctx: withRequestID(t, context.Background(), "reqID"),
+			req: &webhook.RequestBody{},
+			responses: []*webhook.ResponseBody{{Allow: false, Error: &webhook.Error{
+				Code: "theCode", Message: "Some message",
+			}}},
+			expectErr: true,
+			assertError: func(t *testing.T, err error) {
+				assert.Equal(t, &webhook.Error{
+					Code: "theCode", Message: "Some message",
+				}, err)
+			},
 		},
 		"fail/with options": {
 			ctl: &WebhookController{
@@ -383,6 +430,9 @@ func TestWebhookController_Authorize(t *testing.T) {
 			if test.assertRequest != nil {
 				test.assertRequest(t, test.req)
 			}
+			if test.assertError != nil {
+				test.assertError(t, err)
+			}
 		})
 	}
 }
diff --git a/errs/error.go b/errs/error.go
@@ -62,6 +62,11 @@ type ErrorResponse struct {
 	Message string `json:"message"`
 }
 
+// Unwrap implements the Unwrap interface and returns the original error.
+func (e *Error) Unwrap() error {
+	return e.Err
+}
+
 // Cause implements the errors.Causer interface and returns the original error.
 func (e *Error) Cause() error {
 	return e.Err
diff --git a/errs/errors_test.go b/errs/errors_test.go
@@ -1,7 +1,9 @@
 package errs
 
 import (
+	"errors"
 	"fmt"
+	"net/http"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -67,3 +69,61 @@ func TestError_UnmarshalJSON(t *testing.T) {
 		})
 	}
 }
+
+func TestError_Unwrap(t *testing.T) {
+	err := errors.New("wrapped error")
+	tests := []struct {
+		name  string
+		error error
+		want  string
+	}{
+		{"ok New", New(http.StatusBadRequest, "some error"), "some error"},
+		{"ok New v-wrap", New(http.StatusBadRequest, "some error: %v", err), "some error: wrapped error"},
+		{"ok NewError", NewError(http.StatusBadRequest, err, "some error"), "some error: wrapped error"},
+		{"ok NewErr", NewErr(http.StatusBadRequest, err), "wrapped error"},
+		{"ok NewErr wit message", NewErr(http.StatusBadRequest, err, WithMessage("some message")), "wrapped error"},
+		{"ok Errorf", Errorf(http.StatusBadRequest, "some error: %w", err), "some error: wrapped error"},
+		{"ok Errorf v-wrap", Errorf(http.StatusBadRequest, "some error: %v", err), "some error: wrapped error"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := errors.Unwrap(tt.error)
+			assert.EqualError(t, got, tt.want)
+		})
+	}
+}
+
+type customError struct {
+	Message string
+}
+
+func (e *customError) Error() string {
+	return e.Message
+}
+
+func TestError_Unwrap_As(t *testing.T) {
+	err := &customError{Message: "wrapped error"}
+
+	tests := []struct {
+		name    string
+		error   error
+		want    bool
+		wantErr *customError
+	}{
+		{"ok NewError", NewError(http.StatusBadRequest, err, "some error"), true, err},
+		{"ok NewErr", NewErr(http.StatusBadRequest, err), true, err},
+		{"ok NewErr wit message", NewErr(http.StatusBadRequest, err, WithMessage("some message")), true, err},
+		{"ok Errorf", Errorf(http.StatusBadRequest, "some error: %w", err), true, err},
+		{"fail New", New(http.StatusBadRequest, "some error"), false, nil},
+		{"fail New v-wrap", New(http.StatusBadRequest, "some error: %v", err), false, nil},
+		{"fail Errorf", Errorf(http.StatusBadRequest, "some error"), false, nil},
+		{"fail Errorf v-wrap", Errorf(http.StatusBadRequest, "some error: %v", err), false, nil},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var cerr *customError
+			assert.Equal(t, tt.want, errors.As(tt.error, &cerr))
+			assert.Equal(t, tt.wantErr, cerr)
+		})
+	}
+}
diff --git a/webhook/types.go b/webhook/types.go
@@ -1,6 +1,7 @@
 package webhook
 
 import (
+	"fmt"
 	"time"
 
 	"go.step.sm/crypto/sshutil"
@@ -9,8 +10,19 @@ import (
 
 // ResponseBody is the body returned by webhook servers.
 type ResponseBody struct {
-	Data  any  `json:"data"`
-	Allow bool `json:"allow"`
+	Data  any    `json:"data"`
+	Allow bool   `json:"allow"`
+	Error *Error `json:"error,omitempty"`
+}
+
+// Error provides details explaining why the webhook was not permitted.
+type Error struct {
+	Code    string `json:"code"`
+	Message string `json:"message"`
+}
+
+func (e *Error) Error() string {
+	return fmt.Sprintf("%s (%s)", e.Message, e.Code)
 }
 
 // X509CertificateRequest is the certificate request sent to webhook servers for

Original file line number	Diff line number	Diff line change
`@@ -65,6 +65,9 @@ func (wc WebhookController) Enrich(ctx context.Context, req webhook.RequestBod`
`65`	`65`	`return err`
`66`	`66`	`}`
`67`	`67`	`if !resp.Allow {`
	`68`	`+ if resp.Error != nil {`
	`69`	`+ return resp.Error`
	`70`	`+ }`
`68`	`71`	`return ErrWebhookDenied`
`69`	`72`	`}`
`70`	`73`	`wc.TemplateData.SetWebhook(wh.Name, resp.Data)`
`@@ -101,6 +104,9 @@ func (wc WebhookController) Authorize(ctx context.Context, req webhook.Request`
`101`	`104`	`return err`
`102`	`105`	`}`
`103`	`106`	`if !resp.Allow {`
	`107`	`+ if resp.Error != nil {`
	`108`	`+ return resp.Error`
	`109`	`+ }`
`104`	`110`	`return ErrWebhookDenied`
`105`	`111`	`}`
`106`	`112`	`}`