Increase the size of the webauthn_credential credential_id field #18739

zeripath · 2022-02-12T07:37:43Z

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 art27@cantab.net

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 <art27@cantab.net>

zeripath · 2022-02-12T07:39:28Z

I don't know if we should remigrate the U2F keys or not.

Signed-off-by: Andrew Thornton <art27@cantab.net>

fnetX · 2022-02-12T09:55:48Z

remigrate the U2F

What do you mean? Is the original U2F data still present (like redo the migration) or do you mean doing a fix for first-time migrations?

If the first is possible, this might fix some broken keys. Maybe check that the key wasn't removed in the meantime if this is possible?

zeripath · 2022-02-12T10:05:32Z

the original table is still there - because I worried that this might happen.

So we would remigrate the data - but - actually users might have removed those old keys already.

OK I've tried to do that.

Signed-off-by: Andrew Thornton <art27@cantab.net>

fnetX · 2022-02-12T10:58:39Z

actually users might have removed those old keys already.

That's what I meant by

Maybe check that the key wasn't removed in the meantime

But it should be possible to check the webauthn table for fields that are using the maximum length and then try to remigrate that data, right?

Edit: That doesn't make much sense, it's probably better to skip the old migration and (re-)migrate in any case. It would be nonsense to migrate twice - once broken once correctly - and my approach would rely on the data to be already migrated.

Still, checking if the table is already present and not adding new records could be a solution-

…fix of the credID and use recreateTable Signed-off-by: Andrew Thornton <art27@cantab.net>

lunny · 2022-02-12T12:27:46Z

Details

Ah. Checking the records of the webauthn and u2f and decide if the migration is necessary.

zeripath · 2022-02-12T16:31:33Z

@fnetX are you able to test this to see if it works for your data?

lunny · 2022-02-13T12:49:38Z

make L-G-T-M work.

models/migrations/v209.go

lunny · 2022-02-13T16:31:42Z

models/migrations/v209.go

