From 63736e83011e8d8df681dd73898e4f98045baa10 Mon Sep 17 00:00:00 2001 From: Gusted Date: Wed, 28 Aug 2024 07:40:40 +0200 Subject: [PATCH] [FEAT] Add support for webauthn credential level 3 - For WebAuthn Credential level 3, the `backup_eligible` and `backup_state` flags are checked if they are consistent with the values given on login. Forgejo never stored this data, so add a database migration that makes all webauthn credentials 'legacy' and on the next first use capture the values of `backup_eligible` and `backup_state`. As suggested in https://github.com/go-webauthn/webauthn/discussions/219#discussioncomment-10429662 - Adds unit tests. - Add E2E test. --- models/auth/webauthn.go | 23 ++++++++-- models/auth/webauthn_test.go | 14 +++++- models/fixtures/webauthn_credential.yml | 1 + models/forgejo_migrations/migrate.go | 2 + models/forgejo_migrations/v22.go | 17 +++++++ routers/web/auth/webauthn.go | 26 ++++++++--- tests/e2e/webauthn.test.e2e.js | 60 +++++++++++++++++++++++++ 7 files changed, 131 insertions(+), 12 deletions(-) create mode 100644 models/forgejo_migrations/v22.go create mode 100644 tests/e2e/webauthn.test.e2e.js diff --git a/models/auth/webauthn.go b/models/auth/webauthn.go index a65d2e1e34..aa13cf6cb1 100644 --- a/models/auth/webauthn.go +++ b/models/auth/webauthn.go @@ -40,7 +40,7 @@ func IsErrWebAuthnCredentialNotExist(err error) bool { } // WebAuthnCredential represents the WebAuthn credential data for a public-key -// credential conformant to WebAuthn Level 1 +// credential conformant to WebAuthn Level 3 type WebAuthnCredential struct { ID int64 `xorm:"pk autoincr"` Name string @@ -52,8 +52,12 @@ type WebAuthnCredential struct { AAGUID []byte SignCount uint32 `xorm:"BIGINT"` CloneWarning bool - CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"` - UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"` + BackupEligible bool `XORM:"NOT NULL DEFAULT false"` + BackupState bool `XORM:"NOT NULL DEFAULT false"` + // If legacy is set to true, backup_eligible and backup_state isn't set. + Legacy bool `XORM:"NOT NULL DEFAULT true"` + CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"` + UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"` } func init() { @@ -71,6 +75,12 @@ func (cred *WebAuthnCredential) UpdateSignCount(ctx context.Context) error { return err } +// UpdateFromLegacy update the values that aren't present on legacy credentials. +func (cred *WebAuthnCredential) UpdateFromLegacy(ctx context.Context) error { + _, err := db.GetEngine(ctx).ID(cred.ID).Cols("legacy", "backup_eligible", "backup_state").Update(cred) + return err +} + // BeforeInsert will be invoked by XORM before updating a record func (cred *WebAuthnCredential) BeforeInsert() { cred.LowerName = strings.ToLower(cred.Name) @@ -97,6 +107,10 @@ func (list WebAuthnCredentialList) ToCredentials() []webauthn.Credential { ID: cred.CredentialID, PublicKey: cred.PublicKey, AttestationType: cred.AttestationType, + Flags: webauthn.CredentialFlags{ + BackupEligible: cred.BackupEligible, + BackupState: cred.BackupState, + }, Authenticator: webauthn.Authenticator{ AAGUID: cred.AAGUID, SignCount: cred.SignCount, @@ -167,6 +181,9 @@ func CreateCredential(ctx context.Context, userID int64, name string, cred *weba AAGUID: cred.Authenticator.AAGUID, SignCount: cred.Authenticator.SignCount, CloneWarning: false, + BackupEligible: cred.Flags.BackupEligible, + BackupState: cred.Flags.BackupState, + Legacy: false, } if err := db.Insert(ctx, c); err != nil { diff --git a/models/auth/webauthn_test.go b/models/auth/webauthn_test.go index cae590c790..e1cd652009 100644 --- a/models/auth/webauthn_test.go +++ b/models/auth/webauthn_test.go @@ -56,13 +56,23 @@ func TestWebAuthnCredential_UpdateLargeCounter(t *testing.T) { unittest.AssertExistsIf(t, true, &auth_model.WebAuthnCredential{ID: 1, SignCount: 0xffffffff}) } +func TestWebAuthenCredential_UpdateFromLegacy(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + cred := unittest.AssertExistsAndLoadBean(t, &auth_model.WebAuthnCredential{ID: 1, Legacy: true}) + cred.Legacy = false + cred.BackupEligible = true + cred.BackupState = true + require.NoError(t, cred.UpdateFromLegacy(db.DefaultContext)) + unittest.AssertExistsIf(t, true, &auth_model.WebAuthnCredential{ID: 1, BackupEligible: true, BackupState: true}, "legacy = false") +} + func TestCreateCredential(t *testing.T) { require.NoError(t, unittest.PrepareTestDatabase()) - res, err := auth_model.CreateCredential(db.DefaultContext, 1, "WebAuthn Created Credential", &webauthn.Credential{ID: []byte("Test")}) + res, err := auth_model.CreateCredential(db.DefaultContext, 1, "WebAuthn Created Credential", &webauthn.Credential{ID: []byte("Test"), Flags: webauthn.CredentialFlags{BackupEligible: true, BackupState: true}}) require.NoError(t, err) assert.Equal(t, "WebAuthn Created Credential", res.Name) assert.Equal(t, []byte("Test"), res.CredentialID) - unittest.AssertExistsIf(t, true, &auth_model.WebAuthnCredential{Name: "WebAuthn Created Credential", UserID: 1}) + unittest.AssertExistsIf(t, true, &auth_model.WebAuthnCredential{Name: "WebAuthn Created Credential", UserID: 1, BackupEligible: true, BackupState: true}, "legacy = false") } diff --git a/models/fixtures/webauthn_credential.yml b/models/fixtures/webauthn_credential.yml index bc43127fcd..edf9935ebf 100644 --- a/models/fixtures/webauthn_credential.yml +++ b/models/fixtures/webauthn_credential.yml @@ -5,5 +5,6 @@ attestation_type: none sign_count: 0 clone_warning: false + legacy: true created_unix: 946684800 updated_unix: 946684800 diff --git a/models/forgejo_migrations/migrate.go b/models/forgejo_migrations/migrate.go index 598ec8bbaa..cca83d6b4d 100644 --- a/models/forgejo_migrations/migrate.go +++ b/models/forgejo_migrations/migrate.go @@ -80,6 +80,8 @@ var migrations = []*Migration{ NewMigration("Creating Quota-related tables", CreateQuotaTables), // v21 -> v22 NewMigration("Add SSH keypair to `pull_mirror` table", AddSSHKeypairToPushMirror), + // v22 -> v23 + NewMigration("Add `legacy` to `web_authn_credential` table", AddLegacyToWebAuthnCredential), } // GetCurrentDBVersion returns the current Forgejo database version. diff --git a/models/forgejo_migrations/v22.go b/models/forgejo_migrations/v22.go new file mode 100644 index 0000000000..eeb738799c --- /dev/null +++ b/models/forgejo_migrations/v22.go @@ -0,0 +1,17 @@ +// Copyright 2024 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package forgejo_migrations //nolint:revive + +import "xorm.io/xorm" + +func AddLegacyToWebAuthnCredential(x *xorm.Engine) error { + type WebauthnCredential struct { + ID int64 `xorm:"pk autoincr"` + BackupEligible bool `xorm:"NOT NULL DEFAULT false"` + BackupState bool `xorm:"NOT NULL DEFAULT false"` + Legacy bool `xorm:"NOT NULL DEFAULT true"` + } + + return x.Sync(&WebauthnCredential{}) +} diff --git a/routers/web/auth/webauthn.go b/routers/web/auth/webauthn.go index 1079f44a08..5c93c1410e 100644 --- a/routers/web/auth/webauthn.go +++ b/routers/web/auth/webauthn.go @@ -116,6 +116,25 @@ func WebAuthnLoginAssertionPost(ctx *context.Context) { return } + dbCred, err := auth.GetWebAuthnCredentialByCredID(ctx, user.ID, parsedResponse.RawID) + if err != nil { + ctx.ServerError("GetWebAuthnCredentialByCredID", err) + return + } + + // If the credential is legacy, assume the values are correct. The + // specification mandates these flags don't change. + if dbCred.Legacy { + dbCred.BackupEligible = parsedResponse.Response.AuthenticatorData.Flags.HasBackupEligible() + dbCred.BackupState = parsedResponse.Response.AuthenticatorData.Flags.HasBackupState() + dbCred.Legacy = false + + if err := dbCred.UpdateFromLegacy(ctx); err != nil { + ctx.ServerError("UpdateFromLegacy", err) + return + } + } + // Validate the parsed response. cred, err := wa.WebAuthn.ValidateLogin((*wa.User)(user), *sessionData, parsedResponse) if err != nil { @@ -133,13 +152,6 @@ func WebAuthnLoginAssertionPost(ctx *context.Context) { return } - // Success! Get the credential and update the sign count with the new value we received. - dbCred, err := auth.GetWebAuthnCredentialByCredID(ctx, user.ID, cred.ID) - if err != nil { - ctx.ServerError("GetWebAuthnCredentialByCredID", err) - return - } - dbCred.SignCount = cred.Authenticator.SignCount if err := dbCred.UpdateSignCount(ctx); err != nil { ctx.ServerError("UpdateSignCount", err) diff --git a/tests/e2e/webauthn.test.e2e.js b/tests/e2e/webauthn.test.e2e.js new file mode 100644 index 0000000000..30c92c48f0 --- /dev/null +++ b/tests/e2e/webauthn.test.e2e.js @@ -0,0 +1,60 @@ +// Copyright 2024 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: MIT +// @ts-check + +import {expect} from '@playwright/test'; +import {test, login_user, load_logged_in_context} from './utils_e2e.js'; + +test.beforeAll(async ({browser}, workerInfo) => { + await login_user(browser, workerInfo, 'user2'); +}); + +test('WebAuthn register & login flow', async ({browser}, workerInfo) => { + test.skip(workerInfo.project.name !== 'chromium', 'Uses Chrome protocol'); + const context = await load_logged_in_context(browser, workerInfo, 'user2'); + const page = await context.newPage(); + + // Register a security key. + let response = await page.goto('/user/settings/security'); + await expect(response?.status()).toBe(200); + + // https://github.com/microsoft/playwright/issues/7276#issuecomment-1516768428 + const cdpSession = await page.context().newCDPSession(page); + await cdpSession.send('WebAuthn.enable'); + await cdpSession.send('WebAuthn.addVirtualAuthenticator', { + options: { + protocol: 'ctap2', + ctap2Version: 'ctap2_1', + hasUserVerification: true, + transport: 'usb', + automaticPresenceSimulation: true, + isUserVerified: true, + backupEligibility: true, + }, + }); + + await page.locator('input#nickname').fill('Testing Security Key'); + await page.getByText('Add security key').click(); + + // Logout. + await page.locator('div[aria-label="Profile and settings…"]').click(); + await page.getByText('Sign Out').click(); + await page.waitForURL(`${workerInfo.project.use.baseURL}/`); + + // Login. + response = await page.goto('/user/login'); + await expect(response?.status()).toBe(200); + + await page.getByLabel('Username or email address').fill('user2'); + await page.getByLabel('Password').fill('password'); + await page.getByRole('button', {name: 'Sign in'}).click(); + await page.waitForURL(`${workerInfo.project.use.baseURL}/user/webauthn`); + await page.waitForURL(`${workerInfo.project.use.baseURL}/`); + + // Cleanup. + response = await page.goto('/user/settings/security'); + await expect(response?.status()).toBe(200); + await page.getByRole('button', {name: 'Remove'}).click(); + await page.getByRole('button', {name: 'Yes'}).click(); + await page.waitForURL(`${workerInfo.project.use.baseURL}/user/settings/security`); +});