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

fix wrong EC_PARAMETERS #123

Closed
wants to merge 2 commits into from
Closed

Conversation

lau-bin
Copy link

@lau-bin lau-bin commented Jul 10, 2022

Wrong elliptic curve parameters cause the "generate" command to fail with an InvalidEncoding exception

@lau-bin lau-bin requested a review from a team as a code owner July 10, 2022 22:06
@ghost
Copy link

ghost commented Jul 10, 2022

Dear @lau-bin,

In order to potentially merge your code in this open-source repository and therefore proceed with your contribution, we need to have your approval on DFINITY's CLA1.

If you decide to agree with it, please visit this issue and read the instructions there.

— The DFINITY Foundation

Footnotes

  1. Contributor License Agreement

@ghost
Copy link

ghost commented Jul 10, 2022

Dear @lau-bin,

In order to potentially merge your code in this open-source repository and therefore proceed with your contribution, we need to have your approval on DFINITY's CLA1.

If you decide to agree with it, please visit this issue and read the instructions there.

— The DFINITY Foundation

Footnotes

  1. Contributor License Agreement

@ghost ghost added the cla:pending label Jul 10, 2022
@sesi200
Copy link
Contributor

sesi200 commented Jul 12, 2022

Hi @lau-bin, thank you very much for your PR. I'll ping the relevant people so they can review this.

@randombit
Copy link

This change seems correct to me. The DER encoding 06042b8104000a (what is currently used) is not valid DER at all. In contrast 06052b8104000a is the correct DER encoding for the OID 1.3.132.0.10 (secp256k1)

AIUI this "EC PARAMETERS" block is just being printed out for the user during key derivation, and the variable is not used outside of that context. So it seems safe enough to change. Certainly no other software will be able to correctly interpret the value we are printing right now.

@lwshang
Copy link
Contributor

lwshang commented Jul 12, 2022

FYI, on the agent-rs side (used by dfx), such EC PARAMETER was also hard-coded:
https://github.com/dfinity/agent-rs/blob/main/ic-agent/src/identity/secp256k1.rs#L37

And thank you @randombit for pointing out that such bytes are DER encoding of OID.
My plan is to merge this quick fix and make a release soon.
Then I may refactor the code for better maintainability.

@lwshang
Copy link
Contributor

lwshang commented Jul 12, 2022

Hi @lau-bin, would you mind approve DFINITY's CLA? So that I can merge this work.

@lwshang lwshang mentioned this pull request Jul 13, 2022
@lwshang
Copy link
Contributor

lwshang commented Jul 13, 2022

Close by #124

@lwshang lwshang closed this Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants