From 02cd3b6b3bf763662b24bef35279b07dd000b107 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Thu, 16 Sep 2021 23:09:24 +0200 Subject: [PATCH] Fix PR comments --- acme/account.go | 14 +++--- acme/api/account.go | 2 +- acme/api/account_test.go | 52 ++++++++++---------- acme/db/nosql/account.go | 95 ++++++++++++++++++------------------ authority/admin/api/acme.go | 96 +++++++++++++++++++++++++++++-------- 5 files changed, 160 insertions(+), 99 deletions(-) diff --git a/acme/account.go b/acme/account.go index e9d3ac7f..deaf57c8 100644 --- a/acme/account.go +++ b/acme/account.go @@ -44,13 +44,13 @@ func KeyToID(jwk *jose.JSONWebKey) (string, error) { } type ExternalAccountKey struct { - ID string `json:"id"` - ProvisionerName string `json:"provisionerName"` - Name string `json:"name"` - AccountID string `json:"-"` - KeyBytes []byte `json:"-"` - CreatedAt time.Time `json:"createdAt"` - BoundAt time.Time `json:"boundAt,omitempty"` + ID string `json:"id"` + Provisioner string `json:"provisioner"` + Reference string `json:"reference"` + AccountID string `json:"-"` + KeyBytes []byte `json:"-"` + CreatedAt time.Time `json:"createdAt"` + BoundAt time.Time `json:"boundAt,omitempty"` } func (eak *ExternalAccountKey) AlreadyBound() bool { diff --git a/acme/api/account.go b/acme/api/account.go index fa5597e0..8d814d1c 100644 --- a/acme/api/account.go +++ b/acme/api/account.go @@ -257,7 +257,7 @@ func (h *Handler) validateExternalAccountBinding(ctx context.Context, nar *NewAc // about the handler and thus about its dependencies. eabJSONBytes, err := json.Marshal(nar.ExternalAccountBinding) if err != nil { - return nil, acme.WrapErrorISE(err, "error marshaling externalAccountBinding into JSON") + return nil, acme.WrapErrorISE(err, "error marshaling externalAccountBinding into bytes") } eabJWS, err := squarejose.ParseSigned(string(eabJSONBytes)) diff --git a/acme/api/account_test.go b/acme/api/account_test.go index f6a368d8..bced48b2 100644 --- a/acme/api/account_test.go +++ b/acme/api/account_test.go @@ -686,11 +686,11 @@ func TestHandler_NewAccount(t *testing.T) { }, MockGetExternalAccountKey: func(ctx context.Context, provisionerName string, keyID string) (*acme.ExternalAccountKey, error) { return &acme.ExternalAccountKey{ - ID: "eakID", - ProvisionerName: escProvName, - Name: "testeak", - KeyBytes: []byte{1, 3, 3, 7}, - CreatedAt: time.Now(), + ID: "eakID", + Provisioner: escProvName, + Reference: "testeak", + KeyBytes: []byte{1, 3, 3, 7}, + CreatedAt: time.Now(), }, nil }, MockUpdateExternalAccountKey: func(ctx context.Context, provisionerName string, eak *acme.ExternalAccountKey) error { @@ -1059,11 +1059,11 @@ func TestHandler_validateExternalAccountBinding(t *testing.T) { db: &acme.MockDB{ MockGetExternalAccountKey: func(ctx context.Context, provisionerName string, keyID string) (*acme.ExternalAccountKey, error) { return &acme.ExternalAccountKey{ - ID: "eakID", - ProvisionerName: escProvName, - Name: "testeak", - KeyBytes: []byte{1, 3, 3, 7}, - CreatedAt: time.Now(), + ID: "eakID", + Provisioner: escProvName, + Reference: "testeak", + KeyBytes: []byte{1, 3, 3, 7}, + CreatedAt: time.Now(), }, nil }, }, @@ -1200,12 +1200,12 @@ func TestHandler_validateExternalAccountBinding(t *testing.T) { db: &acme.MockDB{ MockGetExternalAccountKey: func(ctx context.Context, provisionerName string, keyID string) (*acme.ExternalAccountKey, error) { return &acme.ExternalAccountKey{ - ID: "eakID", - ProvisionerName: escProvName, - Name: "testeak", - CreatedAt: createdAt, - AccountID: "some-account-id", - BoundAt: boundAt, + ID: "eakID", + Provisioner: escProvName, + Reference: "testeak", + CreatedAt: createdAt, + AccountID: "some-account-id", + BoundAt: boundAt, }, nil }, }, @@ -1235,11 +1235,11 @@ func TestHandler_validateExternalAccountBinding(t *testing.T) { db: &acme.MockDB{ MockGetExternalAccountKey: func(ctx context.Context, provisionerName string, keyID string) (*acme.ExternalAccountKey, error) { return &acme.ExternalAccountKey{ - ID: "eakID", - ProvisionerName: escProvName, - Name: "testeak", - KeyBytes: []byte{1, 2, 3, 4}, - CreatedAt: time.Now(), + ID: "eakID", + Provisioner: escProvName, + Reference: "testeak", + KeyBytes: []byte{1, 2, 3, 4}, + CreatedAt: time.Now(), }, nil }, }, @@ -1271,11 +1271,11 @@ func TestHandler_validateExternalAccountBinding(t *testing.T) { db: &acme.MockDB{ MockGetExternalAccountKey: func(ctx context.Context, provisionerName string, keyID string) (*acme.ExternalAccountKey, error) { return &acme.ExternalAccountKey{ - ID: "eakID", - ProvisionerName: escProvName, - Name: "testeak", - KeyBytes: []byte{1, 3, 3, 7}, - CreatedAt: time.Now(), + ID: "eakID", + Provisioner: escProvName, + Reference: "testeak", + KeyBytes: []byte{1, 3, 3, 7}, + CreatedAt: time.Now(), }, nil }, }, diff --git a/acme/db/nosql/account.go b/acme/db/nosql/account.go index 8647c5e3..d2572fbf 100644 --- a/acme/db/nosql/account.go +++ b/acme/db/nosql/account.go @@ -28,13 +28,13 @@ func (dba *dbAccount) clone() *dbAccount { } type dbExternalAccountKey struct { - ID string `json:"id"` - ProvisionerName string `json:"provisioner_name"` - Name string `json:"name"` - AccountID string `json:"accountID,omitempty"` - KeyBytes []byte `json:"key"` - CreatedAt time.Time `json:"createdAt"` - BoundAt time.Time `json:"boundAt"` + ID string `json:"id"` + Provisioner string `json:"provisioner"` + Reference string `json:"reference"` + AccountID string `json:"accountID,omitempty"` + KeyBytes []byte `json:"key"` + CreatedAt time.Time `json:"createdAt"` + BoundAt time.Time `json:"boundAt"` } func (db *DB) getAccountIDByKeyID(ctx context.Context, kid string) (string, error) { @@ -165,7 +165,7 @@ func (db *DB) UpdateAccount(ctx context.Context, acc *acme.Account) error { } // CreateExternalAccountKey creates a new External Account Binding key with a name -func (db *DB) CreateExternalAccountKey(ctx context.Context, provisionerName string, name string) (*acme.ExternalAccountKey, error) { +func (db *DB) CreateExternalAccountKey(ctx context.Context, provisionerName string, reference string) (*acme.ExternalAccountKey, error) { keyID, err := randID() if err != nil { return nil, err @@ -178,24 +178,24 @@ func (db *DB) CreateExternalAccountKey(ctx context.Context, provisionerName stri } dbeak := &dbExternalAccountKey{ - ID: keyID, - ProvisionerName: provisionerName, - Name: name, - KeyBytes: random, - CreatedAt: clock.Now(), + ID: keyID, + Provisioner: provisionerName, + Reference: reference, + KeyBytes: random, + CreatedAt: clock.Now(), } if err = db.save(ctx, keyID, dbeak, nil, "external_account_key", externalAccountKeyTable); err != nil { return nil, err } return &acme.ExternalAccountKey{ - ID: dbeak.ID, - ProvisionerName: dbeak.ProvisionerName, - Name: dbeak.Name, - AccountID: dbeak.AccountID, - KeyBytes: dbeak.KeyBytes, - CreatedAt: dbeak.CreatedAt, - BoundAt: dbeak.BoundAt, + ID: dbeak.ID, + Provisioner: dbeak.Provisioner, + Reference: dbeak.Reference, + AccountID: dbeak.AccountID, + KeyBytes: dbeak.KeyBytes, + CreatedAt: dbeak.CreatedAt, + BoundAt: dbeak.BoundAt, }, nil } @@ -206,18 +206,18 @@ func (db *DB) GetExternalAccountKey(ctx context.Context, provisionerName string, return nil, err } - if dbeak.ProvisionerName != provisionerName { + if dbeak.Provisioner != provisionerName { return nil, acme.NewError(acme.ErrorUnauthorizedType, "name of provisioner does not match provisioner for which the EAB key was created") } return &acme.ExternalAccountKey{ - ID: dbeak.ID, - ProvisionerName: dbeak.ProvisionerName, - Name: dbeak.Name, - AccountID: dbeak.AccountID, - KeyBytes: dbeak.KeyBytes, - CreatedAt: dbeak.CreatedAt, - BoundAt: dbeak.BoundAt, + ID: dbeak.ID, + Provisioner: dbeak.Provisioner, + Reference: dbeak.Reference, + AccountID: dbeak.AccountID, + KeyBytes: dbeak.KeyBytes, + CreatedAt: dbeak.CreatedAt, + BoundAt: dbeak.BoundAt, }, nil } @@ -240,21 +240,24 @@ func (db *DB) GetExternalAccountKeys(ctx context.Context, provisionerName string return nil, err } - keys := make([]*acme.ExternalAccountKey, len(entries)) - for i, entry := range entries { + keys := []*acme.ExternalAccountKey{} + for _, entry := range entries { dbeak := new(dbExternalAccountKey) if err = json.Unmarshal(entry.Value, dbeak); err != nil { return nil, errors.Wrapf(err, "error unmarshaling external account key %s into dbExternalAccountKey", string(entry.Key)) } - keys[i] = &acme.ExternalAccountKey{ - ID: dbeak.ID, - KeyBytes: dbeak.KeyBytes, - ProvisionerName: dbeak.ProvisionerName, - Name: dbeak.Name, - AccountID: dbeak.AccountID, - CreatedAt: dbeak.CreatedAt, - BoundAt: dbeak.BoundAt, + if dbeak.Provisioner != provisionerName { + continue } + keys = append(keys, &acme.ExternalAccountKey{ + ID: dbeak.ID, + KeyBytes: dbeak.KeyBytes, + Provisioner: dbeak.Provisioner, + Reference: dbeak.Reference, + AccountID: dbeak.AccountID, + CreatedAt: dbeak.CreatedAt, + BoundAt: dbeak.BoundAt, + }) } return keys, nil @@ -266,18 +269,18 @@ func (db *DB) UpdateExternalAccountKey(ctx context.Context, provisionerName stri return err } - if old.ProvisionerName != provisionerName { + if old.Provisioner != provisionerName { return acme.NewError(acme.ErrorUnauthorizedType, "name of provisioner does not match provisioner for which the EAB key was created") } nu := dbExternalAccountKey{ - ID: eak.ID, - ProvisionerName: eak.ProvisionerName, - Name: eak.Name, - AccountID: eak.AccountID, - KeyBytes: eak.KeyBytes, - CreatedAt: eak.CreatedAt, - BoundAt: eak.BoundAt, + ID: eak.ID, + Provisioner: eak.Provisioner, + Reference: eak.Reference, + AccountID: eak.AccountID, + KeyBytes: eak.KeyBytes, + CreatedAt: eak.CreatedAt, + BoundAt: eak.BoundAt, } return db.save(ctx, nu.ID, nu, old, "external_account_key", externalAccountKeyTable) diff --git a/authority/admin/api/acme.go b/authority/admin/api/acme.go index e8b26174..6889764b 100644 --- a/authority/admin/api/acme.go +++ b/authority/admin/api/acme.go @@ -1,28 +1,30 @@ package api import ( + "context" "net/http" "github.com/go-chi/chi" "github.com/smallstep/certificates/api" "github.com/smallstep/certificates/authority/admin" + "github.com/smallstep/certificates/authority/provisioner" "go.step.sm/linkedca" "google.golang.org/protobuf/types/known/timestamppb" ) // CreateExternalAccountKeyRequest is the type for POST /admin/acme/eab requests type CreateExternalAccountKeyRequest struct { - ProvisionerName string `json:"provisioner"` - Name string `json:"name"` + Provisioner string `json:"provisioner"` + Reference string `json:"reference"` } -// Validate validates a new-admin request body. +// Validate validates a new ACME EAB Key request body. func (r *CreateExternalAccountKeyRequest) Validate() error { - if r.ProvisionerName == "" { + if r.Provisioner == "" { return admin.NewError(admin.ErrorBadRequestType, "provisioner name cannot be empty") } - if r.Name == "" { - return admin.NewError(admin.ErrorBadRequestType, "name / reference cannot be empty") + if r.Reference == "" { + return admin.NewError(admin.ErrorBadRequestType, "reference cannot be empty") } return nil } @@ -33,6 +35,38 @@ type GetExternalAccountKeysResponse struct { NextCursor string `json:"nextCursor"` } +// provisionerHasEABEnabled determines if the "requireEAB" setting for an ACME +// provisioner is set to true and thus has EAB enabled. +// TODO: rewrite this into a middleware for the ACME handlers? This probably requires +// ensuring that all the ACME EAB APIs that need the middleware work the same in terms +// of specifying the provisioner; probably a bit of refactoring required. +func (h *Handler) provisionerHasEABEnabled(ctx context.Context, provisionerName string) (bool, error) { + var ( + p provisioner.Interface + err error + ) + if p, err = h.auth.LoadProvisionerByName(provisionerName); err != nil { + return false, admin.WrapErrorISE(err, "error loading provisioner %s", provisionerName) + } + + prov, err := h.db.GetProvisioner(ctx, p.GetID()) + if err != nil { + return false, admin.WrapErrorISE(err, "error getting provisioner with ID: %s", p.GetID()) + } + + details := prov.GetDetails() + if details == nil { + return false, admin.NewErrorISE("error getting details for provisioner with ID: %s", p.GetID()) + } + + acme := details.GetACME() + if acme == nil { + return false, admin.NewErrorISE("error getting ACME details for provisioner with ID: %s", p.GetID()) + } + + return acme.GetRequireEab(), nil +} + // CreateExternalAccountKey creates a new External Account Binding key func (h *Handler) CreateExternalAccountKey(w http.ResponseWriter, r *http.Request) { var body CreateExternalAccountKeyRequest @@ -46,17 +80,28 @@ func (h *Handler) CreateExternalAccountKey(w http.ResponseWriter, r *http.Reques return } - eak, err := h.acmeDB.CreateExternalAccountKey(r.Context(), body.ProvisionerName, body.Name) + eabEnabled, err := h.provisionerHasEABEnabled(r.Context(), body.Provisioner) if err != nil { - api.WriteError(w, admin.WrapErrorISE(err, "error creating external account key %s", body.Name)) + api.WriteError(w, err) + return + } + + if !eabEnabled { + api.WriteError(w, admin.NewError(admin.ErrorBadRequestType, "ACME EAB not enabled for provisioner %s", body.Provisioner)) + return + } + + eak, err := h.acmeDB.CreateExternalAccountKey(r.Context(), body.Provisioner, body.Reference) + if err != nil { + api.WriteError(w, admin.WrapErrorISE(err, "error creating external account key %s for provisioner %s", body.Reference, body.Provisioner)) return } response := &linkedca.EABKey{ - EabKid: eak.ID, - EabHmacKey: eak.KeyBytes, - ProvisionerName: eak.ProvisionerName, - Name: eak.Name, + Id: eak.ID, + HmacKey: eak.KeyBytes, + Provisioner: eak.Provisioner, + Reference: eak.Reference, } api.ProtoJSONStatus(w, response, http.StatusCreated) @@ -66,6 +111,8 @@ func (h *Handler) CreateExternalAccountKey(w http.ResponseWriter, r *http.Reques func (h *Handler) DeleteExternalAccountKey(w http.ResponseWriter, r *http.Request) { id := chi.URLParam(r, "id") + // TODO: add provisioner as parameter, so that check can be performed if EAB is enabled or not + if err := h.acmeDB.DeleteExternalAccountKey(r.Context(), id); err != nil { api.WriteError(w, admin.WrapErrorISE(err, "error deleting ACME EAB Key %s", id)) return @@ -78,6 +125,17 @@ func (h *Handler) DeleteExternalAccountKey(w http.ResponseWriter, r *http.Reques func (h *Handler) GetExternalAccountKeys(w http.ResponseWriter, r *http.Request) { prov := chi.URLParam(r, "prov") + eabEnabled, err := h.provisionerHasEABEnabled(r.Context(), prov) + if err != nil { + api.WriteError(w, err) + return + } + + if !eabEnabled { + api.WriteError(w, admin.NewError(admin.ErrorBadRequestType, "ACME EAB not enabled for provisioner %s", prov)) + return + } + // TODO: support paging properly? It'll probably leak to the DB layer, as we have to loop through all keys // cursor, limit, err := api.ParseCursor(r) // if err != nil { @@ -95,13 +153,13 @@ func (h *Handler) GetExternalAccountKeys(w http.ResponseWriter, r *http.Request) eaks := make([]*linkedca.EABKey, len(keys)) for i, k := range keys { eaks[i] = &linkedca.EABKey{ - EabKid: k.ID, - EabHmacKey: []byte{}, - ProvisionerName: k.ProvisionerName, - Name: k.Name, - Account: k.AccountID, - CreatedAt: timestamppb.New(k.CreatedAt), - BoundAt: timestamppb.New(k.BoundAt), + Id: k.ID, + HmacKey: []byte{}, + Provisioner: k.Provisioner, + Reference: k.Reference, + Account: k.AccountID, + CreatedAt: timestamppb.New(k.CreatedAt), + BoundAt: timestamppb.New(k.BoundAt), } }