Added ECDSA; Added RSA tests; Fixed linting errors; Handling all un-handled errors #35

adam-hanna · 2018-07-14T23:57:28Z

Added ECDSA
Added RSA tests
Fixed linting errors. Mostly just meant adding comments to exported types and functions
Handled all un-handled errors. Please note that this is not backwards compatible as I had to add an error in the return of the KeyStretcher function. Every time m.Write(..) gets called (for example, here), the number of bytes written and an error gets returned. These returned values were just being ignored.

adam-hanna · 2018-07-15T00:02:39Z

I think the build is failing because it's pulling github.com/libp2p/go-libp2p-crypto/pb which doesn't have the required types, rather than using the types in the included subdir.

If you clone and $ go test . everything should pass.

adam-hanna · 2018-08-10T17:02:26Z

Merged #34 and #37

Stebalien

Thanks! I think the main issue here is the dependence on RLP. While ethereum compatibility out of the box would be nice, I'm not convinced it's worth the added complexity (this won't only be used by eth).

Stebalien · 2018-08-10T22:20:06Z

ecdsa.go

+
+	sha256 "gx/ipfs/QmXTpwq2AkzQsPjKqFQDNY2bMdsAT53hUBETeyj8QRHTZU/sha256-simd"
+
+	"github.com/ethereum/go-ethereum/rlp"


This is going to be a bit of a problem. Pulling down all of go ethereum just to build this package isn't really an option and go ethereum is LGPL licensed (while this package is MIT licensed).

Also, looking at the RLP format documentation, I'd rather not touch that. If users need ethereum compatibility, they can convert between that format and whatever sane format we choose in this package.

We should probably use ASN.1 (that's what most systems use).

Stebalien · 2018-08-10T22:22:33Z

ecdsa.go

+		return false
+	}
+
+	return reflect.DeepEqual(ePub, oPub)


It's probably a better idea to just implement this manually. This assumes that two equivalent bigints always have the same internal structure.

Stebalien · 2018-08-10T22:47:56Z

ed25519.go

 	pb "github.com/libp2p/go-libp2p-crypto/pb"
+
+	"gx/ipfs/QmQ51pHe6u7CWodkUGDLqaCEMchkbMt7VEZnECF5mp6tVb/ed25519"


We leave this in the unrewritten state (except for in go-ipfs). Run gx-go rw --fix to undo this.

Stebalien · 2018-08-10T23:10:37Z

key.go

@@ -207,15 +219,21 @@ func KeyStretcher(cipherType string, hashType string, secret []byte) (StretchedK
 	}

 	m := hmac.New(h, secret)
-	m.Write(seed)
+	if _, err := m.Write(seed); err != nil {


This is guaranteed to never return an error.

Stebalien · 2018-08-10T23:11:26Z

key.go

 // (myIV, theirIV, myCipherKey, theirCipherKey, myMACKey, theirMACKey)
-func KeyStretcher(cipherType string, hashType string, secret []byte) (StretchedKeys, StretchedKeys) {
+func KeyStretcher(cipherType string, hashType string, secret []byte) (*StretchedKeys, *StretchedKeys, error) {


This is a public interface so we probably shouldn't change it (unless there's a very good reason).

I agree, but there are quite a few errors that are being ignored in the function. Rather than ignoring them, I think we should return the error. For example, here.

See my comment below (above?). Writes to hmac's can't fail (according to the documentation). We should probably have a comment stating this...

In this comment, I was referring to the switch to return by pointer instead of return by value (although I guess you probably figured that we might as well make that change along with the change to return errors).

My apologies! Didn't see your other comment and didn't know that about HMAC writes! TY!

I'll leave this function definition unchanged, then.

Np, no need to apologize. This is definitely non-obvious (I had to go check the docs thinking "we can't have been that lazy...").

Stebalien · 2018-08-10T23:14:10Z

While you're at it, could you rebase on master and cleanup the git history a bit? Reverse merges get really messy and it would be nice to have all of the documentation/cleanup (thanks, by the way) changes in separate commits.

adam-hanna · 2018-08-10T23:19:46Z

Thanks for the detailed comments! I'll get working on them!

Stebalien

Sorry for the slow turnaround. A few style comments and some bugs I failed to notice.

(then this should be good to go).

Note: always feel free to bug me on IRC (or here) as frequently as you want.

Stebalien · 2018-08-31T23:02:34Z

secp256k1.go

 func (k *Secp256k1PrivateKey) Equals(o Key) bool {
-	sk, ok := o.(*Secp256k1PrivateKey)
+	k, ok := o.(*Secp256k1PrivateKey)


Stebalien · 2018-08-31T23:36:06Z

secp256k1.go

 func (k *Secp256k1PublicKey) Equals(o Key) bool {
-	sk, ok := o.(*Secp256k1PublicKey)
+	k, ok := o.(*Secp256k1PublicKey)


Stebalien · 2018-08-31T23:38:44Z

key.go


 	"github.com/gogo/protobuf/proto"
+	pb "github.com/libp2p/go-libp2p-crypto/pb"


FYI, we usually order our imports:

stdlib

local repo

external repos

(with newlines in-between)

However, the separation between the crypto and non-crypto imports was non-idiomatic.

Stebalien · 2018-08-31T23:40:37Z

ecdsa.go

+}
+
+// GenerateECDSAKeyPairFromKey generates a new ecdsa private and public key from an input private key
+func GenerateECDSAKeyPairFromKey(priv *ecdsa.PrivateKey) (PrivKey, PubKey, error) {


Maybe drop the Generate (we're not really generating a new key)?

Stebalien · 2018-08-31T23:41:10Z

ecdsa.go

+
+// GenerateECDSAKeyPair generates a new ecdsa private and public key
+func GenerateECDSAKeyPair(src io.Reader) (PrivKey, PubKey, error) {
+	priv, err := ecdsa.GenerateKey(ECDSACurve, src)


Can replace with return GenerateECDSAKeyPairWithCurve(ECDSACurve, src).

adam-hanna · 2018-09-02T20:33:52Z

@Stebalien thanks, I'm on it!

1. Added ECDSA Keys 2. Removed GX import refs 3. Added RSA tests 4. Fixed linting warnings

adam-hanna · 2018-09-03T19:17:11Z

@Stebalien, thanks again for all of your help, and sorry for introducing that bug. Great catch!

I just incorporated your latest round of feedback...

Stebalien · 2018-09-03T20:17:26Z

I'd like to get @dignifiedquire to take a look at this before merging as he maintains the JS version. I also noticed that "secp256k1" is also ECDSA (with a specific curve, maybe some bitcoin-specific key formats?). Hopefully @dignifiedquire will know more.

Stebalien · 2018-09-28T17:31:35Z

I've discussed this and, well, we do support a variant of ECDSA (Secp256k1) but we use a hard-coded curve and a minimal key format. The upside is that keys in that format are small. The downside, is that it's a bit less flexible. The downside of this format is that keys are large. However, it supports all "standard" keys so it should help with interoperability with existing systems.

Given that, we've (@dignifiedquire and I) have decided to merge this.

Stebalien · 2018-09-28T17:31:55Z

Thanks @adam-hanna for bearing with this!

adam-hanna · 2018-09-29T16:19:27Z

Thanks for everything!

…

On Fri, Sep 28, 2018, 7:31 PM Steven Allen ***@***.***> wrote: Thanks @adam-hanna <https://github.com/adam-hanna> for bearing with this! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#35 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFL2FjDVZnZrcynjD2oppLvUJd0x5WDeks5ufl0NgaJpZM4VQDm2> .

Stebalien · 2018-11-03T00:46:40Z

package.json

-      "hash": "QmdxUuburamoF6zF9qjeQC4WYcWGbWuRmdLacMEsW8ioD8",
-      "name": "gogo-protobuf",
-      "version": "0.0.0"
-    },


Stebalien suggested changes Aug 10, 2018

View reviewed changes

adam-hanna force-pushed the forPR branch 2 times, most recently from c0b8eee to 9fc17fa Compare August 13, 2018 04:22

Stebalien suggested changes Aug 31, 2018

View reviewed changes

Incorporated latest round of code review and:

5d08597

1. Added ECDSA Keys 2. Removed GX import refs 3. Added RSA tests 4. Fixed linting warnings

adam-hanna force-pushed the forPR branch from 9fc17fa to 5d08597 Compare September 3, 2018 19:13

Stebalien approved these changes Sep 3, 2018

View reviewed changes

Stebalien requested a review from dignifiedquire September 3, 2018 20:12

Stebalien merged commit 0f79fbe into libp2p:master Sep 28, 2018

Stebalien reviewed Nov 3, 2018

View reviewed changes

package.json

"hash": "QmdxUuburamoF6zF9qjeQC4WYcWGbWuRmdLacMEsW8ioD8",

"name": "gogo-protobuf",

"version": "0.0.0"

},

Copy link

Member

Stebalien Nov 3, 2018

Choose a reason for hiding this comment

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

Why?

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added ECDSA; Added RSA tests; Fixed linting errors; Handling all un-handled errors #35

Added ECDSA; Added RSA tests; Fixed linting errors; Handling all un-handled errors #35

adam-hanna commented Jul 14, 2018

adam-hanna commented Jul 15, 2018 •

edited

Loading

adam-hanna commented Aug 10, 2018

Stebalien left a comment

Stebalien Aug 10, 2018

Stebalien Aug 10, 2018

Stebalien Aug 10, 2018

Stebalien Aug 10, 2018

Stebalien Aug 10, 2018

Stebalien Aug 10, 2018

Stebalien Aug 10, 2018

adam-hanna Aug 10, 2018

Stebalien Aug 10, 2018 •

edited

Loading

adam-hanna Aug 11, 2018

Stebalien Aug 11, 2018

Stebalien commented Aug 10, 2018

adam-hanna commented Aug 10, 2018

Stebalien left a comment

Stebalien Aug 31, 2018

Stebalien Aug 31, 2018

Stebalien Aug 31, 2018

Stebalien Aug 31, 2018

Stebalien Aug 31, 2018

adam-hanna commented Sep 2, 2018

adam-hanna commented Sep 3, 2018

Stebalien commented Sep 3, 2018

Stebalien commented Sep 28, 2018

Stebalien commented Sep 28, 2018

adam-hanna commented Sep 29, 2018 via email

Stebalien Nov 3, 2018


		sha256 "gx/ipfs/QmXTpwq2AkzQsPjKqFQDNY2bMdsAT53hUBETeyj8QRHTZU/sha256-simd"

		"github.com/ethereum/go-ethereum/rlp"

		pb "github.com/libp2p/go-libp2p-crypto/pb"

		"gx/ipfs/QmQ51pHe6u7CWodkUGDLqaCEMchkbMt7VEZnECF5mp6tVb/ed25519"


		"github.com/gogo/protobuf/proto"
		pb "github.com/libp2p/go-libp2p-crypto/pb"

Added ECDSA; Added RSA tests; Fixed linting errors; Handling all un-handled errors #35

Added ECDSA; Added RSA tests; Fixed linting errors; Handling all un-handled errors #35

Conversation

adam-hanna commented Jul 14, 2018

adam-hanna commented Jul 15, 2018 • edited Loading

adam-hanna commented Aug 10, 2018

Stebalien 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

Stebalien Aug 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien commented Aug 10, 2018

adam-hanna commented Aug 10, 2018

Stebalien 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

adam-hanna commented Sep 2, 2018

adam-hanna commented Sep 3, 2018

Stebalien commented Sep 3, 2018

Stebalien commented Sep 28, 2018

Stebalien commented Sep 28, 2018

adam-hanna commented Sep 29, 2018 via email

Choose a reason for hiding this comment

adam-hanna commented Jul 15, 2018 •

edited

Loading

Stebalien Aug 10, 2018 •

edited

Loading