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

SECIO spec #106

Merged
merged 10 commits into from
Jun 11, 2019
Merged

SECIO spec #106

merged 10 commits into from
Jun 11, 2019

Conversation

bigs
Copy link
Contributor

@bigs bigs commented Oct 24, 2018

Introduces a spec for SECIO. Various crypto algorithms living in *-libp2p-crypto have been omitted for the time being.

@bigs bigs requested review from Stebalien and raulk October 24, 2018 21:36
@ghost ghost assigned bigs Oct 24, 2018
@ghost ghost added the in progress label Oct 24, 2018
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

General comment: this reads more like a walk though the go-libp2p secio implementation and less like a spec. It would be nice if this were a bit more declarative (list out the message types, explain how they are composed, then explain how to use them) and less "run this, then this, then this".

secio/README.md Outdated
proposal as follows:

*Note: the public key should be serialized per the `Bytes` method from
[go-libp2p-crypto](https://godoc.org/github.com/libp2p/go-libp2p-crypto#Key).*
Copy link
Member

Choose a reason for hiding this comment

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

Let's point at #100 (once it's merged)

secio/README.md Outdated
Pubkey: <public key bytes>,
Exchanges: <comma separated string of supported key exchanges>,
Ciphers: <comma separated string of supported ciphers>,
Hashes: <comma separated string of supported hashes>,
Copy link
Member

Choose a reason for hiding this comment

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

Let's put the actual protobuf here (and say that it's a protobuf).

secio/README.md Outdated

Both peers serialize this message and send it over the wire. Upon deserializing
their peer's message, they verify that the pubkey matches that described by the
peer's peer ID (NOTE: this is sometimes only possible for the peer *initiating*
Copy link
Member

Choose a reason for hiding this comment

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

Given that this is just the spec, I'd just say:

At this point, each peer should check that the public key matches the expected peer ID, if known.

Or something like that. We don't need to explicitly tell the programmer how to implement the protocol step-by-step (i.e., "calculate the peer ID and store it for later").

secio/README.md Outdated
Now the peers prepare a key exchange. Both peers generate an ephemeral key based
on the agreed upon exchange (currently support is only available for elliptic
curve algorithms). Ephemeral keys are generated via
[go-libp2p-crypto](https://godoc.org/github.com/libp2p/go-libp2p-crypto#GenerateEKeyPair).
Copy link
Member

Choose a reason for hiding this comment

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

We should actually spec this.

Copy link
Member

Choose a reason for hiding this comment

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

That's standard ECDHE if I'm not mistaken.

secio/README.md Outdated
Epubkey: <ephemeral pubkey>,
Signature: <sign corpus with local private key>,
}
```
Copy link
Member

Choose a reason for hiding this comment

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

Again, let's just use the protobuf definition.

secio/README.md Outdated
ephemeral key generation, passing it the remote peer's ephemeral key. With the
shared secret generated, both peers stretch the key using the algorithm
described by
[go-libp2p-crypto](https://godoc.org/github.com/libp2p/go-libp2p-crypto#KeyStretcher).
Copy link
Member

Choose a reason for hiding this comment

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

Again, we need to spec this.

opened. Each packet is of the form:

```
[uint32 length of packet | encrypted body | hmac signature of encrypted body]
Copy link
Member

Choose a reason for hiding this comment

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

  • We encode the lengths as big-endian (network-order).
  • Need to specify what "encrypt" means.
  • Need to specify how we mac.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does the padding style for encrypt need to be specified?

Copy link
Member

Choose a reason for hiding this comment

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

Really, we should specify everything. Ideally, we'd point to an RFC. However, we should optimize for merging something that's correct rather than waiting for something that's perfect.

Copy link
Contributor

@richardschneider richardschneider Nov 13, 2018

Choose a reason for hiding this comment

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

I'm assuming that the length includes the encrypted body and the hmac signature.

secio/README.md Outdated
and `k2` the remote peer's key.

Each peer now generates a MAC key and cipher for the remote and local keys
generated in the previous step using the `MacKey` and `CipherKey` from the
Copy link
Member

Choose a reason for hiding this comment

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

Need to specify the order within the result of the stretching.


