Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increase the size of the webauthn_credential credential_id field #18739

Merged
merged 14 commits into from
Feb 13, 2022
2 changes: 2 additions & 0 deletions models/migrations/migrations.go
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,8 @@ var migrations = []Migration{
NewMigration("Add webauthn table and migrate u2f data to webauthn", addWebAuthnCred),
// v208 -> v209
NewMigration("Use base32.HexEncoding instead of base64 encoding for cred ID as it is case insensitive", useBase32HexForCredIDInWebAuthnCredential),
// v209 -> v210
NewMigration("Increase WebAuthentication CredentialID size to 410", increaseCredentialIDTo410),
}

// GetCurrentDBVersion returns the current db version
Expand Down
2 changes: 1 addition & 1 deletion models/migrations/v207.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ func addWebAuthnCred(x *xorm.Engine) error {
Name string
LowerName string `xorm:"unique(s)"`
UserID int64 `xorm:"INDEX unique(s)"`
CredentialID string `xorm:"INDEX"`
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
Expand Down
2 changes: 1 addition & 1 deletion models/migrations/v208.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ func useBase32HexForCredIDInWebAuthnCredential(x *xorm.Engine) error {
// Create webauthnCredential table
type webauthnCredential struct {
ID int64 `xorm:"pk autoincr"`
CredentialID string `xorm:"INDEX"`
CredentialID string `xorm:"INDEX VARCHAR(410)"`
}
if err := x.Sync2(&webauthnCredential{}); err != nil {
return err
Expand Down
118 changes: 118 additions & 0 deletions models/migrations/v209.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
// 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 (
"encoding/base32"
"fmt"
"strings"

"code.gitea.io/gitea/modules/timeutil"
"github.com/tstranex/u2f"
zeripath marked this conversation as resolved.
Show resolved Hide resolved
"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
}

if x.Dialect().URI().DBType == schemas.SQLITE {
// SQLite doesn't support ALTER COLUMN, and it seem to already makes String _TEXT_ by default so no migration needed
// nor is there any need to re-migrate
return nil
}

// 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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just modify the column for non-sqlite database?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

	// 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly it's a nightmare handling all of the hoops which the various databases would require one to jump through to handle the index.

Certainly MSSQL won't allow you adjust a column that has a contraint on it already - that would need to be dropped first but I'm not sure about when you're allowed to readd the constraint.

Then MYSQL - which I think will allow you to change the column if it's empty - but if it contains data I have a feeling it would need to drop the constraint. Then IIRC if I would try to readd the constraint that won't work.

I think Postgres is fine simply because of cascade.

_ = sess.Close()
return err
}
if err := sess.Commit(); err != nil {
_ = sess.Close()
return err
}
if err := sess.Close(); 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(&regs)
if err != nil {
return err
}

for _, reg := range regs {
parsed := new(u2f.Registration)
err = parsed.UnmarshalBinary(reg.Raw)
if err != nil {
continue
}

var 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]
}

return nil
}