-
Notifications
You must be signed in to change notification settings - Fork 745
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
expose http headers & request body via peerinfo context #352
Conversation
rpc/http.go
Outdated
@@ -324,6 +324,7 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { | |||
connInfo.HTTP.Host = r.Host | |||
connInfo.HTTP.Origin = r.Header.Get("Origin") | |||
connInfo.HTTP.UserAgent = r.Header.Get("User-Agent") | |||
connInfo.HTTP.Header = r.Header.Clone() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could consider not cloning since this is on the hot path of requests, having it contractual that it should not be mutated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also not sure. Since the PeerInfo
doesn't outlive the http request, you're probably right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm strongly in favor of not cloning; since it's in the hot-path, for every request, and includes all common headers, it is copying a lot of data over time. Including on every RPC interaction on a provider, and every engine API call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. Removed the Clone()
In order to validate signature, the request body must also be available to the endpoint so I updated to also include the Body
in the PeerInfo.
Alternatively we could enshrine authentication apart of validateRequest
where the rpc server can be configured to specify the name of the authentication header to look out for. wdyt? @protolambda @sebastianst
rpc/http.go
Outdated
@@ -324,6 +324,7 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { | |||
connInfo.HTTP.Host = r.Host | |||
connInfo.HTTP.Origin = r.Header.Get("Origin") | |||
connInfo.HTTP.UserAgent = r.Header.Get("User-Agent") | |||
connInfo.HTTP.Header = r.Header.Clone() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also not sure. Since the PeerInfo
doesn't outlive the http request, you're probably right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not clone the header data (see comment)
rpc/http.go
Outdated
@@ -324,6 +324,7 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { | |||
connInfo.HTTP.Host = r.Host | |||
connInfo.HTTP.Origin = r.Header.Get("Origin") | |||
connInfo.HTTP.UserAgent = r.Header.Get("User-Agent") | |||
connInfo.HTTP.Header = r.Header.Clone() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm strongly in favor of not cloning; since it's in the hot-path, for every request, and includes all common headers, it is copying a lot of data over time. Including on every RPC interaction on a provider, and every engine API call.
85fcc58
to
269abfa
Compare
Agreed! See the reply in the thread with @sebastianst . Curious what you think on also exposing the request body or if we should enshrine request auth in the server itself |
// Read the body and make it available in the request scope. The request's | ||
// Body ReaderCloser is replaced to read over this same slice. | ||
body, err := io.ReadAll(io.LimitReader(r.Body, int64(s.httpBodyLimit))) | ||
if err != nil { | ||
// `httpBodyLimit` is already checked when validated | ||
http.Error(w, err.Error(), http.StatusInternalServerError) | ||
return | ||
} | ||
connInfo.HTTP.Body = body | ||
r.Body = io.NopCloser(bytes.NewReader(body)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exposing the request-body in the RPC is equally expensive in terms of copying the header, possibly worse. Additionally, not all transports have headers or request-bodies. E.g. a websocket starts as HTTP request and only has initial headers and is then upgraded to a websocket. And an IPC based RPC request, or in-process RPC as in many e2e tests, do not have a "request" at all.
To implement authentication, using an approach similar to the JWT authentication that the engine-API uses, might make more sense. There authentication is persistent across the connection, not per request, and trusted in the case of IPC transport. This allows HTTP/Websockets to authenticate, which are also the only publicly-exposed endpoints, and thus make sense to authenticate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looping back here I agree that implementing this at the rpc server level doesn't make sense.
However the performance hit isn't a big deal for the proxy since it's low throughput. Looking through how geth implements auth out of the server impl via the http handlers, we can do something similar as well.
Going to close this PR as no op-geth changes will be needed anymore. It would nice of go-ethereum refactored some of the handler code to work natively with the server
Closing as we can implement auth outside of the server via http handlers instead |
go-ethereum library has very useful rpc libraries for creating json-rpc servers, used extensively in the optimism monorepo.
For the sendRawTransactionConditional endpoint, we want to implement authentication external to the the execution engine.
This requires accessing the headers & body associated with the request which can easily be done by exposing them in the
PeerInfo
set in the context when serving a requests