Fix PR comments

acme/account.go

@@ -52,8 +52,12 @@ type ExternalAccountKey struct {
 	BoundAt   time.Time `json:"boundAt,omitempty"`
 }
 
+func (eak *ExternalAccountKey) AlreadyBound() bool {
+	return !eak.BoundAt.IsZero()
+}
+
 func (eak *ExternalAccountKey) BindTo(account *Account) {
 	eak.AccountID = account.ID
 	eak.BoundAt = time.Now()
-	eak.KeyBytes = []byte{} // TODO: ensure that single use keys are OK
+	eak.KeyBytes = []byte{} // clearing the key bytes; can only be used once
 }

acme/api/account.go

@@ -8,7 +8,6 @@ import (
 	"github.com/go-chi/chi"
 	"github.com/smallstep/certificates/acme"
 	"github.com/smallstep/certificates/api"
-	"github.com/smallstep/certificates/authority/provisioner"
 	"github.com/smallstep/certificates/logging"
 
 	squarejose "gopkg.in/square/go-jose.v2"
@@ -125,7 +124,7 @@ func (h *Handler) NewAccount(w http.ResponseWriter, r *http.Request) {
 			api.WriteError(w, acme.WrapErrorISE(err, "error creating account"))
 			return
 		}
-		if eak != nil { // means that we have a (valid) External Account Binding key that should be used
+		if eak != nil { // means that we have a (valid) External Account Binding key that should be bound, updated and sent in the response
 			eak.BindTo(acc)
 			if err := h.db.UpdateExternalAccountKey(ctx, eak); err != nil {
 				api.WriteError(w, acme.WrapErrorISE(err, "error updating external account binding key"))
@@ -134,7 +133,7 @@ func (h *Handler) NewAccount(w http.ResponseWriter, r *http.Request) {
 			acc.ExternalAccountBinding = nar.ExternalAccountBinding
 		}
 	} else {
-		// Account exists //
+		// Account exists
 		httpStatus = http.StatusOK
 	}
 
@@ -227,19 +226,12 @@ func (h *Handler) GetOrdersByAccountID(w http.ResponseWriter, r *http.Request) {
 
 // validateExternalAccountBinding validates the externalAccountBinding property in a call to new-account
 func (h *Handler) validateExternalAccountBinding(ctx context.Context, nar *NewAccountRequest) (*acme.ExternalAccountKey, error) {
-
-	prov, err := provisionerFromContext(ctx)
+	acmeProv, err := acmeProvisionerFromContext(ctx)
 	if err != nil {
-		return nil, err
-	}
-
-	acmeProv, ok := prov.(*provisioner.ACME) // TODO: rewrite into providing configuration via function on acme.Provisioner
-	if !ok || acmeProv == nil {
-		return nil, acme.NewErrorISE("provisioner in context is not an ACME provisioner")
+		return nil, acme.WrapErrorISE(err, "could not load ACME provisioner from context")
 	}
 
-	shouldSkipAccountBindingValidation := !acmeProv.RequireEAB
-	if shouldSkipAccountBindingValidation {
+	if !acmeProv.RequireEAB {
 		return nil, nil
 	}
 
@@ -249,7 +241,7 @@ func (h *Handler) validateExternalAccountBinding(ctx context.Context, nar *NewAc
 
 	eabJSONBytes, err := json.Marshal(nar.ExternalAccountBinding)
 	if err != nil {
-		return nil, acme.WrapErrorISE(err, "error marshaling externalAccountBinding")
+		return nil, acme.WrapErrorISE(err, "error marshaling externalAccountBinding into JSON")
 	}
 
 	eabJWS, err := squarejose.ParseSigned(string(eabJSONBytes))
@@ -266,8 +258,8 @@ func (h *Handler) validateExternalAccountBinding(ctx context.Context, nar *NewAc
 		return nil, acme.WrapErrorISE(err, "error retrieving external account key")
 	}
 
-	if !externalAccountKey.BoundAt.IsZero() { // TODO: ensure that single use keys are OK
-		return nil, acme.NewError(acme.ErrorUnauthorizedType, "external account binding key with id '%s' was already used", keyID)
+	if externalAccountKey.AlreadyBound() {
+		return nil, acme.NewError(acme.ErrorUnauthorizedType, "external account binding key with id '%s' was already bound to account '%s' on %s", keyID, externalAccountKey.AccountID, externalAccountKey.BoundAt)
 	}
 
 	payload, err := eabJWS.Verify(externalAccountKey.KeyBytes)

acme/api/handler.go

@@ -153,18 +153,12 @@ func (d *Directory) ToLog() (interface{}, error) {
 // for client configuration.
 func (h *Handler) GetDirectory(w http.ResponseWriter, r *http.Request) {
 	ctx := r.Context()
-	prov, err := provisionerFromContext(ctx)
+	acmeProv, err := acmeProvisionerFromContext(ctx)
 	if err != nil {
 		api.WriteError(w, err)
 		return
 	}
 
-	acmeProv, ok := prov.(*provisioner.ACME) // TODO: rewrite into providing configuration via function on acme.Provisioner
-	if !ok || acmeProv == nil {
-		api.WriteError(w, acme.NewErrorISE("provisioner in context is not an ACME provisioner"))
-		return
-	}
-
 	api.JSON(w, &Directory{
 		NewNonce:   h.linker.GetLink(ctx, NewNonceLinkType),
 		NewAccount: h.linker.GetLink(ctx, NewAccountLinkType),

acme/api/middleware.go

@@ -471,6 +471,20 @@ func provisionerFromContext(ctx context.Context) (acme.Provisioner, error) {
 	return pval, nil
 }
 
+// acmeProvisionerFromContext searches the context for an ACME provisioner. Returns
+// pointer to an ACME provisioner or an error.
+func acmeProvisionerFromContext(ctx context.Context) (*provisioner.ACME, error) {
+	prov, err := provisionerFromContext(ctx)
+	if err != nil {
+		return nil, err
+	}
+	acmeProv, ok := prov.(*provisioner.ACME)
+	if !ok || acmeProv == nil {
+		return nil, acme.NewErrorISE("provisioner in context is not an ACME provisioner")
+	}
+	return acmeProv, nil
+}
+
 // payloadFromContext searches the context for a payload. Returns the payload
 // or an error.
 func payloadFromContext(ctx context.Context) (*payloadInfo, error) {

authority/admin/api/eak.go

@@ -29,7 +29,7 @@ func (h *Handler) CreateExternalAccountKey(w http.ResponseWriter, r *http.Reques
 
 	// TODO: Validate input
 
-	eak, err := h.db.CreateExternalAccountKey(r.Context(), body.Name)
+	eak, err := h.acmeDB.CreateExternalAccountKey(r.Context(), body.Name)
 	if err != nil {
 		api.WriteError(w, admin.WrapErrorISE(err, "error creating external account key %s", body.Name))
 		return

authority/admin/api/handler.go

@@ -1,6 +1,7 @@
 package api
 
 import (
+	"github.com/smallstep/certificates/acme"
 	"github.com/smallstep/certificates/api"
 	"github.com/smallstep/certificates/authority"
 	"github.com/smallstep/certificates/authority/admin"
@@ -8,15 +9,18 @@ import (
 
 // Handler is the ACME API request handler.
 type Handler struct {
-	db   admin.DB
-	auth *authority.Authority
+	db     admin.DB
+	auth   *authority.Authority
+	acmeDB acme.DB
 }
 
 // NewHandler returns a new Authority Config Handler.
-func NewHandler(auth *authority.Authority) api.RouterHandler {
-	h := &Handler{db: auth.GetAdminDatabase(), auth: auth}
-
-	return h
+func NewHandler(auth *authority.Authority, adminDB admin.DB, acmeDB acme.DB) api.RouterHandler {
+	return &Handler{
+		db:     adminDB,
+		auth:   auth,
+		acmeDB: acmeDB,
+	}
 }
 
 // Route traffic and implement the Router interface.

authority/admin/db.go

@@ -7,8 +7,6 @@ import (
 
 	"github.com/pkg/errors"
 	"go.step.sm/linkedca"
-
-	"github.com/smallstep/certificates/authority/admin/eak"
 )
 
 const (
@@ -56,7 +54,7 @@ func UnmarshalProvisionerDetails(typ linkedca.Provisioner_Type, data []byte) (*l
 	return &linkedca.ProvisionerDetails{Data: v.Data}, nil
 }
 
-// DB is the DB interface expected by the step-ca ACME API.
+// DB is the DB interface expected by the step-ca Admin API.
 type DB interface {
 	CreateProvisioner(ctx context.Context, prov *linkedca.Provisioner) error
 	GetProvisioner(ctx context.Context, id string) (*linkedca.Provisioner, error)
@@ -69,8 +67,6 @@ type DB interface {
 	GetAdmins(ctx context.Context) ([]*linkedca.Admin, error)
 	UpdateAdmin(ctx context.Context, admin *linkedca.Admin) error
 	DeleteAdmin(ctx context.Context, id string) error
-
-	CreateExternalAccountKey(ctx context.Context, name string) (*eak.ExternalAccountKey, error)
 }
 
 // MockDB is an implementation of the DB interface that should only be used as
@@ -88,8 +84,6 @@ type MockDB struct {
 	MockUpdateAdmin func(ctx context.Context, adm *linkedca.Admin) error
 	MockDeleteAdmin func(ctx context.Context, id string) error
 
-	MockCreateExternalAccountKey func(ctx context.Context, name string) (*eak.ExternalAccountKey, error)
-
 	MockError error
 	MockRet1  interface{}
 }
@@ -183,10 +177,3 @@ func (m *MockDB) DeleteAdmin(ctx context.Context, id string) error {
 	}
 	return m.MockError
 }
-
-func (m *MockDB) CreateExternalAccountKey(ctx context.Context, name string) (*eak.ExternalAccountKey, error) {
-	if m.MockCreateExternalAccountKey != nil {
-		return m.MockCreateExternalAccountKey(ctx, name)
-	}
-	return m.MockRet1.(*eak.ExternalAccountKey), m.MockError
-}

authority/admin/db/nosql/admin_test.go

@@ -11,7 +11,6 @@ import (
 	"github.com/smallstep/certificates/authority/admin"
 	"github.com/smallstep/certificates/db"
 	"github.com/smallstep/nosql"
-	"github.com/smallstep/nosql/database"
 	nosqldb "github.com/smallstep/nosql/database"
 	"go.step.sm/linkedca"
 	"google.golang.org/protobuf/types/known/timestamppb"
@@ -996,7 +995,7 @@ func TestDB_GetAdmins(t *testing.T) {
 		"fail/db.List-error": func(t *testing.T) test {
 			return test{
 				db: &db.MockNoSQLDB{
-					MList: func(bucket []byte) ([]*database.Entry, error) {
+					MList: func(bucket []byte) ([]*nosqldb.Entry, error) {
 						assert.Equals(t, bucket, adminsTable)
 
 						return nil, errors.New("force")
@@ -1006,14 +1005,14 @@ func TestDB_GetAdmins(t *testing.T) {
 			}
 		},
 		"fail/unmarshal-error": func(t *testing.T) test {
-			ret := []*database.Entry{
+			ret := []*nosqldb.Entry{
 				{Bucket: adminsTable, Key: []byte("foo"), Value: foob},
 				{Bucket: adminsTable, Key: []byte("bar"), Value: barb},
 				{Bucket: adminsTable, Key: []byte("zap"), Value: []byte("zap")},
 			}
 			return test{
 				db: &db.MockNoSQLDB{
-					MList: func(bucket []byte) ([]*database.Entry, error) {
+					MList: func(bucket []byte) ([]*nosqldb.Entry, error) {
 						assert.Equals(t, bucket, adminsTable)
 
 						return ret, nil
@@ -1023,10 +1022,10 @@ func TestDB_GetAdmins(t *testing.T) {
 			}
 		},
 		"ok/none": func(t *testing.T) test {
-			ret := []*database.Entry{}
+			ret := []*nosqldb.Entry{}
 			return test{
 				db: &db.MockNoSQLDB{
-					MList: func(bucket []byte) ([]*database.Entry, error) {
+					MList: func(bucket []byte) ([]*nosqldb.Entry, error) {
 						assert.Equals(t, bucket, adminsTable)
 
 						return ret, nil
@@ -1038,13 +1037,13 @@ func TestDB_GetAdmins(t *testing.T) {
 			}
 		},
 		"ok/only-invalid": func(t *testing.T) test {
-			ret := []*database.Entry{
+			ret := []*nosqldb.Entry{
 				{Bucket: adminsTable, Key: []byte("bar"), Value: barb},
 				{Bucket: adminsTable, Key: []byte("baz"), Value: bazb},
 			}
 			return test{
 				db: &db.MockNoSQLDB{
-					MList: func(bucket []byte) ([]*database.Entry, error) {
+					MList: func(bucket []byte) ([]*nosqldb.Entry, error) {
 						assert.Equals(t, bucket, adminsTable)
 
 						return ret, nil
@@ -1056,7 +1055,7 @@ func TestDB_GetAdmins(t *testing.T) {
 			}
 		},
 		"ok": func(t *testing.T) test {
-			ret := []*database.Entry{
+			ret := []*nosqldb.Entry{
 				{Bucket: adminsTable, Key: []byte("foo"), Value: foob},
 				{Bucket: adminsTable, Key: []byte("bar"), Value: barb},
 				{Bucket: adminsTable, Key: []byte("baz"), Value: bazb},
@@ -1064,7 +1063,7 @@ func TestDB_GetAdmins(t *testing.T) {
 			}
 			return test{
 				db: &db.MockNoSQLDB{
-					MList: func(bucket []byte) ([]*database.Entry, error) {
+					MList: func(bucket []byte) ([]*nosqldb.Entry, error) {
 						assert.Equals(t, bucket, adminsTable)
 
 						return ret, nil

authority/admin/db/nosql/eak.go

@@ -1,51 +0,0 @@
-package nosql
-
-import (
-	"context"
-	"crypto/rand"
-	"time"
-
-	"github.com/smallstep/certificates/authority/admin/eak"
-)
-
-type dbExternalAccountKey struct {
-	ID        string    `json:"id"`
-	Name      string    `json:"name"`
-	AccountID string    `json:"accountID,omitempty"`
-	KeyBytes  []byte    `json:"key,omitempty"`
-	CreatedAt time.Time `json:"createdAt"`
-	BoundAt   time.Time `json:"boundAt"`
-}
-
-// CreateExternalAccountKey creates a new External Account Binding key
-func (db *DB) CreateExternalAccountKey(ctx context.Context, name string) (*eak.ExternalAccountKey, error) {
-	keyID, err := randID()
-	if err != nil {
-		return nil, err
-	}
-
-	random := make([]byte, 32)
-	_, err = rand.Read(random)
-	if err != nil {
-		return nil, err
-	}
-
-	dbeak := &dbExternalAccountKey{
-		ID:        keyID,
-		Name:      name,
-		KeyBytes:  random,
-		CreatedAt: clock.Now(),
-	}
-
-	if err = db.save(ctx, keyID, dbeak, nil, "external_account_key", externalAccountKeyTable); err != nil {
-		return nil, err
-	}
-	return &eak.ExternalAccountKey{
-		ID:        dbeak.ID,
-		Name:      dbeak.Name,
-		AccountID: dbeak.AccountID,
-		KeyBytes:  dbeak.KeyBytes,
-		CreatedAt: dbeak.CreatedAt,
-		BoundAt:   dbeak.BoundAt,
-	}, nil
-}

authority/admin/db/nosql/nosql.go

@@ -11,9 +11,8 @@ import (
 )
 
 var (
-	adminsTable             = []byte("admins")
-	provisionersTable       = []byte("provisioners")
-	externalAccountKeyTable = []byte("acme_external_account_keys")
+	adminsTable       = []byte("admins")
+	provisionersTable = []byte("provisioners")
 )
 
 // DB is a struct that implements the AdminDB interface.
@@ -24,7 +23,7 @@ type DB struct {
 
 // New configures and returns a new Authority DB backend implemented using a nosql DB.
 func New(db nosqlDB.DB, authorityID string) (*DB, error) {
-	tables := [][]byte{adminsTable, provisionersTable, externalAccountKeyTable}
+	tables := [][]byte{adminsTable, provisionersTable}
 	for _, b := range tables {
 		if err := db.CreateTable(b); err != nil {
 			return nil, errors.Wrapf(err, "error creating table %s",

authority/admin/db/nosql/provisioner_test.go

@@ -11,7 +11,6 @@ import (
 	"github.com/smallstep/certificates/authority/admin"
 	"github.com/smallstep/certificates/db"
 	"github.com/smallstep/nosql"
-	"github.com/smallstep/nosql/database"
 	nosqldb "github.com/smallstep/nosql/database"
 	"go.step.sm/linkedca"
 )
@@ -746,7 +745,7 @@ func TestDB_GetProvisioners(t *testing.T) {
 		"fail/db.List-error": func(t *testing.T) test {
 			return test{
 				db: &db.MockNoSQLDB{
-					MList: func(bucket []byte) ([]*database.Entry, error) {
+					MList: func(bucket []byte) ([]*nosqldb.Entry, error) {
 						assert.Equals(t, bucket, provisionersTable)
 
 						return nil, errors.New("force")
@@ -756,14 +755,14 @@ func TestDB_GetProvisioners(t *testing.T) {
 			}
 		},
 		"fail/unmarshal-error": func(t *testing.T) test {
-			ret := []*database.Entry{
+			ret := []*nosqldb.Entry{
 				{Bucket: provisionersTable, Key: []byte("foo"), Value: foob},
 				{Bucket: provisionersTable, Key: []byte("bar"), Value: barb},
 				{Bucket: provisionersTable, Key: []byte("zap"), Value: []byte("zap")},
 			}
 			return test{
 				db: &db.MockNoSQLDB{
-					MList: func(bucket []byte) ([]*database.Entry, error) {
+					MList: func(bucket []byte) ([]*nosqldb.Entry, error) {
 						assert.Equals(t, bucket, provisionersTable)
 
 						return ret, nil
@@ -773,10 +772,10 @@ func TestDB_GetProvisioners(t *testing.T) {
 			}
 		},
 		"ok/none": func(t *testing.T) test {
-			ret := []*database.Entry{}
+			ret := []*nosqldb.Entry{}
 			return test{
 				db: &db.MockNoSQLDB{
-					MList: func(bucket []byte) ([]*database.Entry, error) {
+					MList: func(bucket []byte) ([]*nosqldb.Entry, error) {
 						assert.Equals(t, bucket, provisionersTable)
 
 						return ret, nil
@@ -788,13 +787,13 @@ func TestDB_GetProvisioners(t *testing.T) {
 			}
 		},
 		"ok/only-invalid": func(t *testing.T) test {
-			ret := []*database.Entry{
+			ret := []*nosqldb.Entry{
 				{Bucket: provisionersTable, Key: []byte("bar"), Value: barb},
 				{Bucket: provisionersTable, Key: []byte("baz"), Value: bazb},
 			}
 			return test{
 				db: &db.MockNoSQLDB{
-					MList: func(bucket []byte) ([]*database.Entry, error) {
+					MList: func(bucket []byte) ([]*nosqldb.Entry, error) {
 						assert.Equals(t, bucket, provisionersTable)
 
 						return ret, nil
@@ -806,7 +805,7 @@ func TestDB_GetProvisioners(t *testing.T) {
 			}
 		},
 		"ok": func(t *testing.T) test {
-			ret := []*database.Entry{
+			ret := []*nosqldb.Entry{
 				{Bucket: provisionersTable, Key: []byte("foo"), Value: foob},
 				{Bucket: provisionersTable, Key: []byte("bar"), Value: barb},
 				{Bucket: provisionersTable, Key: []byte("baz"), Value: bazb},
@@ -814,7 +813,7 @@ func TestDB_GetProvisioners(t *testing.T) {
 			}
 			return test{
 				db: &db.MockNoSQLDB{
-					MList: func(bucket []byte) ([]*database.Entry, error) {
+					MList: func(bucket []byte) ([]*nosqldb.Entry, error) {
 						assert.Equals(t, bucket, provisionersTable)
 
 						return ret, nil

ca/ca.go

@@ -182,7 +182,7 @@ func (ca *CA) Init(config *config.Config) (*CA, error) {
 	if config.AuthorityConfig.EnableAdmin {
 		adminDB := auth.GetAdminDatabase()
 		if adminDB != nil {
-			adminHandler := adminAPI.NewHandler(auth)
+			adminHandler := adminAPI.NewHandler(auth, adminDB, acmeDB)
 			mux.Route("/admin", func(r chi.Router) {
 				adminHandler.Route(r)
 			})