From 05295d9c6a8b0baaad0a351fb685594191c08db5 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 14 Nov 2024 18:29:36 -0800 Subject: [PATCH] Propagate human errors from webhooks This commit adds a new field error in the webhook response that allows to propagate errors to the client. With ACME, webhook errors are as a new subproblem. --- acme/order.go | 13 ++++++ acme/order_test.go | 52 +++++++++++++++++++++++ authority/provisioner/webhook.go | 6 +++ authority/provisioner/webhook_test.go | 50 ++++++++++++++++++++++ errs/error.go | 5 +++ errs/errors_test.go | 60 +++++++++++++++++++++++++++ webhook/types.go | 16 ++++++- 7 files changed, 200 insertions(+), 2 deletions(-) diff --git a/acme/order.go b/acme/order.go index e941d587a..fa0886daa 100644 --- 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 index be98bc40a..6182fb2bf 100644 --- 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 index 05f972fe6..282241809 100644 --- 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 index 75dd0793b..df5ea76ed 100644 --- 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 index d98f42e29..f01cb4d85 100644 --- 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 index 11590d7d6..5836c5926 100644 --- 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 index 5e0e4d291..c60de7099 100644 --- 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