The SECIO wire protocol features two message types defined in the
[protobuf description language](https://github.com/libp2p/go-libp2p-secio/blob/master/pb/spipe.proto).
These two messages, `Propose` and `Exchange` are the only serialized types
Copy link
Member

Choose a reason for hiding this comment

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

Let's dump the current state of the protobufs here. Relying on a reference that can mutate can render the spec incoherent at a later time. Also, we're seeking to version specs in general, so capturing the current state and versioning the spec as it evolves is fair play.

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind, I see this feedback is recurrent below.

secio/README.md Outdated

### Proposal Generation

SECIO channel negotiation begins with a proposal phase. Both sides generate a
Copy link
Member

Choose a reason for hiding this comment

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

As a prerequisite for SECIO, we need a dedicated channel where both parties have agreed to proceed with SECIO handshake by means of a multiplexer or else.

secio/README.md Outdated

### Key Stretching

Peers now generate their shared secret based on the function generated by the
Copy link
Member

Choose a reason for hiding this comment

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

IIRC, that function is referred to in literature as the KDF (key derivation function), and the overall process is called ECDH key agreement. Let's use these terms to evoke familiar concepts.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think its official name is ECSVDP-DH

- P-384
- P-521

### Ciphers
Copy link
Member

Choose a reason for hiding this comment

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

Ciphers used for key stretching and for message encryption once SECIO channel is established.

Copy link

@GriffinMB GriffinMB Apr 11, 2019

Choose a reason for hiding this comment

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

Why is a deprecated cipher (Blowfish) supported? From the Go package docs:

Blowfish is a legacy cipher and its short block size makes it vulnerable to birthday bound attacks (see https://sweet32.info). It should only be used where compatibility with legacy systems, not security, is the goal.

Deprecated: any new system should use AES (from crypto/aes, if necessary in an AEAD mode like crypto/cipher.NewGCM) or XChaCha20-Poly1305 (from golang.org/x/crypto/chacha20poly1305).

Since the spec is not yet finalized, it would be cool if support was removed. If there is some mitigating circumstance (I haven't gone through the source very thoroughly!), a note about it would be helpful.

Copy link
Member

Choose a reason for hiding this comment

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

👍

Choose a reason for hiding this comment

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

It would also be worth noting what mode the AES ciphers should use. From the Go implementation, it looks like it's CTR!

Also, more generally, is this an implementation of any well-reviewed specification? If not, is there a reason why? I would imagine there are a number of specs for low-overhead, encrypted channels which may prevent future pitfalls.

Copy link
Member

Choose a reason for hiding this comment

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

This is definitely not well-reviewed and will be will be deprecated in favor of TLS quite soon. See: https://github.com/libp2p/go-libp2p-tls/.

(go-ipfs now has experimental support which we'll likely upgrade to default support after a release or two).

Choose a reason for hiding this comment

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

Gotcha, makes sense! And that answers my follow-up questions as well :)

secio/README.md Outdated
SECIO allows participating peers to support a subset of the following
algorithms.

### Exhchanges
Copy link
Member

Choose a reason for hiding this comment

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

Elliptic curves used for ephemeral key generation.

- AES-128
- Blowfish

### Hashes
Copy link
Member

Choose a reason for hiding this comment

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

Hashes used for key stretching, and for HMACs once SECIO channel is established.

Copy link
Contributor

Choose a reason for hiding this comment

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

What mode and padding is AES-* using? Are there any parameters for Blowfish?

secio/README.md Outdated
preferred peer. After swapping if necessary, `k1` becomes the local peer's key
and `k2` the remote peer's key.

Each peer now generates a MAC key and cipher for the remote and local keys
Copy link
Member

@raulk raulk Oct 25, 2018

Choose a reason for hiding this comment

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

These are not generated here. The MAC key is already generated during key stretching above. The cipher was selected above when comparing Propose messages.

What the Go implementation does at this point is prepare the constructs for HMAC and encryption/decryption. I think that's what needs to be stated, i.e. the implementation now has the required parameters to initialise the cryptographic constructs.

secio/README.md Outdated
```

The first packet transmitted by each peer must be the remote peer's nonce. Peers
validate that the remote peer sent them their nonce, closing if unsuccessful.
Copy link
Member

Choose a reason for hiding this comment

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

The target of validation is actually the (d)encryption and HMAC'ing logic. We use a known value (nonce) to send a predictable message so we can validate that both parties have set up the encrypted channels correctly.

secio/README.md Outdated

## Table of Contents

- [Algorithm Support](#algorithm-support)
Copy link
Member

Choose a reason for hiding this comment

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

ToC is missing entries.

secio/README.md Outdated
### Hashes

- SHA-256
- SHA-512
Copy link
Contributor

Choose a reason for hiding this comment

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

Current go implementation does not use dashes in the hash name; see al.go.

@whyrusleeping
Copy link
Contributor

@bigs status on this? Are you planning on continuing this work in the near future?

@bigs
Copy link
Contributor Author

bigs commented Nov 12, 2018

yep @whyrusleeping just lower priority than testbed work at the moment. lotta good feedback re: shaping this into a more formal spec

exchanges. Each peer computes:

```
oh1 := sha256(concat(remotePeerPubKeyBytes, myNonce))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the first use of the word "nonce". Does it refer to the rand field of the Propose message?

Copy link
Member

Choose a reason for hiding this comment

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

Yes.

@ghost ghost assigned yusefnapora May 9, 2019
@yusefnapora
Copy link
Contributor

Hey all, I took a stab at addressing the review feedback here. I'd love if people with more experience could give it a review.

One thing I didn't address was using more standard nomenclature (e.g. KDF, ECDH), as I'm not super confident I'd use the terms correctly. I might read up a bit more and give that a go for the next round.

Copy link
Contributor Author

@bigs bigs left a comment

Choose a reason for hiding this comment

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

i can't approve since i technically opened the PR. i say we add a DRAFT notice to the top and merge as is. what remains is really a few passes to add detail.

@yusefnapora yusefnapora added the maturity:recommendation Promotes a spec from Candidate Recommendation to Recommendation label May 27, 2019
@yusefnapora
Copy link
Contributor

I'm going to go ahead and merge this, since I think it's in a decent state. If I added anyone to the interest group (@Stebalien, @richardschneider, @tomaka, @raulk) that doesn't want to be there, let me know.

@yusefnapora yusefnapora merged commit effa408 into master Jun 11, 2019
@yusefnapora yusefnapora deleted the feat/specio branch June 11, 2019 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in progress maturity:recommendation Promotes a spec from Candidate Recommendation to Recommendation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants