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

fix: add ipv6Check regex for private address #2624

Merged
merged 3 commits into from
Jul 26, 2024

Conversation

marcus-pousette
Copy link
Contributor

Title

fix: add ipv6Check regex for private address

Description

Solves #2623

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
  • I have added tests that prove my fix is effective or that my feature works

@marcus-pousette marcus-pousette requested a review from a team as a code owner July 18, 2024 10:44
@achingbrain
Copy link
Member

achingbrain commented Jul 18, 2024

Will this handle other types of IPv4-Mapped IPv6 addresses made from ipv4 addresses?

E.g.

10.0.0.1 -> ::FFFF:0a00:1 - should be detected as private
142.250.74.238 -> ::FFFF:8efa:4aee - should be detected as public

@marcus-pousette
Copy link
Contributor Author

Will this handle other types of IPv4-Mapped IPv6 addresses made from ipv4 addresses?

E.g.

10.0.0.1 -> ::FFFF:0a00:1 - should be detected as private 142.250.74.238 -> ::FFFF:8efa:4aee - should be detected as public

aaah yes, I realised that the pattern is not perhaps right given your explanation in the github issue

@marcus-pousette
Copy link
Contributor Author

marcus-pousette commented Jul 18, 2024

Hmm this was a bit tricky, it seems like the multiaddr function removes the leading zeros, so for the

example
::ffff:0a00:1

we actually match against
::ffff:a00:1

the same can be for one or many leading zeros,

It feels like this regex problem should have been solved many times other by someone else already. But maybe not in the the flavor how multaddr mutates the string

@achingbrain
Copy link
Member

achingbrain commented Jul 18, 2024

Agreed, it doesn't sound like this should be a new problem.. That said, our version comes from the private-ip module, which hasn't seen an update to these patterns for years.

@marcus-pousette
Copy link
Contributor Author

I did a little bit of a not-like looking regex now where I explicitly try to match against some local ones. It works but might not be ideal

@marcus-pousette
Copy link
Contributor Author

Agreed, it doesn't sound like this should be a new problem.. That said, our version comes from the private-ip module, which hasn't seen an update to these patterns for years.

this one might be right, but the problem is that toString() on a multiaddrs yield a format where leading 0s are removed which I guess does not work well with this pattern

Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

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

This is nearly there - I've suggested a few more test cases for public addresses - it's failing on the first address above the class B range - 172.32.0.1, the others seem fine.

multiaddr('/ip6/::ffff:6500:1a5a/tcp/1000'), // 101.0.26.90
multiaddr('/ip6/::ffff:2801:1409/tcp/1000'), // 40.1.20.9
multiaddr('/ip6/::ffff:5ca8:0001/tcp/1000'), // 92.168.0.1 (not a private range)
multiaddr('/ip6/::ffff:0200:0010/tcp/1000') // 2.16.0.1 (not a private range)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
multiaddr('/ip6/::ffff:0200:0010/tcp/1000') // 2.16.0.1 (not a private range)
multiaddr('/ip6/::ffff:0200:0010/tcp/1000'), // 2.16.0.1 (not a private range)
multiaddr('/ip6/::ffff:ac09:0001/tcp/1000'), // 172.15.0.1 (not a private range)
multiaddr('/ip6/::ffff:ac20:0001/tcp/1000'), // 172.32.0.1 (not a private range)
multiaddr('/ip6/::ffff:c0a7:0001/tcp/1000'), // 192.167.0.1 (not a private range)
multiaddr('/ip6/::ffff:c0a9:0001/tcp/1000') // 192.169.0.1 (not a private range)

@achingbrain achingbrain merged commit a82ff82 into libp2p:main Jul 26, 2024
24 checks passed
@achingbrain achingbrain mentioned this pull request Jul 26, 2024
@marcus-pousette
Copy link
Contributor Author

thanks for making the changes! did not have time to continue this week

@achingbrain
Copy link
Member

No problem - I wanted to get it in the next release, thanks for finding the issue and opening the PR!

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.

2 participants