From 52e2a1668ca9572e0b5ec638d82c4148bedc80fa Mon Sep 17 00:00:00 2001 From: Eric Chiang Date: Mon, 31 Oct 2016 23:00:55 -0700 Subject: [PATCH 1/3] storage/sql: use isolation level "serializable" for transactions --- storage/sql/config_test.go | 27 +++++++++++++++++++++------ storage/sql/sql.go | 25 ++++++++++++++++++++++++- 2 files changed, 45 insertions(+), 7 deletions(-) diff --git a/storage/sql/config_test.go b/storage/sql/config_test.go index 169e89ae..1afcf520 100644 --- a/storage/sql/config_test.go +++ b/storage/sql/config_test.go @@ -36,6 +36,7 @@ func cleanDB(c *conn) error { delete from auth_code; delete from refresh_token; delete from keys; + delete from password; `) return err } @@ -48,6 +49,7 @@ func TestSQLite3(t *testing.T) { s := &SQLite3{":memory:"} conn, err := s.open() if err != nil { + fmt.Fprintln(os.Stdout, err) t.Fatal(err) } return conn @@ -58,15 +60,25 @@ func TestSQLite3(t *testing.T) { }) } +func getenv(key, defaultVal string) string { + if val := os.Getenv(key); val != "" { + return val + } + return defaultVal +} + +const testPostgresEnv = "DEX_POSTGRES_HOST" + func TestPostgres(t *testing.T) { - if os.Getenv("DEX_POSTGRES_HOST") == "" { - t.Skip("postgres envs not set, skipping tests") + host := os.Getenv(testPostgresEnv) + if host == "" { + t.Skipf("test environment variable %q not set, skipping", testPostgresEnv) } p := Postgres{ - Database: os.Getenv("DEX_POSTGRES_DATABASE"), - User: os.Getenv("DEX_POSTGRES_USER"), - Password: os.Getenv("DEX_POSTGRES_PASSWORD"), - Host: os.Getenv("DEX_POSTGRES_HOST"), + Database: getenv("DEX_POSTGRES_DATABASE", "postgres"), + User: getenv("DEX_POSTGRES_USER", "postgres"), + Password: getenv("DEX_POSTGRES_PASSWORD", "postgres"), + Host: host, SSL: PostgresSSL{ Mode: sslDisable, // Postgres container doesn't support SSL. }, @@ -92,4 +104,7 @@ func TestPostgres(t *testing.T) { withTimeout(time.Minute*1, func() { conformance.RunTests(t, newStorage) }) + withTimeout(time.Minute*1, func() { + conformance.RunTransactionTests(t, newStorage) + }) } diff --git a/storage/sql/sql.go b/storage/sql/sql.go index 113fa972..02cc4f4a 100644 --- a/storage/sql/sql.go +++ b/storage/sql/sql.go @@ -45,7 +45,30 @@ func matchLiteral(s string) *regexp.Regexp { var ( // The "github.com/lib/pq" driver is the default flavor. All others are // translations of this. - flavorPostgres = flavor{} + flavorPostgres = flavor{ + // The default behavior for Postgres transactions is consistent reads, not consistent writes. + // For each transaction opened, ensure it has the correct isolation level. + // + // See: https://www.postgresql.org/docs/9.3/static/sql-set-transaction.html + // + // NOTE(ericchiang): For some reason using `SET SESSION CHARACTERISTICS AS TRANSACTION` at a + // session level didn't work for some edge cases. Might be something worth exploring. + executeTx: func(db *sql.DB, fn func(sqlTx *sql.Tx) error) error { + tx, err := db.Begin() + if err != nil { + return err + } + defer tx.Rollback() + + if _, err := tx.Exec(`SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;`); err != nil { + return err + } + if err := fn(tx); err != nil { + return err + } + return tx.Commit() + }, + } flavorSQLite3 = flavor{ queryReplacers: []replacer{ From 786e12b15e40564f5884e5356b92c4aa2912969c Mon Sep 17 00:00:00 2001 From: Eric Chiang Date: Mon, 31 Oct 2016 23:01:31 -0700 Subject: [PATCH 2/3] storage/conformance: expand transaction test suite --- storage/conformance/transactions.go | 130 ++++++++++++++++++++++++++-- 1 file changed, 125 insertions(+), 5 deletions(-) diff --git a/storage/conformance/transactions.go b/storage/conformance/transactions.go index 6fbe10a0..3478460d 100644 --- a/storage/conformance/transactions.go +++ b/storage/conformance/transactions.go @@ -4,6 +4,9 @@ package conformance import ( "testing" + "time" + + "golang.org/x/crypto/bcrypt" "github.com/coreos/dex/storage" ) @@ -17,7 +20,10 @@ import ( // conformance. func RunTransactionTests(t *testing.T, newStorage func() storage.Storage) { runTests(t, newStorage, []subTest{ + {"AuthRequestConcurrentUpdate", testAuthRequestConcurrentUpdate}, {"ClientConcurrentUpdate", testClientConcurrentUpdate}, + {"PasswordConcurrentUpdate", testPasswordConcurrentUpdate}, + {"KeysConcurrentUpdate", testKeysConcurrentUpdate}, }) } @@ -45,10 +51,124 @@ func testClientConcurrentUpdate(t *testing.T, s storage.Storage) { return old, nil }) - t.Logf("update1: %v", err1) - t.Logf("update2: %v", err2) - - if err1 == nil && err2 == nil { - t.Errorf("update client: concurrent updates both returned no error") + if (err1 == nil) == (err2 == nil) { + t.Errorf("update client:\nupdate1: %v\nupdate2: %v\n", err1, err2) + } +} + +func testAuthRequestConcurrentUpdate(t *testing.T, s storage.Storage) { + a := storage.AuthRequest{ + ID: storage.NewID(), + ClientID: "foobar", + ResponseTypes: []string{"code"}, + Scopes: []string{"openid", "email"}, + RedirectURI: "https://localhost:80/callback", + Nonce: "foo", + State: "bar", + ForceApprovalPrompt: true, + LoggedIn: true, + Expiry: neverExpire, + ConnectorID: "ldap", + ConnectorData: []byte(`{"some":"data"}`), + Claims: storage.Claims{ + UserID: "1", + Username: "jane", + Email: "jane.doe@example.com", + EmailVerified: true, + Groups: []string{"a", "b"}, + }, + } + + if err := s.CreateAuthRequest(a); err != nil { + t.Fatalf("failed creating auth request: %v", err) + } + + var err1, err2 error + + err1 = s.UpdateAuthRequest(a.ID, func(old storage.AuthRequest) (storage.AuthRequest, error) { + old.State = "state 1" + err2 = s.UpdateAuthRequest(a.ID, func(old storage.AuthRequest) (storage.AuthRequest, error) { + old.State = "state 2" + return old, nil + }) + return old, nil + }) + + if (err1 == nil) == (err2 == nil) { + t.Errorf("update auth request:\nupdate1: %v\nupdate2: %v\n", err1, err2) + } +} + +func testPasswordConcurrentUpdate(t *testing.T, s storage.Storage) { + // Use bcrypt.MinCost to keep the tests short. + passwordHash, err := bcrypt.GenerateFromPassword([]byte("secret"), bcrypt.MinCost) + if err != nil { + t.Fatal(err) + } + + password := storage.Password{ + Email: "jane@example.com", + Hash: passwordHash, + Username: "jane", + UserID: "foobar", + } + if err := s.CreatePassword(password); err != nil { + t.Fatalf("create password token: %v", err) + } + + var err1, err2 error + + err1 = s.UpdatePassword(password.Email, func(old storage.Password) (storage.Password, error) { + old.Username = "user 1" + err2 = s.UpdatePassword(password.Email, func(old storage.Password) (storage.Password, error) { + old.Username = "user 2" + return old, nil + }) + return old, nil + }) + + if (err1 == nil) == (err2 == nil) { + t.Errorf("update password: concurrent updates both returned no error") + } +} + +func testKeysConcurrentUpdate(t *testing.T, s storage.Storage) { + // Test twice. Once for a create, once for an update. + for i := 0; i < 2; i++ { + n := time.Now().UTC().Round(time.Second) + keys1 := storage.Keys{ + SigningKey: jsonWebKeys[0].Private, + SigningKeyPub: jsonWebKeys[0].Public, + NextRotation: n, + } + + keys2 := storage.Keys{ + SigningKey: jsonWebKeys[2].Private, + SigningKeyPub: jsonWebKeys[2].Public, + NextRotation: n.Add(time.Hour), + VerificationKeys: []storage.VerificationKey{ + { + PublicKey: jsonWebKeys[0].Public, + Expiry: n.Add(time.Hour), + }, + { + PublicKey: jsonWebKeys[1].Public, + Expiry: n.Add(time.Hour * 2), + }, + }, + } + + var err1, err2 error + + err1 = s.UpdateKeys(func(old storage.Keys) (storage.Keys, error) { + err2 = s.UpdateKeys(func(old storage.Keys) (storage.Keys, error) { + return keys1, nil + }) + return keys2, nil + }) + + if (err1 == nil) == (err2 == nil) { + t.Errorf("update keys: concurrent updates both returned no error") + } } } From 8debe683141383201c950dae23da11ec6dc569fd Mon Sep 17 00:00:00 2001 From: Eric Chiang Date: Mon, 31 Oct 2016 23:18:40 -0700 Subject: [PATCH 3/3] Documentation: remove caveat about running multiple instances --- Documentation/storage.md | 7 ------- 1 file changed, 7 deletions(-) diff --git a/Documentation/storage.md b/Documentation/storage.md index 84d29550..dbc46bf0 100644 --- a/Documentation/storage.md +++ b/Documentation/storage.md @@ -4,13 +4,6 @@ Dex requires persisting state to perform various tasks such as track refresh tok Storage breaches are serious as they can affect applications that rely on dex. Dex saves sensitive data in its backing storage, including signing keys and bcrypt'd passwords. As such, transport security and database ACLs should both be used, no matter which storage option is chosen. - -## Caveat: running replicated instances - -Tests still need to be written to validate that multiple instances of dex behave correctly when using the same storage. While there aren't any technical limitations, edge cases have been observed and progress on these kind of bugs can be found on the [dex issue tracker][issues-transaction-tests]. - -The dex team suggests running one dex instance most of the time and two instance during upgrades. - ## Kubernetes third party resources __NOTE:__ Dex requires Kubernetes version 1.4+.