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

MSC3782: Matrix public key login spec #3782

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

tak-hntlabs
Copy link

@tak-hntlabs tak-hntlabs commented Apr 21, 2022

MSC3782 Add public/private key login as a new authentication type

Cryptographic signatures have played a pivotal role in blockchain. They are used to prove ownership of a public address without exposing its private key. This property can be (and has been) used for authentication. A server can use signature verification to prove that the user has possession of the private key to allow the user with that public address to log in.

The proposal is to add a new login type for public key cryptography. Under this new login type, a specific implementation can be supported, such as Ethereum and its signature algorithm - the Elliptic Curve Digital Signature Algorithm (ECDSA).

This new login type would allow any matrix server to authenticate a user without having to redirect the user to a central identity service.

Dendrite implementation: https://github.com/matrix-org/dendrite/pull/2366/files

Signed-off-by: Tak Wai Wong tak@hntlabs.com

@tak-hntlabs tak-hntlabs changed the title Matrix public key login spec MSC3782: Matrix public key login spec Apr 21, 2022
@uhoreg uhoreg added proposal A matrix spec change proposal client-server Client-Server API kind:feature MSC for not-core and not-maintenance stuff needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels Apr 21, 2022
@uhoreg
Copy link
Member

uhoreg commented Apr 21, 2022

You mentioned an implementation in #spec:matrix.org. Can you add a link to your implementation?

Copy link
Member

@uhoreg uhoreg left a comment

Choose a reason for hiding this comment

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

Some housekeeping comments

proposals/3782-matrix-publickey-login-spec.md Outdated Show resolved Hide resolved
proposals/3782-matrix-publickey-login-spec.md Show resolved Hide resolved
This gives the server the ability to raise the bar when it needs to elevate its security requirements.

This spec is version 1.

Copy link
Member

Choose a reason for hiding this comment

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

This proposal should include Security considerations and Unstable prefix sections.

Also, this seems like something that could also be done via an SSO provider that handles the handshaking rather than the Matrix client. What would be the advantage of this over an SSO provider? A short explanation might be worthwhile to put in an Alteratives section.

Copy link
Author

Choose a reason for hiding this comment

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

Good feedback. I'll add those sections, explaining the investigation I've done.

Copy link
Author

Choose a reason for hiding this comment

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

Added Alternatives, and Security Considerations. I will add the Unstable prefix requirements, and update my implementation next week. Have a great weekend, and thanks for the feedback!

Copy link
Author

Choose a reason for hiding this comment

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

@uhoreg wrt to the Unstable section, do I need the Unstable section, and add "/unstable" endpoints if I do the following:

  • hold all the changes in my private branch and keep it updated until the spec is finalized and approved
  • In my implementation, I have config yaml that turns on / off the new feature. It is off by default.
  • I have the only client that can login with the new public key login type. No other clients should be affected if the spec and the implementation changes.

Copy link
Member

Choose a reason for hiding this comment

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

Technically, I think our process says that you don't need it. But I think it's good practice to do so anyways.

@tak-hntlabs
Copy link
Author

You mentioned an implementation in #spec:matrix.org. Can you add a link to your implementation?

Oh sure. I need to refresh with latest dendrite in my fork. I'll post a link here when I'm done resolving all the merge conflicts.

The client is built on top of matrix-javascript-sdk. It's sending the request / response auth data using the matrix client to the /login and the /register endpoints. Because it has a lot of app-related stuff, not sure how useful it'll be. Do you want to see the client-side changes? It's in our private repo. If you want to see the the client-side changes, I'll create a public repo and push the changes there.

@tak-hntlabs
Copy link
Author

tak-hntlabs commented Apr 21, 2022

spec:matrix

Implementation: https://github.com/matrix-org/dendrite/pull/2366/files

Comment on lines 127 to 129
Client sends a HTTP POST request to the server endpoint ```/login``` with no ```auth``` parameter. It should get back a
HTTP/1.1 401 Unauthorized response with a JSON body detailing the supported login flows, params, and a session ID. The
semantics of the JSON response body is explained in the next section.
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently /login does not support UIA, does this MSC depend on #2835 ?

Copy link
Author

@tak-hntlabs tak-hntlabs Apr 22, 2022

Choose a reason for hiding this comment

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

