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

rpc+server: ensure we don't leak unadvertised nodes within invoice routing hints #1981

Merged
merged 9 commits into from Oct 26, 2018

Conversation

Projects
None yet
4 participants
@wpaulino
Collaborator

wpaulino commented Sep 26, 2018

In this commit, we ensure that we don't include routing hints for
unadvertised nodes at the time of invoice creation. Otherwise, this
would lead us to leak these unadvertised nodes to anyone who can get
their hands on the invoice being created. To prevent this, we'll now
look at the network graph and ensure that the node in unadvertised if
all of their edges are unadvertised and only extend to us.

@wpaulino wpaulino assigned wpaulino and unassigned wpaulino Sep 27, 2018

@wpaulino wpaulino added this to the 0.5.1 milestone Sep 27, 2018

@Roasbeef

Nice! This patches a lingering hole w.r.t our handling of non-advertised channels for invoices. The second hole will be patched once we stop showing non-advertised channels by default in the output of describegraph. I have no major comments other than possibly , puling in the new method into the channeldb package, rather than having tacking on a totally unrelated method to the server struct. Generally we should stop adding these utility methods/functions to the server, as we need to get rid of the "God Pointer" pattern, and properly abstract the messaging passing between sub-systems via interfaces. Also pulling this method into the LightningNode struct itself will allow us to effectively unit test it.

