From c3f2fd8ef0a2691fa706ddfcbc2e4440e0cbdc8a Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Thu, 20 Jan 2022 17:24:35 +0100 Subject: [PATCH] Add RW locks to prevent concurrent updates to the DB Although this may slow certain API calls down and may not be, strictly necessary, I think it's best to put all the ACME EAB operations behind RW locks to prevent concurrent updates to the DB and guarantee consistent result sets. --- acme/db/nosql/account.go | 37 --------------------- acme/db/nosql/eab.go | 69 ++++++++++++++++++++++++++++++++++++---- 2 files changed, 63 insertions(+), 43 deletions(-) diff --git a/acme/db/nosql/account.go b/acme/db/nosql/account.go index 603e3f2d..1c3bec5d 100644 --- a/acme/db/nosql/account.go +++ b/acme/db/nosql/account.go @@ -3,7 +3,6 @@ package nosql import ( "context" "encoding/json" - "sync" "time" "github.com/pkg/errors" @@ -12,9 +11,6 @@ import ( "go.step.sm/crypto/jose" ) -// Mutex for locking referencesByProvisioner index operations. -var referencesByProvisionerIndexMux sync.Mutex - // dbAccount represents an ACME account. type dbAccount struct { ID string `json:"id"` @@ -30,21 +26,6 @@ func (dba *dbAccount) clone() *dbAccount { return &nu } -type dbExternalAccountKey struct { - ID string `json:"id"` - ProvisionerID string `json:"provisionerID"` - Reference string `json:"reference"` - AccountID string `json:"accountID,omitempty"` - KeyBytes []byte `json:"key"` - CreatedAt time.Time `json:"createdAt"` - BoundAt time.Time `json:"boundAt"` -} - -type dbExternalAccountKeyReference struct { - Reference string `json:"reference"` - ExternalAccountKeyID string `json:"externalAccountKeyID"` -} - func (db *DB) getAccountIDByKeyID(ctx context.Context, kid string) (string, error) { id, err := db.db.Get(accountByKeyIDTable, []byte(kid)) if err != nil { @@ -73,24 +54,6 @@ func (db *DB) getDBAccount(ctx context.Context, id string) (*dbAccount, error) { return dbacc, nil } -// getDBExternalAccountKey retrieves and unmarshals dbExternalAccountKey. -func (db *DB) getDBExternalAccountKey(ctx context.Context, id string) (*dbExternalAccountKey, error) { - data, err := db.db.Get(externalAccountKeyTable, []byte(id)) - if err != nil { - if nosqlDB.IsErrNotFound(err) { - return nil, acme.ErrNotFound - } - return nil, errors.Wrapf(err, "error loading external account key %s", id) - } - - dbeak := new(dbExternalAccountKey) - if err = json.Unmarshal(data, dbeak); err != nil { - return nil, errors.Wrapf(err, "error unmarshaling external account key %s into dbExternalAccountKey", id) - } - - return dbeak, nil -} - // GetAccount retrieves an ACME account by ID. func (db *DB) GetAccount(ctx context.Context, id string) (*acme.Account, error) { dbacc, err := db.getDBAccount(ctx, id) diff --git a/acme/db/nosql/eab.go b/acme/db/nosql/eab.go index 6b6334a5..51f0e053 100644 --- a/acme/db/nosql/eab.go +++ b/acme/db/nosql/eab.go @@ -4,14 +4,59 @@ import ( "context" "crypto/rand" "encoding/json" + "sync" + "time" "github.com/pkg/errors" "github.com/smallstep/certificates/acme" nosqlDB "github.com/smallstep/nosql" ) +// externalAccountKeyMutex for read/write locking of EAK operations. +var externalAccountKeyMutex sync.RWMutex + +// referencesByProvisionerIndexMutex for locking referencesByProvisioner index operations. +var referencesByProvisionerIndexMutex sync.Mutex + +type dbExternalAccountKey struct { + ID string `json:"id"` + ProvisionerID string `json:"provisionerID"` + Reference string `json:"reference"` + AccountID string `json:"accountID,omitempty"` + KeyBytes []byte `json:"key"` + CreatedAt time.Time `json:"createdAt"` + BoundAt time.Time `json:"boundAt"` +} + +type dbExternalAccountKeyReference struct { + Reference string `json:"reference"` + ExternalAccountKeyID string `json:"externalAccountKeyID"` +} + +// getDBExternalAccountKey retrieves and unmarshals dbExternalAccountKey. +func (db *DB) getDBExternalAccountKey(ctx context.Context, id string) (*dbExternalAccountKey, error) { + data, err := db.db.Get(externalAccountKeyTable, []byte(id)) + if err != nil { + if nosqlDB.IsErrNotFound(err) { + return nil, acme.ErrNotFound + } + return nil, errors.Wrapf(err, "error loading external account key %s", id) + } + + dbeak := new(dbExternalAccountKey) + if err = json.Unmarshal(data, dbeak); err != nil { + return nil, errors.Wrapf(err, "error unmarshaling external account key %s into dbExternalAccountKey", id) + } + + return dbeak, nil +} + // CreateExternalAccountKey creates a new External Account Binding key with a name func (db *DB) CreateExternalAccountKey(ctx context.Context, provisionerID, reference string) (*acme.ExternalAccountKey, error) { + + externalAccountKeyMutex.Lock() + defer externalAccountKeyMutex.Unlock() + keyID, err := randID() if err != nil { return nil, err @@ -62,6 +107,9 @@ func (db *DB) CreateExternalAccountKey(ctx context.Context, provisionerID, refer // GetExternalAccountKey retrieves an External Account Binding key by KeyID func (db *DB) GetExternalAccountKey(ctx context.Context, provisionerID, keyID string) (*acme.ExternalAccountKey, error) { + externalAccountKeyMutex.RLock() + defer externalAccountKeyMutex.RUnlock() + dbeak, err := db.getDBExternalAccountKey(ctx, keyID) if err != nil { return nil, err @@ -83,6 +131,9 @@ func (db *DB) GetExternalAccountKey(ctx context.Context, provisionerID, keyID st } func (db *DB) DeleteExternalAccountKey(ctx context.Context, provisionerID, keyID string) error { + externalAccountKeyMutex.Lock() + defer externalAccountKeyMutex.Unlock() + dbeak, err := db.getDBExternalAccountKey(ctx, keyID) if err != nil { return errors.Wrapf(err, "error loading ACME EAB Key with Key ID %s", keyID) @@ -109,8 +160,8 @@ func (db *DB) DeleteExternalAccountKey(ctx context.Context, provisionerID, keyID // GetExternalAccountKeys retrieves all External Account Binding keys for a provisioner func (db *DB) GetExternalAccountKeys(ctx context.Context, provisionerID string) ([]*acme.ExternalAccountKey, error) { - - // TODO: mutex? + externalAccountKeyMutex.RLock() + defer externalAccountKeyMutex.RUnlock() var eakIDs []string r, err := db.db.Get(externalAccountKeyIDsByProvisionerIDTable, []byte(provisionerID)) @@ -152,6 +203,9 @@ func (db *DB) GetExternalAccountKeys(ctx context.Context, provisionerID string) // GetExternalAccountKeyByReference retrieves an External Account Binding key with unique reference func (db *DB) GetExternalAccountKeyByReference(ctx context.Context, provisionerID, reference string) (*acme.ExternalAccountKey, error) { + externalAccountKeyMutex.RLock() + defer externalAccountKeyMutex.RUnlock() + if reference == "" { return nil, nil } @@ -171,6 +225,9 @@ func (db *DB) GetExternalAccountKeyByReference(ctx context.Context, provisionerI } func (db *DB) UpdateExternalAccountKey(ctx context.Context, provisionerID string, eak *acme.ExternalAccountKey) error { + externalAccountKeyMutex.Lock() + defer externalAccountKeyMutex.Unlock() + old, err := db.getDBExternalAccountKey(ctx, eak.ID) if err != nil { return err @@ -202,8 +259,8 @@ func (db *DB) UpdateExternalAccountKey(ctx context.Context, provisionerID string } func (db *DB) addEAKID(ctx context.Context, provisionerID, eakID string) error { - referencesByProvisionerIndexMux.Lock() - defer referencesByProvisionerIndexMux.Unlock() + referencesByProvisionerIndexMutex.Lock() + defer referencesByProvisionerIndexMutex.Unlock() if eakID == "" { return errors.Errorf("can't add empty eakID for provisioner %s", provisionerID) @@ -245,8 +302,8 @@ func (db *DB) addEAKID(ctx context.Context, provisionerID, eakID string) error { } func (db *DB) deleteEAKID(ctx context.Context, provisionerID, eakID string) error { - referencesByProvisionerIndexMux.Lock() - defer referencesByProvisionerIndexMux.Unlock() + referencesByProvisionerIndexMutex.Lock() + defer referencesByProvisionerIndexMutex.Unlock() var eakIDs []string b, err := db.db.Get(externalAccountKeyIDsByProvisionerIDTable, []byte(provisionerID))