+		_ = sess.Close()
+		return err
+	}
+	if err := recreateTable(sess, new(webauthnCredential)); err != nil {


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

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

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.

Block problem resolved.

Signed-off-by: Andrew Thornton <art27@cantab.net>

zeripath · 2022-02-13T21:09:48Z

OK I've written a test and this appears to be working as intended - so I think we can merge.

…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 <art27@cantab.net>

lunny · 2022-02-14T02:11:57Z

models/migrations/v209.go

+		if err != nil {
+			return err
+		}
+	case schemas.ORACLE:


Wow, Gitea hasn't support ORACLE yet.

) (#18756) * Increase the size of the webauthn_credential credential_id field (#18739) Backport #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 #18727 Signed-off-by: Andrew Thornton <art27@cantab.net> * Ignore the migrate if u2f_registration is not exist (#18760) Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>

* giteaofficial/main: [skip ci] Updated translations via Crowdin Prevent dangling GetAttribute calls (go-gitea#18754) Add example to render html files (go-gitea#18736) Fix a broken link in `commits_list_small.tmpl` (go-gitea#18763) Fix broken cancel button link on patch page (go-gitea#18718) Ignore the migrate if u2f_registration is not exist (go-gitea#18760) [skip ci] Updated translations via Crowdin Increase the size of the webauthn_credential credential_id field (go-gitea#18739) Fix isempty detection of git repository (go-gitea#18746)

* 'main' of https://github.com/go-gitea/gitea: (87 commits) Fix template bug of LFS lock (go-gitea#18784) Various Mermaid improvements (go-gitea#18776) [skip ci] Updated translations via Crowdin Fix display time of milestones (go-gitea#18753) [skip ci] Updated translations via Crowdin Prevent dangling GetAttribute calls (go-gitea#18754) Add example to render html files (go-gitea#18736) Fix a broken link in `commits_list_small.tmpl` (go-gitea#18763) Fix broken cancel button link on patch page (go-gitea#18718) Ignore the migrate if u2f_registration is not exist (go-gitea#18760) [skip ci] Updated translations via Crowdin Increase the size of the webauthn_credential credential_id field (go-gitea#18739) Fix isempty detection of git repository (go-gitea#18746) [skip ci] Updated translations via Crowdin Send mail to issue/pr assignee/reviewer also when OnMention is set (go-gitea#18707) Reduce CI go module downloads, add make targets (go-gitea#18708) Add number in queue status to monitor page (go-gitea#18712) Fix source code line highlighting (go-gitea#18729) Fix forked repositories missed tags (go-gitea#18719) [skip ci] Updated translations via Crowdin ...

…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 <art27@cantab.net>

zeripath added type/bug backport/v1.16 labels Feb 12, 2022

zeripath added this to the 1.17.0 milestone Feb 12, 2022

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 12, 2022

and add to the migration list...

d82a0fa

Signed-off-by: Andrew Thornton <art27@cantab.net>

6543 approved these changes Feb 12, 2022

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 12, 2022

Merge branch 'main' into fix-18727-longer-id

1bb1cba

lunny approved these changes Feb 12, 2022

View reviewed changes

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 12, 2022

zeripath added 2 commits February 12, 2022 10:21

Remigrate and fix test

e5d049d

Signed-off-by: Andrew Thornton <art27@cantab.net>

mssql is annoying

a2c6a27

Signed-off-by: Andrew Thornton <art27@cantab.net>

restrict migration to only keys that are blank credID or are some pre…

4a12a30

…fix of the credID and use recreateTable Signed-off-by: Andrew Thornton <art27@cantab.net>

Merge branch 'main' into fix-18727-longer-id

862d0a7

lunny previously requested changes Feb 13, 2022

View reviewed changes

models/migrations/v209.go Outdated Show resolved Hide resolved

Update models/migrations/v209.go

66db70b

lunny reviewed Feb 13, 2022

View reviewed changes

models/migrations/v209.go Show resolved Hide resolved

lunny reviewed Feb 13, 2022

View reviewed changes

Update models/migrations/v209.go

b9237bd

lunny and others added 5 commits February 14, 2022 01:12

Merge branch 'main' into fix-18727-longer-id

fb1ba41

Move back to alter statements

3dff1ae

Signed-off-by: Andrew Thornton <art27@cantab.net>

Add test

040de03

Signed-off-by: Andrew Thornton <art27@cantab.net>

skip sqlite

ba6f87b

Signed-off-by: Andrew Thornton <art27@cantab.net>

fix postgres test

5ca70f7

Signed-off-by: Andrew Thornton <art27@cantab.net>

zeripath merged commit 32599bf into go-gitea:main Feb 13, 2022

zeripath deleted the fix-18727-longer-id branch February 13, 2022 21:19

zeripath mentioned this pull request Feb 13, 2022

Increase the size of the webauthn_credential credential_id field (#18739) #18756

Merged

zeripath added the backport/done All backports for this PR have been created label Feb 13, 2022

lunny reviewed Feb 14, 2022

View reviewed changes

models/migrations/v209.go

if err != nil {

return err

}

case schemas.ORACLE:

Copy link

Member

lunny Feb 14, 2022

Choose a reason for hiding this comment

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

Wow, Gitea hasn't support ORACLE yet.

go-gitea locked and limited conversation to collaborators Apr 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increase the size of the webauthn_credential credential_id field #18739

Increase the size of the webauthn_credential credential_id field #18739

zeripath commented Feb 12, 2022

zeripath commented Feb 12, 2022

fnetX commented Feb 12, 2022

zeripath commented Feb 12, 2022 •

edited

fnetX commented Feb 12, 2022 •

edited

lunny commented Feb 12, 2022

zeripath commented Feb 12, 2022

lunny commented Feb 13, 2022

lunny Feb 13, 2022

zeripath Feb 13, 2022

zeripath Feb 13, 2022

zeripath commented Feb 13, 2022

lunny Feb 14, 2022

Increase the size of the webauthn_credential credential_id field #18739

Increase the size of the webauthn_credential credential_id field #18739

Conversation

zeripath commented Feb 12, 2022

zeripath commented Feb 12, 2022

fnetX commented Feb 12, 2022

zeripath commented Feb 12, 2022 • edited

fnetX commented Feb 12, 2022 • edited

lunny commented Feb 12, 2022

zeripath commented Feb 12, 2022

lunny commented Feb 13, 2022

lunny Feb 13, 2022

Choose a reason for hiding this comment

zeripath Feb 13, 2022

Choose a reason for hiding this comment

zeripath Feb 13, 2022

Choose a reason for hiding this comment

zeripath commented Feb 13, 2022

lunny Feb 14, 2022

Choose a reason for hiding this comment

zeripath commented Feb 12, 2022 •

edited

fnetX commented Feb 12, 2022 •

edited