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

Only accept an address from a peer if it is a valid IP address #6439

Merged
merged 1 commit into from
Jan 19, 2024

Conversation

matthew1001
Copy link
Contributor

PR description

When using the From address specified by a peer, ensure it is a valid IP address. Otherwise revert to using the UDP source host.

Fixed Issue(s)

See comment #6225 (comment)

Copy link

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.
  • I thought about the changelog and included a changelog update if required.
  • If my PR includes database changes (e.g. KeyValueSegmentIdentifier) I have thought about compatibility and performed forwards and backwards compatibility tests

@matthew1001 matthew1001 changed the title Only accept a address from a peer if it is a valid IP address Only accept an address from a peer if it is a valid IP address Jan 19, 2024
Copy link
Contributor

@matkt matkt left a comment

Choose a reason for hiding this comment

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

LGTM

@matthew1001
Copy link
Contributor Author

I'll submit another PR to honour DNS names DNS peering is enabled for the node, but it would be good to just get this fix in first.

@matthew1001 matthew1001 merged commit 98718ae into hyperledger:main Jan 19, 2024
9 checks passed
Comment on lines +296 to +298
.filter(
fromAddr ->
(!fromAddr.equals("127.0.0.1") && InetAddresses.isInetAddress(fromAddr)))
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like we need to add 255.255.255.255 to this filter. on mainnet we are regularly seeing logs like:

{"@timestamp":"2024-01-24T06:18:16,129","level":"WARN","thread":"vert.x-eventloop-thread-3","class":"VertxPeerDiscoveryAgent","message":"Sending to peer DiscoveryPeer{status=bonding, enode=enode://5d01e72875f3485fdcce5255720696087ea7d704c6632282fbe39329db6417a6f5ef940cf05519677be7f9c31b6b553fe9192e9e9feed37db3fb97bcc7fd099e@255.255.255.255:30313, firstDiscovered=1706077069751, lastContacted=0, lastSeen=1706077069751} failed, native error code -13, packet: 0x3123aed37feabe25bd4cc2625a1d1e2b12a5a1bbe6f3ef594c1ea76abbfbae670ab30477655186a5a3bf4487f2fa183807dad26037e950e89cc07ba20eaf229437e69c2e56d44fe934082147098ca4a0bd99964d67d2046789d080f69d5be2c60101df05cb8489757c7882765f82765fcb84ffffffff8276698276698465b0abe402, stacktrace: io.netty.channel.unix.Errors$NativeIoException: sendToAddress(..) failed: Permission denied","throwable":""}

Copy link
Contributor

Choose a reason for hiding this comment

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

A more robust solution would be to filter by reachable addresses

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could possibly do both - specifically exclude 255.255.255.255 and also apply a InetAddress.isReachable() check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll look at raising a PR that at least adds 255.255.255.255 to the exclude list, possibly with some refactoring of the filter predicate now that it's becoming more complicated.

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.

3 participants