feat: passkey strategy with resident keys #3656

hperl · 2023-12-11T06:52:20Z

TODO

Two distinct strategies, how to prevent passkey + webauthn passkey with too high AAL
- Passkey is currently always AAL1 and not a second factor.
- We could disallow using the same authenticator ID (AAGUID) for both webauthn and passkey. However, this could hinder migration from webauthn+passwordless to passkey.
PassKeys what happens to non-resident keys? For example when migrating WebAuthn passwordless credentials to passkey?
- This work does not yet consider converting passwordless credentials to a passkey. I think the best way would be to do this with user interaction and insert a "nag screen" after login to prompt the user to create a passkey (and optionally delete/deactivate the webauth passwordless credential). The same technique could be used to onboard users to passkeys regardless of their primary login strategy.
2x PassKeys if submission fails
- In some cases it can be that the passkey is created in the browser but not stored in the database. Ultimately, there is nothing that we can do to completely prevent this, but is should be uncommon, because we store and validate the traits first before we create the passkey.
WebAuthn.js - use onload for autocomplete
- Done
How will the deprecation work of the webauthn strategy?
- This will need further design and code.
Clean up webauthnIdentifierNode

Related issue(s)

Docs PR: ory/docs#1626

Checklist

I have read the contributing guidelines.
I have referenced an issue containing the design document if my change
introduces a new feature.
I am following the
contributing code guidelines.
I have read the security policy.
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
[email protected]) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature
works.
I have added or changed the documentation.

Further Comments

…strategy

codecov · 2023-12-12T22:49:36Z

Codecov Report

Attention: 192 lines in your changes are missing coverage. Please review.

Comparison is base (21ab031) 79.47% compared to head (3c8fd05) 78.35%.
Report is 41 commits behind head on master.

❗ Current head 3c8fd05 differs from pull request most recent head 5f58a20. Consider uploading reports for the commit 5f58a20 to get more accurate results

Files	Patch %	Lines
selfservice/strategy/passkey/passkey_settings.go	68.83%	36 Missing and 31 partials ⚠️
selfservice/strategy/passkey/passkey_login.go	74.00%	32 Missing and 27 partials ⚠️
...lfservice/strategy/passkey/passkey_registration.go	73.44%	26 Missing and 21 partials ⚠️
selfservice/strategy/webauthn/registration.go	76.08%	9 Missing and 2 partials ⚠️
x/webauthnx/aaguid/aaguid.go	75.00%	2 Missing and 2 partials ⚠️
selfservice/strategy/webauthn/login.go	76.92%	0 Missing and 3 partials ⚠️
persistence/sql/identity/persister_identity.go	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3656      +/-   ##
==========================================
- Coverage   79.47%   78.35%   -1.13%     
==========================================
  Files         356      352       -4     
  Lines       25999    24312    -1687     
==========================================
- Hits        20662    19049    -1613     
+ Misses       3826     3794      -32     
+ Partials     1511     1469      -42

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

embedx/config.schema.json

identity/extension_credentials.go

Co-authored-by: Jonas Hungershausen <[email protected]>

embedx/config.schema.json

aeneasr

Preliminary review - also pushing some code changes now. Can you please add e2e tests for the various flows?

selfservice/strategy/passkey/passkey_registration.go

aeneasr · 2024-01-02T12:53:35Z

selfservice/strategy/passkey/passkey_registration.go

