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

Implement HTTP request in offchain workers #3447

Closed
wants to merge 3 commits into from

Conversation

@tomaka
Copy link
Member

commented Aug 20, 2019

Implements the currently-unimplemented API.

I'm opening this PR as a draft because I'm going to be away for a bit, and I encourage someone to finish it if they want.

The only thing that's not implemented is a small TODO to fix: we unwrap if we fail to initialize the TlsConnector. How is it even possible to fail to initialize TLS? I have absolutely no idea, and I think it's a platform-specific "just in case" thingy. But since the word unwrap is explicitly written in the code, I guess we have to handle this situation.
I'd like to fix that by having some enum { HttpConnector, HttpsConnector } where we fall back to the HttpConnector if the HTTPS one fails to initialize. We could also just return the error and print a big error saying "failed to initialize offchain workers". But considering that this is the only reason why offchain workers can fail to initialize, it's a bit annoying.

In order to implement this, I had to properly document/define how the API is to be used.

From the point of view of the offchain worker, the order in which functions must be called on an HTTP request is: initialization (start), adding headers, sending non-empty body, sending empty body, waiting for answer, reading headers, reading body until EOF.

Most steps can be skipped (for example, reading the body implicitly waits for the answer), but changing the order of the steps is generally not allowed. The exceptions are: you are still allowed to wait for answer or read response headers after having started to read the body, as long as EOF hasn't been returned.

As side changes, I also:

  • Moved out some utility functions into a new timestamp module.
  • Renamed HttpRequestStatus::Unknown to Invalid and Timeout to IoError (so that we also handle the situation where the remote force-closes the socket for example).

@tomaka tomaka requested a review from tomusdrw Aug 20, 2019

tomaka added 2 commits Aug 20, 2019
@bkchr

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

Why not use surf, they claim to support wasm as a target?

@tomaka

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

Why not use surf, they claim to support wasm as a target?

They seem to require async/await.

@bkchr

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

Ahh okay.
Another question, how do we handle certificates? Or do we just ignore validating them?

@tomaka

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

Another question, how do we handle certificates? Or do we just ignore validating them?

This is done by the hyper_tls library. We use the regular TLS process, with certificate authorities and all.

@tomusdrw
Copy link
Contributor

left a comment

🎉 🎉 👍 OMG, it's so clean! I'll take a shot in falling back to HttpConnector tomorrow.


let (request, _sender) = match self.requests.remove(id) {
Some(HttpApiRequest::NotDispatched(rq, s)) => (rq, s),
_ => unreachable!("we checked for NotDispatched above; qed")

This comment has been minimized.

Copy link
@tomusdrw

tomusdrw Aug 21, 2019

Contributor

Wouldn't it be clearer to remove() the request earlier and just insert it back if we continue? This doesn't seem super future-proof.

}

// Convert the deadline into a `Future` that resolves when the deadline is reached.
let mut deadline = future::maybe_done(match deadline {

This comment has been minimized.

Copy link
@tomusdrw

tomusdrw Aug 21, 2019

Contributor

Should we extract this to a function? Seems repeated in other methods.

This comment has been minimized.

Copy link
@tomaka

tomaka Aug 23, 2019

Author Member

We could put that in the timestamp.rs module indeed.

This comment has been minimized.

Copy link
@tomusdrw

tomusdrw Aug 23, 2019

Contributor

Ok, will do in #3461

}

// If we reach here, that means the `current_read_chunk` is empty and needs to be
// filled with a new chunk from `body`. We block on either the next body of the

This comment has been minimized.

Copy link
@tomusdrw

tomusdrw Aug 21, 2019

Contributor
Suggested change
// filled with a new chunk from `body`. We block on either the next body of the
// filled with a new chunk from `body`. We block on either the next body or the
let mut next_body = future::maybe_done(response.body.next());
futures::executor::block_on(future::select(&mut next_body, &mut deadline));

if let future::MaybeDone::Done(next_body) = next_body {

This comment has been minimized.

Copy link
@tomusdrw

tomusdrw Aug 21, 2019

Contributor

Just to make sure: even if deadline is reached we are not going to discard the next body chunk, right? The fact that you've started polling the response.body.next() doesn't cause any issues?

This comment has been minimized.

Copy link
@tomaka

tomaka Aug 23, 2019

Author Member

Normally no. Calling next() wraps around a &mut Body, which is a stream. If the Body produces an item, next is going to be ready and we'll detect it despite the timeout. Destroying the wrapper around the &mut Body has no consequence.

id: HttpRequestId,
/// Error that happened.
error: hyper::Error,
}

This comment has been minimized.

Copy link
@tomusdrw

tomusdrw Aug 21, 2019

Contributor
Suggested change
}
},
Poll::Ready(Some(Ok(chunk))) => {
let _ = tx.start_send(Ok(chunk));
me.requests.push((id, HttpWorkerRequest::ReadBody { body, tx }));
cx.waker().wake_by_ref(); // notify in order to poll again

This comment has been minimized.

Copy link
@tomusdrw

tomusdrw Aug 21, 2019

Contributor

I'd either add continue here for consistency, or remove it from other branches.

@mxinden

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

I'd like to fix that by having some enum { HttpConnector, HttpsConnector } where we fall back to the HttpConnector if the HTTPS one fails to initialize. We could also just return the error and print a big error saying "failed to initialize offchain workers". But considering that this is the only reason why offchain workers can fail to initialize, it's a bit annoying.

Falling back to http when https fails should be exploitable by a downgrade attack, right? Hope I am not missing something.

@tomusdrw

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

@mxinden well, so the thing is that no https call should work in HttpConnector, so it's just a fallback if for some reason:

  1. One is using http:// URL
  2. HttpsConnector/TlsConnector failed to initialise (as Pierre mentioned in the description).

I think it's a pretty niche case, but might be worth handling anyway.

@burdges

This comment has been minimized.

Copy link

commented Aug 22, 2019

Yeah, I'd avoid the fall back unless it's specifically requested by the caller, probably for testing situations.

@tomusdrw

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

We are not falling back from https to http on a requested URL, we are falling back on supporting http:// requests if we can't initialize HttpsConnector (that supports http:// and https:// requests).
I don't see any security issues here.

@gavofyork

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

so, the only instance where an http: request will happen is if the caller explicitly requests an http URL (and this will work even if the TLS stuff couldn't be initialised)?

@tomusdrw

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

so, the only instance where an http: request will happen is if the caller explicitly requests an http URL (and this will work even if the TLS stuff couldn't be initialised)?

Indeed. That's exactly the case we want to handle.

@tomusdrw

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

Superseded by #3461 (I couldn't push to Pierre's fork).

@tomusdrw tomusdrw closed this Aug 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.