From 3a29a23cdc2823bbce30f5ab99ad01d72208de64 Mon Sep 17 00:00:00 2001 From: zeripath Date: Wed, 16 Feb 2022 21:03:58 +0000 Subject: [PATCH] Attempt to fix the webauthn migration again - part 3 (#18770) v208.go is seriously broken as it misses an ID() check. We need to no-op and remigrate all of the u2f keys. See #18756 Signed-off-by: Andrew Thornton --- models/auth/webauthn.go | 2 +- .../expected_webauthn_credential.yml | 5 +- .../u2f_registration.yml | 0 .../webauthn_credential.yml | 2 +- models/migrations/migrations.go | 8 +- models/migrations/v207.go | 77 +------- models/migrations/v208.go | 38 +--- models/migrations/v209.go | 133 +------------- models/migrations/v210.go | 172 ++++++++++++++++++ .../migrations/{v209_test.go => v210_test.go} | 4 +- 10 files changed, 190 insertions(+), 251 deletions(-) rename models/migrations/fixtures/{Test_increaseCredentialIDTo410 => Test_remigrateU2FCredentials}/expected_webauthn_credential.yml (51%) rename models/migrations/fixtures/{Test_increaseCredentialIDTo410 => Test_remigrateU2FCredentials}/u2f_registration.yml (100%) rename models/migrations/fixtures/{Test_increaseCredentialIDTo410 => Test_remigrateU2FCredentials}/webauthn_credential.yml (96%) create mode 100644 models/migrations/v210.go rename models/migrations/{v209_test.go => v210_test.go} (95%) diff --git a/models/auth/webauthn.go b/models/auth/webauthn.go index e6c446af94..2dc3043780 100644 --- a/models/auth/webauthn.go +++ b/models/auth/webauthn.go @@ -43,7 +43,7 @@ type WebAuthnCredential struct { Name string LowerName string `xorm:"unique(s)"` UserID int64 `xorm:"INDEX unique(s)"` - CredentialID string `xorm:"INDEX"` + CredentialID string `xorm:"INDEX VARCHAR(410)"` PublicKey []byte AttestationType string AAGUID []byte diff --git a/models/migrations/fixtures/Test_increaseCredentialIDTo410/expected_webauthn_credential.yml b/models/migrations/fixtures/Test_remigrateU2FCredentials/expected_webauthn_credential.yml similarity index 51% rename from models/migrations/fixtures/Test_increaseCredentialIDTo410/expected_webauthn_credential.yml rename to models/migrations/fixtures/Test_remigrateU2FCredentials/expected_webauthn_credential.yml index 36b011a9d3..0e68a5d012 100644 --- a/models/migrations/fixtures/Test_increaseCredentialIDTo410/expected_webauthn_credential.yml +++ b/models/migrations/fixtures/Test_remigrateU2FCredentials/expected_webauthn_credential.yml @@ -4,6 +4,9 @@ - id: 2 credential_id: "TVHE44TOH7DF7V48SEAIT3EMMJ7TGBOQ289E5AQB34S98LFCUFJ7U2NAVI8RJG6K2F4TC8AQ8KBNO7AGEOQOL9NE43GR63HTEHJSLOG=" +- + id: 3 + credential_id: "TVHE44TOH7DF7V48SEAIT3EMMJ7TGBOQ289E5AQB34S98LFCUFJ7U2NAVI8RJG6K2F4TC8AQ8KBNO7AGEOQOL9NE43GR63HTEHJSLOG=" - id: 4 - credential_id: "THIS SHOULD NOT CHAGNGE" + credential_id: "TVHE44TOH7DF7V48SEAIT3EMMJ7TGBOQ289E5AQB34S98LFCUFJ7U2NAVI8RJG6K2F4TC8AQ8KBNO7AGEOQOL9NE43GR63HTEHJSLOG=" diff --git a/models/migrations/fixtures/Test_increaseCredentialIDTo410/u2f_registration.yml b/models/migrations/fixtures/Test_remigrateU2FCredentials/u2f_registration.yml similarity index 100% rename from models/migrations/fixtures/Test_increaseCredentialIDTo410/u2f_registration.yml rename to models/migrations/fixtures/Test_remigrateU2FCredentials/u2f_registration.yml diff --git a/models/migrations/fixtures/Test_increaseCredentialIDTo410/webauthn_credential.yml b/models/migrations/fixtures/Test_remigrateU2FCredentials/webauthn_credential.yml similarity index 96% rename from models/migrations/fixtures/Test_increaseCredentialIDTo410/webauthn_credential.yml rename to models/migrations/fixtures/Test_remigrateU2FCredentials/webauthn_credential.yml index 0adf1bc8e2..7f9f10f89a 100644 --- a/models/migrations/fixtures/Test_increaseCredentialIDTo410/webauthn_credential.yml +++ b/models/migrations/fixtures/Test_remigrateU2FCredentials/webauthn_credential.yml @@ -23,7 +23,7 @@ lower_name: "u2fkey-wrong-user-id" name: "u2fkey-wrong-user-id" user_id: 1 - credential_id: "THIS SHOULD NOT CHAGNGE" + credential_id: "THIS SHOULD CHANGE" public_key: 0x040d0967a2cad045011631187576492a0beb5b377954b4f694c5afc8bdf25270f87f09a9ab6ce9c282f447ba71b2f2bae2105b32b847e0704f310f48644e3eddf2 attestation_type: 'fido-u2f' sign_count: 1 diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index bf0008f879..63d1c32259 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -367,11 +367,13 @@ var migrations = []Migration{ // v206 -> v207 NewMigration("Add authorize column to team_unit table", addAuthorizeColForTeamUnit), // v207 -> v208 - NewMigration("Add webauthn table and migrate u2f data to webauthn", addWebAuthnCred), + NewMigration("Add webauthn table and migrate u2f data to webauthn - NO-OPED", addWebAuthnCred), // v208 -> v209 - NewMigration("Use base32.HexEncoding instead of base64 encoding for cred ID as it is case insensitive", useBase32HexForCredIDInWebAuthnCredential), + NewMigration("Use base32.HexEncoding instead of base64 encoding for cred ID as it is case insensitive - NO-OPED", useBase32HexForCredIDInWebAuthnCredential), // v209 -> v210 - NewMigration("Increase WebAuthentication CredentialID size to 410", increaseCredentialIDTo410), + NewMigration("Increase WebAuthentication CredentialID size to 410 - NO-OPED", increaseCredentialIDTo410), + // v210 -> v211 + NewMigration("v208 was completely broken - remigrate", remigrateU2FCredentials), } // GetCurrentDBVersion returns the current db version diff --git a/models/migrations/v207.go b/models/migrations/v207.go index dca2902cd8..f60dfc3dc3 100644 --- a/models/migrations/v207.go +++ b/models/migrations/v207.go @@ -5,86 +5,11 @@ package migrations import ( - "crypto/elliptic" - "encoding/base64" - "strings" - - "code.gitea.io/gitea/modules/timeutil" - - "github.com/tstranex/u2f" "xorm.io/xorm" ) func addWebAuthnCred(x *xorm.Engine) error { - // Create webauthnCredential table - type webauthnCredential struct { - ID int64 `xorm:"pk autoincr"` - Name string - LowerName string `xorm:"unique(s)"` - UserID int64 `xorm:"INDEX unique(s)"` - CredentialID string `xorm:"INDEX VARCHAR(410)"` // CredentalID in U2F is at most 255bytes / 5 * 8 = 408 - add a few extra characters for safety - PublicKey []byte - AttestationType string - AAGUID []byte - SignCount uint32 `xorm:"BIGINT"` - CloneWarning bool - CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"` - UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"` - } - if err := x.Sync2(&webauthnCredential{}); err != nil { - return err - } - - // Now migrate the old u2f registrations to the new format - type u2fRegistration struct { - ID int64 `xorm:"pk autoincr"` - Name string - UserID int64 `xorm:"INDEX"` - Raw []byte - Counter uint32 `xorm:"BIGINT"` - CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"` - UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"` - } - - var start int - regs := make([]*u2fRegistration, 0, 50) - for { - err := x.OrderBy("id").Limit(50, start).Find(®s) - if err != nil { - return err - } - - for _, reg := range regs { - parsed := new(u2f.Registration) - err = parsed.UnmarshalBinary(reg.Raw) - if err != nil { - continue - } - - c := &webauthnCredential{ - ID: reg.ID, - Name: reg.Name, - LowerName: strings.ToLower(reg.Name), - UserID: reg.UserID, - CredentialID: base64.RawStdEncoding.EncodeToString(parsed.KeyHandle), - PublicKey: elliptic.Marshal(elliptic.P256(), parsed.PubKey.X, parsed.PubKey.Y), - AttestationType: "fido-u2f", - AAGUID: []byte{}, - SignCount: reg.Counter, - } - - _, err := x.Insert(c) - if err != nil { - return err - } - } - - if len(regs) < 50 { - break - } - start += 50 - regs = regs[:0] - } + // NO-OP Don't migrate here - let v210 do this. return nil } diff --git a/models/migrations/v208.go b/models/migrations/v208.go index 724b174187..2875406121 100644 --- a/models/migrations/v208.go +++ b/models/migrations/v208.go @@ -5,46 +5,10 @@ package migrations import ( - "encoding/base32" - "encoding/base64" - "xorm.io/xorm" ) func useBase32HexForCredIDInWebAuthnCredential(x *xorm.Engine) error { - // Create webauthnCredential table - type webauthnCredential struct { - ID int64 `xorm:"pk autoincr"` - CredentialID string `xorm:"INDEX VARCHAR(410)"` - } - if err := x.Sync2(&webauthnCredential{}); err != nil { - return err - } - - var start int - regs := make([]*webauthnCredential, 0, 50) - for { - err := x.OrderBy("id").Limit(50, start).Find(®s) - if err != nil { - return err - } - - for _, reg := range regs { - credID, _ := base64.RawStdEncoding.DecodeString(reg.CredentialID) - reg.CredentialID = base32.HexEncoding.EncodeToString(credID) - - _, err := x.Update(reg) - if err != nil { - return err - } - } - - if len(regs) < 50 { - break - } - start += 50 - regs = regs[:0] - } - + // noop return nil } diff --git a/models/migrations/v209.go b/models/migrations/v209.go index b4295d62fa..710684ef50 100644 --- a/models/migrations/v209.go +++ b/models/migrations/v209.go @@ -5,140 +5,13 @@ package migrations import ( - "encoding/base32" - "fmt" - "strings" - - "code.gitea.io/gitea/modules/timeutil" - - "github.com/tstranex/u2f" "xorm.io/xorm" - "xorm.io/xorm/schemas" ) func increaseCredentialIDTo410(x *xorm.Engine) error { - // Create webauthnCredential table - type webauthnCredential struct { - ID int64 `xorm:"pk autoincr"` - Name string - LowerName string `xorm:"unique(s)"` - UserID int64 `xorm:"INDEX unique(s)"` - CredentialID string `xorm:"INDEX VARCHAR(410)"` // CredentalID in U2F is at most 255bytes / 5 * 8 = 408 - add a few extra characters for safety - PublicKey []byte - AttestationType string - AAGUID []byte - SignCount uint32 `xorm:"BIGINT"` - CloneWarning bool - CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"` - UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"` - } - if err := x.Sync2(&webauthnCredential{}); err != nil { - return err - } - - switch x.Dialect().URI().DBType { - case schemas.MYSQL: - _, err := x.Exec("ALTER TABLE webauthn_credential MODIFY COLUMN credential_id VARCHAR(410)") - if err != nil { - return err - } - case schemas.ORACLE: - _, err := x.Exec("ALTER TABLE webauthn_credential MODIFY credential_id VARCHAR(410)") - if err != nil { - return err - } - case schemas.MSSQL: - // This column has an index on it. I could write all of the code to attempt to change the index OR - // I could just use recreate table. - sess := x.NewSession() - if err := sess.Begin(); err != nil { - _ = sess.Close() - return err - } - - if err := recreateTable(sess, new(webauthnCredential)); err != nil { - _ = sess.Close() - return err - } - if err := sess.Commit(); err != nil { - _ = sess.Close() - return err - } - if err := sess.Close(); err != nil { - return err - } - case schemas.POSTGRES: - _, err := x.Exec("ALTER TABLE webauthn_credential ALTER COLUMN credential_id TYPE VARCHAR(410)") - if err != nil { - return err - } - default: - // SQLite doesn't support ALTER COLUMN, and it already makes String _TEXT_ by default so no migration needed - // nor is there any need to re-migrate - return nil - } - - exist, err := x.IsTableExist("u2f_registration") - if err != nil { - return err - } - if !exist { - return nil - } - - // Now migrate the old u2f registrations to the new format - type u2fRegistration struct { - ID int64 `xorm:"pk autoincr"` - Name string - UserID int64 `xorm:"INDEX"` - Raw []byte - Counter uint32 `xorm:"BIGINT"` - CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"` - UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"` - } - - var start int - regs := make([]*u2fRegistration, 0, 50) - for { - err := x.OrderBy("id").Limit(50, start).Find(®s) - if err != nil { - return err - } - - for _, reg := range regs { - parsed := new(u2f.Registration) - err = parsed.UnmarshalBinary(reg.Raw) - if err != nil { - continue - } - - cred := &webauthnCredential{} - has, err := x.ID(reg.ID).Where("id = ? AND user_id = ?", reg.ID, reg.UserID).Get(cred) - if err != nil { - return fmt.Errorf("unable to get webauthn_credential[%d]. Error: %v", reg.ID, err) - } - if !has { - continue - } - remigratedCredID := base32.HexEncoding.EncodeToString(parsed.KeyHandle) - if cred.CredentialID == remigratedCredID || (!strings.HasPrefix(remigratedCredID, cred.CredentialID) && cred.CredentialID != "") { - continue - } - - cred.CredentialID = remigratedCredID - - _, err = x.ID(cred.ID).Update(cred) - if err != nil { - return err - } - } - - if len(regs) < 50 { - break - } - start += 50 - regs = regs[:0] - } + // no-op + // v208 was completely wrong + // So now we have to no-op again. return nil } diff --git a/models/migrations/v210.go b/models/migrations/v210.go new file mode 100644 index 0000000000..cf50760b92 --- /dev/null +++ b/models/migrations/v210.go @@ -0,0 +1,172 @@ +// Copyright 2022 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package migrations + +import ( + "crypto/elliptic" + "encoding/base32" + "fmt" + "strings" + + "code.gitea.io/gitea/modules/timeutil" + "github.com/tstranex/u2f" + + "xorm.io/xorm" + "xorm.io/xorm/schemas" +) + +// v208 migration was completely broken +func remigrateU2FCredentials(x *xorm.Engine) error { + // Create webauthnCredential table + type webauthnCredential struct { + ID int64 `xorm:"pk autoincr"` + Name string + LowerName string `xorm:"unique(s)"` + UserID int64 `xorm:"INDEX unique(s)"` + CredentialID string `xorm:"INDEX VARCHAR(410)"` // CredentalID in U2F is at most 255bytes / 5 * 8 = 408 - add a few extra characters for safety + PublicKey []byte + AttestationType string + AAGUID []byte + SignCount uint32 `xorm:"BIGINT"` + CloneWarning bool + CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"` + UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"` + } + if err := x.Sync2(&webauthnCredential{}); err != nil { + return err + } + + switch x.Dialect().URI().DBType { + case schemas.MYSQL: + _, err := x.Exec("ALTER TABLE webauthn_credential MODIFY COLUMN credential_id VARCHAR(410)") + if err != nil { + return err + } + case schemas.ORACLE: + _, err := x.Exec("ALTER TABLE webauthn_credential MODIFY credential_id VARCHAR(410)") + if err != nil { + return err + } + case schemas.MSSQL: + // This column has an index on it. I could write all of the code to attempt to change the index OR + // I could just use recreate table. + sess := x.NewSession() + if err := sess.Begin(); err != nil { + _ = sess.Close() + return err + } + + if err := recreateTable(sess, new(webauthnCredential)); err != nil { + _ = sess.Close() + return err + } + if err := sess.Commit(); err != nil { + _ = sess.Close() + return err + } + if err := sess.Close(); err != nil { + return err + } + case schemas.POSTGRES: + _, err := x.Exec("ALTER TABLE webauthn_credential ALTER COLUMN credential_id TYPE VARCHAR(410)") + if err != nil { + return err + } + default: + // SQLite doesn't support ALTER COLUMN, and it already makes String _TEXT_ by default so no migration needed + // nor is there any need to re-migrate + } + + exist, err := x.IsTableExist("u2f_registration") + if err != nil { + return err + } + if !exist { + return nil + } + + // Now migrate the old u2f registrations to the new format + type u2fRegistration struct { + ID int64 `xorm:"pk autoincr"` + Name string + UserID int64 `xorm:"INDEX"` + Raw []byte + Counter uint32 `xorm:"BIGINT"` + CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"` + UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"` + } + + var start int + regs := make([]*u2fRegistration, 0, 50) + for { + err := x.OrderBy("id").Limit(50, start).Find(®s) + if err != nil { + return err + } + + err = func() error { + sess := x.NewSession() + defer sess.Close() + if err := sess.Begin(); err != nil { + return fmt.Errorf("unable to allow start session. Error: %w", err) + } + if x.Dialect().URI().DBType == schemas.MSSQL { + if _, err := sess.Exec("SET IDENTITY_INSERT `webauthn_credential` ON"); err != nil { + return fmt.Errorf("unable to allow identity insert on webauthn_credential. Error: %w", err) + } + } + for _, reg := range regs { + parsed := new(u2f.Registration) + err = parsed.UnmarshalBinary(reg.Raw) + if err != nil { + continue + } + remigrated := &webauthnCredential{ + ID: reg.ID, + Name: reg.Name, + LowerName: strings.ToLower(reg.Name), + UserID: reg.UserID, + CredentialID: base32.HexEncoding.EncodeToString(parsed.KeyHandle), + PublicKey: elliptic.Marshal(elliptic.P256(), parsed.PubKey.X, parsed.PubKey.Y), + AttestationType: "fido-u2f", + AAGUID: []byte{}, + SignCount: reg.Counter, + UpdatedUnix: reg.UpdatedUnix, + CreatedUnix: reg.CreatedUnix, + } + + has, err := sess.ID(reg.ID).Where("id = ?", reg.ID).Get(new(webauthnCredential)) + if err != nil { + return fmt.Errorf("unable to get webauthn_credential[%d]. Error: %w", reg.ID, err) + } + if !has { + _, err = sess.Insert(remigrated) + if err != nil { + return fmt.Errorf("unable to (re)insert webauthn_credential[%d]. Error: %w", reg.ID, err) + } + + continue + } + + _, err = sess.ID(remigrated.ID).AllCols().Update(remigrated) + if err != nil { + return fmt.Errorf("unable to update webauthn_credential[%d]. Error: %w", reg.ID, err) + } + } + return sess.Commit() + }() + if err != nil { + return err + } + + if len(regs) < 50 { + break + } + start += 50 + regs = regs[:0] + } + + return nil +} diff --git a/models/migrations/v209_test.go b/models/migrations/v210_test.go similarity index 95% rename from models/migrations/v209_test.go rename to models/migrations/v210_test.go index a929f95adc..3e10b3ce80 100644 --- a/models/migrations/v209_test.go +++ b/models/migrations/v210_test.go @@ -12,7 +12,7 @@ import ( "xorm.io/xorm/schemas" ) -func Test_increaseCredentialIDTo410(t *testing.T) { +func Test_remigrateU2FCredentials(t *testing.T) { // Create webauthnCredential table type WebauthnCredential struct { ID int64 `xorm:"pk autoincr"` @@ -55,7 +55,7 @@ func Test_increaseCredentialIDTo410(t *testing.T) { } // Run the migration - if err := increaseCredentialIDTo410(x); err != nil { + if err := remigrateU2FCredentials(x); err != nil { assert.NoError(t, err) return }