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

Add EIP 1024 APDUs #240

Merged
merged 4 commits into from
Apr 7, 2022
Merged

Add EIP 1024 APDUs #240

merged 4 commits into from
Apr 7, 2022

Conversation

btchip
Copy link
Contributor

@btchip btchip commented Feb 1, 2022

Implement EIP 1024 privacy APIs

Test snapshot pending Speculos merge (https://github.com/btchip/speculos/tree/x25519_goodenough) - js support merged in ledgerjs 6.24.0

Copy link
Contributor

@cseguret-ledger cseguret-ledger left a comment

Choose a reason for hiding this comment

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

Some changes to ask.
However it does not have a test

@cseguret-ledger
Copy link
Contributor

@btchip this PR is linked to this issue? : #189

@btchip
Copy link
Contributor Author

btchip commented Mar 3, 2022

Yes, it's related to this PR, but comes from a business need identified recently

@btchip
Copy link
Contributor Author

btchip commented Mar 22, 2022

Ping. Rebased & modified according to review. Synaps is launching Anima soon (https://synaps.io/did-protocol/) and need this feature as soon as possible.

@krasi-georgiev
Copy link

IIUC this implements eth_decrypt and eth_getEncryptionPublicKey methods for the eth ledger app right?

@btchip
Copy link
Contributor Author

btchip commented Mar 24, 2022

@krasi-georgiev that's correct

@krasi-georgiev
Copy link

@btchip thanks,
will be really interesting in testing it, hope @cseguret-ledger finds the time to do the final review soon.

@cseguret-ledger cseguret-ledger self-requested a review April 5, 2022 09:51
@firnprotocol
Copy link

hi @btchip + all, many thanks for the work. i noticed that MetaMask just deprecated this feature (i.e., for non-ledger accounts; it never supported it for ledger accounts).

can i ask whether the version/spec implemented here is identical to that which MetaMask used to support? if not, how does it differ?

it would be very nice if y'all could coordinate with MetaMask, in order to get (a consistently implemented) version of this feature implemented and non-deprecated across both normal and Ledger accounts in MetaMask. happy to help any way I can. thoughts?

@kdenhartog
Copy link

The reason Metamask deprecated this was because reuse of key material across different curves and for different operations (sign with secp256k1 and KEM with curve25519) goes against best practices in crypto and the design wasn't provably secure. I spent some time looking into this and it seems that this implementation is doing the same thing.

There is a few papers that have studied private key material reuse and found some combinations that are secure, but this one hasn't been studied. There's likely a simple way to solve this by inputing the node path data into an HMAC with different data to generate a different private key point, but we'd want to agree that's the path we want to take forward if that's how we'll go.

Who's best on the Ledger team for me to keep in contact with as I write up a new EIP to achieve some similar functionality?

@btchip
Copy link
Contributor Author

btchip commented Jul 18, 2022

I'll link this issue to our team cryptographer tomorrow

@rdubois-crypto
Copy link

Usually in Cryptography, it is recommended to avoid a duplicate use for the same key, for example the ANSSI recommands that no common key shall be used (here, page 21, first 'rule of use' https://www.ssi.gouv.fr/uploads/2014/11/RGS_v-2-0_B2.pdf, excuse my french).

In this case a master key must be used with a key derivation function (KDF) to derivate distinct keys.
A standardized KDF is described here, section 5.1 https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-108.pdf.

PRF may be instantiated using HMAC, thus the resulting construction is equivalent to your suggestion. Being compatible with the standard could benefit the reuse of existing functions in some existing libraries/HSM.

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

Successfully merging this pull request may close these issues.

6 participants