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.

Sign up for GitHub

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

Jump to bottom

Darc ECDSA #2484

Open
wants to merge 11 commits into
base: darc_identity_test
Choose a base branch
from
Open

Darc ECDSA #2484

wants to merge 11 commits into from

Conversation

pm-duokey
Copy link

What this PR does

See commit message
This PR

🙅‍ Friendly checklist:

  • 0. Code comments are added (or updated) when/where needed and explain the WHY of the code.
  • 1. Design choices, user documentation and any additional doc are added (or updated) in READMEs.
  • 2. Any new behaviour is tested and small units of code that can be are unit tested.
  • 3. Code comments are added on tests to explain what they do.
  • 4. Errors are systematically wrapped with a meaningful message using xerrors.Errorf and the %v verb.
  • 5. Hard limit of 80 chars is always respected.
  • 6. Changes are backward compatible.
  • 7. Indentation level does not exceed 5, although 4 is already suspicious.
  • 8. Functions, files, and packages are kept to a manageable size and decomposed into smaller units if needed.
  • 9. There are no magic values.

ineiti
ineiti requested changes Dec 21, 2021
View reviewed changes
Copy link
Member

@ineiti ineiti left a comment

Choose a reason for hiding this comment

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

Thanks for this PR. I think the most difficult bit is the naming :) You called it ECDSA, but I think it would be more appropriate to call it something like MPCECDSA, or TSMECDSA or such.

darc/darc.go
@@ -964,6 +973,10 @@ func (id Identity) GetPublicBytes() []byte {
return buf
case 4:
return id.EvmContract.Address[:]
case 5:
buf := elliptic.Marshal(id.ECDSA.PublicKey.Curve, id.ECDSA.PublicKey.X, id.ECDSA.PublicKey.Y)
//TODO: add error check here?
Copy link
Member

Choose a reason for hiding this comment

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

What kind of error check can you do here? The GetPublicBytes doesn't return an error, so all you could do is panic.

darc/darc.go Outdated
}
}

//TODO make calls to tsm available
Copy link
Member

Choose a reason for hiding this comment

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

Please resolve all TODOs.

From what I understand, the Verify method will not call to the TSM, no?

darc/darc.go
Comment on lines +1179 to +1180
//necessary function, needs to be refactored only supports elliptic.P256 curve
//needs to be tested Unmarshal might not work
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//necessary function, needs to be refactored only supports elliptic.P256 curve
//needs to be tested Unmarshal might not work
// Tries to convert a string into a sec256k1 point.
// TODO: needs to be tested Unmarshal might not work

darc/darc.go Outdated
@@ -790,6 +793,8 @@ func (s Signer) Identity() Identity {
return NewIdentityProxy(s.Proxy)
case 4:
return NewIdentityEvmContract(s.EvmContract)
case 5:
return NewIdentityECDSA(s.ECDSA.PublicKey)
Copy link
Member

Choose a reason for hiding this comment

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

Here and everywhere else: we should call the ECDSA identity MPCECDSA to indicate we're doing something special..

darc/darc.go
Comment on lines +1182 to +1183
id := make([]byte, hex.DecodedLen(len(in)))
_, err := hex.Decode(id, []byte(in))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
id := make([]byte, hex.DecodedLen(len(in)))
_, err := hex.Decode(id, []byte(in))
buf, err := hex.DecodeString(in)
if err != nil {
return xerrors.Errorf("couldn't parse hex-string: %v", err)
}

darc/darc.go
Comment on lines +1192 to +1194
if err != nil {
return Identity{}, err
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if err != nil {
return Identity{}, err
}

Treat the error as close as possible to the source. Else it is very confusing.

darc/darc.go Outdated
@@ -1447,6 +1500,22 @@ func (kcs SignerX509EC) Sign(msg []byte) ([]byte, error) {
return nil, errors.New("not yet implemented")
}

// new signer creates a signer only with a public key used to verify signatures
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// new signer creates a signer only with a public key used to verify signatures
// NewSignerECDSA only takes a public key as the MPC is needed to sign data.

darc/darc.go Outdated
}}
}

//TODO
Copy link
Member

Choose a reason for hiding this comment

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

One way to implement sign would be to give the necessary configuration when calling NewSignerECDSA, and then having Sign call the MPC backend. But that would introduce a dependency on the MPC code that we probably don't want to have in here.

Suggested change
//TODO
// Sign cannot be implemented, as only the MPC can sign a message.

darc/darc_test.go Outdated
msg := []byte(`Hello World`)

//Signature from code example go-tsm-sdk corresponding to ecdsa public key example
signed, _ := hex.DecodeString("304402204f0b20a44efacec7b0514683233a79552026fe80e468078f6fed6cfe3f3e8a0402201eb12db7f6fe0828cafe8b0a032a37ff377b342799cfe77cfbac40c8ec1fa9e8")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
signed, _ := hex.DecodeString("304402204f0b20a44efacec7b0514683233a79552026fe80e468078f6fed6cfe3f3e8a0402201eb12db7f6fe0828cafe8b0a032a37ff377b342799cfe77cfbac40c8ec1fa9e8")
signed, err := hex.DecodeString("304402204f0b20a44efacec7b0514683233a79552026fe80e468078f6fed6cfe3f3e8a0402201eb12db7f6fe0828cafe8b0a032a37ff377b342799cfe77cfbac40c8ec1fa9e8")
require.NoError(t, err)

ALWAYS do error checking! You will save yourself a lot of pain!

darc/darc_test.go
Comment on lines +770 to +771
var x, _ = new(big.Int).SetString("25613385885653880697990944418179706546134037329992108968315147853972798913688", 10)
var y, _ = new(big.Int).SetString("74946767262888349555270609195205284686604880870734462312238891495596941025713", 10)
Copy link
Member

Choose a reason for hiding this comment

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

Error checking needed.

@ineiti
Copy link
Member

ineiti commented Dec 22, 2021

You can also use the Commit Suggestions in the comments to directly add the suggestions I made.

@sonarcloud
Copy link

sonarcloud bot commented Dec 29, 2021

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

No Coverage information No Coverage information
8.4% 8.4% Duplication

@sonarcloud
Copy link

sonarcloud bot commented Mar 4, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

No Coverage information No Coverage information
3.9% 3.9% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ineiti ineiti ineiti requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@pm-duokey @ineiti