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

Swarm Addrs, Disable secio opt, + tests #1399

Merged
merged 9 commits into from
Jun 27, 2015
Merged

Swarm Addrs, Disable secio opt, + tests #1399

merged 9 commits into from
Jun 27, 2015

Conversation

jbenet
Copy link
Member

@jbenet jbenet commented Jun 19, 2015

This PR adds:

  • ipfs swarm addrs local - command to list out local addresses
  • ipfs id -f=<addrs> - format to print out addrs too
  • ipfs id -f=\n\t - handle \n and \t correctly
  • test for daemon that checks encryption is on
  • --disable-transport-encryption daemon option (see below)
  • test for restricted api
  • test for --unrestricted-api

Left TODO:

  • fix the 'transport should be unencrypted' test

Changes:

commit f0d3d19d9dcb6c793a251d1c7836229de08b8440
Author: Juan Batiz-Benet <juan@benet.ai>
Date:   Fri Jun 19 05:53:00 2015 -0700

    added sharness/t0061-daemon-opts

    Test odd daemon options, like:
    - unrestricted-api
    - disable-transport-encryption (known breakage atm)

    License: MIT
    Signed-off-by: Juan Batiz-Benet <juan@benet.ai>

commit a763bb7d74f129b68f949c7a741a669f2d998f8b
Author: Juan Batiz-Benet <juan@benet.ai>
Date:   Fri Jun 19 03:05:50 2015 -0700

    daemon option to optionally disable secio

    This commit adds an option to turn off all encryption. This is a mode
    used for tests, debugging, achieving protocol implementation interop,
    learning about how the protocol works (nc ftw), and worst case
    networks which _demand_ to be able to snoop on all the traffic.
    (sadly, there are some private intranets like this...). (We should
    consider at least _signing_ all this traffic.)

    Because of the severity of this sort of thing, this is an
    all-or-nothing deal. Either encryption is ON or OFF _fully_.
    This way, partially unencrypted nodes cannot be accidentally left
    running without the user's understanding. Nodes without encrypted
    connections will simply not be able to speak to any of the global
    bootstrap nodes, or anybody in the public network.

    License: MIT
    Signed-off-by: Juan Batiz-Benet <juan@benet.ai>

commit 0b2f1dd774c79006433a63caa57dd0aad3a283e1
Author: Juan Batiz-Benet <juan@benet.ai>
Date:   Fri Jun 19 03:21:01 2015 -0700

    daemon output includes swarm addresses

    daemon output now includes initial swarm addresses. this is not a
    full solution, as a change in network will not trigger re-printing.
    We need a good way to do that.

    This made me re-think how we're outputting these messages, perhaps
    we should be throwing them as log.Events, and capturing some with
    a special keyword to output to the user on stdout. Things like
    network addresses being rebound, NATs being holepunched, external
    network addresses being figured out, connections established, etc
    may be valuable events to show the user. Of course, these should be
    very few, as a noisy daemon is an annoying daemon.

    License: MIT
    Signed-off-by: Juan Batiz-Benet <juan@benet.ai>

commit 767b2b1ec195a230f38afad5e8401d11f0469e80
Author: Juan Batiz-Benet <juan@benet.ai>
Date:   Fri Jun 19 04:20:43 2015 -0700

    ipfs swarm addrs local - show local addrs

    Add a command to return local addresses.

    License: MIT
    Signed-off-by: Juan Batiz-Benet <juan@benet.ai>

commit 62f316971cd4c6b17e98af3480bdd35a8a814144
Author: Juan Batiz-Benet <juan@benet.ai>
Date:   Fri Jun 19 04:47:52 2015 -0700

    ipfs id -f=<addrs> and \n \t

    - added <addrs> field to `ipfs id -f`
    - added \n and \t conversion in `ipfs id -f`

    License: MIT
    Signed-off-by: Juan Batiz-Benet <juan@benet.ai>

commit a098cabc40f2d5732b7fd99fa916ef09576bf6b2
Author: Juan Batiz-Benet <juan@benet.ai>
Date:   Fri Jun 19 05:13:04 2015 -0700

    t0060-daemon: test transport is encrypted

    License: MIT
    Signed-off-by: Juan Batiz-Benet <juan@benet.ai>

@jbenet jbenet added the status/in-progress In progress label Jun 19, 2015
@jbenet jbenet mentioned this pull request Jun 19, 2015
55 tasks
@rht
Copy link
Contributor

rht commented Jun 19, 2015

(secio) this is to put less load on tests/debugs/interop? Is it that significant?

@jbenet
Copy link
Member Author

jbenet commented Jun 19, 2015

@rht it's much easier to debug the protocol if you can nc to it and see it in action. having encrypted output is annoying to work with when trying to get interop on the basic wire/streaming protocols in another language.

@rht
Copy link
Contributor

rht commented Jun 19, 2015

Or just put a backdoor like usual.

@jbenet
Copy link
Member Author

jbenet commented Jun 19, 2015

I've also already gotten requests for an "unencrypted transport" for a particular network. I dont want those to mix with the public nodes though, so hence the total switch.

@jbenet
Copy link
Member Author

jbenet commented Jun 19, 2015

(and probably should still sign it-- can probably resurface that "signed io" pipe)

@jbenet
Copy link
Member Author

jbenet commented Jun 20, 2015

