From b73fe8c157acc673199caa3c1bb84cd8a82ac493 Mon Sep 17 00:00:00 2001 From: max furman Date: Thu, 2 May 2019 15:26:18 -0700 Subject: [PATCH 1/2] Add used OTT to DB during authToken step --- Gopkg.lock | 4 +- authority/authorize.go | 23 ++++++++--- authority/authorize_test.go | 77 ++++++++++++++++++++++++++++++++++++- authority/db_test.go | 11 ++++++ authority/tls_test.go | 20 ++++++++-- db/db.go | 21 +++++++++- db/db_test.go | 74 +++++++++++++++++++++++++++++++++++ db/noop.go | 5 +++ 8 files changed, 222 insertions(+), 13 deletions(-) diff --git a/Gopkg.lock b/Gopkg.lock index d4514ef2..8f6edc08 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -363,7 +363,7 @@ [[projects]] branch = "master" - digest = "1:f1f1df1e19d55a1ef1f0a63633191e7d2a99993c0a17f945c0b9ebd16b17871b" + digest = "1:5e778214d472b6d2ad4d544d293d1478d9b222db8ffc6079623fbe3e58e1841e" name = "github.com/smallstep/nosql" packages = [ ".", @@ -373,7 +373,7 @@ "mysql", ] pruneopts = "UT" - revision = "5a355c598075a346d9ca9b50ec10e3f86ac66148" + revision = "b66b34823456721912ba037126e92414690c07d6" [[projects]] branch = "master" diff --git a/authority/authorize.go b/authority/authorize.go index 62152d09..3f4d048a 100644 --- a/authority/authorize.go +++ b/authority/authorize.go @@ -8,6 +8,7 @@ import ( "github.com/pkg/errors" "github.com/smallstep/certificates/authority/provisioner" + "github.com/smallstep/certificates/db" "github.com/smallstep/cli/jose" ) @@ -72,11 +73,23 @@ func (a *Authority) authorizeToken(ott string) (provisioner.Interface, error) { reuseKey = claims.Nonce } if reuseKey != "" { - if _, ok := a.ottMap.LoadOrStore(reuseKey, &idUsed{ - UsedAt: time.Now().Unix(), - Subject: claims.Subject, - }); ok { - return nil, &apiError{errors.Errorf("authorizeToken: token already used"), http.StatusUnauthorized, errContext} + switch a.db.(type) { + case *db.NoopDB: + if _, ok := a.ottMap.LoadOrStore(reuseKey, &idUsed{ + UsedAt: time.Now().Unix(), + Subject: claims.Subject, + }); ok { + return nil, &apiError{errors.Errorf("authorizeToken: token already used"), http.StatusUnauthorized, errContext} + } + default: + ok, err := a.db.UseToken(reuseKey, ott) + if err != nil { + return nil, &apiError{errors.Wrap(err, "authorizeToken: failed when checking if token already used"), + http.StatusInternalServerError, errContext} + } + if !ok { + return nil, &apiError{errors.Errorf("authorizeToken: token already used"), http.StatusUnauthorized, errContext} + } } } diff --git a/authority/authorize_test.go b/authority/authorize_test.go index 90365655..43b51ff8 100644 --- a/authority/authorize_test.go +++ b/authority/authorize_test.go @@ -117,7 +117,7 @@ func TestAuthority_authorizeToken(t *testing.T) { http.StatusUnauthorized, context{"ott": raw}}, } }, - "ok": func(t *testing.T) *authorizeTest { + "ok/noopdb": func(t *testing.T) *authorizeTest { cl := jwt.Claims{ Subject: "test.smallstep.com", Issuer: validIssuer, @@ -133,7 +133,7 @@ func TestAuthority_authorizeToken(t *testing.T) { ott: raw, } }, - "fail/token-already-used": func(t *testing.T) *authorizeTest { + "fail/noopdb/token-already-used": func(t *testing.T) *authorizeTest { _a := testAuthority(t) cl := jwt.Claims{ @@ -155,6 +155,79 @@ func TestAuthority_authorizeToken(t *testing.T) { http.StatusUnauthorized, context{"ott": raw}}, } }, + "ok/mockNoSQLDB": func(t *testing.T) *authorizeTest { + _a := testAuthority(t) + _a.db = &MockAuthDB{ + useToken: func(id, tok string) (bool, error) { + return true, nil + }, + } + + cl := jwt.Claims{ + Subject: "test.smallstep.com", + Issuer: validIssuer, + NotBefore: jwt.NewNumericDate(now), + Expiry: jwt.NewNumericDate(now.Add(time.Minute)), + Audience: validAudience, + ID: "43", + } + raw, err := jwt.Signed(sig).Claims(cl).CompactSerialize() + assert.FatalError(t, err) + return &authorizeTest{ + auth: _a, + ott: raw, + } + }, + "fail/mockNoSQLDB/error": func(t *testing.T) *authorizeTest { + _a := testAuthority(t) + _a.db = &MockAuthDB{ + useToken: func(id, tok string) (bool, error) { + return false, errors.New("force") + }, + } + + cl := jwt.Claims{ + Subject: "test.smallstep.com", + Issuer: validIssuer, + NotBefore: jwt.NewNumericDate(now), + Expiry: jwt.NewNumericDate(now.Add(time.Minute)), + Audience: validAudience, + ID: "43", + } + raw, err := jwt.Signed(sig).Claims(cl).CompactSerialize() + assert.FatalError(t, err) + return &authorizeTest{ + auth: _a, + ott: raw, + err: &apiError{errors.New("authorizeToken: failed when checking if token already used: force"), + http.StatusInternalServerError, context{"ott": raw}}, + } + }, + "fail/mockNoSQLDB/token-already-used": func(t *testing.T) *authorizeTest { + _a := testAuthority(t) + _a.db = &MockAuthDB{ + useToken: func(id, tok string) (bool, error) { + return false, nil + }, + } + + cl := jwt.Claims{ + Subject: "test.smallstep.com", + Issuer: validIssuer, + NotBefore: jwt.NewNumericDate(now), + Expiry: jwt.NewNumericDate(now.Add(time.Minute)), + Audience: validAudience, + ID: "43", + } + raw, err := jwt.Signed(sig).Claims(cl).CompactSerialize() + assert.FatalError(t, err) + return &authorizeTest{ + auth: _a, + ott: raw, + err: &apiError{errors.New("authorizeToken: token already used"), + http.StatusUnauthorized, context{"ott": raw}}, + } + }, } for name, genTestCase := range tests { diff --git a/authority/db_test.go b/authority/db_test.go index b4e073f1..870a3984 100644 --- a/authority/db_test.go +++ b/authority/db_test.go @@ -13,6 +13,7 @@ type MockAuthDB struct { isRevoked func(string) (bool, error) revoke func(rci *db.RevokedCertificateInfo) error storeCertificate func(crt *x509.Certificate) error + useToken func(id, tok string) (bool, error) shutdown func() error } @@ -33,6 +34,16 @@ func (m *MockAuthDB) IsRevoked(sn string) (bool, error) { return m.ret1.(bool), m.err } +func (m *MockAuthDB) UseToken(id, tok string) (bool, error) { + if m.useToken != nil { + return m.useToken(id, tok) + } + if m.ret1 == nil { + return false, m.err + } + return m.ret1.(bool), m.err +} + func (m *MockAuthDB) Revoke(rci *db.RevokedCertificateInfo) error { if m.revoke != nil { return m.revoke(rci) diff --git a/authority/tls_test.go b/authority/tls_test.go index ea2c4063..7efe7ab9 100644 --- a/authority/tls_test.go +++ b/authority/tls_test.go @@ -640,7 +640,12 @@ func TestRevoke(t *testing.T) { }, "error/db-revoke": func() test { a := testAuthority(t) - a.db = &MockAuthDB{err: errors.New("force")} + a.db = &MockAuthDB{ + useToken: func(id, tok string) (bool, error) { + return true, nil + }, + err: errors.New("force"), + } cl := jwt.Claims{ Subject: "sn", @@ -671,7 +676,12 @@ func TestRevoke(t *testing.T) { }, "error/already-revoked": func() test { a := testAuthority(t) - a.db = &MockAuthDB{err: db.ErrAlreadyExists} + a.db = &MockAuthDB{ + useToken: func(id, tok string) (bool, error) { + return true, nil + }, + err: db.ErrAlreadyExists, + } cl := jwt.Claims{ Subject: "sn", @@ -702,7 +712,11 @@ func TestRevoke(t *testing.T) { }, "ok/token": func() test { a := testAuthority(t) - a.db = &MockAuthDB{} + a.db = &MockAuthDB{ + useToken: func(id, tok string) (bool, error) { + return true, nil + }, + } cl := jwt.Claims{ Subject: "sn", diff --git a/db/db.go b/db/db.go index 886c3ef7..3687b113 100644 --- a/db/db.go +++ b/db/db.go @@ -10,8 +10,9 @@ import ( ) var ( - revokedCertsTable = []byte("revoked_x509_certs") certsTable = []byte("x509_certs") + revokedCertsTable = []byte("revoked_x509_certs") + usedOTTTable = []byte("used_ott") ) // ErrAlreadyExists can be returned if the DB attempts to set a key that has @@ -31,6 +32,7 @@ type AuthDB interface { IsRevoked(sn string) (bool, error) Revoke(rci *RevokedCertificateInfo) error StoreCertificate(crt *x509.Certificate) error + UseToken(id, tok string) (bool, error) Shutdown() error } @@ -126,6 +128,23 @@ func (db *DB) StoreCertificate(crt *x509.Certificate) error { return nil } +// UseToken returns true if we were able to successfully store the token for +// for the first time, false otherwise. +func (db *DB) UseToken(id, tok string) (bool, error) { + // If the error is `Not Found` then the certificate has not been revoked. + // Any other error should be propagated to the caller. + _, found, err := db.LoadOrStore(usedOTTTable, []byte(id), []byte(tok)) + switch { + case err != nil: + return false, errors.Wrapf(err, "error LoadOrStore-ing token %s/%s", + string(usedOTTTable), id) + case found: + return false, nil + default: + return true, nil + } +} + // Shutdown sends a shutdown message to the database. func (db *DB) Shutdown() error { if db.isUp { diff --git a/db/db_test.go b/db/db_test.go index c2b16d57..c5dbe4fe 100644 --- a/db/db_test.go +++ b/db/db_test.go @@ -20,6 +20,17 @@ type MockNoSQLDB struct { del func(bucket, key []byte) error list func(bucket []byte) ([]*database.Entry, error) update func(tx *database.Tx) error + loadOrStore func(bucket, key, value []byte) ([]byte, bool, error) +} + +func (m *MockNoSQLDB) LoadOrStore(bucket, key, value []byte) ([]byte, bool, error) { + if m.get != nil { + return m.loadOrStore(bucket, key, value) + } + if m.ret1 == nil { + return nil, false, m.err + } + return m.ret1.([]byte), m.ret2.(bool), m.err } func (m *MockNoSQLDB) Get(bucket, key []byte) ([]byte, error) { @@ -188,3 +199,66 @@ func TestRevoke(t *testing.T) { }) } } + +func TestUseToken(t *testing.T) { + type result struct { + err error + ok bool + } + tests := map[string]struct { + id, tok string + db *DB + want result + }{ + "fail/force-LoadOrStore-error": { + id: "id", + tok: "token", + db: &DB{&MockNoSQLDB{ + loadOrStore: func(bucket, key, value []byte) ([]byte, bool, error) { + return nil, false, errors.New("force") + }, + }, true}, + want: result{ + ok: false, + err: errors.New("error LoadOrStore-ing token id/token"), + }, + }, + "fail/LoadOrStore-found": { + id: "id", + tok: "token", + db: &DB{&MockNoSQLDB{ + loadOrStore: func(bucket, key, value []byte) ([]byte, bool, error) { + return []byte("foo"), true, nil + }, + }, true}, + want: result{ + ok: false, + }, + }, + "ok/LoadOrStore-not-found": { + id: "id", + tok: "token", + db: &DB{&MockNoSQLDB{ + loadOrStore: func(bucket, key, value []byte) ([]byte, bool, error) { + return nil, false, nil + }, + }, true}, + want: result{ + ok: true, + }, + }, + } + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + ok, err := tc.db.UseToken(tc.id, tc.tok) + if err != nil { + if assert.NotNil(t, tc.want.err) { + assert.HasPrefix(t, tc.want.err.Error(), err.Error()) + } + assert.False(t, ok) + } else { + assert.True(t, ok) + } + }) + } +} diff --git a/db/noop.go b/db/noop.go index 356b7f42..27373fa1 100644 --- a/db/noop.go +++ b/db/noop.go @@ -32,6 +32,11 @@ func (n *NoopDB) StoreCertificate(crt *x509.Certificate) error { return ErrNotImplemented } +// UseToken returns a "NotImplemented" error. +func (n *NoopDB) UseToken(id, tok string) (bool, error) { + return false, ErrNotImplemented +} + // Shutdown returns nil func (n *NoopDB) Shutdown() error { return nil From 81db527f12f15e09229fa84dc7c27ec71c24114f Mon Sep 17 00:00:00 2001 From: max furman Date: Tue, 7 May 2019 11:38:27 -0700 Subject: [PATCH 2/2] NoopDB -> SimpleDB --- authority/authority.go | 6 ++-- authority/authorize.go | 31 +++++------------- authority/authorize_test.go | 5 ++- authority/tls_test.go | 3 -- db/db.go | 2 +- db/noop.go | 43 ------------------------- db/noop_test.go | 21 ------------- db/simple.go | 63 +++++++++++++++++++++++++++++++++++++ db/simple_test.go | 37 ++++++++++++++++++++++ 9 files changed, 112 insertions(+), 99 deletions(-) delete mode 100644 db/noop.go delete mode 100644 db/noop_test.go create mode 100644 db/simple.go create mode 100644 db/simple_test.go diff --git a/authority/authority.go b/authority/authority.go index aad8c3e2..5497dc2d 100644 --- a/authority/authority.go +++ b/authority/authority.go @@ -22,7 +22,6 @@ type Authority struct { intermediateIdentity *x509util.Identity validateOnce bool certificates *sync.Map - ottMap *sync.Map startTime time.Time provisioners *provisioner.Collection db db.AuthDB @@ -40,7 +39,6 @@ func New(config *Config) (*Authority, error) { var a = &Authority{ config: config, certificates: new(sync.Map), - ottMap: new(sync.Map), provisioners: provisioner.NewCollection(config.getAudiences()), } if err := a.init(); err != nil { @@ -58,8 +56,8 @@ func (a *Authority) init() error { var err error - // Initialize step-ca Database if defined in configuration. - // If a.config.DB is nil then a noopDB will be returned. + // Initialize step-ca Database. + // If a.config.DB is nil then a simple, barebones in memory DB will be used. if a.db, err = db.New(a.config.DB); err != nil { return err } diff --git a/authority/authorize.go b/authority/authorize.go index 3f4d048a..bf4ce15f 100644 --- a/authority/authorize.go +++ b/authority/authorize.go @@ -4,19 +4,12 @@ import ( "crypto/x509" "net/http" "strings" - "time" "github.com/pkg/errors" "github.com/smallstep/certificates/authority/provisioner" - "github.com/smallstep/certificates/db" "github.com/smallstep/cli/jose" ) -type idUsed struct { - UsedAt int64 `json:"ua,omitempty"` - Subject string `json:"sub,omitempty"` -} - // Claims extends jose.Claims with step attributes. type Claims struct { jose.Claims @@ -73,23 +66,13 @@ func (a *Authority) authorizeToken(ott string) (provisioner.Interface, error) { reuseKey = claims.Nonce } if reuseKey != "" { - switch a.db.(type) { - case *db.NoopDB: - if _, ok := a.ottMap.LoadOrStore(reuseKey, &idUsed{ - UsedAt: time.Now().Unix(), - Subject: claims.Subject, - }); ok { - return nil, &apiError{errors.Errorf("authorizeToken: token already used"), http.StatusUnauthorized, errContext} - } - default: - ok, err := a.db.UseToken(reuseKey, ott) - if err != nil { - return nil, &apiError{errors.Wrap(err, "authorizeToken: failed when checking if token already used"), - http.StatusInternalServerError, errContext} - } - if !ok { - return nil, &apiError{errors.Errorf("authorizeToken: token already used"), http.StatusUnauthorized, errContext} - } + ok, err := a.db.UseToken(reuseKey, ott) + if err != nil { + return nil, &apiError{errors.Wrap(err, "authorizeToken: failed when checking if token already used"), + http.StatusInternalServerError, errContext} + } + if !ok { + return nil, &apiError{errors.Errorf("authorizeToken: token already used"), http.StatusUnauthorized, errContext} } } diff --git a/authority/authorize_test.go b/authority/authorize_test.go index 43b51ff8..368e9807 100644 --- a/authority/authorize_test.go +++ b/authority/authorize_test.go @@ -117,7 +117,7 @@ func TestAuthority_authorizeToken(t *testing.T) { http.StatusUnauthorized, context{"ott": raw}}, } }, - "ok/noopdb": func(t *testing.T) *authorizeTest { + "ok/simpledb": func(t *testing.T) *authorizeTest { cl := jwt.Claims{ Subject: "test.smallstep.com", Issuer: validIssuer, @@ -133,9 +133,8 @@ func TestAuthority_authorizeToken(t *testing.T) { ott: raw, } }, - "fail/noopdb/token-already-used": func(t *testing.T) *authorizeTest { + "fail/simpledb/token-already-used": func(t *testing.T) *authorizeTest { _a := testAuthority(t) - cl := jwt.Claims{ Subject: "test.smallstep.com", Issuer: validIssuer, diff --git a/authority/tls_test.go b/authority/tls_test.go index 7efe7ab9..142eedde 100644 --- a/authority/tls_test.go +++ b/authority/tls_test.go @@ -592,7 +592,6 @@ func TestRevoke(t *testing.T) { tests := map[string]func() test{ "error/token/authorizeRevoke error": func() test { a := testAuthority(t) - a.db = new(db.NoopDB) ctx := getCtx() ctx["ott"] = "foo" return test{ @@ -609,8 +608,6 @@ func TestRevoke(t *testing.T) { }, "error/nil-db": func() test { a := testAuthority(t) - a.db = new(db.NoopDB) - cl := jwt.Claims{ Subject: "sn", Issuer: validIssuer, diff --git a/db/db.go b/db/db.go index 3687b113..0494c1ac 100644 --- a/db/db.go +++ b/db/db.go @@ -45,7 +45,7 @@ type DB struct { // New returns a new database client that implements the AuthDB interface. func New(c *Config) (AuthDB, error) { if c == nil { - return new(NoopDB), nil + return newSimpleDB(c) } db, err := nosql.New(c.Type, c.DataSource, nosql.WithDatabase(c.Database), diff --git a/db/noop.go b/db/noop.go deleted file mode 100644 index 27373fa1..00000000 --- a/db/noop.go +++ /dev/null @@ -1,43 +0,0 @@ -package db - -import ( - "crypto/x509" - - "github.com/pkg/errors" -) - -// ErrNotImplemented is an error returned when an operation is Not Implemented. -var ErrNotImplemented = errors.Errorf("not implemented") - -// NoopDB implements the DB interface with Noops -type NoopDB int - -// Init noop -func (n *NoopDB) Init(c *Config) (AuthDB, error) { - return n, nil -} - -// IsRevoked noop -func (n *NoopDB) IsRevoked(sn string) (bool, error) { - return false, nil -} - -// Revoke returns a "NotImplemented" error. -func (n *NoopDB) Revoke(rci *RevokedCertificateInfo) error { - return ErrNotImplemented -} - -// StoreCertificate returns a "NotImplemented" error. -func (n *NoopDB) StoreCertificate(crt *x509.Certificate) error { - return ErrNotImplemented -} - -// UseToken returns a "NotImplemented" error. -func (n *NoopDB) UseToken(id, tok string) (bool, error) { - return false, ErrNotImplemented -} - -// Shutdown returns nil -func (n *NoopDB) Shutdown() error { - return nil -} diff --git a/db/noop_test.go b/db/noop_test.go deleted file mode 100644 index 564a586c..00000000 --- a/db/noop_test.go +++ /dev/null @@ -1,21 +0,0 @@ -package db - -import ( - "testing" - - "github.com/smallstep/assert" -) - -func Test_noop(t *testing.T) { - db := new(NoopDB) - - _db, err := db.Init(&Config{}) - assert.FatalError(t, err) - assert.Equals(t, db, _db) - - isRevoked, err := db.IsRevoked("foo") - assert.False(t, isRevoked) - assert.Nil(t, err) - - assert.Equals(t, db.Revoke(&RevokedCertificateInfo{}), ErrNotImplemented) -} diff --git a/db/simple.go b/db/simple.go new file mode 100644 index 00000000..657a518f --- /dev/null +++ b/db/simple.go @@ -0,0 +1,63 @@ +package db + +import ( + "crypto/x509" + "sync" + "time" + + "github.com/pkg/errors" +) + +// ErrNotImplemented is an error returned when an operation is Not Implemented. +var ErrNotImplemented = errors.Errorf("not implemented") + +// SimpleDB is a barebones implementation of the DB interface. It is NOT an +// in memory implementation of the DB, but rather the bare minimum of +// functionality that the CA requires to operate securely. +type SimpleDB struct { + usedTokens *sync.Map +} + +func newSimpleDB(c *Config) (AuthDB, error) { + db := &SimpleDB{} + db.usedTokens = new(sync.Map) + return db, nil +} + +// IsRevoked noop +func (s *SimpleDB) IsRevoked(sn string) (bool, error) { + return false, nil +} + +// Revoke returns a "NotImplemented" error. +func (s *SimpleDB) Revoke(rci *RevokedCertificateInfo) error { + return ErrNotImplemented +} + +// StoreCertificate returns a "NotImplemented" error. +func (s *SimpleDB) StoreCertificate(crt *x509.Certificate) error { + return ErrNotImplemented +} + +type usedToken struct { + UsedAt int64 `json:"ua,omitempty"` + Token string `json:"tok,omitempty"` +} + +// UseToken returns a "NotImplemented" error. +func (s *SimpleDB) UseToken(id, tok string) (bool, error) { + if _, ok := s.usedTokens.LoadOrStore(id, &usedToken{ + UsedAt: time.Now().Unix(), + Token: tok, + }); ok { + // Token already exists in DB. + return false, nil + } + // Successfully stored token. + return true, nil +} + +// Shutdown returns nil +func (s *SimpleDB) Shutdown() error { + return nil +} diff --git a/db/simple_test.go b/db/simple_test.go new file mode 100644 index 00000000..ad5aa70c --- /dev/null +++ b/db/simple_test.go @@ -0,0 +1,37 @@ +package db + +import ( + "testing" + + "github.com/smallstep/assert" +) + +func TestSimpleDB(t *testing.T) { + db, err := newSimpleDB(nil) + assert.FatalError(t, err) + + // Revoke + assert.Equals(t, ErrNotImplemented, db.Revoke(nil)) + + // IsRevoked -- verify noop + isRevoked, err := db.IsRevoked("foo") + assert.False(t, isRevoked) + assert.Nil(t, err) + + // StoreCertificate + assert.Equals(t, ErrNotImplemented, db.StoreCertificate(nil)) + + // UseToken + ok, err := db.UseToken("foo", "bar") + assert.True(t, ok) + assert.Nil(t, err) + ok, err = db.UseToken("foo", "cat") + assert.False(t, ok) + assert.Nil(t, err) + + // Shutdown -- verify noop + assert.FatalError(t, db.Shutdown()) + ok, err = db.UseToken("foo", "cat") + assert.False(t, ok) + assert.Nil(t, err) +}