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

feat(identify): push changes of external addresses #4885

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

nazar-pc
Copy link
Contributor

@nazar-pc nazar-pc commented Nov 17, 2023

Description

Related: #4867.

Notes & open questions

None

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@nazar-pc nazar-pc force-pushed the identify-push-external-addresses branch from df1bcd2 to 1ffc4b9 Compare November 17, 2023 09:49
@nazar-pc
Copy link
Contributor Author

Interestingly, Kademlia test adding_an_external_addresses_activates_server_mode_on_existing_connections (at least) became non-deterministic, so I disabled this new feature for Kademlia tests.

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Thanks, just two comments!

I'd like to get @mxinden's input on whether this should be the default. It seems reasonable to me.

protocols/identify/src/behaviour.rs Outdated Show resolved Hide resolved
protocols/identify/src/behaviour.rs Outdated Show resolved Hide resolved
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Looks good to me apart from the bit-wise OR and the unexplained test failure.

Thank you for the patch.

protocols/kad/tests/client_mode.rs Show resolved Hide resolved
@thomaseizinger thomaseizinger changed the title feat(identify): support pushing changes of external addresses feat(identify): push changes of external addresses Nov 20, 2023
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@mxinden
Copy link
Member

mxinden commented Jan 14, 2024

A historical note, I believe this has not been done in the past due to the following attack:

Attacker continuously tells target a new (fake) external address of the target. Target sends N updates each time where N is the number of connected peers. I.e. an amplification attack.

That said, this was pre-autonat where we could validate external addresses. With AutoNATv2, and only pushing the update after validation, this should be fine.

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Jan 15, 2024

Attacker continuously tells target a new (fake) external address of the target. Target sends N updates each time where N is the number of connected peers. I.e. an amplification attack.

That said, this was pre-autonat where we could validate external addresses. With AutoNATv2, and only pushing the update after validation, this should be fine.

This attack is a problem if we treat an observed address as an external address. We don't do that but users might easily do (take the identify event and call Swarm::add_external_address with it).

This makes me think that this behaviour should definitely not be the default. Instead, I think we should put a good warning and explanation to this option that educates users about this attack vector.

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