server.go Outdated
// publicly advertised within the network.
func (s *server) isNodePublic(pubKey *btcec.PublicKey) (bool, error) {
// First, we'll attempt to retrieve the node from the graph.
remoteNode, err := s.chanDB.ChannelGraph().FetchLightningNode(pubKey)

This comment has been minimized.

@Roasbeef

Roasbeef Oct 3, 2018

Member

Isn't this enough? In that if the node isn't found in our graph, then we can assume that it likely has purely non-advertised channels. For example, here's me trying to lookup laptop node (running the app on testnet) from the faucet:

kek@segspam:~/gocode/src/github.com/lightningnetwork/lnd# tlncli getnodeinfo 028916c993156097a1fdb5b4bbe4116f63bf9afbf4c7210d1c638dd2127abe7ed4
tlncli: command not found

This comment has been minimized.

@Roasbeef

Roasbeef Oct 3, 2018

Member

Ah actually this doesn't work if a direct connection exists between the two endpoints.

server.go Outdated
// terminate the check early.
nodeIsPublic := false
ourPub := s.identityPriv.PubKey().SerializeCompressed()
errDone := errors.New("done")

This comment has been minimized.

@Roasbeef

Roasbeef Oct 3, 2018

Member

Nice touch with errDone here.

server.go Outdated
// isNodePublic attempts to determine if a node with the given public key is
// publicly advertised within the network.
func (s *server) isNodePublic(pubKey *btcec.PublicKey) (bool, error) {

This comment has been minimized.

@Roasbeef

Roasbeef Oct 3, 2018

Member

Seems like we can pull this into a method directly on the node itself in the database. This would allow us to unit test this section with a small mock graph, possibly writing some helper methods to make it easier to make table-driven tests for this use case.

This comment has been minimized.

@wpaulino

wpaulino Oct 18, 2018

Collaborator

Fixed.

@Roasbeef

This comment has been minimized.

Member

Roasbeef commented Oct 3, 2018

Will start to test this on the faucet (using laptop app).

server.go Outdated
ourPub := s.identityPriv.PubKey().SerializeCompressed()
errDone := errors.New("done")
err = remoteNode.ForEachChannel(nil, func(_ *bolt.Tx,

This comment has been minimized.

@halseth

halseth Oct 4, 2018

Collaborator

Seems like this can be simplified down to checking whether we have a node announcement for the node?

This comment has been minimized.

@Roasbeef

Roasbeef Oct 8, 2018

Member

That's insufficient as we'll still have a node announcement for nodes that we have private channels with.

This comment has been minimized.

@halseth

halseth Oct 8, 2018

Collaborator

Interesting, looking at the gossiper it seems like we are not checking this before forwarding incoming node announcements, meaning we potentially are leaking their presence there as well.

This comment has been minimized.

@halseth

halseth Oct 8, 2018

Collaborator

I'm also wondering if it makes sense for a node to send a node announcement if it plans to stay off the radar.

This comment has been minimized.

@wpaulino

wpaulino Oct 9, 2018

Collaborator

Huh, we do forward incoming node announcements without first ensuring that the node is public. Not sending a node announcement when opening private channels might make sense, although some of the information encoded within it (feature bits, addresses, etc.) might prove to be useful for the channel counterparty. Will make a sweep for the remaining checks within the codebase and update the PR.

This comment has been minimized.

@halseth

halseth Oct 10, 2018

Collaborator

Agreed. We should send node announcements also for private channels, but only to the channel counterparty (non-broadcast).

This comment has been minimized.

@wpaulino

wpaulino Oct 18, 2018

Collaborator

Fixed!

@halseth

Alright, solid set of changes! 👍

nodeIsPublic, err = node.isPublic(tx, ourPubKey)
return err
})
if err != nil && err != ErrGraphNodeNotFound {

This comment has been minimized.

@halseth

halseth Oct 19, 2018

Collaborator

Why check ErrGraphNodeNotFound here?

This comment has been minimized.

@wpaulino

wpaulino Oct 22, 2018

Collaborator

Not needed indeed - fixed.

// If it doesn't, we'll skip broadcasting this announcement to
// the rest of our peers.
if !isPublic || err != nil {

This comment has been minimized.

@halseth

halseth Oct 19, 2018

Collaborator

just treat err first, as a regular error?

This comment has been minimized.

@wpaulino

wpaulino Oct 22, 2018

Collaborator

Fixed.

Show resolved Hide resolved discovery/gossiper.go Outdated
Show resolved Hide resolved discovery/gossiper_test.go
return fmt.Errorf("unable to retrieve current node "+
"announcement: %v", err)
}
if err := peer.SendMessage(true, &nodeAnn); err != nil {

This comment has been minimized.

@halseth

halseth Oct 19, 2018

Collaborator

Add a log before sending?

This comment has been minimized.

@wpaulino

wpaulino Oct 22, 2018

Collaborator

This should already be covered by the PEER log, no?

This comment has been minimized.

@halseth

halseth Oct 25, 2018

Collaborator

yes, but if we are grepping on funding logs it would be nice to easily find it.

This comment has been minimized.

@wpaulino

wpaulino Oct 25, 2018

Collaborator

Fixed.

// Ensure we only forward nodes that are publicly advertised to
// prevent leaking information about nodes.
isNodePublic, err := c.graph.IsPublicNode(nodeAnn.PubKeyBytes)
if !isNodePublic || err != nil {

This comment has been minimized.

@halseth

halseth Oct 19, 2018

Collaborator

same, error should be treated.

This comment has been minimized.

@wpaulino

wpaulino Oct 22, 2018

Collaborator

Fixed.

if err != nil {
rpcsLog.Errorf("Unable to get link for "+
"channel %v: %v", chanPoint, err)
if err != nil || !link.EligibleToForward() {

This comment has been minimized.

@halseth

halseth Oct 19, 2018

Collaborator

I think we should keep the existing way of logging the error.

This comment has been minimized.

@wpaulino

wpaulino Oct 22, 2018

Collaborator

Fixed.

Show resolved Hide resolved rpcserver.go
var remotePub [33]byte
copy(remotePub[:], channel.IdentityPub.SerializeCompressed())
isRemoteNodePublic, err := graph.IsPublicNode(remotePub)
if err != nil || !isRemoteNodePublic {

This comment has been minimized.

@halseth

halseth Oct 19, 2018

Collaborator

same, treat error

This comment has been minimized.

@wpaulino

wpaulino Oct 22, 2018

Collaborator

Fixed.

announceChan := completeChan.ChannelFlags&lnwire.FFAnnounceChannel != 0
if !announceChan {
fndgLog.Debugf("Will not announce private channel %v.",
shortChanID.ToUint64())
peerChan := make(chan lnpeer.Peer, 1)
f.cfg.NotifyWhenOnline(completeChan.IdentityPub, peerChan)

This comment has been minimized.

@cfromknecht

cfromknecht Oct 20, 2018

Collaborator

do we only intend to try once?

This comment has been minimized.

@wpaulino

wpaulino Oct 22, 2018

Collaborator

For now yeah. It can be extended once #1595 lands.

// If it doesn't, we'll skip broadcasting this announcement to
// the rest of our peers.
if !isPublic {

This comment has been minimized.

@halseth

halseth Oct 23, 2018

Collaborator

style nit: wrap the isPublic check around the existing append

This comment has been minimized.

@wpaulino

wpaulino Oct 24, 2018

Collaborator

Fixed.

Show resolved Hide resolved fundingmanager.go
@Roasbeef

This comment has been minimized.

Member

Roasbeef commented Oct 25, 2018

Needs rebase!

wpaulino added some commits Oct 17, 2018

channeldb/graph: add method to determine if a node is public
In this commit, we add a method to the ChannelGraph struct that
determines whether a node is seen as public based on graph's source
node's point of view.
discovery: ensure we only broadcast NodeAnnouncements of public nodes
In this commit, we modify the gossiper to no longer broadcast
NodeAnnouncements of nodes who intend to remain private. We do this to
prevent leaking their information to the greater network.
discovery/gossiper_test: modify TestProcessAnnouncement to process node
ann last

In this commit, we modify TestProcessAnnouncement to process the node
announcement last. We do this due to the recent change in the gossiper
where we'll only forward node announcements of nodes who intend to
advertised themselves within the network.

This change was needed in order to allow the node announcement to be
broadcast to the greater network, as otherwise the gossiper would assume
the node intends to stay private due to not having any advertised edges.
@halseth

Two nits, otherwise LGTM 🐐

Show resolved Hide resolved chan_series.go
}
pubKey := peer.PubKey()
fndgLog.Debugf("Sending our NodeAnnouncement to %x", pubKey)

This comment has been minimized.

@halseth

halseth Oct 25, 2018

Collaborator

log channelpoint as well

This comment has been minimized.

@wpaulino

wpaulino Oct 25, 2018

Collaborator

Done

cfromknecht and others added some commits Oct 1, 2018

lnd_test: process all node/edge updates in graph_top_itest
This commit modifies the graph topology test to
properly count channel updates and node
announcments in the event that they are batched
into a single topology update. The prior logic
made the assumption that they were always in
distinct topology updates, so this method should
be more general and robust.
fundingmanager: send NodeAnnouncement to unadvertised channel counter…
…party

In this commit, we modify the funding manager to send our
NodeAnnouncement to our channel counterparty in the event of an
unadvertised channel. We do this to ensure that our counterparty learns
about some information about us that may aid them in one way or another
(e.g., addresses to reconnect, features supported, etc.).
chan_series: filter out nodes who intend to remain private
In this commit, we filter out nodes who intend to remain private. We do
this to prevent leaking information about them by forwarding their
NodeAnnouncements.
rpc: ensure we don't leak unadvertised nodes within invoice routing h…
…ints

In this commit, we ensure that we don't include routing hints for
unadvertised nodes at the time of invoice creation. Otherwise, this
would lead us to leak these unadvertised nodes to anyone who can get
their hands on the invoice being created. To prevent this, we'll now
look at the network graph and ensure that the node in unadvertised if
all of their edges are unadvertised and only extend to us.
lnd_test: update testInvoiceRoutingHints to account
In this commit, we open an additional channel between Bob and Carol to
ensure that Bob gets selected as the only routing hint. Previously Bob
would get selected, but with the recent changes, it would no longer
happen due to him not having any advertised edges.
announceChan := completeChan.ChannelFlags&lnwire.FFAnnounceChannel != 0
if !announceChan {
fndgLog.Debugf("Will not announce private channel %v.",
shortChanID.ToUint64())
peerChan := make(chan lnpeer.Peer, 1)
f.cfg.NotifyWhenOnline(completeChan.IdentityPub, peerChan)

This comment has been minimized.

@Roasbeef

Roasbeef Oct 26, 2018

Member

We shouldn't send this before we actually send over the chan update and chan announce. Otherwise, if this is their first private channel for example, they won't have any visible channels in the graph, so we won't accept the node announcement. The timing is a bit tricky.

@Roasbeef

LGTM 🧞‍♀️

@Roasbeef Roasbeef merged commit f0b8cb1 into lightningnetwork:master Oct 26, 2018

2 checks passed

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

@wpaulino wpaulino deleted the wpaulino:routing-hints-unadvertised-nodes branch Oct 26, 2018

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