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

multi: add onion services support #1159

Merged
merged 17 commits into from Jun 5, 2018

Conversation

Projects
None yet
6 participants
@wpaulino
Collaborator

wpaulino commented Apr 30, 2018

This PR continues the work from #796. Huge thanks to @Crypt-iQ for getting this started! I decided to create my own branch rather than rebasing theirs, but credit has been given to them in the relevant commits.

This PR introduces the following changes:

  • Adds support and tests for onion address serialization in channeldb and lnwire.
  • Updates the Tor-related flags with sane defaults.
  • Fixes an IP leak when doing the fallback SRV lookup.
  • Adds an initial implementation of a Tor Controller that supports authentication, creating v2 onion services, etc.
  • Allows the daemon to establish outbound connections to onion services.
  • Allows the daemon to listen for inbound connections by either automatically creating a v2 onion service or using a v3 onion service manually set up.
  • Updates the Tor documentation with the new relevant changes.

Fixes #186.

@Roasbeef Roasbeef added this to the 0.5 milestone May 1, 2018

@Roasbeef

Nice work! The addition of onion services will be a major boon to routing node operation due to the increased privacy, and also on the client end w.r.t completely handling NAT traversal.

I've yet to test this out locally, so this is an initial review pass. One thing I pointed out is that we'll need to ensure that we properly handle re-connecting back to inbound connections as it'll show the address of the proxy rather than the hidden service address.

The tor package contains utility functions that allow for interacting with the
Tor daemon. So far, supported functions include routing all traffic over Tor's
exposed socks5 proxy and routing DNS queries over Tor (A, AAAA, SRV). In the
future more features will be added: automatic setup of v2 hidden service

This comment has been minimized.

@Roasbeef

Roasbeef May 1, 2018

Member

FWIW, this PR implements most of these features. Still going through the set of commits, so perhaps it's reconciled later in the branch history.

This comment has been minimized.

@wpaulino

wpaulino May 2, 2018

Collaborator

Fixed.

tor/net.go Outdated
if network != "tcp" {
return nil, fmt.Errorf("cannot dial non-tcp network via Tor")
}
return Dial(address, p.SOCKSPort, p.StreamIsolation)

This comment has been minimized.

@Roasbeef

Roasbeef May 1, 2018

Member

Do all these messages need to be public? Given that most callers will interact directly with the this ProxyNet struct?

This comment has been minimized.

@Roasbeef

Roasbeef May 1, 2018

Member

On a second glance, I guess they're nice as it lets callers use specific functionality.

This comment has been minimized.

@wpaulino

wpaulino May 2, 2018

Collaborator

It's either we make them public or we have a constructor for ProxyNet specifying them, so it's pretty much the same thing. If we happen to add some private vars to ProxyNet then we can update it to use a constructor instead.

tor/tor.go Outdated
)
const (
localhost = "127.0.0.1"

This comment has been minimized.

@Roasbeef

Roasbeef May 1, 2018

Member

Do we also need to consider IPv6 localhost, or nah?

This comment has been minimized.

@wpaulino

wpaulino May 2, 2018

Collaborator

Fixed this so that callers can specify the host:port of the SOCKS proxy, rather than just the port. Within lnd, the default is still set to IPv4 localhost.

// Tor will create a new circuit for each set of credentials.
var auth *proxy.Auth
if streamIsolation {
var b [16]byte

This comment has been minimized.

@Roasbeef

Roasbeef May 1, 2018

Member

Nice, this gives us one less dependnacy!

(no need for btcsuite/go-socks anymore)

tor/tor.go Outdated
// Once connected, we'll construct the SRV request for the host
// following the format _service._proto.name. as described in RFC #2782.
host := "_" + service + "._" + proto + "." + name + "."

This comment has been minimized.

@Roasbeef

Roasbeef May 1, 2018

Member

I'll need to double check to ensure that this is compliant with the new DNS scheme, take not of this commit which recently modified the way we construct this query for regular UDP:

This comment has been minimized.

@wpaulino

wpaulino May 2, 2018

Collaborator

Looks good to me. We could call LookupSRV("nodes", "tcp", "nodes.lightning.directory") and that would translate to _nodes._tcp.nodes.lightning.directory., which follows the changes in the commits above.

return nil, errors.New("private key not found in reply")
}
err := ioutil.WriteFile(privateKeyFile, []byte(privateKey), 0600)

This comment has been minimized.

@Roasbeef

Roasbeef May 1, 2018

Member

Given the comment above, perhaps we should encode this in the proper format, so we don't need to do any special decoding on start up.

This comment has been minimized.

@wpaulino

wpaulino May 2, 2018

Collaborator

See comment above.

@@ -604,6 +657,10 @@ func (s *server) Stop() error {
close(s.quit)
if s.torController != nil {

This comment has been minimized.

@Roasbeef

Roasbeef May 1, 2018

Member

Don't see where the controller was even started in the first place.

This comment has been minimized.

This comment has been minimized.

@wpaulino

wpaulino May 31, 2018

Collaborator

This has now been moved to server.Start().

config.go Outdated
defaultTorDNS = "soa.nodes.lightning.directory:53"
defaultTorSOCKSPort = 9050
defaultTorControlPort = 9051
defaultTorPrivateKeyFilename = "onion_private_key"

This comment has been minimized.

@Roasbeef

Roasbeef May 1, 2018

Member

May want to prefix this with v2 (the variable name), and also the file name since a future version might let the user specify their own private key when we extend to also support auto v3 over the control port.

This comment has been minimized.

@wpaulino

wpaulino May 2, 2018

Collaborator

Fixed.

config.go Outdated
ControlPort int `long:"controlport" description:"The port that Tor is listening on for Tor control connections -- NOTE port must be between 1024 and 65535"`
V2 bool `long:"v2" description:"Automatically set up a v2 onion service to listen for inbound connections"`
V3 bool `long:"v3" description:"Use a v3 onion service to listen for inbound connections"`
PrivateKeyPath string `long:"privatekeypath" description:"The path to the private key of the onion service being created"`

This comment has been minimized.

@Roasbeef

Roasbeef May 1, 2018

Member

Similar comment here w.r.t to v2 prefix.

This comment has been minimized.

@wpaulino

wpaulino May 2, 2018

Collaborator

Fixed.

config.go Outdated
@@ -84,6 +86,8 @@ var (
defaultBitcoindDir = btcutil.AppDataDir("bitcoin", false)
defaultLitecoindDir = btcutil.AppDataDir("litecoin", false)
defaultTorPrivateKeyPath = filepath.Join(defaultLndDir, defaultTorPrivateKeyFilename)

This comment has been minimized.

@Roasbeef

Roasbeef May 1, 2018

Member

Similar comment here w.r.t to v2 prefix.

This comment has been minimized.

@wpaulino

wpaulino May 2, 2018

Collaborator

Fixed.

@wpaulino wpaulino force-pushed the wpaulino:onion-service-support branch from a5fa007 to 01161a1 May 2, 2018

@halseth

I don't have much to pick on for this PR! Mostly LGTM, and I have also tested this together with @wpaulino, working as advertised!

I dig how easy it is to set up lnd for Tor, new world order where lnd is over Tor by default incoming 😈

// Dial uses the Tor Dial function in order to establish connections through
// Tor. Since Tor only supports TCP connections, only TCP networks are allowed.
func (p *ProxyNet) Dial(network, address string) (net.Conn, error) {

This comment has been minimized.

@halseth

halseth May 7, 2018

Collaborator

Why not move this to tor.go, and just put the connection logic in here (instead of making these wrap the calls to Dial(..) etc)?

This comment has been minimized.

@wpaulino

wpaulino May 31, 2018

Collaborator

My guess is in order to provide calls like the net package does without the need of a ProxyNet struct.

This comment has been minimized.

@halseth

halseth Jun 1, 2018

Collaborator

Still don't see why you would need that, if the calls are only done only for ProxyNet anyway.

// Once retrieved, we'll ensure these values are of proper length when
// decoded.
serverHash, exists := replyParams["SERVERHASH"]
if !exists {

This comment has been minimized.

@halseth

halseth May 7, 2018

Collaborator

style nit: usually ok is used

This comment has been minimized.

@wpaulino

wpaulino May 31, 2018

Collaborator

Fixed.

}
decodedServerHash, err := hex.DecodeString(serverHash)
if err != nil {
return err

This comment has been minimized.

@halseth

halseth May 7, 2018

Collaborator

Should append to the error which string decoding is failing.

This comment has been minimized.

@wpaulino

wpaulino May 31, 2018

Collaborator

Fixed.

}
decodedServerNonce, err := hex.DecodeString(serverNonce)
if err != nil {
return err

This comment has been minimized.

@halseth

halseth May 7, 2018

Collaborator

same

This comment has been minimized.

@wpaulino

wpaulino May 31, 2018

Collaborator

Fixed.

func computeHMAC256(key, message []byte) []byte {
mac := hmac.New(sha256.New, key)
mac.Write(message)
return mac.Sum(nil)

This comment has been minimized.

@halseth

halseth May 7, 2018

Collaborator

would the last to lines be equivalent to just doing return mac.Sum(message)?

This comment has been minimized.

@cfromknecht

cfromknecht May 10, 2018

Collaborator

Sum prepends the message to the current hash. w/o any calls to Write, it would return message || H_K(nil)

This comment has been minimized.

config.go Outdated
Control: defaultTorControl,
// TODO: change this to v3 once we're able to
// automatically set them up.
V2: true,

This comment has been minimized.

@halseth

halseth May 7, 2018

Collaborator

not sure if it's obvious that this will be true by default. Looks like the default is not shown in lnd -h

This comment has been minimized.

@cfromknecht

cfromknecht May 11, 2018

Collaborator

in testing, i found that this causes a collision when trying to run v3, i get this error:
either tor.v2 or tor.v3 can be set, but not both
I had to set tor.v2=0 and tor.v3=1 to get around it

This comment has been minimized.

@wpaulino

wpaulino May 31, 2018

Collaborator

Fixed.

server.go Outdated
// future, this will be expanded to do so.
if cfg.Tor.Active && cfg.Tor.V2 {
s.torController = tor.NewController(cfg.Tor.Control)
if err := s.torController.Start(); err != nil {

This comment has been minimized.

@halseth

halseth May 7, 2018

Collaborator

Maybe this can be moved to the server's Start method?

This comment has been minimized.

@wpaulino

wpaulino May 31, 2018

Collaborator

Fixed.

## 2. Outbound Connections Only
protocol traffic_ is tunneled over Tor. Users must ensure that when also running
a backend node, that it is also proxying all traffic over Tor. If using the

This comment has been minimized.

@halseth

halseth May 7, 2018

Collaborator

I think we should keep the "Bitcoin full-node" jargon here, as "backend node" might not be known to as many.

This comment has been minimized.

@wpaulino

wpaulino May 31, 2018

Collaborator

Fixed.

@cfromknecht

Looking good! I've tested both v2 and v3 and both work :)

tor/net.go Outdated
// RegularNet is an implementation of the Net interface that defines behaviour
// for regular network connections.
type RegularNet struct{}

This comment has been minimized.

@cfromknecht

cfromknecht May 11, 2018

Collaborator

naming suggestion: ClearNet? :)

This comment has been minimized.

@wpaulino

wpaulino May 31, 2018

Collaborator

Fixed.

func computeHMAC256(key, message []byte) []byte {
mac := hmac.New(sha256.New, key)
mac.Write(message)
return mac.Sum(nil)

This comment has been minimized.

tor/net.go Outdated
// ResolveTCPAddr uses the Tor ResolveTCPAddr function in order to resolve TCP
// addresses over Tor.
func (p *ProxyNet) ResolveTCPAddr(network, address string) (*net.TCPAddr, error) {
if network != "tcp" {

This comment has been minimized.

@cfromknecht

cfromknecht May 11, 2018

Collaborator

should this check include "tcp4" and "tcp6"?

This comment has been minimized.

@wpaulino

wpaulino May 31, 2018

Collaborator

Yep! Fixed.

tor/tor.go Outdated
// Once connected, we'll construct the SRV request for the host
// following the format _service._proto.name. as described in RFC #2782.
host := "_" + service + "._" + proto + "." + name + "."

This comment has been minimized.

@cfromknecht

cfromknecht May 11, 2018

Collaborator

Would string formatting be clearer here?

This comment has been minimized.

@wpaulino

wpaulino May 31, 2018

Collaborator

Fixed.

config.go Outdated
Control: defaultTorControl,
// TODO: change this to v3 once we're able to
// automatically set them up.
V2: true,

This comment has been minimized.

@cfromknecht

cfromknecht May 11, 2018

Collaborator

in testing, i found that this causes a collision when trying to run v3, i get this error:
either tor.v2 or tor.v3 can be set, but not both
I had to set tor.v2=0 and tor.v3=1 to get around it

```
Once v3 onion service support is stable, `lnd` will be updated to also
automatically set up v3 onion services.

This comment has been minimized.

@cfromknecht

cfromknecht May 11, 2018

Collaborator

These docs are great! :) I might suggest adding a sample torrc for v3 nodes as well, it may not be immediately obvious for everyone wrt. what needs to change from the sample v2 torrc given above.

This comment has been minimized.

@wpaulino

wpaulino May 31, 2018

Collaborator

Docs added :)

--tor.active Allow outbound and inbound connections to be routed through Tor
--tor.socks= The port that Tor's exposed SOCKS5 proxy is listening on -- NOTE port must be between 1024 and 65535 (default: 9050)
--tor.dns= The DNS server as IP:PORT that Tor will use for SRV queries - NOTE must have TCP resolution enabled (default: soa.nodes.lightning.directory:53)
--tor.streamisolation Enable Tor stream isolation by randomizing user credentials for each connection.

This comment has been minimized.

@cfromknecht

cfromknecht May 11, 2018

Collaborator

What does everyone think of defaulting to using streamisolation, and instead have a no-streamisolation option?

@wpaulino wpaulino force-pushed the wpaulino:onion-service-support branch from 8095722 to 4333c49 May 11, 2018

@wpaulino wpaulino force-pushed the wpaulino:onion-service-support branch 2 times, most recently from bdcbf49 to bb5cc04 May 31, 2018

@Roasbeef

This comment has been minimized.

Member

Roasbeef commented Jun 1, 2018

Needs a rebase to address conflict after the latest merge.

@wpaulino wpaulino force-pushed the wpaulino:onion-service-support branch from bb5cc04 to c02efa4 Jun 1, 2018

@Roasbeef

LGTM 🕺🏾

@cfromknecht

This comment has been minimized.

Collaborator

cfromknecht commented Jun 2, 2018

Just clarifying, current default is still no-streamisolation correct?

@Roasbeef

This comment has been minimized.

Member

Roasbeef commented Jun 2, 2018

Yep, it has performance implications as you'll incur the overhead of a new circuit for each stream.

@Roasbeef

This comment has been minimized.

Member

Roasbeef commented Jun 2, 2018

Also there're diff forms of isolation: per user, per connection, etc. We can revisit this in the future though, no reason to block this PR from inclusion.

config.go Outdated
cfg.Tor.Control, strconv.Itoa(defaultTorControlPort),
)
switch {
case !cfg.Tor.V2 && !cfg.Tor.V3:

This comment has been minimized.

@Roasbeef

Roasbeef Jun 2, 2018

Member

At this point, if either of them are active, we should ensure that lnd is only listening on local host.

This comment has been minimized.

@Roasbeef

Roasbeef Jun 2, 2018

Member

IMO it shouldn't be assumed that if the user wants to use tor for outbound connections, then they also want to use it for inbound connections. So if v2 and v3 aren't set, then we should also disable listening for the p2p interface all together.

server.go Outdated
@@ -860,6 +913,46 @@ func (s *server) peerBootstrapper(numTargetPeers uint32,
}
}
func (s *server) initTorController() error {

This comment has been minimized.

@Roasbeef

Roasbeef Jun 2, 2018

Member

Missing godoc comment.

config.go Outdated
cfg.Tor.Control, strconv.Itoa(defaultTorControlPort),
)
switch {
case !cfg.Tor.V2 && !cfg.Tor.V3:

This comment has been minimized.

@Roasbeef

Roasbeef Jun 2, 2018

Member

IMO it shouldn't be assumed that if the user wants to use tor for outbound connections, then they also want to use it for inbound connections. So if v2 and v3 aren't set, then we should also disable listening for the p2p interface all together.

config.go Outdated
case cfg.Tor.V2 && cfg.Tor.V3:
return nil, errors.New("either tor.v2 or tor.v3 can be set, " +
"but not both")
case cfg.DisableListen && (cfg.Tor.V2 || cfg.Tor.V3):

This comment has been minimized.

@Roasbeef

Roasbeef Jun 5, 2018

Member

Should be a check right below this to disable listening (automatically) if tor is set, but v2 and v3 aren't.

config.go Outdated
@@ -434,6 +434,12 @@ func loadConfig() (*config, error) {
case cfg.DisableListen && (cfg.Tor.V2 || cfg.Tor.V3):
return nil, errors.New("listening must be enabled when " +
"enabling inbound connections over Tor")
case !cfg.Tor.V2 || !cfg.Tor.V3:

This comment has been minimized.

@Roasbeef

Roasbeef Jun 5, 2018

Member

Should be more like:

case cfg.Tor && (!cfg.Tor.V2 || !cfg.Tor.V3):

Otherwise would always do this.

wpaulino and others added some commits Apr 25, 2018

tor: streamline package to better follow the Effective Go guidelines
In this commit, we clean up the tor package to better follow the
Effective Go guidelines. Most of the changes revolve around naming,
where we'd have things like `torsvc.TorDial`. This was simplified to
`tor.Dial` along with many others.
tor: add onion address implementation
Co-Authored-By: Eugene <crypt-iq@users.noreply.github.com>
lnwire: implement serialization of onion addresses
Co-Authored-By: Eugene <crypt-iq@users.noreply.github.com>
channeldb: implement serialization of onion addresses
Co-Authored-By: Eugene <crypt-iq@users.noreply.github.com>

wpaulino and others added some commits Apr 30, 2018

tor: return the connection's actual remote address rather than the pr…
…oxy's

In this commit, we fix an issue where connections made through Tor's
SOCKS proxy would result in the remote address being the address of the
proxy itself

We fix this by using an internal proxyConn struct that sets the correct
address at the time of the connection.
tor: add inital tor controller implementation
In this commit, we add our inital implementation of a Tor Controller.
This commit includes the ability for the controller to automatically
signal the Tor daemon to create a v2 onion service. This will be
expanded later on to support creating v3 onion services.

Before allowing the controller to interact with the Tor daemon, the
connection must be authenticated first. This commit includes support for
the SAFECOOKIE authentication method as a sane default.

Co-Authored-By: Eugene <crypt-iq@users.noreply.github.com>
config+lnd: update tor config to include onion services flags
In this commit, we update the set of Tor flags to use sane defaults when
not specified. We also include some new flags related to the recent
onion services changes. This allows users to easily get set up on Tor by
only specifying the tor.active flag. If needed, the defaults can still
be overridden.
server: add support to advertise onion addresses
In this commit, we allow `lnd` to properly parse onion addresses in
order to advertise them to the network when set through the
`--externalip` flag.

Co-Authored-By: Eugene <crypt-iq@users.noreply.github.com>
rpcserver: add support to connect to onion addresses
In this commit, we now allow connections to onion addresses due to
recently adding support to properly parse them.

Co-Authored-By: Eugene <crypt-iq@users.noreply.github.com>
discovery+server: use network-specific functions for fallback SRV lookup
In this commit, we fix a bug where a fallback SRV lookup would leak
information if `lnd` was set to route connections over Tor. We solve
this by using the network-specific functions rather than the standard
ones found in the `net` package.
server: add automatic creation of v2 onion services
In this commit, we allow the daemon to use the recently introduced Tor
Controller implementation. This will automatically create a v2 onion
service at startup in order to listen for inbound connections over Tor.

Co-Authored-By: Eugene <crypt-iq@users.noreply.github.com>
server: use advertised address when reestablishing inbound connections
In this commit, we update the way we reestablish inbound connections if
we lose connectivity to a node we have an open channel with. Rather than
fetching the node's advertised port, we'll fetch one of their advertised
addresses instead. This ensure that if the remote node is running behind
a proxy, we do not see the proxy's address.
multi: ensure addresses are no longer assumed to be TCP addresses only
In this commit, we go through the codebase looking for TCP address
assumptions and modifying them to include the recently introduced onion
addresses. This enables us to fully support onion addresses within the
daemon.

@wpaulino wpaulino force-pushed the wpaulino:onion-service-support branch from 8481519 to 2e0484b Jun 5, 2018

@Roasbeef

LGTM ☄️

@Roasbeef Roasbeef merged commit d98d452 into lightningnetwork:master Jun 5, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 54.506%
Details

@wpaulino wpaulino deleted the wpaulino:onion-service-support branch Jun 5, 2018

@bobberb

This comment has been minimized.

bobberb commented Jun 6, 2018

regarding lnd/pull/1159/commits/2e0484be19e1a146895489b1f25cdfce8b23f589 - now we can set our domain names and subdomains as the target?

thank you @wpaulino

@wpaulino wpaulino referenced this pull request Jun 27, 2018

Closed

lnd with a reverse proxy #1039

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment