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.

Sign up for GitHub

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

Jump to bottom

expose http headers & request body via peerinfo context #352

Closed
wants to merge 2 commits into from
Closed

expose http headers & request body via peerinfo context #352

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
269abfa
expose http headers via context
hamdiallam Jul 17, 2024
1737e59
nit
hamdiallam Jul 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions rpc/http.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,19 @@ 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

// 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))
Comment on lines +329 to +338
Copy link
Collaborator

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.

Copy link
Contributor Author

@hamdiallam hamdiallam Aug 7, 2024 •
edited
Loading

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


ctx := r.Context()
ctx = context.WithValue(ctx, peerInfoContextKey{}, connInfo)

Expand Down
8 changes: 8 additions & 0 deletions rpc/http_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package rpc

import (
"context"
"encoding/json"
"fmt"
"net/http"
"net/http/httptest"
Expand Down Expand Up @@ -180,6 +181,7 @@ func TestHTTPPeerInfo(t *testing.T) {
}
c.SetHeader("user-agent", "ua-testing")
c.SetHeader("origin", "origin.example.com")
c.SetHeader("foobar", "baz")

// Request peer information.
var info PeerInfo
Expand All @@ -202,6 +204,12 @@ func TestHTTPPeerInfo(t *testing.T) {
if info.HTTP.Origin != "origin.example.com" {
t.Errorf("wrong HTTP.Origin %q", info.HTTP.UserAgent)
}
if info.HTTP.Header.Get("foobar") != "baz" {
t.Errorf("wrong custom Http.Header")
}
if rpc, _ := parseMessage(json.RawMessage(info.HTTP.Body)); len(rpc) != 1 && rpc[0].Method != "test_peerInfo" {
t.Errorf("unexpected json rpc message body")
}
}

func TestNewContextWithHeaders(t *testing.T) {
Expand Down
5 changes: 5 additions & 0 deletions rpc/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package rpc
import (
"context"
"io"
"net/http"
"sync"
"sync/atomic"

Expand Down Expand Up @@ -218,6 +219,10 @@ type PeerInfo struct {
UserAgent string
Origin string
Host string
// Headers associated with the request
Header http.Header
// The body attached to the request
Body []byte
}
}

Expand Down