Skip to content

Commit 69b8472

Browse files
committed
fix(webauthn): preserve user_handle when adding a second security key
The WebAuthn specification requires that user.id (user_handle) remains stable for the lifetime of an identity. The settings flow was unconditionally overwriting user_handle with the session identity ID each time a new WebAuthn credential was added, causing previously registered authenticators to fail during login because the stored handle no longer matched. Only set UserHandle when it is not yet present (first credential added via settings). Existing credentials keep their original handle. Closes #4519
1 parent 8301fd4 commit 69b8472

File tree

2 files changed

+55
-1
lines changed

2 files changed

+55
-1
lines changed

selfservice/strategy/webauthn/settings.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -283,7 +283,9 @@ func (s *Strategy) continueSettingsFlowAdd(ctx context.Context, ctxUpdate *setti
283283
wc.AddedAt = time.Now().UTC().Round(time.Second)
284284
wc.DisplayName = p.RegisterDisplayName
285285
wc.IsPasswordless = s.d.Config().WebAuthnForPasswordless(ctx)
286-
cc.UserHandle = ctxUpdate.Session.IdentityID[:]
286+
if len(cc.UserHandle) == 0 {
287+
cc.UserHandle = ctxUpdate.Session.IdentityID[:]
288+
}
287289

288290
cc.Credentials = append(cc.Credentials, *wc)
289291
co, err := json.Marshal(cc)

selfservice/strategy/webauthn/settings_test.go

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -368,6 +368,58 @@ func TestCompleteSettings(t *testing.T) {
368368
})
369369
})
370370

371+
t.Run("case=user_handle is not overwritten when adding a second security key", func(t *testing.T) {
372+
// Regression test for https://github.com/ory/kratos/issues/4519
373+
// The user_handle must remain stable per the WebAuthn spec (user.id must not change).
374+
var id identity.Identity
375+
require.NoError(t, json.Unmarshal(settingsFixtureSuccessIdentity, &id))
376+
id.NID = uuid.Must(uuid.NewV4())
377+
_ = reg.PrivilegedIdentityPool().DeleteIdentity(t.Context(), id.ID)
378+
browserClient := testhelpers.NewHTTPClientWithIdentitySessionCookie(t.Context(), t, reg, &id)
379+
380+
// Add an existing WebAuthn credential with a specific user_handle
381+
existingUserHandle := []byte("original-user-handle-12")
382+
existingCredConfig, err := json.Marshal(identity.CredentialsWebAuthnConfig{
383+
Credentials: identity.CredentialsWebAuthn{
384+
{ID: []byte("existing-cred"), DisplayName: "existing key"},
385+
},
386+
UserHandle: existingUserHandle,
387+
})
388+
require.NoError(t, err)
389+
id.Credentials = map[identity.CredentialsType]identity.Credentials{
390+
identity.CredentialsTypeWebAuthn: {
391+
Type: identity.CredentialsTypeWebAuthn,
392+
Config: sqlxx.JSONRawMessage(existingCredConfig),
393+
},
394+
}
395+
require.NoError(t, reg.PrivilegedIdentityPool().UpdateIdentity(t.Context(), &id))
396+
397+
f := testhelpers.InitializeSettingsFlowViaBrowser(t, browserClient, false, publicTS)
398+
399+
// Inject the session to replay
400+
interim, err := reg.SettingsFlowPersister().GetSettingsFlow(context.Background(), uuid.FromStringOrNil(f.Id))
401+
require.NoError(t, err)
402+
interim.InternalContext = settingsFixtureSuccessInternalContext
403+
require.NoError(t, reg.SettingsFlowPersister().UpdateSettingsFlow(context.Background(), interim))
404+
405+
values := testhelpers.SDKFormFieldsToURLValues(f.Ui.Nodes)
406+
values.Set(node.WebAuthnRegister, string(settingsFixtureSuccessResponse))
407+
values.Set(node.WebAuthnRegisterDisplayName, "second key")
408+
body, res := testhelpers.SettingsMakeRequest(t, false, false, f, browserClient, testhelpers.EncodeFormAsJSON(t, false, values))
409+
require.Equal(t, http.StatusOK, res.StatusCode, body)
410+
assert.EqualValues(t, flow.StateSuccess, gjson.Get(body, "state").String(), body)
411+
412+
actual, err := reg.Persister().GetIdentityConfidential(context.Background(), id.ID)
413+
require.NoError(t, err)
414+
cred, ok := actual.GetCredentials(identity.CredentialsTypeWebAuthn)
415+
require.True(t, ok)
416+
417+
var cc identity.CredentialsWebAuthnConfig
418+
require.NoError(t, json.Unmarshal(cred.Config, &cc))
419+
assert.Equal(t, existingUserHandle, cc.UserHandle, "user_handle must not be overwritten when adding a second WebAuthn key")
420+
assert.Len(t, cc.Credentials, 2, "should now have two credentials")
421+
})
422+
371423
t.Run("case=fails to remove security key if it is passwordless and the last credential available", func(t *testing.T) {
372424
conf.MustSet(t.Context(), config.ViperKeyWebAuthnPasswordless, true)
373425
t.Cleanup(func() {

0 commit comments

Comments
 (0)