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

secio, spipe replacement #442

Merged
merged 12 commits into from
Dec 12, 2014
Merged

secio, spipe replacement #442

merged 12 commits into from
Dec 12, 2014

Conversation

jbenet
Copy link
Member

@jbenet jbenet commented Dec 12, 2014

This PR introduces secio, the replacement to spipe.

  • use io.ReadWriter instead of channels + async workers
  • decrypt in place (can't encrypt in place, input mangling)
  • Encrypt-Then-Mac io.ReadWriter
  • a nifty cmd/seccat to try it out! crypto chat with friends!
  • commented everything
  • improved readability
  • improved security:

(1) changed the third message to be the random nonce sent by
the remote party.

(2) determine order based on identities AND the random nonces,
which makes it very unlikely to fail.

and more

@jbenet jbenet added the status/in-progress In progress label Dec 12, 2014
@jbenet
secio: better handshake log msg
8d961fc
@jbenet
secio: encrypt copy
9267f45 
sadly, encrypting needs to copy, as the user supplied buffer
must not be mangled.
@jbenet
Copy link
Member Author

jbenet commented Dec 12, 2014

seccat's pretty cool. try it out:

go install github.com/jbenet/go-ipfs/cmd/seccat
seccat -l :1234

# in another terminal
seccat :1234

and, to see the encryption magic:

# listen with debug on
seccat -debug -v -l :1234

# connect with debug on
seccat -debug -v :1234

@btc
Copy link
Contributor

btc commented Dec 12, 2014

seccat delivers the goods. 👍

@jbenet jbenet mentioned this pull request Dec 12, 2014
Merged
@jbenet jbenet self-assigned this Dec 12, 2014
@btc
Copy link
Contributor

btc commented Dec 12, 2014

Are areas in particular where you wish to focus the reader's eye?

@jbenet
Copy link
Member Author

jbenet commented Dec 12, 2014

warning: not interoperable, as the encryption protocol changed.

Will need rebooting machines, etc.

@jbenet
Copy link
Member Author

jbenet commented Dec 12, 2014

areas in particular where you wish to focus the reader's eye?

The secio module move to io. The actual (very long) handshake is probably not worth time atm unless you want to think hard about security/crypto (should be audited in the future).

@btc
Copy link
Contributor

btc commented Dec 12, 2014

Since you've got this part of the system paged in, are there any important events or metadata (eg. in the handshake or connection initiation) that we should log using json event logger?

btc
btc reviewed Dec 12, 2014
View reviewed changes
crypto/secio/io_test.go
@@ -0,0 +1 @@
package secio
Copy link
Contributor

Choose a reason for hiding this comment

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

rm? placeholder?

@jbenet
Copy link
Member Author

jbenet commented Dec 12, 2014

Since you've got this part of the system paged in, are there any important events or metadata (eg. in the handshake or connection initiation) that we should log using json event logger?

probably just:

  • handshake start
  • handshake finish (with remote peer ID)

things in between should be for debugging but don't actually give us good info atm. (maybe the cipher choice, but meh)

btc
btc reviewed Dec 12, 2014
View reviewed changes
crypto/secio/rw.go

// BufPool is a ByteSlicePool for messages. we need buffers because (sadly)
// we cannot encrypt in place-- the user needs their buffer back.
var BufPool = mpool.ByteSlicePool
Copy link
Contributor

Choose a reason for hiding this comment

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

exposed intentionally?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, so you could swap it out if desired. it's not a great way to do it shrug

Copy link
Contributor

Choose a reason for hiding this comment

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

k

@btc
Copy link
Contributor

btc commented Dec 12, 2014

LGTM

@jbenet
Copy link
Member Author

jbenet commented Dec 12, 2014

We need a good way to merge in protocol breaking changes like this one. auto-update soon, but we also need to update nodes. any ideas?

@jbenet
Copy link
Member Author

jbenet commented Dec 12, 2014

I guess i'll just merge. I'd love to have a single command that updates all the live nodes (solarnet, mars included)

jbenet added a commit that referenced this pull request Dec 12, 2014
@jbenet
Merge pull request #442 from jbenet/io-spipe
4a5a742 
secio, spipe replacement
@jbenet jbenet merged commit 4a5a742 into master Dec 12, 2014
@jbenet jbenet deleted the io-spipe branch December 12, 2014 11:54
@jbenet jbenet removed the status/in-progress In progress label Dec 12, 2014
@btc
Copy link
Contributor

btc commented Dec 12, 2014

warning: not interoperable, as the encryption protocol changed.

The new secio events aren't showing up in the event logs. Or do these changes go live after net is merged?
image

ariescodescream pushed a commit to ariescodescream/go-ipfs that referenced this pull request Oct 23, 2021
@Stebalien
Merge pull request ipfs#442 from libp2p/dependabot/go_modules/go.open…
f919cde 
…census.io-0.22.3

build(deps): bump go.opencensus.io from 0.22.2 to 0.22.3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jbenet jbenet

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jbenet @btc