Refer to [Client-server API](https://spec.matrix.org/v1.2/client-server-api/#post_matrixclientv3register).

Client sends a HTTP POST message to the endpoint ```/register``` with a ```username``` and an ```auth``` JSON body.

Copy link
Contributor

Choose a reason for hiding this comment

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

What if a user wants to change their password/key pair? Is that intended to be unsupported or how does this extend the endpoints to change your password?

Copy link
Author

@tak-hntlabs tak-hntlabs Apr 25, 2022

Choose a reason for hiding this comment

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

This is mentioned in the Private key security section. It relies on software wallet protection, as well as hardware wallet protection. If those protections are compromised, this proposal does not address the issue. I left it as an open issue for another proposal to address.


```json
{
"username": "0x...",
Copy link

Choose a reason for hiding this comment

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

It looks like this proposal fundamentally hinges on using cryptocurrency addresses as usernames. That should be made much clearer much earlier on, in the high-level summary of this proposal. It also raises a couple of issues:

  • Currently, anyone can register an account on any homeserver with a username of the form 0x.... A spec change that treats those identifiers as cryptocurrency addresses, and allows the owners of those addresses to access those accounts, has serious security implications that this proposal doesn't address.
  • More generally, using cryptocurrency addresses as usernames, and looking up those addresses to obtain public keys, feels solidly outside the scope of a general "public key login" MSC, and should really be a dedicated "cryptocurrency login" spec proposal. This is especially true because you don't propose any mechanism for linking cryptographic public keys to MXIDs in the general case.

Copy link
Author

@tak-hntlabs tak-hntlabs Apr 25, 2022

Choose a reason for hiding this comment

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

A new user needs to have the private key in order to register for an account because the signature will be validated as part of the registration process. Registration fails if the signature verification fails. It can't be any person who knows what the public address is. This is described in the End-to-end registration flow

The original intent is to propose a general public key cryptography login, which then allows for a more specific choice like Ethereum and its use of the ECDSA. It is not just for the blockchain community. If you feel that the proposal should be narrowed down to focus on the specific community, I can update the proposal to narrow it down. Can you help me out here? What spec process should I follow if this is not a considered an MSC spec proposal?

This scheme does not require a public address lookup. This proposal is based on the Ethereum spec Ethereum address. See section under EXTERNALLY-OWNED ACCOUNTS AND KEY PAIRS and ACCOUNT CREATION". Quoting from the spec:

"The public key is generated from the private key using the Elliptic Curve Digital Signature Algorithm. You get a public address for your account by taking the last 20 bytes of the Keccak-256 hash of the public key and adding 0x to the beginning."

The public address is recovered from the signature during validation. The server need not do any address lookup.

At the end of the registration process, the public address maps to the localpart as an "m.id.user". This is described in the section Address as localpart.

Copy link

@duxovni duxovni Apr 25, 2022

Choose a reason for hiding this comment

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

A new user needs to have the private key in order to register for an account because the signature will be validated as part of the registration process. Registration fails if the signature verification fails. It can't be any person.

Yes, if a user uses this authentication mechanism while registering their account, they can only choose this username if they own the address in question. But most of the other authentication mechanisms, such as password authentication, don't impose any restrictions on usernames. If I register an account using password authentication, then I can pick whatever username i want, including one that happens to be someone else's Ethereum address. Essentially, this authentication mechanism is mutually exclusive with pretty much all other authentication mechanisms; if a server accepts other authentication mechanisms, it can't accept this one, and if it accepts this authentication mechanism, it can't accept most others. If that's the intent, then that needs to be made much clearer.

The original intent is to propose a general public key cryptography login, which then allows for a more specific choice like Ethereum and its use of the ECDSA. It is not just for the blockchain community. If you feel that the proposal should be narrowed down to focus on the specific community, I can update the proposal to narrow it down. Can you help me out here? What spec process should I follow if this is not a considered an MSC spec proposal?

The issue I'm pointing out is that if you remove all the sections of this proposal that are specifically about Ethereum, what's left isn't a usable proposal, because it doesn't provide any general method of associating public keys and MXIDs. If the intent is that every implementation of this proposal requires using public keys as usernames, then that also needs to be made much clearer, and you need to specify how a public key is converted to a username in non-cryptocurrency scenarios.

Copy link

Choose a reason for hiding this comment

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

Also, if the username is just the last 20 bytes of the public key hash, and no blockchain lookups are required, then why is a chain ID being sent as part of the authentication exchange?

Copy link
Author

Choose a reason for hiding this comment

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

A new user needs to have the private key in order to register for an account because the signature will be validated as part of the registration process. Registration fails if the signature verification fails. It can't be any person.

Yes, if a user uses this authentication mechanism while registering their account, they can only choose this username if they own the address in question. But most of the other authentication mechanisms, such as password authentication, don't impose any restrictions on usernames. If I register an account using password authentication, then I can pick whatever username i want, including one that happens to be someone else's Ethereum address. Essentially, this authentication mechanism is mutually exclusive with pretty much all other authentication mechanisms; if a server accepts other authentication mechanisms, it can't accept this one, and if it accepts this authentication mechanism, it can't accept most others. If that's the intent, then that needs to be made much clearer.

Ah, got it. Now I understand your point. In my dendrite implementation, I had actually added config to give the admin the option to turn off password authentication for that reason. But I wasn't quite sure what it means to have such restrictions at the spec level. Now that you've called it out, it made me think deeper about the implications. I agree with you that if public address is to be used as the user id, then such restriction is necessary to prevent auth issues. I will update the proposal. Thanks for that insight.

The original intent is to propose a general public key cryptography login, which then allows for a more specific choice like Ethereum and its use of the ECDSA. It is not just for the blockchain community. If you feel that the proposal should be narrowed down to focus on the specific community, I can update the proposal to narrow it down. Can you help me out here? What spec process should I follow if this is not a considered an MSC spec proposal?

The issue I'm pointing out is that if you remove all the sections of this proposal that are specifically about Ethereum, what's left isn't a usable proposal, because it doesn't provide any general method of associating public keys and MXIDs. If the intent is that every implementation of this proposal requires using public keys as usernames, then that also needs to be made much clearer, and you need to specify how a public key is converted to a username in non-cryptocurrency scenarios.

Yes, I will make it clearer. In the spec, the mapping from public address to username was mentioned at the end in the Address as localpart section. I will move it earlier in the proposal and make that clear.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for sharing all the resources. These are excellent. I will take a deeper dive into each of them. The reason for not adopting the blockchain account ID in this proposal is because Matrix user's identity has a specific definition. It is expressed as @localpart:domain with this
localpart grammar.

Snippet:

Identifiers must start with one of the characters [a-z], and be entirely composed of the characters [a-z], [0-9], -, _ and .

There is a spec, I believe, to support other identity grammar. I can't find the proposal at the moment. But based on my understanding of the existing codebase, that implementation will be quite a significant change to maintain backward compatibility.

My plan is to use the version number as a way to move forward. i.e. in version 1, support current user identity format per Matrix spec. Then, when the "support other identity grammar" is implemented in Matrix, follow up with a version 2 implementation that uses blockchain ID standard.

Choose a reason for hiding this comment

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

The reason for not adopting the blockchain account ID in this proposal is because Matrix user's identity has a specific definition. It is expressed as @localpart:domain with this
localpart grammar.

The spec also provides recommendations for mapping other chartacters to characters from the allowed set:

  1. Encode any remaining bytes outside the allowed character set, as well as =, as their hexadecimal value, prefixed with =.

If I understand the recommendation correctly, that means : can be mapped to =3a, so CAIP-10 account ID translates into this localpart:

@eip155=3a1=3a0xab16a96d359ec26a11e2c2b3d8f8b8942d5bfcdb

Copy link
Author

Choose a reason for hiding this comment

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

I think you are right. Looking at the parsing logic for userid in the server, it should work as long as there is one and only one :

Copy link
Author

Choose a reason for hiding this comment

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

Quick update. I'm working on incorporating the following feedback:

  1. Implement EIP-4361
  2. Implement CAIP-2 decentralized ID

EIP-4361 is implemented in a branch. Working on the other task. I will update this spec once I'm done with implementation.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for all of your valuable feedback @silverpill , @duxovni , @uhoreg , @deepbluev7 . I have updated the proposal based on your feedback. When you get a chance, please have a look and let me know your thoughts.

These sections have the most significant changes in the latest revision:

There are smaller updates to the rest of the spec to align with the above changes.

In terms of the process, what should I do next?

### Message for the user to sign

The message template is a simplified version of [EIP-4361](https://eips.ethereum.org/EIPS/eip-4361). The prescriptive
nature of the format is intended for regular expression extraction during server validation.

Choose a reason for hiding this comment

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

Why custom message format was chosen instead of EIP-4361?
This makes the proposed auth method incompatible with existing EIP-4361 implementations in JavaScript, Python, Go and other languages (listed here https://docs.login.xyz/).
I think at some point wallets could natively support EIP-4361 and perform additional security checks (e.g. domain validation), that's another reason to stick with the standard.

Copy link
Author

Choose a reason for hiding this comment

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

Great point! I'll change my implementation to see how this works, and then update the spec.

@tak-hntlabs
Copy link
Author

tak-hntlabs commented Aug 9, 2022 via email

@uhoreg
Copy link
Member

uhoreg commented Aug 10, 2022

In general, if you think that a proposal is ready for FCP, you would let the spec core team know that in #sct-office:matrix.org.

However, I don't think this proposal has had enough initial review for FCP. (Unfortunately, I haven't had time to take another look at it. I don't know if anyone else has.) Also, it won't enter FCP until there is a suitable implementation that allows us to test it. I see that you have a server-side implementation in Dendrite, but we would also need some sort of client-side implementation.

Finally, we are aiming to replace Matrix's authentication system with Open ID Connect (OIDC), so that instances can use different authentication methods without having to add Matrix-specific things, so that we don't have to make every client implement each new authentication method. So it's unlikely that we would accept any new authentication methods into the spec, and it may be best to try to figure out how this login method could work with OIDC. There may still be changes that you'd want to make to Matrix, but I'd suggest reworking this in the context of using OIDC.

@tak-hntlabs
Copy link
Author

tak-hntlabs commented Aug 10, 2022 via email

@uhoreg
Copy link
Member

uhoreg commented Aug 11, 2022

You can find more information about it (MSCs, discussion room, progress, etc) at https://areweoidcyet.com/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API kind:feature MSC for not-core and not-maintenance stuff 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.

6 participants