]> source.dussan.org Git - gitea.git/commitdiff
WebAuthn CredentialID field needs to be increased in size (#20530)
authorzeripath <art27@cantab.net>
Sat, 30 Jul 2022 13:25:26 +0000 (14:25 +0100)
committerGitHub <noreply@github.com>
Sat, 30 Jul 2022 13:25:26 +0000 (15:25 +0200)
WebAuthn have updated their specification to set the maximum size of the
CredentialID to 1023 bytes. This is somewhat larger than our current
size and therefore we need to migrate.

The PR changes the struct to add CredentialIDBytes and migrates the CredentialID string
to the bytes field before another migration drops the old CredentialID field. Another migration
renames this field back.

Fix #20457

Signed-off-by: Andrew Thornton <art27@cantab.net>
models/auth/webauthn.go
models/auth/webauthn_test.go
models/migrations/fixtures/Test_storeWebauthnCredentialIDAsBytes/expected_webauthn_credential.yml [new file with mode: 0644]
models/migrations/fixtures/Test_storeWebauthnCredentialIDAsBytes/webauthn_credential.yml [new file with mode: 0644]
models/migrations/migrations.go
models/migrations/v221.go [new file with mode: 0644]
models/migrations/v221_test.go [new file with mode: 0644]
models/migrations/v222.go [new file with mode: 0644]
models/migrations/v223.go [new file with mode: 0644]
routers/web/auth/webauthn.go

index 2dc3043780101593c23618fbf8d26ae482c3ee9d..d3062342f545b387e553f1278273f27bf2ac3815 100644 (file)
@@ -6,7 +6,6 @@ package auth
 
 import (
        "context"
-       "encoding/base32"
        "fmt"
        "strings"
 
@@ -20,14 +19,14 @@ import (
 // ErrWebAuthnCredentialNotExist represents a "ErrWebAuthnCRedentialNotExist" kind of error.
 type ErrWebAuthnCredentialNotExist struct {
        ID           int64
-       CredentialID string
+       CredentialID []byte
 }
 
 func (err ErrWebAuthnCredentialNotExist) Error() string {
-       if err.CredentialID == "" {
+       if len(err.CredentialID) == 0 {
                return fmt.Sprintf("WebAuthn credential does not exist [id: %d]", err.ID)
        }
-       return fmt.Sprintf("WebAuthn credential does not exist [credential_id: %s]", err.CredentialID)
+       return fmt.Sprintf("WebAuthn credential does not exist [credential_id: %x]", err.CredentialID)
 }
 
 // IsErrWebAuthnCredentialNotExist checks if an error is a ErrWebAuthnCredentialNotExist.
@@ -43,7 +42,7 @@ type WebAuthnCredential struct {
        Name            string
        LowerName       string `xorm:"unique(s)"`
        UserID          int64  `xorm:"INDEX unique(s)"`
-       CredentialID    string `xorm:"INDEX VARCHAR(410)"`
+       CredentialID    []byte `xorm:"INDEX VARBINARY(1024)"`
        PublicKey       []byte
        AttestationType string
        AAGUID          []byte
@@ -94,9 +93,8 @@ type WebAuthnCredentialList []*WebAuthnCredential
 func (list WebAuthnCredentialList) ToCredentials() []webauthn.Credential {
        creds := make([]webauthn.Credential, 0, len(list))
        for _, cred := range list {
-               credID, _ := base32.HexEncoding.DecodeString(cred.CredentialID)
                creds = append(creds, webauthn.Credential{
-                       ID:              credID,
+                       ID:              cred.CredentialID,
                        PublicKey:       cred.PublicKey,
                        AttestationType: cred.AttestationType,
                        Authenticator: webauthn.Authenticator{
@@ -164,11 +162,11 @@ func HasWebAuthnRegistrationsByUID(uid int64) (bool, error) {
 }
 
 // GetWebAuthnCredentialByCredID returns WebAuthn credential by credential ID
-func GetWebAuthnCredentialByCredID(userID int64, credID string) (*WebAuthnCredential, error) {
+func GetWebAuthnCredentialByCredID(userID int64, credID []byte) (*WebAuthnCredential, error) {
        return getWebAuthnCredentialByCredID(db.DefaultContext, userID, credID)
 }
 
-func getWebAuthnCredentialByCredID(ctx context.Context, userID int64, credID string) (*WebAuthnCredential, error) {
+func getWebAuthnCredentialByCredID(ctx context.Context, userID int64, credID []byte) (*WebAuthnCredential, error) {
        cred := new(WebAuthnCredential)
        if found, err := db.GetEngine(ctx).Where("user_id = ? AND credential_id = ?", userID, credID).Get(cred); err != nil {
                return nil, err
@@ -187,7 +185,7 @@ func createCredential(ctx context.Context, userID int64, name string, cred *weba
        c := &WebAuthnCredential{
                UserID:          userID,
                Name:            name,
-               CredentialID:    base32.HexEncoding.EncodeToString(cred.ID),
+               CredentialID:    cred.ID,
                PublicKey:       cred.PublicKey,
                AttestationType: cred.AttestationType,
                AAGUID:          cred.Authenticator.AAGUID,
index 216bf110806ebbafd3c90da3a4fa4db9bd3d80c4..cc39691ce2d367646b22807aaaa992a59ec8891c 100644 (file)
@@ -5,7 +5,6 @@
 package auth
 
 import (
-       "encoding/base32"
        "testing"
 
        "code.gitea.io/gitea/models/unittest"
@@ -61,9 +60,7 @@ func TestCreateCredential(t *testing.T) {
        res, err := CreateCredential(1, "WebAuthn Created Credential", &webauthn.Credential{ID: []byte("Test")})
        assert.NoError(t, err)
        assert.Equal(t, "WebAuthn Created Credential", res.Name)
-       bs, err := base32.HexEncoding.DecodeString(res.CredentialID)
-       assert.NoError(t, err)
-       assert.Equal(t, []byte("Test"), bs)
+       assert.Equal(t, []byte("Test"), res.CredentialID)
 
        unittest.AssertExistsIf(t, true, &WebAuthnCredential{Name: "WebAuthn Created Credential", UserID: 1})
 }
diff --git a/models/migrations/fixtures/Test_storeWebauthnCredentialIDAsBytes/expected_webauthn_credential.yml b/models/migrations/fixtures/Test_storeWebauthnCredentialIDAsBytes/expected_webauthn_credential.yml
new file mode 100644 (file)
index 0000000..55a237a
--- /dev/null
@@ -0,0 +1,9 @@
+-
+  id: 1
+  credential_id: "TVHE44TOH7DF7V48SEAIT3EMMJ7TGBOQ289E5AQB34S98LFCUFJ7U2NAVI8RJG6K2F4TC8AQ8KBNO7AGEOQOL9NE43GR63HTEHJSLOG="
+-
+  id: 2
+  credential_id: "051CLMMKB62S6M9M2A4H54K7MMCQALFJ36G4TGB2S9A47APLTILU6C6744CEBG4EKCGV357N21BSLH8JD33GQMFAR6DQ70S76P34J6FR="
+-
+  id: 4
+  credential_id: "APU4B1NDTEVTEM60V4T0FRL7SRJMO9KIE2AKFQ8JDGTQ7VHFI41FDEFTDLBVQEAE4ER49QV2GTGVFDNBO31BPOA3OQN6879OT6MTU3G="
diff --git a/models/migrations/fixtures/Test_storeWebauthnCredentialIDAsBytes/webauthn_credential.yml b/models/migrations/fixtures/Test_storeWebauthnCredentialIDAsBytes/webauthn_credential.yml
new file mode 100644 (file)
index 0000000..c02a76e
--- /dev/null
@@ -0,0 +1,31 @@
+-
+  id: 1
+  lower_name: "u2fkey-correctly-migrated"
+  name: "u2fkey-correctly-migrated"
+  user_id: 1
+  credential_id: "TVHE44TOH7DF7V48SEAIT3EMMJ7TGBOQ289E5AQB34S98LFCUFJ7U2NAVI8RJG6K2F4TC8AQ8KBNO7AGEOQOL9NE43GR63HTEHJSLOG="
+  public_key: 0x040d0967a2cad045011631187576492a0beb5b377954b4f694c5afc8bdf25270f87f09a9ab6ce9c282f447ba71b2f2bae2105b32b847e0704f310f48644e3eddf2
+  attestation_type: 'fido-u2f'
+  sign_count: 1
+  clone_warning: false
+-
+  id: 2
+  lower_name: "non-u2f-key"
+  name: "non-u2f-key"
+  user_id: 1
+  credential_id: "051CLMMKB62S6M9M2A4H54K7MMCQALFJ36G4TGB2S9A47APLTILU6C6744CEBG4EKCGV357N21BSLH8JD33GQMFAR6DQ70S76P34J6FR"
+  public_key: 0x040d0967a2cad045011631187576492a0beb5b377954b4f694c5afc8bdf25270f87f09a9ab6ce9c282f447ba71b2f2bae2105b32b847e0704f310f48644e3eddf2
+  attestation_type: 'none'
+  sign_count: 1
+  clone_warning: false
+-
+  id: 4
+  lower_name: "packed-key"
+  name: "packed-key"
+  user_id: 1
+  credential_id: "APU4B1NDTEVTEM60V4T0FRL7SRJMO9KIE2AKFQ8JDGTQ7VHFI41FDEFTDLBVQEAE4ER49QV2GTGVFDNBO31BPOA3OQN6879OT6MTU3G="
+  public_key: 0x040d0967a2cad045011631187576492a0beb5b377954b4f694c5afc8bdf25270f87f09a9ab6ce9c282f447ba71b2f2bae2105b32b847e0704f310f48644e3eddf2
+  attestation_type: 'fido-u2f'
+  sign_count: 1
+  clone_warning: false
+
index beeba866dc70f817f3ca1a0625223e04464fe335..2719f45efbbbcb70a02b5b6d348f81e963f71d9e 100644 (file)
@@ -400,6 +400,12 @@ var migrations = []Migration{
        NewMigration("Add sync_on_commit column to push_mirror table", addSyncOnCommitColForPushMirror),
        // v220 -> v221
        NewMigration("Add container repository property", addContainerRepositoryProperty),
+       // v221 -> v222
+       NewMigration("Store WebAuthentication CredentialID as bytes and increase size to at least 1024", storeWebauthnCredentialIDAsBytes),
+       // v222 -> v223
+       NewMigration("Drop old CredentialID column", dropOldCredentialIDColumn),
+       // v223 -> v224
+       NewMigration("Rename CredentialIDBytes column to CredentialID", renameCredentialIDBytes),
 }
 
 // GetCurrentDBVersion returns the current db version
diff --git a/models/migrations/v221.go b/models/migrations/v221.go
new file mode 100644 (file)
index 0000000..f3bcfcd
--- /dev/null
@@ -0,0 +1,75 @@
+// 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"
+
+       "code.gitea.io/gitea/modules/timeutil"
+
+       "xorm.io/xorm"
+)
+
+func storeWebauthnCredentialIDAsBytes(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)"`
+               // Note the lack of INDEX here - these will be created once the column is renamed in v223.go
+               CredentialIDBytes []byte `xorm:"VARBINARY(1024)"` // CredentialID is at most 1023 bytes as per spec released 20 July 2022
+               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
+       }
+
+       var start int
+       creds := make([]*webauthnCredential, 0, 50)
+       for {
+               err := x.Select("id, credential_id").OrderBy("id").Limit(50, start).Find(&creds)
+               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)
+                       }
+                       for _, cred := range creds {
+                               cred.CredentialIDBytes, err = base32.HexEncoding.DecodeString(cred.CredentialID)
+                               if err != nil {
+                                       return fmt.Errorf("unable to parse credential id %s for credential[%d]: %w", cred.CredentialID, cred.ID, err)
+                               }
+                               count, err := sess.ID(cred.ID).Cols("credential_id_bytes").Update(cred)
+                               if count != 1 || err != nil {
+                                       return fmt.Errorf("unable to update credential id bytes for credential[%d]: %d,%w", cred.ID, count, err)
+                               }
+                       }
+                       return sess.Commit()
+               }()
+               if err != nil {
+                       return err
+               }
+
+               if len(creds) < 50 {
+                       break
+               }
+               start += 50
+               creds = creds[:0]
+       }
+       return nil
+}
diff --git a/models/migrations/v221_test.go b/models/migrations/v221_test.go
new file mode 100644 (file)
index 0000000..c50ca5c
--- /dev/null
@@ -0,0 +1,65 @@
+// 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"
+       "testing"
+
+       "github.com/stretchr/testify/assert"
+)
+
+func Test_storeWebauthnCredentialIDAsBytes(t *testing.T) {
+       // 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)"`
+               PublicKey       []byte
+               AttestationType string
+               AAGUID          []byte
+               SignCount       uint32 `xorm:"BIGINT"`
+               CloneWarning    bool
+       }
+
+       type ExpectedWebauthnCredential struct {
+               ID           int64  `xorm:"pk autoincr"`
+               CredentialID string // CredentialID is at most 1023 bytes as per spec released 20 July 2022
+       }
+
+       type ConvertedWebauthnCredential struct {
+               ID                int64  `xorm:"pk autoincr"`
+               CredentialIDBytes []byte `xorm:"VARBINARY(1024)"` // CredentialID is at most 1023 bytes as per spec released 20 July 2022
+       }
+
+       // Prepare and load the testing database
+       x, deferable := prepareTestEnv(t, 0, new(WebauthnCredential), new(ExpectedWebauthnCredential))
+       defer deferable()
+       if x == nil || t.Failed() {
+               return
+       }
+
+       if err := storeWebauthnCredentialIDAsBytes(x); err != nil {
+               assert.NoError(t, err)
+               return
+       }
+
+       expected := []ExpectedWebauthnCredential{}
+       if err := x.Table("expected_webauthn_credential").Asc("id").Find(&expected); !assert.NoError(t, err) {
+               return
+       }
+
+       got := []ConvertedWebauthnCredential{}
+       if err := x.Table("webauthn_credential").Select("id, credential_id_bytes").Asc("id").Find(&got); !assert.NoError(t, err) {
+               return
+       }
+
+       for i, e := range expected {
+               credIDBytes, _ := base32.HexEncoding.DecodeString(e.CredentialID)
+               assert.Equal(t, credIDBytes, got[i].CredentialIDBytes)
+       }
+}
diff --git a/models/migrations/v222.go b/models/migrations/v222.go
new file mode 100644 (file)
index 0000000..99acdfd
--- /dev/null
@@ -0,0 +1,64 @@
+// 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 (
+       "context"
+       "fmt"
+
+       "code.gitea.io/gitea/modules/timeutil"
+
+       "xorm.io/xorm"
+)
+
+func dropOldCredentialIDColumn(x *xorm.Engine) error {
+       // This migration maybe rerun so that we should check if it has been run
+       credentialIDExist, err := x.Dialect().IsColumnExist(x.DB(), context.Background(), "webauthn_credential", "credential_id")
+       if err != nil {
+               return err
+       }
+       if !credentialIDExist {
+               // Column is already non-extant
+               return nil
+       }
+       credentialIDBytesExists, err := x.Dialect().IsColumnExist(x.DB(), context.Background(), "webauthn_credential", "credential_id_bytes")
+       if err != nil {
+               return err
+       }
+       if !credentialIDBytesExists {
+               // looks like 221 hasn't properly run
+               return fmt.Errorf("webauthn_credential does not have a credential_id_bytes column... it is not safe to run this migration")
+       }
+
+       // 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)"`
+               // Note the lack of the INDEX on CredentialIDBytes - we will add this in v223.go
+               CredentialIDBytes []byte `xorm:"VARBINARY(1024)"` // CredentialID is at most 1023 bytes as per spec released 20 July 2022
+               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
+       }
+
+       // Drop the old credential ID
+       sess := x.NewSession()
+       defer sess.Close()
+
+       if err := dropTableColumns(sess, "webauthn_credential", "credential_id"); err != nil {
+               return fmt.Errorf("unable to drop old credentialID column: %w", err)
+       }
+       return sess.Commit()
+}
diff --git a/models/migrations/v223.go b/models/migrations/v223.go
new file mode 100644 (file)
index 0000000..d7ee481
--- /dev/null
@@ -0,0 +1,103 @@
+// 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 (
+       "context"
+       "fmt"
+
+       "code.gitea.io/gitea/modules/setting"
+       "code.gitea.io/gitea/modules/timeutil"
+
+       "xorm.io/xorm"
+)
+
+func renameCredentialIDBytes(x *xorm.Engine) error {
+       // This migration maybe rerun so that we should check if it has been run
+       credentialIDExist, err := x.Dialect().IsColumnExist(x.DB(), context.Background(), "webauthn_credential", "credential_id")
+       if err != nil {
+               return err
+       }
+       if credentialIDExist {
+               credentialIDBytesExists, err := x.Dialect().IsColumnExist(x.DB(), context.Background(), "webauthn_credential", "credential_id_bytes")
+               if err != nil {
+                       return err
+               }
+               if !credentialIDBytesExists {
+                       return nil
+               }
+       }
+
+       err = func() error {
+               // webauthnCredential table
+               type webauthnCredential struct {
+                       ID        int64 `xorm:"pk autoincr"`
+                       Name      string
+                       LowerName string `xorm:"unique(s)"`
+                       UserID    int64  `xorm:"INDEX unique(s)"`
+                       // Note the lack of INDEX here
+                       CredentialIDBytes []byte `xorm:"VARBINARY(1024)"` // CredentialID is at most 1023 bytes as per spec released 20 July 2022
+                       PublicKey         []byte
+                       AttestationType   string
+                       AAGUID            []byte
+                       SignCount         uint32 `xorm:"BIGINT"`
+                       CloneWarning      bool
+                       CreatedUnix       timeutil.TimeStamp `xorm:"INDEX created"`
+                       UpdatedUnix       timeutil.TimeStamp `xorm:"INDEX updated"`
+               }
+               sess := x.NewSession()
+               defer sess.Close()
+               if err := sess.Begin(); err != nil {
+                       return err
+               }
+
+               if err := sess.Sync2(new(webauthnCredential)); err != nil {
+                       return fmt.Errorf("error on Sync2: %v", err)
+               }
+
+               if credentialIDExist {
+                       // if both errors and message exist, drop message at first
+                       if err := dropTableColumns(sess, "webauthn_credential", "credential_id"); err != nil {
+                               return err
+                       }
+               }
+
+               switch {
+               case setting.Database.UseMySQL:
+                       if _, err := sess.Exec("ALTER TABLE `webauthn_credential` CHANGE credential_id_bytes credential_id VARBINARY(1024)"); err != nil {
+                               return err
+                       }
+               case setting.Database.UseMSSQL:
+                       if _, err := sess.Exec("sp_rename 'webauthn_credential.credential_id_bytes', 'credential_id', 'COLUMN'"); err != nil {
+                               return err
+                       }
+               default:
+                       if _, err := sess.Exec("ALTER TABLE `webauthn_credential` RENAME COLUMN credential_id_bytes TO credential_id"); err != nil {
+                               return err
+                       }
+               }
+               return sess.Commit()
+       }()
+       if err != nil {
+               return err
+       }
+
+       // 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    []byte `xorm:"INDEX VARBINARY(1024)"` // CredentialID is at most 1023 bytes as per spec released 20 July 2022
+               PublicKey       []byte
+               AttestationType string
+               AAGUID          []byte
+               SignCount       uint32 `xorm:"BIGINT"`
+               CloneWarning    bool
+               CreatedUnix     timeutil.TimeStamp `xorm:"INDEX created"`
+               UpdatedUnix     timeutil.TimeStamp `xorm:"INDEX updated"`
+       }
+       return x.Sync2(&webauthnCredential{})
+}
index 4778c9a9a3690f9125284ce51578ed55ac3b719b..917cbdd57bf67e1c88cefdb2c6650b4a4dd7ba0c 100644 (file)
@@ -5,7 +5,6 @@
 package auth
 
 import (
-       "encoding/base32"
        "errors"
        "net/http"
 
@@ -129,7 +128,7 @@ func WebAuthnLoginAssertionPost(ctx *context.Context) {
        }
 
        // Success! Get the credential and update the sign count with the new value we received.
-       dbCred, err := auth.GetWebAuthnCredentialByCredID(user.ID, base32.HexEncoding.EncodeToString(cred.ID))
+       dbCred, err := auth.GetWebAuthnCredentialByCredID(user.ID, cred.ID)
        if err != nil {
                ctx.ServerError("GetWebAuthnCredentialByCredID", err)
                return