From c242602231869e86186086518239c167b28ad227 Mon Sep 17 00:00:00 2001 From: max furman Date: Thu, 25 Apr 2019 12:37:25 -0700 Subject: [PATCH] reload and shutdown trickery * Only shutdown the database once. * Be careful when reloading the CA. Depending on whether the DB has already been shutdown, and error may be unrecoverable. --- Gopkg.lock | 1 + ca/ca.go | 51 ++++++++++++++++++++++++++++++++++++++++++++++----- db/db.go | 10 +++++++--- db/db_test.go | 42 +++++++++++++++++++++--------------------- 4 files changed, 75 insertions(+), 29 deletions(-) diff --git a/Gopkg.lock b/Gopkg.lock index 43b0e23c..bd6c951b 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -671,6 +671,7 @@ "github.com/smallstep/cli/token/provision", "github.com/smallstep/cli/usage", "github.com/smallstep/nosql", + "github.com/smallstep/nosql/database", "github.com/tsenart/deadcode", "github.com/urfave/cli", "golang.org/x/crypto/ocsp", diff --git a/ca/ca.go b/ca/ca.go index 9cb6167a..5ec05981 100644 --- a/ca/ca.go +++ b/ca/ca.go @@ -3,6 +3,7 @@ package ca import ( "crypto/tls" "crypto/x509" + "log" "net/http" "github.com/go-chi/chi" @@ -123,7 +124,7 @@ func (ca *CA) Run() error { func (ca *CA) Stop() error { ca.renewer.Stop() if err := ca.auth.Shutdown(); err != nil { - return err + log.Printf("error stopping ca.Authority: %+v\n", err) } return ca.srv.Shutdown() } @@ -131,8 +132,9 @@ func (ca *CA) Stop() error { // Reload reloads the configuration of the CA and calls to the server Reload // method. func (ca *CA) Reload() error { - if err := ca.auth.Shutdown(); err != nil { - return err + var hasDB bool + if ca.config.DB != nil { + hasDB = true } if ca.opts.configFile == "" { return errors.New("error reloading ca: configuration file is not set") @@ -140,15 +142,54 @@ func (ca *CA) Reload() error { config, err := authority.LoadConfiguration(ca.opts.configFile) if err != nil { - return errors.Wrap(err, "error reloading ca") + return errors.Wrap(err, "error reloading ca configuration") + } + + logShutDown := func(ss ...string) { + for _, s := range ss { + log.Println(s) + } + log.Println("Continuing to serve requests may result in inconsistent state. Shutting Down ...") + } + logContinue := func(reason string) { + log.Println(reason) + log.Println("Continuing to run with the original configuration.") + log.Println("You can force a restart by sending a SIGTERM signal and then restarting the step-ca.") } + // Shut down the old authority (shut down the database). If New or Reload + // fails then the CA will continue to run but the database will have been + // shutdown, which will cause errors. + if err := ca.auth.Shutdown(); err != nil { + if hasDB { + logShutDown("Attempt to shut down the ca.Authority has failed.") + return ca.Stop() + } + logContinue("Reload failed because the ca.Authority could not be shut down.") + return err + } newCA, err := New(config, WithPassword(ca.opts.password), WithConfigFile(ca.opts.configFile)) if err != nil { + if hasDB { + logShutDown("Attempt to initialize a CA with the new configuration has failed.", + "The database has already been shutdown.") + return ca.Stop() + } + logContinue("Reload failed because the CA with new configuration could " + + "not be initialized.") return errors.Wrap(err, "error reloading ca") } - return ca.srv.Reload(newCA.srv) + if err = ca.srv.Reload(newCA.srv); err != nil { + if hasDB { + logShutDown("Attempt to replace the old CA server has failed.", + "The database has already been shutdown.") + return ca.Stop() + } + logContinue("Reload failed because server could not be replaced.") + return errors.Wrap(err, "error reloading server") + } + return nil } // getTLSConfig returns a TLSConfig for the CA server with a self-renewing diff --git a/db/db.go b/db/db.go index a0bd3a81..e2d044bd 100644 --- a/db/db.go +++ b/db/db.go @@ -37,6 +37,7 @@ type AuthDB interface { // DB is a wrapper over the nosql.DB interface. type DB struct { nosql.DB + isUp bool } // New returns a new database client that implements the AuthDB interface. @@ -59,7 +60,7 @@ func New(c *Config) (AuthDB, error) { } } - return &DB{db}, nil + return &DB{db, true}, nil } // RevokedCertificateInfo contains information regarding the certificate @@ -127,8 +128,11 @@ func (db *DB) StoreCertificate(crt *x509.Certificate) error { // Shutdown sends a shutdown message to the database. func (db *DB) Shutdown() error { - if err := db.Close(); err != nil { - return errors.Wrap(err, "database shutdown error") + if db.isUp { + if err := db.Close(); err != nil { + return errors.Wrap(err, "database shutdown error") + } + db.isUp = false } return nil } diff --git a/db/db_test.go b/db/db_test.go index bd3dab6a..c2b16d57 100644 --- a/db/db_test.go +++ b/db/db_test.go @@ -8,7 +8,7 @@ import ( "github.com/smallstep/nosql/database" ) -type MockdatabaseDB struct { +type MockNoSQLDB struct { err error ret1, ret2 interface{} get func(bucket, key []byte) ([]byte, error) @@ -22,7 +22,7 @@ type MockdatabaseDB struct { update func(tx *database.Tx) error } -func (m *MockdatabaseDB) Get(bucket, key []byte) ([]byte, error) { +func (m *MockNoSQLDB) Get(bucket, key []byte) ([]byte, error) { if m.get != nil { return m.get(bucket, key) } @@ -32,56 +32,56 @@ func (m *MockdatabaseDB) Get(bucket, key []byte) ([]byte, error) { return m.ret1.([]byte), m.err } -func (m *MockdatabaseDB) Set(bucket, key, value []byte) error { +func (m *MockNoSQLDB) Set(bucket, key, value []byte) error { if m.set != nil { return m.set(bucket, key, value) } return m.err } -func (m *MockdatabaseDB) Open(dataSourceName string, opt ...database.Option) error { +func (m *MockNoSQLDB) Open(dataSourceName string, opt ...database.Option) error { if m.open != nil { return m.open(dataSourceName, opt...) } return m.err } -func (m *MockdatabaseDB) Close() error { +func (m *MockNoSQLDB) Close() error { if m.close != nil { return m.close() } return m.err } -func (m *MockdatabaseDB) CreateTable(bucket []byte) error { +func (m *MockNoSQLDB) CreateTable(bucket []byte) error { if m.createTable != nil { return m.createTable(bucket) } return m.err } -func (m *MockdatabaseDB) DeleteTable(bucket []byte) error { +func (m *MockNoSQLDB) DeleteTable(bucket []byte) error { if m.deleteTable != nil { return m.deleteTable(bucket) } return m.err } -func (m *MockdatabaseDB) Del(bucket, key []byte) error { +func (m *MockNoSQLDB) Del(bucket, key []byte) error { if m.del != nil { return m.del(bucket, key) } return m.err } -func (m *MockdatabaseDB) List(bucket []byte) ([]*database.Entry, error) { +func (m *MockNoSQLDB) List(bucket []byte) ([]*database.Entry, error) { if m.list != nil { return m.list(bucket) } return m.ret1.([]*database.Entry), m.err } -func (m *MockdatabaseDB) Update(tx *database.Tx) error { +func (m *MockNoSQLDB) Update(tx *database.Tx) error { if m.update != nil { return m.update(tx) } @@ -100,16 +100,16 @@ func TestIsRevoked(t *testing.T) { }, "false/ErrNotFound": { key: "sn", - db: &DB{&MockdatabaseDB{err: database.ErrNotFound, ret1: nil}}, + db: &DB{&MockNoSQLDB{err: database.ErrNotFound, ret1: nil}, true}, }, "error/checking bucket": { key: "sn", - db: &DB{&MockdatabaseDB{err: errors.New("force"), ret1: nil}}, + db: &DB{&MockNoSQLDB{err: errors.New("force"), ret1: nil}, true}, err: errors.New("error checking revocation bucket: force"), }, "true": { key: "sn", - db: &DB{&MockdatabaseDB{ret1: []byte("value")}}, + db: &DB{&MockNoSQLDB{ret1: []byte("value")}, true}, isRevoked: true, }, } @@ -136,44 +136,44 @@ func TestRevoke(t *testing.T) { }{ "error/force isRevoked": { rci: &RevokedCertificateInfo{Serial: "sn"}, - db: &DB{&MockdatabaseDB{ + db: &DB{&MockNoSQLDB{ get: func(bucket []byte, sn []byte) ([]byte, error) { return nil, errors.New("force IsRevoked") }, - }}, + }, true}, err: errors.New("error checking revocation bucket: force IsRevoked"), }, "error/was already revoked": { rci: &RevokedCertificateInfo{Serial: "sn"}, - db: &DB{&MockdatabaseDB{ + db: &DB{&MockNoSQLDB{ get: func(bucket []byte, sn []byte) ([]byte, error) { return nil, nil }, - }}, + }, true}, err: ErrAlreadyExists, }, "error/database set": { rci: &RevokedCertificateInfo{Serial: "sn"}, - db: &DB{&MockdatabaseDB{ + db: &DB{&MockNoSQLDB{ get: func(bucket []byte, sn []byte) ([]byte, error) { return nil, database.ErrNotFound }, set: func(bucket []byte, key []byte, value []byte) error { return errors.New("force") }, - }}, + }, true}, err: errors.New("database Set error: force"), }, "ok": { rci: &RevokedCertificateInfo{Serial: "sn"}, - db: &DB{&MockdatabaseDB{ + db: &DB{&MockNoSQLDB{ get: func(bucket []byte, sn []byte) ([]byte, error) { return nil, database.ErrNotFound }, set: func(bucket []byte, key []byte, value []byte) error { return nil }, - }}, + }, true}, }, } for name, tc := range tests {