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

MSC1998: Two-Factor Authentication Providers #1998

Open
wants to merge 2 commits into
base: old_master
Choose a base branch
from

Conversation

cyphar
Copy link

@cyphar cyphar commented May 14, 2019

Rendered

Implements: #1997
Signed-off-by: Aleksa Sarai cyphar@cyphar.com

@cyphar cyphar changed the title proposals: add draft MSC1997 proposal MSC1997: Two-Factor Authentication Providers May 14, 2019
@turt2live turt2live added proposal A matrix spec change proposal proposal-in-review labels May 14, 2019
@turt2live
Copy link
Member

@cyphar I've flagged this as ready to review. If that's incorrect, please let me know.

I haven't ready the whole thing yet, but the detail is massively appreciated!

@turt2live turt2live changed the title MSC1997: Two-Factor Authentication Providers MSC1998: Two-Factor Authentication Providers May 14, 2019
@cyphar
Copy link
Author

cyphar commented May 14, 2019

I hope I didn't go overboard, I ended up copying the structure of the actual spec docs but maybe I should've been a bit more terse?

@turt2live turt2live self-requested a review June 12, 2019 01:45
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking pretty good so far!

changelogs/client_server/newsfragments/1998.feature Outdated Show resolved Hide resolved
proposals/1998-two-factor-providers.md Outdated Show resolved Hide resolved
proposals/1998-two-factor-providers.md Outdated Show resolved Hide resolved
proposals/1998-two-factor-providers.md Outdated Show resolved Hide resolved
proposals/1998-two-factor-providers.md Outdated Show resolved Hide resolved
In order to avoid overly-complicating the initial implementation we just
provide a basic TOTP authentication flow and a recovery code flow.

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
@cyphar
Copy link
Author

cyphar commented Jun 12, 2019

@turt2live I've made the corrections you mentioned. One thing with using DELETE is that now it makes slightly less semantic sense to disable only some of the two-factor providers (then again, POST /disable is quite a bit uglier). If only there was a DELETE-PATCH. 😉


* `GET /_matrix/client/r0/account/twoFactor` to get the current state of enabled providers for the account.
* `POST /_matrix/client/r0/account/twoFactor` to configure providers.
* `DELETE /_matrix/client/r0/account/twoFactor` to disable providers.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

turns out I can't read, as vdh points out these should be two_factor: #1998 (comment)

@turt2live
Copy link
Member

@dainnilsson please move your comments to threads on the diff so people can reply.

Comment on lines +146 to +150
### `POST /_matrix/client/r0/account/twoFactor`

This endpoint allows the client to enable (or reset) a two-factor provider. In
order to avoid users locking themselves out of their accounts, servers should
enable `m.login.two-factor.recovery` if it is not already enabled for the user.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some 2FA credential variants will require more round-trips to set up, for example WebAuthn (https://www.w3.org/TR/webauthn/). Does this endpoint allow for that? For example, when setting up a credential the client needs to first get a challenge, and then provide a response based on that challenge to the server. Potentially other protocols might require even more round-trips to complete.

Likewise for TOTP, it's common that as part of credential registration the server requires a valid OTP for the new credential before activating it, to make sure the client has correctly received and supports the parameters used. Perhaps this should be part of the m.login.two-factor.totp registration flow?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps I'm misunderstanding WebAuthn (and I'd love to hear more about it -- I've only interacted with the first-gen FIDO stuff very superficially), isn't it intended to replace passwords as one of the factors of authentication (rather than being an alternative MFA mechanism)? If so, then adding a new login flow (separate to this proposal) would seem to be a better path forward to have WebAuthn supported in Matrix.

But I think you're right that the current design of Matrix's login flow system doesn't allow for multiple ping-pongs between clients and servers. You'd need to create separate login stages (which seems to me to be a little bit scary since forgetting to include a login flow step means that the authentication would succeed if I understand how it works correctly).

Perhaps this should be part of the m.login.two-factor.totp registration flow?

Yes it probably should be, and all MFA registration flows I can think of work that way...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WebAuthn can be used in several ways. Also as single factor. But I believe most common is to use instead of SMS or TOTP codes as second factor.

Comment on lines +146 to +150
### `POST /_matrix/client/r0/account/twoFactor`

This endpoint allows the client to enable (or reset) a two-factor provider. In
order to avoid users locking themselves out of their accounts, servers should
enable `m.login.two-factor.recovery` if it is not already enabled for the user.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One other consideration is that a single 2FA provider may need to support multiple credentials. While I think this might be possible under the current API by leveraging the provider params dict, it does overlay a lot of functionality on a single endpoint. For any given provider we not only have "enable/reset", but also "add credential" and "remove (specific) credential". I'm not sure if this warrants additional endpoints or not.

@dainnilsson
Copy link

@dainnilsson please move your comments to threads on the diff so people can reply.

Done!

@turt2live turt2live added the kind:core MSC which is critical to the protocol's success label Apr 20, 2020
@turt2live turt2live added the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Jun 8, 2021
@warriorzz
Copy link

Any updates on this? I would like to have this merged as I would love to use WebAuthn as second factor when authenticating.

@shoqvalue
Copy link

Same question.

Any updates on this? I would like to have this merged as I would love to use WebAuthn as second factor when authenticating.

@cyphar
Copy link
Author

cyphar commented May 30, 2022

I haven't had much time to review this after I first posted it. It is still a feature I'd like (I still use Matrix daily and wish we'd have 2FA) but it seems that WebAuthn would require a more flexible design than the pretty basic one I came up with here -- this feature would almost certainly turn out better if it's handled by someone with more experience dealing with authentication.

@ShadowJonathan
Copy link
Contributor

ShadowJonathan commented Sep 30, 2022

Isn't this work effectively overshadowed by the OIDC initiative the matrix foundation is currently following?

From what I know, it'd essentially move authentication to somewhere else, which then has to implement webauthn instead. (The focus being that, by default, a server will use matrix-authentication-service as an internal OIDC provider, which already has an issue open about this: matrix-org/matrix-authentication-service#18)

@richvdh
Copy link
Member

richvdh commented Oct 3, 2022

yes, OIDC would obviate the need for this. For now we'll keep it open until we are fully committed to OIDC.

@zincentimeter
Copy link

Is there any follow-up on this feature?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:core MSC which is critical to the protocol's success needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants