Automatically configure `config.connections.incoming`

christianbundy commented 5 years ago

This replaces the old hardcode method with a new automatic configuration that iterates through os.networkInterfaces() and returns a valid configuration that takes advantage of all possible interfaces.

This should give multiserver more addresses to broadcast.

Before

{ net:
   [ { host: '::',
       port: 8008,
       scope: [ 'device', 'local', 'public' ],
       transform: 'shs' } ],
  ws:
   [ { host: '::',
       port: 8989,
       scope: [ 'device', 'local', 'public' ],
       transform: 'shs' } ] }

After

{ net:
   [ { host: '127.0.0.1',
       port: 8008,
       scope: [ 'device' ],
       transform: 'shs' },
     { host: '::1', port: 8008, scope: [ 'device' ], transform: 'shs' },
     { host: '192.168.0.109',
       port: 8008,
       scope: [ 'device', 'local', 'public' ],
       transform: 'shs' },
     { host: '172.18.0.1',
       port: 8008,
       scope: [ 'device', 'local', 'public' ],
       transform: 'shs' },
     { host: 'fce2:9811:4862:81a7:bb08:91d6:2e41:d220',
       port: 8008,
       scope: [ 'device', 'local', 'public' ],
       transform: 'shs' } ],
  ws:
   [ { host: '127.0.0.1',
       port: 8989,
       scope: [ 'device' ],
       transform: 'shs' },
     { host: '::1', port: 8989, scope: [ 'device' ], transform: 'shs' },
     { host: '192.168.0.109',
       port: 8989,
       scope: [ 'device', 'local', 'public' ],
       transform: 'shs' },
     { host: '172.18.0.1',
       port: 8989,
       scope: [ 'device', 'local', 'public' ],
       transform: 'shs' },
     { host: 'fce2:9811:4862:81a7:bb08:91d6:2e41:d220',
       port: 8989,
       scope: [ 'device', 'local', 'public' ],
       transform: 'shs' } ] }

One interesting side-effect here is that if you run a node in a mesh network (e.g. cjdns) then that address is now being broadcast over LAN. If two people use both Scuttlebutt and cjdns on the same LAN then when they're far away I think they'll continue to peer over cjdns because the address is saved to gossip.json. Someone please correct me if I'm wrong here!

Also Travis CI tests by filtering IPv6 addresses on Travis. :tada:

Resolves #47 Resolves #49 Resolves #52 Hopefully takes care of https://github.com/ssbc/patchwork/issues/958 and https://github.com/ssbc/patchwork/issues/1024 Paves the way to merging https://github.com/ssbc/multiserver/pull/42

arj03 commented 5 years ago

It seems like this auto stuff only applies to shs, so I don't see that big of a problem. I mean it can't be combined with noauth, as mentioned here.

I guess this superseeds some of the other PRs?

christianbundy commented 5 years ago

Yeah, I've defaulted to always include the shs transform because I don't understand os.networkInterfaces() well enough to trust that { internal: true } actually means it's a private interface. Would love to learn more about this if anyone would prefer to use noauth. :books:

This definitely supersedes #47 and #49, and maybe #23 (? (cc: @regular)).

arj03 commented 5 years ago

Thats was what I meant, mostly. noauth should only be used with pure scope 'device'.

christianbundy commented 5 years ago

Sweet, are there any changes you'd like to see (or anyone you think should definitely see this)?

arj03 commented 5 years ago

I was just testing this and it works for me, both with existing config and without any incoming. Also for patchbay we change the incoming to allow unix + noauth so I don't see anything really bad coming out of this. So +1 for merging.

christianbundy commented 5 years ago

I want to point out a great observation that @black-puppydog made here:

I had to remove my hardcoded :: config entry (:man_facepalming:)

I'd like to avoid any mandatory config changes, I'm thinking this behavior might be better:

If config.host == null use the auto-detection to bind to all interfaces.
If config.host === '::' use the auto-detection to bind to all interfaces.
If config.host === '0.0.0.0' use the auto-detection to bind to all interfaces.
Otherwise, bind to the exact value in config.host with no automatic shenanigans.

christianbundy commented 5 years ago

Scratch that, it looks like util/fix-connections.js specifically checks to ensure that config.host is represented in config.connections.incoming, so we can only do this automated stuff when config.host == null.

Dominic mentioned that @regular might have some valuable feedback here, so I'm going to wait until I get a :+1: from him until merging this. I think this change might be more elegant upstream (see https://github.com/ssbc/multiserver-scopes/issues/1), but I think this is a decently robust workaround until then.

mixmix commented 5 years ago

Yeah I put a bit of thought into that fix-connections.

There should really be a test around it. The behaviour we do not want under any circumstances is that a person sets some incoming connections explicitly narrowly, as a then some auto smart junk adds connections. This is how it was and it's and its a great way to add security problems /rant

On Sat, 11 May 2019, 04:29 Christian Bundy, notifications@github.com wrote:

Scratch that, it looks like util/fix-connections.js specifically checks to ensure that config.host is represented in config.connections.incoming, so we can only do this automated stuff when config.host == null.

Dominic mentioned that @regular https://github.com/regular might have some valuable feedback here, so I'm going to wait until I get a 👍 from him until merging this. I think this change might be more elegant upstream (see ssbc/multiserver-scopes#1 https://github.com/ssbc/multiserver-scopes/issues/1), but I think this is a decently robust workaround until then.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/ssbc/ssb-config/pull/53#issuecomment-491350025, or mute the thread https://github.com/notifications/unsubscribe-auth/AAUK3HTSI7UINJ4YZHX7TP3PUWPGTANCNFSM4HLTPTWQ .

christianbundy commented 5 years ago

Yep, I'm super appreciative that util/fix-connections.js throws when the config is funky, it acts as a great guard rail even if it's more of a runtime sanity check than a "test" in the dev-y sense of the word.

christianbundy commented 5 years ago

I've actually taken this code and moved it into multiserver where I think it belongs: https://github.com/ssbc/multiserver/pull/49

This means that ssb-config can still listen on :: but when you stringify the address multiserver should output all of the actual addresses that it's listening on. Less code in ssb-config and a smarter multiserver, woo! Would love a review if/when folks have time.

regular commented 4 years ago

@christianbundy thanks for pinging me. The only thing I can add to this conversation is this:

Somewhere you wrote:

Node understands that :: means "listen on all interfaces"

:: is an IPv6 address that has all zeros. My understanding is that Node just passes :: down to the IP stack and that the IP stack's configuration (or socket options) determines if this means "all IPv6" addresses" or "all IPv6 and IPv4 addresses".

A program bound to :: will be given traffic for any actual IPv6 address assigned to the system - it may also receive IPv4 traffic too in the form of IPv6-mapped IPv4 addresses (::ffff:x.x.x.x) although this is dependent on the socket options set by the application.

https://serverfault.com/a/452279/171681

You might want to consider this when coding a special case for '::'. I'm afraid I don't have any more specific advise/opinion on this.

mixmix commented 4 years ago

@christianbundy you've done amazing work. I understand there could be a deeper solution in multiserver but that's beyond my comprehension for the moment.

Because I am more familiar with this, and have tested this working on my local network, I've decided to merge this as a great interim solution. I've factored out the body of your logic into a single file to make this easy to snip out in the future if multiserver things get resolved.

thanks so much for your love and attention :heart: :heart: :heart:

mixmix commented 4 years ago

updated the tests here, and travis says everything's good : https://github.com/ssbc/ssb-config/pull/60

mixmix commented 4 years ago

Wrap up

Published as ssb-config@3.4.0 :popcorn: :fireworks:

Closed https://github.com/ssbc/ssb-config/pull/47 Closed https://github.com/ssbc/ssb-config/issues/49 Closed https://github.com/ssbc/ssb-config/pull/52

Future / TODO

Revisit this if https://github.com/ssbc/multiserver/pull/49 is merged

ssbc / ssb-config