Skip to content

Commit

Permalink
jwk: Auto-remove old keys when upgrading from < beta.7 (ory#925)
Browse files Browse the repository at this point in the history
Closes ory#921

Signed-off-by: arekkas <[email protected]>
  • Loading branch information
arekkas authored Jul 15, 2018
1 parent 35bf581 commit 6ca0733
Show file tree
Hide file tree
Showing 5 changed files with 170 additions and 27 deletions.
15 changes: 14 additions & 1 deletion UPGRADE.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@ before finalizing the upgrade process.
<!-- DON'T EDIT THIS SECTION, INSTEAD RE-RUN doctoc TO UPDATE -->


- [1.0.0-beta.7](#100-beta7)
- [Regenerated OpenID Connect ID Token cryptographic keys](#regenerated-openid-connect-id-token-cryptographic-keys)
- [1.0.0-beta.5](#100-beta5)
- [OAuth 2.0 Client Response Type changes](#oauth-20-client-response-type-changes)
- [Schema Changes](#schema-changes)
- [HTTP Error Payload](#http-error-payload)
- [OAuth 2.0 Clients must specify correct `token_endpoint_auth_method`](#oauth-20-clients-must-specify-correct-token_endpoint_auth_method)
Expand All @@ -19,7 +22,7 @@ before finalizing the upgrade process.
- [Breaking Changes](#breaking-changes)
- [Introspection API](#introspection-api)
- [Introspection is now capable of introspecting refresh tokens](#introspection-is-now-capable-of-introspecting-refresh-tokens)
- [Access Control & Warden API](#access-control-&-warden-api)
- [Access Control & Warden API](#access-control--warden-api)
- [Running the backwards compatible set up](#running-the-backwards-compatible-set-up)
- [Warden API](#warden-api)
- [Warden Groups](#warden-groups)
Expand Down Expand Up @@ -88,6 +91,16 @@ before finalizing the upgrade process.

<!-- END doctoc generated TOC please keep comment here to allow auto update -->

## 1.0.0-beta.7

### Regenerated OpenID Connect ID Token cryptographic keys

This patch resolves an issue which caused the migration to fail from beta.4 to beta.5 / beta.6. The reason being that
the keys stored in the data store had mismatching `kid` values if generated by <= beta.5. This patch runs a SQL migration
script which removes the old key and then, after booting up ORY Hydra, regenerates it.

To apply this change, please run you must run `hydra migrate sql` against your database.

## 1.0.0-beta.5

This patch implements the OpenID Connect Dynamic Client registration specification and thus now supports client authentication
Expand Down
121 changes: 121 additions & 0 deletions jwk/manager_0_sql_migrations_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
/*
* Copyright © 2015-2018 Aeneas Rekkas <[email protected]>
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
* @author Aeneas Rekkas <[email protected]>
* @Copyright 2017-2018 Aeneas Rekkas <[email protected]>
* @license Apache-2.0
*/

package jwk_test

import (
"fmt"
"log"
"testing"

"github.com/jmoiron/sqlx"
"github.com/ory/hydra/jwk"
"github.com/ory/sqlcon/dockertest"
"github.com/rubenv/sql-migrate"
"github.com/stretchr/testify/require"
)

var createJWKMigrations = []*migrate.Migration{
{
Id: "1-data",
Up: []string{
`INSERT INTO hydra_jwk (sid, kid, version, keydata) VALUES ('1-sid', '1-kid', 0, 'some-key')`,
},
Down: []string{
`DELETE FROM hydra_jwk WHERE sid='1-sid'`,
},
},
{
Id: "2-data",
Up: []string{
`INSERT INTO hydra_jwk (sid, kid, version, keydata, created_at) VALUES ('2-sid', '2-kid', 0, 'some-key', NOW())`,
},
Down: []string{
`DELETE FROM hydra_jwk WHERE sid='2-sid'`,
},
},
{
Id: "3-data",
Up: []string{
`INSERT INTO hydra_jwk (sid, kid, version, keydata, created_at) VALUES ('3-sid', '3-kid', 0, 'some-key', NOW())`,
},
Down: []string{
`DELETE FROM hydra_jwk WHERE sid='3-sid'`,
},
},
}

var migrations = &migrate.MemoryMigrationSource{
Migrations: []*migrate.Migration{
{Id: "0-data-0", Up: []string{"DROP TABLE IF EXISTS hydra_jwk"}},
{Id: "0-data-1", Up: []string{"DROP TABLE IF EXISTS hydra_jwk_migration"}},
jwk.Migrations.Migrations[0],
createJWKMigrations[0],
jwk.Migrations.Migrations[1],
createJWKMigrations[1],
jwk.Migrations.Migrations[2],
createJWKMigrations[2],
},
}

func TestMigrations(t *testing.T) {
var dbs = map[string]*sqlx.DB{}
if testing.Short() {
return
}

dockertest.Parallel([]func(){
func() {
db, err := dockertest.ConnectToTestPostgreSQL()
if err != nil {
log.Fatalf("Could not connect to database: %v", err)
}
dbs["postgres"] = db
},
func() {
db, err := dockertest.ConnectToTestMySQL()
if err != nil {
log.Fatalf("Could not connect to database: %v", err)
}
dbs["mysql"] = db
},
})

for k, db := range dbs {
t.Run(fmt.Sprintf("database=%s", k), func(t *testing.T) {
migrate.SetTable("hydra_jwk_migration_integration")
for step := range migrations.Migrations {
t.Run(fmt.Sprintf("step=%d", step), func(t *testing.T) {
n, err := migrate.ExecMax(db.DB, db.DriverName(), migrations, migrate.Up, 1)
require.NoError(t, err)
require.Equal(t, n, 1)
})
}

for step := range migrations.Migrations {
t.Run(fmt.Sprintf("step=%d", step), func(t *testing.T) {
n, err := migrate.ExecMax(db.DB, db.DriverName(), migrations, migrate.Down, 1)
require.NoError(t, err)
require.Equal(t, n, 1)
})
}
})
}
}
14 changes: 11 additions & 3 deletions jwk/manager_sql.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ type SQLManager struct {
Cipher *AEAD
}

var migrations = &migrate.MemoryMigrationSource{
var Migrations = &migrate.MemoryMigrationSource{
Migrations: []*migrate.Migration{
{
Id: "1",
Expand All @@ -63,6 +63,14 @@ var migrations = &migrate.MemoryMigrationSource{
`ALTER TABLE hydra_jwk DROP COLUMN created_at`,
},
},
// See https://github.com/ory/hydra/issues/921
{
Id: "3",
Up: []string{
`DELETE FROM hydra_jwk WHERE sid='hydra.openid.id-token'`,
},
Down: []string{},
},
},
}

Expand All @@ -76,9 +84,9 @@ type sqlData struct {

func (m *SQLManager) CreateSchemas() (int, error) {
migrate.SetTable("hydra_jwk_migration")
n, err := migrate.Exec(m.DB.DB, m.DB.DriverName(), migrations, migrate.Up)
n, err := migrate.Exec(m.DB.DB, m.DB.DriverName(), Migrations, migrate.Up)
if err != nil {
return 0, errors.Wrapf(err, "Could not migrate sql schema, applied %d migrations", n)
return 0, errors.Wrapf(err, "Could not migrate sql schema, applied %d Migrations", n)
}
return n, nil
}
Expand Down
19 changes: 10 additions & 9 deletions jwk/manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,6 @@ func connectToPG() {
}

s := &SQLManager{DB: db, Cipher: &AEAD{Key: encryptionKey}}
if _, err := s.CreateSchemas(); err != nil {
log.Fatalf("Could not create schema: %v", err)
}

managers["postgres"] = s
}

Expand All @@ -76,11 +72,6 @@ func connectToMySQL() {
}

s := &SQLManager{DB: db, Cipher: &AEAD{Key: encryptionKey}}

if _, err := s.CreateSchemas(); err != nil {
log.Fatalf("Could not create schema: %v", err)
}

managers["mysql"] = s
}

Expand All @@ -89,6 +80,11 @@ func TestManagerKey(t *testing.T) {
require.NoError(t, err)

for name, m := range managers {
if m, ok := m.(*SQLManager); ok {
n, err := m.CreateSchemas()
require.NoError(t, err)
t.Logf("Applied %d migrations to %s", n, name)
}
t.Run(fmt.Sprintf("case=%s", name), TestHelperManagerKey(m, ks, "TestManagerKey"))
}
}
Expand All @@ -99,6 +95,11 @@ func TestManagerKeySet(t *testing.T) {
ks.Key("private")

for name, m := range managers {
if m, ok := m.(*SQLManager); ok {
n, err := m.CreateSchemas()
require.NoError(t, err)
t.Logf("Applied %d migrations to %s", n, name)
}
t.Run(fmt.Sprintf("case=%s", name), TestHelperManagerKeySet(m, ks, "TestManagerKeySet"))
}
}
28 changes: 14 additions & 14 deletions jwk/manager_test_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,42 +50,42 @@ func TestHelperManagerKey(m Manager, keys *jose.JSONWebKeySet, suffix string) fu
assert.NotNil(t, err)

err = m.AddKey("faz", First(priv))
assert.Nil(t, err)
require.NoError(t, err)

got, err := m.GetKey("faz", "private:"+suffix)
assert.Nil(t, err)
require.NoError(t, err)
assert.Equal(t, priv, got.Keys)

err = m.AddKey("faz", First(pub))
assert.Nil(t, err)
require.NoError(t, err)

got, err = m.GetKey("faz", "private:"+suffix)
assert.Nil(t, err)
require.NoError(t, err)
assert.Equal(t, priv, got.Keys)

got, err = m.GetKey("faz", "public:"+suffix)
assert.Nil(t, err)
require.NoError(t, err)
assert.Equal(t, pub, got.Keys)

// Because MySQL
time.Sleep(time.Second * 2)

First(pub).KeyID = "new-key-id:" + suffix
err = m.AddKey("faz", First(pub))
assert.Nil(t, err)
require.NoError(t, err)

_, err = m.GetKey("faz", "new-key-id:"+suffix)
assert.Nil(t, err)
require.NoError(t, err)

keys, err = m.GetKeySet("faz")
assert.Nil(t, err)
require.NoError(t, err)
assert.EqualValues(t, "new-key-id:"+suffix, First(keys.Keys).KeyID)

err = m.DeleteKey("faz", "public:"+suffix)
assert.Nil(t, err)
require.NoError(t, err)

_, err = m.GetKey("faz", "public:"+suffix)
assert.NotNil(t, err)
require.Error(t, err)
}
}

Expand All @@ -96,17 +96,17 @@ func TestHelperManagerKeySet(m Manager, keys *jose.JSONWebKeySet, suffix string)
require.Error(t, err)

err = m.AddKeySet("bar", keys)
assert.Nil(t, err)
require.NoError(t, err)

got, err := m.GetKeySet("bar")
assert.Nil(t, err)
require.NoError(t, err)
assert.Equal(t, keys.Key("public:"+suffix), got.Key("public:"+suffix))
assert.Equal(t, keys.Key("private:"+suffix), got.Key("private:"+suffix))

err = m.DeleteKeySet("bar")
assert.Nil(t, err)
require.NoError(t, err)

_, err = m.GetKeySet("bar")
assert.NotNil(t, err)
require.Error(t, err)
}
}

0 comments on commit 6ca0733

Please sign in to comment.