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

Request::remote_addr() is always None in server #1410

Closed
ensc opened this issue Jan 6, 2018 · 11 comments
Closed

Request::remote_addr() is always None in server #1410

ensc opened this issue Jan 6, 2018 · 11 comments

Comments

@ensc
Copy link

ensc commented Jan 6, 2018

Since 0.11.11 (0.11.10 is ok), the address returned by Request::remote_addr() is always None. Code is like

impl <'a> Service for HttpService<'a> {
    fn call(&self, req: Request) -> Self::Future {
        let addr = req.remote_addr();
        let file = self.env.new_file(path, &addr.unwrap().ip());    // <<<< panics here since 0.11.11
@seanmonstar
Copy link
Member

You're right, I forgot about this method. I'll add it back to prevent breaking people, but remote_addr will also be deprecated.

@moderation
Copy link

@seanmonstar with remote_addr being removed is there an alternative method or is the capability being removed all together?

@seanmonstar
Copy link
Member

The remote address likely won't be part of the Request any other way. However, each Service is created per connection, so the address can be stored in a Service. That should be trivial if managing a TcpListener yourself, but a way to access it when using Http::bind doesn't really exist, and could be explored.

@oa-metaswitch
Copy link

+1 for getting some way to access the TcpListener. I'm currently relying on getting access to the remote address, and it would be a shame to have to manage the transport layer manually. Particularly now that support for tokio_proto is going away.

@jedisct1
Copy link
Contributor

jedisct1 commented Feb 6, 2018

+1

@klausi
Copy link
Contributor

klausi commented Mar 4, 2018

My current approach to pass down the IP address to my service (a proxy server use case):

let address: SocketAddr = ([127, 0, 0, 1], port).into();
let mut core = Core::new().unwrap();
let handle = core.handle();

// We can't use Http::new().bind() because we need to pass down the
// remote source IP address to our proxy service. So we need to
// create a TCP listener ourselves and handle each connection to
// have access to the source IP address.
// @todo Simplify this once Hyper has a better API to handle IP
// addresses.
let listener = TcpListener::bind(&address, &handle)
    .chain_err(|| format!("Failed to bind server to address {}", address))?;
let client = Client::new(&handle);
let mut http = Http::<hyper::Chunk>::new();
// Let Hyper swallow IO errors internally to keep the server always
// running.
http.sleep_on_errors(true);

let server = listener.incoming().for_each(move |(socket, source_address)| {
    handle.spawn(
        http.serve_connection(socket, Proxy {
                port,
                upstream_port,
                client: client.clone(),
                source_address,
            })
            .map(|_| ())
            .map_err(|_| ())
    );
    Ok(())
});

println!("Listening on http://{}", address);
core.run(server).chain_err(|| "Tokio core run error")?;
bail!("The Tokio core run ended unexpectedly");

Full source: https://github.com/klausi/rustnish/blob/goal-09/src/lib.rs

@ensc
Copy link
Author

ensc commented Mar 4, 2018

@klausi I do not know how, but Http::bind() seems to workaround the 'static lifetime requirements of the inner function while they still exist when calling handel.spawn() manually. E.g. I tried to write this code but failed to implement bar().

@seanmonstar
Copy link
Member

If there's interest in getting this information even when hyper manages the listener, we could open a new issue about that. This related comment about an API in the client seems relevant.

bors-voyager bot added a commit to rust-lang/crates.io that referenced this issue Aug 15, 2018
1378: Replace `civet` with `hyper` as http server r=carols10cents a=jtgeibel

This is a work in progress implementation switching from `civet` to `hyper` as the base http server.

This is based on `conduit-hyper` which I've updated to `hyper 0.11`.  That source code is [here](https://github.com/jtgeibel/conduit-hyper/blob/master/src/lib.rs).  A huge shout-out to @sfackler and @drbawb as this is forked from their repos.  I'm curios if either of you are still using this code and if you have any feedback.  I plan to eventually publish this crate on crates.io, so please let me know if you would like to be added/removed as an author or if you have any objections.

There are several things I wish to investigate further, including:

* I'm not yet sure if hyper handles panics.  I see no results searching for `catch_unwind`, so I'm assuming panics need to be handled in this layer.
* Add some tests
* request.remote_addr() is deprecated, also see: hyperium/hyper#1410 (comment)

Co-authored-by: Justin Geibel <[email protected]>
@stevedonovan
Copy link

With 0.12, @klausi workaround no longer compiles. Anything further on this issue? It becomes important when providing a webhook for little devices that don't bother to fill in referer.

@klausi
Copy link
Contributor

klausi commented Sep 7, 2018

@stevedonovan my TCPListener workaround still works with Hyper 0.12:

let address: SocketAddr = ([127, 0, 0, 1], port).into();
let mut runtime = Runtime::new().unwrap();

let listener = TcpListener::bind(&address)
    .chain_err(|| format!("Failed to bind server to address {}", address))?;
let client = Client::new();
let http = Http::new();

let server = listener
    .incoming()
    .for_each(move |socket| {
        let source_address = socket.peer_addr().unwrap();
        tokio::spawn(
            http.serve_connection(
                socket,
                Proxy {
                    port,
                    upstream_port,
                    client: client.clone(),
                    source_address,
                },
            ).map(|_| ())
            .map_err(|_| ()),
        );
        Ok(())
    }).map_err(|e| panic!("accept error: {}", e));

println!("Listening on http://{}", address);
runtime.spawn(server);

@klausi
Copy link
Contributor

klausi commented Sep 7, 2018

Ah, just found the usual DoS security weakness again with my code from above. If your server needs to handle many requests and runs out of file descriptors then the code from above will panic and your server will exit and stop.

I wrote about this before and Hyper even has a protection against this built in now - but if you are constructing your TCP listener manually then you are on your own again :-(

scottlamb added a commit to scottlamb/moonfire-nvr that referenced this issue Nov 27, 2018
Some caveats:

  * it doesn't record the peer IP yet, which makes it harder to verify
    sessions are valid. This is a little annoying to do in hyper now
    (see hyperium/hyper#1410). The direct peer might not be what we want
    right now anyway because there's no TLS support yet (see #27).  In
    the meantime, the sane way to expose Moonfire NVR to the Internet is
    via a proxy server, and recording the proxy's IP is not useful.
    Maybe better to interpret a RFC 7239 Forwarded header (and/or
    the older X-Forwarded-{For,Proto} headers).

  * it doesn't ever use Secure (https-only) cookies, for a similar reason.
    It's not safe to use even with a tls proxy until this is fixed.

  * there's no "moonfire-nvr config" support for inspecting/invalidating
    sessions yet.

  * in debug builds, logging in is crazy slow. See libpasta/libpasta#9.

Some notes:

  * I removed the Javascript "no-use-before-defined" lint, as some of
    the functions form a cycle.

  * Fixed #20 along the way. I needed to add support for properly
    returning non-OK HTTP statuses to signal unauthorized and such.

  * I removed the Access-Control-Allow-Origin header support, which was
    at odds with the "SameSite=lax" in the cookie header. The "yarn
    start" method for running a local proxy server accomplishes the same
    thing as the Access-Control-Allow-Origin support in a more secure
    manner.
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

No branches or pull requests

7 participants