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

node: support expressive origin rules in ws.origins #21481

Merged
merged 2 commits into from
Nov 19, 2020
Merged

node: support expressive origin rules in ws.origins #21481

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
081f483
Only compare hostnames in ws.origins
wbt Aug 24, 2020
8430f92
node: more tests + table-driven, ws origin changes
holiman Sep 11, 2020
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
125 changes: 108 additions & 17 deletions node/rpcstack_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package node
import (
"bytes"
"net/http"
"strings"
"testing"

"github.com/ethereum/go-ethereum/internal/testlog"
Expand Down Expand Up @@ -52,25 +53,104 @@ func TestVhosts(t *testing.T) {
assert.Equal(t, resp2.StatusCode, http.StatusForbidden)
}

// TestWebsocketOrigins makes sure the websocket origins are properly handled on the websocket server.
func TestWebsocketOrigins(t *testing.T) {
srv := createAndStartServer(t, httpConfig{}, true, wsConfig{Origins: []string{"test"}})
defer srv.stop()
type originTest struct {
spec string
expOk []string
expFail []string
}
Copy link
Contributor

@fjl fjl Aug 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to make this a table-driven test instead, i.e.

type originTest struct{
    origin string
    spec string
    shouldAllow bool
}

tests := []originTest{
    {origin: "http://localhost:8080", spec: "localhost", shouldAllow: true},
    ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That wouldn't be more concise than the status quo, and I think it would be harder to read/follow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would also be slower if spinning up a new server for every test, as the spec is a command-line parameter passed in server startup.


dialer := websocket.DefaultDialer
_, _, err := dialer.Dial("ws://"+srv.listenAddr(), http.Header{
"Content-type": []string{"application/json"},
"Sec-WebSocket-Version": []string{"13"},
"Origin": []string{"test"},
})
assert.NoError(t, err)
// splitAndTrim splits input separated by a comma
// and trims excessive white space from the substrings.
// Copied over from flags.go
func splitAndTrim(input string) (ret []string) {
l := strings.Split(input, ",")
for _, r := range l {
r = strings.TrimSpace(r)
if len(r) > 0 {
ret = append(ret, r)
}
}
return ret
}

_, _, err = dialer.Dial("ws://"+srv.listenAddr(), http.Header{
"Content-type": []string{"application/json"},
"Sec-WebSocket-Version": []string{"13"},
"Origin": []string{"bad"},
})
assert.Error(t, err)
// TestWebsocketOrigins makes sure the websocket origins are properly handled on the websocket server.
func TestWebsocketOrigins(t *testing.T) {
tests := []originTest{
{
spec: "*", // allow all
expOk: []string{"", "http://test", "https://test", "http://test:8540", "https://test:8540",
"http://test.com", "https://foo.test", "http://testa", "http://atestb:8540", "https://atestb:8540"},
},
{
spec: "test",
expOk: []string{"http://test", "https://test", "http://test:8540", "https://test:8540"},
expFail: []string{"http://test.com", "https://foo.test", "http://testa", "http://atestb:8540", "https://atestb:8540"},
},
// scheme tests
{
spec: "https://test",
expOk: []string{"https://test", "https://test:9999"},
expFail: []string{
"test", // no scheme, required by spec
"http://test", // wrong scheme
"http://test.foo", "https://a.test.x", // subdomain variatoins
"http://testx:8540", "https://xtest:8540"},
},
// ip tests
{
spec: "https://12.34.56.78",
expOk: []string{"https://12.34.56.78", "https://12.34.56.78:8540"},
expFail: []string{
"http://12.34.56.78", // wrong scheme
"http://12.34.56.78:443", // wrong scheme
"http://1.12.34.56.78", // wrong 'domain name'
"http://12.34.56.78.a", // wrong 'domain name'
"https://87.65.43.21", "http://87.65.43.21:8540", "https://87.65.43.21:8540"},
},
// port tests
{
spec: "test:8540",
expOk: []string{"http://test:8540", "https://test:8540"},
expFail: []string{
"http://test", "https://test", // spec says port required
"http://test:8541", "https://test:8541", // wrong port
"http://bad", "https://bad", "http://bad:8540", "https://bad:8540"},
},
// scheme and port
{
spec: "https://test:8540",
expOk: []string{"https://test:8540"},
expFail: []string{
"https://test", // missing port
"http://test", // missing port, + wrong scheme
"http://test:8540", // wrong scheme
"http://test:8541", "https://test:8541", // wrong port
"http://bad", "https://bad", "http://bad:8540", "https://bad:8540"},
},
// several allowed origins
{
spec: "localhost,http://127.0.0.1",
expOk: []string{"localhost", "http://localhost", "https://localhost:8443",
"http://127.0.0.1", "http://127.0.0.1:8080"},
expFail: []string{
"https://127.0.0.1", // wrong scheme
"http://bad", "https://bad", "http://bad:8540", "https://bad:8540"},
},
}
for _, tc := range tests {
srv := createAndStartServer(t, httpConfig{}, true, wsConfig{Origins: splitAndTrim(tc.spec)})
for _, origin := range tc.expOk {
if err := attemptWebsocketConnectionFromOrigin(t, srv, origin); err != nil {
t.Errorf("spec '%v', origin '%v': expected ok, got %v", tc.spec, origin, err)
}
}
for _, origin := range tc.expFail {
if err := attemptWebsocketConnectionFromOrigin(t, srv, origin); err == nil {
t.Errorf("spec '%v', origin '%v': expected not to allow, got ok", tc.spec, origin)
}
}
srv.stop()
}
}

// TestIsWebsocket tests if an incoming websocket upgrade request is handled properly.
Expand Down Expand Up @@ -103,6 +183,17 @@ func createAndStartServer(t *testing.T, conf httpConfig, ws bool, wsConf wsConfi
return srv
}

func attemptWebsocketConnectionFromOrigin(t *testing.T, srv *httpServer, browserOrigin string) error {
t.Helper()
dialer := websocket.DefaultDialer
_, _, err := dialer.Dial("ws://"+srv.listenAddr(), http.Header{
"Content-type": []string{"application/json"},
"Sec-WebSocket-Version": []string{"13"},
"Origin": []string{browserOrigin},
})
return err
}

func testRequest(t *testing.T, key, value, host string, srv *httpServer) *http.Response {
t.Helper()

Expand Down
65 changes: 62 additions & 3 deletions rpc/websocket.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,14 +75,14 @@ func wsHandshakeValidator(allowedOrigins []string) func(*http.Request) bool {
allowAllOrigins = true
}
if origin != "" {
origins.Add(strings.ToLower(origin))
origins.Add(origin)
}
}
// allow localhost if no allowedOrigins are specified.
if len(origins.ToSlice()) == 0 {
origins.Add("http://localhost")
if hostname, err := os.Hostname(); err == nil {
origins.Add("http://" + strings.ToLower(hostname))
origins.Add("http://" + hostname)
}
}
log.Debug(fmt.Sprintf("Allowed origin(s) for WS RPC interface %v", origins.ToSlice()))
Expand All @@ -97,7 +97,7 @@ func wsHandshakeValidator(allowedOrigins []string) func(*http.Request) bool {
}
// Verify origin against whitelist.
origin := strings.ToLower(req.Header.Get("Origin"))
if allowAllOrigins || origins.Contains(origin) {
if allowAllOrigins || originIsAllowed(origins, origin) {
return true
}
log.Warn("Rejected WebSocket connection", "origin", origin)
Expand All @@ -120,6 +120,65 @@ func (e wsHandshakeError) Error() string {
return s
}

func originIsAllowed(allowedOrigins mapset.Set, browserOrigin string) bool {
it := allowedOrigins.Iterator()
for origin := range it.C {
if ruleAllowsOrigin(origin.(string), browserOrigin) {
return true
}
}
return false
}

func ruleAllowsOrigin(allowedOrigin string, browserOrigin string) bool {
var (
allowedScheme, allowedHostname, allowedPort string
browserScheme, browserHostname, browserPort string
err error
)
allowedScheme, allowedHostname, allowedPort, err = parseOriginURL(allowedOrigin)
if err != nil {
log.Warn("Error parsing allowed origin specification", "spec", allowedOrigin, "error", err)
return false
}
browserScheme, browserHostname, browserPort, err = parseOriginURL(browserOrigin)
if err != nil {
log.Warn("Error parsing browser 'Origin' field", "Origin", browserOrigin, "error", err)
return false
}
if allowedScheme != "" && allowedScheme != browserScheme {
return false
}
if allowedHostname != "" && allowedHostname != browserHostname {
return false
}
if allowedPort != "" && allowedPort != browserPort {
return false
}
return true
}

func parseOriginURL(origin string) (string, string, string, error) {
parsedURL, err := url.Parse(strings.ToLower(origin))
if err != nil {
return "", "", "", err
}
var scheme, hostname, port string
if strings.Contains(origin, "://") {
scheme = parsedURL.Scheme
hostname = parsedURL.Hostname()
port = parsedURL.Port()
} else {
scheme = ""
hostname = parsedURL.Scheme
port = parsedURL.Opaque
if hostname == "" {
hostname = origin
}
}
return scheme, hostname, port, nil
}

// DialWebsocketWithDialer creates a new RPC client that communicates with a JSON-RPC server
// that is listening on the given endpoint using the provided dialer.
func DialWebsocketWithDialer(ctx context.Context, endpoint, origin string, dialer websocket.Dialer) (*Client, error) {
Expand Down