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

Added SIP-5 #65

Merged
merged 2 commits into from
Oct 25, 2022
Merged

Added SIP-5 #65

merged 2 commits into from
Oct 25, 2022

Conversation

Vid201
Copy link
Contributor

@Vid201 Vid201 commented Oct 20, 2022

SIP-5: Creating JSON Web Tokens (JWTs)

#62

@Vid201 Vid201 changed the title Added SIP-5 (https://github.com/MetaMask/SIPs/issues/62) Added SIP-5 Oct 20, 2022
SIPS/sip-5.md Outdated Show resolved Hide resolved
SIPS/sip-5.md Outdated Show resolved Hide resolved

## Motivation

Snaps were introduced to extend the functionalities of MetaMask to support more use cases and paradigms. MetaMask currently provides several RPC methods for digitally signing data (described [here](https://docs.metamask.io/guide/signing-data.html)), such as personal_sign and signTypedData. All methods add the prefix "\x19Ethereum Signed Message:\n" to the data, as described in [EIP 191](https://eips.ethereum.org/EIPS/eip-191) and [EIP 712](https://eips.ethereum.org/EIPS/eip-712), which prevents several attack vectors, most notably impersonating transactions. But this also prevents the ability to produce pure signatures over the data, which is needed for other internet data standards (such as JWTs). Actually, a pure signature is possible only with eth_sign, but that method was deprecated and advised not to use. Therefore, there is no safe way to create JWTs containing signatures signed by MetaMask accounts. This SIP proposes a new RPC method for Snaps that enables the creation of signed JWTs.
Copy link
Member

@ritave ritave Oct 25, 2022

Choose a reason for hiding this comment

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

Can you explain the rationale of why the snap itself can't calculate JWT cookies using snap_getBip44Entropy permission?

The above function gives you the raw, extended, private key that is used inside MetaMask itself. With that entropy you can do calculations inside of the snap, without needing additional support from the extension.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is possible. There are two ways to do it without additional support:

  1. Using the RPC method eth_sign, but this method is deprecated, and the user can see a red notice when signing.
  2. Exporting private key with snap_getBip44Entropy, but then the user must confirm that it allows Snap to export private key (when installing Snap) from "main" MetaMask to the Snap. But this would look suspicious to users and prevent them from using the Snap. We also need to sign with the same private key as used on the Ethereum, which means Snap has complete access to the funds on the mainnet.

Understandably, MetaMask and Snap JSON-RPC API cannot support all signing methods, but JWTs are one of the most used standards on the internet, and many Snaps could reuse this functionality.

Copy link
Member

@ritave ritave left a comment

Choose a reason for hiding this comment

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

From the perspective of the Snaps team, this functionality should be handled by the snap_getBip44Entropy.

On the other hand, this SIP has technical merits and so I will approve and merge this proposal on the editor's grounds. I'd like to see if any discussions breaks up concerning this change. If the community supports this change, it will be considered!

Keep in mind, that if no support from the community occurs, there's a high probability the Snaps team will not spend time to implement this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants