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

WebAuthn CredentialID field needs to be increased in size #20530

Merged

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Jul 28, 2022

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

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.

Fix go-gitea#20457

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
Copy link
Contributor Author

Yes this has a migration. I suggest we just port the back the unnecessary migration that is between these two and/or the associated PR.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 28, 2022
Copy link
Member

@jolheiser jolheiser left a comment

Choose a reason for hiding this comment

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

LGTM, until next time 🤠

@GiteaBot 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 Jul 28, 2022
Copy link
Contributor

@Gusted Gusted left a comment

Choose a reason for hiding this comment

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

@jolheiser You're just asking for problems 🙊

@GiteaBot 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 Jul 28, 2022
models/migrations/v221.go Outdated Show resolved Hide resolved
@wxiaoguang
Copy link
Contributor

LGTM, how about setting the length to a large-enough value, like 8000? There should be no difference between 1640 and 8000 on database side, then no worry about overflowing anymore.

@zeripath
Copy link
Contributor Author

The field is indexed and is searched directly so it's potentially bad on db systems like MySQL which store the value directly within the index. I'm not sure if these would store the max string size eg. 8000 bytes or whether they'd just store the string. If it's just the string then it's no worse but I don't know.

The spec has written clearly that this is at most 1023 bytes - previously it wasn't very clear on this hence the change. I guess the question is are they likely to increase it? My suspicion is we'd get more forewarning in that case.

I think if they do increase it again we're probably gonna need to think again about how we implement this. It could be that at that point we'd need to be doing shasums of the credential id searching for that and then doing equality of the credential id - if only to keep the index from being too large.

@zeripath
Copy link
Contributor Author

And in fact I think we've hit the problem I was referring to.

Looking at the tests I see:

       testlogger.go:78: 2022/07/29 14:18:52 ...ations/migrations.go:497:Migrate() [I] Migration[210]: v208 was completely broken - remigrate
        migration_test.go:299: 
            	Error Trace:	migration_test.go:299
            	            				migration_test.go:336
            	Error:      	Received unexpected error:
            	            	migrate: migration[210]: v208 was completely broken - remigrate failed: Error 1071: Specified key was too long; max key length is 3072 bytes
            	Test:       	TestMigrations/Migrate-mysql-1.6.4

Sigh I think we're going to need to do a bit more work here.

@6543 6543 mentioned this pull request Jul 29, 2022
Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
Copy link
Contributor Author

OK I've had to do something a lot more horrible because of MySQL's index issue.

zeripath added 6 commits July 29, 2022 20:10
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Copy link
Contributor

@Gusted Gusted left a comment

Choose a reason for hiding this comment

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

Small nit, otherwise L-gtm and verified migration on local instance.

models/auth/webauthn.go Outdated Show resolved Hide resolved
Signed-off-by: Andrew Thornton <art27@cantab.net>
models/migrations/v223.go Outdated Show resolved Hide resolved
@6543
Copy link
Member

6543 commented Jul 30, 2022

🚀

@6543 6543 merged commit e819da0 into go-gitea:main Jul 30, 2022
6543 pushed a commit to 6543-forks/gitea that referenced this pull request Jul 30, 2022
…0530)

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 go-gitea#20457

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath deleted the fix-20457-increase-webauthn-credential-field branch July 30, 2022 16:30
6543 added a commit that referenced this pull request Jul 30, 2022
…20555)

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>
Co-authored-by: zeripath <art27@cantab.net>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Aug 1, 2022
* giteaofficial/main: (29 commits)
  [skip ci] Updated translations via Crowdin
  Support localized README (go-gitea#20508)
  Clean up and fix clone button script (go-gitea#20415)
  Add disable download source configuration (go-gitea#20548)
  Fix default merge style (go-gitea#20564)
  Update login methods in package docs (go-gitea#20561)
  Add missing Tabs on organisation/package view (Frontport go-gitea#20539) (go-gitea#20540)
  [skip ci] Updated licenses and gitignores
  Add setting `SQLITE_JOURNAL_MODE` to enable WAL (go-gitea#20535)
  Rework file highlight rendering and fix yaml copy-paste (go-gitea#19967)
  Add new API endpoints for push mirrors management (go-gitea#19841)
  WebAuthn CredentialID field needs to be increased in size (go-gitea#20530)
  Add latest commit's SHA to content response (go-gitea#20398)
  Improve token and secret key generation docs (go-gitea#20387)
  [skip ci] Updated translations via Crowdin
  Rework raw file http header logic (go-gitea#20484)
  Update lunny/levelqueue to prevent NPE when reads are performed after close (go-gitea#20534)
  Added guidance on file to choose to download (go-gitea#20474)
  [skip ci] Updated translations via Crowdin
  Ensure that all unmerged files are merged when conflict checking (go-gitea#20528)
  ...
vsysoev pushed a commit to IntegraSDL/gitea that referenced this pull request Aug 10, 2022
…0530)

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 go-gitea#20457

Signed-off-by: Andrew Thornton <art27@cantab.net>
@wxiaoguang wxiaoguang added the backport/done All backports for this PR have been created label Aug 11, 2022
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Webauthn Credential ID length too short
6 participants