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

Add GlobalOnly Transport implementation blocking dials to private IPs #3669

Closed
mxinden opened this issue Mar 23, 2023 · 3 comments · Fixed by #3814
Closed

Add GlobalOnly Transport implementation blocking dials to private IPs #3669

mxinden opened this issue Mar 23, 2023 · 3 comments · Fixed by #3814
Labels
difficulty:easy getting-started Issues that can be tackled if you don't know the internals of libp2p very well help wanted

Comments

@mxinden
Copy link
Member

mxinden commented Mar 23, 2023

Description

Upstream the Kademlia Exporter GlobalOnly Transport implementation to rust-libp2p.

https://github.com/mxinden/kademlia-exporter/blob/master/src/exporter/client/global_only.rs

Motivation

Allows denying all dials to non-public IP addresses. E.g. helpful as dialing private IP addresses can be seen as an attack by cloud providers.

See also #2824 (comment).

Requirements

  1. No dependency on Rust nightly.

Open questions

Are you planning to do it yourself in a pull request?

No

@mxinden mxinden added difficulty:easy help wanted getting-started Issues that can be tackled if you don't know the internals of libp2p very well labels Mar 23, 2023
@tcoratger
Copy link
Contributor

@mxinden In the implementation you mention, there are few positions where is_global() function is used. For example, this one:

Some(Protocol::Ip4(a)) => {
    if a.is_global() {
        self.inner.dial(addr)
    } else {
        debug!("Not dialing non global IP address {:?}.", a);
        Err(TransportError::MultiaddrNotSupported(addr))
    }
}

Considering this PR, this function is notified as unstable. For that, we can use a #![feature(ip)] patch but this is not recommended on a stable version of libp2p. What do you think about that?

@dariusc93
Copy link
Contributor

@mxinden In the implementation you mention, there are few positions where is_global() function is used. For example, this one:

Some(Protocol::Ip4(a)) => {
    if a.is_global() {
        self.inner.dial(addr)
    } else {
        debug!("Not dialing non global IP address {:?}.", a);
        Err(TransportError::MultiaddrNotSupported(addr))
    }
}

Considering this PR, this function is notified as unstable. For that, we can use a #![feature(ip)] patch but this is not recommended on a stable version of libp2p. What do you think about that?

We could extract the logic of the function into our own function to determine if an ip is global or not. We could probably even copy https://github.com/libp2p/rust-libp2p/blob/master/protocols/autonat/src/behaviour.rs#L611-L721 over into a GlobalOnly transport (or maybe into some utility module that autonat and the transport can share?)

@thomaseizinger
Copy link
Contributor

@tcoratger We definitely need to avoid nightly.

I'd recommend we vendor the function in. The impl is not going to change because what is and isn't considered a global IP will be the same tomorrow and the week after :)

Thus, there is effectively no maintenance cost here.

@dariusc93 A shared module has the cost of a dependency which I'd like to avoid.

A private, internal function is a bit of duplication but we can remove that once it hits stable.

@mergify mergify bot closed this as completed in #3814 May 10, 2023
mergify bot pushed a commit that referenced this issue May 10, 2023
Upstreams the `GlobalOnly` transport from @mxinden's kademlia-exporter to rust-libp2p.

Source: https://github.com/mxinden/kademlia-exporter/blob/master/src/exporter/client/global_only.rs.

Resolves: #3669.

Pull-Request: #3814.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty:easy getting-started Issues that can be tackled if you don't know the internals of libp2p very well help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants