Skip to content

Commit

Permalink
fix(mfa): filter credentials on non mfa methods
Browse files Browse the repository at this point in the history
* remove mfa credentials from allowedCredentials when using non mfa routes
* for good measure also remove mfa credentials from FindCredentialById when used on Non-MFA routes
* rename CreateApiKeyError to CheckApiKey
* increase versions of openapi spec
* change defaults for mfa config object in admin spec
* cleanup: remove unused methods from handler/webauthn.go

Closes: #45
  • Loading branch information
Stefan Jacobi committed Apr 10, 2024
1 parent c69314c commit bcab38d
Show file tree
Hide file tree
Showing 11 changed files with 33 additions and 47 deletions.
14 changes: 13 additions & 1 deletion server/api/dto/intern/webauthn_user.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,17 @@ type WebauthnUser struct {
Icon string
DisplayName string
WebauthnCredentials []models.WebauthnCredential
IsMfaUser bool
}

func NewWebauthnUser(user models.WebauthnUser) *WebauthnUser {
func NewWebauthnUser(user models.WebauthnUser, isMfaUser bool) *WebauthnUser {
return &WebauthnUser{
UserId: user.UserID,
Name: user.Name,
Icon: user.Icon,
DisplayName: user.DisplayName,
WebauthnCredentials: user.WebauthnCredentials,
IsMfaUser: isMfaUser,
}
}

Expand All @@ -42,6 +44,11 @@ func (u *WebauthnUser) WebAuthnIcon() string {
func (u *WebauthnUser) WebAuthnCredentials() []webauthn.Credential {
var credentials []webauthn.Credential
for _, credential := range u.WebauthnCredentials {
if !u.IsMfaUser && credential.IsMFA {
// Skip if request is not for an MFA cred but the credential is a mfa cred
continue
}

cred := credential
c := WebauthnCredentialFromModel(&cred)
credentials = append(credentials, *c)
Expand All @@ -52,6 +59,11 @@ func (u *WebauthnUser) WebAuthnCredentials() []webauthn.Credential {

func (u *WebauthnUser) FindCredentialById(credentialId string) *models.WebauthnCredential {
for i := range u.WebauthnCredentials {
if !u.IsMfaUser && u.WebauthnCredentials[i].IsMFA {
// Skip if request is not for an MFA cred but the credential is a mfa cred
continue
}

if u.WebauthnCredentials[i].ID == credentialId {
return &u.WebauthnCredentials[i]
}
Expand Down
2 changes: 1 addition & 1 deletion server/api/handler/login.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func (lh *loginHandler) Init(ctx echo.Context) error {
return echo.NewHTTPError(http.StatusUnauthorized, "api key is missing")
}

err = helper.CreateApiKeyError(h.Config.Secrets, apiKey)
err = helper.CheckApiKey(h.Config.Secrets, apiKey)
if err != nil {
return err
}
Expand Down
27 changes: 0 additions & 27 deletions server/api/handler/webauthn.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,12 @@ package handler

import (
"errors"
"fmt"
"github.com/gobuffalo/pop/v6"
"github.com/gofrs/uuid"
"github.com/labstack/echo/v4"
"github.com/teamhanko/passkey-server/api/dto/intern"
"github.com/teamhanko/passkey-server/api/dto/request"
auditlog "github.com/teamhanko/passkey-server/audit_log"
"github.com/teamhanko/passkey-server/persistence"
"github.com/teamhanko/passkey-server/persistence/models"
"github.com/teamhanko/passkey-server/persistence/persisters"
"net/http"
)

Expand Down Expand Up @@ -51,29 +47,6 @@ func (w *webauthnHandler) handleError(logger auditlog.Logger, logType models.Aud
return nil
}

func (w *webauthnHandler) getWebauthnUserByUserHandle(userHandle string, tenantId uuid.UUID, persister persisters.WebauthnUserPersister) (*intern.WebauthnUser, error) {
user, err := persister.GetByUserId(userHandle, tenantId)
if err != nil {
return nil, fmt.Errorf("failed to get user: %w", err)
}

if user == nil {
return nil, fmt.Errorf("user not found")
}

return intern.NewWebauthnUser(*user), nil
}

func (w *webauthnHandler) convertUserHandle(userHandle []byte) string {
userId := string(userHandle)
userUuid, err := uuid.FromBytes(userHandle)
if err == nil {
userId = userUuid.String()
}

return userId
}

func BindAndValidateRequest[I request.CredentialRequests | request.WebauthnRequests](ctx echo.Context) (*I, error) {
var requestDto I
err := ctx.Bind(&requestDto)
Expand Down
2 changes: 1 addition & 1 deletion server/api/helper/tenant_helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import (
"strings"
)

func CreateApiKeyError(keys []models.Secret, apiKey string) error {
func CheckApiKey(keys []models.Secret, apiKey string) error {
var foundKey *models.Secret
for _, key := range keys {
if strings.TrimSpace(apiKey) == key.Key {
Expand Down
2 changes: 1 addition & 1 deletion server/api/middleware/api_key.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ func ApiKeyMiddleware() echo.MiddlewareFunc {
return echo.NewHTTPError(http.StatusNotFound, "tenant not found")
}

err := helper.CreateApiKeyError(tenant.Config.Secrets, apiKey)
err := helper.CheckApiKey(tenant.Config.Secrets, apiKey)
if err != nil {
return err
}
Expand Down
5 changes: 2 additions & 3 deletions server/api/services/login_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ type LoginService interface {
type loginService struct {
WebauthnService
userId *string
useMFA bool
}

func NewLoginService(params WebauthnServiceCreateParams) LoginService {
Expand All @@ -36,9 +35,9 @@ func NewLoginService(params WebauthnServiceCreateParams) LoginService {

userPersister: params.UserPersister,
sessionDataPersister: params.SessionPersister,
useMFA: params.UseMFA,
},
params.UserId,
params.UseMFA,
}
}

Expand All @@ -64,8 +63,8 @@ func (ls *loginService) Initialize() (*protocol.CredentialAssertion, error) {
fmt.Errorf("failed to create webauthn assertion options for login: %w", err),
)
}
isDiscoverable = false

isDiscoverable = false
} else {
credentialAssertion, sessionData, err = ls.webauthnClient.BeginDiscoverableLogin()

Expand Down
10 changes: 5 additions & 5 deletions server/api/services/registration_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ type RegistrationService interface {
type registrationService struct {
WebauthnService
mapper.AuthenticatorMetadata
UseMFA bool
}

func NewRegistrationService(params WebauthnServiceCreateParams) RegistrationService {
Expand All @@ -39,9 +38,10 @@ func NewRegistrationService(params WebauthnServiceCreateParams) RegistrationServ

userPersister: params.UserPersister,
sessionDataPersister: params.SessionPersister,

useMFA: params.UseMFA,
},
params.AuthenticatorMetadata,
params.UseMFA,
}
}

Expand Down Expand Up @@ -87,7 +87,7 @@ func (rs *registrationService) createOrUpdateUser(user models.WebauthnUser) (*in
return nil, err
}

return intern.NewWebauthnUser(user), err
return intern.NewWebauthnUser(user, rs.useMFA), err
}

func (rs *registrationService) getDbUser(userId string) (*models.WebauthnUser, error) {
Expand Down Expand Up @@ -170,7 +170,7 @@ func (rs *registrationService) geDbtUserAndSessionFromRequest(req *protocol.Pars
}

func (rs *registrationService) createCredential(dbUser *models.WebauthnUser, session *models.WebauthnSessionData, req *protocol.ParsedCredentialCreationData) (*models.WebauthnCredential, error) {
credential, err := rs.webauthnClient.CreateCredential(intern.NewWebauthnUser(*dbUser), *intern.WebauthnSessionDataFromModel(session), req)
credential, err := rs.webauthnClient.CreateCredential(intern.NewWebauthnUser(*dbUser, rs.useMFA), *intern.WebauthnSessionDataFromModel(session), req)
if err != nil {
rs.logger.Error(err)

Expand Down Expand Up @@ -200,7 +200,7 @@ func (rs *registrationService) createCredential(dbUser *models.WebauthnUser, ses
flags.HasBackupEligible(),
flags.HasBackupState(),
rs.AuthenticatorMetadata,
rs.UseMFA,
rs.useMFA,
)

err = rs.credentialPersister.Create(dbCredential)
Expand Down
6 changes: 3 additions & 3 deletions server/api/services/transaction_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ type transactionService struct {
*WebauthnService

transactionPersister persisters.TransactionPersister
useMFA bool
}

func NewTransactionService(params TransactionServiceCreateParams) TransactionService {
Expand All @@ -45,9 +44,10 @@ func NewTransactionService(params TransactionServiceCreateParams) TransactionSer

userPersister: params.UserPersister,
sessionDataPersister: params.SessionPersister,

useMFA: params.UseMFA,
},
transactionPersister: params.TransactionPersister,
useMFA: params.UseMFA,
}
}

Expand All @@ -71,7 +71,7 @@ func (ts *transactionService) Initialize(userId string, transaction *models.Tran
}

credentialAssertion, sessionData, err := ts.webauthnClient.BeginLogin(
intern.NewWebauthnUser(*webauthnUser),
intern.NewWebauthnUser(*webauthnUser, ts.useMFA),
ts.withTransaction(transaction.Identifier, transaction.Data),
)
if err != nil {
Expand Down
4 changes: 3 additions & 1 deletion server/api/services/webauthn_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ type WebauthnService struct {

userPersister persisters.WebauthnUserPersister
sessionDataPersister persisters.WebauthnSessionDataPersister

useMFA bool
}

type WebauthnServiceCreateParams struct {
Expand Down Expand Up @@ -75,7 +77,7 @@ func (ws *WebauthnService) getWebauthnUserByUserHandle(userHandle string) (*inte
return nil, fmt.Errorf("user not found")
}

return intern.NewWebauthnUser(*user), nil
return intern.NewWebauthnUser(*user, ws.useMFA), nil
}

func (ws *WebauthnService) createUserCredentialToken(userId string, credentialId string) (string, error) {
Expand Down
6 changes: 3 additions & 3 deletions spec/passkey-server-admin.yaml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
openapi: 3.1.0
info:
version: '1.1'
version: '1.2'
title: passkey-server-admin
summary: Admin API for Passkey Server
description: 'ADmin API for Hanko Passkey Server. Allows creation and configiration of tenants and api keys, '
Expand Down Expand Up @@ -734,7 +734,7 @@ components:
- required
- preferred
- discouraged
description: defaults to `discouraged` when omitted
description: defaults to `preferred` when omitted
attachment:
type: string
enum:
Expand All @@ -755,7 +755,7 @@ components:
- discouraged
- preferred
- required
description: defaults to `preferred` when omitted
description: defaults to `discouraged` when omitted
required:
- timeout
secret_list:
Expand Down
2 changes: 1 addition & 1 deletion spec/passkey-server.yaml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
openapi: 3.1.0
info:
version: '1.0'
version: '1.1'
title: passkey-server
summary: 'OpenAPI Spec for creating, managing and using passkeys'
description: 'This API shall represent the private and public endpoints for passkey registration, management and authentication'
Expand Down

0 comments on commit bcab38d

Please sign in to comment.