+	if err != nil {
+		return s.handleRegistrationError(w, r, regFlow, params, err)
+	}
+	if params.Register == "" && params.Method != "passkey" {


Is it possible that Register is set to some value, but the method is actually not passkey but e.g. password, and then we use the wrong strategy here?

Not in a benign case, because the register param is set right before submission.

As for an attacker, what would the gain be?

I think the concern is, that params.Register == "values" but params.Method != "passkey".

Which would lead to this strategy being executed, even though the method specifies something else. IMO, we should always return ErrStrategyNotResponsible if params.Method != "passkey"

Requiring the method to be set will further complicate submitting the passkey, which is done through a form submit in JS, see:

kratos/x/webauthnx/js/webauthn.js

Line 121 in 96c2164

document.querySelector(triggerQuerySelector).closest("form").submit()

We don't require the method to be set for webauthn register requests either, btw:

kratos/selfservice/strategy/webauthn/registration.go

Lines 94 to 114 in 96c2164

func (s *Strategy) Register(w http.ResponseWriter, r *http.Request, regFlow *registration.Flow, i *identity.Identity) (err error) {

ctx := r.Context()

if regFlow.Type != flow.TypeBrowser || !s.d.Config().WebAuthnForPasswordless(r.Context()) {

return flow.ErrStrategyNotResponsible

}

var p updateRegistrationFlowWithWebAuthnMethod

if err := s.decode(&p, r); err != nil {

return s.handleRegistrationError(w, r, regFlow, &p, err)

}

regFlow.TransientPayload = p.TransientPayload

if err := flow.EnsureCSRF(s.d, r, regFlow.Type, s.d.Config().DisableAPIFlowEnforcement(ctx), s.d.GenerateCSRFToken, p.CSRFToken); err != nil {

return s.handleRegistrationError(w, r, regFlow, &p, err)

}

if len(p.Register) == 0 {

return flow.ErrStrategyNotResponsible

}

Specifically, I could add a hidden input with name=method and value=passkey in JS. Is it worth it?

I'd say it's worth the effort also submitting the method, since we do have the (sometimes loose) convention to submit the method as well. Since this would just be another edge case, potentially causing issues down the road.

But, in the interest of velocity, I'll leave the decision up to you.

aeneasr · 2024-01-02T12:57:51Z

selfservice/strategy/passkey/passkey_registration.go

+	credentialWebAuthn := identity.CredentialFromWebAuthn(credential, true)
+	credentialsConfig, err := json.Marshal(identity.CredentialsWebAuthnConfig{
+		Credentials: identity.CredentialsWebAuthn{*credentialWebAuthn},
+		UserHandle:  webAuthnSess.UserID,


What about the display name? Do we not need it because it is a PassKey?

The display name of the key will be derived from the AAGUID, see here:

kratos/identity/credentials_webauthn.go

Lines 36 to 39 in 60a1817

id := aaguid.Lookup(credential.Authenticator.AAGUID)

if id != nil {

cred.DisplayName = id.Name

}

aeneasr · 2024-01-02T13:03:54Z

selfservice/strategy/passkey/passkey_registration.go

+
+	ident.UpsertCredentialsConfig(s.ID(), credentialsConfig, 1)
+	passkeyCred, _ := ident.GetCredentials(s.ID())
+	passkeyCred.Identifiers = []string{string(webAuthnSess.UserID)}


I think we should return an easy to understand error if webAuthnSess.UserID is empty. If it is empty, the current validation will pass it as valid

c := i.GetCredentialsOr(identity.CredentialsTypePasskey, &identity.Credentials{}) if len(c.Identifiers) == 0 { return schema.NewMissingIdentifierError() }

and will lead to an empty string identifier. We had this problem once before in the WebAuthn code.

In this case, webAuthnSess.UserID is not user-supplied. It comes from the internal context, which is set here:

kratos/selfservice/strategy/passkey/passkey_registration.go

Lines 295 to 303 in 60a1817

user := &webauthnx.User{

Name: identifier,

ID: []byte(randx.MustString(64, randx.AlphaNum)),

Config: s.d.Config().PasskeyConfig(ctx),

}

option, sessionData, err := webAuthn.BeginRegistration(user)

if err != nil {

return errors.WithStack(err)

}

I can add another check to make sure it really is not empty, but it would be an internal server error, as the user can't do anything against it.

selfservice/strategy/passkey/passkey_login.go

…strategy

jonas-jonas

LGTM, just a few minor remarks.

embedx/config.schema.json

jonas-jonas · 2024-01-10T13:56:41Z

selfservice/strategy/passkey/passkey_registration.go

+	if err != nil {
+		return s.handleRegistrationError(w, r, regFlow, params, err)
+	}
+	if params.Register == "" && params.Method != "passkey" {


I think the concern is, that params.Register == "values" but params.Method != "passkey".

Which would lead to this strategy being executed, even though the method specifies something else. IMO, we should always return ErrStrategyNotResponsible if params.Method != "passkey"

selfservice/strategy/passkey/passkey_registration.go

selfservice/strategy/passkey/passkey_schema_extension.go

selfservice/strategy/passkey/passkey_strategy.go

test/e2e/cypress/integration/profiles/passwordless/flows.spec.ts

x/webauthnx/js/webauthn.js

aeneasr · 2024-01-12T12:45:05Z

@jonas-jonas are you good with the current state?

jonas-jonas · 2024-01-12T12:47:21Z

@aeneasr I'll re-review now.

jonas-jonas

Except for #3656 (comment) this LGTM.

hperl · 2024-01-15T12:47:55Z

@jonas-jonas, @aeneasr, two points remain open:

As I started writing docs, I stumbled over the fact that users need to modify the identity schema to add a passkey: { identifier: true } to their schema, most likely to the same identifier as used for webauthn. Do you think there is even a use case for using different identifiers for webauthn and passkey?
- Resolved, I now also accept webauthn: { identifier: true }, which makes the whole thing backwards-compatible. Thanks @zepatrik for the discussion.
I can't get the coverage up further. Some errors only trigger when the database misbehaves, which would require extensive mocking. How should I proceed? The remaining tests would look more and more artifical.
- Resolved, we'll merge anyways.

hperl · 2024-02-01T08:57:04Z

I'd like to have one more pair of eyes on the security side of this before merging. @zepatrik maybe?

aeneasr · 2024-02-02T11:15:38Z

I found a few bugs when enabling both webauthn and passkey strategies, probably the triggers get confused. Wrote those things in Slack - would be good to get those fixed in my view :)

hperl · 2024-02-08T15:47:38Z

included in #3748

hperl added 16 commits October 30, 2023 07:48

WIP

5ce38ce

WIP

95a1c92

WIP

4a4bad4

WIP: passkey registration

ef339c9

WIP registration

71c76d5

WIP: login

bb16686

WIP: passwordless login

03c1424

WIP: settings

309ac30

test: update snapshots

b190bfa

fix passkey registration

b35a205

test: add registration tests

f47ceea

test: add login tests

82839e1

WIP: settings tests

41f28f0

chore: cleanup

ace0a57

make all tests pass

599ba09

make all tests pass

3d38b98

hperl marked this pull request as ready for review December 12, 2023 08:33

hperl requested review from aeneasr and zepatrik as code owners December 12, 2023 08:33

chore: formatting

d8f024c

hperl requested a review from jonas-jonas December 12, 2023 08:39

Merge remote-tracking branch 'origin/master' into hperl/passwordless-…

97685a8

…strategy

hperl self-assigned this Dec 12, 2023

chore: lint

08ef9de

jonas-jonas reviewed Dec 15, 2023

View reviewed changes

embedx/config.schema.json Outdated Show resolved Hide resolved

identity/extension_credentials.go Outdated Show resolved Hide resolved

Update embedx/config.schema.json

21ec7cb

Co-authored-by: Jonas Hungershausen <[email protected]>

jonas-jonas reviewed Dec 18, 2023

View reviewed changes

embedx/config.schema.json Show resolved Hide resolved

aeneasr reviewed Jan 2, 2024

View reviewed changes

aeneasr changed the title ~~feat: passwordless strategy~~ feat: PassKey strategy with resident keys Jan 3, 2024

hperl requested a review from aeneasr January 10, 2024 12:56

Merge remote-tracking branch 'origin/master' into hperl/passwordless-…

62c6118

…strategy

jonas-jonas requested changes Jan 10, 2024

View reviewed changes

hperl added 2 commits January 12, 2024 11:32

fix: submit form instead of clicking

53e5e55

chore: update snapshots

96c2164

aeneasr previously approved these changes Jan 12, 2024

View reviewed changes

x/webauthnx/js/webauthn.js Outdated Show resolved Hide resolved

x/webauthnx/js/webauthn.js Outdated Show resolved Hide resolved

jonas-jonas previously approved these changes Jan 12, 2024

View reviewed changes

test: improve coverage

4321e75

hperl dismissed stale reviews from jonas-jonas and aeneasr via 4321e75 January 15, 2024 09:24

hperl added 3 commits January 15, 2024 10:30

test: improve coverage

645e831

test: improve coverage

97ec49f

test: fix

27e242d

hperl and others added 4 commits January 15, 2024 14:40

fix: coverage

3261470

fix: mysql tests

85d9ce5

fix: identity schema

3d2a4cb

fix: key for test fixture

3c8fd05

jonas-jonas previously approved these changes Jan 19, 2024

View reviewed changes

hperl added 3 commits February 5, 2024 09:55

WIP: config

8a10153

tweak quickstart

34f48d2

test: add e2e test with both webauthn pwless and passkey

471157a

hperl dismissed jonas-jonas’s stale review via 471157a February 6, 2024 13:05

feat: also handle webauthn passwordless in passkey strategy

5f58a20

hperl closed this Feb 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: passkey strategy with resident keys #3656

feat: passkey strategy with resident keys #3656

hperl commented Dec 11, 2023 •

edited

Loading

codecov bot commented Dec 12, 2023 •

edited

Loading

aeneasr left a comment

aeneasr Jan 2, 2024

hperl Jan 9, 2024

jonas-jonas Jan 10, 2024

hperl Jan 12, 2024

hperl Jan 12, 2024

jonas-jonas Jan 12, 2024

aeneasr Jan 2, 2024

hperl Jan 9, 2024

aeneasr Jan 2, 2024

hperl Jan 9, 2024

jonas-jonas left a comment

jonas-jonas Jan 10, 2024

aeneasr commented Jan 12, 2024

jonas-jonas commented Jan 12, 2024

jonas-jonas left a comment

hperl commented Jan 15, 2024 •

edited

Loading

hperl commented Feb 1, 2024

aeneasr commented Feb 2, 2024

hperl commented Feb 8, 2024

	func (s Strategy) Register(w http.ResponseWriter, r http.Request, regFlow registration.Flow, i identity.Identity) (err error) {
	ctx := r.Context()

	if regFlow.Type != flow.TypeBrowser \|\| !s.d.Config().WebAuthnForPasswordless(r.Context()) {
	return flow.ErrStrategyNotResponsible
	}

	var p updateRegistrationFlowWithWebAuthnMethod
	if err := s.decode(&p, r); err != nil {
	return s.handleRegistrationError(w, r, regFlow, &p, err)
	}

	regFlow.TransientPayload = p.TransientPayload

	if err := flow.EnsureCSRF(s.d, r, regFlow.Type, s.d.Config().DisableAPIFlowEnforcement(ctx), s.d.GenerateCSRFToken, p.CSRFToken); err != nil {
	return s.handleRegistrationError(w, r, regFlow, &p, err)
	}

	if len(p.Register) == 0 {
	return flow.ErrStrategyNotResponsible
	}

	id := aaguid.Lookup(credential.Authenticator.AAGUID)
	if id != nil {
	cred.DisplayName = id.Name
	}

	user := &webauthnx.User{
	Name: identifier,
	ID: []byte(randx.MustString(64, randx.AlphaNum)),
	Config: s.d.Config().PasskeyConfig(ctx),
	}
	option, sessionData, err := webAuthn.BeginRegistration(user)
	if err != nil {
	return errors.WithStack(err)
	}

feat: passkey strategy with resident keys #3656

feat: passkey strategy with resident keys #3656

Conversation

hperl commented Dec 11, 2023 • edited Loading

TODO

Related issue(s)

Checklist

Further Comments

codecov bot commented Dec 12, 2023 • edited Loading

Codecov Report

aeneasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonas-jonas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeneasr commented Jan 12, 2024

jonas-jonas commented Jan 12, 2024

jonas-jonas left a comment

Choose a reason for hiding this comment

hperl commented Jan 15, 2024 • edited Loading

hperl commented Feb 1, 2024

aeneasr commented Feb 2, 2024

hperl commented Feb 8, 2024

hperl commented Dec 11, 2023 •

edited

Loading

codecov bot commented Dec 12, 2023 •

edited

Loading

hperl commented Jan 15, 2024 •

edited

Loading