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 nits #18284

Merged
merged 10 commits into from
Jan 15, 2022
14 changes: 7 additions & 7 deletions models/auth/webauthn.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ package auth

import (
"context"
"encoding/base64"
"encoding/base32"
"fmt"
"strings"

Expand Down Expand Up @@ -94,7 +94,7 @@ type WebAuthnCredentialList []*WebAuthnCredential
func (list WebAuthnCredentialList) ToCredentials() []webauthn.Credential {
creds := make([]webauthn.Credential, 0, len(list))
for _, cred := range list {
credID, _ := base64.RawStdEncoding.DecodeString(cred.CredentialID)
credID, _ := base32.HexEncoding.DecodeString(cred.CredentialID)
creds = append(creds, webauthn.Credential{
ID: credID,
PublicKey: cred.PublicKey,
Expand Down Expand Up @@ -164,13 +164,13 @@ func HasWebAuthnRegistrationsByUID(uid int64) (bool, error) {
}

// GetWebAuthnCredentialByCredID returns WebAuthn credential by credential ID
func GetWebAuthnCredentialByCredID(credID string) (*WebAuthnCredential, error) {
return getWebAuthnCredentialByCredID(db.DefaultContext, credID)
func GetWebAuthnCredentialByCredID(userID int64, credID string) (*WebAuthnCredential, error) {
return getWebAuthnCredentialByCredID(db.DefaultContext, userID, credID)
}

func getWebAuthnCredentialByCredID(ctx context.Context, credID string) (*WebAuthnCredential, error) {
func getWebAuthnCredentialByCredID(ctx context.Context, userID int64, credID string) (*WebAuthnCredential, error) {
cred := new(WebAuthnCredential)
if found, err := db.GetEngine(ctx).Where("credential_id = ?", credID).Get(cred); err != nil {
if found, err := db.GetEngine(ctx).Where("user_id = ? AND credential_id = ?", userID, credID).Get(cred); err != nil {
return nil, err
} else if !found {
return nil, ErrWebAuthnCredentialNotExist{CredentialID: credID}
Expand All @@ -187,7 +187,7 @@ func createCredential(ctx context.Context, userID int64, name string, cred *weba
c := &WebAuthnCredential{
UserID: userID,
Name: name,
CredentialID: base64.RawStdEncoding.EncodeToString(cred.ID),
CredentialID: base32.HexEncoding.EncodeToString(cred.ID),
PublicKey: cred.PublicKey,
AttestationType: cred.AttestationType,
AAGUID: cred.Authenticator.AAGUID,
Expand Down
4 changes: 2 additions & 2 deletions models/auth/webauthn_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
package auth

import (
"encoding/base64"
"encoding/base32"
"testing"

"code.gitea.io/gitea/models/unittest"
Expand Down Expand Up @@ -61,7 +61,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 := base64.RawStdEncoding.DecodeString(res.CredentialID)
bs, err := base32.HexEncoding.DecodeString(res.CredentialID)
assert.NoError(t, err)
assert.Equal(t, []byte("Test"), bs)

Expand Down
2 changes: 2 additions & 0 deletions models/migrations/migrations.go
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,8 @@ var migrations = []Migration{
NewMigration("Add authorize column to team_unit table", addAuthorizeColForTeamUnit),
// v207 -> v208
NewMigration("Add webauthn table and migrate u2f data to webauthn", addWebAuthnCred),
// v208 -> v209
NewMigration("Use base32.HexEncoding instead of base64 encoding for cred ID as it is case insensitive", useBase32HexForCredIDInWebAuthnCredential),
}

// GetCurrentDBVersion returns the current db version
Expand Down
4 changes: 2 additions & 2 deletions models/migrations/v207.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ package migrations

import (
"crypto/elliptic"
"encoding/base64"
"encoding/base32"
zeripath marked this conversation as resolved.
Show resolved Hide resolved
"strings"

"code.gitea.io/gitea/modules/timeutil"
Expand Down Expand Up @@ -67,7 +67,7 @@ func addWebAuthnCred(x *xorm.Engine) error {
Name: reg.Name,
LowerName: strings.ToLower(reg.Name),
UserID: reg.UserID,
CredentialID: base64.RawStdEncoding.EncodeToString(parsed.KeyHandle),
CredentialID: base32.HexEncoding.EncodeToString(parsed.KeyHandle),
zeripath marked this conversation as resolved.
Show resolved Hide resolved
PublicKey: elliptic.Marshal(elliptic.P256(), parsed.PubKey.X, parsed.PubKey.Y),
AttestationType: "fido-u2f",
AAGUID: []byte{},
Expand Down
51 changes: 51 additions & 0 deletions models/migrations/v208.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// 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"

"xorm.io/xorm"
)

func useBase32HexForCredIDInWebAuthnCredential(x *xorm.Engine) error {

// Create webauthnCredential table
type webauthnCredential struct {
ID int64 `xorm:"pk autoincr"`
CredentialID string `xorm:"INDEX"`
}
if err := x.Sync2(&webauthnCredential{}); err != nil {
return err
}

var start int
regs := make([]*webauthnCredential, 0, 50)
for {
err := x.OrderBy("id").Limit(50, start).Find(&regs)
if err != nil {
return err
}

for _, reg := range regs {
credID, _ := base64.RawStdEncoding.DecodeString(reg.CredentialID)
reg.CredentialID = base32.HexEncoding.EncodeToString(credID)

_, err := x.Update(reg)
zeripath marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return err
}
}

if len(regs) < 50 {
break
}
start += 50
regs = regs[:0]
}

return nil
}
1 change: 0 additions & 1 deletion options/locale/locale_en-US.ini
Original file line number Diff line number Diff line change
Expand Up @@ -751,7 +751,6 @@ twofa_failed_get_secret = Failed to get secret.
webauthn_desc = Security keys are hardware devices containing cryptographic keys. They can be used for two-factor authentication. Security keys must support the <a rel="noreferrer" href="https://w3c.github.io/webauthn/#webauthn-authenticator">WebAuthn Authenticator</a> standard.
zeripath marked this conversation as resolved.
Show resolved Hide resolved
webauthn_register_key = Add Security Key
webauthn_nickname = Nickname
webauthn_press_button = Press the button on your security key to register it.
webauthn_delete_key = Remove Security Key
webauthn_delete_key_desc = If you remove a security key you can no longer sign in with it. Continue?

Expand Down
4 changes: 2 additions & 2 deletions routers/web/auth/webauthn.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
package auth

import (
"encoding/base64"
"encoding/base32"
"errors"
"net/http"

Expand Down Expand Up @@ -131,7 +131,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(base64.RawStdEncoding.EncodeToString(cred.ID))
dbCred, err := auth.GetWebAuthnCredentialByCredID(user.ID, base32.HexEncoding.EncodeToString(cred.ID))
if err != nil {
ctx.ServerError("GetWebAuthnCredentialByCredID", err)
return
Expand Down
18 changes: 10 additions & 8 deletions routers/web/user/setting/security/webauthn.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,9 @@ func WebAuthnRegister(ctx *context.Context) {
return
}

_ = ctx.Session.Delete("registration")
if err := ctx.Session.Set("WebauthnName", form.Name); err != nil {
ctx.ServerError("Unable to set session key for WebauthnName", err)
_ = ctx.Session.Delete("webauthnRegistration")
if err := ctx.Session.Set("webauthnName", form.Name); err != nil {
ctx.ServerError("Unable to set session key for webauthnName", err)
return
}

Expand All @@ -51,7 +51,7 @@ func WebAuthnRegister(ctx *context.Context) {
}

// Save the session data as marshaled JSON
if err = ctx.Session.Set("registration", sessionData); err != nil {
if err = ctx.Session.Set("webauthnRegistration", sessionData); err != nil {
ctx.ServerError("Unable to set session", err)
return
}
Expand All @@ -61,20 +61,20 @@ func WebAuthnRegister(ctx *context.Context) {

// WebauthnRegisterPost receives the response of the security key
func WebauthnRegisterPost(ctx *context.Context) {
name, ok := ctx.Session.Get("WebauthnName").(string)
name, ok := ctx.Session.Get("webauthnName").(string)
if !ok || name == "" {
ctx.ServerError("Get WebauthnName", errors.New("no WebauthnName"))
ctx.ServerError("Get webauthnName", errors.New("no webauthnName"))
return
}

// Load the session data
sessionData, ok := ctx.Session.Get("registration").(*webauthn.SessionData)
sessionData, ok := ctx.Session.Get("webauthnRegistration").(*webauthn.SessionData)
if !ok || sessionData == nil {
ctx.ServerError("Get registration", errors.New("no registration"))
return
}
defer func() {
_ = ctx.Session.Delete("registration")
_ = ctx.Session.Delete("webauthnRegistration")
}()

// Verify that the challenge succeeded
Expand Down Expand Up @@ -103,6 +103,8 @@ func WebauthnRegisterPost(ctx *context.Context) {
ctx.ServerError("CreateCredential", err)
return
}
_ = ctx.Session.Delete("webauthnName")

ctx.JSON(http.StatusCreated, cred)
}

Expand Down
2 changes: 1 addition & 1 deletion templates/user/auth/webauthn_error.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
<div class="hide" data-webauthn-error-msg="duplicated"><p>{{.i18n.Tr "webauthn_error_duplicated"}}</div>
<div class="hide" data-webauthn-error-msg="empty"><p>{{.i18n.Tr "webauthn_error_empty"}}</div>
<div class="hide" data-webauthn-error-msg="timeout"><p>{{.i18n.Tr "webauthn_error_timeout"}}</div>
<div class="hide" data-webauthn-error-msg="0"></div>
<div class="hide" data-webauthn-error-msg="general"></div>
</div>
</div>
<div class="actions">
Expand Down
10 changes: 0 additions & 10 deletions templates/user/settings/security/webauthn.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -28,16 +28,6 @@
</div>
</div>

<div class="ui small modal" id="register-device">
<div class="header">{{.i18n.Tr "settings.webauthn_register_key"}}</div>
<div class="content">
<i class="notched spinner loading icon"></i> {{.i18n.Tr "settings.webauthn_press_button"}}
</div>
<div class="actions">
<div class="ui cancel button">{{.i18n.Tr "cancel"}}</div>
</div>
</div>

{{template "user/auth/webauthn_error" .}}

<div class="ui small basic delete modal" id="delete-registration">
Expand Down
14 changes: 6 additions & 8 deletions web_src/js/features/user-auth-webauthn.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export function initUserAuthWebAuthn() {
.then((credential) => {
verifyAssertion(credential);
}).catch((err) => {
webAuthnError(0, err.message);
webAuthnError('general', err.message);
});
}).fail(() => {
webAuthnError('unknown');
Expand Down Expand Up @@ -113,10 +113,10 @@ function webauthnRegistered(newCredential) {

function webAuthnError(errorType, message) {
$('#webauthn-error [data-webauthn-error-msg]').hide();
if (errorType === 0 && message && message.length > 1) {
$(`#webauthn-error [data-webauthn-error-msg=0]`).text(message);
$(`#webauthn-error [data-webauthn-error-msg=0]`).show();
} else {
if (errorType === 'general' && message && message.length > 1) {
$(`#webauthn-error [data-webauthn-error-msg=general]`).text(message);
$(`#webauthn-error [data-webauthn-error-msg=general]`).show();
zeripath marked this conversation as resolved.
Show resolved Hide resolved
} else if (errorType !== 'general') {
$(`#webauthn-error [data-webauthn-error-msg=${errorType}]`).show();
}
$('#webauthn-error').modal('show');
Expand Down Expand Up @@ -149,7 +149,6 @@ export function initUserAuthWebAuthnRegister() {
return;
}

$('#register-device').modal({allowMultiple: false});
$('#webauthn-error').modal({allowMultiple: false});
$('#register-webauthn').on('click', (e) => {
e.preventDefault();
Expand All @@ -167,7 +166,6 @@ function webAuthnRegisterRequest() {
name: $('#nickname').val(),
}).done((makeCredentialOptions) => {
$('#nickname').closest('div.field').removeClass('error');
$('#register-device').modal('show');

makeCredentialOptions.publicKey.challenge = decode(makeCredentialOptions.publicKey.challenge);
makeCredentialOptions.publicKey.user.id = decode(makeCredentialOptions.publicKey.user.id);
Expand All @@ -185,7 +183,7 @@ function webAuthnRegisterRequest() {
webAuthnError('unknown');
return;
}
webAuthnError(0, err);
webAuthnError('general', err.message);
});
}).fail((xhr) => {
if (xhr.status === 409) {
Expand Down