Increase the size of the webauthn_credential credential_id field (#18739) #18756
Conversation
…gitea#18739) Backport go-gitea#18739 Unfortunately credentialIDs in u2f are 255 bytes long which with base32 encoding becomes 408 bytes. The default size of a xorm string field is only a VARCHAR(255) This problem is not apparent on SQLite because strings get mapped to TEXT there. Fix go-gitea#18727 Signed-off-by: Andrew Thornton <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
GiteaBot
added
the
lgtm/need 2
This comment was marked as outdated.
This comment was marked as outdated.
|
Contains backport for #18760 |
GiteaBot
added
lgtm/need 1
GiteaBot
added
lgtm/done
|
make lgtm work |
|
Still does not work I guess it is now base32 and 410 bytes are not enough again... |
|
Sorry, I tested probably while you pressed the merge button |
|
The U2F documentation says that the keyHandle should be at most 255 bytes. https://neowave.fr/pdfs/FIDO-U2F-CHEAT-SHEET.pdf
Base32 represents a 5 bits as 1 byte. 255 * 8 / 5 = 408. a) ensure that your table is varchar(410) and that you're not rerunning on a previously partially migrated db |
|
You are right 410 should be enough when converting 255 bytes into base32, sorry. But. a) I did this on a clean gitea 1.15 database, no partially migrated db b) The migration is broken and truncates data, if gitea depends on truncation (relaxed sql mode that allows it), this should be documented. But I think this is dangerous and should be handled at a layer above the database system itself. In the end I have to look at the code and see what is being migrated exactly - and what exceeds 255 bytes so that converting to base32 exceeds 410 bytes. |
|
In case you're not aware I am trying to change this so it doesn't truncate data. |
|
changing v208 to // Copyright 2021 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"
"encoding/base64"
"fmt"
"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
maxLength := 0
maxLengthID := int64(0)
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)
if maxLength < len(reg.CredentialID) {
maxLength = len(reg.CredentialID)
maxLengthID = reg.ID
}
}
if len(regs) < 50 {
break
}
start += 50
regs = regs[:0]
}
fmt.Printf("MaxLength: %d on ID: %d\n", maxLength, maxLengthID)
return fmt.Errorf("MaxLength: %d on ID: %d\n", maxLength, maxLengthID)
} |
|
I am not sure if this helps, but it seems that the migration happens from the raw blob column. Those have varying sizes from 472 to 1234 bytes. (I typed this before your last comment, this is not meant to be a reply) |
So v207 works (base64) but migrating to base32 fails. I double checked that the column is varchar(410) |
|
the maxlength there is 408 chars not 410 how can 410 not be long enough?! |
|
I bet the table is actually being sync'd at 255 because I forgot to update models/auth/webauthn.go too. |
|
Could you update models/auth/webauthn.go:46 to: CredentialID string `xorm:"INDEX VARCHAR(410)"` |
|
(and restore v208.go) |
|
I can try but this looked correct to me: |
|
I just don't understand why else the db would be moaning about a string being too large when it's smaller than the size of the column. I mean 408 is clearly smaller than 410 (#18756 (comment)) |
|
Me neither, ran a test but still |
|
OK I guess we just try increasing the size of that column until MySQL doesn't moan anymore. Shall we say VARCHAR(500)? You should just be able to update the table to VARCHAR(500) and rerun the migration - (v209 will then likely fail but meh we'll need yet another PR) |
|
I can try after sleeping, I still find it weird that it fails. |
I enabled the Gitea SQL log. Look at the string I repalced with |
MariaDB now asserts that 408 characters is too long for a VARCHAR(410) I have no words. See go-gitea#18756 Signed-off-by: Andrew Thornton <[email protected]>
MariaDB now asserts that 408 characters is too long for a VARCHAR(410) I have no words. See go-gitea#18756 Signed-off-by: Andrew Thornton <[email protected]>
|
I've put up a PR to make it 500. Hopefully that will fix this. Please try that and report if that works. |
|
@zeripath I was trying to understand this migration, but my brain is failing at two points. First of all, I notice that the SQL statement is just "UPDATE" without a where. This means, each record is always being updated, and all credential_id's are the very same in the end. Is this expected? I don't know much about webauthn, but it sounds off that the id is not unique? Secondly, for some reason the size of the records grows with each batch of fifty. I kinda suppose it's re-encoding the data over and over again, that is also likely the reason for the failing size check. (Because of the former which updates all records?) I'll check with varchar 500, but if the code behaves the way it appears to my tired eyes, this will just shift the failing to later. |
|
... indeed: with varchar(500) it now tried inserting a string with 505 characters after a chunk of 50 credentials more. |
|
Bloody hell you're right v208 is totally broken. |
|
without knowing XORM, I suppose it should be - _, err := x.Update(reg)
+ _, err := x.ID(reg.ID).Update(reg)or something like this? |
|
I think the safest thing to do is to simply remigrate all of u2f keys |
The warning is also wired. |
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 <[email protected]>
Backport #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 <[email protected]>
v208.go is seriously broken as it misses an ID() check. We need to no-op and remigrate all of the u2f keys. See go-gitea#18756 Signed-off-by: Andrew Thornton <[email protected]>
Backport #18739
Backport #18760
Unfortunately credentialIDs in u2f are 255 bytes long which with base32 encoding
becomes 408 bytes. The default size of a xorm string field is only a VARCHAR(255)
This problem is not apparent on SQLite because strings get mapped to TEXT there.
Fix #18727
Signed-off-by: Andrew Thornton [email protected]