Darc ECDSA #2484

pm-duokey · 2021-12-21T12:05:35Z

What this PR does

See commit message
This PR

🙅‍ Friendly checklist:

ineiti

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.

ineiti · 2021-12-21T15:20:21Z

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?


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

ineiti · 2021-12-21T15:21:14Z

darc/darc.go

+	}
+}
+
+//TODO make calls to tsm  available


Please resolve all TODOs.

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

ineiti · 2021-12-21T15:39:01Z

darc/darc.go

+//necessary function, needs to be refactored only supports elliptic.P256 curve
+//needs to be tested Unmarshal might not work


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

ineiti · 2021-12-21T15:40:03Z

darc/darc.go

@@ -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)


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

ineiti · 2021-12-21T15:42:26Z

darc/darc.go

+	id := make([]byte, hex.DecodedLen(len(in)))
+	_, err := hex.Decode(id, []byte(in))


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)

}

ineiti · 2021-12-21T15:43:33Z

darc/darc.go

+	if err != nil {
+		return Identity{}, err
+	}


Suggested change

if err != nil {

return Identity{}, err

}

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

ineiti · 2021-12-21T15:45:09Z

darc/darc.go

@@ -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


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.

ineiti · 2021-12-21T15:46:53Z

darc/darc.go

+	}}
+}
+
+//TODO


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.

ineiti · 2021-12-21T15:48:30Z

darc/darc_test.go

+	msg := []byte(`Hello World`)
+
+	//Signature from code example go-tsm-sdk corresponding to ecdsa public key example
+	signed, _ := hex.DecodeString("304402204f0b20a44efacec7b0514683233a79552026fe80e468078f6fed6cfe3f3e8a0402201eb12db7f6fe0828cafe8b0a032a37ff377b342799cfe77cfbac40c8ec1fa9e8")


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!

ineiti · 2021-12-21T15:48:45Z

darc/darc_test.go

+	var x, _ = new(big.Int).SetString("25613385885653880697990944418179706546134037329992108968315147853972798913688", 10)
+	var y, _ = new(big.Int).SetString("74946767262888349555270609195205284686604880870734462312238891495596941025713", 10)


Error checking needed.

ineiti · 2021-12-22T10:28:39Z

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

sonarqubecloud · 2021-12-29T11:22:42Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

No Coverage information
8.4% Duplication

sonarqubecloud · 2022-03-04T09:47:41Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

No Coverage information
3.9% Duplication

pm-duokey added 5 commits December 17, 2021 03:01

new Darc ECDSA

0e73fd2

"implementation ECDSADarc"

596a5c7

ECDSADarc

5c21329

ECDSA signer

3632b20

merge temporary darc branch darc_temp

4a85f31

ineiti requested changes Dec 21, 2021

View reviewed changes

removing superflue import

ec6cdb2

pm-duokey and others added 4 commits December 22, 2021 06:59

ecdsa calypso/api_test.go error

b63fc6f

Add marshalling and a test

2d78896

added tsm to darcs, too, but signature is failing

fdfc3eb

Adding a TSM signer

b67548c

TestClient_Calypso_Simple

a06494a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Darc ECDSA #2484

Darc ECDSA #2484

pm-duokey commented Dec 21, 2021

ineiti left a comment

ineiti Dec 21, 2021

ineiti Dec 21, 2021

ineiti Dec 21, 2021

ineiti Dec 21, 2021

ineiti Dec 21, 2021

ineiti Dec 21, 2021

ineiti Dec 21, 2021

ineiti Dec 21, 2021

ineiti Dec 21, 2021

ineiti Dec 21, 2021

ineiti commented Dec 22, 2021

sonarqubecloud bot commented Dec 29, 2021

sonarqubecloud bot commented Mar 4, 2022

		//necessary function, needs to be refactored only supports elliptic.P256 curve
		//needs to be tested Unmarshal might not work

		id := make([]byte, hex.DecodedLen(len(in)))
		_, err := hex.Decode(id, []byte(in))

	// 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.

+              	}}
+              }
+              //TODO

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

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

		var x, _ = new(big.Int).SetString("25613385885653880697990944418179706546134037329992108968315147853972798913688", 10)
		var y, _ = new(big.Int).SetString("74946767262888349555270609195205284686604880870734462312238891495596941025713", 10)

Darc ECDSA #2484

Are you sure you want to change the base?

Darc ECDSA #2484

Conversation

pm-duokey commented Dec 21, 2021

ineiti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ineiti commented Dec 22, 2021

sonarqubecloud bot commented Dec 29, 2021

sonarqubecloud bot commented Mar 4, 2022