Initial go-libp2p-noise implementation #2
Conversation
Begin noise!!
implemented IK handshake modified IK generated noise file to export needed funcs + made helpers added 2 byte length prefix to messages separated IK and XX related funcs into different files added Noise related fields to Transport, allows for saving of static noise key in Transport pipe from XX->IK works, IK->XXfallback in progress
cleanup code, fix ReadSecure and WriteSecure
| return mb, nil | ||
| } | ||
|
|
||
| func validatePublicKey(k []byte) bool { |
There was a problem hiding this comment.
The lower order point blacklist is unnecessary in noise because the state accumulator ensures that view divergence caused by injecting lower order points via man in the middle attack causes the state hashes to diverge and handshake to fail.
There was a problem hiding this comment.
This file (as well as IK.noise.go) is autogenerated from the noise explorer, could you open an issue there? https://github.com/SymbolicSoft/noiseexplorer
There was a problem hiding this comment.
I'm not going to interact with noiseexplorer explorer because of who the authors are.
Also the code, it generates is pretty unidiomatic and has issues like this.
I'd be interested in providing a more compatible and higher quality implementation of the underlying noise handshakes.
There was a problem hiding this comment.
yeah we would also be interested in that, how do you propose we make that happen?
There was a problem hiding this comment.
@zmanian Any update on a proposed path of action?
xx/XX.noise.go
Outdated
| var ss symmetricstate | ||
| var e Keypair | ||
| var re [32]byte | ||
| name := []byte("Noise_XX_25519_ChaChaPoly_BLAKE2s") |
There was a problem hiding this comment.
Isn't this suppose to be Noise_XX_25519_ChaChaPoly_SHA256?
There was a problem hiding this comment.
yeah definitely, this was autogenerated from the noise explorer which only supported BLAKE2s, I had to manually change the hashing algo to SHA256 but I looks like I didn't change the name. I'll change it now
xx/XX.noise.go
Outdated
| var ss symmetricstate | ||
| var e Keypair | ||
| var re [32]byte | ||
| name := []byte("Noise_XX_25519_ChaChaPoly_BLAKE2s") |
There was a problem hiding this comment.
same as above. the code does use SHA256, just gotta update the string
There was a problem hiding this comment.
Hey @noot and other ChainSafe folks 👋
Thanks for making this implementation happen! I'm getting ready to pull this in to the libp2p org and update it to match a few changes we've made to the spec since it was written.
I found a bug in the Noise pipes implementation as I was reviewing that was causing it to always use XX, but I've got a fix and will make a PR after we get this in. I can tag you on it if you're interested.
Cheers & thanks again 😄
| done := make(chan struct{}) | ||
| go func() { | ||
| defer close(done) | ||
| respConn, respErr = respTransport.SecureOutbound(context.TODO(), resp, initTransport.LocalID) |
There was a problem hiding this comment.
This seems backwards to me. Shouldn't the initiator be the one securing the outbound connection to the responder?
|
Just wanted to write up some things that I found while reviewing today that I'd like to address after we merge in. I'll open issues for these as well so we don't lose track.
|
|
@yusefnapora Hey, just saw this now! Thanks so much for the merge + feedback. I would definitely be interested in keeping up with further updates and issues, going to go through the changes now :) |
This PR includes an initial implementation of the libp2p noise spec
The IK/XX noise files are mostly auto-generated code from the noise explorer + some modifications and helpers.