From fb81407d6f294a229fd833c52fcbd91bf85a8590 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Thu, 21 Apr 2022 13:21:06 +0200 Subject: [PATCH] Fix ACME policy comments --- acme/api/account_test.go | 9 ++++++ acme/api/order.go | 16 ++++++++--- acme/api/order_test.go | 49 +++++++++++++++++++++++++++----- authority/admin/api/acme.go | 8 +----- authority/admin/api/acme_test.go | 16 ++++++----- 5 files changed, 73 insertions(+), 25 deletions(-) diff --git a/acme/api/account_test.go b/acme/api/account_test.go index a457655c..e389b57f 100644 --- a/acme/api/account_test.go +++ b/acme/api/account_test.go @@ -65,6 +65,15 @@ func newACMEProv(t *testing.T) *provisioner.ACME { return a } +func newACMEProvWithOptions(t *testing.T, options *provisioner.Options) *provisioner.ACME { + p := newProvWithOptions(options) + a, ok := p.(*provisioner.ACME) + if !ok { + t.Fatal("not a valid ACME provisioner") + } + return a +} + func createEABJWS(jwk *jose.JSONWebKey, hmacKey []byte, keyID, u string) (*jose.JSONWebSignature, error) { signer, err := jose.NewSigner( jose.SigningKey{ diff --git a/acme/api/order.go b/acme/api/order.go index 820b642f..5bf35a58 100644 --- a/acme/api/order.go +++ b/acme/api/order.go @@ -103,15 +103,23 @@ func (h *Handler) NewOrder(w http.ResponseWriter, r *http.Request) { return } - // TODO(hs): gather all errors, so that we can build one response with subproblems; include the nor.Validate() - // error here too, like in example? + // TODO(hs): gather all errors, so that we can build one response with ACME subproblems + // include the nor.Validate() error here too, like in the example in the ACME RFC? - eak, err := h.db.GetExternalAccountKeyByAccountID(ctx, prov.GetID(), acc.ID) + acmeProv, err := acmeProvisionerFromContext(ctx) if err != nil { - render.Error(w, acme.WrapErrorISE(err, "error retrieving external account binding key")) + render.Error(w, err) return } + var eak *acme.ExternalAccountKey + if acmeProv.RequireEAB { + if eak, err = h.db.GetExternalAccountKeyByAccountID(ctx, prov.GetID(), acc.ID); err != nil { + render.Error(w, acme.WrapErrorISE(err, "error retrieving external account binding key")) + return + } + } + acmePolicy, err := newACMEPolicyEngine(eak) if err != nil { render.Error(w, acme.WrapErrorISE(err, "error creating ACME policy engine")) diff --git a/acme/api/order_test.go b/acme/api/order_test.go index 02034c16..35abab65 100644 --- a/acme/api/order_test.go +++ b/acme/api/order_test.go @@ -761,7 +761,7 @@ func TestHandler_NewOrder(t *testing.T) { err: acme.NewError(acme.ErrorMalformedType, "identifiers list cannot be empty"), } }, - "fail/db.GetExternalAccountKeyByAccountID-error": func(t *testing.T) test { + "fail/acmeProvisionerFromContext-error": func(t *testing.T) test { acc := &acme.Account{ID: "accID"} fr := &NewOrderRequest{ Identifiers: []acme.Identifier{ @@ -770,7 +770,35 @@ func TestHandler_NewOrder(t *testing.T) { } b, err := json.Marshal(fr) assert.FatalError(t, err) - ctx := context.WithValue(context.Background(), provisionerContextKey, prov) + ctx := context.WithValue(context.Background(), provisionerContextKey, &acme.MockProvisioner{}) + ctx = context.WithValue(ctx, accContextKey, acc) + ctx = context.WithValue(ctx, payloadContextKey, &payloadInfo{value: b}) + return test{ + ctx: ctx, + statusCode: 500, + ca: &mockCA{}, + db: &acme.MockDB{ + MockGetExternalAccountKeyByAccountID: func(ctx context.Context, provisionerID, accountID string) (*acme.ExternalAccountKey, error) { + assert.Equals(t, prov.GetID(), provisionerID) + assert.Equals(t, "accID", accountID) + return nil, errors.New("force") + }, + }, + err: acme.NewErrorISE("error retrieving external account binding key: force"), + } + }, + "fail/db.GetExternalAccountKeyByAccountID-error": func(t *testing.T) test { + acmeProv := newACMEProv(t) + acmeProv.RequireEAB = true + acc := &acme.Account{ID: "accID"} + fr := &NewOrderRequest{ + Identifiers: []acme.Identifier{ + {Type: "dns", Value: "zap.internal"}, + }, + } + b, err := json.Marshal(fr) + assert.FatalError(t, err) + ctx := context.WithValue(context.Background(), provisionerContextKey, acmeProv) ctx = context.WithValue(ctx, accContextKey, acc) ctx = context.WithValue(ctx, payloadContextKey, &payloadInfo{value: b}) return test{ @@ -788,6 +816,8 @@ func TestHandler_NewOrder(t *testing.T) { } }, "fail/newACMEPolicyEngine-error": func(t *testing.T) test { + acmeProv := newACMEProv(t) + acmeProv.RequireEAB = true acc := &acme.Account{ID: "accID"} fr := &NewOrderRequest{ Identifiers: []acme.Identifier{ @@ -796,7 +826,7 @@ func TestHandler_NewOrder(t *testing.T) { } b, err := json.Marshal(fr) assert.FatalError(t, err) - ctx := context.WithValue(context.Background(), provisionerContextKey, prov) + ctx := context.WithValue(context.Background(), provisionerContextKey, acmeProv) ctx = context.WithValue(ctx, accContextKey, acc) ctx = context.WithValue(ctx, payloadContextKey, &payloadInfo{value: b}) return test{ @@ -822,6 +852,8 @@ func TestHandler_NewOrder(t *testing.T) { } }, "fail/isIdentifierAllowed-error": func(t *testing.T) test { + acmeProv := newACMEProv(t) + acmeProv.RequireEAB = true acc := &acme.Account{ID: "accID"} fr := &NewOrderRequest{ Identifiers: []acme.Identifier{ @@ -830,7 +862,7 @@ func TestHandler_NewOrder(t *testing.T) { } b, err := json.Marshal(fr) assert.FatalError(t, err) - ctx := context.WithValue(context.Background(), provisionerContextKey, prov) + ctx := context.WithValue(context.Background(), provisionerContextKey, acmeProv) ctx = context.WithValue(ctx, accContextKey, acc) ctx = context.WithValue(ctx, payloadContextKey, &payloadInfo{value: b}) return test{ @@ -863,7 +895,8 @@ func TestHandler_NewOrder(t *testing.T) { }, }, } - provWithPolicy := newProvWithOptions(options) + provWithPolicy := newACMEProvWithOptions(t, options) + provWithPolicy.RequireEAB = true acc := &acme.Account{ID: "accID"} fr := &NewOrderRequest{ Identifiers: []acme.Identifier{ @@ -905,7 +938,8 @@ func TestHandler_NewOrder(t *testing.T) { }, }, } - provWithPolicy := newProvWithOptions(options) + provWithPolicy := newACMEProvWithOptions(t, options) + provWithPolicy.RequireEAB = true acc := &acme.Account{ID: "accID"} fr := &NewOrderRequest{ Identifiers: []acme.Identifier{ @@ -1567,7 +1601,8 @@ func TestHandler_NewOrder(t *testing.T) { }, }, } - provWithPolicy := newProvWithOptions(options) + provWithPolicy := newACMEProvWithOptions(t, options) + provWithPolicy.RequireEAB = true acc := &acme.Account{ID: "accID"} nor := &NewOrderRequest{ Identifiers: []acme.Identifier{ diff --git a/authority/admin/api/acme.go b/authority/admin/api/acme.go index e11ac317..fe667181 100644 --- a/authority/admin/api/acme.go +++ b/authority/admin/api/acme.go @@ -38,13 +38,7 @@ func (h *Handler) requireEABEnabled(next http.HandlerFunc) http.HandlerFunc { ctx := r.Context() prov := linkedca.ProvisionerFromContext(ctx) - details := prov.GetDetails() - if details == nil { - render.Error(w, admin.NewErrorISE("error getting details for provisioner '%s'", prov.GetName())) - return - } - - acmeProvisioner := details.GetACME() + acmeProvisioner := prov.GetDetails().GetACME() if acmeProvisioner == nil { render.Error(w, admin.NewErrorISE("error getting ACME details for provisioner '%s'", prov.GetName())) return diff --git a/authority/admin/api/acme_test.go b/authority/admin/api/acme_test.go index e44b4e9b..aa4aa608 100644 --- a/authority/admin/api/acme_test.go +++ b/authority/admin/api/acme_test.go @@ -13,13 +13,15 @@ import ( "time" "github.com/go-chi/chi" - "github.com/smallstep/assert" - "github.com/smallstep/certificates/acme" - "github.com/smallstep/certificates/authority/admin" - "go.step.sm/linkedca" "google.golang.org/protobuf/encoding/protojson" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/types/known/timestamppb" + + "go.step.sm/linkedca" + + "github.com/smallstep/assert" + "github.com/smallstep/certificates/acme" + "github.com/smallstep/certificates/authority/admin" ) func readProtoJSON(r io.ReadCloser, m proto.Message) error { @@ -45,15 +47,15 @@ func TestHandler_requireEABEnabled(t *testing.T) { Name: "provName", } ctx := linkedca.NewContextWithProvisioner(context.Background(), prov) - err := admin.NewErrorISE("error getting details for provisioner 'provName'") - err.Message = "error getting details for provisioner 'provName'" + err := admin.NewErrorISE("error getting ACME details for provisioner 'provName'") + err.Message = "error getting ACME details for provisioner 'provName'" return test{ ctx: ctx, err: err, statusCode: 500, } }, - "fail/details.GetACME": func(t *testing.T) test { + "fail/prov.GetDetails.GetACME": func(t *testing.T) test { prov := &linkedca.Provisioner{ Id: "provID", Name: "provName",