@whyrusleeping or @cryptix CR pls?

@jbenet
Copy link
Member Author

jbenet commented Jun 20, 2015

Btw, the test indicators look terrible because Travis-CI had a field day with the mac testing. But if you look closely, all the tests in every commit pass correctly on linux.

@jbenet
Copy link
Member Author

jbenet commented Jun 20, 2015

Travis is undergoing a bunch of maintenance for mac builds, so the indicators are all a mess. if you look at the actual results for all the commits, they all passed fine.

@whyrusleeping
Copy link
Member

this LGTM, weird that theres a bunch of different stuff all in the same PR, but i guess that makes testing easier?

@jbenet
Copy link
Member Author

jbenet commented Jun 20, 2015

@whyrusleeping yeah added it all in one PR as one thing motivated another. i can split it up if you want

@jbenet
Copy link
Member Author

jbenet commented Jun 20, 2015

If anyone can take a look at why

in the failing case, the nc outputs nothing in the test case. But if you connect to it manually, it seems to work fine. :/

to test manually, will want to add:

echo "$PORT_SWARM" &&
go-sleep 10s &&

observe the port outout, and nc manually:

nc localhost $PORT_SWARM

if unencrypted {
log.Warningf(`Running with --%s: All connections are UNENCRYPTED.
You will not be able to connect to regular encrypted networks.`, unencryptTransportKwd)
conn.EncryptConnections = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Exposed package globals like this frighten me. Will we remove this once node interop works?

If it stays around, I'd prefer conn.DisableEncryption() and print a warning on each dial as well. Found it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd still prefer if this was an opt-in for the lifetime of the process. No funky on/off switching.

Copy link
Member Author

Choose a reason for hiding this comment

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

Exposed package globals like this frighten me. Will we remove this once node interop works?
If it stays around, I'd prefer conn.DisableEncryption()

I imagine with a function we can also make it one-way-- i.e. can only disable it or something.

btw, nothing prevents people from writing different implementations -- or, if they have access to the process, overwrite random memory. Even without this switch, if you can manipulate the process everything's done.

I'd still prefer if this was an opt-in for the lifetime of the process. No funky on/off switching.

yeah it is

Copy link
Contributor

Choose a reason for hiding this comment

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

I just want to guard (package) users from using this in unintended ways and make it clear to them that this is a development feature. A func could disallow you to switch encryption back on.

The rest is fine by me. Of course we can't do anything meaningful about corruption from the outside.

@jbenet
Copy link
Member Author

jbenet commented Jun 27, 2015

fixed it in a390541

License: MIT
Signed-off-by: Juan Batiz-Benet <juan@benet.ai>
- added <addrs> field to `ipfs id -f`
- added \n and \t conversion in `ipfs id -f`

License: MIT
Signed-off-by: Juan Batiz-Benet <juan@benet.ai>
Add a command to return local addresses.

License: MIT
Signed-off-by: Juan Batiz-Benet <juan@benet.ai>
daemon output now includes initial swarm addresses. this is not a
full solution, as a change in network will not trigger re-printing.
We need a good way to do that.

This made me re-think how we're outputting these messages, perhaps
we should be throwing them as log.Events, and capturing some with
a special keyword to output to the user on stdout. Things like
network addresses being rebound, NATs being holepunched, external
network addresses being figured out, connections established, etc
may be valuable events to show the user. Of course, these should be
very few, as a noisy daemon is an annoying daemon.

License: MIT
Signed-off-by: Juan Batiz-Benet <juan@benet.ai>
This commit adds an option to turn off all encryption. This is a mode
used for tests, debugging, achieving protocol implementation interop,
learning about how the protocol works (nc ftw), and worst case
networks which _demand_ to be able to snoop on all the traffic.
(sadly, there are some private intranets like this...). (We should
consider at least _signing_ all this traffic.)

Because of the severity of this sort of thing, this is an
all-or-nothing deal. Either encryption is ON or OFF _fully_.
This way, partially unencrypted nodes cannot be accidentally left
running without the user's understanding. Nodes without encrypted
connections will simply not be able to speak to any of the global
bootstrap nodes, or anybody in the public network.

License: MIT
Signed-off-by: Juan Batiz-Benet <juan@benet.ai>
Test odd daemon options, like:
- unrestricted-api
- disable-transport-encryption (known breakage atm)

License: MIT
Signed-off-by: Juan Batiz-Benet <juan@benet.ai>
fix the nc wait. the issue was that stdin needs to remain _open_
but not receive any input for some time. If stdin receives (invalid)
input or closes, the other side terminates the connection before
writing out the muxer frames + identify handshake.

This commit also changes the use of `!` for `test_must_fail`

License: MIT
Signed-off-by: Juan Batiz-Benet <juan@benet.ai>
License: MIT
Signed-off-by: Juan Batiz-Benet <juan@benet.ai>
they were failing intermittently

License: MIT
Signed-off-by: Juan Batiz-Benet <juan@benet.ai>
jbenet added a commit that referenced this pull request Jun 27, 2015
Swarm Addrs, Disable secio opt, + tests
@jbenet jbenet merged commit 2d6b787 into master Jun 27, 2015
@jbenet jbenet removed the status/in-progress In progress label Jun 27, 2015
@jbenet jbenet deleted the disable-secio-option branch June 27, 2015 08:00
This was referenced Jun 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants